Skip to content

Release OutputSweeper::pending_sweep flag on future drop#4598

Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
tnull:2026-05-output-sweeper-pending-sweep-guard
May 6, 2026
Merged

Release OutputSweeper::pending_sweep flag on future drop#4598
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
tnull:2026-05-output-sweeper-pending-sweep-guard

Conversation

@tnull
Copy link
Copy Markdown
Contributor

@tnull tnull commented May 5, 2026

regenerate_and_broadcast_spend_if_necessary used pending_sweep: AtomicBool as a single-runner gate but only cleared the flag with an unconditional store(false) after the inner future resolved. If the caller's future was dropped while the inner await was Pending — which tokio::time::timeout, futures::select!, manual JoinHandle::abort, etc. all do — the reset never ran, leaving the flag stuck true and every subsequent call to the function short-circuiting with Ok(()).

Because OutputSweeper is what claims SpendableOutputDescriptors back to the user's wallet after channel closure (including HTLC outputs with time-bounded recovery deadlines), a stuck flag turns into fund-loss exposure: time-sensitive HTLC sweeps simply stop happening, while every other code path keeps queueing new outputs to sweep, until the process is restarted.

Replace the trailing store(false) with an RAII PendingSweepGuard whose Drop impl always releases the flag — covering normal return, error, and cancellation alike.

Co-Authored-By: HAL 9000

`regenerate_and_broadcast_spend_if_necessary` used `pending_sweep:
AtomicBool` as a single-runner gate but only cleared the flag with an
unconditional `store(false)` *after* the inner future resolved. If the
caller's future was dropped while the inner await was `Pending` —
which `tokio::time::timeout`, `futures::select!`, manual
`JoinHandle::abort`, etc. all do — the reset never ran, leaving the
flag stuck `true` and every subsequent call to the function
short-circuiting with `Ok(())`.

Because `OutputSweeper` is what claims `SpendableOutputDescriptor`s
back to the user's wallet after channel closure (including HTLC
outputs with time-bounded recovery deadlines), a stuck flag turns
into fund-loss exposure: time-sensitive HTLC sweeps simply stop
happening, while every other code path keeps queueing new outputs to
sweep, until the process is restarted.

Replace the trailing `store(false)` with an RAII `PendingSweepGuard`
whose `Drop` impl always releases the flag — covering normal return,
error, and cancellation alike.

Co-Authored-By: HAL 9000
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 5, 2026

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

The implementation is correct and clean. Key verification points:

  1. The _guard binding (not _) ensures the guard lives until end of scope, covering the .await.
  2. The guard is created only after the compare_exchange succeeds (early return on failure), so there's no risk of a spurious release.
  3. No await points between compare_exchange and guard construction, so no cancellation gap.
  4. Memory orderings are correct: Acquire on the CAS success pairs with Release on the guard's drop.
  5. The test correctly simulates cancellation by polling once against a PendingKVStore that blocks on write, then dropping the future.

No issues found.

Copy link
Copy Markdown
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly not sure its worth committing the test for this, but whatever.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 77.64706% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.25%. Comparing base (1a26867) to head (6394d18).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/util/sweep.rs 77.64% 18 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4598      +/-   ##
==========================================
- Coverage   86.84%   86.25%   -0.59%     
==========================================
  Files         161      159       -2     
  Lines      109260   109351      +91     
  Branches   109260   109351      +91     
==========================================
- Hits        94882    94319     -563     
- Misses      11797    12416     +619     
- Partials     2581     2616      +35     
Flag Coverage Δ
fuzzing-fake-hashes ?
fuzzing-real-hashes ?
tests 86.25% <77.64%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheBlueMatt TheBlueMatt merged commit b1ea3f9 into lightningdevkit:main May 6, 2026
23 of 25 checks passed
@tnull
Copy link
Copy Markdown
Contributor Author

tnull commented May 6, 2026

Honestly not sure its worth committing the test for this, but whatever.

Yeah, true, I was also on the fence whether to include it or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants