feat(wal): typed wal_append channel end-to-end + WS.CREATE created_at (Wave B stage 1 / kernel M1)#289
Merged
Merged
Conversation
…(K1a+K1b) Stage 1 of Wave B / storage-kernel M1 (storage-audit-2026-07-12-wal.md enhancement #2 + wave-b-ws-mq-scope-2026-07-12.md item 1). Two fixes, shipped together because they compile-depend on each other in three shared producer files (handler_monoio/write.rs, handler_sharded/write.rs, uring_handler.rs): K1b's `encode_workspace_create` signature change lands in the same hunks as K1a's unframing, so splitting them into strictly sequential commits would require a throwaway intermediate revert with no compile-time value. K1a — preserve the caller's WalRecordType through the wal_append channel -------------------------------------------------------------------------- `ShardDatabases::wal_append`/`try_wal_append_required` and `mq_exec::wal_append_on_slice` took a plain `Bytes` payload; the shard event-loop's 1ms-tick drain (`event_loop.rs`, two sites) unconditionally re-wrapped every message as an outer `WalRecordType::Command` record. Every non-Command producer (XactCommit, WorkspaceCreate/Drop, MqCreate/Ack, GraphTemporal) worked around this by pre-framing its own record with `write_wal_v3_record` and sending the ALREADY-FRAMED bytes through — a second, nested WAL frame inside the outer Command frame, recovered on replay via a bespoke per-record-type unwrap. The XactCommit pre-framing additionally passed `txn_id` into the inner frame's `lsn` field, mislabeling a transaction id as a WAL LSN. Fixed structurally: the channel item is now `(WalRecordType, Bytes)` — the real type plus the UNFRAMED payload — threaded through `ShardSlice::wal_append_tx` / `ShardDatabases::wal_append_txs` end-to-end. The drain calls `wal.append(record_type, &payload)` directly, so the WAL writer assigns the real LSN and does the single framing. Every producer's pre-framing was deleted: handler_monoio/handler_sharded write.rs (WS.CREATE/ DROP) and txn.rs (XactCommit — the txn_id-as-lsn mislabel is gone with it), shard/uring_handler.rs's WS batch path, shard/mq_exec.rs (MqCreate/MqAck), and command::temporal::apply_invalidate (GraphTemporal — PR #286 had just added this record's pre-framing; deleted in favor of the typed channel). `apply_invalidate` now RETURNS the raw payload instead of pushing it into `GraphStore::wal_pending` (which stays exclusively Command-typed RESP bytes). The one caller that cannot change its Frame-only return signature without a ~90-call-site test ripple (`command::graph::dispatch_graph_command`, the cross-shard GraphCommand entry point) stashes the payload in a new `GraphStore::temporal_wal_pending` side-channel instead, which its real caller (`shard/spsc_handler.rs`'s GraphCommand arm) `.take()`s right after dispatch. Replay is fully backward compatible, no format bump: every existing direct-type match arm (replay_workspace_wal, replay_mq_wal, replay_temporal_wal in shared_databases.rs) already had a nested-Command unwrap fallback plus, for GraphTemporal, a legacy-raw fallback for even older un-nested records — both untouched, so segments written before this fix keep replaying exactly as before. persistence::recovery.rs Phase 4's on_command closure had no XactCommit arm at all (silent `_ => {}`) — now reachable for the first time because the outer type used to always be Command. Decision: an EXPLICIT documented no-op (counts toward commands_replayed, never dispatches). The forward-image KV payload is redundant in every reachable config — the transaction's individual SET/DEL ops already ride either this same Phase-4 WAL replay as ordinary Command records (--wal-kv-log on) or the AOF, the KV recovery authority in every config (Phase 4b falls back to it whenever kv_commands_replayed == 0). Decoding it would be a no-op at best and risks double-applying a non-idempotent op at worst. wal_v3::replay's replay_wal_v3_dir_commands (the legacy last-resort-fallback path) already had a correct replay_xact_commit call for the real outer type — untouched, and now reachable for the first time too. K1b — WS.CREATE's created_at survives restart -------------------------------------------------------------------------- `encode_workspace_create`/`decode_workspace_create` only ever serialized `[ws_id][name_len][name]` — the created_at computed at WS.CREATE time never reached the WAL payload, so replay_workspace_wal hardcoded `created_at: 0` on every restart. Fixed with a versioned, backward-compatible layout: new records append a trailing `created_at_ms: i64 LE`; the decoder accepts both the old (20+name_len-byte) and new (20+name_len+8-byte) lengths, returning created_at = 0 for old records (matching prior restart behavior exactly — no format bump, mixed-segment compatible). All three producers now pass the already-computed created_at through; replay threads the real value into WorkspaceMetadata. Tests -------------------------------------------------------------------------- - src/persistence/wal_v3/segment.rs: test_typed_channel_preserves_workspace_create_outer_type — sends a WS.CREATE record through the exact (WalRecordType, Bytes) channel + drain pattern event_loop.rs uses, asserts the on-disk outer type is WorkspaceCreate (not Command) and the payload decodes directly. - src/workspace/wal.rs: updated roundtrip tests for the 3-arg signature; added test_workspace_create_legacy_format_no_created_at (pre-K1b 20+name_len-byte payload still decodes, created_at=0) and test_workspace_create_created_at_negative_value. - tests/quickwins_red.rs: updated qw2 for the new wal_append/ try_wal_append_required/set_wal_append_tx signatures (PIN test, no behavior change). Gates (all green unless noted) -------------------------------------------------------------------------- - cargo check/test --release --lib (default features): 4148 passed, 0 failed - cargo check/clippy --no-default-features --features runtime-tokio,jemalloc: clean (-D warnings) - cargo fmt --check: clean - crash_recovery_temporal_mq -- --ignored: 1/1 pass - shardslice_live (incl. ssm3 WS registry restart): 6/6 pass - crash_recovery_wal_recycle_legacy -- --ignored: 1/1 pass - crash_recovery_graph_durability g4/g5 -- --ignored: 2/2 pass - Full tokio-matrix integration suite (--no-fail-fast): all green except 5 pre-existing/environmental failures with ZERO overlap with this diff (confirmed via `git diff --stat`): cmd_flush_dbsize_debug_memory uses a removed --persistence-dir CLI flag (stale since a --dir rename, unrelated file untouched here); mem_watchdog/oom_bypass_closure/spsc_two_db fail on "MOONERR diskfull" — this checkout's volume is at 95% disk usage, tripping the <5% free-space write-pause guard (a known environment gotcha, not a code defect); memory_prometheus_kinds expects 7 Prometheus memory kinds but the binary now reports 8 — pre-existing drift in an unrelated metrics file this change never touches. author: Tin Dang
…ly asymmetry Adversarial review P2 on the K1a typed-channel change: recovery.rs Phase 4 treats XactCommit as a documented no-op while the legacy fallback replay_wal_v3_dir_commands applies it — both newly reachable for real-typed records. Documents why both are correct: the fallback runs only when Phase 4 replayed zero KV commands AND no AOF exists, i.e. exactly when the redundancy argument for the no-op does not hold. 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? |
📝 WalkthroughWalkthroughThe PR preserves producer WAL record types through typed append channels, removes producer-side framing, adds direct GraphTemporal persistence, persists WS.CREATE timestamps with backward-compatible decoding, and updates XactCommit replay accounting. ChangesWAL persistence and recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphCommand
participant GraphStore
participant ShardDatabases
participant WALWriter
Client->>GraphCommand: TEMPORAL.INVALIDATE
GraphCommand->>GraphStore: apply_invalidate()
GraphStore-->>GraphCommand: GraphTemporal payload
GraphCommand->>ShardDatabases: wal_append(GraphTemporal, payload)
ShardDatabases->>WALWriter: typed payload
WALWriter-->>ShardDatabases: persisted WAL record
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/graph/store.rs (1)
249-266: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMark
GraphStoredirty for temporal invalidations.persist_graph_at_checkpoint()(src/graph/recovery.rs) returns early whenGraphStore::is_dirty()is false, so a loneTEMPORAL.INVALIDATEcan leave the graph unsnapshotted while itsGraphTemporalWAL record becomes eligible for recycle. Setgs.mark_dirty()when stagingtemporal_wal_pending.🤖 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/graph/store.rs` around lines 249 - 266, Mark the GraphStore dirty when staging a temporal WAL payload so temporal-only invalidations trigger checkpoint persistence. Update the TEMPORAL.INVALIDATE flow that assigns GraphStore::temporal_wal_pending, calling GraphStore::mark_dirty() in the same path before or alongside the assignment; leave normal WAL handling unchanged.
🤖 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.
Inline comments:
In `@src/workspace/wal.rs`:
- Around line 28-33: Update the documentation for decode_workspace_create to
accurately describe its lenient length handling: retain the compatibility
behavior for the legacy base length, explain that shorter-than-timestamp or
longer payloads are accepted according to the current implementation, and remove
the claim that all other lengths are malformed.
---
Outside diff comments:
In `@src/graph/store.rs`:
- Around line 249-266: Mark the GraphStore dirty when staging a temporal WAL
payload so temporal-only invalidations trigger checkpoint persistence. Update
the TEMPORAL.INVALIDATE flow that assigns GraphStore::temporal_wal_pending,
calling GraphStore::mark_dirty() in the same path before or alongside the
assignment; leave normal WAL handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5d68afd-ef5f-47a6-856f-b461d427a6b2
📒 Files selected for processing (21)
CHANGELOG.mdsrc/command/graph/mod.rssrc/command/temporal.rssrc/graph/store.rssrc/persistence/recovery.rssrc/persistence/wal_v3/segment.rssrc/replication/apply.rssrc/server/conn/handler_monoio/txn.rssrc/server/conn/handler_monoio/write.rssrc/server/conn/handler_sharded/txn.rssrc/server/conn/handler_sharded/write.rssrc/shard/coordinator.rssrc/shard/event_loop.rssrc/shard/mq_exec.rssrc/shard/shared_databases.rssrc/shard/slice.rssrc/shard/spsc_handler.rssrc/shard/uring_handler.rssrc/transaction/abort.rssrc/workspace/wal.rstests/quickwins_red.rs
…h handling CodeRabbit (PR #289): the doc claimed "any other length is malformed", but the decoder deliberately tolerates 0-7 trailing bytes (created_at 0) and ignores extra bytes past the timestamp — the mixed-segment compat + future-suffix contract. Reword the doc to describe the actual behavior; no code change. author: Tin Dang
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/workspace/wal.rs (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest every documented suffix-length compatibility case.
The legacy test covers zero trailing bytes, but the contract also promises that 1–7 bytes default to zero and bytes after a complete timestamp are ignored. Add table-driven cases to prevent replay compatibility regressions.
Also applies to: 135-153
🤖 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/workspace/wal.rs` around lines 28 - 34, Add table-driven coverage for the WAL payload decoder’s documented suffix-length behavior, including 1–7 trailing bytes producing created_at_ms = 0 and payloads with more than 8 trailing bytes ignoring bytes after the complete timestamp. Extend the existing legacy test near the decoder to cover zero bytes and all intermediate lengths while preserving the current expected decoded workspace ID and name.
🤖 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/workspace/wal.rs`:
- Around line 28-34: Add table-driven coverage for the WAL payload decoder’s
documented suffix-length behavior, including 1–7 trailing bytes producing
created_at_ms = 0 and payloads with more than 8 trailing bytes ignoring bytes
after the complete timestamp. Extend the existing legacy test near the decoder
to cover zero bytes and all intermediate lengths while preserving the current
expected decoded workspace ID and name.
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
Storage-kernel M1 stage 1 / Wave B foundation: the cross-thread WAL append channel now preserves the caller's
WalRecordTypeend-to-end — retiring the design where every typed record (XactCommit, WorkspaceCreate/Drop, MqCreate/Ack, GraphTemporal) was pre-framed by its producer, shipped as raw bytes, and blanket re-wrapped as an outerCommandrecord by the event-loop drain. That nesting is what made XactCommit replay functionally dead, mislabeled a txn-id as a WAL LSN, and forced every replayer to grow bespoke unwrap arms (PR #286's fix class).K1a — typed channel
Bytes→(WalRecordType, Bytes). All 15 production send sites across both runtimes converted (compiler-enforced via the arity change); producers'write_wal_v3_recordpre-framing deleted — single framing, real LSN assignment atwal.append.TEMPORAL.INVALIDATEproducer framing (added in fix(shard): TEMPORAL/MQ WAL v3 replay data loss on kill-9 (task #42) #286) replaced by the typed channel; the GraphStoretemporal_wal_pendingside-channel carries the payload from dispatch to the SPSC drain (single producer, single.take()consumer on the same shard thread).recovery.rs: Phase 4 counts-but-never-dispatches (the txn's inner ops already ride--wal-kv-logCommand records or the AOF authority — applying would risk double-apply); the last-resort Phase-4b fallback (replay_wal_v3_dir_commands) DOES apply it — it runs exactly when neither coverage source exists, so the preconditions are complementary (asymmetry note added after review).K1b — WS.CREATE
created_atdurabilityencode/decode_workspace_creategains a versionedcreated_at_mssuffix (old-length records decode as before,created_at 0); restart now restores the real creation time instead of zeroing it — prerequisite for Wave B'sWS.CREATE.APPLYreplication record.Verification
WorkspaceCreate(notCommand) through the exact channel+drain pattern; created_at round-trip + legacy-format tolerance + negative-value handling.-D warningsdefault + tokio, fmt.MOON_BINpinned): same crash suites green, lib 4168/4168.Notes
handler_shardedTEMPORAL.INVALIDATE lacks the replication-fanout block its monoio twin has (tokio has no master-side replication machinery).Summary by CodeRabbit
created_atis now stored and replayed correctly; legacy records continue to load with the prior default.