Repository navigation
fix(hub): keep a head refused for capacity queued until a slot frees - #715
Conversation
TheGreatAxios
left a comment
There was a problem hiding this comment.
Independent review. bun run lint, bun run check, the plan/triage-manager/triage-contracts unit tests (366 pass) and the touched hub Postgres tests (manager routes, manager runs, triage verdicts, interleave, triage liveness, deployer role, manager intake, manager state: 79 pass) all pass. I did not run the chain scenarios.
1. The chain assertion can never pass: the stored row never has waiting for capacity (high)
observe sets error: WAITING_FOR_CAPACITY, isDue is always true for that row, and planTenant always queues the row again in the same pass, through holding. That requeue strips error (const { runId: _run, error: _error, unrouted, ...rest } = row). So the row the reconciler saves is plain queued with no error. I printed status(pass, 1) in the new plan test, and every pass shows {"status":"queued","attempts":1,...} with no error. tests/chain/sidecar-cap.test.ts waits for row?.error === WAITING_FOR_CAPACITY, so it will time out. The reason only reaches the enqueue reason. docs/ARCHITECTURE.md says the reconciler "keeps the head queued with waiting for capacity as its reason", and that is also not what gets saved. Two ways to fix it: keep error on the requeued row for a head waiting for capacity, or have the chain test and the docs assert what is actually stored. If you keep error, the portal still shows no failure, because untriagedText maps only known errors.
2. A dead branch (medium)
if (row.status === "queued" && row.error === WAITING_FOR_CAPACITY) return row; in observe is never reached, for the reason in finding 1. I deleted it and lines 195–197's comment, and all 64 plan and reconciler tests still pass. Removing any of the other three changes makes both new tests fail: the roomless branch, the isDue line, or the attempts exemption. So those tests are load-bearing. The dead branch either goes, or becomes live with the fix in finding 1.
3. The refusal marker outlives a refused run that was later placed (medium, from reading the code)
refusedRunId is cleared only when the listener names another run. Stock retries a refused placement for about two and a half minutes, so the normal recovery is that the same run gets placed, takes the claim and finishes. After that run ends, managedPulls still reports roomless (deployedRunId === refusedRunId) whenever no manager is up. Take a later head of that pull request that has no run yet, for example a push before the listener names its manager, or a manager that never starts for some other reason. That head is marked waiting for capacity, retried every pass, never aged and never charged an attempt. Fix: clear refusedRunId when that run takes the claim, or require that the refused run never took a pass (no history entry with its runId).
4. The route does not tie runId to the pull request or to the caller (low)
isManager checks only that the run is the host's manager anchor run in the tenant. A listener token can send {repo: X, number: 1, runId: <PR 2's manager>} and gets noted: true. If PR 2's run is refused, PR X then waits for capacity indefinitely. The same holds for a manager run that has already completed. The grant itself is narrow: it reaches only this route, and take returns 403, which the test covers. Cross-tenant calls fail because the grant is checked in the path's tenant. The worst case is a bad retry state inside one tenant, and only from the listener, so this is optional hardening: also require that the run is still deployed, or bind it to the deploy that the calling run made.
5. The wrapper adds a step before stock sees a refusal (low)
noteRefusals passes the provisioner's answer and errors through unchanged, wraps every provisioner in sidecarProvisioners (local, Docker, E2B, Railway), and keeps object identity for the placement chooser. A refusal for a run with no claim updates no rows and raises no error. The one cost: stock gets the refusal only after an UPDATE finishes, and that UPDATE scans the tenant's claims on an unindexed jsonb predicate. It runs on every capacity refusal, which is every 1–30 s per waiting placement. It is small, but it does change timing a little.
6. Mail failure when the report call fails (judgement, no change needed)
The order deploy, then report, then mail is right: the report needs the runId, and both calls are idempotent under their effect ids. If the report call fails, the body fails. The run is left deployed but never woken, endNeverRan cleans it up, and the reconciler queues the head again. That matches how a failed deploy behaves today. A failure partway through a batch also drops the later pull requests in that mail, the same as a failed deploy.
7. Portal
Correct that nothing new shows. To show "waiting for capacity" you would need a new field. Any untriaged text reads as a failure, so the existing field cannot carry it.
The commit is a signed Conventional Commit with no attribution, and OpenAPI describes the route.
6ae5584 to
9479e45
Compare
|
Fixed in 9479e45:
|
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at 9479e45. bun run lint, bun run check, the plan/reconciler/triage-manager/triage-contracts unit tests (402 pass) and the touched hub Postgres tests (80 pass) all pass.
All five earlier findings and the race fix are verified. Each fix makes at least one test fail when it is removed:
- Keeping
erroron the re-queued row: both plan tests fail without it. - Reachable branch: the "ages from the next pass" test fails if it is reverted to
return rowor removed. - Take clears
refusedRunId: the new Postgres test fails without it. - 409
run_endedand 409deployed_elsewhere: the route test fails without either check. - Reading the allocation for a refusal that came before the report: the new Postgres test fails without it.
Two remaining nits, both low:
recordDeployedstripsrefusedRunIdfrom the claim before writing the new run, and that step is not load-bearing. I removed it and every test still passes, becauseroomlessalready requiresrefusedRunId === deployedRunId. If the same run is ever named again after stock's retries are exhausted (the allocation is no longerreplacing), the strip would drop a refusal that is still valid. Writing{ ...claim, deployedRunId: runId, ...(waiting && { refusedRunId: runId }) }is simpler and keeps it.- In
tests/chain/sidecar-cap.test.ts,untilnow dumps SECOND's claim and row in every timeout message, including waits that have nothing to do with SECOND. If this was added to debug the CI failure, consider removing it. Otherwise, keep it only on the capacity wait.
|
Fixed in f895b81: recordDeployed now keeps an existing refusedRunId and sets it only when the run's allocation still waits for capacity; the claim/row dump is only on the capacity wait's timeout in sidecar-cap. |
9479e45 to
f895b81
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
No findings.
A pull request whose manager every provisioner refuses for capacity now stays queued until a slot frees. It is no longer marked "run never started", spends no retry attempt and has no backoff. Every reconciler pass asks for a manager again, at the pace the claim's existing hold on a recent deploy ask allows. A head that fails for any other reason keeps today's attempts and backoff exactly.
What changed:
POST /api/tenants/:tenantId/manager/deployed, documented in/openapi.json. The route needs a new, narrow grant:deployedontriage-manager:state. The listener declares it, the Deployer role holds it, and it reaches no other manager route. The run must be a live deployment of the host's manager in the tenant that no other pull request's claim already names.capacity_unavailable, the hub records that run as refused on the claim whose last deployed manager it is.replacingwithcapacity_unavailable.waiting for capacityas its reason. It skips the 10-minute never-started aging for it, and queues it again on every pass without spending an attempt, keeping that reason, even when the tenant's in-flight slots are full, since the head already holds one. Once a manager deployed after the refusal is not refused, the reason is dropped, and from that pass on the head ages and backs off as before.The signal keyed on: stock records a capacity refusal as a
sidecar_allocationrow in statusreplacingwithfailure_code = 'capacity_unavailable', which our provisioners return. That fact only lasts while the placement waits. Stock retries for up to 10 ensure attempts, about two and a half minutes, then fails the allocation assidecar_provisioning_retry_exhausted. On release,beginReleaseoverwrites the code with the run's own reason. The listener also cancels an unwoken manager aftermanagerReadyMs(2 minutes). A reconciler pass every 5 minutes would usually miss that window, so the hub records the refusal at the moment the provisioner answers it, on the claim'srefusedRunId, and the reconciler reads that.The cost: stock gets a capacity refusal only after the hub's UPDATE that records it has finished. That UPDATE matches claims on an unindexed jsonb field, scanning the tenant's claims, and runs on every capacity refusal, every 1 to 30 seconds per waiting placement.
Why the hub cannot read the pull request from stock's mail store instead: the listener's wake mail is not sent until the deployed run has published its public key (
untilReadyin packages/triage-manager/src/mail.ts). A manager refused for capacity never gets a sidecar, so no mail to it is ever stored.Portal: it shows a pull request's triage note only through the existing
untriagedfield, and anyuntriagedtext marks the item as failed. So the capacity reason is not shown there, and showing it would need a new field. The head now shows no failure while it waits.How verified:
bun run check: passes.bun run lint: passes.waiting for capacitystored on its row and its attempts unchanged, and is queued again on each pass. Meanwhile another head's failed runs spend attempts and back off.deployedgrant cannot reach take.tests/chain/sidecar-cap.test.ts, extended to assert the refused head is stored as queued for capacity with 0 attempts before it gets its verdict. On a timeout it now reports the claim'sdeployedRunIdandrefusedRunIdand the head's triage row. CI runs this scenario; I did not run it locally.Closes CL-11633