3B2: native filesystem atomic adapter for the curation store - #139
Conversation
…ore (3B2) The 3B2 contract as failing tests against stub adapter bodies: 1. a missing store file is StoreOnDisk::Missing, an explicit typed absence, not a malformed error; 2. an existing malformed store is a typed CurationFsError::Decode, never empty; 3. a written store survives a reload (restart stand-in); 4. an injected failure before the replace leaves the old store byte-identical and removes the temp; 5. an injected failure during the replace is a typed error and keeps the old store; 6. the temp sibling never becomes the canonical store; 7. re-writing the same canonical store yields byte-identical output; 8. (unix) a write fsyncs the parent directory. The adapter lives in griff-cli, not core — std::fs stays out of the domain model. Core's only change is error plumbing: StoreValidationError / StoreDecodeError / StoreEncodeError now derive thiserror::Error so the adapter can wrap them as sources. load_store / write_store / atomic_write are stubs, so the seven behavioral tests fail; the missing-file test passes against the stub. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
…store (3B2) Implement the adapter so all seven behavioral contract tests pass (the eighth, the parent-dir fsync, runs on unix CI): - load_store: fs::read then decode_store; a NotFound is StoreOnDisk::Missing, any other read error is Io, malformed bytes are a typed Decode refusal. - write_store: encode_store (which validates) then an atomic write. - atomic_write: write a sibling temp, write_all + flush + sync_all, then rename it over the canonical path (atomic replace on both POSIX and Windows), then fsync the parent directory where the platform supports it. Any failure before the rename removes the temp and returns a typed error, leaving the previous store byte-identical; the temp only becomes the store via a completed rename. Parent-directory fsync is unix-only (std cannot fsync a directory handle on Windows, where the rename is the platform's durability barrier) — all such platform detail lives here, none in core. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: dc370e113a
ℹ️ 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".
| griff-swang = { path = "../swang" } | ||
| clap = { workspace = true } | ||
| serde_json = { workspace = true } | ||
| thiserror = { workspace = true } |
There was a problem hiding this comment.
This adds a normal dependency, but the committed Cargo.lock was not updated; I confirmed cargo check -p griff-cli --locked now fails with cannot update the lock file. Any consumer or CI path that builds with --locked will fail even though the manifest is otherwise valid, so the lockfile update needs to be committed with this change.
Useful? React with 👍 / 👎.
| if let Some(dir) = path.parent().filter(|d| !d.as_os_str().is_empty()) { | ||
| File::open(dir)?.sync_all()?; |
There was a problem hiding this comment.
. for bare relative store paths
When the caller passes a bare relative path like Path::new("curation.json"), Path::parent() is Some(""), so this filter skips the directory sync entirely. On Unix that means the common current-directory write path returns success without making the rename crash-durable, despite the adapter's durability contract; treat an empty parent as . before opening it.
Useful? React with 👍 / 👎.
Arbiter review: HOLD — three narrow corrective itemsThe adapter boundary and the core error-plumbing change are correct. CI is green on 1. Post-rename sync failure is currently misclassified
That contradicts the public error contract saying an I/O error leaves the previous store intact. More importantly, the caller cannot distinguish:
Add a typed variant such as: PostCommitDurability { source: io::Error }(or an equivalent name), returned only after a successful rename when parent sync fails. Add an injected failure after rename / at directory sync proving:
Do not attempt rollback after rename; that would create a second, less reliable transaction. 2. Relative paths skip Unix parent-directory fsyncFor Resolve an absent/empty parent to 3.
|
…eplace seam (3B2) Corrective for the PR #139 review. Three narrow gaps, as failing/added tests: 1. A directory-fsync failure AFTER a successful rename is currently reported as a pre-commit CurationFsError::Io, whose contract promises the old store survived — but the new bytes are already published. New variant PostCommitDurability { source } and a FailPoint::AfterReplace; the test wants that typed error, the new bytes on disk, the temp gone, and the error distinguishable from a pre-commit Io. 2. A bare relative store path (curation.json) has an empty parent, which the sync path filtered out, so the Unix directory fsync never ran. New parent_for_sync maps an empty parent to "."; the test pins curation.json -> ., dir/curation.json -> dir, /dir/curation.json -> /dir. Both new items are wired as scaffolding with deliberately-wrong bodies (AfterReplace falls through to a normal Ok; parent_for_sync keeps the empty parent), so the two tests fail. GREEN adds the post-commit path, the real replace_file seam (so FailPoint::DuringReplace exercises the actual fs::rename error branch, corrective #3), and fixes parent_for_sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
…replace seam (3B2) Resolve the three PR #139 correctives; all 9 curation_fs tests pass. 1. A directory-fsync failure after a successful rename is now CurationFsError::PostCommitDurability, distinct from a pre-commit Io: the new bytes are already published, there is no old store to fall back to, and no rollback — a second file transaction cannot undo the first. atomic_write splits pre-commit (Io, old store intact) from post-commit (durability warning) explicitly. 2. sync_parent_dir now derives its directory from parent_for_sync, which maps a bare relative filename's empty parent to ".", so a relative store path still fsyncs a real directory on Unix. 3. The atomic replace is extracted into replace_file(temp, canonical, fail), so FailPoint::DuringReplace returns its error from that seam and atomic_write handles it through the exact same branch as a real fs::rename failure — the OS-replace error path is now covered, not shadowed by an earlier early return. Docs narrow the durability guarantee: temp contents fsynced before publish; atomic replace on a supporting fs; directory entry fsynced on Unix; on Windows this std-only adapter does not fsync the directory handle, so the post-power-loss guarantee is not identical to Unix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
Corrective for the three gaps (RED
|
1 participant
What
The native filesystem backend for the curation store — step 3B2. The
domain model (3B1,
griff-core::curation_store) owns the wire format and doeszero I/O; this adapter reads and writes those bytes on a real filesystem,
durably and atomically, so a decision file survives a crash between
write()and the rename that publishes it.
New module
cli/src/curation_fs.rsingriff-cli— not in core, sostd::fsnever enters the domain model.API
load_store(&Path) -> Result<StoreOnDisk, CurationFsError>—StoreOnDiskisMissing(a typed absence the caller resolves into a fresh empty store) orLoaded(store). A missing file is never conflated with a malformed one.write_store(&Path, &CurationStoreV1) -> Result<(), CurationFsError>—encode_store(which validates) then an atomic write.The write is temp-then-rename: write a sibling temp →
write_all+flush+sync_all→ rename over the canonical path (atomic replace on POSIX and WindowsMoveFileExW) →fsyncthe parent directory where the platform supports it.Any failure before the rename removes the temp and returns a typed error,
leaving the previous store byte-identical.
Contract (all 10)
Missing, not malformed ✅Decoderefusal, never empty ✅fsyncwhere the platform supports it (unix; no-op on Windows,where std cannot fsync a directory handle) ✅
curation_store.rs✅Failure injection is a private test-only
FailPointseam; the public API has noinjection surface.
Core change (error plumbing only)
StoreValidationError/StoreDecodeError/StoreEncodeErrornow derivethiserror::Errorso the adapter can wrap them as#[source]s. No domain-modelbehavior changes.
Validation
cargo test -p griff-cli --lib— 8/8 (7curation_fson Windows; the unixparent-dir-fsync test runs on CI).
cargo test -p griff-core --lib— 275/275 (thiserror plumbing changes nothing).cargo clippy --workspace --all-targets— clean.cargo fmt --check— clean.griff-cli::missing_file_golden(English OS string ona RU-locale box) is unrelated.
3C (OPFS + cockpit flow) remains frozen.
🤖 Generated with Claude Code