Skip to content

fix(lock): discard stale pre-open observations - #193

Merged
steipete merged 1 commit into
mainfrom
fix/lock-preopen-handoff-071
Sep 1, 2026
Merged

fix(lock): discard stale pre-open observations#193
steipete merged 1 commit into
mainfrom
fix/lock-preopen-handoff-071

Conversation

@steipete

@steipete steipete commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem and fix

The 0.7.1 preparation passed PR CI, but the merged-main gate caught a macOS Node 24 Root-async contention failure (path-mismatch, 79/100 completed acquisitions). Publication is still paused: no release tag has been pushed.

A deterministic regression on macOS and isolated Linux confirms a missing handoff case: a waiter samples holder A with pre-open lstat, A releases, and holder B creates the next record before the waiter opens it. Root correctly rejects the A/B identity mismatch. The lock contender previously had no way to discard this stale observation, even though it was not acquiring or removing anything from it.

Record a distinct, current-observation pre-open-change receipt only with strictly known, different, regular-file identities, no observed multiple links, and a stable descriptor reinspection. Only an EEXIST contention snapshot may consume that receipt, after the existing complete canonical-ancestry and current-leaf checks. It discards the observation and charges the normal retry budget; success still requires fresh exclusive creation. It does not read or adopt B's payload, infer that A was unlinked, or grant absence/removal/reclaim authority.

Generic Root reads remain strict. Post-create admission explicitly retains its narrower unlinked-only policy, and held/reclaim/cleanup reads do not opt into changed-observation discard. The creator's exact-byte/token check and original cleanup receipt remain unchanged. Production change: net +30 lines, with no public API, dependency, or native-code change.

Verification

  • Before: the real-filesystem regression fails at the strict pre-open/descriptor identity comparison on macOS and Linux.
  • After: 158 focused lock/identity/provenance/admission/ancestry tests pass, including 36 new regression/control cases.
  • Full native-backed pnpm check: 6,364 passed / 81 skipped.
  • pnpm test:security: 84 passed; locked Rust tests: 63 passed; Clippy with warnings denied passed.
  • Public contention: five repetitions of all four pathname/Root async/sync cases in each native mode, 6,000 completed acquisitions, with release-retry permission controls passing.
  • Docs build, documentation examples, npm/pnpm host-native package smoke, and whitespace/source consistency checks passed.
  • New controls cover stale pre-open samples whose old inode remains linked, generic/strict/unlinked-only readers, creator admission, retry exhaustion, ancestor/Root replacement, unsafe current leaves, observed multiple links, unknown/inexact/drifting/closed descriptors, and historical receipts.
  • The first expanded-test run exposed fixture setup errors (hooks are captured at open entry, and the Root directory must already exist). Those fixtures were corrected without weakening assertions or increasing deadlines. Full proof above is on the corrected frozen candidate.

Frozen tested tree: f572891984b3d03d79220c4c20a4a323f580bc6a.

The dated 0.7.1 changelog and lock documentation are updated as part of the explicitly requested in-progress release. All 0.7.0-and-older notes remain unchanged. The release tag will target latest main only after this fix lands and its prerequisites pass.

Keep ordinary Root reads and post-create admission strict while allowing bounded acquisition-only observation discard when a verified pre-open generation changes. Require exact descriptor and canonical ancestry evidence; never infer unlink, ownership, or removal authority. Add deterministic regression and boundary controls for the 0.7.1 release gate failure.
@steipete
steipete requested a review from a team as a code owner September 1, 2026 17:43
@clawsweeper

clawsweeper Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@steipete

steipete commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Independent Codex autoreview of the final frozen candidate completed scoped-clean at the configured P0 threshold, with no accepted/actionable findings. Reviewed and fully tested tree: f572891984b3d03d79220c4c20a4a323f580bc6a.

The new receipt remains observation-only: the waiter cannot read/adopt the replacement, infer pathname absence or unlink, or acquire release/reclaim authority. Post-create admission remains on its separate unlinked-only policy. All 36 new controls and the wider 158-test owner/provenance/ancestry set passed, followed by the full 6,364-test native-backed check and 6,000 contended acquisitions.

@clawsweeper

clawsweeper Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 1, 2026, 1:46 PM ET / 17:46 UTC.

ClawSweeper review

What this changes

The PR lets a Root-backed asynchronous lock contender discard a verified stale pre-open observation and retry exclusive creation without reading, adopting, or removing the successor lock.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

Ready for maintainer review

Keep open: this is a focused repair for a current-main Root-backed lock contention gap, with scoped identity checks and real-filesystem validation evidence; it remains necessary because current main lacks the new pre-open-change handling.

Priority: P1
Reviewed head: f93d787a7307581bf161c4c5fd15053b20f4d199

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, well-bounded lock-safety repair with credible real-filesystem proof and extensive boundary controls.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The changed production path is the Root lock contender's identity-mismatch handling; the PR supplies an actual filesystem before/after handoff scenario on macOS and Linux, reports the observed after-fix retry without successor parsing or reclamation, and its frozen tested tree exactly matches the reviewed head tree.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production path is the Root lock contender's identity-mismatch handling; the PR supplies an actual filesystem before/after handoff scenario on macOS and Linux, reports the observed after-fix retry without successor parsing or reclamation, and its frozen tested tree exactly matches the reviewed head tree.
Evidence reviewed 7 items Scoped stale-observation receipt: The new receipt is recorded only after an identity failure, known regular-file identities with at most one observed link, and a stable descriptor reinspection.
Acquisition-only consumption: Only the EEXIST contention snapshot requests the changed-observation policy; post-create admission explicitly continues to use the unlinked-only policy.
Boundary controls: The added real-filesystem tests retain the successor record, verify it is not parsed or reclaimed, and reject root, ancestor, leaf, hardlink, and identity-evidence violations.
Findings None None.
Security None None.

How this fits together

Sidecar locks coordinate exclusive work on files beneath a capability-style Root. A contending acquirer samples and opens a lock record, validates its identity and ancestry, then either safely retries creation or evaluates the current holder.

flowchart LR
A[Current lock holder] --> B[Contending lock waiter]
B --> C[Pre-open pathname sample]
C --> D[Opened descriptor identity check]
D --> E[Canonical ancestry recheck]
E --> F[Fresh exclusive lock creation]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus regression coverage production +30, tests +280 The bounded production change is accompanied by targeted handoff, strict-reader, ancestry, and retry-budget coverage.

Technical review

Best possible solution:

Merge the narrowly scoped acquisition-only retry once the pending native checks complete, retaining the strict generic-read, ancestry, and creator-admission boundaries.

Do we have a high-confidence way to reproduce the issue?

Yes. The PR body supplies a deterministic current-main Root-async handoff scenario on real macOS and Linux filesystems, and the added focused test encodes that path; this read-only review did not execute it.

Is this the best way to solve the issue?

Yes. Recording a narrowly scoped stale-generation receipt for the acquisition retry path fixes the handoff without relaxing generic Root reads or granting lock ownership, reclaim, or deletion authority.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3b425f1b2a1e.

Labels

Label changes:

  • add P1: A Root-backed asynchronous lock contention failure is blocking the in-progress release validation and can affect real lock acquisition workflows.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The changed production path is the Root lock contender's identity-mismatch handling; the PR supplies an actual filesystem before/after handoff scenario on macOS and Linux, reports the observed after-fix retry without successor parsing or reclamation, and its frozen tested tree exactly matches the reviewed head tree.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.

Label justifications:

  • P1: A Root-backed asynchronous lock contention failure is blocking the in-progress release validation and can affect real lock acquisition workflows.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production path is the Root lock contender's identity-mismatch handling; the PR supplies an actual filesystem before/after handoff scenario on macOS and Linux, reports the observed after-fix retry without successor parsing or reclamation, and its frozen tested tree exactly matches the reviewed head tree.

Evidence

What I checked:

  • Scoped stale-observation receipt: The new receipt is recorded only after an identity failure, known regular-file identities with at most one observed link, and a stable descriptor reinspection. (src/opened-file-failure.ts:19, f93d787a7307)
  • Acquisition-only consumption: Only the EEXIST contention snapshot requests the changed-observation policy; post-create admission explicitly continues to use the unlinked-only policy. (src/sidecar-lock-acquire.ts:296, f93d787a7307)
  • Boundary controls: The added real-filesystem tests retain the successor record, verify it is not parsed or reclaimed, and reject root, ancestor, leaf, hardlink, and identity-evidence violations. (test/sidecar-lock-root-preopen-boundary.test.ts:17, f93d787a7307)
  • Current-main necessity: Current main has no pre-open-change receipt and its contention read still uses the prior unlinked-only discard option, so the central behavior is not already implemented. (src/sidecar-lock-acquire.ts:296, 3b425f1b2a1e)
  • Proof tree identity: The PR body identifies frozen tested tree f572891984b3d03d79220c4c20a4a323f580bc6a, which is exactly the reviewed head commit's tree; the reported after-fix runs cover actual macOS/Linux filesystem handoff behavior and 6,000 public contention acquisitions. (test/sidecar-lock-root-preopen.test.ts:15, f93d787a7307)
  • Recent lock-area history: Recent current-main lock work includes the guarded-handoff and callback-retry fixes by the same area contributor. (src/sidecar-lock-acquire.ts:296, 790472f73bb4)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit 754f920 into main Sep 1, 2026
25 checks passed
@steipete

steipete commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Landed as 754f920d99af98b75c6b35c9e3f0bf37a1649dc9; its tree exactly matches the reviewed and isolated-tested candidate f572891984b3d03d79220c4c20a4a323f580bc6a.

Exact-head CI, coverage, CodeQL, and benchmarks passed. This includes the macOS Node 24 fallback contention lane that failed on the preceding main commit, plus Linux, Windows, native, musl, and package-install checks.

Before/after proof: deterministic pre-open handoff failed with the strict identity mismatch on macOS and isolated Linux; the final candidate passed 158 focused checks, 6,364 full native-backed tests (81 skipped), 84 security tests, 63 Rust tests, Clippy, docs, host package smoke, and 6,000 contended acquisitions. No test deadlines were increased. The final Codex autoreview was scoped-clean at the configured P0 threshold. Task-owned Linux proof was retained and its lease released.

The 0.7.1 release remains untagged pending the exact merged-main gate. No published package or tag was replaced to resolve this failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant