opencode: a cross-tenant manifest writer lock, and the eviction race it hides

Merged
IC
1mo ago
+2565-231

The cross-tenant writer lock for opencode-handles.json, in both languages, carrying a fix for a race the first version had.

Three repositories write that manifest — this one, anthropic-auth, and openai-auth (which vendors packages/client). Without a lock, a concurrent write drops another tenant's block. The lock is a mkdir on <manifest>.lock with an owner file inside it, a 30s TTL, and renewal by rewriting that owner every 10s.

The race, and why it is not a widened window

The first implementation quarantined a stale lock by renaming it to <lock>.stale-<claimed_at_ms>-<fresh random>. rename(2) has no identity precondition, so:

  1. Evictors A and B both read stale owner S0.
  2. A renames S0 away, re-mkdirs, publishes a fresh owner A1, starts working.
  3. B resumes holding its stale S0 observation and renames <lock> to a target named with a fresh random — which succeeds against A1, because nothing ties the rename to what B observed.
  4. A's release reads lock/owner, gets ENOENT, correctly treats it as a lost lease, and no-ops.
  5. B restores A1 to the canonical path.

The path now holds a fresh-looking lock whose owner has departed. Under the fixed test clock it never ages, so every subsequent claimant fails at the monotonic deadline with manifest lock busy — exactly 30.00s. In production it clears after one TTL, so it is a bounded loud failure rather than a permanent one, but it is real: 1 spontaneous failure in 50 runs under load 42–72.

The fix is a contract change, not a tuning change: the quarantine suffix is the observed owner's nonce. Every racer that saw S0 therefore targets one name, so the delayed loser's rename collides with an occupied, non-empty directory and EEXIST/ENOTEMPTY means "lost the race, retry". The target format and its regex are unchanged.

Evidence

The deterministic regression barriers after B reads S0, lets A evict/re-claim/hold, then lets B attempt its rename. Mutating the fix back to a fresh random turns it red with the production symptom:

aba_observation_cannot_rename_a_replacement_lock ... FAILED
panicked at opencode_files.rs:1146: called `Result::unwrap()` on an `Err` value: Invalid("manifest lock busy")
test result: FAILED. 1 passed; 1 failed; finished in 0.51s

Restored, both pass in 0.22s. I ran that mutation independently of the implementer rather than taking the report.

One honest limit: the second test — two evictors of one stale owner produce exactly one quarantine directory — passes with and without the fix. It documents intent; it is not evidence for the fix. The ABA test is the one that discriminates.

Constants are a frozen cross-repo contract

TTL 30000ms · owner keys exactly {tenant, pid, claimed_at_ms, nonce}, 0600, temp+rename inside the lock dir · renewal rewrites the owner every ≤10000ms · staleness judged from owner.claimed_at_ms only, never mtime · bounded jittered claiming (25–75ms) to a monotonic deadline, then the literal manifest lock busy · release removes the dir only if the nonce matches and the lease is unexpired, else logs lease-lost and no-ops · missing or unparseable owner is BUSY and never evicted · a symlink at the manifest path is refused. The writer preserves foreign tenant blocks structurally (parsed-value equality after a whole-document compact re-serialise), not byte-for-byte — tenants should not expect their formatting back.

anthropic-auth has already landed the same nonce-suffix fix on its side and its independently written regression fails against the pre-fix shape, which is a second implementation agreeing on the mechanism.

Scope

Lock only. migrate-plugin and mint-handle --out were in the same working branch and are deliberately not here; they follow separately. packages/opencode/src/handles.ts becomes a thin re-export of the client implementation because the locked writer belongs beside the handle-file reader — behaviour is preserved through an error-name-preserving shim.

One assertion from the working branch's test file was dropped: it pinned that contract regexes live only in the client package, which depends on a plugin refactor that is not in this PR. It travels with that refactor.

Gate green at a floor of 568, measured rather than copied. cli_opencode 56, ck-auth 25, bun typecheck + hermetic 167.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Adds a cross-tenant writer lock for opencode-handles.json in Rust and TypeScript. Concurrent tenant writes previously could overwrite provider blocks; they now serialize through a 30-second lease, while nonce-based quarantine prevents delayed stale-lock evictors from removing a replacement lock.

Bug Fixes

  • Renews leases every 10 seconds and uses bounded jittered retries before returning lock_busy.
  • Preserves foreign tenant blocks structurally and keeps distinct manifest paths independent.
  • Judges staleness at each observation, validates eviction-critical owner fields, and tolerates unknown keys and malformed diagnostics.
  • Adds stable lock_busy, owner_invalid, and renewal_failed codes; callback failures release the lock and re-raise the original error.
  • Reclaims quarantine directories after an mtime-based window and uses separator-safe basenames for Windows.
  • Fixes a Rust flake where release checked a different clock than acquisition stamped, so the lock directory could be left behind on a loaded machine; release now reads the same clock and the regression test is deterministic.
  • Moves shared handle parsing and locked writing into @cortexkit/claustrum-client while preserving OpenCode reader errors and refusing unsafe parent permissions.
  • Adds Rust and Bun regression coverage, path-component linting, and raises the workspace test floor to 608.