opencode: a cross-tenant manifest writer lock, and the eviction race it hides - #33
opencode: a cross-tenant manifest writer lock, and the eviction race it hides#33iceteaSA wants to merge 5 commits into
Conversation
…it hides Stale evictors could observe S0, then rename a replacement holder's fresh lock because each chose a fresh quarantine suffix.\n\nQuarantine targets now derive from the observed owner's nonce, so racers collide on one occupied target and retry after EEXIST/ENOTEMPTY.\n\nThe Rust and TypeScript implementations share the frozen TTL, renewal, owner-record, retry, release, and tenant-preservation contract.
|
Correcting my own PR description before anyone reviews it on the wrong premise. The TypeScript lock in this commit is not the previously-worked file with a patch applied. It is a reimplementation of the same contract. I described the scope as "extract the lock from the earlier working branch", and at the file level that is what happened — but comparing token streams rather than file names, I found this only because a downstream tenant asked whether one vendor from this commit would get them both the lock fix and the handles reader, which made me diff content instead of checking which files changed. My earlier verification confirmed the right files moved and that the ABA fix was present in both languages — it did not confirm the code around the fix was the code I said it was. What I can assert, having checked each:
So please review the TypeScript half as new code rather than as a re-application of something already looked at. The Rust half and the fix itself stand on the evidence above.
|
|
Correcting a claim in my own correction, which is where this belongs rather than in a reply thread. I wrote that The I have not verified The correction I originally posted said the TypeScript half should be reviewed as new code. This makes that sharper rather than softer: for "the reviewed file plus a patch", absent third-party pins are a gap. For a 302→105 reimplementation, those pins were the thing that would have made behavioural equivalence checkable by someone other than its author, and they do not exist. So the evidence actually standing behind the rewrite is:
Nothing covers owner-key validation, TTL/renewal, symlink refusal, or the release path beyond this repo's own suite. I would rather state that than let a reader infer third-party validation from the word "tenants". The |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/client/src/handles.ts">
<violation number="1" location="packages/client/src/handles.ts:136">
P3: The new client implementation duplicates `packages/opencode/src/bounded-read.ts`, so future fixes to bounded descriptor reads can diverge between handle and auth paths. Move this generic helper to a shared client export and have the OpenCode auth reader use it instead of keeping two copies.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| mtimeMs?: number | ||
| } | ||
|
|
||
| async function readBounded(descriptor: HandleFileDescriptor, cap: number): Promise<{ buffer: Buffer; bytes: number }> { |
There was a problem hiding this comment.
P3: The new client implementation duplicates packages/opencode/src/bounded-read.ts, so future fixes to bounded descriptor reads can diverge between handle and auth paths. Move this generic helper to a shared client export and have the OpenCode auth reader use it instead of keeping two copies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/handles.ts, line 136:
<comment>The new client implementation duplicates `packages/opencode/src/bounded-read.ts`, so future fixes to bounded descriptor reads can diverge between handle and auth paths. Move this generic helper to a shared client export and have the OpenCode auth reader use it instead of keeping two copies.</comment>
<file context>
@@ -0,0 +1,223 @@
+ mtimeMs?: number
+}
+
+async function readBounded(descriptor: HandleFileDescriptor, cap: number): Promise<{ buffer: Buffer; bytes: number }> {
+ if (!descriptor.read) throw new Error('readBounded requires a descriptor exposing read()')
+ const buffer = Buffer.alloc(cap + 1)
</file context>
A tenant classifying a lock failure had only the message text to branch on: distinguishing 'busy, retry later' from 'the owner artefact is wrong' from 'the write was abandoned' meant string-matching our prose, so a copy-edit would silently reclassify a retryable busy-lock as an unknown error with nothing failing loudly. Failures now carry MANIFEST_LOCK.errorCodes -- lock_busy, owner_invalid, renewal_failed -- and the message is explicitly diagnostic. Requested by the openai-auth seat before it writes a consumer-side conformance suite, which is the cheap moment: pinning the strings first would make a later move to codes a breaking change for its tests. Also pins two behaviours the contract relied on without stating: a throwing callback releases the lock and re-raises the original error unwrapped (release sits in a finally, so an enroll path that refuses by throwing costs one operation rather than wedging every tenant for a TTL), and distinct manifest paths do not contend.
|
Second commit, The Failures now carry a stable code, with the closed set exported as The message becomes explicitly diagnostic and free to reword — which is less work to keep honest than three frozen strings. Why now rather than a follow-up: if they pin the strings first, moving to codes later is a breaking change for their suite. Cheap now, expensive in two days. I would rather explain a second commit than have you review this file twice in a week — but you set the scope rule after #28 and I am not going to quietly widen a PR under it, so: this is the last commit here unless review asks for one. Also pinned, both pre-existing and both undocumented, which is how the question surfaced:
Mutation-proved, since a pin nobody has broken is not yet a test: stripping the code assignment (asserted to be exactly one site) turns both code tests red with One thing this does not change: the ABA fix and its reproduction are still the only part of the TypeScript half carrying evidence from outside this repo. The rest of that file remains a rewrite reviewed by nobody but me. |
The test awaited the re-acquire and measured afterwards, so a regression in release-on-throw would block for the full 30s TTL and surface as a suite-level timeout with no attribution -- indistinguishable from a slow box or a hang elsewhere. A fault and an environment condition sharing one symptom is the defect this suite exists to catch, so it should not be the harness's own failure mode. The re-acquire is now raced against a bounded timer whose arm names the property, and the probe that produced the original number (3ms against a 30000ms wedge) leaves three orders of magnitude of headroom. Found by the openai-auth seat reviewing the pin before writing its own.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/client/src/manifest-lock.ts">
<violation number="1" location="packages/client/src/manifest-lock.ts:10">
P2: The new lock error types are not reachable from the package entrypoint. Re-export `ManifestLockErrorCode` and `ManifestLockError` from `packages/client/src/index.ts` so consumers can type their stable `error.code` handling.</violation>
<violation number="2" location="packages/client/src/manifest-lock.ts:31">
P2: Malformed owner records never produce the advertised `owner_invalid` code; acquisition converts them to `lock_busy`, while renewal converts them to `renewal_failed`. Remove the unreachable code or preserve it for callers.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-[A-Za-z0-9_-]+$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } | ||
|
|
||
| /** Thrown by the lock. Branch on `code`; the message is diagnostic and may be reworded. */ | ||
| export type ManifestLockErrorCode = (typeof MANIFEST_LOCK.errorCodes)[number] |
There was a problem hiding this comment.
P2: The new lock error types are not reachable from the package entrypoint. Re-export ManifestLockErrorCode and ManifestLockError from packages/client/src/index.ts so consumers can type their stable error.code handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/manifest-lock.ts, line 10:
<comment>The new lock error types are not reachable from the package entrypoint. Re-export `ManifestLockErrorCode` and `ManifestLockError` from `packages/client/src/index.ts` so consumers can type their stable `error.code` handling.</comment>
<file context>
@@ -4,7 +4,11 @@ import { randomBytes, randomInt } from 'node:crypto'
+export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-[A-Za-z0-9_-]+$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const }
+
+/** Thrown by the lock. Branch on `code`; the message is diagnostic and may be reworded. */
+export type ManifestLockErrorCode = (typeof MANIFEST_LOCK.errorCodes)[number]
+export type ManifestLockError = Error & { code: ManifestLockErrorCode }
export type ManifestHandleAccount = OpenCodeHandleFileV1['providers'][number]['accounts'][number]
</file context>
| const value = JSON.parse(source) as unknown | ||
| if (!value || typeof value !== 'object') throw lockError('owner_invalid', 'manifest lock owner invalid') | ||
| const owner = value as Record<string, unknown> | ||
| if (Object.keys(owner).sort().join('\0') !== [...MANIFEST_LOCK.ownerKeys].sort().join('\0') || typeof owner.tenant !== 'string' || typeof owner.pid !== 'number' || !Number.isInteger(owner.pid) || typeof owner.claimed_at_ms !== 'number' || !Number.isFinite(owner.claimed_at_ms) || typeof owner.nonce !== 'string') throw lockError('owner_invalid', 'manifest lock owner invalid') |
There was a problem hiding this comment.
P2: Malformed owner records never produce the advertised owner_invalid code; acquisition converts them to lock_busy, while renewal converts them to renewal_failed. Remove the unreachable code or preserve it for callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/manifest-lock.ts, line 31:
<comment>Malformed owner records never produce the advertised `owner_invalid` code; acquisition converts them to `lock_busy`, while renewal converts them to `renewal_failed`. Remove the unreachable code or preserve it for callers.</comment>
<file context>
@@ -15,12 +19,16 @@ export function __setManifestLockTestOptions(options?: TestOptions): void { test
+ if (!value || typeof value !== 'object') throw lockError('owner_invalid', 'manifest lock owner invalid')
const owner = value as Record<string, unknown>
- if (Object.keys(owner).sort().join('\0') !== [...MANIFEST_LOCK.ownerKeys].sort().join('\0') || typeof owner.tenant !== 'string' || typeof owner.pid !== 'number' || !Number.isInteger(owner.pid) || typeof owner.claimed_at_ms !== 'number' || !Number.isFinite(owner.claimed_at_ms) || typeof owner.nonce !== 'string') throw new Error('manifest lock owner invalid')
+ if (Object.keys(owner).sort().join('\0') !== [...MANIFEST_LOCK.ownerKeys].sort().join('\0') || typeof owner.tenant !== 'string' || typeof owner.pid !== 'number' || !Number.isInteger(owner.pid) || typeof owner.claimed_at_ms !== 'number' || !Number.isFinite(owner.claimed_at_ms) || typeof owner.nonce !== 'string') throw lockError('owner_invalid', 'manifest lock owner invalid')
return owner as Owner
}
</file context>
… it builds a path from A contender that arrived while an owner was fresh could exhaust its retry window after that owner died, because staleness was frozen at claim start. Judge each observation against the current clock; renewal remains what protects a healthy owner. Validate eviction-critical timestamps and nonces before constructing quarantine paths, surface permanently corrupt owners as owner_invalid, and tolerate unknown keys plus malformed diagnostic fields so independently upgraded readers do not wedge on a healthy newer writer. Exact-key matching was the defect, not a safety property. Nonce validation rejects only path-unsafe shapes and keeps the quarantine regex aligned with that rule, so future path-safe nonce alphabets remain evictable without changing the cross-version ABA target. Leave pre-existing parent modes unchanged while refusing group- or other-writable parents, and re-export ManifestLockError plus ManifestLockErrorCode from the package entrypoint.
There was a problem hiding this comment.
4 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/client/src/manifest-lock.ts">
<violation number="1" location="packages/client/src/manifest-lock.ts:7">
P2: When an owner contains a path-safe non-ASCII nonce near the regex limit, the quarantine rename fails with `ENAMETOOLONG` instead of entering the retry path. Restrict the nonce to a single-byte path-safe alphabet or validate the encoded filename length before accepting the owner.</violation>
<violation number="2" location="packages/client/src/manifest-lock.ts:35">
P2: When an owner record gains an extra field, this reader evicts it once stale, but the Rust reader rejects it because `ManifestLockOwner` uses `#[serde(deny_unknown_fields)]`. A Rust tenant therefore leaves the stale lock busy indefinitely while a TypeScript tenant removes it. Keep unknown-field handling consistent across every lock reader.</violation>
<violation number="3" location="packages/client/src/manifest-lock.ts:61">
P2: When a lock becomes stale after a contender's first observation, this client now evicts it, but the Rust writer still compares `started_at_ms` instead of the current time. Rust contenders can therefore wait until their deadline and report busy while TypeScript contenders make progress. Update the other lock implementation to judge staleness at each observation as well.</violation>
</file>
<file name="packages/client/src/tests/manifest-lock.test.ts">
<violation number="1" location="packages/client/src/tests/manifest-lock.test.ts:170">
P3: The renewal interval rewrites the owner file every 5ms with non-atomic `writeFile` (truncate-then-write), while `withLockCommit` reads it every retry. A read that catches the file mid-truncation makes `parseOwner` throw `owner_invalid`, which `withLockCommit` re-throws at the deadline instead of `lock_busy`, so `rejects.toThrow('manifest lock busy')` can flake. Write the renewed owner atomically (temp file + rename) as the implementation's `writeOwner` does, so readers never observe a partial record.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| let observed: Owner | undefined, ownerReadError: unknown | ||
| try { observed = await readOwner(ownerPath) } catch (error) { ownerReadError = error; if (code(error) !== 'ENOENT' && Date.now() >= deadline) throw code(error) === 'owner_invalid' ? error : lockError('lock_busy', 'manifest lock busy') } | ||
| if (observed && Date.now() - observed.claimed_at_ms >= ttl) { |
There was a problem hiding this comment.
P2: When a lock becomes stale after a contender's first observation, this client now evicts it, but the Rust writer still compares started_at_ms instead of the current time. Rust contenders can therefore wait until their deadline and report busy while TypeScript contenders make progress. Update the other lock implementation to judge staleness at each observation as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/manifest-lock.ts, line 61:
<comment>When a lock becomes stale after a contender's first observation, this client now evicts it, but the Rust writer still compares `started_at_ms` instead of the current time. Rust contenders can therefore wait until their deadline and report busy while TypeScript contenders make progress. Update the other lock implementation to judge staleness at each observation as well.</comment>
<file context>
@@ -52,9 +56,9 @@ async function withLockCommit<T>(path: string, tenant: string, fn: (commit: () =
- if (observed && started - observed.claimed_at_ms >= ttl) {
+ let observed: Owner | undefined, ownerReadError: unknown
+ try { observed = await readOwner(ownerPath) } catch (error) { ownerReadError = error; if (code(error) !== 'ENOENT' && Date.now() >= deadline) throw code(error) === 'owner_invalid' ? error : lockError('lock_busy', 'manifest lock busy') }
+ if (observed && Date.now() - observed.claimed_at_ms >= ttl) {
await testOptions?.beforeEvict?.()
const stale = `${lock}.stale-${observed.claimed_at_ms}-${observed.nonce}`
</file context>
| // Widen the nonce alphabet only after every tenant has this path-safe reader; an older | ||
| // allowlist reader can otherwise wedge forever on the first owner using the new alphabet. | ||
| const staleTarget = `.lock.stale-${owner.claimed_at_ms}-${owner.nonce}` | ||
| if (MANIFEST_LOCK.ownerKeys.some((key) => !Object.hasOwn(owner, key)) || typeof owner.claimed_at_ms !== 'number' || !Number.isInteger(owner.claimed_at_ms) || owner.claimed_at_ms < 0 || typeof owner.nonce !== 'string' || !MANIFEST_LOCK.staleTargetRe.test(staleTarget)) throw lockError('owner_invalid', 'manifest lock owner invalid') |
There was a problem hiding this comment.
P2: When an owner record gains an extra field, this reader evicts it once stale, but the Rust reader rejects it because ManifestLockOwner uses #[serde(deny_unknown_fields)]. A Rust tenant therefore leaves the stale lock busy indefinitely while a TypeScript tenant removes it. Keep unknown-field handling consistent across every lock reader.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/manifest-lock.ts, line 35:
<comment>When an owner record gains an extra field, this reader evicts it once stale, but the Rust reader rejects it because `ManifestLockOwner` uses `#[serde(deny_unknown_fields)]`. A Rust tenant therefore leaves the stale lock busy indefinitely while a TypeScript tenant removes it. Keep unknown-field handling consistent across every lock reader.</comment>
<file context>
@@ -25,10 +25,14 @@ const sleep = async (ms: number) => new Promise((resolve) => setTimeout(resolve,
+ // Widen the nonce alphabet only after every tenant has this path-safe reader; an older
+ // allowlist reader can otherwise wedge forever on the first owner using the new alphabet.
+ const staleTarget = `.lock.stale-${owner.claimed_at_ms}-${owner.nonce}`
+ if (MANIFEST_LOCK.ownerKeys.some((key) => !Object.hasOwn(owner, key)) || typeof owner.claimed_at_ms !== 'number' || !Number.isInteger(owner.claimed_at_ms) || owner.claimed_at_ms < 0 || typeof owner.nonce !== 'string' || !MANIFEST_LOCK.staleTargetRe.test(staleTarget)) throw lockError('owner_invalid', 'manifest lock owner invalid')
return owner as Owner
}
</file context>
| import { dirname, join } from 'node:path' | ||
| import { HANDLE_FILE_CONTRACT, parseHandleFile, type OpenCodeHandleFileV1 } from './handles.js' | ||
|
|
||
| export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-(?!\.{1,2}$)(?!.*[. ]$)[^/\\\x00-\x1f:*?"<>|]{1,128}$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } |
There was a problem hiding this comment.
P2: When an owner contains a path-safe non-ASCII nonce near the regex limit, the quarantine rename fails with ENAMETOOLONG instead of entering the retry path. Restrict the nonce to a single-byte path-safe alphabet or validate the encoded filename length before accepting the owner.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/manifest-lock.ts, line 7:
<comment>When an owner contains a path-safe non-ASCII nonce near the regex limit, the quarantine rename fails with `ENAMETOOLONG` instead of entering the retry path. Restrict the nonce to a single-byte path-safe alphabet or validate the encoded filename length before accepting the owner.</comment>
<file context>
@@ -4,7 +4,7 @@ import { randomBytes, randomInt } from 'node:crypto'
import { HANDLE_FILE_CONTRACT, parseHandleFile, type OpenCodeHandleFileV1 } from './handles.js'
-export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-[A-Za-z0-9_-]+$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const }
+export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-(?!\.{1,2}$)(?!.*[. ]$)[^/\\\x00-\x1f:*?"<>|]{1,128}$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const }
/** Thrown by the lock. Branch on `code`; the message is diagnostic and may be reworded. */
</file context>
| export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-(?!\.{1,2}$)(?!.*[. ]$)[^/\\\x00-\x1f:*?"<>|]{1,128}$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } | |
| export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-(?!\.{1,2}$)(?!.*[. ]$)[^/\\\x00-\x1f\x7f-\uffff:*?"<>|]{1,128}$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } |
| const ownerPath = join(`${path}.lock`, 'owner') | ||
| const current = JSON.parse(await readFile(ownerPath, 'utf8')) as Record<string, unknown> | ||
| current.claimed_at_ms = Date.now() | ||
| await writeFile(ownerPath, `${JSON.stringify(current)}\n`, { mode: 0o600 }) |
There was a problem hiding this comment.
P3: The renewal interval rewrites the owner file every 5ms with non-atomic writeFile (truncate-then-write), while withLockCommit reads it every retry. A read that catches the file mid-truncation makes parseOwner throw owner_invalid, which withLockCommit re-throws at the deadline instead of lock_busy, so rejects.toThrow('manifest lock busy') can flake. Write the renewed owner atomically (temp file + rename) as the implementation's writeOwner does, so readers never observe a partial record.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/tests/manifest-lock.test.ts, line 170:
<comment>The renewal interval rewrites the owner file every 5ms with non-atomic `writeFile` (truncate-then-write), while `withLockCommit` reads it every retry. A read that catches the file mid-truncation makes `parseOwner` throw `owner_invalid`, which `withLockCommit` re-throws at the deadline instead of `lock_busy`, so `rejects.toThrow('manifest lock busy')` can flake. Write the renewed owner atomically (temp file + rename) as the implementation's `writeOwner` does, so readers never observe a partial record.</comment>
<file context>
@@ -85,14 +86,93 @@ describe('manifest writer lock', () => {
+ const ownerPath = join(`${path}.lock`, 'owner')
+ const current = JSON.parse(await readFile(ownerPath, 'utf8')) as Record<string, unknown>
+ current.claimed_at_ms = Date.now()
+ await writeFile(ownerPath, `${JSON.stringify(current)}\n`, { mode: 0o600 })
+ }, 5)
+ try {
</file context>
|
Gated green at The ABA fix is right and your evidence for it holds. I checked the quarantine name is built from the observed owner rather than a fresh value, which is the whole contract change: format!("{}.stale-{}-{}", lock.display(), observed.claimed_at_ms, observed.nonce)Every racer that saw S0 targets one name, so the delayed loser hits an occupied directory instead of renaming a live holder away. That is the mechanism, and your Your two self-corrections are the reason this review could be shaped properly: reviewing the TypeScript half as new code, and withdrawing the third-party-pin claim, both changed what I looked at. Two places the two languages disagree, and both are new in this diffYou describe the constants as a frozen cross-repo contract. These are inside that contract, and inside this PR's own diff — 1. An owner with an extra key wedges the Rust side permanentlyThe TypeScript side checks the required keys are present and tolerates extras. The Rust side refuses them. This is not an oversight on one side — your TS suite has a test for it by name, Reproduced rather than argued, on an owner carrying one extra diagnostic field: The consequence is unbounded, and that is what makes this the one I would fix before merge. The claim loop reads the owner as Three repos writing one artefact is exactly the situation where one of them adds a field. 2. Staleness is judged against different clocks
The case that separates them: a lock that is fresh when I start and ages past the TTL while I retry — an owner claimed 25s ago that dies as I arrive. TypeScript sees 30s at T+5s and evicts. Rust computes 25s for the entire window and fails at the deadline with Bounded and loud, unlike the first one — the next invocation starts with a stale observation and evicts correctly. Worth fixing because your contract text says staleness is judged from What I am asking forThe first one before merge, because its failure mode is permanent and manual. The second because it is two lines away and the contract sentence needs the clock named either way. Both are Rust-side and both are in this diff, so this is not a new commit's worth of scope. If you would rather land the lock as it stands and fix these in the follow-up that carries Not blocking, recorded
|
…aleness at observation The TypeScript reader already treated owner keys as required-not-exclusive and judged staleness against the current clock (71f927f); the Rust reader still refused unknown keys (deny_unknown_fields) and compared against the clock captured at claim start. The first wedges every Rust contender permanently the moment any tenant adds a diagnostic field; the second makes a lock that ages past TTL during retries fail busy at the deadline instead of evicting. Both pinned RED-first; quarantine name format unchanged.
|
Both fixed here, You caught the thing I had written down and implemented on one side only: "owner keys are required, not exclusive" was pinned by name in the TypeScript suite and contradicted by 1. Unknown / malformed owner fields. 2. Staleness clock. Judged against Four tests, RED first: ENAMETOOLONG: agreed, not worth a commit. The blind conformance suite from openai-auth lands after this merge, as you say. |
…on ladder lands Fifth sibling wave this session, and this one carries the fix for the defect I found at 4f8b1f8: subc-transport 0.6.0 exposes `connection_file::discover(explicit)` and `discovery_candidates(explicit, env_named)` — the READER's ladder, callable, with the exclusive SUBC_CONNECTION_FILE semantics inside the helper rather than left to callers. Lock-only here. Converting this CLI's copy into a call is a separate change with its own test, not something to fold into a dependency bump — the copy is currently correct and the conversion has to prove the rungs still agree. HOW IT SURFACED, because the diagnosis was wrong twice before it was right: 1. PR #33's gate failed on clippy. Read as the usual stale-branch lock. 2. Restored MASTER's Cargo.lock at their head to isolate it. STILL FAILED — which reads as "their Rust is broken", and I nearly reported that. 3. Neither lock satisfied the manifests, and no manifest in the PR had changed. That points away from the branch entirely: a wave had landed WHILE I WAS GATING. MY ISOLATION TECHNIQUE ASSUMES MASTER IS CURRENT, and that assumption is invisible in its result. When a wave lands mid-gate both locks are stale, the control fails identically to the subject, and the reading flips from "your branch is behind" to "your code is broken" with nothing in the output distinguishing them. The discriminator is comparing the lock against the sibling manifests on disk rather than against master — one is a claim about who is behind, the other about what is required.
1 participant
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, andopenai-auth(which vendorspackages/client). Without a lock, a concurrent write drops another tenant's block. The lock is amkdiron<manifest>.lockwith 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:<lock>to a target named with a fresh random — which succeeds against A1, because nothing ties the rename to what B observed.lock/owner, getsENOENT, correctly treats it as a lost lease, and no-ops.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/ENOTEMPTYmeans "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:
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 fromowner.claimed_at_msonly, never mtime · bounded jittered claiming (25–75ms) to a monotonic deadline, then the literalmanifest 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-authhas 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-pluginandmint-handle --outwere in the same working branch and are deliberately not here; they follow separately.packages/opencode/src/handles.tsbecomes 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_opencode56,ck-auth25, bun typecheck + hermetic 167.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a cross-tenant writer lock for
opencode-handles.jsonin Rust and TypeScript so concurrent tenant writes no longer overwrite each other's provider blocks. Quarantine targets for stale-lock eviction derive from the observed owner nonce, so a delayed evictor can't rename a replacement holder's fresh lock; staleness is judged at each observation against the current clock, and lock failures carry stablelock_busy/owner_invalid/renewal_failedcodes instead of requiring message-string matching.Bug Fixes
manifest lock busyafter the deadline; staleness is judged at each observation so a contender arriving while the owner is fresh retries correctly afterward.owner_invalid, and tolerates unknown keys or malformed diagnostic fields so upgraded readers don't wedge.@cortexkit/claustrum-clientwhile preservingpackages/opencodereader errors; pre-existing parent modes stay unchanged and group- or other-writable parents are refused.Written for commit 839326c. Summary will update on new commits.