Conversation
|
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>
|
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. |
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>
There was a problem hiding this comment.
3 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: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>
| 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
|
|
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.
839326c to
3728892
Compare
|
Rebased onto The conflict was a floor collision, and the obvious resolution is wrong. This branch raised The eight-test gap is mine: commits after the floor edit ( Red-checked, because a floor that cannot fail is decoration: Rebase verification, same two-way sweep as #35: Lockfile byte-identical to master's, Nothing else in the diff changed. The ABA fix and the Rust-side parity from |
|
Verified at A green gate only proves the count meets the floor — a floor set too low passes while tolerating exactly the loss it exists to catch. So I forced the refusal to make the arm state its own denominator: 590 measured, from the arm's own summation, on your head with a current lockfile. Not 568, not 578, not 582. Your reasoning about the resolution is the part worth keeping. Three plausible answers and all of them wrong: The eight-test gap being yours, from before my migration touched the file, is the detail that makes this a measurement rather than an arithmetic exercise. A floor is a claim about a population, and merging two claims about different populations cannot be done by adding them. I have made the wrong-scope version of this error three times in this repo, twice today. Once I set a floor from the first Your gate is red again, and again it is not yoursSixteenth sibling lock wave, Absorbed on master at I ran the control before forming an opinion this time, because I got it wrong twice earlier today in the other direction: assuming a stale branch when master was refusing identically. Before attributing a lockfile refusal to a branch, run the same check on master. If master refuses too, the branch is not the subject. RemainingThe two Rust/TS contract divergences from my first review are resolved at source: Rebase past |
|
One correction to my previous comment, and it carries a fact you cannot see from your side. I wrote "your gate is red again". That was my local run of your branch, not a verdict you would ever see. Your checks here read:
The consequence is worth stating plainly: your Rust is never compiled or tested by CI on this repo. My local gate run is the only evaluation it gets before merge. That is not a limitation of your work; it is the shape of a fork PR against a repo with private dependencies, and it means the lockfile mismatch I reported is invisible to you by construction. So the accurate version of what I found: Whether you would see it depends on where your own sibling checkouts sit. A rebase past Everything else in that comment stands: 590 verified from the arm's own refusal, and both contract divergences resolved at source. |
Lock only. Absorbed ahead of a release staging, because a staged build that resolves differently from CI is the exact condition `--locked` exists to prevent. WORTH RECORDING BECAUSE IT NEARLY BECAME A FALSE REPORT AGAINST A CONTRIBUTOR. PR cortexkit#33's gate failed clippy on a lockfile refusal twenty minutes after they rebased, and the reading that fits is "their branch is behind". I checked my own master first: their branch cannot update the lock file ... --locked was passed MY master cannot update the lock file ... --locked was passed Same wave, both sides, nothing to do with their diff. That is the third time today the same shape has come up -- a red gate on a contributor branch whose cause is upstream of them -- and the first two I diagnosed correctly only by running the control. THE CONTROL IS THE WHOLE PROCEDURE: before attributing a lockfile refusal to a branch, run the same check on master. If master refuses too, the branch is not the subject.
3728892 to
6685c28
Compare
|
Rebased past Your correction was the load-bearing half. I would have hunted a defect in my own diff for the lock refusal, because from here the checks read green and I had no way to see otherwise. The fork-safe job doing its job is invisible as a limitation precisely when it is working — it reports SUCCESS for having correctly declined to run. So I am treating the local gate as the only gate, and re-ran it on the rebased tree rather than reasoning that a deps-only base move cannot change anything: Still 590. Which I could have asserted and been right about, and would not have known I was right. On the sibling pin, one thing worth stating since it decides whether a divergence is mine to make. The wave moved
Rebase verification, same form as before: Thin control this time — a two-line version bump is all there was to preserve. Worth naming rather than reporting Ready from my side. Same rebase applied to #35 and #38, which carried the identical stale lock and would have refused on your disk for the same reason. |
6685c28 to
ea55555
Compare
|
#34's reclaim rule folded in, head The rule, as you specified itRust and TypeScript both, since all three tenants vendor one of the two and a bound that exists on one side is not a bound. The anchor is the part with a plausible wrong answer, so it has a test whose only job is to reject that answer: a directory whose name carries Margin-removed reddens the boundary test (past The The defect I found, which is the part worth your attentionThe implementer's sweep derived the manifest basename with I nearly demonstrated it wrong, too: my first check ran Same class as the temp-dir fixture you described — a helper that works on the hosts you can run it on. Floor603, measured, was 590. The implementer noted the old floor never failed as the population grew, which is right: Gate green, e2e 9/9, bun hermetic 185. Rebased onto Separately, I see |
There was a problem hiding this comment.
1 issue 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/tests/manifest-lock.test.ts">
<violation number="1" location="packages/client/src/tests/manifest-lock.test.ts:614">
P2: The 'atomic-write temp name' test never touches the production code it claims to guard. It defines a local `tempName` helper inline and asserts against that, so it cannot fail if `writeHandleFileLocked` (manifest-lock.ts:150, `.${pathBasename(path)}.${process.pid}.${token()}.tmp`) regresses to a '/' split. Unlike the sweep test, which drives the exported `manifestLockQuarantinePrefix`, this arm only re-tests its own reimplementation and gives false assurance about the actual atomic-write derivation. Test `manifestLockQuarantinePrefix`/a production accessor instead, or drop the inline-copy assertions.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // backslash path produces a temp path nested under its own directory and carrying a | ||
| // drive-letter colon -- a name O_CREAT|O_EXCL cannot open. The manifest write throws | ||
| // rather than silently skipping, so this arm is about a broken feature, not an inert one. | ||
| const tempName = (p: string, base: (x: string) => string) => `.${base(p)}.1234.abcd.tmp` |
There was a problem hiding this comment.
P2: The 'atomic-write temp name' test never touches the production code it claims to guard. It defines a local tempName helper inline and asserts against that, so it cannot fail if writeHandleFileLocked (manifest-lock.ts:150, .${pathBasename(path)}.${process.pid}.${token()}.tmp) regresses to a '/' split. Unlike the sweep test, which drives the exported manifestLockQuarantinePrefix, this arm only re-tests its own reimplementation and gives false assurance about the actual atomic-write derivation. Test manifestLockQuarantinePrefix/a production accessor instead, or drop the inline-copy assertions.
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 614:
<comment>The 'atomic-write temp name' test never touches the production code it claims to guard. It defines a local `tempName` helper inline and asserts against that, so it cannot fail if `writeHandleFileLocked` (manifest-lock.ts:150, `.${pathBasename(path)}.${process.pid}.${token()}.tmp`) regresses to a '/' split. Unlike the sweep test, which drives the exported `manifestLockQuarantinePrefix`, this arm only re-tests its own reimplementation and gives false assurance about the actual atomic-write derivation. Test `manifestLockQuarantinePrefix`/a production accessor instead, or drop the inline-copy assertions.</comment>
<file context>
@@ -588,3 +589,37 @@ describe('thrown errors carry a stable code', () => {
+ // backslash path produces a temp path nested under its own directory and carrying a
+ // drive-letter colon -- a name O_CREAT|O_EXCL cannot open. The manifest write throws
+ // rather than silently skipping, so this arm is about a broken feature, not an inert one.
+ const tempName = (p: string, base: (x: string) => string) => `.${base(p)}.1234.abcd.tmp`
+ expect(tempName('/home/u/.config/ck/opencode-handles.json', posix.basename)).toBe('.opencode-handles.json.1234.abcd.tmp')
+ expect(tempName('C:\\Users\\u\\AppData\\ck\\opencode-handles.json', win32.basename)).toBe('.opencode-handles.json.1234.abcd.tmp')
</file context>
|
Two more commits, both from a defect found after the last review round. Head is now
|
There was a problem hiding this comment.
1 issue found across 1 file (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="scripts/check-path-rendering.py">
<violation number="1" location="scripts/check-path-rendering.py:304">
P2: A marker inside a string can suppress a real path-component finding because this call checks raw lines instead of actual comments. Parse comment tokens or pass comment-stripped source before honoring the pragma.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| raw_lines = source.splitlines() | ||
| rel = path.relative_to(ROOT).as_posix() | ||
| for lineno in typescript_path_component_calls(source): | ||
| if has_not_a_path_pragma(raw_lines, lineno): |
There was a problem hiding this comment.
P2: A marker inside a string can suppress a real path-component finding because this call checks raw lines instead of actual comments. Parse comment tokens or pass comment-stripped source before honoring the pragma.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-path-rendering.py, line 304:
<comment>A marker inside a string can suppress a real path-component finding because this call checks raw lines instead of actual comments. Parse comment tokens or pass comment-stripped source before honoring the pragma.</comment>
<file context>
@@ -112,14 +294,31 @@ def main() -> int:
+ raw_lines = source.splitlines()
+ rel = path.relative_to(ROOT).as_posix()
+ for lineno in typescript_path_component_calls(source):
+ if has_not_a_path_pragma(raw_lines, lineno):
+ continue
+ stripped = raw_lines[lineno - 1].strip()
</file context>
|
Ready to merge, and the reclaim rule from #34 is in here rather than filed behind it — which was the right call and I should say why I think so now that I can see it built. What I verified at your head
|
|
Retracting the rebase request: it was unnecessary, and the mechanism I gave you was wrong. Your branch is fine and the cause was on my side the whole time. What I checked that I should have checked firstYour branch never touches What actually turned master red after #38Master was already at 0.17.26. I had been absorbing sibling lock waves locally and had not pushed them, so what sat on So the story I told you — that your branch's age was the problem — was really about my unpushed commits, and rebasing you would have fixed nothing. The shape of the error, since it is the useful partI had three true facts: master went red, its lock pinned an old version, and a fork PR had just merged. I invented a mechanism that fit all three and was still wrong, then banked it as a rule and acted on it twice. An invented mechanism that fits is worse than no explanation, because it stops the search — this one stopped it for four hours. The check that would have caught it costs one command and I ran it only when a second PR made me ask "why does this keep landing on contributors". Where that leaves #33Unchanged from my last comment: ready to merge as it stands, gate green on your code with the reclaim rule verified at source. No rebase needed. I will merge once I have pushed my own lock so that the tree the merge publishes is current — that is my prerequisite, not yours. One part of my earlier comment does still stand, and it is worth keeping separate from the retraction: gating your branch with master's lockfile copied in does test a hybrid tree. It is the right thing to do, because it isolates your code from a lock failure that is not yours — but the green it produces is evidence about your code, not about what merges. |
|
#33 now conflicts, and I caused it by merging #41 ahead of it. The conflict is one line, and it is the exact line you warned would collide. The overlapNothing in Please do not resolve it by arithmeticNeither number is right and their sum is not either. This is the same trap that produced the wrong 582 on this file a week ago: three plausible resolutions, all wrong, because a floor is a claim about a population and merging two claims about different populations cannot be done by adding deltas. Measure the merged population instead. The arm reports its own denominator if you force a refusal — set it absurdly high and read the number out of the failure: Then pin N. That is the only value that is a measurement rather than an estimate, and it is how I verified your 590 on the earlier collision. And your slack argument still holdsYou left #38's floor at 578 against a measured 579 deliberately, on the grounds that a third branch editing that line creates a conflict whose correct value cannot be derived from any of the three sides. That reasoning is why this is a one-line conflict rather than a three-way one, and I would keep it: a floor is a minimum, so slack is safe in either merge order, while every branch bumping it to the exact measured value guarantees a collision. What I owe you on orderingI merged #41 while #33 was sitting reviewed and ready, without checking whether the two touched a common file. One Everything else about #33 is unchanged: gate green, reclaim rule verified at source, no rebase needed. Once the floor is resolved I will merge it. |
|
Ordering note, since it changes what is worth doing first: #33 is the head of the queue. So merging #35 or #40 ahead of #33 would give you a second and third conflict to resolve on top of the This is the check I did not run before merging #41, which is what put you in this position. |
…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.
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.
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.
… 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.
…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.
This branch raised the floor 564 -> 568 when it was written; master reached 578 on its own. The rebase has to pick one, and neither is right: summing the deltas gives 582, and the real count is 590 -- commits after the floor edit added Rust tests without touching it, so the arithmetic was wrong before the rebase started. Measured with the same summation the arm uses, and pinned at the measured value: 591 fails, which is what makes it a floor rather than a number.
Quarantine directories must persist as the ABA guard for delayed stale\nobservations. Reclaiming only after the mtime-based bounded window keeps\nthat guard intact while preventing unbounded retained owner records.
`path.split('/')` returns the whole path when the separator is a
backslash, so the prefix match never fires and the sweep becomes a
silent no-op -- on the platform this repo's CI calls the load-bearing
leg, and the one where an unbounded quarantine directory is hardest to
notice. node:path's basename is the separator-correct form on both.
…the platform claim
The quarantine sweep's prefix and the atomic-write temp name were both derived with
`path.split('/')`. On Windows there is no '/' in the path, so the split returns the
whole path. Two different consequences:
quarantine sweep prefix never matches a bare directory entry -> reaps nothing,
silently, on the one platform the bound is hardest to observe
atomic write temp name embeds separators and a drive-letter colon, join()
nests it under its own directory, O_CREAT|O_EXCL throws ->
the manifest write fails outright
The second is the more serious of the two and was missed when the first was fixed:
that audit inspected the function it had changed rather than sweeping the class.
Both now use pathBasename. The platform claim is tested without pretending a POSIX
runner is Windows -- the prefix derivation takes the basename FUNCTION, so the test
drives it with both path.posix.basename and path.win32.basename. A filesystem test
cannot reach this: on Linux a '/' split and basename agree, so the defect passes
every filesystem test in the file. Verified by mutation: restoring the split inside
the helper reddens the assertion (not an import error, which proves only that the
helper is missing).
The Rust side already used path.file_name() at every site and needed no change.
79337bf to
04f8fa4
Compare
|
Rebased onto master The floor: 608, measured at the tipYour ordering note was right that it is one line, but resolving it had a wrinkle worth recording, because I nearly shipped the wrong number twice. Master and this branch had both moved the floor independently — 580 vs 603 — and master had also changed the command itself, adding Then the rebase replayed two floor-touching commits, and I measured at the first one: 596. That is a real measurement of a real tree — just not this one. Twelve tests live in commits replayed after that point, and the tip measures 608. A mid-rebase measurement is as wrong as adding the deltas, in a way that looks more legitimate because a test run produced it. Verified rather than asserted: The comment in Rest of the stateOne thing that changed outside the diff: master's lock now wants Nothing else outstanding from me on this one. #35 has a fresh Cubic finding I have already fixed on its own branch ( |
|
Holding the merge: the floor line at your head is a placeholder, not 608, and the arm passes with the check disarmed. Two literal occurrences at Why it passed rather than erroring outI drove your own
So The shape, because it is the one your own comment warns aboutYour rewritten comment block at 238-256 is good: it names the arithmetic trap, the mid-rebase trap, and the This is the same family as the What I verified while I was in thereEverything else at this head is clean, so this is one line rather than a re-review: Your 596-vs-608 account is right and worth keeping in the comment: a mid-rebase measurement is a real measurement of a real tree, just not the one being merged, and that is more convincing than a summed delta precisely because a test run produced it. Put 608 on the line and I will merge, then #35 and #40 behind it. One suggestion, take it or leave it: |
|
One correction to my own review, since it changes whether the suggestion is worth your time. I offered the non-numeric floor guard as "take it or leave it", which implied it was about your placeholder. It is not — master has the same hole, measured by driving master's own So this is a latent defect in a file you happen to have open, not a guard against your own typo. Master is not currently exposed because its floor is No obligation. If you add it, the class closes with your change and you found it. If you would rather keep this PR to one line, I will add it after yours lands — I am deliberately not touching Either way #33 only needs the number. |
|
Taking the "leave it" branch, and saying why rather than just declining.
I am not widening it, for the same reason you are holding off One qualification I would rather volunteer than have you discover: I have made two authoring errors in this exact file in the last day. The rebase produced a comment reading If you would rather it all arrive together, say so and I will add the guard here — it is three lines and I have no other claim on the time. My read is that unblocking two clean PRs wins. |
|
We disagree about a specific byte, so here is my evidence and the shape of the mistake I think is happening — because if I am the one who is wrong I would rather you can see exactly where. You say Two occurrences of the literal in that file at that sha — line 238 in the comment, line 257 in the live command. I used the API deliberately as a second instrument rather than re-reading my own clone twice. The blob shas matching means my object store and GitHub's agree about the bytes, so the disagreement is not a stale fetch on my side. The reading that would explain itYour working tree probably does carry 608, and you are reading it there. Worth checking with This does not change the queueYour reasoning for declining the guard is right and I am not asking you to revisit it — three clean PRs behind a latent hole is the correct trade, and I will take the guard after this lands. And volunteering the two authoring errors before I found them is worth more than the errors cost. The mid-rebase 596 is the one I would keep in the comment: a real measurement of a real tree, just not the one being merged, and more convincing than a guess precisely because a test run produced it. Push the number and I will merge, then #35 and #40 behind it. If it is already committed and I am reading a ghost, say so and the problem is mine. |
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 now derive from the observed owner nonce, closing the race where a delayed evictor could rename a replacement holder's fresh lock, and lock failures carry stablelock_busy,owner_invalid, andrenewal_failedcodes.Bug Fixes
@cortexkit/claustrum-clientwhile preservingpackages/opencodereader errors and refusing group- or other-writable parents.Written for commit 04f8fa4. Summary will update on new commits.