amadan

carlos / platform Public

activator: per-host gates serialise a host's group; the global lock guards only the map (Task 7b fix round 3)

- hostGate (mutex + owner) per host, looked up under strictMu, which never
  covers a start or filesystem work. Under the host's gate: the ownership
  check and an ordinary spawn's Cgroups.Start, a strict spawn taking
  ownership, the exit path's CleanupHost, a retry's directory removal, and
  S2's pending read. A stalled start blocks only its own host.
- The exit path removes nothing while any owner, live or pending, holds the
  group, and holds the gate from that check through the removal.
- pending is read and written only under the gate.
- With Strict nil, spawn and the exit path take no new lock and create no
  gate: main's path exactly.
- Tests (with -race and a per-backend hook): an older exit racing a new
  strict owner whose cleanup fails leaves the directory; a pending
  transition racing refusals; host A's stalled start does not block host
  B's spawns, S2 or exit cleanup; switched off, spawn and exit take no
  lock.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 81184b910fd17c73774ae2974f91a10f44978b24 parent b1c27d8
2 files changed, +363 −53
  • internal/activator/backend_exec.go +103 −45
  • internal/activator/backend_exec_test.go +260 −8
diff --git a/internal/activator/backend_exec.go b/internal/activator/backend_exec.go
index e8515f3f5..0db744e1c 100644
--- a/internal/activator/backend_exec.go
+++ b/internal/activator/backend_exec.go
@@ -177,13 +177,54 @@ type ExecBackend struct {
// preflight. Nil: Cgroups.StrictPreflight, the very function
// PrepareStrict runs, so S2 and the spawn cannot disagree.
strictPreflight func() error
- // strictOwners holds, per host, the strict handle that owns the host's
- // group: from the strict spawn until its cleanup succeeds, including
- // any retries. While a host has an owner, no other spawn of that host,
- // strict or ordinary, may use the group, and the exit path may not
- // remove it (moves design §5.3).
- strictMu sync.Mutex
- strictOwners map[string]*strictOwner
+ // strictGates holds one hostGate per host that has spawned with Strict
+ // set. strictMu guards only the map's lookup and insert, never a start
+ // or any filesystem work, so a stalled start blocks only its own host.
+ strictMu sync.Mutex
+ strictGates map[string]*hostGate
+ // strictHook, when set, is called at named points of the strict
+ // cleanup path. Test-only: it makes interleavings deterministic.
+ strictHook func(point string)
+}
+
+// hostGate serialises everything that touches one host's group once Strict
+// is wired (moves design §5.3): the ownership check and an ordinary start,
+// a strict spawn taking ownership, the exit path's directory removal, a
+// retry's removal, and S2's read. owner, and owner.pending, are read and
+// written only with mu held. While owner is set, from a strict spawn until
+// its cleanup succeeds (retries included), no other spawn of the host may
+// use the group and nothing but the owner may remove it.
+type hostGate struct {
+ mu sync.Mutex
+ owner *strictOwner
+}
+
+// gate returns host's gate, creating it.
+func (b *ExecBackend) gate(host string) *hostGate {
+ b.strictMu.Lock()
+ defer b.strictMu.Unlock()
+ if b.strictGates == nil {
+ b.strictGates = map[string]*hostGate{}
+ }
+ g := b.strictGates[host]
+ if g == nil {
+ g = &hostGate{}
+ b.strictGates[host] = g
+ }
+ return g
+}
+
+// lookupGate returns host's gate, or nil if it never had one.
+func (b *ExecBackend) lookupGate(host string) *hostGate {
+ b.strictMu.Lock()
+ defer b.strictMu.Unlock()
+ return b.strictGates[host]
+}
+
+func (b *ExecBackend) hook(point string) {
+ if b.strictHook != nil {
+ b.strictHook(point)
+ }
}
// ErrStrictGroupInUse is a spawn refused because a strict child of the same
@@ -274,22 +315,24 @@ func (b *ExecBackend) tier(rt registry.Route) cgroup.Limit {
// it drains and closes the group, for a plain one it does nothing.
//
// A strict handle owns the host's group from the spawn until its cleanup
-// succeeds (strictOwners). While it does, every other spawn of the host is
-// refused, an ordinary one included: the check and an ordinary spawn's
-// start happen under strictMu, so no strict group can be prepared for the
-// host in between. With Strict nil (a box with no moves wiring) none of
-// this runs: the spawn is main's exactly.
+// succeeds (hostGate.owner). While it does, every other spawn of the host
+// is refused, an ordinary one included: the check and an ordinary spawn's
+// start happen under the host's gate, so no strict group can be prepared
+// for the host in between. With Strict nil (a box with no moves wiring)
+// none of this runs and no lock is taken: the spawn is main's exactly.
func (b *ExecBackend) spawnInCgroup(rt registry.Route, build func() *exec.Cmd) (*exec.Cmd, func(), error) {
if b.Strict == nil {
cmd, err := b.Cgroups.Start(rt.Host, b.tier(rt), build)
return cmd, func() {}, err
}
strict := b.Strict(rt.Host)
- b.strictMu.Lock()
- if o := b.strictOwners[rt.Host]; o != nil {
- b.strictMu.Unlock()
+ g := b.gate(rt.Host)
+ g.mu.Lock()
+ if o := g.owner; o != nil {
+ pending := o.pending
+ g.mu.Unlock()
cause := ErrStrictGroupInUse
- if o.pending {
+ if pending {
cause = ErrStrictCleanupPending
}
if strict {
@@ -299,15 +342,12 @@ func (b *ExecBackend) spawnInCgroup(rt registry.Route, build func() *exec.Cmd) (
}
if !strict {
cmd, err := b.Cgroups.Start(rt.Host, b.tier(rt), build)
- b.strictMu.Unlock()
+ g.mu.Unlock()
return cmd, func() {}, err
}
o := &strictOwner{}
- if b.strictOwners == nil {
- b.strictOwners = map[string]*strictOwner{}
- }
- b.strictOwners[rt.Host] = o
- b.strictMu.Unlock()
+ g.owner = o
+ g.mu.Unlock()
sp, err := b.prepareStrictProc(rt.Host, b.tier(rt))
if err != nil {
b.disown(rt.Host, o, false)
@@ -337,10 +377,12 @@ func (b *ExecBackend) prepareStrictProc(host string, limit cgroup.Limit) (strict
return p, nil
}
-// strictOwner is the strict handle that owns a host's group. pending is
-// set once its first cleanup attempt failed and retries own the group.
-// drained records a KillAndEmpty that already succeeded: the handle refuses
-// a second one, so a retry after a failed Close only closes.
+// strictOwner is the strict handle that owns a host's group. pending
+// (guarded by the host gate's mu) is set once its first cleanup attempt
+// failed and retries own the group. drained records a KillAndEmpty that
+// already succeeded: the handle refuses a second one, so a retry after a
+// failed Close only closes. sp and drained are touched only by the one
+// goroutine running the owner's cleanup at a time.
type strictOwner struct {
sp strictProc
pending bool
@@ -348,7 +390,8 @@ type strictOwner struct {
}
// attempt is one cleanup attempt: drain (unless already drained), then
-// close.
+// close. It runs outside the host gate (a drain may take strictDrainTimeout);
+// the owner it belongs to keeps every other user of the group out meanwhile.
func (o *strictOwner) attempt(host string) error {
if !o.drained {
ctx, cancel := context.WithTimeout(context.Background(), strictDrainTimeout)
@@ -371,7 +414,7 @@ func (o *strictOwner) attempt(host string) error {
// success the owner lets go and the exit path's cleanupHost removes the
// group as before. On a failure the owner stays, marked pending, retried on
// strictCleanupBackoff: until a retry succeeds every spawn of the host is
-// refused and the exit path leaves the group directory alone, because the
+// refused and only the owner may remove the group directory, because the
// handle's identity is that directory and removing it would strand the
// handle for ever.
func (b *ExecBackend) settleStrict(host string, o *strictOwner) {
@@ -379,34 +422,46 @@ func (b *ExecBackend) settleStrict(host string, o *strictOwner) {
b.disown(host, o, false)
return
}
- b.strictMu.Lock()
+ b.hook("cleanup-failed")
+ g := b.gate(host)
+ g.mu.Lock()
o.pending = true
- b.strictMu.Unlock()
+ g.mu.Unlock()
go b.retryStrictCleanup(host, o, append([]time.Duration(nil), strictCleanupBackoff...))
}
// disown ends o's ownership of host's group. removeGroup removes the group
-// directory first, under the same lock, so no spawn can recreate it in
+// directory first, under the host gate, so no spawn can recreate it in
// between: the retry owner's last act.
func (b *ExecBackend) disown(host string, o *strictOwner, removeGroup bool) {
- b.strictMu.Lock()
- defer b.strictMu.Unlock()
- if b.strictOwners[host] != o {
+ g := b.gate(host)
+ g.mu.Lock()
+ defer g.mu.Unlock()
+ if g.owner != o {
return
}
if removeGroup {
b.Cgroups.CleanupHost(host)
}
- delete(b.strictOwners, host)
+ g.owner = nil
}
-// cleanupHost is the exit path's CleanupHost, skipped while a strict
-// handle's cleanup is pending: the retry owner removes the group once the
-// handle is closed.
+// cleanupHost is the exit path's CleanupHost. With Strict nil it is main's
+// call, with no lock. Otherwise it runs under the host gate and removes
+// nothing while any strict owner, live or pending, holds the group: the
+// owner removes it once its handle is closed.
func (b *ExecBackend) cleanupHost(host string) {
- if b.strictCleanupPending(host) {
+ if b.Strict == nil {
+ b.Cgroups.CleanupHost(host)
+ return
+ }
+ g := b.gate(host)
+ g.mu.Lock()
+ defer g.mu.Unlock()
+ if g.owner != nil {
return
}
+ b.hook("exit-cleanup-checked")
b.Cgroups.CleanupHost(host)
}
@@ -425,13 +480,16 @@ func (b *ExecBackend) retryStrictCleanup(host string, o *strictOwner, schedule [
log.Printf("activator: %s: strict group still not cleaned up after %d retries; spawns of this host stay refused until the agent restarts", host, len(schedule))
}
-// strictCleanupPending reports whether host's group is owned by a strict
-// handle whose cleanup is being retried.
+// strictCleanupPending reports, under the host gate, whether host's group
+// is owned by a strict handle whose cleanup is being retried.
func (b *ExecBackend) strictCleanupPending(host string) bool {
- b.strictMu.Lock()
- defer b.strictMu.Unlock()
- o := b.strictOwners[host]
- return o != nil && o.pending
+ g := b.lookupGate(host)
+ if g == nil {
+ return false
+ }
+ g.mu.Lock()
+ defer g.mu.Unlock()
+ return g.owner != nil && g.owner.pending
}
// NextSpawnStrict is the source's S2 input (moves design §7.3, §5.3): will
diff --git a/internal/activator/backend_exec_test.go b/internal/activator/backend_exec_test.go
index 51b3d0f2c..1b2af1451 100644
--- a/internal/activator/backend_exec_test.go
+++ b/internal/activator/backend_exec_test.go
@@ -761,14 +761,11 @@ func TestStrictCleanupGivesUpButStaysRefused(t *testing.T) {
// ownStrictForTest registers sp as host's strict owner, as a strict spawn
// does, so a test can drive settleStrict directly.
func (b *ExecBackend) ownStrictForTest(host string, sp strictProc) *strictOwner {
- b.strictMu.Lock()
- defer b.strictMu.Unlock()
- if b.strictOwners == nil {
- b.strictOwners = map[string]*strictOwner{}
- }
- o := &strictOwner{sp: sp}
- b.strictOwners[host] = o
- return o
+ g := b.gate(host)
+ g.mu.Lock()
+ defer g.mu.Unlock()
+ g.owner = &strictOwner{sp: sp}
+ return g.owner
}
// dirStrict is a strict handle whose identity is a real directory: the
@@ -978,3 +975,258 @@ func TestALiveStrictGroupRefusesOrdinarySpawns(t *testing.T) {
settle2()
}
}
+
+// hookAt returns a strictHook that calls fn at point and does nothing at
+// any other point.
+func hookAt(point string, fn func()) func(string) {
+ return func(p string) {
+ if p == point {
+ fn()
+ }
+ }
+}
+
+// TestAnOlderExitCannotRemoveANewOwnersGroup: the exit path's removal and
+// a strict spawn's ownership are decided under the same host gate. An exit
+// that has checked "no owner" holds the gate through its removal, so a
+// strict spawn arriving meanwhile waits, then owns a group it created
+// itself; an exit that runs once any owner exists, live or pending, removes
+// nothing. A strict child whose cleanup then fails keeps its directory.
+func TestAnOlderExitCannotRemoveANewOwnersGroup(t *testing.T) {
+ oldBackoff := strictCleanupBackoff
+ strictCleanupBackoff = []time.Duration{time.Hour}
+ t.Cleanup(func() { strictCleanupBackoff = oldBackoff })
+ host := "race.example.com"
+ b := testExecBackend(t)
+ b.Cgroups = delegatedManager(t)
+ groupDir := filepath.Join(b.Cgroups.Root(), cgroup.ExecName(host))
+ b.Strict = func(string) bool { return true }
+ var handle *dirStrict
+ b.prepareStrict = func(string, cgroup.Limit) (strictProc, error) {
+ handle = newDirStrict(t, groupDir)
+ handle.killFails = 100
+ return handle, nil
+ }
+ if err := os.MkdirAll(groupDir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+
+ // The older exit has found no owner and is about to remove the group.
+ // A strict spawn starts right then and must wait for the gate.
+ spawned := make(chan error, 1)
+ var settle func()
+ var cmd *exec.Cmd
+ b.strictHook = hookAt("exit-cleanup-checked", func() {
+ go func() {
+ var err error
+ cmd, settle, err = b.spawnInCgroup(registry.Route{Host: host}, func() *exec.Cmd { return exec.Command("/bin/true") })
+ spawned <- err
+ }()
+ select {
+ case err := <-spawned:
+ t.Errorf("a strict spawn took the host while an exit held its gate: %v", err)
+ spawned <- err
+ case <-time.After(150 * time.Millisecond):
+ }
+ })
+ b.cleanupHost(host)
+ b.strictHook = nil
+ if err := <-spawned; err != nil {
+ t.Fatalf("strict spawn after the exit: %v", err)
+ }
+ if _, err := os.Stat(groupDir); err != nil {
+ t.Fatalf("the new owner has no group directory: %v", err)
+ }
+ // Any owner, live as here, keeps the directory from an exit.
+ b.cleanupHost(host)
+ if _, err := os.Stat(groupDir); err != nil {
+ t.Fatalf("an exit removed a live owner's group: %v", err)
+ }
+ _ = cmd.Wait()
+ settle() // the drain fails: the owner turns pending
+ if !b.strictCleanupPending(host) {
+ t.Fatal("cleanup is not pending after a failed drain")
+ }
+ b.cleanupHost(host)
+ if _, err := os.Stat(groupDir); err != nil {
+ t.Fatalf("an exit removed a pending owner's group: %v", err)
+ }
+ if !handle.identityOK() {
+ t.Fatal("the retained handle lost its group")
+ }
+}
+
+// TestAPendingTransitionRacesARefusal: a spawn refused while the owner is
+// between its failed cleanup attempt and turning pending sees a live owner
+// (ErrStrictGroupInUse); once pending, ErrStrictCleanupPending. pending is
+// only ever read and written under the host gate, so -race stays quiet
+// however the two interleave.
+func TestAPendingTransitionRacesARefusal(t *testing.T) {
+ oldBackoff := strictCleanupBackoff
+ strictCleanupBackoff = []time.Duration{time.Hour}
+ t.Cleanup(func() { strictCleanupBackoff = oldBackoff })
+ host := "transition.example.com"
+ b := testExecBackend(t)
+ b.Strict = func(string) bool { return false }
+ o := b.ownStrictForTest(host, &flakyStrict{recordingStrict: newRecordingStrict(), killFails: 100})
+ spawn := func() error {
+ _, _, err := b.spawnInCgroup(registry.Route{Host: host}, func() *exec.Cmd { return exec.Command("/bin/true") })
+ return err
+ }
+ var during error
+ b.strictHook = hookAt("cleanup-failed", func() { during = spawn() })
+ b.settleStrict(host, o)
+ b.strictHook = nil
+ if !errors.Is(during, ErrStrictGroupInUse) {
+ t.Fatalf("refusal before the owner turned pending: %v, want ErrStrictGroupInUse", during)
+ }
+ if err := spawn(); !errors.Is(err, ErrStrictCleanupPending) {
+ t.Fatalf("refusal after: %v, want ErrStrictCleanupPending", err)
+ }
+
+ // The same transition against concurrent refusals, for -race.
+ host2 := "transition2.example.com"
+ o2 := b.ownStrictForTest(host2, &flakyStrict{recordingStrict: newRecordingStrict(), killFails: 100})
+ var wg sync.WaitGroup
+ stop := make(chan struct{})
+ var looping sync.WaitGroup
+ for i := 0; i < 8; i++ {
+ wg.Add(1)
+ looping.Add(1)
+ go func() {
+ defer wg.Done()
+ first := true
+ for {
+ _, _, err := b.spawnInCgroup(registry.Route{Host: host2}, func() *exec.Cmd { return exec.Command("/bin/true") })
+ if !errors.Is(err, ErrStrictGroupInUse) && !errors.Is(err, ErrStrictCleanupPending) {
+ t.Errorf("spawn beside an owner: %v", err)
+ }
+ _ = b.NextSpawnStrict(host2)
+ if first {
+ looping.Done()
+ first = false
+ }
+ select {
+ case <-stop:
+ return
+ default:
+ }
+ }
+ }()
+ }
+ looping.Wait()
+ b.settleStrict(host2, o2) // turns pending while the refusals run
+ time.Sleep(20 * time.Millisecond)
+ close(stop)
+ wg.Wait()
+}
+
+// TestAStalledStartBlocksOnlyItsHost: host A's ordinary start stalls inside
+// Cgroups.Start, holding A's gate. Host B's ordinary and strict spawns and
+// its NextSpawnStrict proceed; the global map lock is never held across a
+// start.
+func TestAStalledStartBlocksOnlyItsHost(t *testing.T) {
+ b := testExecBackend(t)
+ b.Cgroups = delegatedManager(t)
+ b.strictPreflight = func() error { return nil }
+ b.Strict = func(h string) bool { return h == "b-strict.example.com" }
+ b.prepareStrict = func(string, cgroup.Limit) (strictProc, error) { return newRecordingStrict(), nil }
+ release := make(chan struct{})
+ entered := make(chan struct{})
+ var once sync.Once
+ doneA := make(chan error, 1)
+ go func() {
+ cmd, settle, err := b.spawnInCgroup(registry.Route{Host: "a.example.com"}, func() *exec.Cmd {
+ once.Do(func() { close(entered); <-release })
+ return exec.Command("/bin/true")
+ })
+ if err == nil {
+ _ = cmd.Wait()
+ settle()
+ }
+ doneA <- err
+ }()
+ <-entered
+ within := func(what string, fn func()) {
+ t.Helper()
+ done := make(chan struct{})
+ go func() { fn(); close(done) }()
+ select {
+ case <-done:
+ case <-time.After(2 * time.Second):
+ t.Fatalf("%s was blocked by host A's stalled start", what)
+ }
+ }
+ within("host B's ordinary spawn", func() {
+ cmd, settle, err := b.spawnInCgroup(registry.Route{Host: "b.example.com"}, func() *exec.Cmd { return exec.Command("/bin/true") })
+ if err != nil {
+ t.Errorf("host B: %v", err)
+ return
+ }
+ _ = cmd.Wait()
+ settle()
+ })
+ within("host B's strict spawn", func() {
+ cmd, settle, err := b.spawnInCgroup(registry.Route{Host: "b-strict.example.com"}, func() *exec.Cmd { return exec.Command("/bin/true") })
+ if err != nil {
+ t.Errorf("host B strict: %v", err)
+ return
+ }
+ _ = cmd.Wait()
+ settle()
+ })
+ within("host B's NextSpawnStrict", func() {
+ if c := b.NextSpawnStrict("b-strict.example.com"); !c.OK {
+ t.Errorf("S2 for host B: %+v", c)
+ }
+ })
+ within("host B's exit cleanup", func() { b.cleanupHost("b.example.com") })
+ close(release)
+ if err := <-doneA; err != nil {
+ t.Fatalf("host A: %v", err)
+ }
+}
+
+// TestSwitchedOffSpawnsTakeNoNewLock: with Strict nil, a spawn and its exit
+// path are main's: no host gate is created and the global map lock is
+// never taken (held by the test throughout, a spawn that wanted it would
+// hang).
+func TestSwitchedOffSpawnsTakeNoNewLock(t *testing.T) {
+ old := spawnHealthTimeout
+ spawnHealthTimeout = 300 * time.Millisecond
+ t.Cleanup(func() { spawnHealthTimeout = old })
+ b := testExecBackend(t)
+ b.Cgroups = delegatedManager(t)
+ host := "off.example.com"
+ argvPath, _ := stageRecordingLive(t, b, host, "v1")
+ b.strictMu.Lock()
+ defer b.strictMu.Unlock()
+ done := make(chan struct{})
+ go func() {
+ defer close(done)
+ rt := registry.Route{Host: host, Kind: "instance", Socket: filepath.Join(t.TempDir(), "i.sock")}
+ ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+ defer cancel()
+ proc, _, _ := b.Start(ctx, rt)
+ if proc != nil {
+ _ = proc.Stop(context.Background())
+ }
+ }()
+ select {
+ case <-done:
+ case <-time.After(5 * time.Second):
+ t.Fatal("a switched-off spawn waited on the strict map lock")
+ }
+ waitForFile(t, argvPath, time.Second)
+ // The exit path's call, again, with the lock held.
+ cleaned := make(chan struct{})
+ go func() { b.cleanupHost(host); close(cleaned) }()
+ select {
+ case <-cleaned:
+ case <-time.After(2 * time.Second):
+ t.Fatal("a switched-off exit cleanup waited on the strict map lock")
+ }
+ if b.strictGates != nil {
+ t.Fatalf("a switched-off spawn created host gates: %v", b.strictGates)
+ }
+}