fix(api): remove sandboxes from store when node crashes#3238
Conversation
PR SummaryHigh Risk Overview Reviewed by Cursor Bugbot for commit 9c94403. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dead-node sweeper mechanism to automatically purge sandbox records from the store when their host node crashes or is removed from the fleet. It tracks node liveness across replicas using Redis, records local unreachability, and implements a background sweeper that kills sandboxes associated with dead nodes. The review feedback highlights two key improvement opportunities: first, preventing potential indefinite blocking during shutdown when acquiring the concurrency semaphore in the sweep loop by using a context-aware select block; second, increasing the discoveryFreshnessMax threshold from 30 to 60 seconds to tolerate minor network jitter and prevent premature skipping of sweep cycles.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for _, sbx := range toKill { | ||
| sem <- struct{}{} | ||
| wg.Add(1) | ||
| go func(sbx sandbox.Sandbox) { | ||
| defer wg.Done() | ||
| defer func() { <-sem }() | ||
|
|
||
| s.kill(ctx, sbx) | ||
| }(sbx) | ||
| } |
There was a problem hiding this comment.
When acquiring the semaphore inside the loop, the sweeper can block indefinitely if the concurrency limit is reached and the active goroutines are slow. If the application is shutting down and the context is cancelled, this blocking behavior prevents a graceful and timely shutdown.
Additionally, a standard break statement inside a select block in Go only breaks out of the select, not the surrounding for loop. To safely abort the loop and proceed to wait for already running goroutines, we should use a labeled break.
| for _, sbx := range toKill { | |
| sem <- struct{}{} | |
| wg.Add(1) | |
| go func(sbx sandbox.Sandbox) { | |
| defer wg.Done() | |
| defer func() { <-sem }() | |
| s.kill(ctx, sbx) | |
| }(sbx) | |
| } | |
| loop: | |
| for _, sbx := range toKill { | |
| select { | |
| case <-ctx.Done(): | |
| break loop | |
| case sem <- struct{}{}: | |
| } | |
| wg.Add(1) | |
| go func(sbx sandbox.Sandbox) { | |
| defer wg.Done() | |
| defer func() { <-sem }() | |
| s.kill(ctx, sbx) | |
| }(sbx) | |
| } |
| // discoveryFreshnessMax is how stale the last successful node discovery may | ||
| // be before the sweep refuses to run | ||
| discoveryFreshnessMax = 30 * time.Second |
There was a problem hiding this comment.
The discoveryFreshnessMax is set to 30 * time.Second, which is very close to the 20-second sync interval (cacheSyncTime). If a single sync cycle is slightly delayed (e.g., due to slow network or Nomad response) or fails once, the elapsed time since the last successful discovery will easily exceed 30 seconds, causing the dead-node sweeper to skip the cycle and log warning messages.
Consider increasing discoveryFreshnessMax to at least 60 * time.Second (or 2 * cacheSyncTime) to tolerate minor network jitter or a single missed sync cycle without immediately disabling the sweeper.
| // discoveryFreshnessMax is how stale the last successful node discovery may | |
| // be before the sweep refuses to run | |
| discoveryFreshnessMax = 30 * time.Second | |
| // discoveryFreshnessMax is how stale the last successful node discovery may | |
| // be before the sweep refuses to run | |
| discoveryFreshnessMax = 60 * time.Second |
d206cdf to
9c94403
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9c94403. Configure here.
| logger.WithNodeID(sbx.NodeID), | ||
| ) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Routing catalog not cleared
Medium Severity
When the dead-node sweep removes a sandbox with KillReasonNodeGone and getOrConnectNode returns nil, removeSandboxFromNode returns before routingCatalog.DeleteSandbox. The store entry is still removed, but the Redis routing catalog can keep pointing at the dead node until TTL expires.
Reviewed by Cursor Bugbot for commit 9c94403. Configure here.
1 participant
No description provided.