Define retention roots and GC liveness - #68
Conversation
Code Lawyer Activity Summary
All five were resolved one at a time with deterministic RED → GREEN contract cycles and separate commits. Final local evidence is fully green: 4/4 new laws in debug and release; all workspace, strict Clippy, doctest, MSRV, fuzz-crate, 105-case debug/release crash-matrix, documentation, whitespace, deny, and audit gates passed. @codex review please |
|
Warning Review limit reached
Next review available in: 6 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughChangesRetention and GC liveness contract
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 3
🤖 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 `@docs/adr/0009-retention-roots-release-and-gc-liveness.md`:
- Around line 96-102: Update the closure traversal and GC planning contract
described in the “Closure traversal” section to define implementation-enforced
hard ceilings for every caller-supplied resource limit, including roots, nodes,
depth, encoded bytes, and physical bytes inspected. Validate and reject any
request exceeding those ceilings before traversal or materialization begins,
while preserving the existing fail-closed behavior.
- Around line 79-94: Bind the exact RetentionRealizationProfile identity,
version, and digest into retained transition state, liveness snapshots, and
minimum receipt evidence. During revalidation and snapshot admission, require
the referenced profile to be available and exactly matching; reject unavailable
or mismatched profiles instead of resolving through the current registry entry.
Update the related namespace-generation and snapshot evidence paths
consistently.
- Around line 154-176: Revise the “State meanings” section to explicitly scope
Staged, Retained, and Released to candidate bytes or namespace generations, and
Orphaned and Collectible to physical material. Replace the “A path cannot move”
statement with a rule that these state transitions require the defined evidence
and cannot be inferred from existence, age, recent access, or absence from a
single catalog.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 04ae9b59-d76e-43e1-8e1d-4eb771334817
📒 Files selected for processing (4)
CHANGELOG.mddocs/adr/0009-retention-roots-release-and-gc-liveness.mddocs/adr/README.mdxtask/tests/retention_liveness_protocol_contract.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Runtime fuzz smoke
- GitHub Check: Documentation and workflow integrity
- GitHub Check: Rust quality gates
🧰 Additional context used
📓 Path-based instructions (5)
docs/adr/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Cross-subsystem decisions affecting identity, formats, durability, recovery, concurrency, GC, encryption, or public compatibility require a slugged ADR under
docs/adr/, never a bare number.
Files:
docs/adr/README.mddocs/adr/0009-retention-roots-release-and-gc-liveness.md
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Use stable, pinned Rust with edition 2024;
cargo fmt --checkand Clippy with-D warningsmust pass.
Files:
xtask/tests/retention_liveness_protocol_contract.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not useunwrap,expect,panic!,todo!,unimplemented!,dbg!, stdout/stderr printing, unchecked indexing, lossy casts, or unsafe code. Unsafe requires a dedicated crate with documented invariants, tests, and measured necessity.
Use checked arithmetic for externally influenced values andTryFrom/try_frominstead of potentially lossyasconversions.
Public functions must not take boolean parameters; use enums instead. Prefer typed newtypes for IDs, lengths, offsets, generations, and namespaces.
Parse and validate untrusted bytes before admitting them as trusted types; make illegal states unrepresentable where practical.
Prefer concrete types, synchronous core APIs, and traits only at real substitution boundaries; do not introduce async without demonstrated consumer need.
Do not let HashMap iteration order affect identity, serialization, tests, or behavior.
Use hexagonal architecture: domain and port modules own invariants and semantic capabilities, dependencies point inward, and core/ports must not import adapters, filesystems, networks, CLIs, runtimes, or application policy.
Keep codecs at ingress and egress boundaries. Ports exchange semantic or validated types, not serializer-owned JSON, CBOR, or wire values.
Keep files near 200 lines, review files over 300, and do not exceed 500; keep functions near 20 logical lines, review over 40, do not exceed 60; limit nesting to 3 and parameters to 5.
Do not create genericutils.rs,helpers.rs,common.rs,misc.rs,shared.rs,manager.rs,service.rs,types.rs, ormodels.rs; name files after the concept they own.
Keep items private by default; usepub(crate)unless external consumers require public visibility. Public validated types must have private fields and checked constructors, and consequential staged work, plans, commits, and results should use#[must_use].
UseOptionfor normal absence, typed errors by boundary, preserved error sources, and expected/observed state in...
Files:
xtask/tests/retention_liveness_protocol_contract.rs
**/*.{rs,json,cbor}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,json,cbor}: Identity, persistence, comparison, tests, and protocol behavior must be deterministic and independent of iteration order, host state, clocks, locale, and serializer defaults.
Boundary JSON and CBOR must use a named canonical profile with golden fixtures; reject duplicate fields and noncanonical identity-bearing encodings. Hash only typed, domain-separated canonical bytes.
Files:
xtask/tests/retention_liveness_protocol_contract.rs
**/*.{rs,bin}
📄 CodeRabbit inference engine (AGENTS.md)
Durable formats must define magic bytes, versioning, canonical encoding, explicit endianness, bounds, checksums, and golden fixtures; reject trailing bytes, duplicate fields, noncanonical encodings, overflow, invalid ordering, excessive depth, and unknown mandatory flags. Do not use arbitrary Serde or Rust struct output as a durable format.
Files:
xtask/tests/retention_liveness_protocol_contract.rs
🧠 Learnings (2)
📚 Learning: 2026-07-27T22:37:16.896Z
Learnt from: flyingrobots
Repo: flyingrobots/keep PR: 49
File: src/layout/record_length.rs:29-29
Timestamp: 2026-07-27T22:37:16.896Z
Learning: This repository targets Rust 1.96 (per `Cargo.toml` `rust-version` and `rust-toolchain.toml`). When writing or reviewing Rust code, only use APIs/language features stabilized in Rust 1.96 or earlier. Avoid using newer std/library APIs that wouldn’t be available on Rust 1.96 (e.g., you may rely on `u64::is_multiple_of` since it’s stabilized by 1.96).
Applied to files:
xtask/tests/retention_liveness_protocol_contract.rs
📚 Learning: 2026-07-29T05:54:58.524Z
Learnt from: flyingrobots
Repo: flyingrobots/keep PR: 63
File: xtask/src/golden_file_worldline/b3sum_oracle.rs:15-21
Timestamp: 2026-07-29T05:54:58.524Z
Learning: In the flyingrobots/keep Rust codebase, prefer fallible conversions using `TryFrom`/`try_from` (e.g., `u64::try_from(payload.len())`) instead of potentially lossy `as` casts. If the chosen target architecture makes conversion failure logically unreachable, still keep the `TryFrom`-based conversion per repository policy, and do not require fabricated negative-test cases solely to cover an unreachable defensive failure path.
Applied to files:
xtask/tests/retention_liveness_protocol_contract.rs
🔇 Additional comments (5)
docs/adr/0009-retention-roots-release-and-gc-liveness.md (1)
1-78: LGTM!Also applies to: 104-153, 177-192, 200-219, 228-240, 242-305
xtask/tests/retention_liveness_protocol_contract.rs (2)
1-2: LGTM!Also applies to: 5-106
3-3: 📐 Maintainability & Code QualityNo change needed here.
CI runs
cargo test --workspace --all-featuresfor both debug and release profiles, so thisrepository-tasks-gated test module is enabled by the coverage path.docs/adr/README.md (1)
63-63: LGTM!CHANGELOG.md (1)
306-311: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81231078d8
ℹ️ 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".
Review Remediation Activity Summary
Each finding was resolved in its own RED → GREEN commit. Consolidated verification passed: strict xtask Clippy; contract 4/4 in debug and release; source-structure and documentation-refusal checks; pinned @codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 327a77915b
ℹ️ 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".
Self-audit finding
@codex Please confirm this protocol-version correction during the replacement-head review. |
Additional self-audit finding
@codex Please verify that the replacement head makes receipt evidence as exact as the snapshot it proves. |
Activity Summary — replacement head
|
| Source | Priority | File | Finding | Commit | Outcome |
|---|---|---|---|---|---|
| Codex | P0 | ADR-0009 | GC could retire record spans inside immutable segments | bcb8efb |
Whole-segment liveness and compaction-successor law pinned |
| Codex | P0 | ADR-0009 | Segment unlink could precede catalog retirement | c5265ea |
Durable catalog successor now precedes every unlink |
| Codex | P0 | ADR-0009 | Recovery-protected artifacts lacked explicit disposition | 6c22f84 |
Durable finalize-or-retire receipt required |
| Codex | P0 | ADR-0009 | GC mutation lacked crash-state and retry protocol | 7453b40 |
Intent, ordered sync, receipt, and idempotent recovery defined |
| Codex | P1 | ADR-0009 | Retention artifacts conflicted with the exact v1 root shape | bd8dbb5 |
Versioned v2 namespace and one-way recovery-safe migration defined |
| Codex | P1 | ADR-0009 | Permanent namespace tombstones had no total bound | 6fca251 |
Fixed admission ceiling refuses before persistent staging |
| Codex | P1 | ADR-0009 | Snapshot roots and catalog were generation-only | f6b54d1 |
Full generation-plus-digest coordinates required |
| Codex | P1 | ADR-0009 | Historical retry could report current success | 55aa9ed |
Already-committed succeeds only while exact successor is current |
| Self | P1 | ADR-0009 | Namespace continuity was assigned to forbidden v1 state | 7a2fb0f |
Rule corrected and pinned to version 2 |
| Self | P1 | ADR-0009 | Retention receipts were weaker than snapshot evidence | a5f9dfe |
Root, manifest, and catalog receipt digests required |
Each item used an isolated RED → GREEN contract-test cycle and its own commit. Final local evidence: cargo fmt --all --check; strict xtask Clippy; focused contract 4/4 in debug and release; source-structure check; documentation-refusal check; pinned markdownlint-cli2 0.23.2, lychee 0.21.0, and actionlint 1.7.12 documentation-integrity check; branch and whole-history whitespace checks; no Python sources; ADR 493/500 lines. Worktree clean and branch exactly matches the pushed head.
@codex review please — confirm the exact replacement head a5f9dfea1f55dc623c3e6c684cef4f23b6c9c91c.
Self-audit finding — unpublished replacement slice
@codex Please include the permanent size guard in the replacement-head review. |
Activity Summary — second replacement head
|
| Source | Priority | Finding | Commit | RED | Outcome |
|---|---|---|---|---|---|
| Codex | P1 | Active readers had no observable GC safety-fence lifecycle | 356033a |
Contract omitted ReaderFence, acquisition order, lifetime, and drain law |
v2 readers take shared reader.lock before HEAD; GC takes writer authority then the exclusive reader lock and retains both through durable retirement |
| Self | P2 | Reader-fence prose crossed the 500-line hard maximum | 9e788bf |
Exact line-count test failed at 501 | Permanent <= 500 law added; redundant prose tightened; ADR is exactly 500 lines |
| Codex | P1 | A catalog successor could invalidate a currently retained closure | a1c2e56 |
Contract omitted retained-root validation during catalog publication | Candidate catalog now proves every current retained closure before HEAD replacement and binds the preserved manifest coordinate |
| Codex | P2 | Ordinary anchors were incorrectly claimed to retain physical representations | 9953c23 |
Contract omitted the representation expressiveness boundary | Specific representation requires a future typed extension; only a distinct LayoutId can use its own anchor |
Each issue used a deterministic RED → GREEN cycle and a separate commit. Final local evidence on 9953c23ca5067d01b845fbb02f214b78492b65ee: strict xtask Clippy; focused contract 5/5 in debug and release; source-structure check; documentation-refusal check; pinned documentation-integrity check; branch and whole-history whitespace; pure-Rust tracked-source inventory; 500/500 ADR size law. Worktree clean and branch equals the pushed head.
@codex review please — review this exact replacement head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9953c23ca5
ℹ️ 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".
| Before replacing `HEAD`, version-2 catalog publication holds that authority, | ||
| admits the current global retention manifest, and verifies every current | ||
| retained closure against the candidate catalog with its exact profile and | ||
| limits. Candidate publication refuses before mutation when closure evidence is |
There was a problem hiding this comment.
When a caller admits a root with limits below the protocol ceilings, this new catalog-publication check requires the candidate to verify that closure with its exact limits, but the durable namespace generation described at lines 111–114 stores only the profile coordinate and anchors. After restart there is therefore no authoritative value for publication to use: choosing current request/default limits can accept or reject the same catalog successor differently. Bind the admitted limits into each namespace generation and its digest so this validation is reproducible.
AGENTS.md reference: AGENTS.md:L38-L40
Useful? React with 👍 / 👎.
| Planning is observational and carries no mutation authority. A version-2 reader | ||
| takes a kernel-managed shared lock on persistent `reader.lock` before opening | ||
| `HEAD`. Its admitted snapshot owns a `ReaderFence` that retains that lock for |
There was a problem hiding this comment.
If the reader.lock directory entry is replaced after a reader acquires its shared lock but before GC opens the entry, the reader and GC can hold locks on different inodes; GC then obtains its supposedly exclusive fence and may unlink a segment still needed by that reader. Persistence and lock lifetime alone do not prevent this filesystem race, so both acquisition paths must open without following links and verify after acquisition that the locked device/inode is still the canonical admitted entry, refusing any drift.
AGENTS.md reference: AGENTS.md:L80-L81
Useful? React with 👍 / 👎.
| - the current catalog successor that names none of the candidates; | ||
| - the admitted segment-pool identity; and | ||
| - the exact candidate segment digests, lengths, and verification evidence in | ||
| canonical segment-digest order. |
There was a problem hiding this comment.
With repeated catalog or retention updates, old catalogs, namespace generations, and global manifests become unreachable, but this execution protocol authorizes only the segment-digest candidates recorded in the intent. Those immutable metadata artifacts are not segments and no other retirement transition is defined for them, so even a single bounded namespace can grow the store without bound until publication fails for lack of space. Add a bounded, crash-safe liveness and retirement protocol for superseded metadata artifacts, or explicitly include typed metadata retirement units in GC.
Useful? React with 👍 / 👎.
| Every namespace has an immutable `RootGeneration`. A transition supplies: | ||
|
|
||
| 1. the exact `RetentionNamespace`; | ||
| 2. an expected state of either absent or one exact `RootGeneration`; | ||
| 3. the complete candidate anchor set; and | ||
| 4. one exact `RetentionRealizationProfileId`; and | ||
| 5. explicit closure-admission limits. |
There was a problem hiding this comment.
When a client retries against a restored or forked store whose current namespace has the same RootGeneration but different canonical bytes, this request shape treats the states as equal because its expectation contains only the generation. The transition can then replace a root the client never observed and later permit collection of its closure; require the expected state to include the root's canonical digest (and compare the complete coordinate) rather than relying on generation alone.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
1 participant
Problem
M4 lacked an accepted authority model for retention roots, release, immutable liveness snapshots, and fail-closed garbage collection.
Invariant affected
Keep may collect physical material only when an exact immutable snapshot proves that no retained reconstruction anchor requires it.
Approach
Add ADR-0009, index it, record the unreleased design contract, and enforce its core laws with a repository-owned xtask test. The decision uses validated namespaces,
BlobId/LayoutIdanchors, bounded canonical closure, monotonic root and liveness generations, a nonzero deterministic realization profile, and shared exclusive writer authority for retention, publication, recovery, and GC execution.Alternatives rejected
Git refs, leases, reference counts, unversioned tracing, caller-supplied physical closure, and retaining every representation are rejected in the ADR.
Failure modes
Stale generations, missing or corrupt closure members, ambiguous physical realizations, zero witnesses, changed catalog or liveness views, and active safety fences all refuse rather than weaken liveness.
Tests
The new contract test passes 4/4 in debug and release. Full workspace checks, strict Clippy, debug/release tests, doctests, MSRV 1.96, fuzz-crate checks, both 105-case crash matrices, documentation refusal/integrity, whitespace, cargo-deny, and cargo-audit pass locally.
Benchmark impact
No runtime implementation changes; no benchmark delta.
Format/API compatibility
No shipped format or public API changes. ADR-0009 constrains the future #19 implementation and requires versioned canonical formats.
Recovery implications
Retention and GC execution share the existing exclusive store writer authority with catalog publication and recovery. Interrupted future publication must follow the durable-store recovery law.
Security implications
Receipts prove narrow physical claims only and expose no content, keys, or unbounded paths. Release makes no secure-erasure claim.
Closes #18.