Repository navigation
fix(id/currency_poc): one filesystem identity contract for FileIntentStore (#4578) - #4622
Conversation
…Store (#4578) Identity is the conversion_id in the record content; the canonical file {id:032x}.json is the only path the store writes. get(), unresolved(), and put() now share one directory scan grouped by content ID: - duplicate copies of one record collapse to one record, so recovery can never reconcile one conversion twice; - a record stored only under a non-canonical name is found by get(), so prepare()'s never-replace guard cannot be bypassed into writing a conflicting canonical twin; - conflicting records under one conversion_id fail closed: get()/put() error with a diagnostic naming the paths, and unresolved() skips the conversion and reports every copy through unreadable_entries(); - put() refuses a record that differs from the stored one in anything but state, and canonicalizes legitimate writes by removing the stray copies they supersede. Regression tests: duplicate copies reconcile exactly once, misnamed record blocks a conflicting prepare, misnamed-only record refuses a conflicting put and canonicalizes a matching one, conflicting copies fail closed and are reported.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 57 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 6f71aa4a9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let scan = self.scan()?; | ||
| let copies = scan.copies_for(record.conversion_id); |
There was a problem hiding this comment.
If conversion B is stored only under A's canonical filename, copies_for(A) excludes that file because its content identifies B. The new get(A) consequently returns None, so prepare(A) passes its never-replace guard and put(A) overwrites B's only write-ahead record at the rename. Previously, get(A) returned B and prepare(A) rejected the conflicting record. Check the canonical destination's ownership independently of the content-ID grouping and refuse to overwrite another conversion's record.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3d4db6: get() and put() now check the canonical path's occupant independently of the content scan (IntentScan::entry_at + canonical_occupant_error). If the canonical file for conversion A stores conversion B's record, both fail closed with a typed error naming both conversions — A never reads as absent, so prepare()'s never-replace guard cannot be bypassed, and B's only write-ahead record is never overwritten. Regression test: canonical_file_holding_another_conversions_record_blocks_get_and_put (occupant byte-identical after both refusals; get(9) and the listing still find the occupant by content identity).
| for stray in &stray_paths { | ||
| fs::remove_file(stray).map_err(|error| { |
There was a problem hiding this comment.
When a conversion has misnamed or duplicate copies, a state-advancing put() durably writes the new canonical record before deleting the old-state copies here. A crash between those operations, or a deletion failure, leaves records that differ only in state. IntentScan::distinct() then treats them as conflicting identities: subsequent get() and put() fail, and unresolved() skips the conversion indefinitely, preventing automatic recovery even though its legs are unambiguous. Make canonicalization crash-recoverable so an interrupted legitimate update does not create an unrecoverable identity conflict.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3d4db6: IntentScan::distinct() now resolves copies that differ ONLY in state to the most advanced state (new IntentState::precedence — Recorded < Submitted < Abandoned < Posted, documented on the method), while copies differing in any other field stay an ambiguous identity. The crash residue of a state-advancing put (new canonical written, superseded copies not yet removed) therefore stays readable and reconcilable: get() returns the advanced record, unresolved() lists the conversion once, and the next put() canonicalizes the residue away. Regression test: state_only_divergent_copies_resolve_to_the_most_advanced_state.
P1: a canonical file holding ANOTHER conversions record now fails get()/put() closed (canonical_occupant_error) instead of reading as absent — prepare()s never-replace guard can no longer be bypassed and the occupants only write-ahead record is never overwritten. P2: copies differing ONLY in state resolve deterministically to the most advanced state (IntentState::precedence) in IntentScan::distinct, so the residue of a state-advancing put interrupted between the canonical write and stray cleanup stays readable and reconcilable instead of bricking the conversion as an ambiguous identity. Regression tests cover both.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3d4db6a51
ℹ️ 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".
| Some(record) => { | ||
| if record.state.is_unresolved() { | ||
| records.push(record); |
There was a problem hiding this comment.
If conversion 1 is stored in stray.json while its canonical filename contains conversion 2, this listing returns conversion 1 even though the new canonical-occupant guard makes get(1) and put(1) fail. reconcile() then propagates that put() failure through ? in recovery.rs, preventing unrelated later conversions from recovering; under RecoveryPolicy::Resubmit, it can also submit conversion 1 before encountering this already-detectable persistence block. Apply the canonical-occupancy check during listing and skip/report the blocked conversion, as is already done for conflicting copies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6002db2b1 (queue task 6487). unresolved() now applies the canonical-occupancy check during listing: a conversion whose canonical file currently stores another conversion's record is skipped and reported through unreadable_entries() with a "blocked" reason naming the occupant — the same treatment as conflicting copies — so recovery never receives a record whose state-advancing put() is guaranteed to fail, and later conversions reconcile normally. Regression test: canonically_blocked_conversion_is_skipped_and_reported_during_listing (blocked conversion skipped/reported, occupant reconciles to Abandoned, stranded copy left on disk). Full currency_poc suite: 33 passed.
| stray_paths = copies | ||
| .iter() | ||
| .map(|copy| copy.path.clone()) | ||
| .filter(|path| *path != canonical) |
There was a problem hiding this comment.
On a case-insensitive filesystem, such as default macOS APFS, a record for conversion 10 stored as 0000000000000000000000000000000A.json aliases the lowercase canonical destination, but this lexical comparison classifies it as a stray. The subsequent rename replaces that entry with the new record, then remove_file(stray) deletes the newly persisted canonical record through its uppercase alias. Consequently, put() returns success with no write-ahead record remaining, allowing submission without recoverable intent data. Exclude filesystem aliases of the canonical destination from cleanup rather than comparing path spelling alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6002db2b1 (queue task 6487). put()'s cleanup now skips any stray that is the canonical file itself under another name: the new same_file() helper compares canonicalised paths and, on unix, device+inode (which also covers hardlinks and symlinks whose canonical spellings differ). Such an alias is left in place — listings group by content identity, so it never double-counts — and entry_at() uses the same check, so canonical-occupancy detection is alias-aware too. Regression test: put_never_deletes_a_filesystem_alias_of_the_canonical_file (symlink alias planted in the store; after a state-advancing put, the record survives and the alias is untouched). Full currency_poc suite: 33 passed.
…4578) P1 (intent.rs cleanup): a stray path that is a filesystem alias of the canonical file (case-insensitive spelling, symlink, hardlink) is never a cleanup target - after the rename it names the record just persisted, so removing it would delete the write-ahead record. same_file() compares canonicalised paths and unix dev+inode; entry_at() is alias-aware too. P2 (unresolved listing): a conversion whose canonical file stores another conversions record is blocked - get/put fail closed on the occupancy - so the listing now skips and reports it through unreadable_entries() like a conflicting copy, instead of handing recovery a record whose state-advancing put aborts the scan. Regression tests: put_never_deletes_a_filesystem_alias_of_the_canonical_file, canonically_blocked_conversion_is_skipped_and_reported_during_listing. cargo test -p id --lib currency_poc (in pkgs/id): 33 passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6002db2b1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if path.extension().and_then(|ext| ext.to_str()) != Some("json") { | ||
| continue; |
There was a problem hiding this comment.
On a case-insensitive filesystem such as default macOS APFS, a file named 0000000000000000000000000000000a.JSON also occupies conversion 10's lowercase canonical path, but this filter excludes it entirely. Consequently, the new get(10) returns None, allowing prepare() with different legs to bypass the never-replace guard, and put() overwrites the existing intent. The previous direct canonical-path read found that record and rejected conflicting requests. Unlike the addressed alias-cleanup issue, this bypass occurs before any identity or cleanup guard sees the file; preserve canonical-path lookup even when the directory entry's extension has different casing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bc0a779: the scan extension match is now case-insensitive (eq_ignore_ascii_case("json")), so a ....JSON file is scanned like any other intent file — on a case-insensitive filesystem it is grouped by content identity and matched as the canonical occupant (the new entry_at also falls back to a case-insensitive file-NAME comparison), so get(10) finds it and a conflicting put/prepare() fails closed. Regression: uppercase_extension_alias_is_scanned_and_blocks_conflicting_put in pkgs/id/src/currency_poc/tests.rs (get + unresolved via the alias, conflicting put refused, record intact). Laptop cargo test --lib currency_poc (pkgs/id, world): 35 passed.
| // NON-canonical file is a #4125 skipped entry, not this | ||
| // conversion's canonical record.) | ||
| let canonical = self.path_for(conversion_id); | ||
| if let Some(bad) = scan.unreadable.iter().find(|entry| entry.path == canonical) { |
There was a problem hiding this comment.
On a case-insensitive filesystem, a corrupt canonical file spelled 0000000000000000000000000000000A.json is scanned into unreadable under that uppercase spelling, but this lexical comparison misses the lowercase canonical path. With no parseable copies, get(10) then returns None instead of failing closed, allowing prepare() to overwrite the existing file and submit a replacement conversion. Previously, reading the canonical path directly returned a parse error. Apply filesystem-alias matching to unreadable entries as well as parseable occupants so changing filename casing cannot bypass this guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bc0a779: get() no longer compares the canonical path lexically against unreadable entries — the new IntentScan::unreadable_at matches by exact path, by same_file filesystem identity (canonicalize + device/inode), or by case-insensitive file-name equality, so a corrupt canonical file spelled with uppercase hex digits fails get() closed instead of reading as absent. Regression: unreadable_uppercase_canonical_spelling_fails_get_closed. Laptop currency_poc suite: 35 passed.
| fn entry_at(&self, path: &Path) -> Option<&ScannedIntent> { | ||
| self.entries | ||
| .iter() | ||
| .find(|entry| entry.path == path || same_file(&entry.path, path)) |
There was a problem hiding this comment.
For a store containing N ordinary, correctly named records, unresolved() calls entry_at() once per conversion, causing this search to execute same_file() N(N−1)/2 times—even when every record is already Posted and the listing returns nothing. Each unsuccessful comparison performs filesystem canonicalization and metadata lookups, turning the previously linear recovery listing into quadratic filesystem I/O. Since terminal records remain in the store, recovery becomes progressively more expensive as conversion history grows. Index exact paths and filesystem identities during the scan rather than repeatedly resolving every preceding entry against each canonical path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bc0a779: scan() now builds an occupant index once per scan — each entry is resolved a single time into by_path (its own path + canonicalized path) and by_file_id (device+inode) maps — and entry_at answers from the index (one canonicalize/metadata of the QUERY path at most, plus a syscall-free lexical name fallback). No more pairwise same_file() calls: occupant resolution is linear in the number of entries, so unresolved() over N terminal records no longer performs N(N-1)/2 filesystem resolutions.
Fixes #4578. Queue task 6487.
Problem
FileIntentStoreused canonical filenames forget()but content-based directory scanning forunresolved(): duplicate copies of one intent reconciled multiple times, and a parseable misnamed record was invisible toprepare()'sget()-based never-replace guard, so a conflicting canonical record could be written over it.Fix — one identity contract, documented on the store and the trait
A conversion's identity is the
conversion_idin the record content; the canonical file{id:032x}.jsonis the only path the store writes.get(),unresolved(), andput()now share one directory scan grouped by content ID:get()finds a record stored only under a non-canonical name, soprepare()'s guard sees it and a conflicting prepare is refused; no canonical twin is written.conversion_idmakeget()/put()error with a diagnostic naming the paths;unresolved()skips that conversion and reports every copy throughunreadable_entries()(the id(currency_poc): one corrupt or foreign .json file in the FileIntentStore directory aborts recovery for every pending conversion #4125 skipped-entry surface) while other conversions proceed.put()canonicalizes: a write that differs from the stored record in anything other thanstateis refused; a legitimate (identical or state-advancing) write lands at the canonical path and removes the stray copies it supersedes. Unreadable entries are never deleted (id(currency_poc): one corrupt or foreign .json file in the FileIntentStore directory aborts recovery for every pending conversion #4125 unchanged); a corrupt canonical file still failsget()closed.Tests
New regression tests in
currency_poc/tests.rs: duplicate copies reconcile exactly once; misnamed record blocks a conflicting prepare; misnamed-only record refuses a conflicting put and canonicalizes a matching state-advancing put; conflicting copies fail closed and are reported while a good conversion still reconciles.Verification: targeted
cargo test --lib currency_pocwas started on the laptop but the box is saturated by concurrent fleet builds (load ~26); per fleet CI-first policy this PR's CI is the verification surface — task 6487 is parked on its result.Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.