fix(persistence): segment_plane_scan must unwrap v0.6.0's nested-Command plane framing (task #69)#331
Conversation
…and plane framing (task #69) v0.6.0's shard event-loop wal_append drain hardcoded WalRecordType::Command as the outer wrapper for every plane record (WorkspaceCreate/Drop, MqCreate/Push/Pop/Ack/Trigger/Drop, TemporalUpsert, GraphTemporal) — verified against the v0.6.0 tag's event_loop.rs (lines 1452/2070). segment_plane_scan classified segments purely from the OUTER record-type byte, so after a v0.6.0->v0.7.0 upgrade every plane record in a pre-upgrade segment was invisible to the scan and the whole segment was misclassified as recyclable pure-KV history. WS/MQ/temporal-upsert have no snapshot or checkpoint format in any mode (WAL-only replay, task #43), so recycle_aggressive/ recycle_segments_before could permanently delete the sole durable copy of that history — the release-blocking data-loss bug for the v0.7.0 tag. segment_plane_scan now peeks a Command record's payload for a nested v3 record frame (length-prefix + CRC32C validated, matching read_wal_v3_record's own framing) and classifies the INNER type against the same blocking set, mirroring the unwrap shared_databases.rs::replay_workspace_wal/replay_mq_wal/replay_temporal_wal already perform via read_wal_v3_record(&record.payload) at replay time. Fails closed on an unrecognized inner type; an ordinary non-nested Command payload is unaffected and stays recyclable. Byte-slice peek only, no allocation, no LZ4 decompression. Red/green TDD: added failing tests for nested WorkspaceCreate (blocks recycle) and nested GraphTemporal (flags has_graph_temporal, does not block), a fail-closed test for an unrecognized nested inner type, and a pinning test that a plain (non-nested) Command payload stays recyclable. All four failed against unmodified code, all pass after the fix (3x consecutive runs, both default and runtime-tokio+jemalloc feature matrices). fmt and both clippy matrices clean. CHANGELOG.md: added [Unreleased] entry documenting the fix. 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? |
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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 |
2 participants
Summary
Deep-review Finding 1 (High, orchestrator-confirmed against the v0.6.0 tag): v0.6.0 binaries framed EVERY plane record (WS/MQ/TemporalUpsert/GraphTemporal) nested inside an outer
CommandWAL record (v0.6.0event_loop.rs:1452,2070hardcoded the type).segment_plane_scanjudgedblocks_recyclefrom the outer byte only, so after a v0.6.0→v0.7.0 upgrade,recycle_aggressive/recycle_segments_before(autovacuum Pass C, checkpoint Finalize, max-wal-size) could delete the sole durable copy of WS/MQ/temporal history — these planes have no snapshot format. Invisible to the crash matrix because every cell writes post-K1 direct-typed records.Fix:
peek_nested_plane_record_type— allocation-free byte-slice peek (length bounds + CRC32C over the identical spanread_wal_v3_recordchecks) wired into the scan'sCommandarm; inner type matched against the same blocking set. Fail-closed on valid-frame-with-unknown-inner-type. Scan and replay (shared_databases.rs:379,932,1321) now agree byte-for-byte on what a nested record is.Verification
wal_v3::segmentmodule (26) +test_recycle_*plane-history family (6), 3× each, both default and tokio+jemalloc matrices.Blocks the v0.7.0 tag (upgrade path, REL-02).