Fix EventQueue losing state on persistence failures - #1050
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! |
Jolah1
left a comment
Jolah1
left a comment
There was a problem hiding this comment.
Nice work!
The production approach looks correct: the operation mutex spans snapshot creation, persistence completion, and the in-memory commit. This fixes failed enqueue/acknowledgement behavior and serializes successful queue mutation.
The two ordering-related tests do not regress the ordering failures described in #1027. The concurrency test uses immediately completing writes and only checks that 200 adds plus 200 acknowledgements leave an empty queue. The restart test checks restoration after a failed acknowledgement, and its final assertion would also pass against the old implementation.
you should add a deterministic store that can hold a write and record when later writes are submitted. The test should verify that a second mutation cannot submit its write while the first is pending, then reload the final bytes and confirm they match the newest committed in-memory state.
| async fn event_queue_concurrent_enqueue_and_ack_ordered() { | ||
| let store = Arc::new(FaultingStore::new()); | ||
| let logger = Arc::new(TestLogger::new()); | ||
| let event_queue = | ||
| Arc::new(EventQueue::new(Arc::clone(&store) as Arc<DynStore>, Arc::clone(&logger))); |
There was a problem hiding this comment.
The implementation structurally serializes these operations, but I don’t think this test specifically regresses the ordering race from #1027: successful writes complete immediately, and the final empty queue follows from having equal numbers of adds and acknowledgements. Would it be worth using a controllable pending write and asserting that the second mutation cannot submit its write until the first completes?
Alternatively, the test name/comments could be narrowed so they don’t claim coverage of failure modes 3 and 4.
| /// bytes back must restore exactly the last successfully committed state — no | ||
| /// resurrected already-acked events and no missing newer events. | ||
| #[tokio::test] | ||
| async fn event_queue_restart_resurrection() { |
There was a problem hiding this comment.
This test passes against the old mutation-before-persistence behavior: the failed acknowledgement removes a from memory, but storage remains [a, b], so deserializing storage still returns a.
Could this instead reproduce restart resurrection by delaying an older write, allowing a newer acknowledgement snapshot to be attempted, and then completing the writes in the problematic order? That would verify that a stale snapshot cannot become the state restored after restart.
3 participants
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)