Repository navigation
fix(hub): start a fresh local sidecar on every boot instead of respawning one - #702
Conversation
TheGreatAxios
changed the title
|
Triage: Fail @TheGreatAxios, please address the following:
Notes:
|
![corbits-triage[bot]](https://avatars.githubusercontent.com/u/275831056?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes the hub's local sidecar so each boot retires the previous sidecar and starts a fresh one, rather than respawning it, and replaces a sidecar that is retired, exited, or unreachable while the hub runs. The main defect is that the disconnect grace timer starts late, so a stale sidecar can still accept new work; the replacement path also tears down the working sidecar before checking capacity.
| unreachableSince ??= now(); | ||
| return now() - unreachableSince >= CONNECT_GRACE_MS ? "disconnected" : undefined; |
There was a problem hiding this comment.
P1 · Disconnect grace is measured from the first ensure that sees the gap, not from the disconnect
unreachableSince is only set inside unusable(), which runs on ensure. If the connection dropped well before the next ensure, the two-minute clock starts at that ensure, so a disconnected sidecar is accepted and the work fails on stock's 120s connect timeout.
Trigger: the sidecar's connection drops at T0 and a deployment is placed at T0+10 min. isConnected is false, unreachableSince is set to T0+10 min, and the ensure returns the disconnected sidecar as accepted.
Fix: record the disconnect time when the connection is lost (or when it was last seen connected), not on the first ensure that observes it. For example, track lastConnectedAt on each connect and compare now() - lastConnectedAt against the two-minute threshold in unusable().
| async function isRetired(sidecarId: string): Promise<boolean> { | ||
| const row = await db.query.sidecar.findFirst({ columns: { retiredAt: true }, where: eq(schema.sidecar.id, sidecarId) }); | ||
| return row === undefined || row.retiredAt !== null; | ||
| } |
There was a problem hiding this comment.
P2 · Missing sidecar row treated as retired can replace a freshly started sidecar
isRetired returns true when no sidecar row exists, and unusable checks it on every ensure. If stock only creates the row after the sidecar connects, an ensure right after spawn sees the new sidecar as retired, stops it, and starts another. This can repeat.
Trigger: two ensures in quick succession after a fresh spawn, before the sidecar has connected and its row exists.
Fix: treat a missing row as "not yet registered" for a newly spawned sidecar rather than as retired. Only mark a sidecar retired when stock has explicitly retired its identity, or when the row exists with a retired status.
| if (reason === undefined) return { kind: "running", manifest, process: running, minted: false }; | ||
| try { | ||
| await replace(manifest, running, reason); | ||
| } catch (error) { | ||
| return { kind: "rejected", code: "sidecar_replace_failed", message: errorText(error), retryable: true }; | ||
| } | ||
| } | ||
| const full = shortfall(await capacity.freeBytes()); | ||
| if (full !== undefined) { | ||
| return { kind: "rejected", code: CAPACITY_UNAVAILABLE, message: full, retryable: true }; | ||
| } | ||
| request.signal?.throwIfAborted(); | ||
| const minted = manifest === null; | ||
| const current = manifest ?? { sidecarId: request.sidecarId, token: request.token, hubWebSocketUrl: request.hubWebSocketUrl, dataDir }; | ||
| const current = { sidecarId: request.sidecarId, token: request.token, hubWebSocketUrl: request.hubWebSocketUrl, dataDir }; |
There was a problem hiding this comment.
P2 · Replacement runs before the capacity check, so a full host retires the working sidecar and starts nothing
sidecarFor calls replace() (stop, sidecarStopped with endReconnectWindow, clear holds) before it checks capacity.freeBytes(). If the capacity check then fails, the old sidecar and its allocations have already been retired, and no replacement starts. Stock must re-place every allocation onto a host that is short on memory.
Trigger: a sidecar is flagged unusable (retired, or disconnected for two minutes), and free disk is below 1 GiB during the same ensure. The ensure returns capacity_unavailable after the old sidecar has been torn down.
Fix: run the capacity check first and return capacity_unavailable before calling replace(). Only retire the old sidecar once a replacement is known to be possible.
if (reason === undefined) return { kind: "running", manifest, process: running, minted: false }; should move the capacity check ahead of the replace() call on the unusable path.
f649310 to
3bab08f
Compare
The local provisioner goes back to stock's model, as its own harness runs it: ensure starts a sidecar process for the allocation and destroy stops it and removes its files, so cleanup is always confirmed and no process keeps the memory of deployments it no longer hosts. Each sidecar advertises one active deployment. A late ensure for a destroyed identity is fenced in memory, as the hub mints a fresh identity for every ensure after a restart. Boot never respawns: it retires every sidecar an earlier boot listed, in this layout or the single shared sidecar's, and removes their files. A sidecar that exits on its own is retired at once. The provisioner refuses a start past the configured number of sidecars, or while the box lacks the memory for one beside those still connecting, as retryable capacity. The stock provisioner conformance suite runs against it again.
…CARS TRIAGE_MAX_LOCAL_SIDECARS (default 8) replaces TRIAGE_SIDECAR_MAX_DEPLOYMENTS and bounds the sidecar processes the hub runs on its own machine, one per deployment or probe. Past it, placement sends a deployment to the first matching sandbox provisioner; with none, the local provisioner refuses it as capacity_unavailable and stock retries the placement with backoff. Settings counts the placements waiting under that code.
The shared sidecar scenario now checks that the listener and two managers each run in a sidecar process of their own, each manager receiving only its own mail, and that a released manager's sidecar and files are gone. The cap scenario sets TRIAGE_MAX_LOCAL_SIDECARS to 2 and checks that the second manager is refused for capacity, logged, and runs once the first ends. Settling waits up to 150 seconds, as each deployment now starts a sidecar process and a judged pull request starts two in turn.
ARCHITECTURE, the local provisioner page, SELF_HOST, DEV, the provisioners overview and the user manual's example describe a sidecar process per deployment, cold boot, retirement on exit, TRIAGE_MAX_LOCAL_SIDECARS and what a deployment refused for capacity waits for.
3bab08f to
1bf0abd
Compare
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Runs each local allocation in a sidecar process of its own that destroy stops and removes, retires every earlier sidecar at boot, and caps the processes with TRIAGE_MAX_LOCAL_SIDECARS plus a memory check that counts sidecars not yet connected.
- apps/hub/src/sidecar/local-provisioner.ts:229 (medium). The memory check counts
starting.sizebut notreserved. An allocation joinsstartingonly aftermkdir,mkdtempand the manifest write (line 285). Scenario: two ensures for different allocations, 1.5 GiB free. Both awaitfreeBytes(). A resumes, passes, and adds itself toreserved; B resumes and also passes, because A is not instartingyet. Two sidecars start on memory for one. The cap half ofnoRoomalready countsreserved, and the memory half needs to do the same:shortfall(free, starting.size + reserved.size). - apps/hub/src/server.ts:248 (medium). The memory reservation is never released for a probe on connect. Stock emits
sidecar.allocated.connectedonly forbinding.kind !== "probe"(vendor/interchange/packages/hub-sessions/src/ws/sidecar-handler.ts:2431), so a probe's sidecar stays instartinguntil its process exits. Scenario: with no sandbox registered, probes run locally. Each running probe sidecar is then charged a full 1 GiB in every later memory check on top of its real use, which already shows in the box's figures. The docs promise a probe's sidecar keeps running once a deployment adopts it, so after adoption the double count lasts as long as that deployment. On a machine with a few GiB, starts are refused for memory that is not in use. Release the reservation on the probe's own connect signal, or on adoption. - apps/hub/src/sidecar/local-provisioner.ts:321 (low). Boot retires sidecars in stock and deletes their data directories, but it never stops their processes: the manifest holds no pid, and
Bun.spawnchildren outlive a hub that dies without runningshutdown. In the container the hub is PID 1, so its death takes the children with it. Inbun run dev,--watchrestarts and any self-host under a supervisor, each crash instead leaves up toTRIAGE_MAX_LOCAL_SIDECARSorphaned sidecars holding memory with their data directory removed underneath them. With one process per allocation this is up to 8 processes rather than 1. Unverified whether a retired sidecar exits once its reconnect is refused. Either record the pid in the manifest and signal it at boot, or say in docs/provisioners/local.md that a crash outside a container can leave processes behind. - apps/hub/src/sidecar/local-provisioner.ts:204 (low). In
retireExited, ifsidecarStoppedthrows,removeStorednever runs. The crashed sidecar's manifest and data directory then stay until the next boot. The only trace islocal_sidecar_retire_failed, and nothing retries it. Removing the files does not depend on stock, so it can go first, or in afinally. - apps/hub/src/sidecar/manifest-store.ts:78 (nit). Boot logs and skips an unreadable manifest, then
clear()deletes it. So the sidecar it named is never passed tosidecarStopped, and its allocation waits out stock's reconnect window instead of being placed again at once, as the docs say happens to every sidecar boot finds. This is rare, but a corrupt or partial manifest is exactly the crash leftover boot exists for. The allocation id is in the file name, so the docs could at least admit the exception.
Checked and fine:
- Each spawn gets its token only through its environment, and the manifest holds no token.
- No holds, hosted flag,
WAITING_FOR_SLOT,TRIAGE_SIDECAR_MAX_DEPLOYMENTSorrestore.tscode is left. - The remaining fences are in-memory only and are needed by the conformance suite.
- Ensure and destroy are serialized per allocation, and an exit after a destroy is not treated as a crash.
- Docs,
.env.exampleand the user manual match the new cap. bunx bun@1.4.4 check --no-scriptis clean for apps/hub and tests/chain, and the offline sidecar tests pass (21/21, conformance included). The postgres and chain suites were not run.
…p a failed retirement for boot The memory check now also counts allocations past it whose sidecar has not started yet, so two ensures side by side cannot both start on memory for one. A sidecar counts until the hub holds its connection, read from stock's sidecar inventory, since stock emits no connected event for a probe's sidecar. A sidecar whose retirement fails after it exits keeps its manifest, so the next boot retires it.
|
On review 5678783241:
|
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Counts every sidecar not yet connected toward memory, side-by-side ensures included, releases that count once the hub holds the sidecar's welcomed connection for deployments and probes alike, and keeps a failed retirement's manifest for the next boot.
No findings.
The hub's one shared local sidecar process grew by tens of MB to over 100 MB with every finished run, because it keeps every workflow it ever loaded in memory. With this change, each deployment the hub runs on its own machine gets a sidecar process of its own, which is stopped and removed when the deployment ends. A cap limits how many run at once.
Why the shared sidecar grew
The sidecar process imports each deployment's workflow package from that deployment's own closure directory, through
materializeDeploymentClosureandloadWorkflowDefinitionFromClosurein stock'sworkflow-host-wiring.ts. Each import URL is unique and imported modules are never unloaded, so every finished deployment's module graph stayed in the process. Undeploy removed the files but not the modules.Measurement
These runs used the chain harness: a fake GitHub, a stub model, macOS and Bun 1.4.4. Each run is one pull request triaged to a verdict, five at a time. Samples were taken once every allocation except the listener's was released.
Before, on main (one shared sidecar):
In run A, run 35 never reached a verdict, so that run stopped there. In run B, 3 of the 50 runs got no verdict within 5 minutes.
After, on this branch (one sidecar per deployment, default cap of 8). RSS is summed over every local sidecar process and its workflow processes:
The after measurement was not run on the same machine: 20 runs of per-deployment sidecars need more memory than was free beside other work. The proof that memory is released per run is the provisioner test that a released allocation's process is gone, the chain test
local-sidecars.test.ts, and the hub's memory in production after deploy, which is to be read before and after the first day.What changed
TRIAGE_MAX_LOCAL_SIDECARS(default 8) replacesTRIAGE_SIDECAR_MAX_DEPLOYMENTS. Past it, placement sends a deployment to the first matching sandbox provisioner. With none, the provisioner refuses it as retryablecapacity_unavailable, and so does a start with less than 1 GiB free beside the sidecars still connecting.reconciler.ts:defaultRetryDelay,MAX_ENSURE_ATTEMPTS), and then fails the deployment. The manager never ran, so the pull request gets no comment or label. The head stays queued until the reconciler marks it never started after 10 minutes; its next pass (every 5 minutes) queues it again. Keeping such a head queued until capacity frees is CL-11633.fences.json, the encrypted token in the manifest and the restore path.restore.tsis nowboot.ts.local_sidecar_refusedwith the reason.docs/ARCHITECTURE.md,docs/provisioners/local.md,docs/provisioners/README.md,SELF_HOST.md,DEV.md,.env.exampleand the user manual's example cap.How it was verified
bun run lintandbun checkpass forapps/hub.apps/hub/src/sidecartests pass, includingplacement.postgres.test.ts. They include stock's provisioner conformance suite and provisioner tests for:local-sidecars.test.tsreplacesshared-sidecar.test.tsand checks that the listener and two managers each run in their own sidecar, and that a released manager's sidecar and files are gone.sidecar-cap.test.tschecks that a manager refused for capacity runs once a slot frees. All five chain shards pass in CI on this head.Closes CL-11625