Make a cancelled run's sandbox release reportable — and actually release it - #46
Merged
Merged
Conversation
`Launcher.cancel` on Dispatron's side means "the resources are gone". Against
Claw nothing could establish that, because three places each dropped the
answer: `safeStopWorkload` logged a non-2xx, a timeout or a missing
`SAFE_API_URL` and returned `void`; `cancelTask`'s DAG-root branch returned
`{ok: true}` after awaiting it; and the route forwarded that. So an accepted
cancellation whose SaFE stop failed was byte-identical to one that worked, a
GPU workload kept running, and the counter built to catch exactly that stayed
at zero.
`POST /v1/tasks/:taskId/cancel` now carries `released`, one of `confirmed` /
`unconfirmed` / `nothing_held`. `safeStopWorkload` returns its outcome instead
of only logging it, and `stopAllHandlesForDag` aggregates: confirmed only if
every handle was confirmed, so one failure among several cannot be averaged
away by its neighbours. A 404 from SaFE is `confirmed` -- the workload is gone,
which is the state the stop was reaching for.
Three things this deliberately does not do:
- It does not infer release from the handle map. `destroy` runs BEFORE the
stop, so the map is empty whether the stop worked or not; the return value
is the only place the answer exists, and one test asserts the map is empty
in precisely the case the answer is `unconfirmed`.
- It does not let a failed release fail the cancel. The verdict is written
first and stays written; the sweeper and Dispatron both rely on that.
- It does not change the status code or any existing field. `released` is
added, and omitted rather than guessed on the non-root branch, which stops
no sandbox and so establishes nothing -- a client that does not know about
the field reads an unchanged response.
A handle registered with no SaFE workload id behind it (agent-sandbox, written
that way by Brain's ensureHands) now reports `unconfirmed` rather than sharing
a falsy early return with "no such handle". Something is held and this path has
never been able to release it, so `nothing_held` would assert the opposite of
what is true.
Tests cover the acceptance cases that could not be written before: a non-2xx, a
timeout, an unset `SAFE_API_URL` (its own file, since config reads it once at
module scope), a DAG where one of several stops fails, a task that never
recorded a handle, and a 404.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four defects from the first review round, two of which let the forbidden
inference back in through a door this change had opened itself.
**A repeat or concurrent cancel reported `nothing_held` for a release that had
failed.** `stopAllHandlesForDag` read an empty handle map as "nothing was ever
held" — but the map is emptied *before* the stop is attempted, so a second
cancel after a failed one read exactly the emptiness the first cancel created
and issued a clean bill of health for a workload still holding a GPU. That is
the one inference the whole feature exists to refuse, arriving from the other
side. The same hole was reachable whenever anything else got to the handles
first: a concurrent cancel, the agent_done path, or the sweeper. An outcome
short of `confirmed` is now written to a `dag-unreleased.<root>` record, in its
own key space because the handle entry is deleted exactly when it would be
needed, and an empty map answers `nothing_held` only when that record has
nothing outstanding. A confirmed release clears its entry, so the record is not
a latch that makes a DAG unconfirmed forever.
**A 2xx from SaFE's stop was reported as `confirmed`.** It is not a release:
`stopWorkload` sets the Workload's phase and issues a Kubernetes delete, then
returns, while the job-manager tears the data-plane objects down afterwards —
requeuing every 10s for as long as any remain and only then dropping
`WorkloadFinalizer`. So a 200 and a Pod still holding a GPU coexist on any
normal controller latency, and persist on a stuck controller. Calling that
`confirmed` would have retold the exact lie this field was added to stop
telling, one layer further in. That same finalizer makes the truth cheap: the
Workload survives in etcd until the teardown finishes, so one GET separates
"gone" from "still going". A 2xx stop is now followed by a single 5s read, and
answers `confirmed` only on a 404/410. It waits for no teardown — a workload
still present is `unconfirmed`, because at that instant it is.
**Cleanup that threw produced a 500 for a cancellation already written.**
`loadPlatformKeyForSession`'s query and the registry destroy both sat outside
`safeStopWorkload`'s try/catch, so a database or KV error propagated through the
aggregate and out of `cancelTask` — after the verdict was committed, and
abandoning every handle after the first. Each is now contained per handle,
converted to `unconfirmed`, and the loop continues. Containment lives in the
stopper rather than at the callers so all three entry points get it.
**A registry that could not be read was reported as `nothing_held`.** The KV
adapter turned every `get` failure, connection errors included, into `null`,
which `listForDag` renders as `{}`. An unreachable NATS would have reported a
clean release for every DAG in the fleet. `get` now returns `null` only for an
absent key; a corrupt payload throws rather than reading as absent.
Also from the round: the sweeper reached `handleMap()` directly, bypassing the
seam, and counted DAGs rather than releases — it now goes through the seam and
logs how many of a tick's reconciliations could not be established, since it is
the one teardown path with no caller to report to.
Tests: R7 now asserts the per-handle branch, which the aggregate cannot tell
apart; R10 goes over HTTP via `app.inject` instead of matching the handler's
source text, which could not see a status code — this needed an
`interruptDelivery` seam, and it lets R10 also assert the interrupt is really
published. R11-R16 cover the four defects above: repeat cancel, the confirmed
release clearing, a stop accepted but unfinished, the confirming read timing
out, an unreadable registry, and cleanup that throws.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cked nothing — review round 2 Two of round 1's own fixes were wrong, in opposite directions. Both are corrected here, with the reasoning recorded so neither is reattempted. **The unreleased record was in a bucket that forgets.** It went into `BRAIN_REGISTRY` beside the handle map, which is short-lived coordination state with a bucket-wide TTL — `BRAIN_REGISTRY_TTL_MS`, five minutes by default, sized for `lock.<key>`. Evidence of a leaked GPU that expires after five minutes is evidence only for as long as nobody was going to look, and the second cancel that round 1 was about is exactly the caller who arrives later. It was also a JSON blob under one key, so read-modify-write: two cancels racing lose a mark, and the lost one is a workload reported as released. It now lives on the DAG root's `claw_tasks.metadata`. Durable, and each write is a single jsonb merge statement, atomic on the row, that preserves siblings rather than replacing them — `derived.handle_last_user` sits one key over and losing it would take the DAG's whole teardown plan with it. It is also now readable: `publicTaskRow` strips only the three credential fields, so `GET /v1/tasks/:taskId` shows *which* handle was not released and what workload it was, which `released: "unconfirmed"` alone cannot say. **The confirming read could not confirm anything.** Round 1 established that a 2xx from SaFE's stop is not a release — `stopWorkload` sets the phase and issues a Kubernetes delete, then returns, while the job-manager tears the data plane down afterwards under `WorkloadFinalizer` — and closed the gap by following the stop with `GET /api/v1/workloads/<id>`, confirming only on a 404. That read is database-backed, not etcd-backed: `GetWorkload` filters on `is_deleted = false`, and the stop path writes `SetWorkloadStopped` (phase, end_time, deletion_time) and never `is_deleted`. It answers 200 for a workload that stopped perfectly normally, so the check would have reported `unconfirmed` for every successful cancellation in the fleet. A field that cries wolf constantly is worse than the silence it replaced, because the one real failure becomes indistinguishable from the noise. So the read is gone and `confirmed` means what the handoff defined it to mean: SaFE accepted the stop. The gap is documented at the call site instead of papered over — the finalizer state that would answer the question lives on the CR in etcd and no endpoint Claw can reach exposes it, so closing it properly needs a data-plane-completion signal from SaFE. What this change delivers is still what was asked: the refused stop, the timeout, the unreachable SaFE and the unconfigured one are no longer silently dropped. **A tombstone is not a corrupt entry.** Making the KV adapter propagate read failures (round 1, so an unreadable registry could not read as `nothing_held`) turned every deleted key into an error: `kv.get` does not filter DEL/PURGE — it answers with the tombstone, whose value is empty — and an empty body is exactly what a strict JSON parse calls corrupt. Every handle this module destroys leaves one behind, so a completely clean teardown would have read as unreadable and every later cancel of that DAG answered `unconfirmed` forever. DEL/PURGE and empty values are absent; only a genuinely unparseable payload is unknown. `scanPrefix` now makes the same three distinctions, where it previously skipped unparseable entries and handed the sweeper a short list it called complete. The adapter moved behind `makeKvStore(kv)`, taking the bucket instead of closing over the module's own, because which answers mean "absent" and which mean "unknown" is now load-bearing for whether a DAG can be reported as holding nothing. Tests: R13 and R14 replaced — R13 now asserts the *absence* of the second request, R14 covers the tombstone, the empty value, the corrupt payload and the unreachable bucket. New `unreleased-record-scenario.test.ts` runs the record's real statements against PGlite with `clawTasksSchemaSql()`, since an in-memory Map cannot disagree with its own implementation while hand-written jsonb merges against a shared column very much can: U1-U7 cover sibling survival, per-handle accumulation, partial and full clears, a no-op clear, writing to the root and not a node of the same DAG, and a missing root reading as unknown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… — review round 3 The round found that the feature was reporting on a teardown that had never run, and three ways the report could still confirm a workload nobody released. **The API read the wrong KV bucket, so no API-side teardown has ever torn anything down.** `sandbox-stopper` built its `DagHandleMap` over `infra/nats.kv`, which is `BRAIN_REGISTRY`. Brain writes handles to a bucket of its own, `DAG_HANDLES` (`brain/src/sandbox/handles.ts`). Both sides used the same map class, key shape and encoding, and nothing ever errored — the reader simply asked a bucket that had nothing in it. Every teardown the API ran found an empty map and issued no stop: on the cancel path, the agent_done path and the sweeper alike, a DAG's sandboxes outlived their DAG. That is the precise shape of bug this whole feature was asked to make visible, so reporting over the top of it would have turned a silent leak into a confident `nothing_held` — worse than the silence, because now something is asserting. The API now attaches to `DAG_HANDLES` with `ttl: 0` and `widenOnly`: a handle has to outlive its DAG, which for a long evaluation is hours, and `BRAIN_REGISTRY`'s five-minute TTL — sized for `lock.<key>` — is part of why the wrong bucket looked plausible. `widenOnly` because this process is not the authority on a bucket Brain owns. `sandbox-handle-bucket.test.ts` pins the two names to each other, which is what it takes: a name that is wrong but *consistent* is invisible to a unit test, and to any integration test that stubs the registry. **The record was written after the attempt, so the window between them confirmed.** `destroy` removes the mapping first; until the outcome was recorded the handle was in neither place, and a concurrent cancel landing there saw an empty map and an empty record and answered `confirmed` for a workload whose only stop was still in flight. A process dying mid-stop left no trace the attempt had happened. The mark now precedes the stop, so the window fails safe: the worst it produces is an `unconfirmed` that a completed release clears. **The aggregate trusted its own loop.** `stopAllHandlesForDag` snapshots the handles registered *now*; one that leaked earlier is no longer among them, because agent_done tears a handle down as soon as its last user finishes. So a later cancel that released everything it could see reported `confirmed` over the top of the workload nobody released. It now consults the record before confirming. **A payload of the wrong shape read as an empty map.** `JSON.parse` accepts `null`, `false` and `7`, and the cast that followed accepted all three; `DagHandleMap` then reads them as a DAG holding no handles — an unknown wearing the one answer that must never be invented. Shape is as much of the contract as syntax. Also: every catch on this path exists to keep a cancellation alive, so reading `.message` off a rejection that is not an `Error` would have defeated the containment at the moment it was needed. Tests the round proved were passing for the wrong reason: R12 started from an empty record, so it passed with the `clear` deleted — it now seeds the DAG as already leaking. R15 replaced `listForDag` with a throwing function, asserting only that the caller catches; it now drives the real adapter, which is the part that used to turn read failures into `nothing_held`. New R17 (a handle that leaked earlier is not confirmed away) and R18 (on record before the stop runs). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zoroyihan7
changed the title
Found while auditing the bucket change from the last commit, and it is the other half of why that bug survived: the NATS allow-list agreed with it. The api user was granted `$KV.BRAIN_REGISTRY.dag-handles.>` — a permission for a key nobody has ever written, since brain registers handles under `$KV.DAG_HANDLES.dag-handles.*` — and nothing granted `KV_DAG_HANDLES` at all. So the code and the deployment described the same wrong layout, consistently, and a subject that is wrong but agreed on by both sides looks exactly like a subject that is right. It also means the previous commit alone would have failed in the cluster while passing every test in the repo. A denied publish does not raise in the NATS client, so the new code's failure mode there is a teardown that silently does nothing — indistinguishable from the failure mode it just fixed, and precisely the kind of invisibility this PR exists to end. The api user now holds every subject class the stopper reaches, each for a different operation: `$KV.DAG_HANDLES.dag-handles.*` for put and delete (`.*`, not `.>`, because a handle key is one token — a task id has no dots, and it is the grant brain already holds); STREAM INFO/CREATE/UPDATE for the reconcile, CREATE included because api can start first on a fresh cluster; DIRECT.GET and the MSG.GET fallback for reads; the ordered-consumer lifecycle for the sweeper's `kv.keys()`; and `$JS.FC.KV_DAG_HANDLES.>`, without which that scan stops yielding — without throwing — once the bucket is large enough for the server to apply backpressure, handing the sweeper a short list of DAGs and no indication of it. The stale BRAIN_REGISTRY grant is removed. Left in place it is a standing invitation to put the code back: a grant describing a layout nothing implements, sitting in the file somebody reads to find out what the layout is. B3 and B4 pin the allow-list to the bucket constant the code names. Nothing tied the two together before, which is how they drifted and then agreed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Half the reason the record lives on `claw_tasks.metadata` rather than in KV, and the half nothing asserted. A `released: "unconfirmed"` says a sandbox was not released; it cannot say which handle, or what workload to go and look for, and an operator holding only the first has nothing to act on. `publicTaskRow` strips the three credential fields and redacts the rest, so whether the record is on the readable side of that is a property of somebody else's function. A redactor that grew a rule for `*_id`, or for anything under an unfamiliar key, would take the actionable half away silently and leave an endpoint that still answers and no longer helps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Review trail, and two things that need a humanThree adversarial review rounds against this branch; 13 defects found and fixed, four of them mine from earlier rounds. Full history is in the commit messages. CI has been green on every commit (47/47); locally 2586 tests, 0 failures, all The rounds were worth it — the two most serious findings were not in the feature at all, but underneath it: 1. No API-side sandbox teardown has ever run. 2. The NATS allow-list agreed with it. The api user was granted Both are fixed here, and
|
The last way a false clear could get in, found tracing every path to `stopAllHandlesForDag` for one. `destroy` removes the handle mapping and returns the workload id. When that call throws, two things may have happened: nothing, or the server executed the delete and the response was lost. The second leaves the handle gone from the map with nothing recorded anywhere — so the next caller reads an empty map, an empty record, and answers `nothing_held` for a workload that was never stopped. The previous commit reasoned that the aggregate consulting the record covers this; it does not, because in this branch there is no record to consult. It is now recorded with an empty workload id, which is the truth: `destroy` is what would have returned one. The deliberate cost is that nothing can ever clear this entry — no later teardown revisits a handle that is no longer in the map — so the DAG answers `unconfirmed` from then on. That is the right way round to be wrong. A standing false alarm on a DAG whose KV read failed is visible and checkable; a false clear on a live GPU is neither, and is the whole reason the field exists. Also recorded, at `rememberOutcome`: a failed record write is not a silent loss of evidence in the way it first appears. The record lives on `claw_tasks`, and a database that cannot take this write could not have taken the cancellation's own verdict either — that write happens first and throws, so the request fails loudly long before reaching here. What the catch covers is the narrow case of a write failing on its own, where this caller still gets its answer and only the next caller's view of it is lost. R19 covers both halves: the lost response is recorded, and the next cancel over the now-genuinely-empty map still answers `unconfirmed`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`stubSafe`'s doc still described the confirming read that round 2 removed, and carried a `present` option nothing used any more — so the file explained the fake in terms of a design the code no longer has, which is the sort of comment that gets believed over the code next to it. It now says what the `read` counter is actually for: R13 asserts it is empty, because the absent second request IS the property under test. The reason the read cannot work is kept with it, since a reader who does not know it will reasonably wonder why the obvious check is missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bucket fix two commits ago makes a teardown path live that has never run. The round found two ways that path destroys a sandbox somebody is still using. Both are older than this branch; both become reachable because of it, which makes them this branch's to fix. A change whose whole purpose is to stop sandboxes leaking must not start killing live ones on the way. **The sweeper reaped every standalone task's sandbox, while it was running.** `reapOrphanHandles` looked up the handle's owner with `task_id = $1 AND dag_node_id = '__dag_root__'`. That is not a narrowing of the same row — for half the handles it is a different row. Brain registers under `dag_root_task_id ?? task_id`, so a standalone task owns a handle under its own task id, and a standalone task's `dag_node_id` is NULL. Every one of them matched nothing, read as `missing`, and was torn down mid-run. Keyed on the primary key now; a row that is absent is an orphan, a row that is present and not terminal owns its sandbox whatever shape of task it is. **`agent_done` released handles other nodes were still using.** `handle_last_user` is the last node in *topological* order that names the handle, not the last one to finish. Two siblings that both use a handle created upstream order as [root, A, B], so the map says B — and B finishing first tore the sandbox down with A still running on it. Giving B the higher priority makes that the normal case, not a rare one. The teardown is now deferred while any sibling is non-terminal; the DAG-root transition tears the handles down a moment later anyway, so the wait is bounded by the DAG's own remaining work. Three more from the round, all in this branch's own code: **The window between `destroy` and the first record.** `destroy` is the point of no return — it drops the mapping, and anything not written down by then is unrecoverable. A concurrent cancel landing between the drop and the mark read an empty map and an empty record and answered `nothing_held`; a process dying there left that state permanently. The handle is now looked up and recorded *before* the mapping is dropped, at the cost of one KV read on a path already making an HTTP call. **The record was keyed by handle name, so a rebuild's outcome could clear another workload's.** A handle name is reused; an older stop succeeding and clearing a newer workload's failure is a confirmed release invented out of two unrelated events. Entries are keyed by workload identity now, and cleared only for the workload whose release was established. **And that key could be redacted away entirely.** `redactPublicJson` replaces the value under any key containing a sensitive *word* — `isSensitiveKey` splits and matches each — and handle names are chosen by whoever wrote the DAG. A DAG may legitimately declare a handle called `token`, and `token`, `w-1:token` and `handle_token` are all redacted alike, taking the workload id with them and leaving the caller knowing a sandbox leaked but not which one. The key is a `sha256(handle \0 workload)` digest: hex only, so it cannot spell any word the redactor looks for. Both names live in the value, where they are data. U9 runs that round trip for a handle actually named `token`. L1-L4 cover the two live-sandbox cases in both directions — not reaped while running, still reaped when terminal; not released with a sibling live, released once they are terminal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two holes the last round named that I reported as fixed and had not fixed. B3 asserted only the bare `$JS.API.CONSUMER.CREATE.KV_DAG_HANDLES`. The server names an ordered consumer itself, so the request subject has no trailing token -- and addresses it as `<stream>.<name>` when it does. Both forms are granted; only one was checked, so the `.>` grant could be deleted with this test green and `kv.keys()` failing in the cluster. INFO is now asserted too, for the same reason. And the check matched raw file text, so a subject *named in a comment* read as a permission. This file explains several, including the stale grant B4 exists to keep out, so the one place that reads it had to distinguish a grant from a sentence about a grant. It now compares against the parsed grant lines, as B4 already did. Neither changes the deployment: both grants were already in nats-values.yaml. What changes is whether removing one is caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I had reported this one as deferred, on the grounds that fixing it properly
means giving `KVStore` a revision and that is a contract two packages
implement. The deferral was wrong, for a reason I only found when checking
whether the race predates this branch: **it does not.**
`DagHandleMap.destroy` rewrites the whole DAG row and its write carries no
revision. This module is the only caller of it anywhere — Brain registers and
looks up, never destroys — and its calls went to the wrong bucket until this
branch fixed that. So the lost update has never been reachable, and pointing
the API at the bucket Brain actually writes is what makes it reachable. A race
this branch introduces is this branch's to avoid, not to note.
What it costs is not a misreport:
API reads `{a: Wa}` and prepares to write the row without `a`
Brain registers `b`, writing `{a: Wa, b: Wb}`
API writes `{}` — `b` is gone, and with it the only reference to Wb
Nothing then knows Wb exists: not the cancel, not the unreleased record, not a
later sweep. It holds its GPU until something outside Claw notices. Every other
defect in this branch has been a wrong answer about a workload; this one
destroys the evidence that the workload is there at all, which is strictly
worse and is the opposite of what the PR is for.
The removal is now conditional on the revision the row was read at, retrying on
conflict — five attempts, because each retry means a registration landed, which
is rare and self-limiting, while a row that will not settle is a broken
invariant rather than a busy one and is raised rather than spun on inside a
cancel request. A conflict is matched narrowly, so a genuine failure is raised
instead of being retried into the attempt limit and raised later with worse
context. Giving up must not return `null`: that reads as "no such handle" and
renders as `nothing_held`, inventing the one answer this must never invent out
of a bucket that was merely busy.
It lives in `sandbox-stopper` rather than in `DagHandleMap` because that class
is shared with Brain. Giving `KVStore` a revision is still the right fix and
still belongs in its own review; what does not belong there is a race this
branch is about to create.
C1-C4 drive it against a bucket that enforces `previousSeq` the way NATS does,
since the property under test is the revision check itself — a fake without one
would pass whether the code sent a revision or not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…und 4 **The sweeper stopped the sandbox the NEXT task in the session was reusing.** The registering task being terminal does not mean the sandbox is idle: Brain keeps a finished task's pod warm as `hands.<session>`, and the next message in the same session reuses it via `tryReuseSessionSandbox` without moving the DAG handle's ownership. T1 completes, T2 picks up the same workload, and a sweep reading only T1 stops the pod T2 is running on. The two are sequential — this needs no race, it is the ordinary shape of a session, and the previous commit's fix did not touch it because L1/L2 only ever had one owner. Ownership should move with the reuse and that is Brain's to do; until it does, a session with live work keeps its sandboxes. L5 covers it. **Recording before the destroy was not protection, because the destroy ran anyway.** `rememberOutcome` swallows its write failure — deliberately, so bookkeeping cannot fail a cancellation — and the code then dropped the mapping regardless. That loses the handle exactly as completely as not recording at all: a concurrent cancel reads an empty map and an empty record and answers `nothing_held`. The mark now gates the destroy. A sandbox that stays registered until something retries costs a deferred reap; dropping the only reference to it costs the sandbox. **A destroy that found nothing left a false alarm nothing could clear.** With the lookup now preceding it, a slower caller marks, then finds the handle already gone — taken by a faster caller who marked, stopped and cleared. The slower caller's mark is speculative, made before it knew it had anything to do, and no later teardown revisits a handle that is no longer in the map: the DAG reports `unconfirmed` for ever over a workload that was released perfectly well, which teaches an operator to ignore the field. It is retracted, safely, because the other caller's record stands until their release is established. Tests the round proved were passing for the wrong reason, both confirmed by mutation here: - **L1 survived restoring the exact predicate it exists to forbid.** Its stub answered the new query by prefix before reaching the old one. Reordered so the old predicate is matched first; restoring it now fails L1, checked by reverting the line and running the file. - **R18 watched `fetch`, so it passed with the record written after the destroy** — the ordering it exists to forbid. It now observes inside `destroy`, which is the instant the evidence must already exist. - **U10 is new**: the digest key had no direct regression test, and a key made of the handle name alone passed every other test in the file, because every other test uses one workload per name. It pins two workloads under one name as separate entries, with a clear on the older leaving the newer outstanding. R19's comment is corrected: the workload id now comes from the lookup that precedes the destroy, so the entry names the workload even when `destroy` never answered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…— round 5 Four of this round's six findings are defects the previous round's fixes introduced. The worst is mine and is the exact failure this PR exists to stop. **The retraction deleted the winner's failure record.** Round 4 added it on the reasoning that a caller who loses the destroy race can safely clear its own speculative mark, "because the winner's record stands". It does not: both callers derive the same key from the same (handle, workload) pair, so there is one record. Two cancels, the winner's stop returns 503 and is recorded, the loser's `destroy` returns null and clears it — and the DAG then answers `nothing_held` for a workload nobody released. A false clear on a live GPU is the one answer this field must never invent, so the retraction was a worse bug than the standing false alarm it was fixing. Reverted, with the cost it leaves written down rather than re-discovered. **The CAS retry could remove a workload it never recorded.** The revision check stops a concurrent registration being overwritten; it does not stop a *retry* removing the wrong thing. Between re-reads a rebuild can register a different workload under the same handle name, and deleting that entry drops the only reference to a live sandbox — the failure the revision check exists to prevent, arriving by the other door. The removal is now bound to the workload the caller recorded, and a changed identity aborts rather than deletes. **The aggregate confirmed over handles registered while it ran.** It walked a snapshot and then checked only the unreleased record, so a handle registered mid-teardown — a rebuild, or a node starting after the snapshot — was a live sandbox neither stopped nor counted, and the interrupt is not published until after this returns. It re-reads now. This bounds what the call may claim; it does not fix the race, because a workload exists before its handle is registered, and that is recorded as needing coordination that does not exist. **The agent-sandbox branch bypassed the gate** the workload-id branch had gained, dropping the mapping when the record had not landed — the same total loss of the handle, on the one path that cannot stop the sandbox by any other means. **The record was written to a row that did not exist.** `mark`/`clear`/`any` demanded `dag_node_id = '__dag_root__'`, which is not the owner row for a standalone task — Brain registers those under their own task id, and their `dag_node_id` is NULL. The statement matched nothing and reported success, so `rememberOutcome` returned true for a record that was never made and the caller dropped the mapping on the strength of it. Keyed on `task_id` now, and a write matching no row raises instead of passing. **The CAS tests were partly checking the fake.** It treated an absent revision as a permanent conflict, where real NATS treats it as an unconditional write — so C1 passed whether or not the code sent a revision, and C2 never checked the conditionality in its own title. The fake now matches NATS, C2 asserts both halves, and C5 is new for the identity binding. Verified by mutation: dropping either revision argument now fails four of the five. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I claimed the CAS tests failed under either revision being dropped. That was wrong, and the review was right: under a faithful mutation -- passing `undefined` as the revision rather than switching to `put` -- only C3 failed. My own mutation swapped in `kv.put`, which the fake rejects outright, so the tests failed for a reason unrelated to the revision check and the gap stayed hidden behind a green result. C1 looks like it covers the rewrite path and does not: its row empties, so its first write is the DELETE, and it pins that argument alone. The two writes are separate arguments in separate branches and need separate cover. C6 removes a handle from a row that does not empty, so the write is a rewrite, and lands a registration after the read that an unconditional rewrite would discard. Verified by the faithful mutation: C3 and C6 fail, the rest pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Six review rounds in: this should be split, and here is the evidenceI am marking this draft. Two counterexamples survive that need no race at all — both reproduced against the real registry, CAS and stopper — and neither can be fixed from the API side, because in both the handle registry is simply wrong about what exists. 1. After a rebuild, a cancel stops the old workload and reports 2. A reused sandbox is never registered against the DAG that reuses it, so a cancel reports Both are sequential — the ordinary shape of a session — and no amount of CAS, re-reading or record-keeping in What this means for the handoff's premiseThe handoff says "which workload does this task hold" is solved. It is not. The registry misses sandboxes acquired by reuse and points at stale ones after a rebuild. That is why every round here found new P1s: the reporting layer is being asked to assert something the layer beneath it cannot support. To be precise about what is and is not sound, because the distinction matters:
Proposed split
Reverting the wiring to the old bucket and shipping the reporting alone is not an alternative: against the wrong bucket every DAG answers What is in the branch and worth keeping either wayFourteen defects fixed across six rounds, each with regression cover, several verified by mutation: the bucket/allow-list mismatch that meant no API-side teardown had ever run; the sweeper reaping live standalone tasks and live session-reused sandboxes; Happy to carve it up along those lines on request; I did not want to restructure someone else's roadmap unasked. |
zoroyihan7
marked this pull request as draft
September 14, 2026 12:32
Round 6's two blocking findings. I had reported them as unfixable "from the API side" and stopped there, which was the wrong boundary to stop at: Brain is in this repo, and the review had already said where the fix belongs. Both are sequential -- no race -- and both ended with a cancel reporting a release that never happened. **Reuse never registered.** `ensureHands` returns early when it adopts a warm sandbox, and that return skipped handle registration entirely. T1 creates W and ends; a DAG in the same session reuses W; the handle still names T1, so the reusing DAG's map is empty. Cancelling it issues zero stops and answers `nothing_held` -- and the sweeper, reading only T1, tears W down under the DAG now running on it. Ownership now moves with the sandbox, at the moment it moves. **Rebuild left the map naming a corpse.** The old workload is stopped and its handle left in place, so registering the replacement hit `DagHandleMap.create`'s refusal to overwrite, the exception was swallowed at the call site, and the new sandbox ran unreferenced. A later cancel stopped the dead workload, got its 404, and called the sandbox released. `create`'s refusal is right against a mistaken double-create and wrong at the two moments a handle legitimately changes hands. `replaceDagHandle` is that second operation: destroy then create, so the write is a replacement rather than an overwrite `create` rejects, logging the previous workload id because it is the one identifier that otherwise disappears exactly when someone needs it. It moves the name and frees nothing -- the rebuild path has already stopped the old workload, and the reuse path must not stop a sandbox it is adopting. `registerDagHandle` is removed rather than kept beside it. It had no callers left, and leaving a "refuse if the name exists" primitive next to a "take the name over" one preserves the exact footgun that caused this: three call sites reached for the stricter of the two and swallowed the rejection. H1-H4 cover the takeover, the create-when-free case, sibling handles surviving a replacement (one row holds them all, and replace destroys before it creates), and the reuse path registering before it returns. The existing SaFE-namespace test selected its call by first occurrence, which silently retargeted it at the new reuse registration; it now selects by what the call is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…andle I introduced `replaceDagHandle` in the previous commit as destroy-then-create, and raised the non-atomicity as something worth challenging. Checking it myself rather than waiting: the exposure is real, and it is the same false clear this whole line of work exists to remove, reintroduced one layer down. Backend's teardown decides a DAG holds no sandbox by finding no handle. Between the destroy and the create the name resolves to nothing, so a cancel landing in that window reads an empty map, finds nothing on record, and answers `nothing_held` for a workload that is running. Replacing a name must never look, even briefly, like never having had one. `DagHandleMap.replace` is therefore a single put: read the row, set the one key, write it back, and return what the name pointed at before. That is where the operation belongs anyway -- beside `create` and `destroy`, which share the row and the encoding -- rather than composed out of them by a caller that cannot see the gap it is opening. It moves the name and frees nothing. Stopping the workload that was there is the caller's business: the rebuild path has already done it, and the reuse path must not stop a sandbox it is adopting. The protocol package had no test for this class at all. The new file covers the atomicity directly, by watching every write rather than only the end state -- an intermediate row without the handle is invisible to an after-the-fact assertion, since it is gone by the time the call returns. Verified by mutation: spelling `replace` as destroy-then-create fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… round 7 Round 7's remaining blocker, plus the CAS half of its Q2. (Its other finding, the destroy-then-create window, was already closed by the previous commit, which it had not seen.) **A swallowed registration left a live sandbox nobody could account for.** All three sites caught the failure, logged it, and returned the sandbox anyway -- so `ensureHands` handed back a workload while the DAG's handle map stayed empty, Backend's teardown found nothing, and a cancel reported `nothing_held` and issued zero stops. Reproduced by the review on both the reuse and rebuild paths. The old comment called this non-fatal because "the DAG can still complete with sandbox-per-node semantics". It can -- but the handle is also the only record of what the DAG holds, and losing reuse is a cost while losing the ability to account for a GPU is not one to take silently. The registration now throws and the callers propagate it: failing the turn is loud and retryable, succeeding quietly is how the leak becomes invisible. **Brain's unconditional row write could resurrect what Backend had removed.** Backend now removes handles under a revision-conditional write; a plain read-modify-write from this side puts back a stale snapshot, reviving a reference to a stopped workload or undoing the removal of a running one. The registration is written against the bucket with `create`/`update` and the revision it read, retrying on conflict -- the same shape as Backend's `destroyHandleCas`, because the two writers have to agree on a row version. Still one write that sets the key, never a delete and a create, so the handle is never momentarily absent. **H4 was a source-level assertion that proved nothing.** The review showed an early `return` inserted at the top of the helper left both its regexes matching. It is behavioural now: a bucket that refuses every write, asserting the turn fails rather than returning an unowned sandbox. H5 is new for the conflict retry. Five existing tests started failing on this change, and that is the finding rather than the breakage: `ensure-hands-create-identity*.test.ts` are named for "the identity it registered" and were green while the registration inside threw `dag-handles.not_initialized` and the call site ate it. They bind a bucket with real revision semantics now. In the kubernetes file it is imported inside the hook, because that suite sets its env at module scope and a static import that reaches config.js reads them too early -- which turned it into a suite asserting it was "meaningless outside kubernetes mode". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rade The first question anyone will ask about making the registration throw is whether it newly fails turns. It does not, for a reason that is not local to the function: `initDagHandles` is awaited unqualified during boot in index.ts and nothing catches it, so a process that is serving requests has a bound map by construction, and `dag-handles.not_initialized` is reachable only from a test. The failures this surfaces are writes that were genuinely refused. That is the trade being made on purpose, and it is the only one -- worth stating where the throw is, so the next reader does not re-derive it or soften the throw on a guess. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Correction: the split recommendation above is supersededMy earlier comment argued this should be split, on the grounds that the two remaining counterexamples "cannot be fixed from the API side". The first half was true and the conclusion did not follow — Brain is in this repo, and the review had already located the fix there. I drew a scope boundary at the
Three further defects surfaced from those fixes and are also closed: the replacement had to become one write, because destroy-then-create leaves the handle briefly absent and an absent handle is exactly how teardown concludes a DAG holds nothing; Brain's row write had to become revision-conditional, or it resurrects an entry Backend's CAS removal had just deleted; and the swallowed registration failure had to become fatal, since a registration is the only record of what a DAG holds. That last one had a tell worth repeating: making it throw turned five existing tests red — So the layering argument for splitting is gone; the review confirmed there is no obstacle to finishing in one PR. The rollout question in my earlier comment still stands and is unaffected by any of this: Review round 8 is in flight against the current head; I will report its verdict and take this out of draft if it is clean. |
…andle name
Three of round 8's findings.
**Making registration fatal fixed the wrong half.** The turn now fails, but the
workload it created is still running: `hands.<session>` is already READY and
keepalive is already registered by then, and the failure cleanup above only
reaps PENDING entries. So the leak survived the fix that was supposed to stop
it -- with nothing pointing at the workload at all, which is worse than the
misreport it replaced. Both create paths now tear the sandbox down before
rethrowing, in the same shape as the rollback the pending KV write already
does. A rollback that itself fails logs the workload id, because that is all
anyone will have to find it with.
**Re-cancelling a finished DAG stopped a sandbox a newer DAG was reusing.** The
DAG-root branch tore handles down even when its UPDATE matched no rows.
Sequentially: D1 finishes without its agent_done teardown firing (the
topological last user deferred to a live sibling, and the sibling that finished
last was not the last user), D2 reuses the warm sandbox and adds its own
reference, and a second cancel of D1 stops that workload -- killing D2 and
answering `cancelled: 0, released: "confirmed"`. Both halves wrong, and the
`confirmed` is the worse one: it names as cleanly released the thing it just
broke. A DAG already terminal is no longer cancelled again, and its leftovers
stay the sweeper's, under the session guard. The wider problem -- two DAGs
sharing one workload while both are live -- is not solved by this and is
recorded at the site.
**A handle named `__proto__` registered successfully and stored nothing.**
`row[name] = info` hits `Object.prototype`'s setter, so the row serialised as
`{}` while the call reported success, and Backend read the DAG as holding
nothing. Admission accepts the name, so this is reachable by writing a DAG.
Rows are written with `defineProperty` and read by own-property only, in the
map and in Brain's writer.
Writing my own test caught a second copy of it I had missed: `listForDag` and
`listAll` build their OUTPUT with `out[name] = info`, so the stored entry would
have been correct and the DAG would still have enumerated as empty -- and that
enumeration is what teardown walks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last of round 8's blocking findings. **A cancel during provisioning missed the workload entirely.** The workload exists from the moment SaFE assigns its id; the handle was registered at the end of `ensureHands`, after poll, bootstrap and health. Everything in between was time a cancel could arrive, find no handle, conclude the DAG held nothing, and say exactly that -- while the workload it missed kept its GPU. This is the gap the code has carried a comment about for several rounds, on the grounds that it needed cancel and provisioning to coordinate. It did not. `onProvisioned` already exists and already fires on workload assignment, precisely so the `hands.<session>` entry can be written before any of that work -- and it already rolls the workload back if that write fails. The DAG handle is registered there now, carrying what teardown needs: the id, the key to stop it with, the namespace to poll. `hands_url` and `token` are not known yet and the registration at the end fills them in by replacing the entry. A registration that cannot be written rolls the workload back the same way the pending write does, because a workload nothing can account for must not outlive the call that made it. **An existing empty row wedged the registration.** "Is there a row to build on" and "does the key exist" are different questions, and they come apart for an entry present with an empty value: nothing to build on, but the key is there. Conflated, the write went out as a `create` against an existing key, was refused, and was refused again for all five attempts -- failing against a row it could have updated. A DEL/PURGE tombstone is the opposite case and still takes `create`, which is what the client does over a tombstone. **The unsupported-handle branch destroyed without binding to what it recorded.** It records `(handle, "")` and then removed whatever currently answered to the name. An entry since replaced by a real SaFE workload was deleted there -- and that branch discards the id `destroy` returns, so the workload was neither recorded nor stopped, and lost its only reference. It is bound to the empty id now, so a changed identity aborts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ence
Two of round 9's three findings, plus the test gaps it proved by mutation. The
third — the provisioning window — is not fixed; see below.
**A failed reuse registration left the adoption half-done.**
`acceptExistingSandbox` has already cleared the idle markers and registered
keepalive by the time the DAG registration runs, and the runner has not been
handed the identity yet -- so throwing unwound neither, `reapPendingHands`
skips the READY entry, and the turn's own teardown answers `no_sandbox`. The
sandbox stayed active, owned by a session whose turn had just failed and
claimed by no DAG. The adoption is undone now: the ticker is stopped and the
idle marker put back, in reverse order so the entry is never active with
nobody pinging it. Deliberately not stopped -- this path did not create the
sandbox, and another session's warm pod is not this turn's to destroy on the
way out.
**`retryTask` could delete a leak record written while it ran.** It wrote back
`{...task.metadata, retried_into}` -- a snapshot read before the INSERT -- and
`updateTask` merges at the top level, so anything written to the row in between
was replaced by its older value. The sweeper writes exactly such a thing, and
losing it turns a workload it could not stop into `nothing_held` for whoever
asks next. It patches the one key it means to set; a patch cannot lose a
sibling it never mentions.
**Three tests were passing for the wrong reason, each confirmed by reverting
the protection here.** H7 matched "a stop exists somewhere in the hook", and
the hook already contained one for the pending KV write's own rollback -- so
deleting the new rollback outright left it green; it now requires the stop to
follow *this* registration's failure and precede its rethrow. The protocol
`__proto__` test covered the write and `listForDag` but not `listAll`, which is
what the sweeper walks. And nothing covered Brain's own row writer at all:
`replaceDagHandle` writes the row itself rather than going through
`DagHandleMap`, so reverting it alone to a plain assignment left every test in
both packages green. H8 covers it.
**Still open: a cancel during provisioning.** Registering at `onProvisioned`
narrowed the window but did not close it -- the workload exists before that
hook commits, and nothing on the create path checks `options.signal`, so an
abort does not stop a creation already in flight. Closing it needs a real
barrier between cancel and provisioning rather than an earlier write, and the
agent-sandbox path has no early registration at all. Recorded rather than
claimed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Narrows the provisioning window round 9 reported still open. It does not close it, and the commit before this one says so; this is the part of it that is a choice rather than a design problem. Both records are written in `onProvisioned` for the same reason -- the workload exists from the moment SaFE assigns its id, and poll, bootstrap and health all happen afterwards. Their order was arbitrary and is not: only one of them is what a cancel reads. Backend decides whether a DAG holds a sandbox from the handle map, so every instant in which the session entry exists and the handle does not is an instant a cancel answers `nothing_held` over a live workload. Reversed, the worst a cancel can see is a handle whose session entry has not landed yet, which errs towards reporting a workload that is there -- the direction this whole change exists to err in. What remains open is the part an ordering cannot fix: the workload exists before this hook is called at all, and nothing on the create path checks `options.signal`, so an abort does not stop a creation already in flight. That needs a barrier between cancel and provisioning. The agent-sandbox path also still has no early registration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… session
Round 10, including a regression I shipped two commits ago.
**`retryTask` was destroying the whole metadata column.** Round 9 reported the
original write as losing a concurrently-written record, and I "fixed" it by
patching only `{retried_into}` on the belief that `updateTask` merges its
patch. It does not: `updateTask` assigns (`metadata = $1`); the function that
merges is `applyTaskStatusTransition`, whose SQL I had read and attributed to
the wrong caller. So the fix replaced the column with a single key and
destroyed `derived` -- handle_last_user, root_node_id, schema_digest -- along
with the unreleased record, deterministically and with no concurrency needed.
Worse than the bug it was fixing.
Both wrong versions look right on the page, which is the argument for doing it
in the statement: `COALESCE(metadata,'{}') || jsonb_build_object(...)` reads
nothing into this process, so nothing can go stale between a read and a write,
and every key the call does not name survives.
**The adoption undo targeted a session that does not exist.** It preferred
`identity.sessionId`, which for agent-sandbox is the Router's session id, while
keepalive registrations and the `hands.<session>` key are both keyed by Claw's.
The unregister and the idle write went somewhere else entirely and the undo
reported success. It uses the Claw session now.
**And it could not have noticed either way.** `markHandsIdle` reports failure
instead of throwing -- `superseded` on a revision conflict, `failed` otherwise
-- so the catch around it established nothing. A conflict is the ordinary case
here, since the entry is live and its TTL is being refreshed underneath; left
unchecked, the local registration is gone while KV still says active, and the
next tick finds the workload again and pings a sandbox no turn owns. The
outcome is checked and an incomplete undo is logged as such.
U11 covers the retry write against a real database, after a first attempt at it
proved vacuous: `retryTask` refuses chat rows and anything with a
`dag_root_task_id`, so seeding a DAG root meant the write never ran and the
test passed with the regression in place. It seeds a standalone row -- which is
also the only kind this can affect -- and asserts the retry actually happened.
Verified by mutation: restoring the full-replace fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 11's one new finding. The clone's metadata expression already subtracts the fields a replacement must not inherit; `sandbox_release` is a fourth, and the sharpest, because it is evidence about a workload the PREVIOUS run held. Carried over, it lands on a row that has never had a sandbox and can never clear it: releasing the replacement's own workload clears the replacement's own entry, never the inherited one. Meanwhile the original's copy IS cleared when that workload is finally released. So the evidence migrates to exactly the row it is not about, and stays there, reporting a leak that does not exist while the one that does goes unrecorded. U12 covers it; verified by mutation. Also of note from the round, since it changes what I was about to build: the acquisition-intent marker I proposed -- write the handle with an empty workload id before acquiring -- is not sound in that form. Two reasons worth recording. Empty workload ids are legitimately permanent for agent-sandbox, so "empty" cannot also mean "acquisition in flight"; and a cancel landing on the marker records `(handle, "")` persistently and drops the mapping, after which the real release clears `(handle, realId)` and the DAG reads unconfirmed for ever -- no failure required. The direction is right and needs explicit acquisition phases with per-attempt identity, which is its own change. So the two acquisition windows -- create and adopt -- remain open and remain documented as open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six conflicted files, all two independent features landing in the same place. Both sides' intents kept throughout; where they could not be, it is stated. nats-values.yaml and infra/nats.ts — this branch adds the DAG_HANDLES bucket (and the grant fix behind it: the old allow-list named a bucket nobody writes, which is why DAG teardown destroyed nothing), main adds DOORBELL_FLOOR. Both buckets are needed and both sets of grants kept, in all five api sub-lists. A missing NATS subject fails at runtime with a permission error rather than at build time, so the risk here is asymmetric and the merge was checked grant by grant against both stages. One clause was dropped deliberately: main's comment said api may not write DAG_HANDLES rows, which this branch's cancel teardown must do to free a handle — its surviving reasoning is folded into the kept comment, and the bucket is still attached bindOnly so api never creates it. Both sides also bumped "four buckets" to "five" counting different fifths; the count is now stated with the bucket named. routes/tasks.ts, tasks/sweeper.ts — independent additions, kept side by side. tasks/lifecycle.ts — the one place the two features genuinely interact, since cancellation is what both touch. Import sets unioned; the merged flow orders main's cancelUnheldRun and transitionCancellation against this branch's sandbox release rather than concatenating them. cancel-release-confirmation.test.ts — R9 is the only case reaching the non-root branch, which main now settles through transitionCancellation, i.e. inTransaction: a pooled connection of its own that a db.query stub never sees, so the transaction went through to a real database. Switched to test/support/db-stub.ts, which main added for exactly this, and added an assertion that a transaction was opened so the case cannot pass by returning early. Confirmed it still fails if the branch leaks a `released` field. Verified: utils 76, protocol 94, hands 222, brain 1810, api 1689 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntory Round 43 rejected the premise of the previous fix, and was right to: I had ordered the handle release before retention on the grounds that a failed release leaves everything retryable. A delete whose ACK is lost has committed, so that is not true -- the container ends with no handle, no retention record and no session binding, because the caller provisions a replacement whose pending write takes the binding. The same window opens on a crash between the two, and for a DAG that REUSED the container its handle was also its only way back into that path. So the reference that replaces the handle now lands first. What that costs is a stale handle when the release fails, and that is made recoverable rather than permanent: `replaceDagHandle` takes an optional `mayTakeFrom`, and a registration refused by a workload that has been RETAINED may take the name. Answered from the retention ledger -- what `retainContainer` writes first and what a lost projection is restored from -- because "the release threw" says nothing about whether the delete landed, while the ledger settles whose the container is. An unreadable ledger answers "not retained", keeping the refusal. All four registration sites supply it; a guard nobody routes through never fires. **The occupancy check reads both key names.** Writing the canonical one was half of it: `readHandsEntry` is canonical-first and returns the first non-null it finds, which is right for "which binding is in force" and wrong for "is this slot free". With both keys present and the canonical one a tombstone, an empty PUT or this workload's own row, it never saw another workload holding the legacy name, and the migration then deleted that live binding as the older of the pair. **The inventory understands a pending handle.** `isUsable` already accepts a session row with `status: "pending"` and no endpoint; the DAG-handle conversion did not translate the flag, so an ordinary queued workload was counted `unreadable` -- a census reporting a row it understands perfectly well as one it could not parse. The ordering test from the previous commit asserted the order this reverses. It now pins the settled contract, plus the failure the ordering exists for: a release that throws must not cost the container its retention record. Verified by the ten suites covering these paths (127 tests, 0 failures) and both typechecks. The full suites are not measurable on this host right now -- it is shared and its load average has been 100-330 throughout, which turns every wall-clock assertion into a coin flip; CI is the gate for those. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stale handle **B1: reading both names was not checking both.** The loop broke on the first usable row before knowing whose it was, so a canonical row belonging to this very workload -- or unparseable bytes there -- hid a live row under the legacy name, and the migration then deleted that live binding as the older of the pair. Every name is examined now, anything unreadable or anybody else's refuses, and unreadable is explicitly not evidence that the slot is free. Worth recording what the two names actually are: `handsSessionKey` encodes only ids that need re-keying, so for an ordinary session id the canonical name IS the legacy spelling and there is one name. They diverge only for ids starting with the retention prefix. That is narrower than it first sounded, and it is why an earlier version of this test passed with the fix removed -- it used an invented canonical key, so neither name matched anything and nothing was read at all. **B2 (1): releasing a retention now frees any handle still naming it.** The hand-over frees that handle itself, but a release that did not land leaves it behind -- and the retention record was the only evidence letting a later registration take the name back. Deleting the record when the work finishes stripped that evidence at the moment it stops being reproducible: the session binding is already gone, so nothing re-enters the hand-over path and every replacement is refused for the life of the DAG. **B2 (2): a cancel does not stop a container retention is protecting.** The argument is the review's and it is right: with the handle release landed, this cancel would never have seen the workload at all, so a failed bookkeeping write cannot be what shortens a protected container's life. The check sits beside the existing shared-holder gate, reads rather than infers, and answers `unconfirmed` when it cannot read -- the direction every other unknown on this path takes. That check first went in off-seam and turned all 24 cancel tests into `unconfirmed`, because its failure direction is refusal and no test binds that bucket. It is on `handleRegistry` now, where the file's comment already explains why every registry read on this path has to be substitutable. Three of the four mutations were not caught by the tests I first wrote for them. R26 passed with the retention gate deleted, because the shared-holder check threw first -- so R28 is now the control that proves the gate is what refuses. H26 passed with the loop's early exit restored, for the invented-key reason above. H27 covers the retention-release cleanup structurally, and says why: its behaviour needs a whole keepalive sweep. Verified by the twelve suites covering these paths (167 tests, 0 failures) and both typechecks. Full suites are CI's -- this host has been at load 100-330 all session, which makes every wall-clock assertion here a coin flip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI caught what my local runs could not: L2, L4 and L8 in teardown-must-not-kill-live went red because the retention gate's read was not stubbed there. I had fixed exactly that in the cancel suite and never looked for the other files on the same path -- `grep -rl handleRegistry` names three, and I had checked one. The gate now defaults to "nothing retained" in that file too. Round 45's three findings, two of them flat bugs in what I wrote: **The empty owner was a regression I introduced.** `owner && owner !== workloadId` skips the comparison when the owner is empty -- and a legitimate agent-sandbox READY binding carries `workloadId: ""`, its identity being the Router session. So a SaFE rollback could overwrite a live Router binding with its own PENDING row. The strict inequality I had before was right; the guard I added to it was the bug. **The retention gate never fired.** `hands.retained-*` is not a prefix wildcard: NATS treats `*` as one only when it is a whole token, so that filter was a literal nobody writes and every scan returned empty. Nothing could have caught it, because every test of that gate stubs the method -- the filter itself was never executed. It reads `hands.*` and `retention.*` now and screens by prefix, reads the LEDGER as well as the projection (the ledger is written first and the projection restored from it, so a projection-only read misses a retained container), and throws on an unreadable record instead of skipping it, which is what "unreadable answers unconfirmed" was supposed to mean. **The release order was wrong again, mirrored.** Deleting the retention records before freeing the handle leaves, on a failure between them, a handle with no evidence behind it and no sweep that will revisit it -- the entry is gone from the retention set. Freeing the handle first leaves the retention standing when it fails, and the next sweep runs it again. The opposite residue is harmless: a freed handle whose records outlive it by one sweep. B5 asserts the filter shapes against Brain's own key builders, since a drift there reads as "nothing is retained". It failed on its own comment first -- the comment names the broken filter to explain it -- so it reads code, not prose. Round 45 also corrected two of my premises: an incomplete census does not skip retentions already found, and ids beginning `=` re-key too, so its round-44 two-name findings narrow to re-keyed ids only. Verified by 14 suites, 192 tests, 0 failures -- the list built with `grep -rl` this time rather than from memory. Five mutations, each caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI's red on the last head was mine: the reordered handle release threw bare into
the retention sweep, and the outer handler marked the whole sweep incomplete,
which disturbed the walk's budget and queue. Six roster-tick tests said so, with
a virtual clock, so it was not this host's noise.
Containing it to its own entry was not enough either: in those fixtures the DAG
handle KV is unbound, so the release throws every sweep and nothing is ever
released. That is a missing binding, not a transient failure, so it goes through
a seam -- `KeepaliveDeps.releaseDagHandles`, beside `listDagHandles`, whose
comment already gives the reason ("needs JetStream otherwise").
**Round 46's remaining blocker: the release's return contract.**
`releaseHandlesForWorkload` retried a CAS conflict five times and then returned
as though it had succeeded. The caller deleted the retention records on the
strength of that, so a burst of conflicts ended with the handle still naming the
workload and its evidence gone -- unrecoverable once the conflict passed.
Exhaustion now throws. The early exits are separated while I was there: an
absent row, a tombstone or an empty value means nothing names this workload, so
that IS released; an unparseable row is not, for the same reason unreadable is
never evidence elsewhere in this change.
Round 46 also confirmed what I had asked about and could not settle myself: the
strict `owner !== workloadId` cannot lock agent-sandbox out, because the only
callers of that rollback are inside SaFE's `makeOnProvisioned` and SaFE rejects
an empty id before the hook runs; reading the ledger before the projection is
the right order; and the reverse residue of the release ordering is idempotent,
though it lasts until the next successful cleanup rather than exactly one sweep.
And it caught one I would have missed: cancel-release-no-safe-url passed with
the retention check THROWING -- its log said `retention_check_failed` while the
assertion expected `unconfirmed` anyway. Green for the wrong reason, and the
gate's total absence would have looked the same. Stubbed now.
Verification note: node is no longer installed on this host, so the release
contract above is the one change here not run locally -- typecheck only, before
the toolchain vanished. Everything else was green beforehand: roster-tick 25/25,
dag-handle-ownership 27/27, cancel-release-no-safe-url 1/1 with a clean log.
CI is the check for the rest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ns in Round 47 confirmed round 46's three closed -- it enumerated all four release call sites and found none that relied on the function never throwing -- and found one more of mine. The handle cleanup I put in the retention read phase is a full-table scan followed by a CAS per handle, and only the enumeration carries a limit. The phase's declared worst case is `CENSUS_READ_BUDGET_MS + LIVE_WORK_READ_CEILING_MS` = 50s, and every sweep span and refresh interval is derived from it. A read answering `clear` at 48s followed by a 9s cleanup puts the phase at 57s with nothing having timed out, so configurations near the allowed minimum would be derived from a number the phase can exceed. So the cleanup is bounded on this side of the call, like every other term in that ceiling, and the ceiling now includes the bound. Exceeding it is not a failed sweep: the retention records stay and the next sweep retries, which is the recovery a refused CAS already had. Round 47 also narrowed a claim of mine. "An unparseable row blocks the release" holds for the re-read inside release handling, not generally: the initial scan still skips bad JSON and the protocol layer filters malformed handles, both of which predate this PR. Verification: node is still absent from this host, so this change is unrun locally -- the test added with it is likewise unrun. CI is the check. The previous head is 51/51 green, which covered the release contract that was in the same position a commit ago. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ved anything CI caught what I could not run: `test/keepalive-roster-tick.test.ts(1310,54): error TS2304: Cannot find name 'now'`. I copied the tick invocation from a neighbouring test without its `let now = 0` -- the line reads like context and is actually a declaration. Two more in the same test, neither of which the compiler would have caught: `kv.get(key) !== undefined` asserts nothing. `kv` here is a real KV surface, so `get` returns a promise and a promise is never `undefined` -- the check passes whatever the store holds. The file's own convention is to read the backing map, which is what `values.has(...)` does. And the timing assertion could pass without the sweep ever reaching the release: a bound that does not exist looks exactly like a phase that returned early. It now records that the release was entered and asserts that first, which is the same trap as R26 passing with its gate deleted and H26 passing on an invented key. Still unrun locally -- node remains absent from this host. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard added in the last commit did its job on the first run: "the sweep never reached the handle release, so this proves nothing about its bound". It was right -- the fixture seeded a retention but bound no provider, so the live-work read never answered `clear` and the release was never entered. The timing assertion would have passed on a phase that returned early, which is indistinguishable from a bound that does not exist. It binds a provider that answers with a clear marker now, the way the retention-release tests in this file already do. Worth saying plainly: that guard is the only reason this did not land as a green test asserting nothing. Three of my tests this week passed for the wrong reason -- a gate deleted, an invented key, a promise compared to undefined -- and the cheapest defence has been to assert that the code under test was actually entered, before asserting anything about what it did. Still unrun locally; node remains absent from this host. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Review converged — round 48 returns a clean gate and recommends merge at Forty-eight rounds is a lot for a change that adds one response field, so here is what actually happened, and what a reviewer is accepting. What the field cost
The rule that came out of it: a session entry is not a licence to stop what it names. Rounds 41–48 were spent on interactions with #35 and #36, which landed mid-review and restructured the same functions. Several defects existed in neither parent, only in the merge. Residual risks a merger is accepting
Verification noteNode disappeared from the development host partway through, so the last four commits were written without a local typecheck or test run; CI was the only verifier. That is visible in the history — the ceiling test needed three pushes, and one of those was its own guard catching it asserting nothing. The guard stayed. |
…und it Review found the sweeper's orphan branch cannot stop anything. rememberOutcome writes the unreleased record to the DAG root claw_tasks row before the destroy AND before the stop; unreleasedRecord.mark deliberately throws when that row does not exist, so the gate turns a bookkeeping failure into "unconfirmed" with no stop issued at all. Proven against real Postgres with a SaFE stand-in: root row present, one POST /api/v1/workloads/<id>/stop and the mapping destroyed; row absent and everything else identical, zero requests, five more sweeps change nothing. The detail that decides the severity is that a missing root row is not an odd state — it is the definition of the orphan reapOrphanHandles exists to reap (`status = owner?.status ?? "missing"`). So the one path built to clean up stranded handles was dead for its own primary case and could not self-heal, and DAG_HANDLES is created with no TTL by design, so the dangling reference outlives Postgres. mark now distinguishes "the record could not be written" from "there is nowhere for a record to live" via a typed NoRecordHome, and only the former withholds the stop. The record exists to inform the next reader through the DAG root row; if that row is gone there is no reader, so withholding protects nothing. mark still throws where a row exists — that is what stops a mapping being dropped on evidence that never landed — which keeps this consistent with the reviewer's own finding about clear (checked and agreed not-real: mark and clear carry the identical WHERE, so clear's zero-row case means "no row", i.e. "no record", not a stale one left behind). Four more in the same file: The shared-holder and retention checks moved ahead of both the record and the destroy. They mean "do not destroy", so evaluating them afterwards left a permanent unreleased marker on a container that was legitimately retained or shared, and the DAG reported a leak that did not exist. otherDagHolding and retained() no longer walk the registry serially under one shared deadline. Both did a read per handle per other DAG root against a ttl:0 bucket whose rows only accumulate, so past the timeout every cancel refused to stop anything — and since the interrupt is published only after cancelTask returns, cancel latency grew with the bucket. Bounded fan-out, first-answer-wins. The unstoppable agent-sandbox branch no longer writes a record nothing can clear. Not reachable on this cluster (it runs safe-workload) but real in kubernetes mode, where those DAGs reported "unconfirmed" for life and each rebuild added another row. Elsewhere: replaceDagHandle's refusal guard no longer treats an empty workload_id as evidence that the existing handle is stale, and attemptRemedies sleeps between rollback attempts rather than only in its catch, so three attempts are no longer burned in the same few milliseconds of one transient failure. Two comments claimed api reconciles the DAG_HANDLES bucket. It does not: bindDagHandles attaches with bindOnly, and DAG_HANDLES is deliberately kept out of ensureKvBuckets. Brain owns that configuration for the life of the cluster. Both are corrected, and they say what the real cost is — the same false claim was cited on a sibling review to justify accepting a defect, which is what a fallback nobody implements is good for. Verified: utils 76, protocol 94, hands 222, brain 1817, api 1706 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. Every regression test was confirmed to fail without its fix; the one timing-sensitive assertion was run five times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main now carries #49 (post-v2 cleanup and the CodeQL queue). No conflicts: its 74 files do not overlap this branch's changes in any conflicting region. Verified after the merge: utils 76, protocol 94, hands 222, brain 1854, api 1751 -- 0 fail. Build, typecheck and lint-tests-must-resolve clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`scanPrefix` awaited `kv.get(key)` inside `for await (const key of kv.keys())`. `kv.keys()` is an ordered-consumer subscription; awaiting another JetStream request inside that loop stalls its message pump and the consumer ends early, with no error anywhere. Measured against the live DAG_HANDLES bucket, 21 keys all live: getting inside the loop returned 1, draining first returned 21. Silent truncation is the worst shape this could take, because every registry-wide scan is built on this one call and a short list reads exactly like a small registry. `DagHandleMap.listAll()` is the entry point of `reapOrphanHandles`, so orphan collection has been walking a fraction of the registry; `workloadHeldByOtherDag` and `releaseHandlesForWorkload` scan the same way, which is how a shared workload could be seen as held by a single-key read and unheld by a scan in the same instant. Verified on the real bucket before and after: listAllDagHandles() returned 1 row naming neither of two rows that lookupDagHandle read back fine; after, 23 rows including both. This is pre-existing on main — neither open PR touched this file — and is kept as its own commit so it can be taken separately if wanted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sandbox-stopper: a shared workload survived a concurrent release. Two DAGs holding one workload both read that the other still held it, both deleted their own handle, both skipped the stop, and neither left a record — so the workload kept running with every reference to it gone. Reproduced three times in a row against real NATS/Postgres: zero stop requests, both rows null, the follow-up answering nothing_held, which is the exact false assertion this branch exists to stop inventing, arriving on the feature's own path. That one is a regression from my own previous fix, and running the identical harness against both orderings is what established it rather than argument. With the check after the destroy, "both decline" needs a cycle and cannot happen; moving it ahead of the destroy put all four reads before both writes, so both observations hold at once. The move did buy something real — the old order left a permanent unreleased marker on a container that never leaked — so this keeps the checks where they are and makes the DECISION follow the ACT on the shared-decline branch: once this DAG's own row is provably gone, ask once more. Whichever side removes its row last sees an empty answer and becomes the last holder, and the cycle is back. sweeper: the orphan sweep stopped sandboxes that were still working. Ending a chat retains background shells but leaves the sandbox in the ordinary hands.<session> binding with no retention record, so once no task was running the sweep passed its liveness check and killed a live workload — reproduced with a real background process still running at /stop. Two independent review chunks found this separately. The sweep now reads the session binding's background verdict, with the workload id matched first so a session that has rebuilt cannot hold its dead workload's handle open, and defers for at most one verdict lifetime so a fleet that publishes no verdicts cannot make the sweep permanently dead — which was itself a P1 on this branch. That verdict predicate was moved into @claw/protocol rather than copied, along with the idle-period rules and the field declarations it depends on. Testing bgRunning > 0 alone accepts a verdict from a previous idle period, and two copies drifting apart is a defect shape this repo has shipped before. ensure-hands: a DAG could not recover onto its own sandbox. A create node never consulted its own handle row, so a session whose slot named a different or dead workload sent the DAG to adopt that one and then failed registration against its own handle — a recoverable sandbox failure became a task failure, with an extra workload created and stopped per attempt. The own handle is now consulted first, and acted on only when the two records disagree, which is what keeps cross-DAG warm-pod sharing intact. All four were re-verified after the KV scan fix in the preceding commit, since that changed what every registry-wide scan can see. Verified: utils 76, protocol 101, hands 222, brain 1861, api 1768 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. Every regression test confirmed to fail without its fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ling The orphan sweep deferred a reap whenever ANY task in the session was non-terminal, whatever workload that task was on. So T1 finishes on W1; T2 in the same session changes image, cannot reuse W1 and creates W2; T2 sits preparing; and W1 is held indefinitely with nothing using it. When W1 occupies the GPU W2 is queued for, that is a deadlock the sweep itself creates — the deferral prevents the work that would end the deferral — and it only clears when T2 times out. Reproduced with a one-slot GPU fake, so "T2 made progress" is an assertable outcome rather than a boolean. The guard now asks whether live work holds one of THIS candidate's workloads, from three sources with different failure directions: the DAG handle registry read from the leader (Brain registers at SaFE-id time, while the task is still preparing and its pod may be queued — and the load-bearing answer here is the negative one, which a replica read would get wrong in the one direction that costs a running pod), claw_tasks.sandbox_workload_id (written only once a run reports itself running, so it can only add to the set, never remove), and the hands.<session> binding (a property of the session rather than of a task, so applied separately — a live task can be one read away from adopting that slot). Every way the question can go unanswered defers rather than reaps, each with its own log line: the live-task query throwing, more live rows than the scan limit (a truncated list is a different question, not a smaller answer), a leader read throwing, a live task not yet named by any store, an unreadable binding, and a candidate with no workload at all — which falls back to exactly the old session-wide question. This narrowing was not available before the preceding scanPrefix commit. With the scan truncating to one row out of twenty-three, "who holds this workload" could not be answered, and the session-wide guard was the right call. Verified that prerequisite against the live bucket before relying on it. The comment that argued for the coarse form is rewritten rather than left standing: it names the new question, the deadlock the old form produced, and keeps the original trade explicit — the sweep still never permits a stop it cannot establish is safe; what changed is that "cannot establish" now means the answer was unavailable rather than that some unrelated task was running. Ten new cases, three confirmed failing at baseline, and the must-not-regress guards proven to bite by a seven-mutation matrix — including that a genuine orphan is still reaped and that the background-work guard still fires. Verified: utils 76, protocol 101, hands 222, brain 1861, api 1778 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four defects an independent review reproduced. Three of them are gaps in fixes landed earlier on this branch, and all three end the same way: a sandbox with background work still running in it gets stopped. Each of those fixes was verified; none checked whether the evidence that work is live survives the path it added. The last-holder re-check asked too narrow a question. It re-asks whether any other DAG holds the workload, but reuses the `retained` value computed before the decline — so when A drops its handle and B then hands the workload to retention and releases the last one, the re-check stops a container whose background work is now retained. Reproduced through the production retainContainer and handle-release functions: the retention record is still valid, /stop is issued, and the call answers "confirmed". The re-check itself stays — it closed a race where a shared workload survived a concurrent release with every reference deleted — it now just re-reads retention too. The bounded fan-out bounded only its second half. The listAll() that runs before it is still a serial read-per-key, so the 10s budget can be gone before the concurrent phase starts: measured with 1,400 handles and 8ms per read, the whole stop path returns "unconfirmed" at 10,011ms having issued zero stops. The own-handle recovery path returned a sandbox without restoring the bookkeeping that keeps it alive. With the session record expired and the DAG handle still naming a healthy workload, recovery succeeded, markHandsIdle then answered "gone", the orphan sweep read no_binding as reclaimable, and stopped a workload with live background processes. It now restores the binding before it returns. And a scan that could truncate silently: isValidDagHandleToken awaited kv.get() inside `for await (kv.keys())`, the ordered-consumer stall this branch already fixed twice. A truncated scan there does not fail loudly — a token that IS valid is not found, registry.ts caches that in deniedTokens, and the sandbox.use node it belongs to is answered 401 from then on. api's infra/dag-handles.ts had the same shape and is fixed with it. Every other KV scan in the tree was checked and already drains first. Reported rather than fixed, because no correct fix exists in the file that would hold it: switching image stops a sandbox that still has background shells. The reuse path abandons W1 for a replacement and the new binding overwrites W1's background evidence, so the guard is not wrong about what it reads — what it reads stopped being able to answer for W1 the moment W2 was bound. The fix belongs in ensure-hands.ts, where retainInsteadOfDestroying already does the right two things: write the retention before the slot moves, and release the handles naming that workload so this sweep never reaches it. The sweeper comment now names that function, and the api-side contract it has to satisfy is pinned by a test. Verified: utils 76, protocol 101, hands 222, brain 1865, api 1788 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last of the four defects from this review round, and the one whose fix did not live in the file that found it. When a rebuild abandons the session's sandbox rather than releasing it — the spec-changed and unhealthy paths, both of which return null at `skipped_other_owner` because another DAG still holds the workload — the caller then writes the new sandbox's binding over `hands.<session>`. That slot is a single slot, so the moment it moves it can no longer answer for the old container, and the api-side background-work guard reads `other_sandbox` and lets the cleanup through. A sandbox with the user's background shells still running in it is stopped. Reproduced through the production reuse and registration paths. The container is now retained before the slot can move, so the evidence that it is working stops depending on a slot that is about to name something else: the retention is keyed by the container's own generation, carries the whole binding and the live-work verdict, and is what api's retained() already consults and refuses a stop on. Two deliberate departures from the plan this was handed with, both checked against the source rather than assumed: It is gated on the live-work count, not unconditional. `clear` leaves the path byte-for-byte as it was and the container is still reaped — retaining everything would break a rebuild that legitimately should discard the old sandbox, and re-create the GPU deadlock the orphan sweep exists to break. It does not release the handles naming the workload, and retainInsteadOfDestroying grew a parameter so it can say so. That function's own comment records its precondition — it is only reached when entryOwnedByAnother said no other DAG holds the workload — and this new call site is reached precisely when that check said the opposite. Releasing here would take a live sibling DAG's only reference to the sandbox it is running in: the same mis-stop entryOwnedByAnother exists to prevent, by a quieter route. Leaving the handle is an already-supported state — replaceDagHandle may take the name from a retained workload, and api's own comment says a handle still naming a retained container is a bookkeeping failure rather than a licence to stop it — and the keepalive retention sweep frees it when the work finishes. What collects the container was read, not assumed: runRetentionReadPhase makes every retention a census target, re-runs countLiveWork inside the container each sweep, and on `clear` releases the handles and then the retention, in that order. Verified: brain 1869, api 1788 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. Two of the four new cases fail without the fix on the outcome that matters (the workload stopped while its background shell was alive); the other two are controls that pass either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zoroyihan7
force-pushed
the
feat/cancel-release-confirmation
branch
from
September 17, 2026 18:04
ed43a2e to
59a3b10
Compare
…t get back Five defects an independent review reproduced, all in code I added over the last two rounds. Four of the five are the same mistake wearing different clothes: a signal that is close enough to look like an answer being used for a question it cannot actually answer. retainDisplacedSandbox read a binding and then deleted the key unconditionally. Between those, another caller can move the session's one slot to a new sandbox — so the delete took the NEW binding, and the replacement's background protection went from running to no_binding even when no replacement had been created. The delete is now conditional on the revision the decision was read at. The retention collector freed every handle naming a workload the moment its shell count came back clear. But a shell count answers whether anything is RUNNING in the container, and the question before a release is whether any DAG still HOLDS it — a DAG sitting between two nodes runs nothing and still owns the handle its next node resolves, so clear is silent about it rather than negative. The collector now re-reads the handle table immediately before the release and keeps both the handle and the retention records when anyone still holds it. That test is a handle read and not a lease read, and the difference is not stylistic: runScope is the run gate's key, ws.<workspaceId> under the default, and a HandleInfo carries neither the lock key nor the workspace — so a lease lookup keyed off a handle would answer "no lease" for a perfectly live sibling and delete its handle. That is the defect, not the fix. It sits at the call site rather than inside releaseHandlesForWorkload because the other three callers need the release to be absolute: each runs after a confirmed stop, where a handle left naming a dead workload makes replaceDagHandle refuse every replacement for the life of that DAG. Only the keepalive caller frees handles for a workload that is still alive, and only it knows that. The last-holder re-check made its two reads parallel, which changed which orderings are possible without making the pair atomic: retention could answer false, another holder could then write the retention and release its handle, and the DAG read that followed would answer null. And a credentials read that failed returned without recording anything, on a path that may already have dropped its own handle — leaving both handles gone, no stop issued, and every later teardown answering nothing_held. Also: a sandbox the provider had already confirmed dead was retained for ever, because counting live work in a dead container answers unknown, and unknown retains — so the collector could never reach clear either. Verified: brain 1877, api 1790 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. Every fix has a regression test confirmed to fail without it, and each guard has a control proving it still lets the ordinary case through — an unheld retained container is still released and collected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zoroyihan7
force-pushed
the
feat/cancel-release-confirmation
branch
from
September 17, 2026 18:09
59a3b10 to
fdfc038
Compare
Two regressions from the commit before this one, both reproduced against real NATS, and both found by measuring rather than reading. A dead shared sandbox's stale handle blocked its own replacement. The previous round stopped retaining a container the provider had confirmed dead — correct, since a retention over a dead container is a record nothing can ever release — but the new branch returned without freeing the handle that still named it. replaceDagHandle will not point a name away from a workload still on record, so the next registration was refused and the replacement workload was created and immediately stopped. Measured: at the previous commit a replacement registers; at HEAD two consecutive retries both fail. The displaced-sandbox path refuses this release for a good reason — a sibling may be running in that container — and the provider's confirmation is what removes it. A handle naming a dead workload is not a reference to anything: the sibling's next node probes it and fails either way, and keeping the row also stops that sibling ever re-registering. Releasing is strictly better for the sibling, not merely acceptable. The confirmation itself is narrow enough to rely on: only a Router 404/410 raises SandboxGoneError, while an unreachable backend is a 502 and never reaches this branch. And the holder check added last round doubled a scan rather than replacing one. dagsHoldingWorkload enumerated the handle table and then releaseHandlesForWorkload enumerated it again internally, both inside the one release budget. Measured against a real bucket with 200 rows and replicated-read latency: one pass is 201 gets and 5.48s, so two passes miss the 10s ceiling — the baseline released on its first sweep, HEAD timed out on two consecutive ones, and a timed-out release never runs the retention delete either, so an idle container stayed retained and pinged for ever. The holders now come from the same pass the release uses. The agent that wrote the doubled scan stated in its own report that the holder read happened "in place of the release, never alongside it". It had not checked that the release enumerates internally. That is the second unverified-cost claim on these branches, so the fix for this one is measured, not argued: the numbers above are from a run, not an estimate. Verified: brain 1880, api 1790 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. Each fix has a regression test confirmed to fail without it, and a control proving the displaced-sandbox path still retains rather than releases while there is a live container to argue about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reproduced against real NATS and a real TaskRunner: a sibling DAG registering a handle concurrently costs five CAS conflicts, the release throws, and the task reports failure and ACKs. No nak, no redelivery, and the race would have cleared in milliseconds. At the commit before the gone-path release, the same scenario registers its replacement and continues. The release's failure propagates rather than being swallowed, and that stays — a release that cannot commit means the registration after it would be refused anyway, so throwing names the cause one GPU create earlier. What was wrong is that the throw could not say what kind of failure it was. Its message read "N attempts exhausted, or its row could not be read": honest about not knowing, and the not-knowing was the defect, because those two need opposite answers and nothing downstream could tell them apart without matching on prose. So the throw site splits them, since it is the last place the difference still exists: a genuinely contended row raises DagHandleContendedError, a row that is null or not an object raises an ordinary Error naming it. replaceDagHandle in the same file already separates its own two this way. The policy — a lost race earns a redelivery — is one line in isRetryable beside the other retryable classes, matched by name rather than by import, so the retry policy does not end up pointing at the module that owns the DAG_HANDLES bucket. Every other way that release can fail was enumerated and left terminal: a malformed row, a non-JSON value, the not-initialised boot-order error, the scan timeout, and raw KV errors. One case moves from terminal to retryable, and one is split out of the same throw specifically so it does not move with it. The asymmetry is why the direction matters: a nak spends one delivery from a bounded budget, an ack ends the task and no later reader can tell it existed. The retry converges, read rather than assumed: a SaFE workload id is never reissued so the probe answers dead again and the same branch is selected; a partial release is not redone because the scan only enters rows that still name the gone workload; and the winning DAG's registration is simply not one of them. Also corrected: that function's own doc asserted "It is also retryable: the next attempt re-probes, gets dead again, and tries the release again." That was false at HEAD — it is the guarantee this commit makes true. It is the fourth comment on these branches to assert a mechanism that did not exist. Verified: brain 1883, api 1790 — 0 fail. Build, typecheck and lint-tests-must-resolve clean. The regression test asserts the delivery was naked and the retry succeeded, not which error class was thrown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y it Three findings from the same review round, all downstream of the previous commit's error classification: it named a contended DAG-handle row as worth a redelivery, and the three ways that judgement failed to take effect. The default path did not consult it. BRAIN_LAZY_SANDBOX defaults to true and is true in the deployment, so an ordinary chat turn opens its sandbox at the first tool call -- inside agent-loop's tool-dispatch catch, which renders anything thrown as result text for the model. The class became a sentence, the model read it, the turn ended normally, and the delivery was acked and reported failed: false. The eager path naked correctly the whole time; which of the two raised the error is not a distinction a redelivery should turn on. The open is now tagged with SandboxAttachError and the loop rethrows its cause when isRetryable accepts it -- narrow on purpose: a permanent open failure is still better told to the model, and a transient failure of a tool that RAN is still a tool result. A scan that overran its ceiling was classified as permanent on the reasoning that the store had failed. Measured, it does not hold: with a 10.5s enumeration delay injected the release failed at ~10,005ms and acked, and with the delay lifted the same release completed in 2.89ms. A timeout produces no read at all and forecloses nothing about the next one -- and the busy bucket that overruns a scan is the same one that loses a CAS race, so the old grouping denied a second delivery to the condition most likely to need it. Both scan sites now raise DagHandleScanTimeoutError so they cannot drift apart. And the redelivery itself could strand a record. releaseHandlesForWorkload walks one DAG row at a time with no transaction over the set, so a release that frees a sibling's name and then exhausts its attempts on its own row throws with the first deletion already durable. The retry re-runs against a table where the sibling reference is gone, entryOwnedByAnother answers false where it answered true, and the branch that knew the container was absent is not taken -- leaving mayDestroy to read unknown from a container that is not there and write a retention nothing can ever release. The gone check now also sits before that retention, after the destroy that a gone container still needs. Three comments claiming a scan timeout was permanent were corrected rather than left to outlive the behaviour they described. brain 1893 pass / api 1790 pass / 0 fail. Each fix reverted individually to confirm its test fails without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three findings, all created by the previous commit. It made a failed lazy sandbox open reach the task runner so the delivery could be naked -- and a nak re-runs the whole turn. The P1: the rethrow leaves the loop before the turn's results are appended to the history, so a redelivery repeats every tool that already completed in that turn. For a read that is waste; for an MCP call that wrote outside this process it is a second write, measured as a doubled external append. The eager path is safe from this for a reason rather than by luck -- it opens the sandbox before any tool has run. So the rethrow now carries that condition with it: nothing in this run may have executed yet. Otherwise the redelivery is given up and the model is told, which is what shipped before any of this. A lost retry beats a side effect nothing downstream can undo, and a resumed run -- whose earlier work is in the recovered history -- stays on the same conservative side. Tagging the open also broke the stage that needs no loop at all. Marketplace tool installs, plugin resolution and pre-run hooks call sandbox() before the agent loop starts and propagate straight to the runner, where the wrapper's name replaced the class isRetryable matches on: a previously-retryable contention became terminal. isRetryable now unwraps the tag. That stage is also the safest place to retry, since nothing has run yet. And the sub-agent entry point opens the sandbox itself and renders its own errors as text, so a turn whose first tool is a `task` -- the ordinary shape for a delegating agent -- still lost its redelivery. Same guard, same call. The earlier tests missed all three because they drove a stubbed router: they never ran a second tool, never resumed, and never took the task branch. The new ones do. brain 1897 pass / api 1790 pass / 0 fail. Each fix reverted individually: the side-effect guard fails 2 tests, the task branch 1, the unwrap 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard added last commit asked "has anything in this run executed yet" and read `totalToolCalls` for the answer. That counter is incremented after the start event is published, and `runTaskTool` is dispatched with Promise.all over a batch -- so a sibling suspended inside its own start event had not reached its increment. The first task's failing open read 1, concluded it was alone, and naked. Promise.all does not cancel anything: the sibling then started, ran its sub-agent and wrote, and the redelivery wrote again. Reproduced against a real ledger: two rows where the baseline has one. The counter was never wrong; it was answering a different question. It is a reporting figure, published after the fact and restored from a checkpoint, and neither property is a defect there. So the guard gets its own registration instead: `toolsStarted`, incremented as the first statement of both tool paths, before any await can yield. An async function runs to its first await when it is called, so by the time `batch.map` has built its promises every task in the batch is counted. Both counters are consulted, because neither covers the other's half. `toolsStarted` knows what this process has begun, including a sibling that has not published anything yet; `totalToolCalls` arrives from the checkpoint and knows about work done under an earlier delivery, which `toolsStarted` has no memory of. The new test holds one task inside its start event and fails the other's open during that window, which is the interleaving as reported. brain 1898 pass / api 1790 pass / 0 fail. Reverting the new predicate fails that test alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
weilei0120
approved these changes
Sep 21, 2026
zoroyihan7
added a commit
that referenced
this pull request
Sep 21, 2026
Two branches that changed the same three mechanisms, resolved by keeping the question each side was answering rather than the code each side wrote. The retry strip in lifecycle.ts is a union: this branch dropped `sandbox` and main dropped `sandbox_release`, for the same reason stated twice -- both are evidence about a workload the PREVIOUS run held, and inheriting either one attributes a leak to a task that never had a sandbox. reapOrphanHandles keeps main's rewrite -- the registry census, the unreleased and deferred counters, and the task_id keying whose old `dag_node_id` predicate matched a different row for half the handles -- with this branch's leadership check re-applied at the top of the iteration. Its accounting test moves from `sweeperPorts.handleMap` to `handleRegistry.listAll`, because after the rewrite the port is not what the traversal reads: a test stubbing it would have driven a loop that ignored the stub and passed for no reason. `sweeperPorts` loses the entry for the same reason. reapPendingHands ends with ONE ownership gate, not two. Both branches built one -- this branch by comparing when the entry was stamped against when this run asked, main by comparing the task that wrote it -- and they disagreed about an entry carrying no task id: this branch skipped it, main reaped it. Main is right, and the argument is not the one either side made. Every entry this build writes carries both a task id and a runScope, so a predecessor's entry is named by its own task and the age test has nothing left to decide. An entry with NEITHER field can only come from a process older than both, and `runScope` is what `collectAbandonedPending` collects by -- so skipping it does not defer the teardown, it leaks the workload for good. The fixtures that modelled a predecessor without a task id were modelling a row this build cannot produce. One fix on top of the merge, which the merge is what made small: main gave `stopNamedSandbox` an outcome, and `destroyHands` consults it, but `collectAbandonedPending` was left on the old void contract and read "it returned" as "it stopped". It returns normally when it cannot address the entry and when the deployment can issue no stop at all -- so the collector could revoke the token and delete the entry behind a workload still running, which is the leak it exists to end, reached through its own cleanup. The comment above the catch already made this argument for the throwing branch; it now covers both. Also drops two Chinese comment lines that reached main through #46. brain 1925 pass / api 1940 pass / 0 fail, build and lint clean. Reverting the collector fix fails its new test alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zoroyihan7
added a commit
that referenced
this pull request
Sep 21, 2026
…eld it lacks codex found two defects in the work that answered the last review, and a third older one. Both of mine are here. The merge that combined the two ownership gates kept only the task-id one, on the argument that an entry carrying no task id could only come from a process older than both fields and so had no collector behind it -- making a reap the only thing between the fleet and a permanent leak. That argument is refuted by this branch's own history: 94b63ef wrote `runScope` into the pending entry and not yet `taskId`, so a rolling upgrade across that commit produces exactly the scoped-but-unnamed entry it said could not exist. The merge reaped it, which is the mis-kill this branch was opened to stop -- a lazy chat turn that never asked for a sandbox, failing, and tearing down the workload a sibling is still queueing for. The first correction deferred every scoped entry to the collector and was wrong the other way, which a test that was already there caught: the bucket's TTL is DEFAULT_BRAIN_REGISTRY_TTL_MS, 5 minutes, and collectAbandonedPending does not look until SANDBOX_PENDING_ABANDONED_AFTER_MS, 2 hours. An entry nobody refreshes evaporates hours before the collector could reach it, so deferring it is not deferral -- it is the workload leaking with nothing left that names it. So the gate is neither field: it is whether the lease behind the entry is still held, read the same way the collector reads it (readRunLeaseState over lock.<runScope>). Held means somebody is alive and the workload is theirs. Free, unscoped or unreadable means this path is the only teardown it will get. The second: the `collected` event added last commit fired even when deleteHandsEntryIfRevision lost its CAS -- the workload was down but the record it was asked to remove was still there, which is the same overclaim the split was made to end, one step further along. It now returns before that line. Not fixed here, and not this PR's: codex also reports that handleRegistry's retained() treats a stale-replica empty read as "no retention", which can let a protected container be stopped. That code came in with #46 and is already on main (0d57088); it belongs in its own change rather than in a merge-resolution commit. brain 1929 pass / api 1940 pass / 0 fail. Each fix reverted individually fails its own test and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was asked, and what it turned out to require
The handoff asked for one field: let a caller tell an accepted cancellation whose sandbox stop failed from one that worked.
POST /v1/tasks/:taskId/cancelnow carries it.{ "ok": true, "cancelled": 1, "released": "confirmed" } // the stop returned 2xx or 404 { "ok": true, "cancelled": 1, "released": "unconfirmed" } // logged and swallowed today { "ok": true, "cancelled": 0, "released": "nothing_held" } // no handle was ever recordedMaking that field true turned out to require fixing the teardown it reports on. The short version:
Reporting on top of that would have turned a silent leak into a confident
nothing_held, which is worse than the silence. So the wiring is fixed here, and so is what fixing it exposed.What the diff contains
The reporting.
safeStopWorkloadreturns its outcome instead of only logging it;stopAllHandlesForDagaggregates —confirmedonly if every handle was confirmed and nothing is outstanding on record and no handle appeared during the teardown.Never inferring release from the handle map, in both directions.
destroyruns before the stop, so the map is empty either way. The handoff warns against reading that as success; it arrives just as easily asnothing_held, which is the subtler half. An outcome short ofconfirmedis recorded on the DAG root'sclaw_tasks.metadata— durable, atomic per row, keyed by workload identity, digest-keyed so a handle namedtokenis not redacted away, and visible throughGET /v1/tasks/:taskIdso the caller learns which handle leaked.Teardown no longer destroys live sandboxes. Activating the path exposed four ways it would: the sweeper reaping standalone tasks mid-run and pods the next task in the session had reused,
agent_donereleasing a handle a sibling was still using, and re-cancelling a finished DAG stopping what a newer DAG had adopted.Handle ownership. Reuse never registered ownership at all, so a DAG that adopted a warm sandbox held one the map did not name. Rebuild left the map naming a stopped workload, because the replacement's registration hit
create's overwrite refusal and the exception was swallowed. Both are fixed, with areplacethat is a single write — destroy-then-create leaves the handle briefly absent, and an absent handle is exactly how teardown concludes a DAG holds nothing.Concurrency. Handle removal and registration are both revision-conditional now; previously a plain read-modify-write on either side could discard a handle the other had just committed, losing a live workload's only reference.
Known-open, and deliberately not claimed otherwise
nothing_held. Closing this needs explicit acquisition phases with per-attempt identity and a cancel barrier the API can read — reviewed and agreed as its own change. A cheaper marker was designed and rejected: it would have made a DAG readunconfirmedfor ever, with no failure involved.confirmedmeans SaFE accepted the stop, which is the handoff's own definition and the strongest thing SaFE's API can be asked. It is not proof the GPU is free: teardown is asynchronous under a finalizer, and the apiserver's read is database-backed, so a verification read cannot establish it either. That reasoning is recorded at the call site so it is not reattempted.DAG_HANDLEShas no TTL and has never been swept, because the sweeper has been reading the wrong bucket for its whole existence. The first sweep after this lands walks the accumulated backlog, and its destroy condition includes a DAG root row that is merely missing:nats kv ls DAG_HANDLES | wc -lA large number means a burst of SaFE stop calls, and entries of unknown age whose owners are gone will be treated as orphans.
Review
Twelve adversarial review rounds; 34 defects fixed, several of them regressions introduced by earlier rounds' fixes and caught by later ones. Key fixes are mutation-verified — the test is reverted against the fix to confirm it actually fails. CI green on every commit; 2619 tests and every
claw/scripts/lint-*.shpassing.🤖 Generated with Claude Code