Skip to content

feat(hub): run every Triage deployment in one local sidecar - #678

Merged
TheGreatAxios merged 10 commits into
mainfrom
cl-11587-one-sidecar
Oct 27, 2026
Merged

TheGreatAxios merged 10 commits into
mainfrom
cl-11587-one-sidecar

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Every Triage deployment on the hub's machine now runs in one local sidecar process instead of one sidecar process each. The sidecar still starts a workflow child process for each deployment. The first deployment starts the sidecar and every later one joins it, using Interchange's stock support for placing work on a sidecar a provisioner already runs. The sidecar stays up for the life of the hub. When a deployment ends, the hub removes it from the sidecar.

What changed:

  • Local provisioner. It keeps one process manifest, a holds file with one entry per deployment, and a fences file with each destroyed generation, so a late ensure stays refused after a restart. Each destroy forgets the fences of allocations and probes stock has ended, and an ensure for one stock ended is refused from its record. Ensure and destroy wait until restore has finished, and fail if it fails. An unreadable fences file is moved aside. A destroy answers required whenever the sidecar hosts the allocation's copy, whatever identity it names. An ensure that fails or is abandoned leaves the hold as it was and discards a sidecar minted under its identity. Each allocation the provisioner accepts is recorded as placed, so a manager that joins the sidecar is timed from then.
    • On boot it restarts the sidecar if any held deployment is still allocated, or if the hub still waits for the sidecar to remove one that ended. Otherwise a removed copy would hold a slot for good.
    • The sidecar's exit is logged with its code or signal, since one exit stops every deployment.
    • Memory is checked only when the sidecar starts; after that the cap is the guard.
    • On the first boot after upgrade, each sidecar the old one-per-deployment layout left is reported stopped through stock's sidecarStopped with no reconnect window, so its deployments are placed again at once. Its manifest, token and data directory are then removed. One that cannot be retired keeps its files for the next boot.
  • Cap. TRIAGE_SIDECAR_MAX_DEPLOYMENTS (default 16) sets how many active deployments the sidecar accepts.
  • Overflow. A placement chooser, shared by deployments and probes, counts the local sidecar's active deployments and probes, plus placements waiting there for a slot. Once the cap is reached it sends the next deployment to the first matching sandbox provisioner. With no sandbox, the deployment stays local and waits in Interchange's placement retry until a slot frees.
  • Stuck managers and judges. A manager or judge is deployed together with the mail that starts it. So one that has sat on its sidecar for five minutes without running is stuck.
    • Each reconcile pass ends it, frees its slot and logs triage_deployment_never_ran, and it no longer holds its pull request's claim.
    • The five minutes start when the hub sees it reach its sidecar, so a manager that waited for a slot is not cut short.
  • The listener is excluded from that rule. It stays deployed, without running, until its first pull request arrives, so a quiet workspace's listener would otherwise be ended.
  • Waiting for a slot. A run whose allocation has not reached a sidecar yet (provisioning or replacing, as between retries of a placement refused for room) keeps its claim for up to half an hour from the allocation's creation, whatever its failure code.
  • Settings. The workflow versions route now returns the sidecar's active count (deployments and probes), waiting placements, cap and overflow, and Settings → Triage shows them in one sentence. The counts cover the whole hub.
  • Docs. .env.example, SELF_HOST, ARCHITECTURE, DEV, the provisioner docs and the user manual are updated.

How it was verified:

  • bun run check and bun run lint pass.
  • bun --conditions=intx-src test apps/hub passes (443 tests), and the web tests pass.
  • bun run test:chain passes: 9 tests across 3 files. Two of those files are new.
    • tests/chain/shared-sidecar.test.ts: the listener and two live managers run on one sidecar. Each manager handles only its own pull request's mail. Both are then released with cleanup confirmed, while the sidecar keeps running.
    • tests/chain/sidecar-cap.test.ts: with a cap of 2 and no sandbox, the second manager is refused for room and runs once the first ends.
  • New unit tests cover:
    • restore, including the case where only a removal is still pending;
    • the sidecar's exit log;
    • the provisioner's fences for a destroyed identity and for a conflicting one;
    • placement and overflow counting against Postgres, including probes and waiting placements;
    • fences surviving a restart until stock ends the allocation, and each write dropping ended ones;
    • a corrupt fences file moved aside and a late ensure for an ended allocation refused;
    • a destroy naming a joined allocation's offered identity answering required;
    • ended fences forgotten at the next destroy;
    • a failed ensure leaving the hold as it was and the next ensure minting a fresh identity;
    • each joined allocation reported placed;
    • a probe overflowing to the sandbox when the local sidecar is full;
    • an ensure and a destroy sent before restore answering from the restored sidecar;
    • retiring and removing an old-layout fixture, keeping a sidecar whose retirement failed;
    • the five-minute rule timed from placement, the half-hour slot wait in both of stock's retry states (and its end once the allocation has failed), and the listener being left alone.
  • The Settings sentence was not checked in a browser.

Closes CL-11587

@linear-code

linear-code Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

CL-11587

CL-10721

CL-10719

@TheGreatAxios
TheGreatAxios force-pushed the cl-11587-one-sidecar branch 2 times, most recently from 9086fce to 5c5fd7c Compare October 27, 2026 06:57
@TheGreatAxios
TheGreatAxios marked this pull request as ready for review October 27, 2026 06:57
@corbits-triage

corbits-triage Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Triage: Unsure

Asking a maintainer about:

  • pull request is over the files limit: 33 files (max 30)

Notes:

  • Risk: medium (33 files, 7 recently fixed files)

@corbits-triage corbits-triage Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidates all Triage deployments on a hub's machine into one local sidecar process, with a shared cap, overflow to sandbox provisioners, a five-minute stuck-manager reaper, and a settings readout of sidecar load. The most important defects are a stale destroy that can report cleanup as required for the live replacement, and a corrupt holds file that blocks hub startup.

Not reviewed in full: 13 files, over the size limit.

  • docs/ARCHITECTURE.md
  • docs/Corbits Triage User Manual/Corbits Triage User Manual.md
  • docs/DEV.md
  • docs/SELF_HOST.md
  • docs/provisioners/README.md
  • docs/provisioners/local.md
  • packages/triage-contracts/src/hub-responses.ts
  • packages/triage-contracts/src/listener.ts
  • packages/triage-judge/src/triage-judge-bitbucket-datacenter.ts
  • packages/triage-judge/src/triage-judge-github.ts
  • tests/chain/shared-sidecar.test.ts
  • tests/chain/sidecar-cap.test.ts
  • tests/chain/stack.ts

Comment on lines +241 to +246
return { kind: "destroyed", cleanup: request.sidecarId === manifest?.sidecarId ? "required" : "confirmed" };
}

// Ensure or destroy may reach an allocation before restore does; their
// outcome wins.
async function restoreAllocation(
{ manifest, isLive }: { readonly allocationId: string; readonly manifest: LocalSidecarManifest; readonly isLive: IsLocalSidecarLive },
): Promise<void> {
if (allocations.has(manifest.allocationId)) return;
if (!(await isLive(manifest))) {
await removeStored(manifest.allocationId, manifest.dataDir);
logAllocation("local_sidecar_removed", manifest);
return;
}
const request = { ...manifest, adapterManifest };
const handle = spawnSidecar({ request, dataDir: manifest.dataDir });
const managed: ManagedProcess = { handle, exited: false };
void trackExit(managed);
allocations.set(manifest.allocationId, {
kind: "live",
generation: manifest.generation,
sidecarId: manifest.sidecarId,
dataDir: manifest.dataDir,
process: managed,
});
logAllocation("local_sidecar_respawned", manifest);
}

async function removeOrphan(
{ allocationId, dataDir }: { readonly allocationId: string; readonly dataDir: string },
): Promise<void> {
const state = allocations.get(allocationId);
if (state?.kind === "live" && state.dataDir === dataDir) return;
if ((await manifests.readDataDir(allocationId)) === dataDir) return;
async function forget(): Promise<void> {
await manifests.remove();
await manifests.writeHolds([]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 · A stale destroy can report cleanup 'required' for the sidecar's live replacement

The old destroy path returned confirmed early when the hold was at a newer generation, so a delayed destroy for a superseded identity could not touch the replacement. The new code drops that guard. When the stale request names the sidecar's id, it still returns required. The hub then has to remove the allocation's copy from the sidecar, which is keyed by allocation id and can take down the newer generation's deployment. The hub also keeps its slot reserved while it waits for that removal.

Trigger: a destroy for generation 1 arrives after generation 2 of the same allocation was ensured on the sidecar's id (manifest.sidecarId). The hold stays at generation 2, but the reply is cleanup required.

Fix: restore the generation check so that a destroy whose generation is older than the hold's returns confirmed without touching the live copy, for example by comparing request.generation to the hold before choosing required.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changed on purpose. Answering confirmed for the shared sidecar's id makes stock retire that sidecar (retireSidecar runs before the lease check in the reconciler), and every later placement is refused as retired until the hub restarts. A stale destroy cannot reach the live copy with required: the stock release path checks generation and lease first, and undeploy only targets a connection whose fence generation equals the target. The test "a destroy older than the allocation's hold leaves the live generation held" covers it.

Comment thread apps/hub/src/sidecar/manifest-store.ts Outdated
}
async function readHolds(): Promise<LocalSidecarHold[]> {
const text = await readOptional(holdsFile);
return text === null ? [] : (JSON.parse(text) as LocalSidecarHold[]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · A corrupt holds file makes boot throw instead of recovering

readHolds calls JSON.parse with no guard, while the manifest reader catches parse errors and removes the bad file. A torn or corrupt holds.json therefore throws from restoreSidecar and stops hub startup. The next boot throws again because nothing removes the file.

Trigger: holds.json is truncated or corrupted on disk, for example after a crash during a write, and the hub boots.

Fix: wrap the parse in a try/catch like the manifest reader does, log the error, remove or quarantine the file, and return [].

Comment thread apps/hub/src/sidecar/placement.ts Outdated
// With nowhere else to go the deployment still goes local: stock retries a full sidecar's placement with backoff,
// so it waits there until a deployment ends.
if (sandbox === undefined) return local;
return (await activeDeployments(db, local.id)) >= maxDeployments ? sandbox : local;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Cap check races concurrent placements, so overflow can go to a full local sidecar

The chooser counts active local allocations and then places the deployment, with no lock or reservation. Two placements running side by side can both read count below the cap and both pick local. The extra one then waits in stock's slot retry on the local sidecar, even though a sandbox was available to take it.

Trigger: a burst of manager deployments arrives while the sidecar sits one slot below TRIAGE_SIDECAR_MAX_DEPLOYMENTS and a sandbox provisioner is registered.

Fix: reserve the slot atomically, for example with a conditional insert or a transaction with a row lock on the local sidecar row, so the count check and the placement decision cannot interleave.

Comment on lines +252 to +256
async function restoreSidecar({ isLive, hasWork }: LocalSidecarRecords): Promise<void> {
const stored = await manifests.read();
const recorded = await manifests.readHolds();
if (stored === null) {
if (recorded.length > 0) await forget();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Old per-allocation manifests and data dirs are never cleaned up after upgrade

Restore now reads only sidecar.json and holds.json. The old manifests/<allocationId>.json files and <allocationId>-XXXXXX data dirs are ignored, and the orphan sweep that used to remove them was deleted. On an existing install they stay on disk for good, along with their encrypted tokens. Allocations still marked allocated are never respawned and are not removed by restore either.

Trigger: upgrading a hub that had local sidecars running under the previous layout.

Fix: add a one-time migration in restoreSidecar that finds the legacy manifests and data dirs, removes them (and their tokens) after confirming no live allocation references them, and marks any leftover allocated rows so they are reconciled rather than left orphaned.

@corbits-triage corbits-triage Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runs every Triage deployment on a hub's machine inside one shared local sidecar process, with a configurable active-deployment cap, overflow to sandbox provisioners, a five-minute stuck-manager rule, a half-hour slot wait, and new Settings counts. The most important defect is that destroy answers can leave slots reserved for copies that were never deployed, which is the leak this change is meant to prevent.

Not reviewed in full: 17 files, over the size limit.

  • apps/hub/src/workflow-versions.test.ts
  • apps/hub/src/workflow-versions.ts
  • apps/web/src/components/settings/TriageSection.tsx
  • apps/web/src/lib/tenant-entities.ts
  • docs/ARCHITECTURE.md
  • docs/Corbits Triage User Manual/Corbits Triage User Manual.md
  • docs/DEV.md
  • docs/SELF_HOST.md
  • docs/provisioners/README.md
  • docs/provisioners/local.md
  • packages/triage-contracts/src/hub-responses.ts
  • packages/triage-contracts/src/listener.ts
  • packages/triage-judge/src/triage-judge-bitbucket-datacenter.ts
  • packages/triage-judge/src/triage-judge-github.ts
  • tests/chain/shared-sidecar.test.ts
  • tests/chain/sidecar-cap.test.ts
  • tests/chain/stack.ts

sidecarId: request.sidecarId,
});
return { kind: "destroyed", cleanup: "confirmed" };
return { kind: "destroyed", cleanup: request.sidecarId === manifest?.sidecarId ? "required" : "confirmed" };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 · Destroy answers 'required' for copies that were never deployed, so their slot may never free

Why it is wrong: the provisioner returns required for any destroy whose sidecarId matches the sidecar's id. Stock then waits for the sidecar to acknowledge removal, but an allocation that never deployed a copy has nothing to acknowledge. Its row stays in replacing, releasing or destroy_failed with deploymentCleanupConfirmed false, and placement.ts's holdsSlot keeps counting it against the cap.

Trigger: a manager's ensure joins the shared sidecar and is then refused or replaced before any deploy reaches it, for example with sidecar_reuse_rejected while still provisioning. Stock destroys it with the sidecar's id, gets required, and no copy ever appears to be removed.

Suggested fix: only answer required when a copy was actually deployed for that allocation, and otherwise answer confirmed. For example, check the holds or manifest for a deployed copy before choosing the cleanup value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trigger does not occur. A placement refused for room (sidecar_reuse_rejected) keeps the allocation on its offered id, because markAllocated leaves it unchanged on that error, so its destroy already answers confirmed. required is only answered for the shared sidecar's own id, where confirmed would retire the sidecar. The chain test sidecar-cap.test.ts (second manager refused, then runs once the first ends) covers it.

const stale = existing !== undefined && existing.generation > request.generation;
if (!stale) {
const sidecarIds = existing?.kind === "live" ? [request.sidecarId, existing.offeredSidecarId] : [request.sidecarId];
holds.set(request.allocationId, { kind: "destroyed", generation: request.generation, sidecarIds });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Destroyed fences are only held in memory, so a restart forgets them

Why it is wrong: the contract requires a provisioner to store its holds durably, because the hub never re-announces them. persistHolds writes only live holds, and a destroyed fence is written only when it replaces a live hold. A destroyed fence for an allocation with no live hold is never persisted.

Trigger: a destroy runs for an allocation with no live hold, the hub restarts, and then a delayed ensure arrives for the same generation and destroyed sidecar identity. That ensure is accepted again.

Suggested fix: persist every destroyed fence when it is recorded, not only when it overwrites a live hold, for example by calling persistHolds right after holds.set(...) in the destroy path.

Comment thread apps/hub/src/sidecar/placement.ts Outdated
if (sandbox === undefined) return local;
const at = now();
reservations = reservations.filter((until) => until > at);
if ((await activeDeployments(db, local.id)) + reservations.length >= maxDeployments) return sandbox;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Placement count ignores probes on the local sidecar, so the cap can be exceeded

Why it is wrong: activeDeployments counts only sidecarAllocation rows. Probes also run on the local sidecar, because probeSidecarProvisioners uses the same list, and they are tracked in workflowProbe rows that this count never reads. A full sidecar can therefore look as if it has room.

Trigger: code-review probes occupy sidecar capacity while the allocation count is still under the cap. The chooser picks local, and the sidecar rejects the placement.

Suggested fix: include active workflowProbe rows for the local provisioner in the count used by the chooser, so probes and deployments draw from the same budget.

Comment thread apps/hub/src/manager-state.ts Outdated
if (run.status === "running") return true;
const age = now.getTime() - run.createdAt.getTime();
if (run.allocationStatus === "allocated" && run.sidecarId !== null) return !startedByMail || onSidecarMs(run, now) < NEVER_RAN_MS;
if (run.failureCode === WAITING_FOR_SLOT) return age < SLOT_WAIT_MS;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 · Slot-wait rule keys on failureCode alone, ignoring allocation status

Why it is wrong: the rule checks only failureCode === "sidecar_reuse_rejected". It does not confirm the allocation is still provisioning. If an allocation has failed or moved to another non-waiting state but keeps the leftover code, the run still gets the 30-minute claim.

Trigger: an allocation is refused for room, then ends in a non-waiting state while the run is still in a live status. The run keeps the pull request's claim long after it stopped waiting.

Suggested fix: require the allocation's status to be provisioning as well as the failure code before applying the half-hour window, for example by checking both fields together.

Comment thread apps/hub/src/sidecar/placement.ts Outdated
Comment on lines +33 to +38
inArray(allocation.status, ["provisioning", "allocated"]),
and(inArray(allocation.status, ["replacing", "releasing", "destroy_failed"]), eq(allocation.deploymentCleanupConfirmed, false)),
);
}

async function activeDeployments(db: DB["db"], provisionerId: string): Promise<number> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 · Placements still waiting for a slot are counted as active local deployments

Why it is wrong: holdsSlot counts every provisioning allocation. That includes placements refused for room (sidecar_reuse_rejected) that are only waiting to retry. They are not on the sidecar, but they inflate the active count, so Settings can report more active deployments than the cap.

Trigger: the cap is reached with no sandbox, later managers queue in placement retry, and each one is counted in active.

Suggested fix: exclude provisioning allocations that carry the sidecar_reuse_rejected code from the active count, or report waiting placements as a separate number in Settings.

@corbits-triage corbits-triage Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidates all Triage deployments on a hub machine into one shared local sidecar process, with a per-deployment cap, overflow to sandbox provisioners, a five-minute stuck-manager rule, and restore/fence persistence across restarts. The most important defects are in restore ordering: ensures or destroys that arrive before restore runs can spawn a duplicate sidecar or misreport the persisted sidecar, and a corrupt fences file silently drops late-ensure protection.

Not reviewed in full: 21 files, over the size limit.

  • apps/hub/src/sidecar/placement.postgres.test.ts
  • apps/hub/src/sidecar/placement.ts
  • apps/hub/src/sidecar/restore.ts
  • apps/hub/src/sidecar/spawn.ts
  • apps/hub/src/workflow-versions.test.ts
  • apps/hub/src/workflow-versions.ts
  • apps/web/src/components/settings/TriageSection.tsx
  • apps/web/src/lib/tenant-entities.ts
  • docs/ARCHITECTURE.md
  • docs/Corbits Triage User Manual/Corbits Triage User Manual.md
  • docs/DEV.md
  • docs/SELF_HOST.md
  • docs/provisioners/README.md
  • docs/provisioners/local.md
  • packages/triage-contracts/src/hub-responses.ts
  • packages/triage-contracts/src/listener.ts
  • packages/triage-judge/src/triage-judge-bitbucket-datacenter.ts
  • packages/triage-judge/src/triage-judge-github.ts
  • tests/chain/shared-sidecar.test.ts
  • tests/chain/sidecar-cap.test.ts
  • and 1 more

P3 · Provisioner conformance suite removed for the local provisioner, so the stock contract is no longer exercised · apps/hub/src/sidecar/local-provisioner.conformance.test.ts:1

Why it is wrong: the deleted conformance test was the only place the local provisioner was checked against the shared provisioner contract. The rewritten provisioner now reports destroy as required or confirmed and accepts an ensure with a different sidecarId. Nothing in the diff replaces that coverage, so these contract changes are untested against stock.

Trigger: a later change breaks the local provisioner's ensure or destroy results against the stock contract, and no test catches it.

Suggested fix: restore the describeProvisionerConformance("local-process", ...) suite and add cases for the new required and confirmed destroy outcomes and for an ensure carrying a different sidecarId.

Not checked against the file.

Comment on lines +335 to +338
manifest = stored;
for (const hold of live) holds.set(hold.allocationId, { kind: "live", ...hold });
await manifests.writeHolds(live);
const child = start(stored);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 · Restore overwrites state from ensures that ran first and can start a second sidecar process

Why it is wrong: if an ensure reaches the provisioner before restore runs, manifest is still null. That ensure mints a new sidecar, writes sidecar.json, and spawns a process. Restore then reads that manifest and calls start(stored) again, leaving two processes under one identity and dropping the first running handle. Restore also replaces the in-memory holds and holds.json with only the restored list, losing holds written by earlier ensures, and the old sidecar's manifest and token are lost.

Trigger: stock ensures a deployment or probe on boot before local.restore runs, while a stored sidecar from the previous boot exists.

Suggested fix: make every provisioner entry point wait on a single restore promise, or reject ensure and destroy with a retryable error until restore completes. Restore should merge holds rather than overwrite them, and should never assign manifest while a start is already in flight.

// A previous hub process may have left this allocation's state on disk.
const dataDir = await manifests.readDataDir(request.allocationId);
if (dataDir !== null) await removeStored(request.allocationId, dataDir);
return { kind: "destroyed", cleanup: request.sidecarId === manifest?.sidecarId ? "required" : "confirmed" };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · A destroy before restore answers 'confirmed' for the persisted sidecar's id

Why it is wrong: manifest stays null until restore loads it, so request.sidecarId === manifest?.sidecarId is false for the stored sidecar. The destroy then returns cleanup: "confirmed". Per the code's own comments, stock then retires that identity and stops placements on the sidecar, but the copy is never undeployed and keeps holding a slot.

Trigger: a destroy for an allocation hosted on the existing sidecar is processed after boot but before restore has set manifest.

Suggested fix: either hold destroy until restore finishes, or compare against the persisted manifest read from disk rather than the in-memory manifest, and return required whenever the id matches the stored sidecar.

Comment thread apps/hub/src/sidecar/manifest-store.ts Outdated
return { ...stored, token: await cipher.decrypt(stored.token, manifestTokenAad(stored.allocationId)) };
return schema.assert(JSON.parse(text));
} catch (error) {
await fs.rm(file, { force: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · A corrupt fences file is deleted, dropping late-ensure fences after a restart

Why it is wrong: readList deletes any unreadable list file and returns an empty list. For fences.json, that removes every destroyed-generation fence, so a late ensure for a destroyed generation is accepted again after a restart. The change says fences must survive restarts, but this path does not keep them.

Trigger: fences.json is truncated or unparseable at boot, for example after a crash during a write, and stock then sends a late ensure for an allocation that was destroyed earlier.

Suggested fix: do not delete fences on parse failure. Move the corrupt file aside and fail closed, refusing ensures for unknown generations until the fences can be rebuilt, or write fences atomically so a crash cannot leave a truncated file.

@corbits-triage corbits-triage Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidates all Triage deployments on the hub's machine into one local sidecar process, with a local provisioner that persists holds and fences, a per-sidecar cap with overflow to sandbox provisioners, and a five-minute reaper for stuck managers and judges. The most important defect is that a destroy for a live copy can report confirmed when the offered identity differs from the shared sidecar, which would let stock retire an identity whose copy is still running.

Not reviewed in full: 22 files, over the size limit.

  • apps/hub/src/sidecar/manifest-store.ts
  • apps/hub/src/sidecar/placement.postgres.test.ts
  • apps/hub/src/sidecar/placement.ts
  • apps/hub/src/sidecar/restore.ts
  • apps/hub/src/sidecar/spawn.ts
  • apps/hub/src/workflow-versions.test.ts
  • apps/hub/src/workflow-versions.ts
  • apps/web/src/components/settings/TriageSection.tsx
  • apps/web/src/lib/tenant-entities.ts
  • docs/ARCHITECTURE.md
  • docs/Corbits Triage User Manual/Corbits Triage User Manual.md
  • docs/DEV.md
  • docs/SELF_HOST.md
  • docs/provisioners/README.md
  • docs/provisioners/local.md
  • packages/triage-contracts/src/hub-responses.ts
  • packages/triage-contracts/src/listener.ts
  • packages/triage-judge/src/triage-judge-bitbucket-datacenter.ts
  • packages/triage-judge/src/triage-judge-github.ts
  • tests/chain/shared-sidecar.test.ts
  • and 2 more

sidecarId: request.sidecarId,
});
return { kind: "destroyed", cleanup: "confirmed" };
return { kind: "destroyed", cleanup: request.sidecarId === manifest?.sidecarId ? "required" : "confirmed" };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 · Destroy answers confirmed for a live copy whose offered identity differs from the sidecar's

Why it is wrong: a live hold always runs on the shared sidecar (manifest.sidecarId), not on the identity it was offered. The return value compares the request against the manifest, but when the request names the offered identity the code reports confirmed, which tells stock the worker is gone. The contract reserves confirmed for a worker that is really gone and uses required for a copy that remains.

Trigger: allocation b is ensured with offered id sc_b but accepted on the shared sidecar sc_a. A later destroy names sc_b, for example if stock kept the offered id. The destroy returns confirmed, so stock never undeploys b's copy and it keeps holding a slot.

Suggested fix: compare the request against the sidecar the copy actually runs on, and return required whenever a live hold exists on manifest.sidecarId, whatever identity was offered. Add a test for a destroy that names the offered identity of a joined allocation.

Comment on lines +304 to 309
}

async function forget(): Promise<void> {
await manifests.remove();
await manifests.writeHolds([]);
await fs.rm(dataDir, { recursive: true, force: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Destroyed fences pile up in memory and every destroy queries all of them

Why it is wrong: persistFences trims only the file. The in-memory holds map never drops a destroyed entry, even after stock has ended that allocation. Every later destroy, and every ensure over a fence, sends all ids ever destroyed to records.stillOpen.

Trigger: a long-running hub that cycles through many pull requests. Each manager and judge allocation leaves a destroyed entry, so each destroy queries a list that keeps growing, and memory grows with it.

Suggested fix: when stock reports an allocation ended, delete its entry from holds as well as from the fences file, and pass only the still-open ids to records.stillOpen. Add a test that a destroyed entry is dropped from memory once stock has ended it.

Comment on lines +242 to +244
const sidecar = await sidecarFor(request);
if (sidecar.kind === "rejected") return sidecar;
holds.set(request.allocationId, { kind: "live", allocationId: request.allocationId, generation: request.generation, offeredSidecarId: request.sidecarId });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Sidecar identity minted from an ensure that then throws or times out is kept

Why it is wrong: the rollback runs only when the spawn itself fails. If persistHolds or persistFences throw afterwards, or stock stops awaiting the ensure, stock treats the offered identity as failed and retires it. The manifest still names that identity, so every later deployment is told to join a retired sidecar.

Trigger: the first ensure mints the sidecar, then the holds file write fails, or the hub-side ensure times out.

Suggested fix: wrap the whole ensure path after sidecarFor in the same rollback, and clear the manifest's sidecar identity when any later step fails. Add a test where the holds write fails after a successful spawn and the next ensure mints a fresh identity.

Comment thread apps/hub/src/server.ts Outdated
Comment on lines +248 to +250
composition.sidecarRouter.events.on("sidecar.allocated.connected", function placed(allocation) {
notePlaced(allocation);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Placement time recorded only on connection events, which joins may not emit

Why it is wrong: on a shared sidecar, later deployments join a connection that already exists, so a per-connect event may never fire for them. With no notePlaced entry, onSidecarMs falls back to max(createdAt, hubStartedAt). A manager that waited for a slot is then reaped as soon as it lands.

Trigger: a manager waits more than five minutes for a slot, then joins an already-connected sidecar without a sidecar.allocated.connected event.

Suggested fix: record the placement time when the hub actually places the allocation on the sidecar, not only on the connect event, for example at the point the join is accepted. Add a test where the join arrives on an existing connection and the manager is not reaped.

Comment thread apps/hub/src/server.ts Outdated
Comment on lines +242 to +248
sidecarProvisionerChooser: placeDeployments,
probeSidecarProvisioners: sidecarProvisioners,
probeSidecarCapabilityRules: workloadProvisioners.length > 0 ? [{ capability: "isolation:workload", effect: "require" }] : [],
extraRunTokenRoutes: MANAGER_RUN_TOKEN_ROUTES,
});
const stock = composition.server;
composition.sidecarRouter.events.on("sidecar.allocated.connected", local.connected);
composition.sidecarRouter.events.on("sidecar.allocated.connected", function placed(allocation) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 · Probes are never overflowed: their chooser is not set, so they always go to the local sidecar even when it is full

Why it is wrong: sidecarProvisionerChooser is passed only for deployments. The probe registry has no chooser, so it takes the first matching provisioner, which is the local one. A probe on a full sidecar waits there, even when a sandbox could take it. The cap and overflow count assume probes share the local slots, so probes waiting on the local sidecar can delay the deployments that depend on them.

Trigger: the local sidecar has cap active deployments and probes, and a non-isolated workflow's probe is placed. It goes to local rather than an available sandbox and waits in stock's retry until a slot frees.

Suggested fix: pass the same overflow chooser to the probe registry, or document and test the intended behaviour explicitly. Add a test where the local sidecar is full and a probe is placed on a sandbox.

The local provisioner starts one sidecar on the first ensure and answers
every later ensure with it. Holds are kept per allocation in a holds file
beside one process manifest; destroy releases the hold and leaves the copy
for the hub to undeploy. Boot respawns the sidecar once for the holds still
allocated and removes the per-allocation layout. TRIAGE_SIDECAR_MAX_DEPLOYMENTS
(default 16) sets the active deployments the sidecar advertises.
…s full

A placement chooser counts the local sidecar's active allocations as stock
counts a sidecar's room and sends a deployment to the first matching sandbox
provisioner once the cap is reached. With no sandbox it stays local and
waits in stock's placement retry until a slot frees.
The workflow versions route the portal reads gains the local sidecar's
active deployments, cap and overflow, and Settings -> Triage says it in
one sentence. Architecture, provisioner and user docs describe the one
process, its cap and the overflow.
A manager or judge is deployed with the mail that starts it, so one still
deployed on its sidecar after five minutes is stuck: the reconcile pass
ends it and logs triage_deployment_never_ran, and it no longer holds its
pull request's claim. A listener is exempt, since it stays deployed until
its first pull request. A run whose placement waits for a free slot keeps
its claim.
…retire legacy sidecars

- Placement counts active probes on the local sidecar with its deployments,
  and counts placements waiting for a slot apart; Settings shows them as
  `waiting`.
- The half-hour slot wait applies only while the allocation is still
  provisioning.
- Every destroyed fence is persisted, restored on boot while stock may still
  ensure that allocation or probe, and pruned once it cannot.
- On boot, each sidecar the per-allocation layout left is reported stopped
  through stock's sidecarStopped with no reconnect window, then its manifest
  and data directory are removed; one that cannot be retired is kept for the
  next boot.
…s on each write

Stock records a placement refused for room as `replacing` with
`sidecar_reuse_rejected` until it retries, never as `provisioning`. The
half-hour claim and the waiting count now match that row, and the dead
`provisioning` exclusion in the active count is gone. Each fence write drops
fences whose allocation or probe stock has ended.
… a sidecar

Stock moves a placement refused for room between `replacing` and
`provisioning` on each retry and clears the refusal's code in the latter, so
the half-hour wait now covers any allocation in either state, timed from the
allocation's creation, whatever its failure code.
…eadable fences

Ensure and destroy wait on the provisioner's restore, so none can start a
second sidecar or answer for a sidecar restore has not loaded yet; a failed
restore fails them. An unreadable fences file is moved aside and the fences
rebuilt from the allocations and probes stock has ended, each at its current
generation for every identity.
…led ensures, overflow probes

- A destroy is `required` whenever the sidecar hosts the allocation's copy,
  whatever identity it names; a fence remembers that for a repeated destroy.
- Fences live in their own map, and each destroy forgets those of
  allocations and probes stock has ended. An ensure for one stock ended is
  refused from its record, which replaces rebuilding lost fences.
- An ensure that fails or is abandoned after the sidecar starts leaves the
  hold as it was and discards a sidecar minted under its identity.
- The provisioner reports each accepted allocation, so a manager that joins
  the sidecar is timed from its placement without a connection event.
- Probes use the deployments' placement chooser and overflow to a sandbox.
- A repeated destroy keeps the identities its first fence named.
- The never-ran reaper cancels through stock's lifecycle service.
- Drop the probe chooser: probes never see the local sidecar beside a sandbox.
- Drop the half-hour slot wait: stock gives up a placement after 10 ensure
  attempts, so past the cap a deployment overflows to a sandbox or retries for
  about two and a half minutes, fails, and is tried again on the next event.
  Docs and Settings say so.
- Remove a log-only test and a duplicate fence-pruning test.
@TheGreatAxios
TheGreatAxios merged commit 8971984 into main Oct 27, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant