amadan

carlos / platform Public

zdd plan 3 final fix: parent-limit repair obligations survive a partial raise and a failed restore (F11)

The overlap marks the parent potentially changed (ops.raised) before it
asks RaiseGenParent, so a raise that wrote memory.max and then failed a
later write is restored by the member's cleanup. A raise refused before
any write (errNoGenContainment, cgroup.ErrNoDelegation) owes nothing.
leaveSet keeps raised when RestoreGenParent fails; the member still
leaves (its termination is proven) and finishPendingStops retries the
restore off a.mu (retryParentRestoreLocked) until it lands. While a
restore is owed with no member, a new overlap is refused as busy.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 1f5de209b49f774b3aa933514b5bb030e3abda97 parent fc14e7f
5 files changed, +195 −20
  • internal/activator/activator.go +3 −0
  • internal/activator/opset.go +68 −12
  • internal/activator/swap.go +19 −5
  • internal/activator/swap_fixture_test.go +12 −3
  • internal/activator/swap_parent_repair_test.go +93 −0
diff --git a/internal/activator/activator.go b/internal/activator/activator.go
index 590d3d96f..8125f6d4e 100644
--- a/internal/activator/activator.go
+++ b/internal/activator/activator.go
@@ -2938,6 +2938,9 @@ func (a *Service) finishPendingStops() {
// Members first (zero-downtime spec §7.4): a generation whose stop
// could not be proven is finished here once it exits.
a.reapMembersLocked(e)
+ // A parent restore that failed when the last member left keeps its
+ // obligation and is retried here (final review F11).
+ a.retryParentRestoreLocked(e)
if !e.pendingStop || len(e.ops.members) > 0 {
continue
}
diff --git a/internal/activator/opset.go b/internal/activator/opset.go
index 3688b2ac5..f1944be8b 100644
--- a/internal/activator/opset.go
+++ b/internal/activator/opset.go
@@ -12,6 +12,7 @@ import (
"log"
"time"
+ "github.com/carlosframework/platform/internal/cgroup"
"github.com/carlosframework/platform/internal/instancectl"
"github.com/carlosframework/platform/internal/registry"
)
@@ -62,11 +63,17 @@ type straySidecar struct {
}
// opSet is an entry's operation set. raised records that the parent's
-// limit is at twice the tier (spec §7.3), so the last member to leave
-// restores it.
+// limit may be above the tier (spec §7.3): it is set before a raise is
+// asked, since a raise that failed partway may still have changed the
+// parent, and cleared only by a restore that succeeded. The last member to
+// leave restores it; a restore that fails keeps raised, and the sweep
+// retries it (retryParentRestoreLocked, final review F11). While raised is
+// set with no member, no overlap is admitted. restoring marks a retry in
+// flight.
type opSet struct {
- members map[uint64]*genMember
- raised bool
+ members map[uint64]*genMember
+ raised bool
+ restoring bool
}
// opJoinBound bounds the barrier's wait. A member's owner needs at most a
@@ -281,22 +288,21 @@ func (a *Service) dropMember(e *actEntry, m *genMember) bool {
// retiring child is empty). The restore is to the CURRENT generation's
// captured tier (spec §7.3): the incumbent's after a failed successor, the
// successor's after a switch. done closes last.
+//
+// A restore that fails keeps raised (final review F11): the member still
+// leaves, since its termination is proven, but the obligation stays on the
+// entry and the sweep retries it (retryParentRestoreLocked).
func (a *Service) leaveSet(e *actEntry, m *genMember) {
a.mu.Lock()
restore := e.ops.raised && len(e.ops.members) == 1 && e.ops.members[m.gen.N] == m
rt, tier := e.route, e.gen.Tier
a.mu.Unlock()
+ restored := false
if restore {
- if gb, ok := a.backend.(SwapBackend); ok {
- rctx, cancel := context.WithTimeout(context.Background(), instancectl.DeadlineControl)
- if err := gb.RestoreGenParent(rctx, rt, tier); err != nil {
- log.Printf("activator: restore parent limit for %s: %v", rt.Host, err)
- }
- cancel()
- }
+ restored = a.restoreParent(rt, tier)
}
a.mu.Lock()
- if restore {
+ if restored {
e.ops.raised = false
}
if e.ops.members[m.gen.N] == m {
@@ -306,6 +312,56 @@ func (a *Service) leaveSet(e *actEntry, m *genMember) {
close(m.done)
}
+// restoreParent puts rt's parent back at tier, and reports whether it did.
+// A backend without the swap seam has no parent to restore. Never call it
+// with a.mu held.
+func (a *Service) restoreParent(rt registry.Route, tier cgroup.Limit) bool {
+ gb, ok := a.backend.(SwapBackend)
+ if !ok {
+ return true
+ }
+ rctx, cancel := context.WithTimeout(context.Background(), instancectl.DeadlineControl)
+ defer cancel()
+ if err := gb.RestoreGenParent(rctx, rt, tier); err != nil && !parentUntouched(err) {
+ log.Printf("activator: restore parent limit for %s: %v (kept; the sweep retries)", rt.Host, err)
+ return false
+ }
+ return true
+}
+
+// parentUntouched reports whether a parent-limit error says no parent
+// limit is in this box's hands at all: no generation containment
+// (errNoGenContainment) or no delegated cgroup manager. Both are refused
+// before any write, and both are fixed for the agent's life, so a raise
+// they refuse changed nothing and a restore they refuse has nothing to
+// put back.
+func parentUntouched(err error) bool {
+ return errors.Is(err, errNoGenContainment) || errors.Is(err, cgroup.ErrNoDelegation)
+}
+
+// retryParentRestoreLocked retries a parent restore that failed when the
+// last member left (final review F11). It runs only while the set is empty
+// and no retry is in flight; the restore itself runs off a.mu. An entry
+// that is gone takes its obligation with it: the next cold spawn under an
+// empty parent writes the tier afresh (cgroup.ensureParent). The caller
+// holds a.mu.
+func (a *Service) retryParentRestoreLocked(e *actEntry) {
+ if !e.ops.raised || e.ops.restoring || len(e.ops.members) > 0 {
+ return
+ }
+ e.ops.restoring = true
+ rt, tier := e.route, e.gen.Tier
+ go func() {
+ ok := a.restoreParent(rt, tier)
+ a.mu.Lock()
+ e.ops.restoring = false
+ if ok && len(e.ops.members) == 0 {
+ e.ops.raised = false
+ }
+ a.mu.Unlock()
+ }()
+}
+
// reapMembersLocked finishes members whose owner could not prove their stop.
// Once a member's process has exited (or it never had one), the reaper
// proves its generation child empty (reapExited) and only then takes it out
diff --git a/internal/activator/swap.go b/internal/activator/swap.go
index bee8dd5b0..66c1de21a 100644
--- a/internal/activator/swap.go
+++ b/internal/activator/swap.go
@@ -375,7 +375,11 @@ func (a *Service) swapOverlap(ctx context.Context, gb SwapBackend, at Attempt, n
return errSwapLayout
}
}
- if len(e.ops.members) > 0 {
+ // A parent still marked raised with no member is a restore that
+ // failed and awaits the sweep's retry (final review F11): an overlap
+ // raising it again now could have that retry lower it under the
+ // successor.
+ if len(e.ops.members) > 0 || e.ops.raised {
return errSwapBusy
}
a.addMemberLocked(e, m)
@@ -407,13 +411,23 @@ func (a *Service) swapOverlap(ctx context.Context, gb SwapBackend, at Attempt, n
a.mu.Unlock()
// Step 2: the parent raise, before the successor spawns (spec §7.3).
- if err := gb.RaiseGenParent(ctx, rt, incumbent.Tier, succTier); err != nil {
- a.dropMember(e, m)
- return abandon(fmt.Errorf("raise parent limit: %w", err))
- }
+ // The parent is marked potentially changed BEFORE the raise is asked: a
+ // raise that wrote memory.max and then failed a later write has changed
+ // it, and the member's cleanup (leaveSet) must restore it (final review
+ // F11).
a.mu.Lock()
e.ops.raised = true
a.mu.Unlock()
+ if err := gb.RaiseGenParent(ctx, rt, incumbent.Tier, succTier); err != nil {
+ if parentUntouched(err) {
+ // Refused before any write: there is nothing to restore.
+ a.mu.Lock()
+ e.ops.raised = false
+ a.mu.Unlock()
+ }
+ a.dropMember(e, m) // restores a possibly changed parent (leaveSet)
+ return abandon(fmt.Errorf("raise parent limit: %w", err))
+ }
// Step 3a: the §7.4 checks again, at the exec (spec §6.3, §7.4). The
// raise (and plan 5's pin publication, just above this step) took time:
diff --git a/internal/activator/swap_fixture_test.go b/internal/activator/swap_fixture_test.go
index 114884bd3..228562d7c 100644
--- a/internal/activator/swap_fixture_test.go
+++ b/internal/activator/swap_fixture_test.go
@@ -62,6 +62,11 @@ type fakeSwapBackend struct {
// genStuck makes every generation spawned from then on a stuck
// fakeProc: its Stop answers ErrStillRunning until exit().
genStuck bool
+ // raiseErr is what RaiseGenParent answers after recording the call: a
+ // raise whose first write landed and a later one failed.
+ raiseErr error
+ // restoreFails is how many more RestoreGenParent calls fail.
+ restoreFails int
}
func (g *fakeSwapBackend) StartStaged(ctx context.Context, _ registry.Route, gen Gen) (Gen, Proc, error) {
@@ -122,19 +127,23 @@ func (g *fakeSwapBackend) RaiseGenParent(_ context.Context, _ registry.Route, in
g.record(fmt.Sprintf("raise:%d+%d", inc.MemoryMax, succ.MemoryMax))
g.gmu.Lock()
g.raises++
- hook := g.onRaise
+ hook, err := g.onRaise, g.raiseErr
g.gmu.Unlock()
if hook != nil {
hook()
}
- return nil
+ return err
}
func (g *fakeSwapBackend) RestoreGenParent(_ context.Context, _ registry.Route, tier cgroup.Limit) error {
g.record(fmt.Sprintf("restore:%d", tier.MemoryMax))
g.gmu.Lock()
+ defer g.gmu.Unlock()
g.restores++
- g.gmu.Unlock()
+ if g.restoreFails > 0 {
+ g.restoreFails--
+ return errors.New("cgroup: parent memory.max: device or resource busy")
+ }
return nil
}
diff --git a/internal/activator/swap_parent_repair_test.go b/internal/activator/swap_parent_repair_test.go
new file mode 100644
index 000000000..c1443aefa
--- /dev/null
+++ b/internal/activator/swap_parent_repair_test.go
@@ -0,0 +1,93 @@
+package activator
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/carlosframework/platform/internal/cgroup"
+)
+
+func (a *Service) parentRaisedForTest(host string) bool {
+ a.mu.Lock()
+ defer a.mu.Unlock()
+ e := a.entries[host]
+ return e != nil && e.ops.raised
+}
+
+// Final review F11(a): a raise that failed partway (memory.max written, a
+// later write refused) may have changed the parent, so the overlap's
+// cleanup restores it. The parent is marked potentially changed before the
+// raise is asked.
+func TestSwapOverlapRestoresAPartlyRaisedParent(t *testing.T) {
+ f := newSwapFixture(t)
+ f.b.gmu.Lock()
+ f.b.raiseErr = errors.New("cgroup: parent memory.swap.max: invalid argument")
+ f.b.gmu.Unlock()
+ res := f.a.Swap(context.Background(), f.attempt(ModeOverlap), f.newest)
+ if res.Outcome != SwapAbandoned || !strings.Contains(res.Reason, "raise parent limit") {
+ t.Fatalf("Swap = %+v, want abandoned at the raise", res)
+ }
+ gens, raises, restores := f.b.calls()
+ if gens != 0 || raises != 1 || restores != 1 {
+ t.Fatalf("StartStaged %d, raises %d, restores %d: a partly raised parent must be restored", gens, raises, restores)
+ }
+ if f.a.parentRaisedForTest(f.host) {
+ t.Fatal("the restored parent is still marked raised")
+ }
+}
+
+// Final review F11(b): a restore that fails after the last member left
+// keeps its obligation. The parent stays marked raised, no new overlap is
+// admitted onto it (it would raise again under a pending restore), and
+// the sweep retries the restore, outside a.mu, until it lands.
+func TestSwapOverlapRetriesAFailedParentRestore(t *testing.T) {
+ f := newSwapFixture(t)
+ f.b.gmu.Lock()
+ f.b.restoreFails = 1
+ f.b.gmu.Unlock()
+ if res := f.a.Swap(context.Background(), f.attempt(ModeOverlap), f.newest); res.Outcome != SwapDone {
+ t.Fatalf("Swap = %+v", res)
+ }
+ if _, _, restores := f.b.calls(); restores != 1 {
+ t.Fatalf("restores = %d, want the one that failed", restores)
+ }
+ if !f.a.parentRaisedForTest(f.host) {
+ t.Fatal("a failed restore dropped its obligation: the parent is no longer marked raised")
+ }
+ if _, members := f.a.genStateForTest(f.host); len(members) != 0 {
+ t.Fatalf("members = %v: the retired generation is proven, it leaves the set", members)
+ }
+ f.rev.Add(1)
+ if res := f.a.Swap(context.Background(), f.attempt(ModeOverlap), f.newest); res.Outcome != SwapAbandoned || !strings.Contains(res.Reason, errSwapBusy.Error()) {
+ t.Fatalf("Swap over a pending restore = %+v, want abandoned: busy", res)
+ }
+ f.a.sweepOnce()
+ waitFor(t, "the restore retry", func() bool { return !f.a.parentRaisedForTest(f.host) })
+ if _, raises, restores := f.b.calls(); raises != 1 || restores != 2 {
+ t.Fatalf("raises %d, restores %d: want one raise and the retried restore", raises, restores)
+ }
+}
+
+// A raise refused before any write (no generation containment, no
+// delegated manager) changed nothing: no restore is owed, and the next
+// overlap is not held busy by an obligation that could never clear.
+func TestSwapOverlapOwesNoRestoreForARaiseRefusedBeforeAnyWrite(t *testing.T) {
+ for _, refusal := range []error{errNoGenContainment, cgroup.ErrNoDelegation} {
+ f := newSwapFixture(t)
+ f.b.gmu.Lock()
+ f.b.raiseErr = fmt.Errorf("parent: %w", refusal)
+ f.b.gmu.Unlock()
+ if res := f.a.Swap(context.Background(), f.attempt(ModeOverlap), f.newest); res.Outcome != SwapAbandoned {
+ t.Fatalf("%v: Swap = %+v, want abandoned", refusal, res)
+ }
+ if _, raises, restores := f.b.calls(); raises != 1 || restores != 0 {
+ t.Fatalf("%v: raises %d, restores %d: want no restore", refusal, raises, restores)
+ }
+ if f.a.parentRaisedForTest(f.host) {
+ t.Fatalf("%v: an untouched parent is marked raised", refusal)
+ }
+ }
+}