fix: make the restore journal and component renames power-loss durable

[Fooftilly]

Fixes #110 (EF-017).

What was wrong

The restore transaction is journaled well enough to recover from a process that dies between two system calls — the RestoreCrash injection tests prove that — but os.replace() alone does not put a directory entry on stable storage. _atomic_write_json() fsynced the journal's temporary and then replaced the journal name without syncing the journal directory; _move_old_component(), _install_new_component() and _restore_rollback_component() renamed canonical, rollback and staged paths without syncing either affected parent.

So the recovery state machine's assumption — that a persisted phase and the rename it describes are consistent after a restart — held for a process crash but not for a machine/power/filesystem crash, which can lose an unsynced directory entry and leave the journal and the component in different phases.

Verified against current master (664cb8e) before changing anything: both gaps are still present there.

The ordering, and how it reuses #129

No second fsync abstraction. backend/fs_durability.py — the convention #129 introduced for rename-based replacement (sync the contents before the replace, sync the directory after it; content durability raises, directory durability is best-effort and returns whether the platform could establish it) — is generalized with two additions:

• fsync_directories(*paths) — every restore rename crosses directories (live storage ↔ the rollback tree, the staging tree → live storage), and a rename changes an entry at both ends. This collects the source and destination parents of a batch of moves, collapses repeats so a database and its WAL sidecars cost one sync per directory, attempts every directory even after one refuses, and answers once for the whole set.
• fsync_open_file(fd) — the fsync_file_path() guarantee for a file this process is still holding. On macOS that is F_FULLFSYNC, not os.fsync(). backend/backup_restore.py now has no ad-hoc os.fsync() calls left.

Restore then follows three orderings:

Journal — fsync contents → replace journal → fsync journal directory → phase persisted. _write_journal() raises rather than returning when the directory sync is refused: the step that follows is a rename whose correct recovery depends on the phase being readable.

Component — persist "rename starting" → rename → fsync every directory the rename changed → persist "rename completed". This covers live → rollback, staged → live, and rollback → live (rollback runs the same rename in the opposite direction, so it owns the same boundary). A fresh rollback directory also has its own entry synced before anything is moved into it. A batch that fails partway — a database whose sidecar rename raises — still syncs what already moved before the error unwinds, so no completed rename is ever left with no barrier behind it.

Staged payload — each extracted file is flushed while its handle is open, and every directory the extraction wrote into is synced up to the extraction root, before anything can rename it onto a canonical name. Renaming tree/files/pdfs makes that entry durable, not the entries inside it.

The four outcomes, kept distinct

reason	meaning
restore_dir_not_durable	a directory the next rename needs could not be made durable, or a move would reach outside the storage root — nothing has been moved
rename_not_durable	the rename happened, but its directory entries are not confirmed — the journal does not claim the move completed, and the transaction rolls back
journal_not_durable	the phase itself is not on stable storage — nothing proceeds as though recovery could read it
staging_not_durable	a staged tree's entries could not be confirmed — staging fails, having installed nothing
(unsupported)	a directory fsync the platform does not implement is not a failure: fsync_directory() reports it durable because nothing stronger exists to ask for

No phase is ever recorded as persisted when its boundary was not established.

Containment

Both filesystem sinks this PR touches prove containment inline, on the value that reaches them, in the pattern services/work_pdf_replace.atomic_replace_managed_pdf_bytes() documents — the paths are built from a request's staging token and a journal's transaction id, so a crafted token or a damaged journal must not be able to point a restore rename or fsync outside the tree it belongs to:

• _replace_durably() canonicalizes both ends of every move through their parent, rebuilds them against the resolved root with join+normpath, proves each is inside with a standalone startswith before any rename, and re-checks immediately before os.replace. A move reaching outside is refused before anything is renamed, so it raises the "nothing has been moved" reason.
• _fsync_dirs_under(root, paths) is the one place this module reaches the directory convention, and applies the same rebuild-and-prove to every directory it syncs.

The parent is resolved but never the last component: resolving the leaf would rename what a symlink points at instead of the symlink. The parent must be resolved, because PRKS_STORAGE may itself be a symlink while the maintenance tree anchors to _resolved_storage_root() — a live path under the link and a rollback path under the target have to land in one comparable namespace.

Portability

Unchanged behaviour on Windows: fsync_directory() returns True on non-POSIX because there is no directory handle to flush, so fsync_directories() and _mkdir_owner_durable() always succeed there and restore is not made unusable.

Fail-safe recovery

A durability failure never deletes the last known-good copy:

• A refused sync during live → rollback leaves the previous component in the rollback tree, which is exactly what rollback needs to put it back.
• A refused sync during staged → live still has new_install_started persisted, so rollback removes the installed copy and restores the previous one.
• When rollback's own moves are not durable, _rollback_from_journal() reports it and both apply_restore() and recover_incomplete_restore() keep the journal and the rollback tree instead of cleaning them away. The next startup replays them — replay is already non-destructive — and that replay re-syncs both ends of the move the refused pass already made, because cleanup follows it.
• The commit record is the one write that must not raise. Everything is installed and bound by then, and the replace has already happened when only its directory sync is refused; rolling back there is the unsafe move, because a crash during that rollback could leave a surviving committed journal describing a half-rolled-back library. Restore instead keeps the journal and the rollback tree, warns, and lets the next start resolve whichever phase survived — keep_restored if the entry is there, restored_previous if it is not.

Scope

• The existing journal/state-machine architecture is untouched; only the persistence boundaries around it are.
• _atomic_write_json() has two users. The journal treats the write as a boundary and refuses to advance; staging metadata does not (losing it costs a re-upload, not a library) and logs the weaker guarantee instead of failing a verified backup. No unrelated filesystem cleanup was broadened.
• No [Audit Finding] Make PDF linearization preserve managed-file durability guarantees #112 behaviour changed — pdf_linearize and work_pdf_replace only gained shared helpers alongside the ones they already use. No [Audit Finding] Make post-delete cleanup failures recoverable #91 delete-cleanup work here.

Review rounds

Bots, on the first commit — four findings, all verified and fixed in b321362 (detail in the resolved threads): Greptile's P1 unsynced staged payloads (→ the staged-payload ordering above), Qodo's two high findings on the commit record and the consumed-rollback replay (→ the two fail-safe rules above), and the CodeQL path-injection alert.

CodeQL took a third commit, 386a738, because my first two attempts aimed at the wrong sink. Running the query locally (codeql/python-queries Security/CWE-022/PathInjection.ql, CLI 2.27.0) against this branch and against master showed what it actually is:

master  backend/backup_restore.py:2484  os.replace(staged, live)
branch  backend/backup_restore.py:2371  os.replace(src, dest)

Same flow, same single sink — master carries this alert too, in _install_new_component(). Pulling the rename into the shared _replace_durably() moved the line, and a relocated alert counts as new on a PR. The flow is real either way, so 386a738 sanitizes it at the sink. The query now reports 26 sinks on this branch against master's 27: the new alert is gone and master's own goes with it. The five that remain in this file (isfile(meta_path), open(meta_path), isdir(staged), lexists(staged), isfile(staged_db)) are pre-existing on master, outside what #110 asked for, and untouched here.

Maintainer review — two merge-blocking findings, both reproduced and fixed in 4fbf860:

• the batch of renames synced only after the whole loop, so a sidecar rename that raised left the completed database rename with no barrier behind it. The syncs now run in finally;
• the containment root used abspath, which broke a legitimate live → rollback move when PRKS_STORAGE is a symlink — a regression 386a738 introduced. Containment is now canonical on both sides, as described above.

Each of the four new tests for these fails without the fix; the symlink one reproduces the reported restore_dir_not_durable / move_containment refusal exactly.

Tests

tests/test_restore_durability.py (35 tests) spies on the calls rather than pretending CI can stage a power loss:

• journal: fsync-file → replace → fsync-dir, same-directory replace costs exactly one sync, phase refused on a failed directory sync, journal not replaced when the contents cannot be synced, unsupported errno still counts as persisted while EIO does not, staging metadata reports the weaker guarantee without raising;
• component moves/installs: the full step sequence including which persist claims what, both parents synced across directories, one sync per directory for the database + sidecars, a batch that fails partway still syncing what already moved, every refusal path leaving old_moved / new_installed unset with the material still recoverable, and the rollback directory refused before anything moves into it;
• recovery: the directories it moves the previous library back into are synced, a refused sync keeps the journal and the rollback tree, the replay confirms the entry the refused pass left unsynced before cleanup, and the replay stays non-destructive;
• containment: inside the root, the root itself, an outside directory (asserting it is never opened), a sibling whose name merely extends the root, a move reaching outside refused before it happens, and a symlinked storage root that must not be mistaken for an escape;
• the fs_durability additions: dedupe, both-parent ordering, every-directory-attempted, unsupported vs. failed, and fsync_open_file() success/raise.

tests/test_backup_restore.py (104 tests) adds the whole-transaction cases: a refused directory sync during a real apply_restore() stops it and the next recover_incomplete_restore() returns the previous library intact; an unconfirmed commit record does not roll back a finished restore; a failing sidecar rename leaves the previous database and its sidecar byte-for-byte in place; a full restore runs through a symlinked storage root; every staged payload file's inode is flushed during staging; and a staged tree whose directory entries cannot be synced fails staging without touching the live library.

Commands actually run

• python3.12 run_tests.py — full unit/API/Node/static suite, 2290 tests, OK; dependency preflight OK
• python3.12 -m unittest tests.test_backup_restore — 104 tests, OK (existing RestoreCrash injection tests unchanged and passing)
• python3.12 -m unittest tests.test_restore_durability — 35 tests, OK
• codeql database analyze … Security/CWE-022/PathInjection.ql (CLI 2.27.0) on this branch and on master — 26 sinks vs. 27, re-run after each change to the sink
• each new regression test re-run with its fix reverted, to confirm it fails
• ruff check . — clean on every touched file (6 pre-existing B023 errors in untouched files, identical before and after this branch)
• pyright — 0 errors, 0 warnings
• python3.12 tests/e2e/run.py --jobs 4 --no-pointer-capture --affected — selected smoke (9 tests), PASS. The full E2E gate was not run: this is backend-only with no UI or API contract change.

The one red check

github-advanced-security is GitHub's own Copilot code-scanning reviewer failing at session creation with CAPIError: 400 The requested model is not supported — it never analyzed the diff, its file-exclusion list excludes *.py, and it has failed identically on every head of this PR. Nothing in this PR can change it; see the comment for the log.

🤖 Generated with Claude Code

https://claude.ai/code/session_014SNZ6ukWrT4M4tUFDB44mV

Summary by CodeRabbit

• Reliability
  • Restore operations now flush extracted files and synchronize affected directories before installation.
  • Restores stop with a clear error when required durability cannot be confirmed.
  • Recovery information is preserved when durability is uncertain, enabling recovery on the next startup.
  • Directory safety checks prevent synchronization outside the restore area.
• Bug Fixes
  • Improved rollback and crash recovery for interrupted restore operations.
  • Completed restores are no longer rolled back solely because commit confirmation is unavailable.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: ac4ff807-4fa2-4a60-b367-d02bc0f7838d

📥 Commits

Reviewing files that changed from the base of the PR and between 386a738 and 4fbf860.

📒 Files selected for processing (3)

• backend/backup_restore.py
• tests/test_backup_restore.py
• tests/test_restore_durability.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The restore transaction now enforces filesystem durability for staged payloads, journal writes, directory creation, component renames, rollback, and recovery. Tests cover ordering, refusal paths, cross-directory moves, containment, and replay behavior.

Changes

Restore durability

Layer / File(s)	Summary
Durability primitives and staging AGENTS.md, backend/fs_durability.py, backend/backup_restore.py, tests/test_restore_durability.py	Added open-file and directory synchronization helpers. Staged payload files and extracted trees are synchronized before installation. Atomic metadata writes synchronize parent directories within the storage root.
Durable journal and restore operations backend/backup_restore.py, tests/test_backup_restore.py, tests/test_restore_durability.py	Journal phases, component moves, installations, and rollback-directory creation now use root-scoped durability checks. Cross-directory renames synchronize both parent directories and report distinct durability failures.
Rollback, recovery, and durability validation backend/backup_restore.py, tests/test_backup_restore.py, tests/test_restore_durability.py	Rollback and startup recovery retain journal and rollback state when durability is uncertain. Tests verify ordering, refusal and retry behavior, containment, commit handling, and replay recovery.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: High

Suggested reviewers: claude

Sequence Diagram(s)

sequenceDiagram
  participant RestoreTransaction
  participant fs_durability
  participant Filesystem
  participant Recovery
  RestoreTransaction->>fs_durability: sync journal or component boundaries
  fs_durability->>Filesystem: fsync files and affected directories
  Filesystem-->>RestoreTransaction: success or durability refusal
  RestoreTransaction->>Recovery: preserve replay state on refusal
  Recovery->>Filesystem: retry rollback operations

Loading

Merge Risk: ⚪ Minimal · up to 4fbf8

Restore durability and recovery behavior is covered by the reported implementation changes and focused tests, with no current merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.19% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 109 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR meets the coding requirements in #110. backend/fs_durability.py adds open-file and directory fsync helpers, including cross-directory synchronization and unsupported-directory handling. `back…
Out of Scope Changes check	✅ Passed	The changed code and tests remain within #110. Durability helpers, staging synchronization, recovery-state handling, containment checks, documentation, and focused tests support restore crash consiste…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main changes: making restore journal updates and component renames durable against power loss.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Make restore renames durable across power loss

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Fsync journal contents and directory entries before advancing restore phases.
• Sync both parents for component renames and retain recovery state on failures.
• Add ordering, refusal, rollback, and end-to-end durability coverage.

Diagram

graph TD
  S["Persist rename start"] --> R["Rename entries"] --> D{"Parents durable?"}
  D -->|Yes| C["Persist completion"]
  D -->|No| B["Rollback library"] --> Q{"Rollback durable?"}
  Q -->|Yes| X["Clean recovery state"]
  Q -->|No| K["Keep recovery state"]

Loading

High-Level Assessment

The proposed approach is appropriate: it extends the existing filesystem durability abstraction instead of introducing restore-specific fsync logic, and aligns journal phases with the exact rename durability boundaries recovery assumes. Ad hoc fsync calls or copy-based replacement were considered less suitable because they would duplicate platform handling or substantially increase I/O and transaction complexity.

Files changed (5) +944 / -37

Enhancement (1) +43 / -0

fs_durability.pyAdd open-file and multi-directory fsync helpers +43/-0

Add open-file and multi-directory fsync helpers

• Adds a platform-aware durability barrier for already-open files and a deduplicating helper that attempts every affected directory. Cross-directory renames can now report one durability result covering both source and destination parents.

backend/fs_durability.py

Bug fix (1) +214 / -37

backup_restore.pyEnforce durable restore and rollback transitions +214/-37

Enforce durable restore and rollback transitions

• Flushes journal files and syncs journal directories before phases are considered persisted. Component installs, rollback moves, directory creation, and sidecar renames now sync every affected parent and preserve recovery artifacts when durability cannot be confirmed.

backend/backup_restore.py

Tests (2) +678 / -0

test_backup_restore.pyVerify failed durability keeps the previous library recoverable +54/-0

Verify failed durability keeps the previous library recoverable

• Adds an end-to-end restore test that refuses a directory sync, verifies recovery state remains, and confirms a later recovery restores the original database and PDFs.

tests/test_backup_restore.py

test_restore_durability.pyTest restore durability ordering and recovery behavior +624/-0

Test restore durability ordering and recovery behavior

• Adds focused coverage for journal flush ordering, cross-directory rename syncing, batching, refusal handling, durable rollback, replay safety, and unsupported filesystem behavior. It also tests the new shared durability primitives and open-file failure propagation.

tests/test_restore_durability.py

Documentation (1) +9 / -0

AGENTS.mdDocument restore durability invariants +9/-0

Document restore durability invariants

• Documents the required journal and component-rename ordering for machine-crash recovery. It also prohibits advancing phases or deleting rollback data after a refused durability boundary.

AGENTS.md

[greptile-apps]

Confidence Score: 4/5

The PR is not yet safe to merge because a power loss can corrupt newly installed restore payloads whose contents were never flushed before their names became durable.

Findings

1.  Unsynced Restore Payloads ▶

Summary

This PR adds crash-durability barriers around restore journals, component renames, rollback replay, and uploaded-file writes, with extensive ordering and refusal-path tests. The directory-entry ordering is substantially strengthened, but the install path omits the file-content half of the repository’s rename durability convention.

• Adds shared open-file and multi-directory sync helpers.
• Syncs restore journal replacement and cross-directory component renames.
• Retains recovery material when rollback directory syncs fail.
• Adds focused durability and whole-transaction recovery tests.
• Still needs to flush extracted staged payload contents before installing them.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Extract payload into staging] --> B[Payload files closed without fsync]
    B --> C[Rename staged component into live storage]
    C --> D[Sync source and destination directories]
    D --> E[Persist new_installed]
    E --> F[Persist committed]
    F --> G[Delete staging and rollback material]
    B -. power loss may discard dirty bytes .-> H[Durable live name with incomplete contents]

Loading

_{Reviews (1) · Last reviewed commit: "fix: make the restore journal and compon..."}

[Fooftilly]

CI status on b321362, and what bc17c72 does about it.

github-advanced-security — failing, and not this PR's. The job log shows GitHub's own Copilot code-scanning reviewer (ccr_security) dying at session creation:

Error creating PR review request: SessionModelError: Execution failed:
CAPIError: 400 The requested model is not supported.
##[error]Process completed with exit code 1.

It never analyzed the diff — it failed before producing any finding — and its own FileExclusionPatterns list excludes *.py, so this PR's changes were not in its scope regardless. It failed identically on both heads so far (6180642 at 07:15 and b321362 at 11:12), which is the "reproduces identically" evidence a re-run would be buying; a third run of a GitHub-side 400 would add nothing, so I have not spent one. There is no fix to port into this PR: nothing in the diff can change the outcome. Flagging it here rather than leaving it silent.

CodeQL — the alert this PR owns. Alert #143 (uncontrolled data used in a path expression) was real: _replace_durably() derived its fsync targets from dirname() of the move paths, which trace back to the request's staging token and the journal's transaction id. b321362 routed every directory sync through one containment helper. The CodeQL check on that head concluded at 11:12:27, before Analyze (python) finished uploading at 11:13:22, and has not been re-posted since — so I could not tell from it whether the guard took.

Rather than wait on an ambiguous check, bc17c72 writes the guard the way this repository already knows CodeQL accepts for this sink. services/work_pdf_replace.atomic_replace_managed_pdf_bytes() carries that shape and documents why: rebuild the path against the trusted base with join + normpath, then a standalone startswith(base) immediately before the sink, plus a second standalone check rejecting a sibling whose name merely extends the base. _fsync_dirs_under() now matches it. Containment semantics are unchanged — outside the root still answers False and is never opened — and the four TestSyncContainment cases still pass.

Everything else is green on b321362: Ruff, Pyright, ESLint, SonarCloud, and all three CodeQL Analyze jobs. Locally on bc17c72: full unit suite 2285 OK, ruff check clean on touched files, pyright 0 errors.

---

Generated by Claude Code

[Fooftilly]

Two merge-blocking durability/compatibility issues remain at the current head.