Skip to content

Land the workflow-run substrate latency fix and remove the trivial agent path#81

Open
alexanderguy wants to merge 101 commits into
mainfrom
intr-256-land-the-workflow-run-substrate-latency-fix-and-remove-the
Open

Land the workflow-run substrate latency fix and remove the trivial agent path#81
alexanderguy wants to merge 101 commits into
mainfrom
intr-256-land-the-workflow-run-substrate-latency-fix-and-remove-the

Conversation

@alexanderguy

@alexanderguy alexanderguy commented Jul 5, 2026

Copy link
Copy Markdown
Member

Summary

Lands the workflow-run substrate latency fix and removes the trivial
in-process agent path, then builds live source rotation on top of the
resulting supervised single-step deployment and cleans up the wire
surface the removals left behind.

  • Flattens per-message substrate cost. Delta-scoped claim-check
    validation becomes the production default, repo-store commit trees
    assemble without the on-disk git index, a per-repo object cache backs
    the git calls, and the substrate mirror slices new turns from the
    reactor's in-memory array instead of reloading the conversation from
    disk each message. Per-message growth flattens and the dispatch loop
    no longer wedges as history grows; exactly-once is preserved and the
    index-free tree assembly is proven byte-identical to canonical git.

  • Removes the trivial in-process agent path. Single-agent instances
    now deploy as single-step workflows at the head. The in-process
    session runtime, the warm-harness launch entry point, the session-start
    wire path, and the dead agent-metadata persistence layer are retired,
    and the orchestrator's trivial deploy branch is deleted.

  • Rotates inference sources on a running deployment. A supervised
    single-step child swaps its warm agent's sources in place on a
    sources-updated control frame, with a mutable sources reference
    covering the cold pre-first-build window. The rotation routes
    hub → sidecar → supervisor → child, and the sidecar answers the inbound
    sources.update with session.ack / session.error.

  • Hardens the supervised deployment lifecycle. A spawn that throws
    after the child process starts but before the ready handshake routes
    through the single teardown owner, so a failed startup neither orphans
    the child process nor leaks its mail subscription. A source rotation is
    persisted to the deployment record before it is committed in memory, so
    a failed persist leaves no partial rotation, and the rotation survives
    both a recycle respawn and a full sidecar restart.

  • Hardens deploy against concurrent same-address deploys. An
    in-flight reservation rejects a second deploy for an address still
    being provisioned, and a failed single-step spawn reverses the
    recorded hub key so a retry starts clean.

  • Answers malformed inbound control frames. A malformed request or
    pack frame is answered through its correlation key
    (session.error / agent.error / repo.pack.reject) so the hub's
    request no longer hangs to its timeout instead of being silently
    dropped.

  • Retires the orphaned session.abort / grants.update wire
    surface.
    Retiring the in-process session runtime left the hub able to
    send these two frames with no live sidecar receiver. Their senders,
    frame types, and test mocks are removed, and the docs that described the
    retired grant/abort mechanism are corrected.

  • Serializes the substrate-mirror entry points. The awaited
    onRunBoundary mirror and the fire-and-forget onStateChanged mirror
    shared the mirrored-count state with no serialization; both, and the
    restore that can re-enter the mirror, now run on one per-instance tail
    so overlapping runs cannot collide on a boundary and drop a turn.

Verification

  • make all is green on HEAD: lint (prettier + eslint, including
    gen-api-docs --check), build, admin-ui build, and the full test
    target — 3814 package/unit tests and 344 integration tests pass with
    zero failures.
  • The branch is reviewed as a whole against main over its full range,
    and each commit was reviewed as it landed. The reviewer-of-record
    audits are clean: commit-message style across every commit (no
    prefixes, filenames, tracker references, cross-commit references, or
    over-length lines), a --stat scan for binary or out-of-scope files,
    and the main..HEAD scope.
  • The whole-branch review found no correctness defects. The findings it
    surfaced were comment and documentation staleness left by the
    removals; the branch-caused ones are corrected here (the trivial-path
    comment residue and the harness-design restore-model doc), and the
    remaining lower-priority residue is tracked in INTR-264.
  • The supervised-lifecycle and rotation-durability fixes are covered by
    regression tests verified to fail without the fix, including the
    spawn-teardown subscription release, the first-build-vs-rotation race,
    and the persist-fails-leaves-no-partial-effect atomicity guard.
  • The two substrate-mirror concurrency defects are covered by
    barrier-driven regression tests verified to fail without the fix.

Follow-ups

  • INTR-261 — the dead session.abort / grants.update wire surface is
    removed on this branch. What remains under the issue is deciding the
    supervised replacement
    for each retired semantic (instant live grant
    re-point; a supervisor cancel/drain for abort), which is a design
    decision, not cleanup.
  • INTR-257 — reject base-type / delete-shape mismatches in the delta
    tree write (guard implemented on this branch).
  • INTR-264 — reconcile the remaining stale architecture docs and
    comments left by the trivial/in-process removal (the
    unified-execution-host design doc, the harness-design package-structure
    section, and a dead supervisor re-export). Documentation only; no
    correctness change.

Closes INTR-256
Closes INTR-262

The baseline built its isogit ContextStore without a signer, so its
per-turn context commits skipped the sshsig the unified path pays on
every turn. That understated the baseline floor and inflated the
measured unified-minus-baseline delta. Generate a keypair and pass a
CommitSigner mirroring the production agent-repo signer so both paths
pay the same per-commit signing cost.
The claim-check walk re-reads, re-parses and re-validates the entire
retained consumed dedup index on every commit, so the per-turn
substrate cost grows with the retained set. Behind the experiment flag
BENCH_DELTA_SCOPE_CLAIMCHECK (default off), validate only the
per-commit delta against the prior tree, keyed by (filename, blob OID):
retained entries are skipped via OID equality (the immutability proof
that preserves exactly-once), added entries are parsed and dedup
checked, removed entries are checked below the watermark. A new
optional priorListDirOids closure on the validatePush args surfaces
prior blob OIDs from the prior commit's tree in a single read. With the
flag off the walk is byte-identical to before.
The delta-scoped claim-check path accepts a below-watermark prune that
the exhaustive suffix guard would reject; the domain owner confirmed the
relaxation is structural (redundant for exactly-once). Its safety rests
on the prune boundary being strict at receivedAt >= watermark, mirroring
the strict receivedAt < watermark stale-reject so the entry exactly at
the watermark is both retained and not stale-rejected. Assert both sides
of that boundary under the delta flag so a later refactor cannot widen
the comparison and open a reprocess gap, and record at the check which
structural properties the suffix guard provided that this path drops.
The latency benches spawn a sidecar subprocess through a curated env
allowlist that never inherited the parent process env, so setting
BENCH_DELTA_SCOPE_CLAIMCHECK on the bench command never reached the
process that runs validatePush. The supervisor owning every workflow-run
write lives in that sidecar, and the child proxies its writes to it, so
the flag gates a module const the benched process was always reading as
unset -- the delta path stayed dormant and the per-leg slopes measured
the exhaustive walk regardless of the flag.

Forward the flag through the spawn env so parent and child agree, and
emit a one-line startup marker from the sidecar reflecting the resolved
module const, so a run's effective state is observable directly instead
of inferred from timing. Unset stays unset, preserving default-OFF.
The repo-store passed no isomorphic-git cache to its git.* calls, so
every git.add / remove / updateIndex / commit / listFiles re-read and
re-parsed the whole on-disk index from scratch. On a workflow-run repo
the per-message write sequence flips between the events and workflow-run
refs, forcing a full resetIndexToRef each leg, so the repeated index
re-parse grew the per-message cost with accumulated history.

Give each repo directory one long-lived cache object and thread it
through every object, index, and commit call the store makes. The store
is the single writer under withRepoLock, and the on-disk repo stays
authoritative on every axis the cache touches: the index is persisted on
each mutation and stat-guarded on read, cached objects are OID-keyed and
content-addressed, packs are enumerated from disk per read, and refs are
never cached. So the cache is a pure accelerator that can be dropped at
any instant without changing a result.

Two bounds keep a warm store from retaining parsed packfiles without
limit: a dir's cache is rebuilt after a fixed number of calls, and the
store keeps a bounded, least-recently-used set of dir caches. The cache
is also dropped after a received pack, which writes objects and advances
a ref past the cache, so the next read rebuilds from the mutated repo.

The ref-reading calls (resolveRef, listBranches, listTags,
currentBranch) take no cache parameter and read only refs, so they are
left untouched.
writeTreeUnderLock built each commit by resetting the single shared
on-disk index to the target ref, staging every file, and committing the
index-derived tree. Because one index is shared across a repo's refs, a
workflow-run repo that alternates the events and workflow-run refs every
message reset the whole index each leg, and each staged blob
re-serialized the entire index, so per-message latency grew with
accumulated history.

Assemble the commit tree directly instead. Pin the ref tip under the
lock, splice the write's blobs and its clearPrefix deletion onto that
commit's root tree while reusing every untouched subtree by object id,
and commit the assembled tree oid via git.commit({ tree }). The index is
off the write path entirely, so the per-commit cost tracks the size of
the change rather than the repo.

validatePush now sources its prospective-tree closures from the
assembled tree oid — the exact tree the commit will carry — while the
prior-tree closures still read the parent commit. The assembly writes
only unreferenced blob and tree objects until the commit lands, so a
rejected push needs no rollback: it leaves the ref, the index, and the
working tree untouched. The working tree is still materialized for the
paths a write touches (the workflow-run claim-check scan reads those
files from disk), but only after validation passes.

This removes the index-reset, path-snapshot, prefix-clear, file-staging,
and working-tree-rollback helpers, along with the ref-to-index cache
they served.
Every claim-check leg — enqueue, dequeue, markConsumed, replay —
cleared and rebuilt the whole addresses/<addr>/ subtree from a full
merge output that re-supplied every consumed/ dedup entry's bytes, and
read the whole subtree (readAddressSubtree) to run its dedup checks. So
each leg's cost grew with the retained consumed set, which grows one
entry per message.

Add a writeTreeDelta primitive: computeDelta runs under the per-repo
lock against the pinned parent tip and returns the exact files to put
and the paths to delete; the substrate assembles the new tree by
splicing those onto the parent root tree and carrying every untouched
entry — sibling subtrees and sibling blobs alike — forward by its
object id. assembleTree now takes a delete-set (exact files or subtree
prefixes) rather than a single clear-prefix; writeTree and
writeTreePreservingPrefix map their clear-prefix onto it and keep
byte-identical behavior.

Rewrite the four claim-check ops on top of it. Each names only the
entries it moves — enqueue puts one inbox entry; dequeue moves inbox to
processing; markConsumed moves processing to consumed and advances the
watermark; replay moves processing back to inbox — so the untouched
consumed/ index is carried by oid instead of re-hashed and
re-materialized. The dedup checks read filenames and blob oids from the
parent tree (consumed/ is keyed by messageId, an exact lookup; inbox/
and processing/ are the bounded queues), reading blob bytes only for
the one entry a leg actually moves. markConsumed still reads each
consumed entry's bytes to find the below-watermark tail to prune — its
filenames carry only the messageId, so the receivedAt lives in the
bytes — which is the one leg that still scans the consumed index.

A new OID-equivalence test pins the delta path byte-identical to a
full-replace writeTree across the move shapes, the same guard the
index-free assembly carries.
The delta primitive silently resolved contradictions the way
assembleTree happened to: a put and a delete of the same path, or a
put under a deleted subtree, both let the put win, and a delta path
outside the declared change scope would let a scoped handler skip a
region the write actually mutated. Both are silent-divergence risks
on the exactly-once path even though the claim-check callers never
produce them.

Reject them loudly. writeTreeDelta now throws on a path that is both
put and deleted, or a put that lands under a subtree-prefix delete,
and throws when any put or delete falls outside changedPathPrefixes.
It also pins the parent tip exactly once under the lock and threads
that single oid into the assembly, so computeDelta's dedup reads,
validation, and the committed tree provably share one pre-image
rather than relying on two resolves returning the same value. A
delete of an absent path is documented as an idempotent no-op,
matching the working-tree removal.

Extend the guards' tests: OID-equivalence now also covers a
standalone exact-file delete and a cascade delete that empties a
directory and its parent, plus the two ambiguity shapes and the
out-of-scope rejection. Add the two claim-check rejections that
lacked a behavior test — duplicate_inbox and replay_collision.
The delta claim-check write is O(delta), but under the delta-scope
validation flag validateClaimCheckSubtree still computed each
PROSPECTIVE consumed entry's OID by reading and hashing its blob, so the
per-leg validation stayed O(consumed) even though the prior side already
took its OIDs straight from the tree listing. buildTreeReadClosures only
exposed readBlob/listDir for the prospective side — there was no
prospective counterpart to priorListDirOids.

Add listDirOids to buildTreeReadClosures: the assembled prospective
tree's readTree listing already carries each entry's OID, so surface it
exactly as the prior side does. Thread it through validatePush and route
the prospective consumed-OID resolver through the listing, falling back
to hashing only when a caller does not supply it (a hand-built
validatePush in a test). The prior and prospective resolvers now share
one listing-or-hash helper. An OID read from the listing is the same
content-addressed id hashing the bytes would produce, so validation is
unchanged; it just stops re-reading the retained consumed set, making
the delta-scoped enqueue and dequeue legs O(delta) on validation too.
Reviewer should-fixes for the claim-check delta work, none changing
behavior on the exercised paths:

- assembleTree now throws tree_name_collision when a name is written
  both as a file and as a directory (a put/base `foo` plus a put or
  delete under `foo/`). The blob-put branch would otherwise win and
  drop the subtree side silently; this covers the plain writeTree and
  writeTreePreservingPrefix paths too, not just the delta guard.
- Document the latent mode downgrade: puts are written as 100644, so a
  put overwriting a non-100644 base entry would lose its mode. No
  caller writes anything else today.
- Fix the stale readProcessingEntry comment that described the old
  whole-working-tree reset; the delta write materializes only the
  touched paths.

Tests:
- Add the last untested claim-check rejection, invalid_inbox_filename,
  via a malformed inbox entry seeded through a permissive handler.
- Adopt the reviewer's independent-oracle delta test: it compares
  writeTreeDelta's committed tree against isomorphic-git's index-based
  git.add + git.commit, an oracle that does not run through
  assembleTree (the shipped write-tree-delta.test.ts full-replace
  oracle does, so a shared bug would pass both sides).
- Adopt the shared-cache repack-transparency test into storage-isogit:
  an object migrating out of a GC-pruned pack still reads under the same
  warm cache, and a genuinely-dropped object surfaces NotFound, not
  stale bytes.

The delta-adversarial test references store.writeTreeDelta, which type-
checks because that method is already on the RepoStore interface.
A no-slash delete naming a directory, or a trailing-slash delete
descending into a base blob, previously dropped the mismatched base
entry silently. Reject both loudly with delete_type_mismatch so a
caller cannot commit a tree that contradicts its stated delete intent.

A put that descends into a base blob is a file-to-directory
replacement, not a delete against the wrong base type; it is left for
the write path rather than rejected here. Today's claim-check callers
never emit any of these shapes, so reachable writes are unchanged.
The claim-check delta reads -- the per-address listing and the bytes of
the entry each leg moves -- ran against the raw repo dir through helpers
the kind had duplicated from the store, so the per-repo object cache
never reached them even though they run under the same write lock as the
store's own reads. Hand computeDelta cache-backed prior-tree closures
(listDirOids and readBlobByOid) and route the reads through them, then
delete the duplicated tree-walk and by-oid blob-read helpers.
Delta-scoped claim-check validation was gated behind the bench-only
BENCH_DELTA_SCOPE_CLAIMCHECK flag, with an exhaustive legacy path
beside it. Make the delta path unconditional and delete the flag, its
OFF path, the sidecar startup marker, and the test env forwarding.

The consumed dedup index is now always validated by its per-commit
delta: retained entries are proven immutable by git blob OID equality
without a byte re-read, added entries are parsed at the transition
check, and a prune is bound to the watermark. Exactly-once rests on
four pillars -- consumed immutability, the below-watermark prune
bound, watermark monotonicity, and enqueue's dedup (index hit above
the watermark, stale-reject below it) against the operator retention
horizon.

The suffix relation the legacy path also enforced is deliberately not
kept: it is structural hardening, not correctness, and computing it
costs the O(retained) prior read the delta path exists to avoid. On
the receivePack path the substrate supplies no prospective OID
listing, so consumed OIDs are hashed from bytes there rather than read
from the tree listing -- correct and the same order as the legacy
walk, with no delta speed-up.
Capture how to run the two off-by-default latency benches (gate and D2
per-leg attribution) and how to read their result JSON, so the measure
loop is reproducible from the repo rather than from throwaway scripts.
The WAL mirror sliced its per-boundary new turns out of a full
baseStorage.load() every message -- an O(N) re-read and re-parse of the
whole conversation just to drop the already-mirrored prefix, the one
remaining per-message growth term on the reply path.

The isogit store now retains the array it was last handed by writeTurns
-- the reactor's live turns array, by reference, not a copy -- and
exposes it through a DurableMirrorReads capability (peekTurns). The
mirror slices the new turns from that array and reads only the bounded
metadata.json via a new metadata-only loadMetadata, so it never reparses
turns.jsonl. This is safe because the local store is single-writer and
in-process: nothing else writes turns.jsonl, so the last-written array
equals the on-disk state at the mirror boundary, and
restoreFromSubstrate seeds it through the same writeTurns. load now
composes loadMetadata, so its behaviour is unchanged.
The in-memory transport's federation inbound path appends to INBOX
unconditionally, so delivering the same Message-ID twice yields two
INBOX messages -- there is no inbox-level dedup. Record that as a test
so the baseline is explicit: it is the single-agent mail contract that
the supervisor FIFO plus markConsumed dedup upgrades to exactly-once.
launchSession and deployWorkflowDefinition built the workflow-deploy
orchestrator with the same two callbacks -- the launch-session bridge
over executeLaunchPhases and the multi-step bridge over
sendMultiStepDeployFrame -- duplicated verbatim, differing only in the
workflow-repo writer, the director registry, and the deploy args.

Factor that construction into a shared runWorkflowDeploy helper the two
call sites pass those three inputs to. No behavior change: launchSession
still uses the no-op repo writer and its trivial bindings, and
deployWorkflowDefinition still uses the hub writer and the multi-step
args, so each still takes the branch it did before.
A one-step workflow has no distinct steps: its lone step is the
deployment head. The general path provisioned each step and then
spawned the supervised child at the head, so a one-step deploy
provisioned twice -- a redundant agent at the step address plus the
child at the head -- and staged the child's deploy tree at the step
address, where the child never looked. Production single agents
avoided this by taking the trivial branch, so the general single-step
path was unused and broken.

Route a one-step deploy through a dedicated head hand-off.
resolveStepAddress is the single owner of the head/step collapse: one
step resolves to the deployment address, more than one to the per-step
address. The host threads the deployment's step count into the child's
spawn env so the hub push and the child's deploy-tree read derive the
same address for the lone step.

The orchestrator's single-step branch hands the whole deploy to
deploySingleStepAtHead, which stages the head's deploy tree and fires
the workflow frame in one call. The sidecar initializes the head's
deploy-tree repo and records the hub key before the pack arrives, so
the follow-up pack applies and its hub-signed commit verifies. The
per-step launch loop is skipped for one step; the multi-step path is
untouched.

An unregistered inference provider is no longer rejected at deploy
time for a one-step workflow, because the provisioning that ran that
check is skipped. The provider still cannot be substituted: the
child's resolution is exact-match and admits no adapter for an unknown
provider, so the deploy is admitted and the run fails instead. The
test asserting deploy-time rejection is skipped pending a decision on
a deploy-boundary source gate.
The single-step head deploy skips the hub provisioning that ran
canBuildSource, so a workflow whose step pins an inference provider the
sidecar cannot build was admitted and failed only when the step's
inference first resolved, at run time. The no-conjure invariant held --
the child's exact-match resolution admits no substitute adapter -- but
deploy-time admission control was lost, and a misconfigured source
produced a false "deploy succeeded" signal.

Add a source-admission gate at the sidecar deploy router, where the
buildable-provider set (the adapter registry) is known; the hub is a
different process and cannot own this check. The gate validates every
step's pinned source before the workflow-process child is spawned,
reusing the harness builder's canBuildSource predicate against the one
registry. A rejected provider throws back through the deploy frame, so
the deploy rejects synchronously at deploy time. It covers single- and
multi-step, and every step's source, not just the default.

This is distinct from the orchestrator's operator-approval check: that
gates on whether the operator approved a provider:model pair; this
gates on whether the provider is buildable at all. A source can be
approved yet unbuildable.
`deployMultiStep` did everything inline in one `try/finally`: claimed
the deployment slug, materialized the workflow definition and grants on
disk, constructed the supervisor, did the single-step key/repo/hub-key
registration, spawned the child, registered the live deployment, and
unwound partial state on failure.

Split it into two owners. `deployMultiStep` claims the deployment slug,
materializes the deploy-only durable state -- `workflow.json` (via a new
`materializeWorkflowJson` helper) and the step grants -- and hands a
`WorkflowDeploySpec` to `spawnWorkflowDeployment`, releasing the slug if
any of that throws. `spawnWorkflowDeployment` is the single owner of
the spawn sequence: supervisor construction, the single-step
key/repo/hub-key registration, the spawn, the live mail/signal/drain and
address registrations, and the unwind of exactly those. It derives every
per-deployment value from the spec rather than from a deploy frame, so a
future caller can drive the same spawn from a source other than a live
frame.

The slug claim stays ahead of every durable write, preserving the
router's guarantee that a colliding deploymentId is rejected before any
repo state is touched. `writeStepGrants` runs before supervisor
construction -- it needs only the repo store and step strategy -- so the
materialize phase stays contiguous. No behavior change.
A workflow deployment holds three inputs only in the frame and in
memory: its pinned per-step sources, its session id, and (single-step)
its head hub key. Nothing on the sidecar's disk carries them, and there
is no enumerable record of which deployments are active -- so a
deployment cannot be re-established after a sidecar process restart.

Write a `deployment.json` beside each deployment's workflow-run
substrate at `workflow-runs/<deploymentId>/`, carrying those three
inputs plus the head address and the definition id. The definition
itself stays in
`workflow.json`, referenced by id, and the grants stay in the step
repos, so neither is duplicated.

The record is written after the slug is claimed and before the child is
spawned, so a crash mid-spawn leaves a record a later boot can re-drive.
A soft-failed deploy deletes it in the failure unwind, and undeploy
deletes it at teardown, so only a live or crash-interrupted deployment
ever has one. The set of active workflow deployments is now a durable,
inspectable on-disk fact rather than in-memory-only state.
Workflow deployments run in a supervised child process the sidecar
spawns, tracked only in memory. A process restart lost them: nothing
re-spawned the child or re-registered the head's mailbox, so a
deployed agent went silent until it was redeployed. The legacy
single-agent path already survived a restart by scanning its on-disk
config; workflow deployments had no equivalent.

Drive a restore pass at boot, before the hub connection opens, so a
single-step head's transport registration is live before the hub can
route to it. The pass scans the persisted per-deployment records and,
for each, re-reads the stored definition, re-validates it through the
same wire and structural gates a fresh deploy frame clears, re-runs
the source-admission gate, and routes through the shared spawn core
the live deploy path uses. The core gains a guard that refuses to
spawn a second supervisor for an address already live, so a restore
pass and the legacy restore cannot both stand up one address.

Restore is soft-fail per record: an unbuildable provider, a corrupt
definition, or a spawn failure is logged and the record is left on
disk for a later boot to retry -- it is never deleted, unlike the
deploy path's cleanup of a record it just wrote. A record whose
address no longer derives its own directory name is skipped.

The mechanism is step-count agnostic: it rebuilds the deployment spec
from the record and routes through the same core the live deploy path
uses, so a genuine multi-step deployment restores exactly the way a
single-step one does.
The SIGTERM-then-SIGKILL child termination the recycle path uses
(killChildHandle) and its resolve-only deadline (waitDeadline, plus
the default timer functions) are the exact machinery the spawn path
needs to bound its ready handshake and force down a child that never
signals ready. Both were module-private to the recycle module.

Move them into a shared supervisor-internal module so the spawn path
can reuse one implementation rather than growing a second copy.
killChildHandle takes an explicit dependency object -- optional
injectable timers plus the caller's logger -- instead of the recycle
context, so its SIGKILL-escalation warning is attributed to whichever
path initiated the kill. The recycle path passes its own logger and
timers. The one observable change is that warning's prefix, now
"child termination" rather than "recycle" to match the shared home.

DEFAULT_KILL_TIMEOUT_MS moves with the helpers and is re-exported from
the supervisor barrel at its existing name, so the public surface is
identical.
The supervisor's spawn awaited the child's `ready` handshake with no
bound: a child that spawned but neither signalled ready nor exited
blocked spawn forever. The sidecar awaits restore spawns serially at
boot before it connects to the hub, so one wedged child took the whole
sidecar dark on a restart, silently.

Race the ready wait against a deadline. On expiry, kill the child --
SIGTERM then SIGKILL, since a wedged child may ignore SIGTERM, and
SIGKILL guarantees its exit settles -- and reject the spawn. The
ready promise's three outcomes (ready, child-exit failure, timeout)
are folded into values so the race never rejects and the single
deadline-timer clear runs on every path before we act on the result;
a race that could reject would skip the clear on the child-exit path
and leak an armed deadline that keeps the event loop alive.

Callers need no change. A live deploy surfaces the rejection as a
deploy failure; boot-time restore logs it, keeps the record, and
moves to the next deployment -- a wedged child no longer hangs either.

The timeout is operator config: `CHILD_READY_TIMEOUT_MS` resolved at
the sidecar boot edge and threaded to every supervisor, defaulting to
30s. The kill escalation reuses the existing 5s kill-timeout default.
The hub routes mail purely on an in-memory address index. A workflow
deployment address enters that index only when the deployment is first
sent to the sidecar; on a WS close the hub removes it, and nothing ever
re-adds it. Single-agent sessions come back through the reconnect
frame's challenge, but workflow deployments are on a separate,
sidecar-local restore path the hub never hears about. So after a WS
reconnect or a sidecar restart the hub silently drops mail to a live
workflow deployment -- it lands in the undelivered queue until the
deployment is redeployed.

Have the sidecar announce the workflow-substrate addresses it currently
hosts on every connect, and have the hub re-register them for routing.
These addresses are hub-minted and carry no per-address key, so they
re-register the same unchallenged way they first entered the index at
deploy time -- the challenge is for session addresses, which have keys.
The sidecar reads them live from its active-supervisor set (so a deploy
arriving during the restore window is included) and carries them in a
new frame field on both the register and reconnect frames; the hub
tracks them in a per-connection set so a close removes exactly this
connection's routes, and a later connection reclaiming an address is
not clobbered by the prior owner's close.

Keep that set physically distinct from the challenged session set --
they differ on the challenge/re-add path -- but give the connection's
ownership readers a unified view. Pack-transfer authorization, in-flight
cancellation, and disconnect teardown ask "does this connection own the
address," which must be true for a reconnected workflow deployment too;
otherwise its workflow-run pack pushes -- which keep the hub's read-only
run-observation mirror current -- would be rejected as unrouted after a
reconnect while its mail resumed. The disconnect event now carries every
owned address, so its field is renamed to match.

This is a pre-existing gap, independent of the disk-restore feature: it
bites even on a bare reconnect with the sidecar process still alive.
The deploy ack returned the supervisor principal key for every
workflow deployment. That is right for a genuine multi-step
deployment, whose head is workflow-derived and has no agent identity,
but wrong for a single-step head: that head IS an agent identity. It
signs its own outbound mail and its reconnect challenges with the
agent key, and the hub records the ack's key and later verifies the
challenge signature against it. Acking the supervisor key there would
make a reconnect challenge verify a supervisor-key signature that was
never produced.

The single-step branch already loads the agent keypair to register the
head's outbound crypto; surface its public key as the ack instead. A
multi-step deployment still acks the supervisor key its workflow-run
events are signed with. This is inert for the single-step workflow
deployments that exist today -- their head is workflow-derived, so the
hub never records or verifies the acked key -- but it is the
prerequisite for routing single-agent instance deploys through this
path, where the head is a real instance whose reconnect must verify.
Deploying an agent instance took the legacy trivial in-process path:
the sidecar built the harness in its own process. Route it through the
deploy core instead, so an instance runs as a supervised
workflow-process child on the same substrate as multi-step workflows --
the last thing keeping the trivial path alive.

A new SessionService entry point wraps the instance's harness as a
one-step workflow and deploys it at the head with the instance's real
identity, then the production instance route calls it in place of
launchSession. The real agentId is preserved (not collapsed to a
derived deployment id) so the spawned child resolves the instance's
skills and pinned tool packages; the head's ack carries the agent key,
so reconnect verifies. No workflow_deployment row is written -- a plain
instance has no workflow asset -- and the single step's inference source
is pinned to the instance's default source, which the route already
resolved and authorized.

The child never runs the wrap's walk-only tool factories: it
materializes tools from the deploy tree and calls the real agent builder
reading only id, prompt, and capabilities. A tool-less instance gets an
empty materialization slot and never hits the bare fallback that would
touch those factories. An integration test proves it end to end -- a
real sidecar spawns a real child that instantiates and runs the wrapped
agent to a real inference reply -- the path unit tests leave mocked.

launchSession is now unused in production and remains only as dead code
alongside the rest of the trivial in-process machinery.
A single agent instance and every workflow step now run inside a
supervised workflow-process child, but the deploy wire format pinned
exactly one inference source per step. The child's reactor does
runtime cross-provider failover by walking an ordered source list
forward from its default, so pinning a single source silently
disabled failover that the legacy in-process harness had.

Carry each step's full ordered source chain end to end. The per-step
sources map becomes a non-empty list keyed by step id across the wire
frame, the supervisor deploy frame, the substrate-config env the
child parses, and the on-disk restore record. The child builds the
step env from the whole chain and pins the reactor's initial source
to its head, so forward-only failover walks the tail.

The single-agent-at-head path pins the instance's whole resolved
source list rather than only its default. It asserts the list is
non-empty and its head is the default source, because the reactor
resolves the initial source by id and fails over forward with no
wrap; a default that is not the head would leave part of the chain
unreachable or disable failover entirely, so the deploy fails loudly
instead.

The source-admission gate and projection validator both walk every
source in every step's chain, and the validator rejects an empty
chain, so an unbuildable failover target or a missing initial source
is caught at deploy and restore rather than deep in a running child.
Unit coverage shows the per-step source chain is threaded to the
workflow-process child, but not that the child's reactor honors it at
runtime across the process boundary. Add an end-to-end test that
deploys a single-agent instance with a two-element source chain whose
head is a dead provider returning HTTP 500 and whose tail is the
healthy mock inference server.

The child exhausts its mechanical retries against the head, fails over
forward to the tail, and returns the tail's live reply. Failover is
proven by the reply equalling the healthy source's output: the dead
head only ever returns 500s and cannot produce that string. Run
completion alone proves nothing, because a total inference failure
also completes the run with a synthesized provider-error reply; a
sibling negative-control test pins that contract so the reply-equality
proof is grounded rather than assumed.
The agent launcher built a throwaway single-step workflow, synthesized
an approval set for it, and ran it through the workflow-deploy
orchestrator -- which, for that wrap, did nothing but round-trip back
into the same deploy-tree, pack, and session-start phases. None of the
wrap reached the wire: the repo writer wrote nothing, the approval set
gated an agent that always passed, and the orchestrator result was
discarded.

Collapse the launcher to call the provision phases directly, and have
the multi-step branch's per-step launch callback share that one method
rather than duplicating the same phase call. The approval-set builder
and the no-op workflow repo writer that only the wrap used are removed.
The launch behavior is unchanged, which the deploy integration suite
and the launcher's own unit tests confirm.

The wrap used to narrow the orchestrator's deploy content through the
manifest-validating bridge before writing it. Export that bridge so the
integration fixtures forward orchestrator-shaped deploy content the same
validated way rather than casting `unknown`, keeping the validation the
collapse would otherwise drop.
The sidecar no longer persists a per-agent `agent.json` HarnessConfig
and self-restores an in-process harness from it. A deployment now runs
as a supervised workflow-process child, and its restart record is a
per-deployment `deployment.json` under `workflow-runs/<deploymentId>/`
that the boot scan re-drives through the same spawn path a live deploy
uses.

Rewrite the deploy/undeploy, directory-layout, self-restoration, and
security-model sections to describe the deployment-record model. The
per-agent key custody, challenge/response, and authority model are
unchanged and left as-is.
The deployment record is the sole restore source for a deployment's
inference sources and hub key, and a source rotation overwrites it in
place. A bare truncating write could leave a torn record on a crash
mid-rotation, and the boot scan then skips the deployment entirely --
losing it rather than restoring a stale source list. Route the write
through a temp-file-plus-rename helper that fsyncs before the rename, so
a reader only ever observes a complete record across both process death
and power loss.
The recycle path awaited the respawned child's ready handshake with no
deadline, so a child that spawned but never emitted ready parked the
supervisor in the recycling phase forever with no automatic recovery --
inbound mail queued against a dispatch loop that never started, and only
an external undeploy could free it. The spawn path already bounds the
same handshake; mirror it here: race the ready outcome against a
resolve-only deadline and, on timeout or a handshake failure, reap the
new child and throw so the supervisor's recycle-failure teardown lands a
clean stopped state the operator can see and redeploy.

The new child is reaped in the recycle path itself because it is never
installed on supervisor state before the throw, so the failure teardown
(which reaps the prior cohort) would otherwise leak it. The
credentials-snapshot read sits outside this deadline by design; a wedged
substrate is bounded at its own layer, not by this handshake timer.
The single-step source-rotation handler persisted the deployment record
before committing the new sources to the in-memory respawn hint. A
recycle that interleaved the persist await read the stale hint through
dynamicSpawnEnv and stood the child up on the prior sources, while the
durable record had already moved on -- the running child contradicted
durable intent, and a restart would "correct" it to the sources the
child should already have had.

Swap the in-memory hint synchronously before the persist so an
interleaving recycle respawns on the same sources being persisted. The
durable write still precedes the live deliverSources swap, and the hint
rolls back on a failed persist so the hint and the record stay in
agreement in the common failure case. The only residual disagreement is
a child briefly ahead of durable truth on a failed persist that a
recycle interleaved, which the next recycle heals -- the benign
direction.

The record writer is injected so a test can block or fail the persist at
the interleave point.
The child run tests injected a substrate whose write path discarded
the staged tree, so the run event log the runtime committed never
landed on disk and the workflow-run repo-store adapter read it back
as an empty log. A completed run then surfaced a terminalStatus with
no matching terminal event in its result events. The runtime, the
adapter, and the emitter all tolerated the empty read, so the
infidelity stayed invisible.

Persist the events the way the real substrate does: run the merge
callback against the prior entries under the preserved prefix, then
write the merged subtree under the repo dir so the adapter's disk
read round-trips. A completed run's emitted terminal frame now
carries the real committed seq, which the warm-path test asserts is
non-zero so a regression to a discarding substrate is caught.
The child's terminal-event emitter coerced a missing terminal event
to seq 0 and an absent RunFailed message to an empty string, and
derived the frame kind from the run's terminal status rather than the
committed event. A run whose committed log lacked a terminal event, or
carried one whose kind disagreed with the status, emitted a frame that
did not mirror any on-disk audit entry -- desyncing the supervisor
from the durable log that resume reads, so it would settle a run the
log still shows in-flight.

Source every field from the committed terminal event, and throw when
it is absent or its kind disagrees with the terminal status. No frame
keeps the supervisor and the durable log agreeing the run is
unsettled, so the next recycle or restart resumes it, and a genuine
producer bug surfaces loudly through the caller's completion
continuation rather than as silent audit corruption. This mirrors the
supervisor's own terminal-event synthesis.
The reconnect ownership challenge looks a deployment's public key up
by address, but workflow-deployment addresses had nowhere on the hub
to hold that key -- only launched agents' agent_instance rows carried
one. Add an address column and a nullable public_key column to the
workflow_deployment projection row, with a unique index on address so
a double-insert fails loud. The deploy path records the address at
insert; the public key stays null until the sidecar's deploy-ack
persists the minted deployment key.

No behavior change yet: nothing reads the new columns. This is the
storage the deploy-ack persistence and the reconnect challenge build on.
Every deployment already mints an Ed25519 key for its address and signs
reconnect challenges with it, but only single-step launched agents
acked that key; a multi-step deployment acked the supervisor principal
key, which the hub discarded and which does not match what the key
store signs a challenge with. So a workflow-deployment address had no
key the hub could verify a reconnect challenge against.

Ack the deployment address's own key for every deployment, and persist
it where the reconnect challenge reads it: a launched agent on its
agent_instance row, a workflow-derived deployment on its
workflow_deployment row (the deploy-ack handler no longer no-ops for
those addresses). lookupPublicKey now routes by address space --
workflow-derived to the deployment row, launched to the instance row --
rather than one table, and filters the deployment row to a live
deployment so a torn-down one's key cannot satisfy a challenge. A null
or absent key returns null, which fails the challenge closed.

No behavior change yet: nothing sends a reconnect frame, so no
challenge runs. This is the key storage and lookup the reconnect flip
builds on.
Workflow-substrate deployment addresses (ins_dep_...) re-registered
through a keyless `workflowAddresses` frame field that routed them
without any ownership proof. A sidecar holding a valid token could
name another deployment's address on reconnect and capture its mail.
Launched-agent addresses never had this gap: they prove ownership via
Ed25519 challenge/response on reconnect.

Close it by routing deployment addresses through that same challenged
path:

- Remove the `workflowAddresses` field from the register and reconnect
  frames. The sidecar announces deployments in the reconnect frame's
  `agentAddresses`, and connect() sends an empty first-connect register
  followed by the challenged reconnect.
- The hub challenges every reconnect address uniformly and routes it
  only after a valid signature. A workflow-derived address is tracked
  on the connection's workflow set and skips the agent_instance-only
  reconnect reaction, but enters the routing table only post-challenge.
- lookupPublicKey resolves a deployment key from workflow_deployment,
  gated on a live `deployed` status, and fails closed on a missing key,
  mirroring the launched-agent path.
- When a new connection's verified reconnect reclaims a workflow
  address, evict it from the superseded connection's owned set, so that
  connection's later close does not cancel the new owner's in-flight
  pack transfer.

Rewrite the tests that encoded the keyless model to assert the
ownership invariant: challenged happy-path, wrong-key rejection,
fail-closed on a missing key, close eviction, reclaim-only-via-its-own-
challenge, and superseded-reclaim not cancelling the new owner's
transfer. Wire the challenge round-trip into the hub-link integration
harness and replace the invalid deployment fixtures (dep_..., and
hyphenated ins_dep-...) with real ins_dep_ addresses; those fixtures
were rejected by parseAgentAddress and never exercised the
workflow-derived branch at all.

The register frame's routing path is not modified here.
`handleRegister` routed every address a register frame named on the
sidecar token alone, with no per-address ownership proof -- the same
token-only routing the reconnect challenge exists to prevent, reachable
through the sibling register frame. A token-holding sidecar could name
another deployment's (or agent's) keyed address in a register frame and
capture its future mail; the ghost-cleanup even evicted the true owner
first, so a refused-but-partially-applied claim could also strand the
victim.

Gate routing on key existence: a register frame routes an address only
when `lookupPublicKey` returns null for it -- a genuine keyless
first-deploy, matching the documented token-bounded first-deploy trust
model. An address that already has a stored key is refused and must
prove ownership through the challenged reconnect path.

The keyless-only set is computed up front, before the ghost-cleanup and
every routing mutation, so a refused (keyed) address touches nothing:
no eviction of a live owner, hence no downgrade from hijack to a denial
of service on the victim. When the key lookup is not configured, or the
lookup throws (a transient DB failure), the handler fails closed --
routes nothing and logs an error -- rather than routing unverified or
letting the rejection float out of the void-dispatched handler and
crash the hub. `handleRegister` becomes async to await the lookup,
mirroring `handleReconnect`.
The reconnect challenge looks up each claimed address's public key with
`await lookupKey(addr)` inside a `Promise.all`, unguarded, and the
handler is dispatched as `void handleReconnect(...)`. A rejecting lookup
-- a transient database failure on the production reconnect path, which
every honest sidecar exercises -- floats out as an unhandled promise
rejection that can terminate the hub.

Catch the lookup error per address and fail closed: treat the address as
unverifiable (null key), so it fails its challenge and stays unrouted,
and log the failure. The hub keeps running and the sidecar retries on
its next reconnect. This mirrors the register key-existence gate, which
fails closed on the same error.
`handleRegister` cleared the connection's entire owned set on every
re-register and rebuilt it from the frame's addresses. Since the gate
routes only keyless first-deploys, a re-register on a live connection
dropped every keyed route the connection had proved via challenged
reconnect -- the connection self-evicted its own routes until its next
reconnect. A sidecar that first-deploys agent-a and later first-deploys
agent-c would also lose agent-a's route to the second register.

Make re-register additive: the connection inherits every address it
already owns and ADDS the frame's keyless first-deploys. Register never
drops an owned route; removal happens via undeploy or disconnect, not
register-omission. The cross-connection ghost-cleanup still runs for the
newly-claimed addresses (a first-deploy claimed from another ws still
evicts that ws), but a connection's own inherited routes are left alone.

This is the correct contract under the challenged model: a register
frame no longer carries the sidecar's complete live set (keyed addresses
arrive via reconnect), so an omitted address no longer means "removed."
Gating register routing on an async key lookup means a frame that
depends on the just-registered route -- connector.state.changed, mail,
a pack push -- can arrive during the lookup window and be dropped,
because addressIndex is not yet populated when its handler runs.

Serialize each connection's frame dispatch: a frame that establishes or
reads routing waits on a per-ws promise chain for earlier such frames to
finish, so it observes their completed effects. Frames that are terminal
responses to an already-issued outbound request (session.ack/error,
agent.deploy.ack, agent.error, agent.undeploy.ack, repo.pack.ack/reject)
plus liveness (ping) BYPASS the chain: they resolve the very promises an
in-flight queued handler blocks on, so queuing them would deadlock the
challenge round-trip (challenge.response's agent.reconnected reaction
runs sendSourcesUpdate, which awaits a later session.ack). A
frameBypassesQueue switch classifies every frame variant, with an
assertNever default, so a new frame type is a compile error rather than
a latent deadlock or a silent bypass hole.

The bypass set is safe on two counts: every frame that can resolve a
pending request is in it (no queued handler can wedge on a queued
response), and no bypass handler reads addressIndex or connectorStates
(so the poisoning-vector connector.state.changed stays queued and cannot
be poisoned out of band).

The deadlock guard is mutation-checked: routing session.ack through the
queue instead of bypass wedges the reconnect, so the bypass
classification is load-bearing, not a latency nicety. The
routing-mechanics unit tests that asserted synchronously now settle the
now-async dispatch first.
The boot-time reconnect re-announce reads each deployment's deploy ref
before sending the challenged reconnect frame. That read ran inside an
unguarded async IIFE in the connection open handler, outside the
message-queue tail catch, so a rejection (corrupt or unreadable ref
state) became an unhandled promise: the reconnect frame was never sent,
the sidecar's deployment routes silently vanished, and nothing was
logged.

Wrap the re-announce in a catch that logs the failure and closes the
socket, forcing a clean reconnect retry on the existing 3s backoff. The
send sits after the ref-read loop, so a throw skips it and no partial
reconnect frame goes out.

The test drives a rejecting deploy-ref read and asserts the socket is
closed (a reconnect is scheduled) rather than the failure swallowed; it
fails both when the catch is absent and when the catch logs without
closing.
The recycle respawn spawns the new workflow-process child and wires its
IPC channels, then re-reads per-step credentials before the ready
handshake. That read is a substrate call that can reject -- a grants
file that became malformed is precisely the grant-refresh path recycle
doubles as. It ran with no guard around the new handle, so on a
rejection triggerRecycle unwound and the supervisor's recycle-failure
teardown reaped only the PRIOR cohort (state.handle during recycling).
The freshly-spawned child leaked its OS process and both IPC channels.

Wrap the credentials read in a catch that reaps the new child (kill,
then finalize the pumps) and rethrows, mirroring the spawn path, which
already routes this same throw through its teardown owner. The reap that
the handshake-failure path open-coded is extracted into reapUnreadyChild
so both failure points share one kill-first sequence.
The spawn-failure catch tore the cohort down with an unguarded
`await shutdownInternal(...)` before rethrowing the spawn cause. That
teardown walks several individually-unguarded steps (mail unsubscribe,
child kill, broadcaster dispose, accumulator stop); a throw in any of
them propagated out of the catch and replaced the original spawn cause,
hiding the real startup failure behind a secondary teardown error.

Guard the teardown with a `.catch` that logs the secondary failure and
lets the original `cause` rethrow, mirroring the recycle-failure catch
that already preserves its cause this way.

The test injects a throwing mail-unsubscribe so a ready-timeout spawn's
teardown fails, and asserts the surfaced error is the ready timeout, not
the teardown error.
The source-rotation persist rolls back `currentSources` on a failed
write. Its comment credited that safety to the hub awaiting each
sources.update ack before sending the next -- but the hub does no such
pacing; it dispatches rotations fire-and-forget. The real serializer is
the sidecar's per-connection inbound-frame queue: each hub frame's
handler runs to completion before the next begins, so no second rotation
is ever in flight to have its committed table clobbered by the rollback.

Rewrite the rollback comment to name the true mechanism, and mark the
frame queue itself so its ordering reads as load-bearing. A future
change that parallelized inbound-frame dispatch would break the
rollback; the corrected comments make that dependency explicit at both
ends. No behavior changes.
The public_key column comment justified its nullability partly by a
"pre-migration" deployment reading null. No such row can exist: the
sibling address column was added NOT NULL with no default, which
Postgres rejects on a non-empty table, and the repo's migration
convention (matching the credential.provider_id add) is that the table
is empty when the column lands. The two positions contradicted each
other.

Drop the impossible pre-migration case from the key comment -- the
nullable window is only the real not-yet-acked state between
deploy-start and deploy-ack -- and document on the address column why
the NOT NULL add needs no default or backfill. Comment-only.
The claim-check API header still described the four inbox/processing/
consumed operations as routing through `writeTreePreservingPrefix` with
the per-address subtree as `preservePrefix` and the substrate replacing
that subtree wholesale via `clearPrefix`. The operations were converted
to `writeTreeDelta`: each scopes the change to the per-address prefix
and returns a targeted set of `puts`/`deletes` from a `computeDelta`
callback that reads the prior tree via `listDirOids`/`readBlobByOid`,
and the substrate applies that delta over the prior tree rather than
rewriting the subtree.

Rewrite the header to describe the delta mechanism the code now uses.
Comment-only.
The design doc's symbol-fate section claimed two changes that never
happened. It said `wrapHarnessAsSingleStepWorkflow` was renamed to
`wrapHarnessAsSingleStepAgent` (a symbol that does not exist; the
original name shipped) and that `deriveDeploymentId` was deleted (it is
alive and called at four workflow-host wiring sites). The same
non-existent `wrapHarnessAsSingleStepAgent` also appeared earlier, in
the workflow-level defaulting section.

Correct all three references to match the shipped symbols. Doc-only.
Retiring the in-process (single-agent) harness deleted its build path
and event forwarder, leaving `createDefaultHarnessBuilder` as a
source-admission check only. Comments across three packages still
asserted present-tense wiring to that removed machinery: that
`default-harness.ts` reaches the tool-materialization module, that the
per-step tool caps are the same ones "the in-process harness builder"
uses, and that the step-invoker event filter mirrors a forwarder in
`default-harness.ts` that no longer exists.

Reword each to name the current single consumer (the workflow-process
child's substrate factory) and the live source of the loader caps (the
sidecar boot edge), and drop the broken forwarder pointers. Two honest
"legacy in-process harness" behavior-lineage notes lose their now-stale
tail too. Comment-only; the possibly-dead `assetRoot` default beside one
of these comments is left for a separate change.
`SourcesUpdateFrame.sources` validated as an unconstrained
`InferenceSource.array()`, but the doc asserted the list is non-empty
with element 0 equal to `defaultSource` -- neither of which the boundary
checked. The sibling deploy frame already tightened its per-step source
arrays to `.atLeastLength(1)`.

Tighten `sources` to `.atLeastLength(1)`, matching that sibling and the
frame's own contract: an empty rotation has no source for the agent to
swap to. This is safe against the live producer -- the hub's
`pushInstanceSourceUpdate` returns early when there is no head source,
so it never emits an empty list. The `defaultSource`-equality claim is
producer-enforced only and cannot be expressed structurally, so the doc
is reworded to say so rather than asserting a check that does not exist.
Four orchestrator deploy tests asserted `result.publicKey` only with
`toBeTruthy()`, which passes on any non-empty value. The sibling
workflow-host-wiring tests already assert the minted key against
`/^[0-9a-f]{64}$/`. Tighten these to the same hex-64 shape so a mangled
or wrong-shaped key is caught, not just an absent one.
The `deploy.apply.error` frame reported a failed tool-package apply
from the sidecar to the hub. Its whole path is dead: the sidecar sender
was already removed, production never wires the `emitDeployApplyError`
callback (the step tool cache is built without it), and the hub-side
event was declared with a permanently-empty subscriber set. So the frame
type, the emitter type, the emit call sites, the hub receive/dispatch
cases, and the empty event registration are all unreachable.

Remove them: the DeployApplyErrorFrame type and its SidecarFrame union
membership, the DeployApplyErrorEmitter type, the emitDeployApplyError
parameter and its three call sites, the two dispatch switch cases, and
the sidecar-events entry and subscriber set. The production behavior
on a failed apply is unchanged -- the failure is still written to the
durable rejected-apply audit and then thrown; only the never-consumed
frame emission is gone.

`DeployApplyErrorCategory` stays: it is the tool-packaging error
taxonomy used by the loader and atomic-apply layers, independent of the
frame. Stale prose describing the removed "frame channel" is updated to
the throw/audit path, and the tests keep their throw-path coverage by
asserting the durable audit instead of the removed emission.
`shutdownInternal` owns the invariant that teardown always kills the
child and always reaches `stopped`, but three steps were unguarded while
every neighbor already logs-and-continues: the recycle-policy stop, the
mail unsubscribe, and the child kill. Because the unsubscribe runs
before the kill, a throwing unsubscribe skipped the kill entirely --
orphaning the child process -- and, having thrown past the phase
assignment, left the supervisor wedged in `stopping`. Both the shutdown
and recycle-failure paths route through here, so both were exposed.

Wrap each of the three steps in its own try/catch that logs at warn and
continues (matching the existing unregisterAddress guard), and move the
`state = { phase: "stopped" }` transition into a `finally` so it runs
regardless. Independent guards so a throw in one step cannot skip a
later one; the kill is always attempted and the supervisor always
reaches `stopped`.

This keeps the function's established best-effort-and-log contract -- it
does not rethrow or aggregate, which would change the `shutdown()` and
recycle callers and risk re-masking the original cause. Errors surface
via `logger.warn`; nothing is silently swallowed. The tests inject a
throwing unsubscribe and a throwing kill and assert the child is still
killed and a second shutdown is an idempotent no-op.
Three comments broke mid-sentence with a dangling word on its own line
(instance deploy-at-head, single-step deploy docstring, repo-store cache
note). Reflow each so the sentence reads continuously. Comment-only.
shutdownInternal guarded its teardown one step at a time, but that
approach left an escape hatch: the pre-try steps (the accumulator-stop
loop, the cohort abort/reject/dispose/wake block) ran outside the
finally, so the documented "always reach stopped" invariant did not
actually hold. The step-by-step guarding also left the spawn-cleanup
regression test vacuous -- with the mail unsubscribe now guarded inside
shutdownInternal, a thrown unsubscribe no longer reaches the spawn-catch
the test meant to exercise.

Stop enumerating throw sources; make the invariant structural. Run the
whole teardown body in one try and move the two load-bearing actions --
the child kill and the phase transition to stopped -- into the finally,
so a throw in any step still runs both. Make the one pre-try step that
can actually throw total at its owner: guard the broadcaster dispose's
per-listener onDispose loop (log at error, continue), covering both its
shutdown and recycle call sites. Guard the drain-accumulator stop loop
per-iteration. The steps that provably cannot throw (cohort abort,
rejectCohortAwaiters, wakeDispatch) get no guards but sit inside the try
so the invariant survives future edits.

The spawn-failure catch's cause guard stays as defense-in-depth for a
distinct invariant -- the spawn cause must survive the unwind -- and its
comment is corrected to say so rather than list now-guarded steps. The
spawn-cleanup test and its comments are repaired to match; the
discriminating coverage of the teardown guards lives in the shutdown
teardown-robustness tests that drive shutdownInternal directly.
The deploy-apply removal's reflow left "the harness tears / down" split
across two lines mid-phrase. Reflow so the sentence reads continuously.
Comment-only.
@alexanderguy

Copy link
Copy Markdown
Member Author

Remediation update: fixes from a full-branch review

This pushes 15 commits (b00e9f7f..HEAD, +748/−520 across 31 files) addressing findings from a full-branch code review. Each behavioral change ships with a mutation-verified regression test; every finding is dispositioned (fixed or verified non-issue).

Error-path fixes (behavioral)

  • Reconnect deploy-ref read no longer silently drops a deployment's routes: a failed read logs and closes the socket to force a clean reconnect (restoring behavior that had regressed).
  • Recycle respawn reaps the freshly-spawned child instead of leaking its process + IPC channels when the pre-handshake credentials read throws.
  • Spawn failure preserves the original cause when teardown also throws.
  • shutdownInternal is structurally total: the child kill and the transition to stopped live in a finally, so the supervisor always kills the child and always reaches stopped regardless of which teardown step throws; the terminal broadcaster's dispose is made total at its owner.
  • sources.update boundary rejects an empty sources list (matching the deploy frame's per-step arrays and the live producer, which never emits empty).

Dead-code removal

  • Removes the fully-dead deploy.apply.error frame + emitter subsystem (−238 lines net). DeployApplyErrorCategory (the live tool-packaging error taxonomy) is retained.

Documentation / comment accuracy

  • Corrects ~a dozen comments and docs to match shipped code: the delta claim-check mechanism (writeTreeDelta vs the old wholesale-rewrite), the in-process-harness removal references across three packages, the migration/schema NOT NULL contradiction, the source-rotation rollback rationale (attributed to the sidecar frame queue, not hub ack-pacing that does not exist), and stale symbol fates in the execution-host design doc.

Verification

  • make all green on HEAD: 3840 main + 480 DB tests, 0 fail.
  • Whole-branch review clean (cross-commit findings fixed); commit-message + binary + scope audits clean.
  • No follow-up issues deferred; no open TODOs.

@alexanderguy alexanderguy marked this pull request as ready for review July 7, 2026 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant