server: fix corner case shutdown hang#2110
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential shutdown hang in the daemon by improving the lifecycle management of the auxiliary channel closer component. By integrating the component into the server's start/stop sequence and ensuring that proof import operations are context-aware, the system can now cleanly shut down even if a proof courier becomes unresponsive during a cooperative channel close. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces lifecycle management for the AuxChanCloser component by implementing Start and Stop methods and integrating them into the server's initialization and shutdown sequences. It also refactors importOutputProofs to accept a context.Context, replacing context.Background() to allow for proper cancellation during asset-aware channel closures. I have no feedback to provide.
|
@GeorgeTsagk: review reminder |
GeorgeTsagk
left a comment
GeorgeTsagk
left a comment
There was a problem hiding this comment.
Lgtm
needs a rebase to resolve a conflict
also would be nice to add a test, although setup might be tricky (non-blocking, leaving it up to you)
github-project-automation
Bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
Thread a context.Context through importOutputProofs so callers can control cancellation. The importCommitTx call site passes its existing context.Background(), preserving current behavior.
Embed a ContextGuard in AuxChanCloser and add Start/Stop methods so the daemon can cleanly cancel in-flight importOutputProofs calls on shutdown. FinalizeClose now creates a quit-cancellable context via WithCtxQuitNoTimeout, which propagates through the proof courier's BackoffHandler.wait select on ctx.Done(). FinalizeClose brackets the import call with Wg.Add/Done so that Stop() waits for in-flight calls to fully unwind before returning, preventing use-after-close on downstream dependencies. AuxChanCloser.Stop is called early in the server shutdown sequence, before other Aux* components, so cancellation fires before dependencies like ChainBridge or ProofArchive are torn down.
Yeah, I looked into this but it's indeed difficult to set up an actually-good one. I think the best proof of functionality here, verifiable by direct inspection of the diff, is that this simply brings AuxChanCloser in line with the way other aux components are handled (and they don't have any similar tests either, presumably for the same reason). |
jtobin
force-pushed
the
ioproofs-clean-shutdown
branch
from
0e0dd08 to
2b8bc30
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-project-automation
Bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
3 participants
Resolves #2109. This is a borderline-overfitting issue identified by Opus while diagnosing #2108, but the fix is simple, and does marginally improve the daemon's stability, so IMO it's worth making.
Previously an unresponsive proof courier could cause importOutputProofs to block indefinitely while a co-op close was in flight, preventing tapd from shutting down cleanly. The fix simply wires up AuxChanCloser with a ContextGuard lifecycle in the same manner as other Aux components (AuxFundingController, AuxSweeper, etc.), also threading a cancellable context through importOutputProofs.
(Shutting down the daemon in this situation is safe in terms of ultimately handling the coop close; the retry logic is handled by lnd.)