This draft is well-reasoned and ready to graduate to human review. I traced every changed path carefully and found the logic sound.
The core bug: when a sandbox's node is removed while a disk-backed deploy is waiting for teardown, the deploy's waitForPoolDrained call holds the per-app lock for up to the full PoolReadyTimeout (60 s) on every periodic resync, wedging Meet and hot-path reconcilers indefinitely. Three interlocking mechanisms fix it:
drainStaleDiskPools — non-blocking on resyncs (launcher.go)The function now returns (bool, error). The newDrains counter tracks whether this call actually wrote any new drain requests. If all stale pools are already in the drained state (DesiredInstances == 0 && len(ReferencedByVersions) == 0), the function returns (false, nil) — telling the caller to skip the service for this tick — instead of blocking in waitForPoolDrained. The first pass (which actually writes the drain request and does the block) still calls waitForPoolDrained, which is the right behaviour for an ordinary disk deploy.
The state machine is correct: a pool that hasn't been written yet has non-zero desired or non-empty refs, so it falls through to the write path; a pool written but not yet fully torn down has DesiredInstances == 0 and empty refs, so it's skipped; a pool that is gone entirely doesn't appear in stalePools at all, so the slice empties and the function returns (true, nil).
SweepOrphanedSandboxes — periodic GC in nodehealth (nodehealth/controller.go)Terminal sandboxes on deleted nodes are collected every minute. The grace period (DefaultGracePeriod, 5 min) is applied via UpdatedAt on the sandbox entity, which is the right timestamp to use: it reflects when the sandbox was last written, not when the node was deleted, so a freshly-STOPPED sandbox on a just-removed node stays alive long enough for a race-free deletion. Once collected, the pool's sandbox list shrinks to zero and waitForPoolDrained can satisfy.
markNodeSandboxesDead now includes STOPPED (nodehealth/controller.go)The previous code skipped STOPPED. That was correct when the sandbox's runner could still do the DEAD transition, but on a permanently failed node that runner is gone. Now nodehealth promotes STOPPED → DEAD for unhealthy nodes, which lets the launcher's drain poller see termination and proceed.
updatePool idempotent guard (launcher.go)A no-op equality check before the EAC write prevents the periodic resync from bumping UpdatedAt on every tick, which would have triggered watcher storms and reset the sandbox sweep grace period clock.
sandboxpool/manager.go — skip decommissioned poolsPools with DesiredInstances == 0 and empty refs are skipped during scale-down scanning, avoiding a dead load of the baked AppVersion (which may have been deleted) while waiting for sandbox entity cleanup.
The logic is precise. The key invariant — "first pass blocks, subsequent resyncs don't" — is correctly encoded in newDrains. The comment on lines 1493–1498 explains the two-phase design clearly. The SweepOrphanedSandboxes grace period check uses time.UnixMilli(e.UpdatedAt()).After(cutoff), which correctly respects the injected NowFunc in tests via c.now() for the cutoff calculation.
All new tests are substantive:
TestUpdatePoolSkipsUnchangedEntity — verifies the idempotent guard doesn't advance revision or UpdatedAtTestStoppedDisklessSandboxDoesNotBlockDiskPoolDrain — verifies the diskless STOPPED fast path in hasActiveSandboxForPoolTestDiskDrainWaitsForStoppedSandboxToDie — verifies that the second resync returns in < 500 ms and doesn't rewrite the pool (the PoolReadyTimeout is set to 2 s in that test, giving a 4× safety margin)TestSweepOrphanedSandboxes and TestSweepOrphanedSandboxesHonorsGracePeriod — cover the happy path and grace period boundaryTestMarksStoppedSandboxDeadAfterGracePeriod — renames the existing test and updates the assertion to match the new behaviour (STOPPED → DEAD for failed nodes)The TestDiskDrainWaitsForStoppedSandboxToDie timing assertion (500 ms) is a wall-clock bound in an in-memory test environment, which is normally fine here, but worth noting as a potential flake source on heavily loaded CI machines.
No concerns block merging. This is ready for human review.
time.Since(started) assertion with a logical assertion (e.g. checking that Reconcile returned before the pool's last sandbox was marked DEAD, or simply trusting the revision check below it), since the revision assertion already proves the pool was not rewritten.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1162,
"verdict": "ready",
"event": "comment",
"summary": "This draft is well-reasoned and ready to graduate to human review. I traced every changed path carefully and found the logic sound.\n\n## What the change fixes\n\nThe core bug: when a sandbox's node is removed while a disk-backed deploy is waiting for teardown, the deploy's `waitForPoolDrained` call holds the per-app lock for up to the full `PoolReadyTimeout` (60 s) on every periodic resync, wedging `Meet` and hot-path reconcilers indefinitely. Three interlocking mechanisms fix it:\n\n### 1. `drainStaleDiskPools` — non-blocking on resyncs (`launcher.go`)\n\nThe function now returns `(bool, error)`. The `newDrains` counter tracks whether this call actually wrote any new drain requests. If all stale pools are already in the drained state (`DesiredInstances == 0 \u0026\u0026 len(ReferencedByVersions) == 0`), the function returns `(false, nil)` — telling the caller to skip the service for this tick — instead of blocking in `waitForPoolDrained`. The first pass (which actually writes the drain request and does the block) still calls `waitForPoolDrained`, which is the right behaviour for an ordinary disk deploy.\n\nThe state machine is correct: a pool that hasn't been written yet has non-zero desired or non-empty refs, so it falls through to the write path; a pool written but not yet fully torn down has `DesiredInstances == 0` and empty refs, so it's skipped; a pool that is gone entirely doesn't appear in `stalePools` at all, so the slice empties and the function returns `(true, nil)`.\n\n### 2. `SweepOrphanedSandboxes` — periodic GC in nodehealth (`nodehealth/controller.go`)\n\nTerminal sandboxes on deleted nodes are collected every minute. The grace period (`DefaultGracePeriod`, 5 min) is applied via `UpdatedAt` on the sandbox entity, which is the right timestamp to use: it reflects when the sandbox was last written, not when the node was deleted, so a freshly-STOPPED sandbox on a just-removed node stays alive long enough for a race-free deletion. Once collected, the pool's sandbox list shrinks to zero and `waitForPoolDrained` can satisfy.\n\n### 3. `markNodeSandboxesDead` now includes STOPPED (`nodehealth/controller.go`)\n\nThe previous code skipped STOPPED. That was correct when the sandbox's runner could still do the DEAD transition, but on a permanently failed node that runner is gone. Now nodehealth promotes STOPPED → DEAD for unhealthy nodes, which lets the launcher's drain poller see termination and proceed.\n\n### 4. `updatePool` idempotent guard (`launcher.go`)\n\nA no-op equality check before the EAC write prevents the periodic resync from bumping `UpdatedAt` on every tick, which would have triggered watcher storms and reset the sandbox sweep grace period clock.\n\n### 5. `sandboxpool/manager.go` — skip decommissioned pools\n\nPools with `DesiredInstances == 0` and empty refs are skipped during scale-down scanning, avoiding a dead load of the baked `AppVersion` (which may have been deleted) while waiting for sandbox entity cleanup.\n\n## Code quality\n\nThe logic is precise. The key invariant — \"first pass blocks, subsequent resyncs don't\" — is correctly encoded in `newDrains`. The comment on lines 1493–1498 explains the two-phase design clearly. The `SweepOrphanedSandboxes` grace period check uses `time.UnixMilli(e.UpdatedAt()).After(cutoff)`, which correctly respects the injected `NowFunc` in tests via `c.now()` for the cutoff calculation.\n\n## Tests\n\nAll new tests are substantive:\n- `TestUpdatePoolSkipsUnchangedEntity` — verifies the idempotent guard doesn't advance revision or `UpdatedAt`\n- `TestStoppedDisklessSandboxDoesNotBlockDiskPoolDrain` — verifies the diskless STOPPED fast path in `hasActiveSandboxForPool`\n- Extension of `TestDiskDrainWaitsForStoppedSandboxToDie` — verifies that the second resync returns in \u003c 500 ms and doesn't rewrite the pool (the `PoolReadyTimeout` is set to 2 s in that test, giving a 4× safety margin)\n- `TestSweepOrphanedSandboxes` and `TestSweepOrphanedSandboxesHonorsGracePeriod` — cover the happy path and grace period boundary\n- `TestMarksStoppedSandboxDeadAfterGracePeriod` — renames the existing test and updates the assertion to match the new behaviour (STOPPED → DEAD for failed nodes)\n\nThe `TestDiskDrainWaitsForStoppedSandboxToDie` timing assertion (`500 ms`) is a wall-clock bound in an in-memory test environment, which is normally fine here, but worth noting as a potential flake source on heavily loaded CI machines.\n\nNo concerns block merging. This is ready for human review.",
"comments": [
{
"path": "controllers/deployment/launcher_test.go",
"line": 3508,
"side": "RIGHT",
"body": "The 500 ms wall-clock bound for \"an already-requested drain should not occupy the launcher until timeout\" runs in a real goroutine on potentially loaded CI hardware. If this ever flakes, consider replacing the `time.Since(started)` assertion with a logical assertion (e.g. checking that `Reconcile` returned before the pool's last sandbox was marked DEAD, or simply trusting the revision check below it), since the revision assertion already proves the pool was not rewritten."
}
],
"posted_to_pr": true,
"draft": true
}