fix(persistence): MqDrop tombstone — DEL/UNLINK/FLUSHALL/FLUSHDB no longer resurrect durable MQ streams on kill-9 (task #46)#301
Merged
Conversation
…LUSHDB no longer resurrect on kill-9 (task #46) Root cause: MQ durable streams live as ordinary keys in the shard keyspace, but replay_mq_wal had no way to represent "this queue was deleted after these pushes". A generic DEL/UNLINK/FLUSHDB/FLUSHALL removed the stream from the live db and the DurableQueueRegistry, but replay unconditionally reapplies every MqCreate/MqPush/... record on boot regardless of that delete — a kill-9 after the delete resurrected the full pre-delete content on the next restart. Same bug class as the already-fixed KV/vector cold-plane resurrection (PR #257), now closed for MQ. Reproduced via the crash-matrix RED cell cross_plane_seeded_red_mq_generic_del_resurrection (kernel M3 brief §1.4), now un-gated (no more harness::red_guard) and green 3/3 consecutive runs, plus a full 44/44 default-GREEN crash-matrix pass. Fix: a new MqDrop WAL v3 record (discriminant 0x75) is emitted whenever a durable MQ stream is removed via generic DEL/UNLINK/FLUSHDB/FLUSHALL. Layout matches the other versioned MQ records: [version:u8][db_index:u32] [key_len:u32][key:N], fails closed on any malformed/future-version payload. New hooks mq_exec::auto_drop_mq_streams / auto_drop_mq_streams_on_flush are wired into every connection-layer write path that already runs the equivalent vector/text index-parity hooks (handler_monoio/mod.rs, handler_sharded/mod.rs, the sharded MULTI/EXEC helper in server/conn/shared.rs) plus the replica-side apply_index_parity_hooks in replication/apply.rs — a replica must tombstone its OWN WAL too, or it resurrects the stream on its own restart even though its live copy stayed correctly deleted via normal command replication. Boot-time replay applies apply_mq_drop (shared_databases.rs) strictly in WAL order alongside every other MQ record, so a Drop only kills records that PRECEDE it for that key — a later MqCreate/MqPush for the same key survives intact (create -> drop -> create round-trips a kill-9 with the second incarnation whole; proven by both a unit test and the new crash-matrix cell cross_plane_mq_create_drop_create_survives). segment_plane_scan's plane-history block set gained MqDrop alongside the other MQ discriminants so autovacuum/recycle never deletes a sealed segment still holding an unfloored tombstone. The MQ WAL fuzz target now also fuzzes decode_mq_drop. Replication decision: MQ effect records replicate live only at num_shards == 1 (the pre-existing gate shared with MqCreate/etc). MqDrop follows the same posture — MQ._REPL.DROP is emitted and applied by replication::apply::apply_mq exactly like the other MQ replay commands. Separately, a replicated generic DEL/UNLINK/FLUSHDB/FLUSHALL already removes the stream from the replica's live keyspace via normal command replication regardless of that gate; what it did not do before this fix is tombstone the replica's own wal-v3 MQ plane, which apply_index_parity_hooks now closes. Scope decision (documented in code): DurableQueueRegistry entries are NOT db-indexed (pre-existing limitation, same as MqCreate's registry) — a key match tombstones regardless of which db the deleting command ran in, and FLUSHDB drops every registered durable queue exactly like FLUSHALL since the registry cannot scope to one db. Two different dbs sharing an MQ queue NAME is not a supported configuration. Test-gotcha fixed along the way: the crash-matrix DEL/FLUSHALL scenarios' original sync-marker strategy waited on an AOF-family write to prove the delete was durable — correct pre-fix (DEL only touched the AOF), but MqDrop lands on the wal-v3 MQ plane via a separate fire-and-forget channel drained on its own 1ms tick, so an AOF-only marker no longer proves the tombstone itself reached disk. Both tests now also sync a throwaway durable queue's MQ.PUSH (wal-v3-family) after the delete/flush before crashing. Files touched: - src/mq/wal.rs — encode_mq_drop/decode_mq_drop + MQ_REPL_DROP + is_mq_replay_command update + module docs + unit tests - src/persistence/wal_v3/record.rs — WalRecordType::MqDrop = 0x75 - src/persistence/wal_v3/replay.rs — MqDrop routed through on_command - src/persistence/wal_v3/segment.rs — MqDrop added to plane-history block set - src/shard/shared_databases.rs — apply_mq_drop + replay dispatch wiring + MqReplayStats.drop + 4 new unit tests - src/shard/mq_exec.rs — auto_drop_mq_streams / auto_drop_mq_streams_on_flush / emit_mq_drops - src/server/conn/handler_monoio/mod.rs, src/server/conn/handler_sharded/mod.rs, src/server/conn/shared.rs — hook call sites (DEL/UNLINK + FLUSHDB/FLUSHALL) - src/replication/apply.rs — MQ_REPL_DROP apply arm + apply_index_parity_hooks replica-side tombstone - fuzz/fuzz_targets/mq_wal_record.rs — fuzz decode_mq_drop - tests/crash_matrix_cross_plane/tests_seeded_red.rs — un-gated the RED cell, fixed its wal-v3-family sync race, added a FLUSHALL sibling and a create->drop->create ordering test - CHANGELOG.md — new [Unreleased] entry + removed stale #46 caveats author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds the ChangesMQ durable-stream tombstones
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionHandler
participant mq_exec
participant WAL
participant Replica
participant Replay
Client->>ConnectionHandler: DEL, UNLINK, FLUSHDB, or FLUSHALL
ConnectionHandler->>mq_exec: drop durable MQ streams
mq_exec->>WAL: append MqDrop
mq_exec->>Replica: replicate MQ_REPL_DROP
WAL->>Replay: replay MqDrop in order
Replay->>Replica: remove prior durable MQ state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/shard/shared_databases.rs (1)
432-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting
shared_databases.rs— it exceeds the 1500-line limit.The file is at ~2290 lines and this PR adds ~180 lines of tests. The coding guidelines state "No single Rust file should exceed 1500 lines." The test module (lines 1353–2290) is a natural extraction candidate — moving it to a
testssubmodule or sibling file would bring the production code well under the limit without behavior changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shard/shared_databases.rs` at line 432, Split the tests currently housed in shared_databases.rs into a separate tests submodule or sibling test file, keeping the production definitions such as MqReplayStats in shared_databases.rs. Preserve all test behavior, imports, visibility, and module wiring while reducing the source file below the 1500-line limit.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/shard/shared_databases.rs`:
- Line 432: Split the tests currently housed in shared_databases.rs into a
separate tests submodule or sibling test file, keeping the production
definitions such as MqReplayStats in shared_databases.rs. Preserve all test
behavior, imports, visibility, and module wiring while reducing the source file
below the 1500-line limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c46ee11-ede1-4eed-94b2-fb0bbdb1ce51
📒 Files selected for processing (13)
CHANGELOG.mdfuzz/fuzz_targets/mq_wal_record.rssrc/mq/wal.rssrc/persistence/wal_v3/record.rssrc/persistence/wal_v3/replay.rssrc/persistence/wal_v3/segment.rssrc/replication/apply.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/shared.rssrc/shard/mq_exec.rssrc/shard/shared_databases.rstests/crash_matrix_cross_plane/tests_seeded_red.rs
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
2 participants
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.
Summary
Kernel M3 stage 3, task #46 (filed during PR #291's adversarial review): a durable MQ stream deleted via generic
DEL/UNLINK/FLUSHALL/FLUSHDBwas resurrected on kill-9 restart — wal-v3 replay unconditionally reapplied priorMqCreate/MqPushrecords with no way to represent the deletion. Same bug class as the KV/vector cold-plane resurrection fixed in PR #257, now closed for MQ.Fix
New
MqDropWAL record (discriminant0x75, versioned[version][db_index][key_len][key], fail-closed decode), modeled on theWorkspaceDrop/0x61precedent:cross_plane_mq_create_drop_create_survives).segment_holds_plane_history's MQ discriminant set so WAL recycle never prunes an unfloored tombstone.decode_mq_dropadded to the MQ WAL fuzz target.Crash cell
cross_plane_seeded_red_mq_generic_del_resurrectionflips from RED (red_guard-gated) to a default-GREEN tripwire.Scope decision (documented in code):
DurableQueueRegistryis not db-indexed (pre-existing), so FLUSHDB drops all durable queues like FLUSHALL, and a key match tombstones regardless of db. Two dbs sharing an MQ queue name is not a supported configuration.Test-harness note: the DEL/FLUSH crash scenarios now sync a throwaway durable queue's
MQ.PUSH(wal-v3-family) after the delete before crashing — an AOF-family marker alone no longer proves the tombstone reached disk.Gates
Refs: task #46, kernel M3 stage 3, PR #291 (MQ effect records), PR #257 (cold-plane tombstone precedent).
Summary by CodeRabbit
Bug Fixes
DEL,UNLINK,FLUSHDB, orFLUSHALL, including after crashes and restarts.Tests