fix: publish managed PDF bytes under the platform durability barrier (F_FULLFSYNC)

[Fooftilly]

Fixes #134 (EF-031).

What was wrong

PRKS has one durability convention, in backend/fs_durability.py: file contents are flushed with the strongest barrier the platform offers before anything may treat them as stored, and the directory entry is flushed after the rename that created it. #129 established the convention and #133 added fsync_open_file(fd) for a file this process still holds open.

The two paths that publish canonical managed-PDF bytes were only half converted. Verified against current master (9b2d8dc) before changing anything — they were the only two bare content os.fsync() calls left in backend/:

• atomic_replace_managed_pdf_bytes() — content sync before os.replace()
• store_new_managed_pdf_bytes() — content sync after the exclusive create

Both already routed their directory sync through fsync_managed_pdf_parent() → fsync_directory(), but their content sync was still os.fsync(fp.fileno()). On macOS that is not a durability barrier: it returns once the write reaches the drive, without waiting for the drive's own cache — exactly the window a power loss falls into. So an annotation materialization or an upload could report success over bytes that never reached stable storage.

Linearization already flushes through fsync_file_path(), which meant the optional step was the only one applying the strong barrier. maybe_linearize_pdf_in_place() is skipped whenever PRKS_PDF_LINEARIZE=0, qpdf is missing, or it declines the rewrite, and even when it runs there is a crash window between the weak publish and its durable replace. An optimization must never be what makes the first write durable.

What changed

Both sites now call fsync_open_file() from backend/fs_durability.py. No platform logic is duplicated here — F_FULLFSYNC, _sync_descriptor and the fcntl import stay owned by fs_durability, and a new static test keeps it that way.

Ordering and fail-closed semantics are unchanged in shape and now rest on the shared primitive. Where the barrier falls differs between the two shapes, and the docstrings say so rather than generalizing to the weaker of the two:

• Replacement — barrier before the canonical name. Write a sibling temporary → flush() → fsync_open_file() → os.replace() → directory sync. A refused content sync removes the temporary and propagates, so the previous canonical PDF is untouched and nothing was published.
• New exclusive store — barrier before the report. The exclusive create already holds the final name (reserving it is the point of creating it exclusively), so there is no rename for the sync to precede. What it precedes is the report: create → write → flush() → fsync_open_file() → directory sync → (optional) linearize → return the name. The file counts as a stored PDF only when that name comes back for a Work to reference. A refused content sync falls into the existing OSError handler, which removes the partial file and raises ManagedPdfStoreError("write_failed"), so no name is handed back.

Neither shape lets a caller learn of a managed PDF whose bytes have not passed the barrier.

Containment is untouched: safe_pdf_path_under_dir() at runtime plus the CodeQL-documented normpath(join(base, basename)) + startswith(base) rebuild before every filesystem sink.

File contents vs. directory entries

The two halves of the convention stay distinct rather than converging on one state machine, per fs_durability's own contract:

• File-content durability is fatal. It is a hard requirement, fsync_open_file() raises, and the caller abandons the write rather than presenting it as durable.
• Directory-entry durability is best-effort. The operation does not exist on every platform or filesystem, and by the time it runs the rename has already happened and cannot be unwound. fsync_managed_pdf_parent() keeps returning a boolean rather than raising.

Reviewing that boolean (as #134 suggests) — it was previously dropped at both call sites. It is now recorded in a metadata-only log line (pdf_replace_dir_sync_failed / pdf_upload_dir_sync_failed) so the weaker guarantee is surfaced, without changing return types, control flow, or inventing a restore-style state machine. The bytes are durable either way; only the entry is weaker.

Callers

Checked every caller of both helpers. None assumed the old bare-os.fsync behaviour:

• replace_managed_work_pdf() — a failing atomic_replace_managed_pdf_bytes() raises before os.replace(), so no COW exclusive file exists yet and cow_exclusive_unreferenced is correctly still False.
• backend/server.py PDF upload adapter — maps ManagedPdfStoreError onto a response; unchanged.
• backend/backup_restore.py only references atomic_replace_managed_pdf_bytes in prose, as the containment pattern it mirrors.

Tests

New: tests/test_managed_pdf_write_durability.py (12 tests), call-order assertions throughout, no timing.

• atomic_replace_managed_pdf_bytes() syncs through fsync_open_file() strictly before os.replace(), and the directory after — with the synced descriptor already carrying the whole body.
• store_new_managed_pdf_bytes() syncs before the directory sync, before linearization, and before returning a name.
• A raising fsync_open_file() leaves the previous canonical PDF intact with no temp leftovers; the upload path raises ManagedPdfStoreError("write_failed"), removes the partial file, and never linearizes.
• With _FULLFSYNC forced on (the pattern tests/test_pdf_linearize_durability.py already uses), both paths issue F_FULLFSYNC on the descriptor that becomes canonical, and plain os.fsync is never called.
• With _FULLFSYNC off, the plain os.fsync fallback still flushes what it published.
• A failed directory sync is reported, not raised — the write survives.
• Statically: no os.fsync(, F_FULLFSYNC, _sync_descriptor or import fcntl in work_pdf_replace.py.

Falsifiability checked: 11 of the 12 fail against the pre-change implementation.

Run:

• python -m unittest tests.test_managed_pdf_write_durability tests.test_pdf_linearize_durability tests.test_restore_durability — 74 tests, OK
• python run_tests.py (full unit/API/structural/Node suite) — 2308 tests, OK
• ruff check and pyright on both touched files — clean

No E2E was run: this is a filesystem-durability change with no UI surface.

Scope

Two functions in backend/services/work_pdf_replace.py plus one new test file, and a prose-only follow-up commit correcting the module's durability wording. No storage-layout, restore (#110), or linearization (#112) behaviour changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QLLQeu89khtoaZtom35buk

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when replacing or uploading managed PDFs by synchronizing file contents before publication.
  • Failed content synchronization now stops the operation and cleans up incomplete or temporary files.
  • Completed PDF writes are preserved when directory synchronization is unavailable, with a warning recorded instead of failing the operation.
  • Improved platform-specific synchronization support, including macOS full-file syncing and standard fallback behavior.

[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: 5cafe60a-a450-4b0c-b2fa-464d0ba9e834

📥 Commits

Reviewing files that changed from the base of the PR and between e9dba30 and 552086b.

📒 Files selected for processing (2)

• backend/services/work_pdf_replace.py
• tests/test_managed_pdf_write_durability.py

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/test_managed_pdf_write_durability.py
• backend/services/work_pdf_replace.py

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

---

📝 Walkthrough

Walkthrough

Managed PDF replacement and upload paths now use shared durability primitives. Content-sync failures abort and clean up writes. Directory-sync refusals log warnings while retaining completed files. Tests cover ordering, cleanup, platform barriers, and primitive usage.

Changes

Managed PDF durability

Layer / File(s)	Summary
Managed PDF write paths backend/services/work_pdf_replace.py	Replacement and upload paths use fsync_open_file before publication. Content-sync failures continue to abort writes. Directory-sync refusals now log warnings after successful publication.
Durability validation tests/test_managed_pdf_write_durability.py	Tests cover synchronization ordering, cleanup, directory-sync warnings, macOS _FULLFSYNC, fallback behavior, and use of backend.fs_durability.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 55208

The change is intended to make managed PDF writes durable while failing closed on content-sync errors. No current production risk requiring resolution before merge is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 2 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 satisfies the coding requirements in #134. atomic_replace_managed_pdf_bytes uses shared fsync_open_file() after writing and before os.replace(). store_new_managed_pdf_bytes uses the sam…
Out of Scope Changes check	✅ Passed	The changes stay within #134. Source changes update managed PDF replacement and exclusive-create durability. Tests validate the shared durability contract and prevent duplicated platform logic. No unr…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: managed PDF bytes now publish only after the platform durability barrier, including F_FULLFSYNC where applicable.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• 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

Use platform durability barrier for managed PDF publication

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Route canonical managed-PDF writes through the platform's strongest available durability barrier.
• Preserve fail-closed content syncing while reporting directory-sync failures without rejecting
 durable writes.
• Test ordering, cleanup, fallback barriers, and linearization independence across both write paths.

Diagram

sequenceDiagram
    participant C as Caller
    participant R as Replace path
    participant S as Store path
    participant D as Durability
    participant F as Filesystem
    alt Replace existing PDF
        C->>R: Write temporary
        R->>D: Sync open file
        D->>F: Strong content barrier
        R->>F: Atomic replace
        R->>D: Sync directory
    else Store new PDF
        C->>S: Exclusive write
        S->>D: Sync open file
        D->>F: Strong content barrier
        S->>D: Sync directory
        S->>F: Optional linearize
    end

Loading

High-Level Assessment

The shared fsync_open_file() primitive is the appropriate approach because it centralizes platform-specific barrier selection and preserves the established durability contract. Calling F_FULLFSYNC directly would duplicate platform logic, while reopening files through fsync_file_path() would add unnecessary descriptor handling for files already open.

Files changed (2) +512 / -5

Bug fix (1) +44 / -5

work_pdf_replace.pyApply strong durability barriers to canonical PDF writes +44/-5

Apply strong durability barriers to canonical PDF writes

• Replaces direct 'os.fsync()' calls in replacement and new-upload paths with the shared platform-aware 'fsync_open_file()' primitive. Content-sync failures remain fatal and clean up unpublished files, while failed directory syncs now emit metadata-only warnings without rejecting durable writes.

backend/services/work_pdf_replace.py

Tests (1) +468 / -0

test_managed_pdf_write_durability.pyCover managed-PDF durability ordering and failure semantics +468/-0

Cover managed-PDF durability ordering and failure semantics

• Adds regression coverage for sync-before-publication ordering, complete-body flushing, cleanup after content-sync failures, and best-effort directory-sync reporting. It also verifies macOS-style full barriers, plain-fsync fallback behavior, linearization independence, and centralized ownership of platform logic.

tests/test_managed_pdf_write_durability.py

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; the changed write paths preserve cleanup and API behavior while strengthening content durability.

Summary

This PR routes both canonical managed-PDF write paths through the shared platform-aware content durability primitive and reports best-effort directory-sync failures without changing write outcomes.

• Replacements now fully synchronize temporary-file contents before publishing them with os.replace().
• New uploads fully synchronize their contents before directory synchronization and optional linearization.
• Failure-path and platform-branch tests cover ordering, cleanup, logging, F_FULLFSYNC, and fallback behavior.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Write complete PDF body] --> B[Flush Python buffer]
    B --> C[fsync_open_file]
    C -->|failure| D[Remove unpublished or partial file]
    D --> E[Propagate mapped write failure]
    C -->|success: replacement| F[os.replace publishes canonical name]
    C -->|success: new upload| G[Keep exclusively created canonical file]
    F --> H[Best-effort parent directory sync]
    G --> H
    H -->|failure| I[Emit metadata-only warning]
    H -->|success or reported failure| J[Return success]
    J --> K[Optional linearization for uploads]

Loading

_{Reviews (1) · Last reviewed commit: "fix: publish managed PDF bytes under the..."}

[Fooftilly]

All 10 check runs are green on 552086b and the branch is mergeable. CodeRabbit, Qodo and Greptile each reported no actionable findings.

The one item still showing as failed is CodeRabbit's non-blocking Docstring Coverage pre-merge check (36.36% vs. an 80% default threshold). Not acting on it, for two reasons:

1. It is not in the code this PR changes. Every function the diff actually touches is documented — atomic_replace_managed_pdf_bytes(), store_new_managed_pdf_bytes() and fsync_managed_pdf_parent() all carry docstrings, and two of them were expanded here to state exactly where each path's durability barrier falls. The three undocumented functions in work_pdf_replace.py (_stale_body, _sync_mentioned_roles, __init__) are pre-existing and untouched.

2. The gap is test scaffolding, and closing it would be padding. 21 of the 24 undocumented functions are in the new test file: the _WriteHarness spy methods (replace, linearize, steps), setUp, one-line accessors (temp_leftovers, stored_pdfs, canonical_bytes), local closures, and a few test_* methods whose names already state the assertion — e.g. test_the_temp_is_synced_through_the_shared_primitive_before_it_is_published. Adding """Return the temp leftovers.""" to those raises a metric without telling a reader anything.

For scale, the two sibling suites this file was modelled on sit at 22.7% (test_pdf_linearize_durability.py) and 20.6% (test_restore_durability.py); the new file is at 32.3%. Docstrings in this repo carry the why — the durability contract, the failure semantics, why a patch lands on module globals — and those are present, at the module, class and non-obvious-test level. Happy to add them anywhere a reviewer finds the intent genuinely unclear.

---

Generated by Claude Code