Repository navigation
feat(id): durable replication receiver + verifier (#806 phase 3) - #4634
Conversation
DurableReceiver wraps the phase-1 ReplicationTracker with durable storage: pinned-source verification, staged transfers that commit only on the signed terminal frame, atomic snapshot install (scratch + fsync + rename), append-only tail commits, and restart/reconnect recovery that recomputes the verified cursor from the committed journal bytes. Refuses corrupt committed journals at open. 11 tests cover terminal-only commit, truncation, tamper, untrusted source, gap, fork, unsupported version, oversize, nonce replay, reset/version bump, and restart.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 39 minutes. View limit details
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. CodeRabbit Verify
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2742f8435
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| file.write_all(&bytes)?; | ||
| file.sync_all()?; |
There was a problem hiding this comment.
If the process crashes during this append, or write_all partially writes before returning an error such as disk-full, the committed journal retains a partial batch. Poisoning the session does not undo those writes, and open rejects an unterminated entry as CorruptReplica, preventing recovery through a fresh replication session. A partial batch ending at an entry boundary is instead silently treated as fully committed on restart. Persist a recoverable committed-length boundary or equivalent transaction record so failed/interrupted appends can be rolled back to the last complete committed batch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a805749. Tail appends are now transactional: before appending, the receiver durably records the journal pre-append length in an append-pending record (written through a scratch file + rename, so the record itself is never torn) and removes it only after the appended bytes fsync. At open, a leftover record truncates the journal back to the recorded length before validation — so a crash or partial write mid-append can neither wedge the replica with an unterminated line nor silently promote a batch that stops at an entry boundary. An append error also attempts an immediate rollback in-process. New test interrupted_tail_append_is_rolled_back_on_open covers both debris shapes (torn line and complete extra entry).
| payload_digest: crate::world::replication::payload_digest(&oversized), | ||
| payload: oversized, | ||
| }; | ||
| let error = receiver.accept_frame(&sign(envelope, &key)).unwrap_err(); |
There was a problem hiding this comment.
This test always panics before reaching accept_frame: its payload exceeds MAX_REPLICATION_PAYLOAD_BYTES, so SignedReplicationEnvelope::sign returns None, which the local sign helper immediately unwraps. Consequently, the new receiver test suite fails rather than exercising oversized-frame rejection. Construct and sign the deliberately invalid envelope through a test-only path that bypasses the production signer's size guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a805749. The oversized test now signs through a test-only sign_unbounded helper that replicates the signing-bytes contract (domain + every envelope field + exact payload bytes) and signs directly, bypassing the production signer size guard — so the frame passes signature verification and actually reaches the payload-bound check in ReplicationTracker::accept. If the signing contract changes, the helper frames fail verification and the test fails loudly rather than silently passing.
| ReplicationKind::Tail => { | ||
| match self.staged.as_mut() { | ||
| Some(staged) => staged.extend(&accepted.payload), | ||
| None => self.staged = Some(Staged::Tails(accepted.payload)), |
There was a problem hiding this comment.
If a pinned source sends a correctly signed base-cursor tail as the first frame after reconnect, the tracker accepts it because begin_session resets verification to the chain base, but this branch stages it for append to the existing journal. A tail containing the matching Created header followed by a valid terminal therefore appends a second header, reports a committed cursor that does not describe the stored bytes, and makes the next open fail with CorruptReplica. A terminal-first session likewise overwrites the committed cursor with the base without changing the journal. Although the current writer emits a snapshot first, the receiver must enforce that ordering rather than treating these accepted frame sequences as durable commits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a805749. The receiver now enforces snapshot-first ordering itself: on a session first frame it decodes the frame (shape only) and refuses any non-snapshot kind with the new ReceiverError::SessionMustStartWithSnapshot BEFORE the tracker sees it, so a refusal leaves tracker, staging, journal, and the committed cursor all untouched. A base-chained tail can no longer be appended to an existing journal, and a terminal-first session can no longer move the committed cursor without moving the journal. New test tail_or_terminal_first_in_a_session_is_refused covers both, and verifies the replica still opens cleanly at the prior committed cursor.
… oversized test (#806 phase 3) Codex review on PR #4634: (P1) a crashed or partial tail append could wedge the replica as CorruptReplica or silently promote an uncommitted batch - tail appends now write a durable append-pending record (pre-append length, installed by rename) before appending and remove it after fsync; open rolls a leftover record back. (P2) a session starting with a base-chained tail or a terminal could corrupt committed state - the receiver now refuses a non-snapshot first frame (SessionMustStartWithSnapshot) before the tracker sees it. (P2) the oversized-payload test panicked in the bounded signer - it now signs through a test-only unbounded path replicating the signing-bytes contract. New tests: tail/terminal-first refusal, interrupted-append rollback (torn and entry-boundary debris).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8057499a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Err(error) = append_result { | ||
| // Best-effort immediate rollback; the pending | ||
| // record makes the next open roll back regardless. | ||
| let _ = self.roll_back_to(prior_len); | ||
| return Err(error); |
There was a problem hiding this comment.
If a tail append fails during write_all or sync_all, this path leaves the append-pending record behind even when immediate rollback succeeds. Calling begin_session on the poisoned receiver then permits a reconnect snapshot to commit successfully without clearing that record. On the next open, roll_back_interrupted_append truncates the replacement snapshot to the old pre-append length, silently dropping committed entries or corrupting a reset snapshot. Recover and clear the pending transaction before allowing a replacement snapshot to commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 80c8353. Two layers: (1) commit_staged's Snapshot arm now runs roll_back_interrupted_append before installing a replacement snapshot, so a stale append-pending record is recovered and cleared before the snapshot commits and can never truncate it at the next open; (2) the tail-append error path now clears the append-pending record (plus directory fsync) whenever the immediate rollback succeeds — the record is only left behind when rollback itself failed, in which case the next open or a snapshot commit's recovery performs it. Regression test: stale_append_pending_record_cannot_roll_back_a_replacement_snapshot. Replication suite on the laptop: 33 passed. (Reply posted after the merge — GitHub refused review-comment replies with 422 abuse errors during the shift.)
| world_id: WORLD.to_string(), | ||
| source, | ||
| header_version: 1, | ||
| kind: ReplicationKind::Tail, |
There was a problem hiding this comment.
The unbounded signer fixes the previously reported signing panic, but the new snapshot-first guard now rejects this test's first Tail frame with SessionMustStartWithSnapshot, before the tracker checks its payload size. Consequently, the assertion expecting PayloadTooLarge still fails. Make the oversized envelope a snapshot, or establish a valid snapshot-first session before submitting the oversized tail, so this test exercises the intended size check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 80c8353. The oversized test now builds the oversized envelope as a Snapshot frame (still signed through the test-only sign_unbounded helper), so it passes the snapshot-first guard and reaches the tracker's payload-size check: oversized_payload_is_refused passes, asserting PayloadTooLarge. The same commit also fixes three pre-existing E0618 compile errors in this test module (the receiver helper fn shadowed by local bindings named receiver, via a new reopen helper) — the module did not compile at a805749, which is why this assertion had never actually run. Replication suite on the laptop: 33 passed. (Reply posted after the merge — GitHub refused review-comment replies with 422 abuse errors during the shift.)
…oversized test + receiver test compile (#806 phase 3) Codex re-review of a805749 raised two findings: (P1) a tail append whose immediate rollback succeeded left its append-pending record behind, so a later replacement snapshot could be truncated at next open - commit_staged now recovers/clears any pending append before a snapshot commit, and a successful immediate rollback clears the record too; regression test stale_append_pending_record_cannot_roll_back_a_replacement_snapshot. (P2) the oversized-frame test used a Tail frame and was refused by the snapshot-first guard before reaching payload validation - it now sends a Snapshot frame. Also fixes three pre-existing E0618 compile errors in the receiver tests (helper fn shadowed by a local binding named receiver) via a reopen helper: the module did not compile at a805749. Tests: cargo test -p id --lib replication - 32 passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80c8353cbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let accepted = tracker.accept(frame_bytes)?; | ||
| (accepted, tracker.cursor(), tracker.header_version()) |
There was a problem hiding this comment.
If the pinned source sends a correctly signed snapshot whose envelope and Created header both specify version 0, followed by its terminal frame, the receiver commits it successfully: ReplicationTracker::accept only rejects versions above MAX_JOURNAL_VERSION. However, validate_committed_journal explicitly rejects version 0, so the next open fails with CorruptReplica, even if this snapshot replaced a previously valid replica. Enforce the same supported-version range during frame acceptance as during reopening, before advancing the tracker or replacing committed bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e768cd7. ReplicationTracker::accept now enforces the full supported journal range (header_version == 0 or > MAX_JOURNAL_VERSION both refuse with UnsupportedJournalVersion) before any tracker state advances — the same range validate_committed_journal enforces at open, so an accepted frame can no longer commit a journal the next open would reject. Regression test: version_zero_snapshot_is_refused_before_any_entry. Replication suite on the laptop: 33 passed; Codex re-review of e768cd7 reported no findings and PR #4634 merged as 844fac4. (Reply posted after the merge — GitHub refused review-comment replies with 422 abuse errors during the shift.)
…hase 3) Codex re-review of 80c8353 (P2, comment 4449166369): ReplicationTracker::accept only rejected header versions above MAX_JOURNAL_VERSION, so a correctly signed version-0 snapshot committed - then validate_committed_journal rejected it at the next open, bricking the replica and destroying any valid journal it replaced. Accept now enforces the full supported range 1..=MAX_JOURNAL_VERSION before any state advances. Regression test: version_zero_snapshot_is_refused_before_any_entry. Tests: cargo test -p id --lib replication - 33 passed.
Phase 3 of #806 (F12.13): durable receiver + verifier
Implements phase 3 of the phased plan in
doc/2026-11-01_design_id806.md§7, on top of phase 1 (PR #4447, wire contract +ReplicationTracker) and phase 2 (PR #4589, writer snapshot-plus-subscribe feed; merged as 51535c2, this branch's base).What lands
New module
pkgs/id/src/world/replication_receiver.rs:DurableReceiver, a durable replica of one world's journal from one pinned source.ReplicationTrackerbuilt with the receiver's pinned source; untrusted signers, bad signatures, nonce replays, gaps, forks, stale replays, unsupported journal versions, and oversized payloads are refused by the tracker before payload staging.journal.jsonl, and the directory is fsynced, so a crash mid-install never leaves a torn journal (a leftover scratch file is cleaned up at open). Committed tail batches append + fsync; committed bytes are never rewritten.DurableReceiver::openre-reads the committed journal, refuses it if it is not a well-formed chain for this world at a supported header version (decodable lines, exactly one leadingCreatedheader), and recomputes the verified cursor from the entry bytes under the pinned source. Every session opens with a snapshot (the phase-2 feed contract), which rebases the tracker; staged bytes from a dead session are discarded.Scope note — module transfer
The packet scope mentions module transfer/hash checks. The phase-1/2 contract as built carries journal bytes only — there is no module-bytes frame kind in v1, so there is no module payload for the receiver to hash-check. Module hashes named by journal entries (e.g.
BuilderInstalled.module_hash) are covered by the cursor chain like every other entry byte. This is recorded in the module docs; adding a module frame kind is a wire-contract change for a later phase, not something phase 3 can invent receiver-side.Tests
13 tests in the module (
cargo test --lib replication_receiver): snapshot commits only on terminal; truncated/abandoned transfer commits nothing; tail batch withholds append until its terminal, then appends exactly; tampered payload refused with committed replica intact; untrusted source refused; gap and fork tails refused; unsupported journal version refused before any entry; oversized payload refused (signed through a test-only unbounded path, per Codex); captured frames replayed into a new-nonce session refused; reset snapshot replaces the journal and bumps the header version; restart reopens committed state and a corrupt journal is refused at open; frames without a session are refused; a session starting with a tail or terminal is refused before the tracker sees it; an interrupted tail append (torn or entry-boundary debris under a durable append-pending record) is rolled back at open.Refs #806 (phase 3 of 5; #806 closes when phase 5 lands). Queue task 6536.