fix(lock): scope reentrancy to logical owners

[steipete]

Summary

• add reentrantOwner to async and sync file-lock acquisition so only the same explicitly named logical holder can reuse an in-process canonical lock
• reference-count owner-matched handles with idempotent releases, while different or absent owners wait under normal contention instead of entering the critical section
• restore the documented symlink-aliased session-lock shape without changing jsonStore, which remains ownerless and canonically queued

Follow-up to #63 and #60 after downstream consumer re-verification found the deliberate reentrant session-lock contract.

Compatibility and safety

The removed allowReentrant boolean does not return. Callers that intentionally nest acquisition must pass the same operation-scoped owner string through one manager domain. The docs explicitly warn that a process-wide/shared constant is unsafe because it would recreate the unrelated-task admission bug fixed by #63.

Async and sync acquisitions key held state by canonical target path. Matching defined owners increment one refcount and unlink only on the final idempotent release. Mismatched and missing owners are never admitted through that branch; a known live in-process holder is also not stale-reclaimed by its own manager.

Proof

• same owner across real and symlinked paths shares canonical target and sidecar identity; first and duplicate release preserve the sidecar, final release removes it
• different-owner and absent-owner concurrent read-modify-write tasks both wait and persist final count 2
• sync acquisition has owner/refcount, alias, timeout, and nested withFileLockSync parity
• a real child-process holder still blocks the parent until release
• the [Bug] jsonStore({ lock: true }) silently drops concurrent in-process updates: lock reentrancy is process-scoped, not owner-scoped #60 packaged jsonStore reproduction still persists count 3
• generated types accept reentrantOwner for async and sync while rejecting allowReentrant with TS2353

Validation

• pnpm check — 57 files, 622 passed, 7 skipped; package check included
• pnpm test:security — 62 passed
• pnpm docs:site
• packaged-artifact behavior validation — all owner, alias, release, cross-process, sync, JSON-store, and type-contract clauses passed
• autoreview — clean, no accepted/actionable findings

pnpm native:test could not run locally because this host has no cargo binary; native code is untouched, and hosted Rust CI is required before merge.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed July 27, 2026, 3:48 AM ET / 07:48 UTC.

ClawSweeper review

What this changes

This PR adds an owner-scoped reentrantOwner option to asynchronous and synchronous file locks so only acquisitions with the same logical owner can share a canonical in-process lock.

Merge readiness

⛔ Blocked until stronger real behavior proof is added - 5 items remain

Keep open for maintainer review: this is a substantial public lock-semantics change that appears to preserve the data-loss fix while restoring narrowly scoped nesting, but it needs maintainer acceptance of the new owner-token contract and stronger after-fix runtime evidence. Likely related people: steipete is the strongest routing candidate from the immediately preceding merged lock/store work (high confidence).

Priority: P2
Reviewed head: fa2f429aa247057a9127eee9448fc00a8bed1620
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The patch is focused and includes substantial regression coverage, but its real after-fix behavior proof is not yet inspectable from the PR body.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: The PR describes packaged-runtime validation, but it does not include a redacted runtime transcript, terminal capture, linked artifact, or other observable after-fix proof for the new owner-scoped behavior; add that evidence and redact private paths or identifiers. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The PR describes packaged-runtime validation, but it does not include a redacted runtime transcript, terminal capture, linked artifact, or other observable after-fix proof for the new owner-scoped behavior; add that evidence and redact private paths or identifiers. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	5 items	Current-main provenance: The immediately preceding merged lock/store change is merge commit 6009ba6253a7b38de6c8755e8666f8f3c3b2f12a; its stated behavior serializes jsonStore mutations and removes process-scoped reentrancy. This PR is a follow-up that adds a separate, owner-scoped contract rather than an already-implemented main-branch fix. Proposed lock lifecycle: The branch adds owner identity and reference counting to held asynchronous locks, returning a shared held-lock handle only for matching owners; different and absent owners continue through ordinary contention. Synchronous parity: The branch independently introduces global synchronous held-lock tracking, owner matching, reference-counted release, and idempotent handles, making the change broader than the already-merged asynchronous JSON-store repair.
Findings	None	None.
Security	None	None.

How this fits together

The sidecar-lock subsystem coordinates access to a target file by creating and validating a separate lock file. Public file-lock and JSON-store callers feed lock options into it; successful acquisition returns a release handle, while other callers retry, time out, or use stale-lock recovery.

flowchart LR
  A[Application lock caller] --> B[File-lock API]
  B --> C[Canonical target resolution]
  C --> D{Existing in-process holder?}
  D -->|Same logical owner| E[Reference-counted shared handle]
  D -->|Different or no owner| F[Sidecar contention and retry]
  F --> G[Exclusive sidecar lock]
  E --> H[Release final sidecar reference]
  G --> H

Loading

Decision needed

Question	Recommendation
Should reentrantOwner become the supported public mechanism for deliberately nested file-lock acquisition, including its synchronous API parity, rather than keeping all nested acquisition unsupported after the JSON-store fix?	Accept owner-scoped reentrancy: Adopt reentrantOwner as the documented opt-in contract, contingent on runtime proof and completed platform validation.

Why: The patch deliberately adds a new public behavioral contract after current main removed the unsound boolean; choosing the permanent reentrancy API and caller obligations requires maintainer intent beyond mechanical correctness.

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The PR describes packaged-runtime validation, but it does not include a redacted runtime transcript, terminal capture, linked artifact, or other observable after-fix proof for the new owner-scoped behavior; add that evidence and redact private paths or identifiers. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Resolve merge risk (P1) - reentrantOwner is a new public lock-semantics contract: a reused operation-wide owner value would again admit unrelated work into one critical section, so maintainers should explicitly accept the documented caller responsibility before merge.
• Resolve merge risk (P1) - The branch changes both asynchronous and synchronous release lifecycles; the failed Linux musl native check and remaining in-progress native checks should be resolved or understood before landing, even though the diff does not modify native code.
• Complete next step (P2) - The remaining blocker is maintainer acceptance of a new public lock contract plus contributor-supplied real behavior proof, not a narrow mechanical repair.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Lock surface	8 files affected; 514 added, 55 removed	The change spans both public lock APIs, shared lock lifecycle code, migration docs, and dedicated regression coverage.
New regression coverage	263 test lines added	The tests cover the central alias, owner-isolation, release, synchronous, and cross-process scenarios that determine lock safety.

Merge-risk options

Maintainer options:

1. Confirm the public owner-token contract (recommended)
   Approve the owner-scoped API only after reviewing its upgrade semantics and obtaining packaged-runtime proof that matching owners share exactly one lock while different and absent owners serialize.
2. Keep current main semantics
   Close or narrow the proposal if the repository does not want callers to carry a public logical-owner token for nested locks.

Technical review

Best possible solution:

Land a single documented owner-scoped reentrancy contract only after maintainers confirm that explicit caller-supplied owner strings are the intended public API and the contributor provides redacted packaged-runtime evidence for the owner-isolation and final-release behavior across supported platforms.

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

Unclear for the new behavior: the PR describes concrete packaged-runtime scenarios and adds focused tests, but the available PR body provides assertions rather than a redacted live transcript, artifact, or observable output for this branch.

Is this the best way to solve the issue?

Unclear: owner-scoped reference counting is a focused alternative to the removed process-scoped boolean and preserves the JSON-store safety boundary, but maintainers must first confirm that this new public API is preferable to leaving nesting unsupported.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6009ba6253a7.

Labels

Label changes:

• add P2: This is a bounded but compatibility-sensitive public lock API change with direct data-integrity implications for consumers that need nested locking.
• add merge-risk: 🚨 compatibility: Existing callers moving from the removed boolean to reentrantOwner must adopt a new ownership discipline, and incorrect shared owner values change contention behavior.
• add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
• add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR describes packaged-runtime validation, but it does not include a redacted runtime transcript, terminal capture, linked artifact, or other observable after-fix proof for the new owner-scoped behavior; add that evidence and redact private paths or identifiers. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

• P2: This is a bounded but compatibility-sensitive public lock API change with direct data-integrity implications for consumers that need nested locking.
• merge-risk: 🚨 compatibility: Existing callers moving from the removed boolean to reentrantOwner must adopt a new ownership discipline, and incorrect shared owner values change contention behavior.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR describes packaged-runtime validation, but it does not include a redacted runtime transcript, terminal capture, linked artifact, or other observable after-fix proof for the new owner-scoped behavior; add that evidence and redact private paths or identifiers. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Current-main provenance: The immediately preceding merged lock/store change is merge commit 6009ba6253a7b38de6c8755e8666f8f3c3b2f12a; its stated behavior serializes jsonStore mutations and removes process-scoped reentrancy. This PR is a follow-up that adds a separate, owner-scoped contract rather than an already-implemented main-branch fix. (src/json-document-store.ts:94, 6009ba6253a7)
• Proposed lock lifecycle: The branch adds owner identity and reference counting to held asynchronous locks, returning a shared held-lock handle only for matching owners; different and absent owners continue through ordinary contention. (src/sidecar-lock.ts:243, fa2f429aa247)
• Synchronous parity: The branch independently introduces global synchronous held-lock tracking, owner matching, reference-counted release, and idempotent handles, making the change broader than the already-merged asynchronous JSON-store repair. (src/file-lock-sync.ts:52, fa2f429aa247)
• Regression coverage: The branch adds focused tests for symlink aliases, same-owner reuse, different-owner contention, idempotent release, sync behavior, timeout behavior, and cross-process locking. (test/file-lock-reentrancy.test.ts:30, fa2f429aa247)
• Repository policy: The reviewed repository policy treats filesystem locking and public option contracts as security-sensitive, requiring conservative compatibility handling and focused regression coverage; the PR follows the focused-test and documentation direction but still requires review of the public contract. (AGENTS.md:1, 6009ba6253a7)

Likely related people:

• steipete: Authored the immediately preceding merged JSON-store serialization work and this follow-up owner-scoped lock proposal, connecting them to the current lock and store behavior. (role: recent area contributor; confidence: high; commits: 6009ba6253a7, fa2f429aa247; files: src/json-document-store.ts, src/sidecar-lock.ts, src/file-lock-sync.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Post a redacted packaged-runtime transcript or terminal capture showing same-owner alias reuse, different-owner serialization, idempotent release, and final sidecar removal.
• Resolve or explain the Linux musl native-check failure before merge.

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.