Cold resume can overcommit a runner's capacity when two resumes race
SessionService.ResumeSession's cold-resume branch checks coldResumeFree (capacity total minus used minus anything still creating on the runner) and, if it sees a free slot, dispatches the resume and transitions the session straight from suspended_cold to running. It never passes through creating, so a concurrent resume reading the same runner in that window sees the same free slot and does the same thing.
I hit this with two sessions pinned to one runner that has exactly one free slot: both ResumeSession calls read coldResumeFree() == 1, both dispatch, both land in running. The runner ends up hosting one more session than its CapacityTotal allows.
The normal placement path doesn't have this gap: FleetService's scheduler transitions a session to creating (an atomic CAS via Transition, guarded by the session's current state) before it dispatches, so freeCapacity's len(creating) count reflects the claim for anyone reading after it. Cold resume skips that intermediate state entirely, so there's nothing for a second reader to see.
I reproduced it with a unit test driving the real SessionService against fakes for the two I/O ports it touches (FleetRepository, SessionRepository), with a barrier forcing both goroutines through the capacity read before either commits:
resumes that succeeded: 2 (runner had 1 free slot)
resume commands actually dispatched to the runner: 2
sess_a final state: running
sess_b final state: running
Race-detector clean across 5 runs; it's a logical race in the check-then-act sequence, not a Go memory race.
Two ways I can see to close it, and I don't want to guess which fits the rest of the fleet code better:
- run the cold-resume dispatch through the same
creating reservation the placement path already uses
- a per-runner lock around the check-dispatch-transition sequence, similar to what
nameLock does in internal/controld/runners.go for the connection-lifecycle writes, scoped to controlapp instead
Happy to send a PR either way once you've got a preference, or if there's a reservation mechanism already planned for controlapp that I'm not seeing.
Cold resume can overcommit a runner's capacity when two resumes race
SessionService.ResumeSession's cold-resume branch checkscoldResumeFree(capacity total minus used minus anything stillcreatingon the runner) and, if it sees a free slot, dispatches the resume and transitions the session straight fromsuspended_coldtorunning. It never passes throughcreating, so a concurrent resume reading the same runner in that window sees the same free slot and does the same thing.I hit this with two sessions pinned to one runner that has exactly one free slot: both
ResumeSessioncalls readcoldResumeFree() == 1, both dispatch, both land inrunning. The runner ends up hosting one more session than itsCapacityTotalallows.The normal placement path doesn't have this gap:
FleetService's scheduler transitions a session tocreating(an atomic CAS viaTransition, guarded by the session's current state) before it dispatches, sofreeCapacity'slen(creating)count reflects the claim for anyone reading after it. Cold resume skips that intermediate state entirely, so there's nothing for a second reader to see.I reproduced it with a unit test driving the real
SessionServiceagainst fakes for the two I/O ports it touches (FleetRepository,SessionRepository), with a barrier forcing both goroutines through the capacity read before either commits:Race-detector clean across 5 runs; it's a logical race in the check-then-act sequence, not a Go memory race.
Two ways I can see to close it, and I don't want to guess which fits the rest of the fleet code better:
creatingreservation the placement path already usesnameLockdoes ininternal/controld/runners.gofor the connection-lifecycle writes, scoped tocontrolappinsteadHappy to send a PR either way once you've got a preference, or if there's a reservation mechanism already planned for
controlappthat I'm not seeing.