fix(preservation-spine): classify races and make checkpoint create/unlink durable - #59
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCheckpoint publication and recovery now synchronize directory metadata and discard unpublished empty genesis files. Verification uses canonical records. Capture, manifest, restore, and Windows file handling now report standardized errors and clean up resources. ChangesPreservation integrity hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the purpose, lists the main fixes, states scope limits, and records Linux test results. It does not complete the required Truth status, Prior art and reuse, Docs and registries, or Risk sections. It also does not confirm the required green-set command and checkbox requirements. Resolution Complete all template sections. Add claim statuses with evidence, the ADR-18 reuse-gate verdict and required probes or an applicability explanation, docs and registry confirmations, blast radius, rollback plan, and explicit confirmation that the required green-set command and new regression tests pass. Include the exact pytest summary for the required green set or explain any known limitation.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
floss | c1e34d9 | Aug 24 2026, 12:13 AM |
![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: c1e34d903c
ℹ️ 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".
| raise CheckpointIntegrityError( | ||
| "checkpoint file changed while acquiring append handle" | ||
| ) | ||
| _fsync_parent_directory(path) |
There was a problem hiding this comment.
During the first checkpoint append, this fsync makes the newly created but still-empty checkpoints.jsonl directory entry durable before _append_with_intent creates the recovery intent. A crash in that interval therefore leaves a durable empty file with no intent; subsequent loads fail with checkpoint file is empty, while subsequent appends take the existing-file path and fail the same way, requiring manual deletion. Create and persist the intent before publishing the target, or recover an empty no-intent genesis file safely.
Useful? React with 👍 / 👎.
c1e34d9 to
d8c01fe
Compare
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
floss | 7edd1c8 | Commit Preview URL Branch Preview URL |
Sep 06 2026, 05:44 AM |
|
Addressed the Codex P1 in Genesis now writes and fsyncs the append intent before publishing the empty
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48e38f8e90
ℹ️ 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".
| metadata = _validated_file_state(path) | ||
| if metadata.st_size != 0: | ||
| return | ||
| _remove_empty_checkpoint_file(path) |
There was a problem hiding this comment.
When another process has this empty path open and writes a genesis record concurrently with load or append, this cleanup can delete the newly written checkpoint: _remove_empty_checkpoint_file only checks the size before os.unlink, while the directory lock is cooperative and does not prevent writes through an existing descriptor. A record written between the final size check and unlink becomes unnamed and is lost; the cleanup should retain and lock the file identity through removal or report drift when ownership of the empty file cannot be proven.
Useful? React with 👍 / 👎.
kalisam
left a comment
kalisam
left a comment
There was a problem hiding this comment.
Hermes adversarial review — verdict: COMMENT / land WITH #43 (stacked)
Head 48e38f8e, 2 commits / 13 files / +454 −27, strict fast-forward of #43 head 7f27578c (body cites 073c85e6 — wrong SHA, different tree).
Verified repairs of #43 bugs: empty-genesis wedge (intent-before-publish + _discard_unpublished_genesis), capture FileNotFound races → CaptureDrift, fd leak on fdopen/open_osfhandle failure, verification-digest-unbound unmasking, idempotent-verify byte recheck, octal >0o377 and non-ASCII bundle-head classification. Small, aimed, regression-tested. This is the right follow-up.
Still open here:
_remove_empty_checkpoint_file(checkpoint.py:885-904) — size check thenos.unlinkis racy if any concurrent process can write genesis while holding no lock on this path; confirm_locked_directoryscope covers all writers or hold the fd across check+unlink.- Windows durability is a documented no-op (
_fsync_parent_directoryreturns onnt; fsync tests skipped on Windows; CI ubuntu-only) — this workspace runs on Windows. Say "best-effort on NT" in the module docstring or add FlushFileBuffers on a directory handle + a windows-latest test job. - The new dedupe test monkeypatches a PASS restore that production cannot emit (opaque-plane deadlock, see #43 review Critical 1) — green here does not witness the real pipeline.
CaptureDrift/realCheckpointIntegrityErrorstill swallowed by the CLI's dummy exception class (#43cli.py:116-125) — your classification work is invisible to operators until that import is fixed.
Merge sequence: #43 with its blockers addressed, then this immediately. Do not merge #43 alone.
…reate/unlink durable Bare FileNotFound during resolve/read aborted capture outside the evidence hierarchy. Checkpoint names were fsynced but their directory entries were not, so a crash could drop a committed intent. Windows handle errors and malformed capsule input (non-ASCII bundle HEAD, octal > 0o377) raised unclassified OSError/ValueError/UnicodeDecodeError. Also: fdopen failures now close the raw fd; unbound verification digests are their own blocker; verify dedupes restore-verified; duplicate-path errors are plane-neutral.
… empty log The first append fsynced an empty checkpoints.jsonl directory entry before the recovery intent existed. A crash in that window left a durable empty file: load failed with "checkpoint file is empty", and the next append took the existing-file path and failed the same way. Write and fsync the intent first, then publish the empty target. A leftover empty file with no intent is unpublished genesis and is discarded.
…indows CI advisory - checkpoint.py module docstring: add Windows durability note explaining that parent-directory fsync is a no-op on NT (no FlushFileBuffers on directory handles); directory-entry durability is best-effort on Windows. - python-ci.yml: remove job-level continue-on-error from full-suite-advisory so a red advisory is VISIBLE (step-level continue-on-error remains so it never blocks); the job stays out of required checks. - python-ci.yml: add preservation-spine-windows-advisory job on windows-latest — advisory only, step-level continue-on-error, not a required check. Surfaces Windows-specific durability behavior visibly. - PR59 body: fix base SHA reference from 073c85e to 7f27578.
59-A: _remove_empty_checkpoint_file now opens the target descriptor, verifies regular-file type and zero length with os.fstat, closes the descriptor, and unlinks while its callers hold _locked_directory(parent). Both call sites are already under the directory lock, preventing a concurrent checkpoint append between the size check and unlink. Focused verification: 48 passed, 2 skipped.
48e38f8 to
7edd1c8
Compare
Hermes Review — Post-Fix RoundVerdict: Comment (fixes landed, rebased onto PR43 tip) Fixed this round
Verification
NoteBranch was force-pushed after rebasing onto the updated PR43 tip ( Hermes Agent — FLOSSI0ULLK |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7edd1c8ce8
ℹ️ 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".
| metadata = _validated_file_state(path) | ||
| if metadata.st_size != 0: | ||
| raise CheckpointIntegrityError( | ||
| "checkpoint recovery refused to remove a non-empty file" | ||
| ) | ||
| return | ||
| _remove_empty_checkpoint_file(path) |
There was a problem hiding this comment.
When an already committed checkpoints.jsonl is accidentally truncated to zero while no append is active, the normal absence of an intent causes this branch to classify it as unpublished and unlink it. append_checkpoint can then accept a new sequence-0 genesis, silently resetting the checkpoint chain; before this change, the empty file produced an integrity error. A zero-length file without an intent is insufficient evidence that no committed chain previously existed, so recovery needs a durable creation marker or must continue failing closed.
Useful? React with 👍 / 👎.
1 participant
What this is
Stacked on #43 (
feat/preservation-spine-standalone@4daefa0). Classifies the remaining bareOSError/ValueError/UnicodeDecodeErrorpaths and makes checkpoint create/unlink crash-durable on POSIX.Does not change ADRs, integrity zomes, consensus logic, or delete anything.
Fixes
resolve(strict=True)/read_bytesFileNotFound →CaptureDriftcheckpoints.jsonland creating/unlinking the append-intent fileos.fdopenfailure closes the raw fd (checkpoint + seal)open_osfhandleOSError →CheckpointIntegrityError/CapsuleVerificationErrorCapsuleVerificationError> 0o377→CapsuleVerificationErrorverification-digest-unboundis no longer masked byrestore-check-blockedrestore-verified; digest hashes canonical JSON after asserting file bytes match that formLeft for later (called out on #43): frozen
input_shasmutation; unbundledrefs/stash; tracked plane stores diff+hash, not reconstructable worktree bytes.Verification
Scope: downloaded
packages/preservation_spinesuite at7f27578cplus the new regressions. Windows handle conversion and the Windows parent-fsync no-op were patched from source but not executed (Linux only).Summary by CodeRabbit