Skip to content

Skip mixed-mode assertion for replayed monitor updates#4506

Merged
TheBlueMatt merged 1 commit intolightningdevkit:mainfrom
joostjager:fix-revert-mixed-mode-check
Mar 23, 2026
Merged

Skip mixed-mode assertion for replayed monitor updates#4506
TheBlueMatt merged 1 commit intolightningdevkit:mainfrom
joostjager:fix-revert-mixed-mode-check

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Mar 23, 2026

Already identified in #4436 (comment), but not implemented in the final PR.

Fixes fuzz strings:

fefdffffffffff004111700000001d0100000000000580b832b240b8b8                                                                                                                           
c985f28584a0a221ffabac1aac1a041ab1ffa246a0abac1a8484                                                                                                                                   
b2ffb70001ff35410510b7b7ff40                                                                                                                                                           
2e227412b969ffb0ffa2a2ffadffb9acb9b9b9b9ab05b3b3b912843470                                                                                                                             
fe01000005ffff5941117031b81c00b8b8b8b8b832b2b8b832b2b8b8                                                                                                                               
feffff21ffff05ff5941117030b8b8b8b8b831b2b8b8

@joostjager
Skip mixed-mode assertion for replayed monitor updates
88f99de 
When a node restarts and switches from async to sync persistence, the
in-flight monitor updates from the previous session are replayed as
background events. These replayed updates are resubmitted to the Watch
which now returns Completed, while earlier in-flight updates are still
queued as background events. This triggered a false panic in the
assertion that guards against out-of-order monitor update completion.

Track whether an update is a replay (already present in
in_flight_monitor_updates) and skip the assertion for replays, since
the remaining in-flight updates will be submitted by subsequent
background events.

AI tools were used in preparing this commit.
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Mar 23, 2026 •
edited
Loading

👋 Thanks for assigning @TheBlueMatt 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.

@joostjager joostjager mentioned this pull request Mar 23, 2026
Open
@joostjager joostjager marked this pull request as ready for review March 23, 2026 11:47
@joostjager joostjager requested a review from TheBlueMatt March 23, 2026 11:47
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 23, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (88f99de).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4506      +/-   ##
==========================================
- Coverage   86.18%   86.17%   -0.02%     
==========================================
  Files         160      160              
  Lines      107536   107537       +1     
  Branches   107536   107537       +1     
==========================================
- Hits        92680    92668      -12     
- Misses      12231    12243      +12     
- Partials     2625     2626       +1
Flag Coverage Δ
tests 86.17% <100.00%> (-0.02%) ⬇️

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.

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

ldk-claude-review-bot commented Mar 23, 2026 •
edited
Loading

Good — both paths call handle_new_monitor_update_locked_actions_handled_by_caller where the fix lives.

I've thoroughly reviewed the entire diff. The change is logically sound:

  1. is_replay correctly identifies replayed updates (found already in in_flight_updates)
  2. Skipping the assertion for replays is safe because during startup replay, earlier in-flight updates may not have been submitted to the Watch yet
  3. Non-replay updates still get the full assertion check
  4. The is_replay flag is correctly scoped within the function
  5. No other code paths are affected

No issues found.

No issues found.

The PR correctly fixes a panic that occurs during startup replay when switching from async to sync persistence across a restart. The is_replay flag accurately identifies replayed updates (already present in in_flight_updates), and skipping the mixed-mode assertion for these is the right fix since earlier in-flight updates may not have been submitted to the Watch yet during replay. Non-replay updates retain the full assertion coverage.

TheBlueMatt
TheBlueMatt approved these changes Mar 23, 2026
View reviewed changes
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.

oops thanks

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Mar 23, 2026

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@TheBlueMatt TheBlueMatt merged commit b3a99f6 into lightningdevkit:main Mar 23, 2026
21 of 22 checks passed
@joostjager joostjager self-assigned this Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

Assignees

@joostjager joostjager

Labels

None yet

Projects

Weekly Goals
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@joostjager @ldk-reviews-bot @ldk-claude-review-bot @TheBlueMatt