Skip to content

Cold resume can overcommit a runner's capacity when two resumes race #30

Description

@AmirF194

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions