feat(journal): opt-in retention via RetainableJournal::truncate_before by laconc · Pull Request #62 · kassian-dev/ironstate

laconc · 2026-08-29T03:43:00Z

Implements proposal E from the adopter feedback. Now based directly on main, since #61 has merged.

RetainableJournal itself is a capability rather than a change to Journal, but this PR is not purely additive — it also changes VersionedEvent, which Journal::events_since returns:

a required seq field, so a subscription can key on the record's real sequence rather than an index into the flattened result;
#[non_exhaustive] plus a VersionedEvent::new constructor, so the next field is additive for adapters instead of breaking.

Both break any downstream code that built or exhaustively destructured VersionedEvent with a struct literal. That is deliberate and it is the right release for it: #61 already took ironstate-journal to 0.2.0 on main, so the break is paid once rather than twice. Release notes should call it out — adapters need the constructor, not a field list.

A log that only grows cannot satisfy a retention policy or a right-to-erasure obligation. The snapshot machinery that makes truncation safe already existed; this exposes it.

Why a supertrait rather than part of `Journal`

Unlike streams and transactions (#61), retention is not something every adapter must answer. An append-only store may have no way to drop records, and some domains forbid it outright. Opt-in is the honest shape.

The two things it has to get right

Truncation never renumbers. A Seq is an identity, and Subscription holds high-water marks that refer to it — renumbering on truncation would silently corrupt every one of them. Retained records keep their sequence numbers; MemoryJournal's streams carry a truncated base for this. A Seq below the horizon reports UnknownSeq — the record existed once, but this journal can no longer answer for it — rather than quietly returning a neighbour.

Truncation is refused when it would strand the stream. Replay resumes from the newest snapshot and reads every record after it, so truncating past that point leaves the stream unresumable. JournalError::NoSnapshotForTruncation names both the snapshot it found and the one it needed:

refusing to truncate before Seq(9): the newest snapshot is Some(Seq(3)).
Truncating here would discard records still needed to replay from that snapshot,
leaving the stream unresumable.
Take a snapshot at or after Seq(8) first, then truncate.

Contract property 10

Truncation preserves resume — after truncating at a snapshot boundary the stream resumes to a bit-identical aggregate at the same entropy position, and reports the new horizon. Reachable from journal_contract_test!(J, A, retainable) (or forkable, retainable), and run against MemoryJournal by default.

That property is what makes retention safe to offer at all: dropping history must be invisible to everything downstream of the snapshot it was taken against.

Also: the hash-chain proposal is deferred, on purpose

Recorded in AGENTS.md under "What deliberately doesn't exist", with its reactivation trigger. The short version, after evaluating it against a second ironstate consumer:

A chain is evidence only when its root is externally anchored, and anchoring is application infrastructure — so ironstate would ship the half that doesn't deliver the property on its own.
Of the two known consumers, only one has the requirement. The other has no audit machinery at all and could not consume it anyway: its production adapter is async and structurally cannot implement the synchronous Journal, so a decorator would only ever wrap its test twin.
Which means the obvious shape is wrong. If this is ever upstreamed it must be pure functions — link_hash(prev, seq, encoded_events, entropy_pos) plus verify_chain over an iterator — hashing bytes the adapter already persists (no new StableHash bound on A::Event), per batch rather than per event, carrying a per-link algorithm tag so the chain outlives its hash function.

The adopter who needs it is building it downstream first; we upstream once the design is proven against a real anchor.

Verification

make check, make msrv, make doc, make wasm, make deny all pass.

🤖 Generated with Claude Code

laconc · 2026-08-29T04:25:37Z

Update: review fixes pushed (`90f11fb`, `20ccaef`)

A full-stack review turned up 15 findings; 12 are fixed here and 3 documented. Five were confirmed with probe tests before being reported. Everything below was in my own work on this stack.

Truncation × entropy bookkeeping — the worst of it

A fully-truncated stream reported head() == None, so execute took its rewind anchor as DrawPos(0) and reissued draws already consumed. Entropy positions ran backwards while sequences ran forwards — a direct violation of the determinism contract. The records are gone; the history is not.
events_since silently returned a gapped list when read from below the horizon. A subscription resuming from a stale high-water mark got non-contiguous records and dropped one, with no error anywhere. It now refuses; retained_from says where a valid read starts.
entropy_pos now consults a snapshot at the requested Seq before the records — a snapshot records the position at its own sequence and outlives the record there. Without this, head() and entropy_pos(head) disagreed after a full truncation.
entropy_pos(_, Seq(0)) short-circuited before the horizon check and fabricated DrawPos(0) for a genesis truncation had discarded.
truncate_before created the stream via stream_mut, so a refused truncation left a phantom entry behind; and it never bounded at against the head, so a snapshot recorded past the head authorised discarding everything.
fork could hand back a branch with no replay base as Ok. New JournalError::NoBaseForFork refuses it where it happens.

Conformance — two self-inflicted coverage regressions

Round-trip had moved behind run_contract_forkable, so the two-argument journal_contract_test! silently stopped checking the single most important journal property. It never needed fork; that was implementation convenience. Restored to the base suite over events_since, with the per-step variant staying behind forking.
run_contract_forkable drove every history twice from a differently-advanced runner — double the suite time, and a fork failure was irreproducible against the case just reported. One pass now.
Property 11 additionally checks positions stay total and monotonic across the retained range and that everything below the horizon is UnknownSeq — the failure mode retention introduces, invisible to properties 2 and 10 on an untruncated journal.

Keying

VersionedEvent now carries the Seq of the record it came from. events_since flattens events across records, so its index is not a sequence: as soon as one decide emitted two events, the hidden-info subscription recorded a mark past the stream head and dropped every later event as a duplicate.

Documented rather than fixed

Each at the point an implementor meets it: execute_in is one-call-per-transaction while head/entropy_pos take no tx; Subscription::deliver is idempotent but not atomic and is bound to Tx = (); and the conformance suite cannot yet drive an adapter whose Tx is a real transaction — the sharpest finding, since that is the adapter shape the seam exists for. Such adapters are held by a Tx = () twin, as async-store already is.

Also: a rollback test in tests/transactional.rs whose assertion was vacuously true — deleting the entire rollback half would not have turned it red — is rewritten so it can fail.

Surface trimmed: StreamId::MAIN/main() removed (unused), and streams() moved from Journal to RetainableJournal, where sweeping expired history is an actual caller.

All five gates pass.

(Comment from Claude, acting on behalf of the repo owner.)

Copilot

🟡 Changes recommended

There is a newly introduced expect(...) panic in MemoryJournal::append_in plus a documented “additive”/API-surface mismatch (removed StreamId::main and moved streams() off Journal) that should be resolved or explicitly documented before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds an opt-in retention capability to ironstate-journal via the new RetainableJournal supertrait, implementing safe truncation semantics (no renumbering; refusal when truncation would break replay) and extending the conformance suite with a truncation-preserves-resume property. The PR also strengthens sequencing semantics by tagging each returned VersionedEvent with the record Seq it came from (so subscriptions can key idempotency correctly when a record contains multiple events).

Changes:

Introduces RetainableJournal (truncate_before, retained_from, streams) and implements it for MemoryJournal with a retained-horizon model.
Extends VersionedEvent with seq: Seq and updates journals/examples to populate and use it correctly.
Adds retention-focused tests (tests/retention.rs) and a new conformance property (property 11) wired into journal_contract_test!.

File summaries

File	Description
app/crates/ironstate-journal/src/journal.rs	Adds `RetainableJournal`, new retention/fork errors, and `VersionedEvent.seq`; removes `StreamId::main` helpers.
app/crates/ironstate-journal/src/memory.rs	Implements truncation without renumbering via a per-stream `truncated` base; updates addressing/reads/forks across the horizon.
app/crates/ironstate-journal/src/contract.rs	Adds `run_contract_retainable` + property 11; makes round-trip a non-forkable property.
app/crates/ironstate-journal/src/macros.rs	Extends `journal_contract_test!` to optionally run retainable properties; default memory journal run includes retainable.
app/crates/ironstate-journal/tests/retention.rs	New focused regression/behavior tests around truncation horizon, addressing, resume, and fork interactions.
app/crates/ironstate-journal/tests/transactional.rs	Updates test journal `events_since` to set `VersionedEvent.seq`; refines staged-vs-durable side-effect modeling.
app/crates/ironstate-journal/src/subscription.rs	Documents that delivery is idempotent (not atomic) and why `execute_in` isn’t usable here.
app/crates/ironstate-journal/src/replay.rs	Adds documentation about `execute_in` constraints with non-transaction-aware reads.
app/crates/ironstate-journal/src/sim.rs	Removes now-nonexistent `Journal::streams` forwarding.
app/crates/ironstate-journal/src/lib.rs	Re-exports `RetainableJournal` and the new contract runner.
app/crates/examples/hidden-info/src/main.rs	Fixes subscription high-water mark to use `event.seq` (record seq) rather than flattened index.
app/crates/examples/async-store/src/main.rs	Updates returned events to include the record `seq`; removes sync-twin-only `streams` helper usage.
AGENTS.md	Records deferral rationale for hash-chained append / tamper evidence.

Review details

Files reviewed: 13/13 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

It introduces a semver-breaking public struct field addition (and a contract numbering mismatch vs the PR description) that should be resolved/clarified before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 12/12 changed files
Comments generated: 3
Review effort level: Lite

Copilot

🟡 Changes recommended

There are confirmed correctness/API-footgun issues in MemoryJournal::truncate_before semantics (e.g., Seq(0) handling and snapshot eligibility) and a macro-API usability gap that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

app/crates/ironstate-journal/src/contract.rs:442

The PR description calls this new retention check “contract property 11”, but the code (and panic messages) label it as “Property 10” (property_10_truncation_preserves_resume). Please align the property number to avoid confusion when a failing run reports a property number that doesn’t match the PR/design text.

/// Property 10 — **truncation preserves resume**. After truncating at a
/// snapshot boundary, the stream resumes to a bit-identical aggregate and the
/// same entropy position.
///
/// This is what makes retention safe to offer at all: dropping history must be

Files reviewed: 12/12 changed files
Comments generated: 3
Review effort level: Lite

Copilot

🟡 Changes recommended

There are a few concrete doc/test issues to address (and a public-API “additive” claim mismatch around VersionedEvent) before this should be approved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

app/crates/ironstate-journal/src/memory.rs:202

The comment about after: None is misleading: per Journal::events_since docs, None means “from the stream’s start / genesis (Seq(0))”, not “from the truncation horizon”. On a truncated stream None becomes invalid because genesis is below the retained horizon, which is why this method returns UnknownSeq { at: Seq(0) } in that case.

        stream: &StreamId,
        after: Option<Seq>,
    ) -> Result<Vec<VersionedEvent<A>>, JournalError> {

Files reviewed: 12/12 changed files
Comments generated: 3
Review effort level: Lite

Copilot

🟡 Changes recommended

There are a couple of concrete issues to address (a hot-path unnecessary genesis clone in MemoryJournal::stream_mut, and a rustdoc/API contract mismatch for truncate_before error behavior).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

app/crates/ironstate-journal/src/memory.rs:122

stream_mut eagerly builds a genesis snapshot (cloning self.genesis) even when the stream already exists. This adds avoidable per-append cloning overhead on hot paths; only construct the genesis snapshot in the vacant-entry case.

    fn stream_mut(&mut self, id: &StreamId) -> &mut Stream<A> {
        let genesis = self.genesis_snapshot();
        self.streams
            .entry(id.clone())
            .or_insert_with(|| Stream::new(genesis))
    }

Files reviewed: 12/12 changed files
Comments generated: 1
Review effort level: Lite

Copilot

🟡 Changes recommended

A few newly added/updated API docs contain inaccuracies/invalid snippets (notably around retention read guidance and truncate_before error behavior) that should be corrected before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

app/crates/ironstate-journal/src/journal.rs:344

RetainableJournal::truncate_before can also return JournalError::UnknownSeq (e.g., unknown stream, Seq(0), or at past head + 1), but the rustdoc only mentions NoSnapshotForTruncation and Storage. Documenting UnknownSeq here makes the API contract accurate for callers.

    /// Truncation must be refused unless a snapshot at or after `at - 1`
    /// exists, since replay resumes from the newest snapshot and would
    /// otherwise need records that are gone. Sequence numbers of the *retained*
    /// records never change: a `Seq` is an identity, and subscriptions hold

app/crates/ironstate-journal/src/contract.rs:484

This comment likely meant to reference property 9 (out-of-range addressing). As written it says “property 2 and property 10 cannot see”, but this code is inside property 10 itself, so that’s self-referential and confusing when correlating test output to the numbered properties.

    // Positions stay total and monotonic over what remains, and everything below
    // the horizon is now out of range — the new failure mode retention adds,
    // which property 2 and property 10 cannot see on an untruncated journal.

Files reviewed: 12/12 changed files
Comments generated: 1
Review effort level: Lite

Follows the renumbering in the parent commit: out-of-range addressing took the free number 9, so truncation-preserves-resume becomes 10 rather than leaving a hole an adapter author would have to ask about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Review flagged an `expect` inside `append_in`, a public trait method. The crate's standing invariant is that a fallible operation returns a typed error rather than panicking, and "the invariant holds one line above" is a weaker guarantee than not needing one. - `append_in` computes the new head from `truncated + records.len()`, the same arithmetic `Stream::head()` does, instead of unwrapping its `Option`. - `stream_mut` uses the entry API rather than insert-then-`expect`. - `truncate_before`, also a public trait method, re-fetches mutably with a `let ... else` returning the same `UnknownSeq` its read-only checks would have, instead of asserting the lookup cannot fail. `MemoryJournal` now contains no `expect` or `unwrap` at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…wnSeq at a real API Two review notes on public surface. **`VersionedEvent` gained a field this release**, which is breaking for any adapter constructing or exhaustively destructuring it — and adapters must construct it, in `events_since`. Rather than spend that break silently, the struct is now `#[non_exhaustive]` with a `VersionedEvent::new` constructor. The next field is then additive for every adapter instead of breaking. This is the release to do it in: 0.2.0 already breaks the trait, and the alternative is paying the same cost again later. Making it `#[non_exhaustive]` immediately broke the three out-of-crate construction sites, which is the mechanism proving the constructor was needed; all three now go through it. **`JournalError::UnknownSeq` pointed at a "snapshot horizon"**, which is a phrase rather than an API. It now names `head(stream)` for the upper bound and `RetainableJournal::retained_from(stream)` for the lower — the two calls that actually answer the question, the second being the one retention introduces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…order Three more review findings. **`truncate_before` accepted `Seq(0)`.** `at` names the first record to keep, and genesis is not a record — so the call reported success while `retained_from` still said `Seq(1)`, having kept nothing "from Seq(0) onward" because there is no such thing. Now refused. **A snapshot recorded past the head could authorise a truncation.** The safety check took the newest snapshot by `Seq` over *all* stored snapshots, so a bogus one beyond the head satisfied it — vouching for a truncation no real base covered. `latest_snapshot_at` now ignores snapshots above the head, since replay could never start from one anyway. Both cases get a regression test. **`journal_contract_test!` only matched `forkable, retainable`.** The markers name capabilities, which are a set rather than a sequence, so the reversed order failing with a macro error was a pure footgun. A second arm forwards it. Also corrects the `events_since` comment: it claimed `None` means "from this stream's start", which reads as the horizon. `None` is `after = Seq(0)` — genesis — so on a truncated stream it is itself below the horizon and refused. The comment now says that, and names the call that does read everything retained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… comment Three review notes, all on clarity rather than behaviour. A test comment said `None` means "from this stream's start, which is now the horizon" while the assertion directly below expects `UnknownSeq { at: Seq(0) }`. The assertion was right: `None` is `after = Seq(0)` — genesis — so after truncation it is itself below the horizon. The comment now says that. `RetainableJournal::streams()` promised nothing about order, and a test was comparing the returned `Vec` directly. `MemoryJournal` happens to be backed by a `BTreeMap`, so its order is in fact stable — but that is an implementation detail no adapter owes, and the test's intent is that the *set* of streams is unchanged. The rustdoc now says the order is unspecified, and the test sorts before comparing. The semver consequences of `VersionedEvent` gaining a required field and `#[non_exhaustive]` are now stated in the PR description rather than left under an "additive" summary that stopped being true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`truncate_before`'s rustdoc listed only `NoSnapshotForTruncation` and `Storage`, but it also returns `UnknownSeq` for a `Seq(0)` truncation point, for an `at` beyond one past the head, and for an unknown stream. A caller cannot handle deliberately what the contract does not mention. Checking the rest of the surface for the same omission found two more, both introduced by this stack: - `fork` gained `NoBaseForFork`, and its refusal below the retained horizon, neither of which were listed. - `events_since` gained a refusal when read from below the horizon — including `None`, which means genesis — and still claimed only `Storage`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The comment told callers to pass `Some(Seq(retained_from - 1))`, which is not valid Rust twice over: `retained_from` is a method, and `Seq` has no `Sub`. It now shows the real expression — `events_since(stream, Some(Seq(journal.retained_from(stream).0 - 1)))` — and notes that it is `Seq(0)` on an untruncated stream, so it degenerates to the same read as `None`. That last claim is now a test rather than an assertion in prose: the two agree before truncation, and afterwards `None` is refused while the horizon expression still returns exactly the retained records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…t behind Three things this PR needed and did not have. **The version was already published.** 0.2.0 went to crates.io when the reshape merged, so this PR's breaking changes — `VersionedEvent` gaining a required field and `#[non_exhaustive]`, `streams` moving to `RetainableJournal` — would have shipped nothing: `release.yml` publishes only what is ahead of crates.io, and 0.2.0 is not ahead of 0.2.0. Bumped to 0.3.0, with the workspace requirement following. **0.2.0 had no changelog entry at all.** It was published by version-ahead detection rather than a release-plz PR, so nothing generated one. Recorded now, after the fact and marked as such, because a published version with no record of what changed is worse than a late entry. **The README documented the pre-reshape API.** It is the front page on crates.io and docs.rs for a release that has been live since the reshape merged: `execute`/`resume` without a `StreamId`, `fork` listed as a core operation, "seven-property conformance suite", and no mention of either headline feature. It now shows the real signatures, introduces streams, has a section on appending inside the caller's transaction — including why `execute_in` does not evolve the aggregate — and names the two opt-in capabilities. Also refuses `truncate_before` at or below an existing horizon, per review: those records are gone, so the call could not keep anything "from `at` onward" and returned `Ok(())` while `retained_from` stayed put. Truncating to exactly the current horizon remains an honest no-op. Regression test added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The Journal::events_since rustdoc currently misstates when None/genesis becomes out-of-range under retention, which can mislead adapter/caller behavior and should be corrected before release.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

app/crates/ironstate-journal/src/journal.rs:293

The events_since rustdoc currently says UnknownSeq applies when after is below the retained horizon “including None”, but None/Seq(0) is only out of range once a stream has actually been truncated. The actionable contract is that after must not refer to history that is no longer retained; callers of retainable journals can compute an after that means “everything still retained” using retained_from(stream) - 1.

    ///
    /// Returns [`JournalError::UnknownSeq`] if `after` is below the stream's
    /// retained horizon — including `None`, which means genesis — since the
    /// result would otherwise have a silent gap in it. Returns
    /// [`JournalError::Storage`] if the underlying store failed.

app/crates/ironstate-journal/src/contract.rs:519

The new property-10 entropy-position equality assertion message is missing the [proven] label used by other contract failures in this module.

    assert_eq!(
        entropy_after.draws(),
        entropy_before.draws(),
        "property 10: truncation moved the resume entropy position, case {case}",
    );

app/crates/ironstate-journal/src/contract.rs:524

The new property-10 retained_from assertion message is missing the [proven] label used by other contract failures in this module.

    assert_eq!(
        journal.retained_from(stream),
        at,
        "property 10: retained_from must report the new horizon, case {case}",
    );

Files reviewed: 16/17 changed files
Comments generated: 1
Review effort level: Lite

AGENTS.md is unambiguous — "every analysis/test-macro claim is labeled `[proven]` or `[sampled]`. No unlabeled claims" — and the conformance suite was following it about half the time. Properties 2, 7, 8 and 9 carried `[proven]`; 1, 3, 4, 5 and 6 carried nothing, and property 10 was split down the middle, three labelled and three not. Review caught the property 10 half. The rest is the same defect with an older date, so all 26 assertions now carry the label. Failure output is classifiable by grep again, which is the point of having a vocabulary. One thing this does not settle, deliberately: whether a suite driven by generated command streams should say `[proven]` at all, rather than `[sampled]`. The checks that loop over every `Seq` in a journal are exhaustive *for that journal*, but the journal itself was sampled — so the honest label arguably differs per assertion, and for some of them it is not `[proven]`. That question is about the crate's honesty vocabulary and deserves deciding on its own rather than inside a retention PR; consistency first, correctness of the choice second. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

There is a verified performance regression in MemoryJournal::stream_mut (unconditional genesis cloning) and a documented contract mismatch in Journal::events_since rustdoc that should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 16/17 changed files
Comments generated: 2
Review effort level: Lite

Two review findings. `stream_mut` hoisted `genesis_snapshot()` above the entry lookup, so every append cloned the whole aggregate genesis state — only to discard it when the stream already existed. That came from the earlier refactor away from insert-then-`expect`; the entry API was right, the eager clone was not. Destructuring `self` splits the borrow so the clone can happen inside the vacant arm, where it belongs: once per stream rather than once per append. The `events_since` rustdoc overstated the refusal. It read as though `None` is refused whenever it is below the horizon, but a stream with no history is not below anything — reads from it succeed with an empty list, and the conformance suite depends on exactly that: property 8 calls `events_since(untouched, None)` on a stream nothing has touched. The doc now separates the two cases: a stale mark on a truncated stream is refused; an empty stream is not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

There are confirmed correctness/documentation issues (contract property 10 can silently skip failures, events_since docs don’t precisely match the allowed horizon boundary, and MemoryJournal::latest_snapshot can select out-of-range snapshots affecting resume).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

app/crates/ironstate-journal/src/contract.rs:463

property_10_truncation_preserves_resume returns early if the pre-truncation resume fails, which can hide failures when run_contract_retainable is run on its own. Since the function is described as a contract property, it should fail loudly (with a property-numbered message) if resume unexpectedly errors.

    let Some(head) = journal.head(stream) else {
        return;
    };
    let Ok((before, entropy_before)) = resume::<A, J>(journal, stream, seed) else {
        return;
    };

app/crates/ironstate-journal/src/journal.rs:291

Journal::events_since docs are a bit inconsistent with the actual retention semantics: None is “from genesis” (after = Seq(0)), and on journals that truncate it can be refused; additionally, adapters (and MemoryJournal) intentionally allow after == retained_from(stream) - 1 as the correct way to read “everything still retained”. Clarifying the docs avoids implying None means “from whatever is still retained”, and avoids overstating the UnknownSeq lower bound.

    ///
    /// # Errors
    ///
    /// Returns [`JournalError::UnknownSeq`] if `after` is below the stream's
    /// retained horizon, since the result would otherwise have a silent gap in

Files reviewed: 16/17 changed files
Comments generated: 1
Review effort level: Lite

`latest_snapshot` took the highest-`Seq` snapshot without regard for whether the stream actually reaches that far, and `resume` uses it as its replay base — so a snapshot recorded beyond the head was handed back verbatim as the resumed aggregate. Wrong state, returned as `Ok`, which is the failure mode this crate exists to make impossible. Verified before and after: with a bogus snapshot at `Seq(1000)` on a six-record stream, `resume` returned `total: 9999` instead of the true `28`. The regression test fails without the fix. This is the same defect I fixed one call site earlier: `latest_snapshot_at` already ignores out-of-range snapshots so one cannot authorise a truncation. Filtering there and not here meant a snapshot too bogus to permit truncation was still trusted to reconstruct state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

🔵 Needs a closer look

MemoryJournal::fork currently accepts Seq(0) as a fork point (including for missing streams), conflicting with the documented “below/at horizon is UnknownSeq” semantics and producing a fork whose head() cannot equal the fork point.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

app/crates/ironstate-journal/src/memory.rs:272

MemoryJournal::fork currently treats Seq(0) as a valid fork point (including returning Ok for an unknown stream), but Seq(0) is genesis rather than a record sequence and the ForkableJournal docs say at at/below the retained horizon should be UnknownSeq. Allowing Seq(0) also produces a branch whose head() cannot equal the fork point (it becomes None). Reject Seq(0) consistently and remove the special-case Ok path for missing streams.

    }
}

impl<A: AggregateRules + Clone> ForkableJournal<A> for MemoryJournal<A> {
    fn fork(&self, stream: &StreamId, at: Seq) -> Result<Self, JournalError> {

Files reviewed: 16/17 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🔵 Needs a closer look

It introduces a new public capability trait plus nontrivial retention/contract semantics in core journaling code, warranting final human verification despite strong test coverage.

Review details

Files reviewed: 16/17 changed files
Comments generated: 0 new
Review effort level: Lite

Stacked on #62 (it uses `VersionedEvent::new`, which that PR adds). The seam a journal exposes for the caller's transaction — `Journal::Tx`, `execute_in`, `Pending` — is the reason the trait was reshaped, and it had **no runnable example**. An adopter could only learn it from `ironstate-journal/tests/transactional.rs`, which isn't indexed as an example and isn't where anyone looks. Every other tier has one: `ledger`, `async-store`, `catalog-ctx`. ## The domain does the arguing An order ships. Three things must become true at once: - the `Shipped` event is in the log, - the order's read-model row says `shipped`, - a "send the shipment confirmation" job is on the queue. If the append commits and the job doesn't, the customer is never told their order shipped, and nothing in the system knows. **That isn't a failure you can retry your way out of afterwards** — the information that it needed doing is gone. Which is why it's one transaction and not a queue publish. Stating it in a domain rather than abstractly matters here: "atomicity" as a word convinces nobody to restructure their write path. ## What it shows ```rust let mut tx = store.begin(); let pending = execute_in(&mut journal, &mut tx, &stream, &order, &cmd, &mut ctx)?; tx.upsert_read_model(id, OrderRow::shipped()); // the caller's own writes, tx.enqueue(Job::ShipmentConfirmation { id }); // in the same transaction store.commit(tx); let seq = pending.commit(&mut order); ``` `Store` is a few dozen lines of in-memory tables standing in for SQLite or Postgres. The point isn't the storage — it's that `Transaction` is the *caller's* type, the journal writes into it rather than around it, and nothing is visible until `commit`. Dropping it is the rollback, and needs no code. ## Four tests, not one - a rolled-back transaction leaves no event, no projection, no queued job **and an un-evolved aggregate** — that last one being the half a journal-only check would miss; - a commit lands all four; - a retry after a rollback behaves like a first attempt — no sequence consumed, the job queued exactly once; - staged writes are invisible to the journal's own reads until commit, which is *why* `execute_in` is one call per transaction. ## The README covers the two traps Why `execute_in` deliberately does not evolve the aggregate (a rollback would otherwise leave it silently ahead of the durable log — the failure the seam exists to prevent, reintroduced one layer up), and the one-call-per-transaction constraint, since `head`/`entropy_pos` take no `tx` and see committed state only. ## Indexing All three places AGENTS.md requires: the examples table, the root README, and `docs/testing.md`. It stays out of the guide, like the other adoption recipes — the guide's arc is core → aggregate → journal, and this is a recipe for an integration rather than a step in learning the family. ## Verification `cargo run -p outbox` prints the store before, after a rollback, and after a commit. `make check`, `make msrv`, `make doc`, `make wasm`, `make deny` all pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

laconc requested a lite review from Copilot August 29, 2026 04:28

Copilot started reviewing on behalf of laconc August 29, 2026 04:29 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/src/memory.rs Outdated

Comment thread app/crates/ironstate-journal/src/journal.rs

Comment thread app/crates/ironstate-journal/src/journal.rs

laconc force-pushed the feat/journal-retention branch 2 times, most recently from f454cbe to f54fcc5 Compare August 29, 2026 15:07

Copilot AI review requested due to automatic review settings August 29, 2026 15:13

laconc force-pushed the feat/journal-retention branch from f54fcc5 to 5b16393 Compare August 29, 2026 15:13

Copilot started reviewing on behalf of laconc August 29, 2026 15:14 View session

laconc force-pushed the feat/journal-retention branch from 5b16393 to 6bc00aa Compare August 29, 2026 15:15

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/src/journal.rs

Comment thread app/crates/ironstate-journal/src/journal.rs Outdated

Comment thread app/crates/ironstate-journal/src/contract.rs

Copilot AI review requested due to automatic review settings August 29, 2026 15:19

laconc force-pushed the feat/journal-retention branch from 21262da to 86b3611 Compare August 29, 2026 15:19

Copilot started reviewing on behalf of laconc August 29, 2026 15:19 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/src/memory.rs

Comment thread app/crates/ironstate-journal/src/memory.rs Outdated

Comment thread app/crates/ironstate-journal/src/macros.rs

Copilot AI review requested due to automatic review settings August 29, 2026 15:26

laconc force-pushed the feat/journal-retention branch from 86b3611 to 7cc61ab Compare August 29, 2026 15:26

Copilot started reviewing on behalf of laconc August 29, 2026 15:27 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/tests/retention.rs Outdated

Comment thread app/crates/ironstate-journal/src/journal.rs

Comment thread app/crates/ironstate-journal/tests/retention.rs Outdated

Copilot AI review requested due to automatic review settings August 29, 2026 15:30

Copilot started reviewing on behalf of laconc August 29, 2026 15:31 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/src/journal.rs

Copilot AI review requested due to automatic review settings August 29, 2026 15:34

Copilot started reviewing on behalf of laconc August 29, 2026 15:35 View session

Base automatically changed from feat/journal-core-reshape to main August 29, 2026 15:35

laconc force-pushed the feat/journal-retention branch from 807a9d4 to 98001f4 Compare August 29, 2026 15:35

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/src/memory.rs Outdated

Copilot AI review requested due to automatic review settings August 29, 2026 15:40

laconc and others added 8 commits August 29, 2026 11:49

Copilot started reviewing on behalf of laconc August 29, 2026 15:50 View session

laconc force-pushed the feat/journal-retention branch from b80a8aa to 0c4d9cb Compare August 29, 2026 15:50

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/src/contract.rs

Copilot AI review requested due to automatic review settings August 29, 2026 15:54

Copilot started reviewing on behalf of laconc August 29, 2026 15:54 View session

laconc mentioned this pull request Aug 29, 2026

docs(examples): add outbox, the transactional-outbox recipe #66

Merged

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/src/memory.rs Outdated

Comment thread app/crates/ironstate-journal/src/journal.rs Outdated

Copilot AI review requested due to automatic review settings August 29, 2026 15:58

Copilot started reviewing on behalf of laconc August 29, 2026 15:58 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread app/crates/ironstate-journal/src/memory.rs

Copilot AI review requested due to automatic review settings August 29, 2026 16:03

Copilot started reviewing on behalf of laconc August 29, 2026 16:03 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 29, 2026 16:08

Copilot started reviewing on behalf of laconc August 29, 2026 16:08 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

laconc merged commit 025291d into main Aug 29, 2026
10 checks passed

laconc deleted the feat/journal-retention branch August 29, 2026 17:53

Uh oh!

Conversation

laconc commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why a supertrait rather than part of Journal

The two things it has to get right

Contract property 10

Also: the hash-chain proposal is deferred, on purpose

Verification

Uh oh!

laconc commented Aug 29, 2026

Update: review fixes pushed (90f11fb, 20ccaef)

Truncation × entropy bookkeeping — the worst of it

Conformance — two self-inflicted coverage regressions

Keying

Documented rather than fixed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

laconc commented Aug 29, 2026 •

edited

Loading

Why a supertrait rather than part of `Journal`

Update: review fixes pushed (`90f11fb`, `20ccaef`)