Repository navigation
Adopt verified adapter publication after a recovery race - #1045
Conversation
taras
added
the
ci-main-red-fix
PR #1045: Adopt verified adapter publication after a recovery race2 files, +123 / -2 Scope✅ PR scope looks good. StructuralOxlint structural signals:
Slop✅ Slop indicators look low. Static AnalysisOxlint: 5 diagnostics across 1 file (3 rules) no-array-sort (2): packages/acp/src/adapter-snapshots.ts CorrectnessFILE: packages/acp/src/adapter-snapshots.ts No other extraneous code patterns detected. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Independent review (Fable, first-green, once): PASS
Reviewed head 5a2f25fccdf29403657f46cc7e3542aff98cc1a8 against base 87fbc1b008aad9c48dd8beddd1830cd538504330, scoped to the #1043 main-health repair. The PR merged as 2b783c3a while this review was in progress, so both notes below are follow-up material rather than blockers.
Correctness. The preserved main failure (run 41259756614, test-deno (13/19)) is ENOTEMPTY on the recovery holder's rename(staging, target) in AM19. Recovery tickets exclude other recoveries only; the ordinary publication path is lock-free, so after the holder removes the abandoned target an ordinary attempt can publish into the gap and the holder's rename collides. The change in packages/acp/src/adapter-snapshots.ts catches only that rename, accepts only EEXIST/ENOTEMPTY, and adopts the competitor only when the existing published(snapshot) verifier (marker, snapshot digest, entry point, content digest) answers true. Every other errno, and an unverified tree, rethrow the original error; a foreign-snapshot marker still raises AdapterSnapshotError from the verifier. Ticket release and owned-staging removal stay in their existing ensure finalizers; the winner's tree is never removed or overwritten; the final published() check still runs. No retry, extra removal, API, observer, dependency or delay is introduced.
Tests. AM21 drives a real second materialization inside the removal-to-rename gap through FsApi.around rm middleware (an established pattern elsewhere in the repo) and checks adopted bytes, winner sentinel kept, stale bytes gone, no staging residue, finished ticket and idempotent reuse. The preserved pre-fix run of AM21 fails with real ENOTEMPTY with production unchanged, so it discriminates the defect. AM22 (wrong content digest → refusal, foreign bytes untouched) and AM23 (owned staging removed → ENOENT preserved beside a valid winner) bound the catch to the errno set and to verification.
Evidence. First green CI run 41262239075 at this exact head: every job success, including test-deno (13/19) (~4m40s, inside the 420 s shard bound). Independent focused run of packages/acp/tests/adapter-materialization.test.ts at this head on macOS arm64 under Deno: 25 steps pass. Local macOS runs do not claim Linux scheduling parity; Linux coverage is the CI run above.
Performance. Measurement is not applicable: the changed boundary is a cold error-recovery branch reached once per agent preparation at most, adding one published() call that already runs three times per materialization, with no UI, input or long-lived state. No benchmark was run and no timing assurance is made.
Minimalism. 11 source lines; the comment records the one surprising fact (a lock-free publisher sits outside recovery exclusion). Nothing to remove.
Two non-blocking inline notes follow: Windows errno coverage for the collision branch is unverified in CI, and the invalid-publication refusal surfaces a raw rename errno. Neither changes the verdict.
| // Recovery excludes other recoveries, but a lock-free publication | ||
| // can win the gap between removal and rename. | ||
| const code = Reflect.get(Object(error), "code"); | ||
| if ((code !== "EEXIST" && code !== "ENOTEMPTY") || !(yield* published(snapshot))) { |
There was a problem hiding this comment.
Non-blocking — Windows errno coverage is unverified. The test-deno/test-node/test-bun jobs run on ubuntu-latest; the filesystem-contract job does run on windows-2025, but it exercises packages/runtime/tests/host-files.test.ts, which never renames a directory onto a non-empty directory. So the accepted set {EEXIST, ENOTEMPTY} is verified on Linux (CI) and macOS (local) and not on Windows, where the errno for this collision may differ. Consequence: on Windows a holder losing this gap would still refuse, exactly as before this PR — not a regression, but recovery convergence there remains unproven. Recommendation: before relying on it on Windows, add a Windows witness for the collision errno (a probe in the filesystem-contract job, or a Windows row for this tier), or record the limit in the comment above this check.
| // can win the gap between removal and rename. | ||
| const code = Reflect.get(Object(error), "code"); | ||
| if ((code !== "EEXIST" && code !== "ENOTEMPTY") || !(yield* published(snapshot))) { | ||
| throw error; |
There was a problem hiding this comment.
Non-blocking — diagnosability of the invalid-publication refusal. When the verifier answers false the raw rename error is rethrown, so the caller sees ENOTEMPTY: directory not empty, rename .staging-… -> <digest> and nothing says a competing publication was present but failed content verification (AM22's case). Optional follow-up: wrap it in AdapterSnapshotError naming the unverified publication with the rename error as cause; AM22's errno expectation can then read the cause. Fine to leave as is under the accepted scope.
A recovery holder can remove an abandoned adapter target while an ordinary materialization publishes the valid snapshot into that space. Its subsequent rename then fails with EEXIST or ENOTEMPTY, which failed AM19 on main.
The recovery path now adopts the competing publication only for those collision errors and only after the existing validation succeeds. Invalid content and unrelated errors still fail; the losing staging directory and ticket retain their existing cleanup ownership.
AM21 deterministically fails before the fix. AM21–AM23 verify the winning publication, invalid-byte refusal and unrelated-error propagation. All 25 materialization cases pass under Deno, Node and Bun on macOS; the independent Codex Planner accepts the exact signed feedback commit. Required CI, including composability for this labelled repair, supplies native platform and exhaustive evidence. The permanent one-time first-green independent review remains a delivery gate.
Repairs the failure tracked in #1043. The automatic main-health issue remains open until main CI succeeds; this PR does not close it by keyword.