sandbox cache: StartRemoving state transition not broadcast, all allocations see stale Running state
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 1.6k
- Forks
- 438
- PR merge metrics
- No merged PRs in 30d
Description
Summary
When StartRemoving atomically writes a state transition to Redis (e.g. Running → Killing), it never calls publishSandboxEvent. The per-allocation in-process sandbox cache (introduced in #3593) therefore never learns about the new state, and every allocation continues to serve the old Running state from memory until the sandbox is eventually deleted and a remove event arrives.
Code Path
Step 1 — Lua script writes new state to Redis, but no event is published
// state_change.go
updated := sbx
updated.State = newState // e.g. Killing
written, err := startTransitionScript.Run(ctx, s.redisClient,
[]string{key, transitionKey, resultKey},
newData, transitionID, ...) // Redis now has State=Killing
// StartRemoving returns here — no publishSandboxEvent call
return updated, false, s.createCallback(...), nil
Step 2 — createCallback publishes only a routing key, not a sandboxEvent
// state_change.go — createCallback
s.publisher.Publish(cbCtx, getTransitionRoutingKey(teamID.String(), sandboxID, transitionID))
// payload: "lock:sandbox:storage:...:transition:<uuid>"
Step 3 — dispatch routes the payload to waiters, never to cache.apply
// subscription_manager.go
func (m *subscriptionManager) dispatch(payload string) {
if isSandboxEvent(payload) { // strings.HasPrefix(payload, "{") → false for routing keys
m.cache.apply(evt) // never reached
return
}
// falls through to waiter fan-out
}
Impact
Between startTransitionScript.Run() and the final Remove() call (which does publish a remove event), every allocation returns State = Running from TeamItems for a sandbox that Redis already has as Killing or Pausing. For transient transitions, restoreToRunning calls Update() which publishes correctly, but the first half of the round-trip remains invisible to the cache.
Concretely:
TeamItems(states: [Running])returns sandboxes that are mid-removal — callers may act on them incorrectly.- Metrics and user-facing sandbox list show inflated
Runningcounts during high-eviction periods.
Fix
In StartRemoving, after startTransitionScript.Run() succeeds, broadcast the updated sandbox:
s.publisher.publishSandboxEvent(ctx, sandboxEvent{
Op: sandboxEventOpUpdate,
Sandbox: &updated,
})
This mirrors exactly what Update() does in operations.go. dispatch() will call cache.apply({update, updated}), replacing the cached entry with the correct state.
The createCallback path does not need a separate publish because:
- For permanent removals (
TransitionExpires), the subsequentRemove()call already publishes aremoveevent. - For transient transitions,
restoreToRunningcallsUpdate()which already publishes anupdateevent.
Related
- #3593 — introduces the per-allocation cache that this change affects
packages/api/internal/sandbox/storage/redis/state_change.gopackages/api/internal/sandbox/storage/redis/operations.go(reference: correct publish pattern inUpdate())
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in packages/api/internal/sandbox/storage/redis/state_change.go at StartRemoving, then compare its transition handling with the publish pattern in operations.go's Update(). Trace subscription_manager.go dispatch to confirm the cache receives the sandbox update event; done means the cache reflects the new transition state after StartRemoving without changing the existing callback behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, redis
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 86/100