Detect dead sandboxes on failed runners#762
Conversation
When a distributed runner dies, its session-scoped READY status clears after ~60s, but nothing on the coordinator reacted to that signal. Sandboxes stayed in RUNNING forever and the coordinator kept routing traffic to dead IPs. Adds a nodehealth controller that watches Node entities. When a node loses READY, it starts a 5-minute grace period, then marks the node's sandboxes DEAD. The grace period protects the normal restart path where containers survive on containerd and the runner re-adopts them via reconcileSandboxesOnBoot(). Closes MIR-1009
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controllers/nodehealth/controller.go (1)
62-79: Handled dead nodes will be rescanned forever.Once a node crosses the grace period, nothing transitions it out of the “expired” path. With the 30s resync in
components/coordinate/coordinate.go, a permanently dead node will keep emitting WARNs and re-listing its sandboxes every cycle even after everything is alreadyDEAD/STOPPED. Consider tracking an “already handled” state and clearing it on recovery.Also applies to: 105-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/nodehealth/controller.go` around lines 62 - 79, The current logic in controller.go uses trackUnhealthy()/now()/gracePeriod to detect expired nodes and calls markNodeSandboxesDead(node.ID) but never records that the node has been handled, so recovered nodes still re-enter the expired path and dead nodes are reprocessed every resync; add an “alreadyHandled” flag/state keyed by node.ID inside the controller (e.g., handledNodes map or similar) and modify the flow in the functions that call trackUnhealthy and markNodeSandboxesDead to: 1) check handledNodes before calling markNodeSandboxesDead and skip/log once if already handled, 2) set handledNodes[node.ID]=true immediately after successfully calling markNodeSandboxesDead, and 3) clear handledNodes[node.ID] when node.Status indicates recovery (use the same place that resets trackUnhealthy on healthy status) so the node can be reprocessed on subsequent failures; apply the same change to the other block that mirrors this logic (the block around the markNodeSandboxesDead call in the 105–145 region).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/nodehealth/controller.go`:
- Around line 56-79: The Reconcile loop currently starts the grace timer for any
non-READY state including operator-initiated compute_v1alpha.DISABLED; change
Reconcile so DISABLED nodes do not enter the grace/tracking path: detect
node.Status == compute_v1alpha.DISABLED near the top of Reconcile (before
calling trackUnhealthy), call c.clearTracking(node.ID) and return nil so we
don't start the grace timer or later call c.markNodeSandboxesDead; this keeps
the existing trackUnhealthy/clearTracking/markNodeSandboxesDead logic intact but
skips grace handling for DISABLED nodes.
---
Nitpick comments:
In `@controllers/nodehealth/controller.go`:
- Around line 62-79: The current logic in controller.go uses
trackUnhealthy()/now()/gracePeriod to detect expired nodes and calls
markNodeSandboxesDead(node.ID) but never records that the node has been handled,
so recovered nodes still re-enter the expired path and dead nodes are
reprocessed every resync; add an “alreadyHandled” flag/state keyed by node.ID
inside the controller (e.g., handledNodes map or similar) and modify the flow in
the functions that call trackUnhealthy and markNodeSandboxesDead to: 1) check
handledNodes before calling markNodeSandboxesDead and skip/log once if already
handled, 2) set handledNodes[node.ID]=true immediately after successfully
calling markNodeSandboxesDead, and 3) clear handledNodes[node.ID] when
node.Status indicates recovery (use the same place that resets trackUnhealthy on
healthy status) so the node can be reprocessed on subsequent failures; apply the
same change to the other block that mirrors this logic (the block around the
markNodeSandboxesDead call in the 105–145 region).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4768fce7-411e-4c13-b841-2ddcfb117508
📒 Files selected for processing (3)
components/coordinate/coordinate.gocontrollers/nodehealth/controller.gocontrollers/nodehealth/controller_test.go
DISABLED is set by Drain() during graceful shutdown, which handles its own sandbox cleanup. Starting a grace timer on DISABLED nodes would race with the drain. Skip them. Also track when a node has already been handled so we don't re-scan its sandboxes on every 30s resync. The handled flag clears when the node recovers, is removed, or enters DISABLED.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/nodehealth/controller.go`:
- Around line 97-100: markNodeSandboxesDead currently logs per-Patch failures
but returns nil, allowing Reconcile to call setHandled even when some sandboxes
failed to be patched; modify markNodeSandboxesDead to collect any per-patch
errors (e.g., aggregate into a multi-error or wrap the first error) and return a
non-nil error when any patch failed, and ensure Reconcile only calls
setHandled(node.ID) when markNodeSandboxesDead returns nil; apply the same
change to the other similar call site (the second invocation around lines
176-195) so partial failures prevent marking the node as handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfd86600-9ce8-4856-a3f0-ee6e23d4c3d2
📒 Files selected for processing (2)
controllers/nodehealth/controller.gocontrollers/nodehealth/controller_test.go
If some sandbox patches fail in markNodeSandboxesDead, return the error so Reconcile doesn't call setHandled. This ensures the controller retries on the next resync rather than permanently skipping a node with sandboxes stuck in RUNNING.
evanphx
left a comment
evanphx
left a comment
There was a problem hiding this comment.
Approving — ship it. 🚢
Evan + Claude took a look at this together. Clean, well-scoped, good test matrix, and the grace-period / handled-latch design choices all make sense given the reconcile-on-boot recovery path you're protecting.
One thing we spotted that's worth a follow-up (not a blocker):
In controllers/nodehealth/controller.go:179-185:
_, err := c.eac.Patch(ctx, entity.New(
entity.DBId, sb.ID,
(&compute_v1alpha.Sandbox{
Status: compute_v1alpha.DEAD,
}).Encode,
).Attrs(), 0)This reads like "patch just Status to DEAD", but Sandbox.Encode() in api/compute/compute_v1alpha/schema.gen.go:1103 doesn't quite play along. Most fields are gated behind !Empty / len() > 0 checks, but HostNetwork is emitted unconditionally at line 1107:
attrs = append(attrs, entity.Bool(SandboxHostNetworkId, o.HostNetwork))So the patch actually sends two attrs: Status=DEAD and HostNetwork=false. Any sandbox on a dead node that was using host networking will silently get HostNetwork flipped to false on its way to DEAD. Probably harmless in practice since the sandbox is terminal, but it's a latent trap for the next person who tries a targeted Encode-based patch.
Two ways to deal with it, whenever you feel like it:
- Build the attr list by hand here — just
entity.Ref(SandboxStatusId, sandboxstatusToId[DEAD])— and sidestepEncodeentirely. - Or fix the generator to gate
Boolfields the same way as the other scalars. That's the real fix and probably affects other kinds too, so it's its own PR.
Also a tiny aside: Patch(..., 0) skips optimistic concurrency, where most other callers thread the version through. Low risk here since the runner's already gone, but figured we'd mention it.
Really nice work on this one — the grace-period reasoning in the PR description is great context to have in the commit trail.
— Evan & Claude
phinze
deleted the
phinze/mir-1009-coordinator-doesnt-detect-dead-sandboxes-on-failed-runners
branch
2 participants
When a distributed runner dies (process crash, node failure), its session-scoped
READYstatus on the Node entity clears after the ~60s session TTL. The scheduler already uses this to avoid placing new sandboxes on dead nodes, but nothing was watching for existing sandboxes that were already running there. They'd stay inRUNNINGstatus forever, and the coordinator would keep routing traffic to their IPs, causing request timeouts for as long as the node was down.The fix is a new
nodehealthcontroller on the coordinator that watches Node entities. When a node loses READY, the controller starts a 5-minute grace period. If the node doesn't recover within that window, it lists all sandboxes scheduled to the node and marks them DEAD. The activator's existing watch pipeline picks up the status change and immediately invalidates cached leases, so traffic stops routing to the dead IPs. The pool controller then sees the reduced capacity and spins up replacements on healthy nodes.The grace period is the interesting design choice here. Containers survive miren process restarts (containerd keeps running), and
reconcileSandboxesOnBoot()re-adopts them when the runner comes back. We verified on Garden that all 13 sandboxes survived today's restart with zero downtime. Marking sandboxes DEAD immediately would break this recovery path and cause unnecessary churn on every upgrade or restart. Five minutes is generous for any normal restart (typically 15-30 seconds) while still bounding the impact of a real failure.We also discovered during investigation that
reconcileSandboxesOnBoot()reattaches logs and re-reserves IPs for surviving sandboxes, but doesn't re-register cgroup metrics monitoring. That's tracked separately in MIR-1013.Validated on Toys with a coordinator + distributed runner topology. Killed the runner process and watched the coordinator detect the node going not-ready after session TTL, tick through the grace period for 5 minutes, then mark 3 sandboxes dead. The pool controller replaced them on surviving nodes within seconds. Also verified the restart path: bounced the runner, it recovered in under a second, grace period cleared, no sandbox disruption.
Closes MIR-1009