fix: ecdsa background tasks should be infallible#2133
Conversation
Code ReviewClean, correct change. Making background triple/presignature generation tasks infallible is the right approach -- these are infinite loops spawned into tracking::spawn, and having them return anyhow::Result was misleading since the ? on new_channel_for_task would silently kill the entire background generation loop on a transient channel error. Now errors are logged and the loop continues, which is the correct resilience behavior for background tasks. Minor non-blocking observations:
Approved. |
|
@claude review
you are right about this, might be something worth thinking about in the future |
|
No critical issues found beyond what the previous review already identified. The change correctly converts infinite background loops from fallible to infallible, preventing transient channel errors from permanently killing triple/presignature generation. Error handling via ✅ Approved |
| let channel = match client.new_channel_for_task(task_id, participants.clone()) { | ||
| Ok(channel) => channel, | ||
| Err(err) => { | ||
| tracing::warn!( |
There was a problem hiding this comment.
Given that this was an error we exited on earlier, how severe is it? Should it be an error log instead?
There was a problem hiding this comment.
I think as we are continuing execution anyway, a warning is more appropriate.
On a more general note, if we begin really seeing this in the logs a full revamp might be necessary here, as technically it should be a pretty infrequent behavior
There was a problem hiding this comment.
Generally speaking we should not use Error logs unless something unexpected happens imo. Network connection drops are expected during normal operations and should therefore be warnings as I see it.
Closes #2122
I took the chance to fix triple generation as well.