Fix EventQueue losing state on persistence failures - #1050
Open
elnafateh wants to merge 1 commit into
Open
Conversation
add_event and event_handled mutated the in-memory queue before the mutation was durably persisted. A failed or delayed persist could then cause duplicate events on replay, silently skipped acknowledgements, or resurrected/lost events after a restart. Serialize both operations end-to-end behind an operation lock, so the in-memory queue is only advanced after its snapshot has been persisted, and persistence writes are submitted in mutation order. Add regression tests for failed-enqueue replay, failed-ack retries, concurrent enqueue/ack under load, and restart behavior, using a fault-injecting KVStore. Fixes lightningdevkit#1027.
|
I've assigned @tnull as a reviewer! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1027.
Problem
EventQueue::add_event and EventQueue::event_handled mutate the in-memory queue before the mutation is durably persisted. If the persist call fails or races with another operation, in-memory and persisted state can diverge, causing:
Fix
Serialize add_event/event_handled end-to-end behind a tokio::sync::Mutex (operation_lock) spanning snapshot-compute → persist → in-memory-commit. In-memory state is now only ever advanced after its snapshot is durably persisted, and persistence submissions happen strictly in mutation order.
This closes all four failure modes structurally:
(1)/(2): a failed persist means the in-memory mutation never happens, so retries repeat the same logical operation instead of duplicating/skipping.
(3)/(4): full serialization means there's no concurrent-write-submission window left to race — persisted state can never fall behind or get reordered relative to in-memory state.
I verified that SqliteStore, VssStore, and PostgresStore all allocate their write version synchronously at the top of write(), before any async work — which is what makes serializing at this layer sufficient to also close the built-in stores' residual version-ordering window. Custom KVStore implementations that defer version allocation to a later async step wouldn't get that same guarantee from this fix alone — worth flagging as a documented expectation of KVStore implementors if that's not already stated.
Testing
Added a fault-injecting KVStore wrapper (FaultingStore) that can be told to fail the next N writes, with regression tests for:
Failed-enqueue replay (no duplicate)
Failed-ack retry (no skip)
Concurrent enqueue/ack under load (no lost updates, persisted state matches in-memory)
Restart resurrection (persisted bytes never reflect an uncommitted mutation)