Fix Critical Error handling in HTLC interception #294
Conversation
elnosh
left a comment
elnosh
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the change.
Could you update the TODO comment at the top saying this needed to be done.
|
Change looks good - would be nice to add test coverage for this shutdown case |
|
Thanks for the feedback! 🙌 You're absolutely right — test coverage for the shutdown behavior is important. I will add a test to ensure that when a CriticalError occurs, the expected behavior happens |
elnosh
left a comment
elnosh
left a comment
There was a problem hiding this comment.
heh sorry, with this Could you update the TODO comment at the top saying this needed to be done. I meant update the TODO comment in handle_intercepted_htlc
| async fn test_intercepted_htlc_critical_error() { | ||
| // Interceptor that will return a critical error. | ||
| let mut mock_interceptor = MockTestInterceptor::new(); | ||
| mock_interceptor | ||
| .expect_intercept_htlc() | ||
| .returning(|_| Err(CriticalError::InterceptorError("critical failure".into()))); | ||
| mock_interceptor | ||
| .expect_notify_resolution() | ||
| .returning(|_| Ok(())); | ||
|
|
||
| let (interceptor_trigger, interceptor_listener) = triggered::trigger(); | ||
| let mock_request = create_intercept_request(interceptor_listener); | ||
|
|
||
| let mock_interceptor: Arc<dyn Interceptor> = Arc::new(mock_interceptor); | ||
| let interceptors = vec![mock_interceptor]; | ||
| let (_, shutdown_listener) = triggered::trigger(); | ||
|
|
||
| let result = handle_intercepted_htlc( | ||
| mock_request, | ||
| &interceptors, | ||
| interceptor_trigger, | ||
| shutdown_listener, | ||
| ) | ||
| .await; | ||
|
|
||
| assert!(result.is_err()); | ||
| let err_string = format!("{:?}", result.unwrap_err()); | ||
| assert!(err_string.contains("critical failure")); | ||
| } |
There was a problem hiding this comment.
hmm this checks that the returned error is the one returned from interceptor but not sure if @carlaKC meant to add a case where other interceptors received a shutdown signal if a critical error was returned from another one. Although we are just mocking interceptors so I'm babbling here.
There was a problem hiding this comment.
to add a case where other interceptors received a shutdown signal if a critical error was returned from another one
This is what I meant.
Sadly I don't think that we'll be able to mock the interceptors to check that each of them trigger shutdown. Was thinking of a very simple interceptor that just blocks until we get shutdown and then we add a timeout on the test to make sure that nothgin is blocking.
There was a problem hiding this comment.
so there is no way to get an interceptor to blocked until we got a shutdown
There was a problem hiding this comment.
I'm not sure what you mean? You can implement an interceptor that listens on the shutdown listener for the purpose of the test and that should block?
We wait for all the interceptors to exit when we get the shutdown signal, so they should block if the trigger isn't hit by a critical error.
There was a problem hiding this comment.
ok,
i will probably look into how mock interceptor are done i.e
create_intercept_request
then i will implement an interceptor that listens on the shutdown listener and mock it
|
@mubarak23 any updates here? Will be closing due to inactivity if review is not addressed soon. |
i was not able to fully implement interceptor, but i will share what i have and get pointers in the right direction |
|
Closing this out due to inactivity, feel free to discuss approaches on the issue. |
3 participants
closes #272
TODO: This needed to be done.
Currently, when a CriticalError occurs during HTLC interception in handle_intercepted_htlc, the function immediately returns without waiting for other running interceptors to complete. This can leave interceptors in an inconsistent state and prevent proper cleanup.
Changes