Implement retention namespace generation transitions - #78
Merged
Conversation
reject_trailing_bytes reported LengthChanged.observed as the expected length plus the single byte that detected the overrun, so a 6-byte stage declared at 3 bytes was reported as 4. Two unreachable fallback arms reported the expected length itself as the observation. The three disagreement sites now report the file's metadata length through one helper, so observed names what the filesystem holds. The pinned trailing- bytes law expects observed: 6 for the 6-byte fixture; it failed before this change and passes now. Self-review finding A8 (P3). Refs #78
FilesystemPlatformAdmissionError gained MigrationRecord and RootIdentityChanged on this branch, and every future admission boundary (recovery, GC) will add a refusal of its own. Without #[non_exhaustive] each addition would be a breaking change for any caller that matched the enum exhaustively. Inside the crate only matches! uses exist, so nothing changes here. A source contract in tests/version_two_admission_contract.rs pins the attribute; it failed before this change and passes now. Self-review finding X1 (P3). Refs #78
The formats registry described keep.segment-store/v2 as "retention transition implementation planned", and the version-two overview said no version-2 writer was available and version 1 remained the only admitted production store. Both predate the migration authority, FilesystemVersionTwoAdmission, and FilesystemRetentionPublicationAuthority on this branch. Both pages now name what is implemented with executable evidence and what remains planned in issue #19, and the overview points at the requirements ledger as the authority on which requirements are proven. Nonclaims in retention.md and requirements.md are unchanged. Self-review finding X2 (P3). Refs #78
The retention FIFO laws fell back to spawning mkfifo(1) on hosts where rustix
compiles mknodat out. A spawned child briefly holds copies of every open
descriptor in the process, and a flock lives as long as any copy of its
descriptor, so while one FIFO law spawned, another law's writer lock stayed
held across its drop-and-reopen and refused with WriterLock { source: Busy }.
Measured locally: 3 of 6 parallel lib runs failed, 0 of 4 single-threaded,
every failure a FIFO law.
The fallback is deleted and the module is compiled only on Linux, where CI
runs it through mknodat. A contract test walks src/ and refuses any module
that names process::Command or Command::new; it failed before this change and
passes now. No production path ever spawned a process.
Self-review finding R18 (P4), pulled forward because it explained the flake.
Refs #78
Owner
Author
Self-Code Review — resolution progress, 21 of 37Continuing the approved order from the court-date comment. Same discipline as the first report: one Red → Green → Commit per finding under all gates.
Flake root cause. The intermittent Remaining: 16 findings — D1 (six commits), D2, R13, R14+R16+R22+R23, R17, R19, R20+X3, R21, R24. Next push after slice 30 with the final Activity Summary and a re-review request. |
…stage The retention stage, the migration fixed-record stage, the retention current-state reader, and the version-two record reader each carried their own copy of the same primitive: open read-only without following links and without blocking, check kind and length, read exactly the expected bytes, refuse trailing bytes, and for stages reverify device and inode identity on both sides of the read. Four copies of a security-relevant read path drift independently; the FIFO refusal, for one, had to be added to each. filesystem_exact_record now owns EntryIdentity, a typed ExactRecordRefusal, verify_named, require_absent, and link_without_replacement; the bounded readers arrive with their consumers in later commits, since deny(warnings) refuses a primitive with no consumer. Refusals carry no protocol vocabulary; each consumer maps them onto its own exact messages or typed variants, so no observable refusal changes. The retention stage is ported first, its messages preserved byte for byte. Two laws pin the primitives (different bytes and a byte-equal substitute refuse; absence and no-replacement linking hold), and a contract test keeps ported modules from reopening records themselves. Self-review finding D1 (P3), commit 1 of 6. Refs #78
…-record module The migration fixed-record stage carried the second copy of the stage primitives: its own device-and-inode identity type, its own no-follow non-blocking exact read with double metadata reverification, its own absence check, and its own no-replacement hard link. It now consumes filesystem_exact_record like the retention stage, mapping each shared refusal onto its existing messages byte for byte, so no observable refusal changes. The module is added to the exact-record consumer contract, which refuses any reimplementation of the primitives. The migration laws and the crash matrix's migration paths cover the port. Self-review finding D1 (P3), commit 2 of 6. Refs #78
…cord module The retention current-state reader carried the third copy of the bounded read: open without following links or blocking, refuse a non-regular entry or a length disagreement, read exactly the declared bytes, refuse trailing bytes. The head, manifest, committed-root, and predecessor-root reads all go through it, as does the catalog-head binding. filesystem_exact_record gains read_exact_optional and the KindOrLength refusal, pinned by two laws (absent reads as None while an exact record reads its bytes; a short file or a directory refuses before any byte is read). The retention reader is now a wrapper that maps shared refusals onto RecordLengthOverflow, RecordTrailingBytes, and RecordKindOrLength, so every typed refusal callers already downcast to is unchanged. Both stage mappers fold the new variant into their kind-or-length message. The module joins the consumer contract. Self-review finding D1 (P3), commit 3 of 6. Refs #78
…dule The version-two record reader carried the fourth copy of the bounded read for FORMAT, migration.intent, and migration.receipt. It now consumes filesystem_exact_record's new read_exact_regular, which keeps absence as the filesystem's own NotFound error because those records are required, and maps each shared refusal onto VersionTwoRecordRefusal by record name: LengthOverflow, KindOrLength, and TrailingBytes are unchanged for callers. One law pins the required read (an exact record reads its bytes; an absent one is NotFound), and the module joins the consumer contract, so all four readers and stages now share one no-follow, non-blocking, exact-length implementation. Self-review finding D1 (P3), commit 4 of 6. Refs #78
…rd module The version-one segment and catalog publishers linked their sealed stages into the immutable pools through filesystem_catalog_artifact's own copy of the no-replacement hard link. The retention and migration stages already use the shared primitive, so the copy is deleted and both publishers call filesystem_exact_record::link_without_replacement directly. Behaviour is identical: an existing target is left for post-link verification, never replaced. The crash matrix's segment and catalog publication protocols cover the repoint. Self-review finding D1 (P3), commit 5 of 6. Refs #78
…ryIdentity The recovery namespace, the migration inventory directory, and the migration namespace directory each declared an identical private DirectoryIdentity (device and inode read from a pinned Dir) to detect a replaced protocol directory between phases. The shared exact-record module's EntryIdentity is the same pair, so it gains of_directory and the three copies are deleted; every comparison site is unchanged in meaning. Recovery and migration laws and the crash matrix cover the identity checks. Self-review finding D1 (P3), commit 6 of 6. Refs #78
The version-1 physical pool names and the version-2 retention pool and namespace names each carried an identical private DigestHex renderer for the 64-lowercase-hex digest component that namespace and pool admission later parse. Two copies of the emitter for one on-disk grammar can drift. adapters::digest_hex now owns the renderer and both pool-name modules consume it. Filename output is unchanged; the golden worldline, conformance, and retention namespace laws pin the grammar. Self-review finding D2 (P3). Refs #78
…espace link_root and synchronize_root_namespace receive the candidate root from the orchestrator but used whatever namespace directory the attempt held, so a root from another namespace handed to either phase would have been linked into, or synchronized as, the admitted directory. The attempt now records the admitted namespace's name beside its capability, and both phases require the root they receive to name exactly that namespace, refusing with the new RetentionCurrentStateRefusal::AttemptNamespaceDisagreed otherwise. A law drives synchronize_root_namespace with a root from a second namespace after admission; it failed before this change and passes now. The Display impl is split into a short fmt over a const message table so it stays under clippy's line ceiling; every message is unchanged. Self-review finding R13 (P4). Refs #78
When no retention head is published and the preparation carries no forward publication (the byte-identical already-committed shape), the initial-path check refused as StaleCommittedRetry, a name that means "another successor is current". Nothing is current in that state; the retry claims a commit that cannot have happened. The refusal is now CommittedRetryOverAbsentHead, with its own description. The successor-over-absent-head law now downcasts to the refusal it actually receives, ExpectedCurrentOverAbsentHead, instead of asserting only the error kind. Of the grouped findings, R14 (the authority doc listing the pinned root), R16 (the LivenessGeneration import), and R22 (the stale identity-probe comment) were resolved by the PublicationAttempt and identity-policy commits earlier in this pass; this commit closes the group. Self-review findings R14, R16, R22, R23 (P4). Refs #78
… onto it Every filesystem law ended with a manual drop(authority); sandbox.remove()? pair, and a law that returned early on a failed assertion left its sandbox behind in the scratch root. Two test modules also carried their own private TestDirectory with a different scratch location. The shared sandbox now removes itself on drop; remove(self) stays for laws that want to observe the removal error. A law pins the behaviour (a dropped sandbox no longer exists); it failed before this change and passes now. The recovery-stage materialization and writer-lock laws use the shared sandbox, and the 43 manual teardown lines in the retention laws are gone; locals drop in reverse declaration order, so the authority releases its handles before the sandbox removes the directory. Self-review finding R17 (P4). Refs #78
tests/version_two_admission_contract.rs pinned the exact text of two function signatures to show that retention publication accepts only version-two authority and the version-one publishers only version-one authority. A signature contract breaks on any harmless reformatting and proves nothing the compiler does not already enforce. The doc of FilesystemRetentionPublicationAuthority::open now carries a compile_fail doctest that hands it a FilesystemPlatformAdmission; the doctest passes only while that call fails to type-check, which is the actual boundary. The source contract keeps per-file markers (each publisher names its own admission type and never the other) without pinning a signature. Self-review finding R19 (P4). Refs #78
…he census The namespace census carried its own copies of the pool-name grammar (is_pool_name, is_lower_hex, the suffixes and hex widths) apart from the module that emits those names, and classified every entry through a full metadata call when the directory listing already reports the file type. The predicates and constants now live in filesystem_retention_pool_name next to root(), manifest(), and namespace(), so emission and admission cannot drift. The census reads DirEntry::file_type, and its doc states the bound: one visit per roots entry, root pool, and manifest pool, two counters, no entry bytes, no link following, work proportional to the entry count under the 4,096-namespace ceiling. Behaviour and refusals are unchanged; the namespace, capacity, and expectation laws cover the census. Self-review findings R20 and X3 (P4). Refs #78
Two README lines ran past 80 columns because a Markdown URL cannot wrap. Both now use reference-style links with the targets collected at the end of the file; the rendered text and link targets are unchanged, and the phrases the README contract tests pin are untouched. Self-review finding R21 (P5). Refs #78
retention.md and recovery.md both sat at exactly the 300-line review threshold, so every later sentence had to be paid for by rewrapping an earlier one. Closure admission and the generation transition now live in retention-publication.md; the ordered one-way migration protocol and partial migration recovery live in migration-recovery.md. Each old page keeps its record grammars and routes to its new companion, and the version-two overview routes to all four. No sentence changed meaning; no section-anchor link pointed into the moved sections. The protocol contract pins the moved phrases on their new pages, reads the recovery pair as the one contract they state, and adds both new pages to the review-threshold list. The pages now stand at 242, 65, 242, and 67 lines. Self-review finding R24 (P5). Refs #78
Owner
Author
Self-Code Review — Activity Summary: 37 of 37 resolvedAll findings from the court-date comment are closed on
Two things worth a reviewer's eye.
Process notes. Gate runs on the local machine were repeatedly killed for memory during this pass; the chain was run one heavy step at a time at two build jobs, and no commit landed without every gate green. The @codex the branch is ready for your re-review at |
The exact-record law for byte-equal substitution deleted the record and wrote a new file at the same path. On APFS the new file always receives a fresh inode number, so the law passed locally; on ext4 the kernel may hand the new file the inode number just freed, the identity matched, and CI refused the law with "byte-equal substitute admitted" on abb5cf6. The law now creates the substitute beside the original and renames it over the entry. Two files that exist at the same time cannot share an inode, so the substitute's identity differs on every filesystem, which is the situation the primitive exists to refuse. The retention and migration substitution laws already used rename and passed on Linux; only this one was platform-bound. Refs #78
![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: abb5cf6591
ℹ️ 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".
is_pool_name accepted any 16 lowercase hex digits as the generation component, so an entry such as 0000000000000000-<digest>.root passed the census although root and liveness generations are positive by definition and the emitters can never produce that name. The census then treated an impossible coordinate as canonical and publication continued over it. The predicate now parses the generation component and requires a positive value, refusing zero as NoncanonicalPoolEntry like any other malformed name. A law plants a zero-generation root pool entry and requires the refusal; it failed before this change and passes now. Codex review, third pass (filesystem_retention_pool_name.rs). Refs #78
admit_capacity bounded only the creation of a new namespace directory: a candidate whose namespace already existed returned Ok without comparing the census against RetentionManifest::MAXIMUM_ENTRY_COUNT. A store that had somehow accumulated more than 4,096 namespace directories therefore kept publishing successors although no manifest can describe it. The census is now compared against the ceiling before the existing-namespace exception applies; an over-full store refuses as NamespaceCapacity for every candidate until recovery reduces it. A law fills the pool to 4,097 namespaces after publishing generation one and requires a successor in the existing namespace to refuse with an unchanged witness; it failed before this change and passes now. Exactly 4,096 still admits a successor (existing law). Codex review, third pass (filesystem_retention_namespace.rs). Refs #78
FilesystemStoreMigrationAuthority admitted the published version-one root shape but never looked inside staging. A store carrying current.seg or current.cat from an interrupted version-one publication could therefore publish its migration intent, marker, and receipt with that recovery evidence still in place; afterwards the version-one recovery constructors refuse the version-two markers and version-two stage recovery does not yet exist, so a recoverable crash state became stranded. verify_namespace now opens staging without following links and requires it to be empty before the intent is observed, refusing at the Namespace boundary with the reason. A law plants a retained stage and requires observe_intent to refuse with the version-one witness unchanged and no intent or intent stage written; it failed before this change and passes now. The retention fixtures, which migrate a clean store, are unaffected. Codex review, third pass (filesystem_migration_authority.rs). Refs #78
… construction FilesystemVersionTwoAdmission::reopen admitted every version-two directory and record, then returned a value holding only the writer lock. The publication authority reopened retention, roots, and manifests by name, so a directory renamed and replaced between reopen and open was opened without repeating admission; the advisory lock does not prevent such a replacement. The admission now retains the three capabilities it admitted and hands them to the authority through into_parts, so the authority never resolves those names itself. Current-state verification additionally requires each name to still resolve to the pinned directory, refusing with the new ProtocolDirectoryReplaced when the namespace no longer describes the admitted state, so a swapped-in directory is neither opened nor published into. A law publishes generation one, reopens, swaps retention out from under the admission, and requires open to succeed against the pinned directory and verification to refuse; it failed before this change and passes now. Codex review, second pass (filesystem_version_two_admission.rs), P1. Refs #78
require_current_catalog bound this store's catalog HEAD to the generation and
digest the closure was verified against, but it reopened only HEAD. A
preparation verified from a byte-identical store, or before this store lost
its catalog pool entry, still published: the head agreed while the catalog it
named was absent or corrupt, and the receipt cited evidence these pools could
not reproduce.
Publication now also reopens the catalog pool entry HEAD selects, bounded by
the head's declared length through the shared exact-record reader, decodes it,
and requires its generation and digest to equal the head's, refusing with
CatalogAbsent, CatalogRefused { source }, or CatalogChanged. A law deletes the
selected catalog after the snapshot and requires CatalogAbsent; it failed
before this change and passes now.
Closure-member segments are deliberately not re-read at publication: every
read authenticates them, and their re-verification under authority belongs to
retention recovery. requirements.md records the nonclaim.
Codex review, third pass (filesystem_retention_catalog.rs), P1.
Refs #78
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
1 participant
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.
Goal
Implement the accepted
keep.segment-store/v2retention and one-way migration campaign tracked by #19 without weakening Keep's exact-byte, fail-closed storage law.The affected invariant is that retention authority must be generation-checked, canonical, deterministic, bounded, and durable before any reader or writer may treat a version-2 state as admitted. This branch preserves every version-1 segment, catalog, and publication-head byte.
Plan
KEEP-CRASH-053..=073process-death evidence.Current status
This is a Draft-turned-ready PR for review of the landed slices; it does not
yet close #19. Fresh migration and fresh retention publication both have
production filesystem adapters.
FilesystemStoreMigrationAuthorityexecutesthe 21-phase forward migration;
FilesystemRetentionPublicationAuthorityexecutes the 17-phase forward retention publication against a completely
migrated version-2 root, for both initial and successor generations. Both stage
exclusively, verify device-and-inode identity at every transition, hard-link
immutable pool entries without replacement, and leave every admitted version-1
byte unchanged.
observe_currentreturns the published head and itscross-verified manifest; verification admits a successor only when the prepared
head names that manifest as its exact predecessor, returns an exact
already-committed candidate with zero mutation, and refuses a superseded one.
Version-1 reopen refuses a migrated root;
admit_version_twoandreopen_version_twoown the version-2 namespace boundary.Both adapters are intentionally one-shot forward writers, not restart engines.
A process that opens a retained stage prefix has no lawful continuation
implementation and refuses; the crash matrix covers
KEEP-CRASH-001..=035,not the retention boundaries
036..=052or migration boundaries053..=073.Version 1 therefore remains the only admitted restart-safe production store.
The README was rewritten as a front door (
855db515,d7518a8e,06072f44):it routes requirement status to the segment-store-v2 ledger instead of
restating it, and carries a Mermaid architecture diagram.
The branch contains 57 commits spanning the #19 campaign and is 0 commits
behind
origin/main; no rebase or history rewrite has been performed.Failure modes
Validation
cargo fmt --all --checkcargo check --workspace --all-targets --all-features --lockedcargo check --workspace --all-targets --no-default-features --lockedcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo clippy --workspace --all-targets --no-default-features --locked -- -D warningscargo test --workspace --all-features --lockedcargo test --workspace --all-features --release --lockedcargo test --workspace --doc --lockedcargo +1.96.0 check --workspace --all-targets --all-features --lockedcargo xtask golden-file-worldline-checkcargo xtask conformance-checkcargo xtask source-structure-checkcargo xtask durability-crash-matrixcargo xtask documentation-refusal-checkcargo xtask documentation-integrity-checkcargo deny checkcargo auditCompatibility, recovery, performance, and security
Alternatives rejected
Open questions
Peer review is most useful on partial-prefix recovery classification, retention publication ordering, and the immutable reader-fence aperture before those irreversible protocol slices are implemented.
Closes #19 only when every unchecked Plan item and the colocated requirements ledger are complete.