Repository navigation
Add cache-aware placement affinity - #2867
ValentaTomas wants to merge 43 commits into
Conversation
Record recent successful placements and use them as a small, gated best-of-K bias so warm nodes are preferred without bypassing placement filters.
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit 531e633. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 5 Tests Failed:
View the full list of 5 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![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
Creating and executing a Redis pipeline inside the loop in record defeats the purpose of pipelining and results in multiple sequential round-trips. Similarly, performing sequential Redis queries inside the loop in scores on the critical path of sandbox creation introduces unnecessary latency; both should be batched using a single pipeline outside the loops to improve performance.
Use a single pipeline for affinity reads and writes to keep the placement path bounded when the flag is enabled.
Use error-aware Redis checks and apply the repository formatter to the new feature flag.
Move cache affinity rollout knobs into the existing flag so Redis TTL, scoring, limits, and timeouts can be adjusted without a deploy.
Keep resume origin-node affinity as the primary signal and skip per-snapshot build keys so the Redis placement cache stays bounded.
Avoid Redis affinity lookups when resume already has a ready origin node and skip disabled affinity dimensions.
Use team-scoped template and base-template affinity as a low-cardinality fallback for resumes when origin-node placement is unavailable.
Track team-level placement history as the weakest cache-affinity fallback alongside template and base-template signals.
Keep template/team dimensions available for experiments but disable them by default until base build metadata is available.
Remove proxy affinity dimensions so the first rollout only uses concrete build cache signals.
![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.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Affinity node repeatedly selected despite ResourceExhausted failures
- Added the ResourceExhausted node to nodesExcluded to prevent affinity augmentation from re-selecting it on retry.
Or push these changes by commenting:
@cursor push bbadc5591a
Preview (bbadc5591a)
diff --git a/packages/api/internal/orchestrator/placement/placement.go b/packages/api/internal/orchestrator/placement/placement.go
--- a/packages/api/internal/orchestrator/placement/placement.go
+++ b/packages/api/internal/orchestrator/placement/placement.go
@@ -102,6 +102,7 @@
switch statusCode {
case codes.ResourceExhausted:
+ nodesExcluded[failedNode.ID] = struct{}{}
failedNode.PlacementMetrics.Skip(sbxRequest.GetSandbox().GetSandboxId())
logger.L().Warn(ctx, "Node exhausted, trying another node", logger.WithSandboxID(sbxRequest.GetSandbox().GetSandboxId()), logger.WithNodeID(failedNode.ID))
default:You can send follow-ups to the cloud agent here.
Exclude nodes that reject placement as exhausted so affinity retries can fall through to other candidates.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: ResourceExhausted tight retry loop on last remaining node
- Added explicit check to exit retry loop with 'no nodes available' error when the last remaining node returns ResourceExhausted, preventing infinite retries until context timeout.
Or push these changes by commenting:
@cursor push 7334edb433
Preview (7334edb433)
diff --git a/packages/api/internal/orchestrator/placement/placement.go b/packages/api/internal/orchestrator/placement/placement.go
--- a/packages/api/internal/orchestrator/placement/placement.go
+++ b/packages/api/internal/orchestrator/placement/placement.go
@@ -104,6 +104,9 @@
case codes.ResourceExhausted:
if len(nodesExcluded)+1 < len(clusterNodes) {
nodesExcluded[failedNode.ID] = struct{}{}
+ } else {
+ // All nodes are exhausted; exit the retry loop.
+ return nil, errors.New("no nodes available")
}
failedNode.PlacementMetrics.Skip(sbxRequest.GetSandbox().GetSandboxId())
logger.L().Warn(ctx, "Node exhausted, trying another node", logger.WithSandboxID(sbxRequest.GetSandbox().GetSandboxId()), logger.WithNodeID(failedNode.ID))You can send follow-ups to the cloud agent here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38d8022e3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75d9d736d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Affinity bonus not normalized to placement score scale
- Normalized the affinity bonus by dividing it by totalCapacity to ensure consistent relative impact across nodes of different sizes.
Or push these changes by commenting:
@cursor push 878be92bc5
Preview (878be92bc5)
diff --git a/packages/api/internal/orchestrator/placement/placement_best_of_K.go b/packages/api/internal/orchestrator/placement/placement_best_of_K.go
--- a/packages/api/internal/orchestrator/placement/placement_best_of_K.go
+++ b/packages/api/internal/orchestrator/placement/placement_best_of_K.go
@@ -63,7 +63,7 @@
score := (cpuRequested + float64(reserved) + config.Alpha*usageAvg) / totalCapacity
if len(affinityScores) > 0 {
- score -= affinityScores[0][node.ID]
+ score -= affinityScores[0][node.ID] / totalCapacity
}
return scoreYou can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53bfe748ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Count exhausted-pool retries so a fully saturated cluster exits after the normal retry budget.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9277fc3cd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Exhausted retry counter unreachable due to attempt increment
- Removed the attempt++ increment from the exhausted pool retry path so maxExhaustedRetries (100) can be reached before the outer loop terminates at maxRetries (3).
Or push these changes by commenting:
@cursor push 9277fc3cd9
You can send follow-ups to the cloud agent here.
Let ResourceExhausted pool retries use their own retry cap instead of consuming the create-failure retry budget.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202bab4374
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d362021fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Affinity bonus not normalized by totalCapacity in Score
- Normalized the affinity bonus by dividing it by totalCapacity to match the base score normalization, ensuring consistent placement behavior across nodes with different CPU counts.
Or push these changes by commenting:
@cursor push b7048af342
Preview (b7048af342)
diff --git a/packages/api/internal/orchestrator/placement/placement_best_of_K.go b/packages/api/internal/orchestrator/placement/placement_best_of_K.go
--- a/packages/api/internal/orchestrator/placement/placement_best_of_K.go
+++ b/packages/api/internal/orchestrator/placement/placement_best_of_K.go
@@ -63,7 +63,7 @@
score := (cpuRequested + float64(reserved) + config.Alpha*usageAvg) / totalCapacity
if len(affinityScores) > 0 {
- score -= affinityScores[0][node.ID]
+ score -= affinityScores[0][node.ID] / totalCapacity
}
return scoreYou can send follow-ups to the cloud agent here.
Always pass affinity scores to PlaceSandbox (a nil map is handled downstream) and document the on-demand node discovery fallback.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Exhausted retry sleep ignores context cancellation
- Replaced unconditional time.Sleep with context-aware select statement that exits early on context cancellation.
Or push these changes by commenting:
@cursor push 1b3e4e5c91
Preview (1b3e4e5c91)
diff --git a/packages/api/internal/orchestrator/placement/placement.go b/packages/api/internal/orchestrator/placement/placement.go
--- a/packages/api/internal/orchestrator/placement/placement.go
+++ b/packages/api/internal/orchestrator/placement/placement.go
@@ -78,7 +78,14 @@
return nil, err
}
clear(nodesExhausted)
- time.Sleep(100 * time.Millisecond)
+
+ timer := time.NewTimer(100 * time.Millisecond)
+ select {
+ case <-ctx.Done():
+ timer.Stop()
+ return nil, fmt.Errorf("request timed out during exhausted retry %d", exhaustedRetries)
+ case <-timer.C:
+ }
continue
}You can send follow-ups to the cloud agent here.
Replace the blocking time.Sleep between exhausted-pool retries with a select on ctx.Done so placement bails out promptly when the client disconnects instead of looping for up to maxExhaustedRetries.
Tag created-sandbox metrics with whether placement landed on a cache-affinity-preferred node, so the feature's hit rate and load skew are observable when it is enabled.
Remove exhausted-node retry/backoff changes from the affinity PR so the branch only carries scheduling affinity behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Redis trim wipes affinity set
- Changed ZRemRangeByRank end parameter from -cfg.topNodes-1 to -(cfg.topNodes+1) to prevent deletion of entire set when member count equals topNodes.
Or push these changes by commenting:
@cursor push 2190fb6d68
Preview (2190fb6d68)
diff --git a/packages/api/internal/orchestrator/placement_affinity.go b/packages/api/internal/orchestrator/placement_affinity.go
--- a/packages/api/internal/orchestrator/placement_affinity.go
+++ b/packages/api/internal/orchestrator/placement_affinity.go
@@ -99,7 +99,7 @@
pipe := a.redis.Pipeline()
key := placementAffinityKey(clusterID, placementAffinityKindBuild, buildID)
pipe.ZIncrBy(ctx, key, 1, nodeID)
- pipe.ZRemRangeByRank(ctx, key, 0, -cfg.topNodes-1)
+ pipe.ZRemRangeByRank(ctx, key, 0, -(cfg.topNodes + 1))
pipe.Expire(ctx, key, cfg.ttl)
_, err := pipe.Exec(ctx)You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 531e633. Configure here.
| pipe := a.redis.Pipeline() | ||
| key := placementAffinityKey(clusterID, placementAffinityKindBuild, buildID) | ||
| pipe.ZIncrBy(ctx, key, 1, nodeID) | ||
| pipe.ZRemRangeByRank(ctx, key, 0, -cfg.topNodes-1) |
There was a problem hiding this comment.
Redis trim wipes affinity set
Medium Severity
ZRemRangeByRank with end rank -topNodes-1 removes the whole sorted set when it already has exactly topNodes members (stop becomes -1, i.e. lowest through highest). Steady-state affinity for a build with ~topNodes distinct nodes is cleared on every record.
Reviewed by Cursor Bugbot for commit 531e633. Configure here.
|
Temporarily closing. |
2 participants
Add disabled-by-default cache-aware placement affinity using recent successful build placements. Redis state is bounded by TTL/top-N limits and is best-effort.