fix(aws-control): open lock files non-truncating before the acquire

[jeeshofone]

Problem / Motivation

#9248: a lock file opened with open(path, "w") is truncated before any lock is held. On Windows the acquire routes to msvcrt.locking on the already-truncated file, so a contending process can observe or produce an empty lock file and crash out of the critical section. The overnight sweep fixed most subsystems (#9250 session_pid, #9275 issue_radar, #9279 webhooks + mcp_tools/control, #9237 work_ledger), but the four aws_control sites were still live on main, and every merged fix re-derived its own local touch-then-"r+" preamble — nothing stops the class growing back.

This also covers the scope of #9264 (aws_control sites) and #9267 (consolidate a shared helper).

Why it matters

Intermittent-by-interleaving Windows corruption: this exact class reddened Backend Tests (Windows) shards across four unrelated PRs before #9248 named it. The aws_control sites guard the backup/library/shares stores — user data writes serialized by locks that could flicker empty under contention.

What changed

Motivation → approach → change:

• platform_compat.open_lock_file(path) (new, next to file_lock): create-or-open via os.open(..., O_RDWR | O_CREAT) — one syscall, never truncates — yielding a raw fd ready for file_lock / flock_exclusive. Future lock sites get the non-truncating shape by construction instead of by convention (Consolidate the non-truncating lock-open preamble into one shared helper #9267's ask).
• Converted the four aws_control lock sites (backup.py, library.py, shares.py ×2) to it (aws_control lock sidecars are opened truncating before the acquire (4 sites, #9248 sweep) #9264's ask).
• Fleet-wide contract test: scans the source tree for a truncating open(..., "w") whose descriptor is handed to a file_lock-family acquire within the next two lines, and fails naming the file and line. deploy/pending.py / deploy/profiles.py are exempted while PR fix: avoid truncating lock files before acquiring lock on Windows #9269 (which owns those sites) is in flight; the exemption comment says to drop them once it merges.

Tests

• TestOpenLockFile::test_preserves_existing_content — the defect shape: pre-existing lock-file bytes survive open + exclusive lock.
• TestOpenLockFile::test_creates_missing_file_and_is_lockable — create-if-absent, lockable via flock_exclusive.
• TestOpenLockFile::test_no_lock_site_opens_truncating — the repo-wide contract scan. Detector verified against the pre-fix tree, where it named exactly the expected offenders and nothing else.
• aws_control suites (backup / library / storage): 202 passed.
• black gate, flake8 (CI scope), isort, mypy on the touched source files: clean.

Manual verification

Ran the contract scanner standalone against current main: the only hits are the two exempted deploy sites (#9269's scope). No behavioral change on POSIX beyond the open mode; lock-file content is never meaningful to the lock itself.

Screenshots / video

N/A — backend-only, no UI surface.

Related Issues

• Lock files opened truncating before the acquire: five sites still carry the Windows defect fixed in 9237 #9248 (umbrella; remaining sites)
• aws_control lock sidecars are opened truncating before the acquire (4 sites, #9248 sweep) #9264 (aws_control sites — covered here)
• Consolidate the non-truncating lock-open preamble into one shared helper #9267 (shared helper consolidation — covered here)
• fix: avoid truncating lock files before acquiring lock on Windows #9269 (deploy sites — deliberately NOT touched here; scan exemption documents the handoff)

Pattern harvest

A lock-file open must never truncate: acquiring a lock cannot destroy the thing it serializes on. Codified as open_lock_file + a source-scanning contract test so the invariant is enforced, not remembered.

Rule candidate: any file descriptor handed to a file_lock-family acquire must come from a non-truncating open (open_lock_file, "r+", "a+", or O_RDWR|O_CREAT) — never open(path, "w"); enforced repo-wide by the contract scan in test_platform_compat.py::TestOpenLockFile::test_no_lock_site_opens_truncating.

Checklist

• Tests added/updated and passing locally
• Lint/type gates clean (black, flake8, isort, mypy)
• No unrelated changes

Contribution License Agreement

By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.

[dwu96]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed cf7de1c6e61a9d6ec5ed15556c714ffa0444bf20 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] cf7de1c

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of cf7de1c6e61a9d6ec5ed15556c714ffa0444bf20 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

The contract scan claims fleet-wide enforcement but its detector shape misses live in-class offenders, so the invariant is believed enforced while still open.

Watch

• The scan's acquire regex (\b(file_lock|flock_exclusive|acquire_lock)\(\s*\w+\.fileno\(\), two-line window) cannot match cron.py:4514 — lock.open("w") followed three lines later by try_acquire_lock(fd.fileno()), which routes to msvcrt.locking on Windows, i.e. the exact Lock files opened truncating before the acquire: five sites still carry the Windows defect fixed in 9237 #9248 defect on the cron store. \bacquire_lock never matches try_acquire_lock (no word boundary after _), and the acquire sits outside the window. So the docstring's claim "Every site was converted … a new offender fails here" and the description's "the only hits are the two exempted deploy sites" are both false: the class can grow back (and already persists) through any spelling other than the one converted — the precise failure the PR says the test prevents. pod/runtime.py:1060 (open(lock_file, "w") → fcntl.flock(fh)) likewise escapes via the fd-object handoff, though as POSIX-only it's the survivable variant.
  Clears when: the detector matches try_acquire_lock/acquire_lock (fixing the \b miss), widens or removes the two-line window, and either cron.py:4514 is converted here or explicitly exempted-with-handoff like the deploy sites.

Suggestions

• Invert the scan: find every file_lock-family call site and assert the fd's origin is open_lock_file (or another non-truncating open), rather than pattern-matching the truncating shape — positive identity survives new spellings; the current negative match doesn't.

[DESIGN-REVIEWED] cf7de1c

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed cf7de1c6e61a9d6ec5ed15556c714ffa0444bf20 via the fork AI-review pipeline; updated in place on each push.

Review details

The diff is a clean, mechanical fix: open_lock_file yields a raw int fd via non-truncating os.open, closed in finally; call sites pass that int to file_lock(fd, ...) which takes fd: int. Semantics unchanged (old fd.fileno() was also an int; lock file was already empty under "w"). No grounded defect survives.

No findings.

[OPUS-REVIEWED] cf7de1c

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of cf7de1c6e61a9d6ec5ed15556c714ffa0444bf20 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything needed. Verified findings: (1) the two exempted deploy files are already fixed on base (touch + "r+"), so the exemption is dead and the description's manual-verification claim doesn't hold on this tree; (2) cron.py:4514 is a live unfixed sibling the new scanner is structurally blind to (its acquire regex \bacquire_lock\( doesn't match try_acquire_lock( past the underscore, and the acquire sits 3 lines below a 2-line window); (3) work_ledger._open_lock (work_ledger.py:532) already exists as a named non-truncating lock helper, so two helper spellings now coexist. The four aws_control conversions themselves are a genuine fix with linked provenance (#9248/#9264/#9267).

First-Principles-Verdict: CONCERNS

The "fleet-wide" scanner misses the one live sibling (cron.py:4514) and exempts two deploy files that are already fixed on base.

Not justified as shipped

• Item 3 — symptom-level coverage: the scan's acquire regex \bacquire_lock\( cannot match try_acquire_lock( (underscore kills the word boundary) and its 2-line window stops short, so cron.py:4514 — lock.open("w") then try_acquire_lock(fd.fileno()) at line 4517 — stays live while the description claims the class is closed.
• Item 4 — exempts nothing: deploy/pending.py:113-114 and deploy/profiles.py:130-131 already use touch + "r+" on this base (0 scanner hits in deploy/, counted), contradicting "the only hits are the two exempted deploy sites".

What this change ships

Intent: stop Windows lock acquisition from truncating the lock file it serializes on — a FIX (linked #9248, #9264).

1. Four aws_control stores (backup state, library ledger, shares ×2) no longer truncate their lock file before locking — justified
2. New public helper open_lock_file in platform_compat for future lock sites — justified
3. Repo-wide test failing any new truncating lock open — symptom-level, blind to try_acquire_lock and >2-line gaps; 1 counted unfixed sibling: cron.py:4514
4. Scanner exemption for two deploy files — zero consumers, both files already fixed on base

Watch

• work_ledger._open_lock (work_ledger.py:532, 3 call sites) plus five deploy touch-"r+" preambles remain a second spelling of the helper this PR adds; the description's own complaint ("every merged fix re-derived its own preamble") still holds at 8 sites. Clears when: those sites use open_lock_file, or the divergence is accepted as deferred in Consolidate the non-truncating lock-open preamble into one shared helper #9267.
• Convert or exempt cron.py:4514. Clears when: it uses open_lock_file and the scan (widened to try_acquire_lock, larger window) names it on the pre-fix tree.

Subtractions

• Delete the exempt set in test_no_lock_site_opens_truncating — its own comment says drop it once the deploy fix merges, and this base shows it has (0 hits in deploy/).

[FIRST-PRINCIPLES-REVIEWED] cf7de1c

[jeeshofone]

Disposition (cf7de1c6e) — the empty-file locking finding is refuted by this PR's own Windows shard; the real Windows failure was in my test and is fixed

Finding ("msvcrt.locking cannot lock byte 0 of a missing/empty lock file, so operations raise instead of writing"): NOT TAKEN — the evidence points the other way. On this PR's own Windows shard, TestOpenLockFile::test_creates_missing_file_and_is_lockable — which creates the lock file EMPTY via open_lock_file and immediately takes an exclusive flock_exclusive on it — passed. Windows region locks may extend beyond end-of-file (documented _locking behavior), which is also why every merged sibling fix in this family (work-ledger, session_pid, issue_radar, hooks) produces empty lock files via touch-then-"r+" and holds locks on them in CI today. If empty-file locking raised, all of those would be red.

What WAS failing on the Windows shard — and is a good catch to arrive at, just not the stated one: test_preserves_existing_content read the lock file's bytes while holding the exclusive lock. Windows msvcrt region locks are MANDATORY, so the read answers EACCES — the test was accidentally asserting POSIX advisory semantics. Restructured: content is verified with the fd open but the lock NOT held (which is exactly the non-truncation property under test), plus after release; the lock is still taken with content present to prove lockability. Green locally; the Windows shard verifies the mandatory-lock path this push.

Also: PR Hygiene wanted an explicit Rule candidate: line in Pattern harvest — added to the description.

[iamwhatever]

Tier 1 auto-approve: fix (5 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: root cause is open(lock_path, "w") truncating a lock file before the lock is held (GH-9248); a new open_lock_file helper uses O_RDWR|O_CREAT and the three aws_control lock sites switch to it. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.