| index 995290fc0..f55457041 100644 |
| --- a/internal/rollout/sim_test.go |
| +++ b/internal/rollout/sim_test.go |
| @@ -9,6 +9,7 @@ import ( |
| "math/rand" |
| "os" |
| "path/filepath" |
| + "runtime" |
| "runtime/debug" |
| "slices" |
| "sort" |
| @@ -100,14 +101,17 @@ import ( |
| // - I9: a stale grant never clears a later failure (the crash-epoch hard |
| // limit, readiness round 2): when a signed `carlos instances retry` |
| // grant issued at step t clears anything (RetryHostAt), no unanswered |
| -// failure fact of the grant's (build, host) has a start after t. The |
| -// facts are the model's (sim.facts), never the book's verdicts: every |
| -// failure, hold-down and hold run the model delivers that changes the |
| -// model's own restriction state (sim.grec, the hold-ran restriction), |
| -// and every intent a restart reconciles. A fact is answered by a clear |
| -// the spec allows to lift it, issued after its start: a grant for its |
| -// (build, host), a pointer naming its build (a record), or a lift of |
| -// its host's hold-ran restriction (a hold run). |
| +// failure fact of the grant's (build, host) has a start after t (a fact |
| +// that began in t's own step is skipped). The facts are the model's |
| +// (sim.facts), never the book's verdicts: every failure, hold-down and |
| +// hold run the model delivers that changes the model's own restriction |
| +// state (sim.grec, the hold-ran restriction), and every intent a |
| +// restart reconciles. A failure that makes a new record is a fact of |
| +// every host of the app, wherever it ran, since a new record holds |
| +// every host (spec §8.4). A fact is answered by a clear the spec allows |
| +// to lift it, issued after its start: a grant for its build on the |
| +// grant's host, a pointer naming its build (a record), or a lift of its |
| +// host's hold-ran restriction (a hold run). |
| // - I10: durable before use: every non-zero generation PublishedEpoch |
| // returns is the app's generation in the book's last landed file, |
| // re-read from disk at that moment, under the identity the file holds. |
| @@ -119,7 +123,8 @@ import ( |
| // re-reads its pending grants; agent restarts, which drop every execution |
| // in flight and reopen the book from disk under a new Manager; box |
| // rebuilds, which remove the book's file and open a new book; and a Retry |
| -// on an instance its hold-ran marker holds, followed by a wake stopped |
| +// on an instance its hold-ran marker holds, followed by a wake, of that |
| +// instance or of another that never served the marked build, stopped |
| // before it settles (rewakeArm). The grant queue and the facts outlive |
| // restarts and rebuilds. |
| // |
| @@ -162,6 +167,34 @@ func TestSimInvariants(t *testing.T) { |
| |
| const simSteps = 400 |
| |
| +// TestSimSeedsEndTheirGoroutines: an agent restart drops the attempts parked |
| +// in Swap (agentRestart), and the seed's teardown (runSimSeed) must end their |
| +// goroutines. A goroutine left parked holds its sim, so a 3000-seed sweep |
| +// would keep every seed with a restart for the life of the process. |
| +func TestSimSeedsEndTheirGoroutines(t *testing.T) { |
| + if os.Geteuid() == 0 { |
| + t.Skip("as root no seed keeps its book on disk, so no seed restarts its agent") |
| + } |
| + opt, books := t.TempDir(), t.TempDir() |
| + staged := map[string]bool{} |
| + before := runtime.NumGoroutine() |
| + // The seeds whose book is on disk (seed%4 == 3) are the ones that restart. |
| + for seed := int64(3); seed < 16; seed += 4 { |
| + if v := runSimSeed(seed, opt, books, staged, simSteps); v != nil { |
| + t.Fatalf("rolloutsim: %s", v.line) |
| + } |
| + } |
| + // An ended Manager's own goroutines leave on their cancelled context, a |
| + // moment after it. |
| + deadline := time.Now().Add(5 * time.Second) |
| + for runtime.NumGoroutine() > before && time.Now().Before(deadline) { |
| + time.Sleep(10 * time.Millisecond) |
| + } |
| + if after := runtime.NumGoroutine(); after > before { |
| + t.Fatalf("%d goroutines before the seeds, %d after them", before, after) |
| + } |
| +} |
| + |
| // The activator's first-wake window, as spawnPlanned reads it: B's health |
| // window ends firstWakeBudget less firstWakeKillReserve after the wake's |
| // start, and B is exec'd only with firstWakeMinVerdict of it left. |
| @@ -193,6 +226,13 @@ func runSimSeed(seed int64, opt, books string, staged map[string]bool, steps int |
| if s != nil && s.cancel != nil { |
| s.cancel() |
| } |
| + if s != nil { |
| + // Ends every attempt still parked in Swap (one an agent restart |
| + // dropped, or one a violation left), without returning from Swap, |
| + // so none of them touches the book or the model, and nothing holds |
| + // the sim after the seed. |
| + close(s.dead) |
| + } |
| if s != nil && s.dir != "" { |
| // Writable again, so the seed's book can go (and t.TempDir's |
| // own cleanup never meets a read-only directory). |
| @@ -306,11 +346,14 @@ type simGrant struct { |
| |
| // simFact is one of I9's failure facts: something the model delivered that |
| // restricts build on host (kind says what), by an execution that began at |
| -// start. |
| +// start. app is a new record, made by a failure on host: it holds every host |
| +// of the app (spec §8.4: "the build's next failure writes a new record, |
| +// which holds every host"), so the fact is every host's, not host's alone. |
| type simFact struct { |
| build, host, kind string |
| start time.Time |
| marker bool // a hold run: its host's hold-ran restriction |
| + app bool // a new record: every host of the app |
| } |
| |
| // simRec is the model's own failure record of a build (sim.grec): the hosts |
| @@ -419,12 +462,17 @@ type sim struct { |
| // retry clears and which are held down, so a failure is a fact only when |
| // it changes what the spec says restricts its host (plan finding 7: an |
| // unchanged record or a pre-retry outcome changes nothing). Kept from the |
| - // model's events; a record leaving the book (a pass) and a restart's |
| - // reread of the disk are the only things it takes from the book. |
| + // model's events. It takes three things from the book: a record leaving |
| + // it (a pass), a restart's reread of the disk, and a grant's clear, where |
| + // deliver marks the host retried once the book's Lookup shows the record |
| + // now lists it (a clear I9 has vetted first). |
| grec map[string]*simRec |
| - // rewake is the host rewakeArm granted a retry to, which rewakeRun wakes |
| - // and stops at the next step. |
| + // rewake is the host rewakeRun wakes and stops at the next step: the one |
| + // rewakeArm granted a retry to, or another on its channel. |
| rewake string |
| + // rewakeFail is the build whose first wake on rewake fails its health |
| + // check (another instance's wake, M17's witness), or "". |
| + rewakeFail string |
| |
| // late holds an ordinary wake's healthy event (GenHealthy, not a first |
| // wake) that the activator has not yet delivered, per host: its |
| @@ -437,6 +485,9 @@ type sim struct { |
| swaps map[string]*simSwap |
| events chan simEvent |
| draining bool |
| + // dead is closed when the seed ends (runSimSeed): an attempt goroutine |
| + // still parked in Swap, or about to report on events, exits instead. |
| + dead chan struct{} |
| } |
| |
| func newSim(seed int64, opt, books string, staged map[string]bool) *sim { |
| @@ -449,7 +500,7 @@ func newSim(seed int64, opt, books string, staged map[string]bool) *sim { |
| holdRanAt: map[string]time.Time{}, holdRanSet: map[string]time.Time{}, retryAt: map[string]map[string]time.Time{}, |
| clearedAt: map[string]time.Time{}, failedAt: map[string]time.Time{}, late: map[string]simLate{}, swaps: map[string]*simSwap{}, |
| issued: map[[2]string][]time.Time{}, signed: map[string][]time.Time{}, lifted: map[string][]time.Time{}, grec: map[string]*simRec{}, |
| - events: make(chan simEvent)} |
| + events: make(chan simEvent), dead: make(chan struct{})} |
| if seed%4 == 3 && os.Geteuid() != 0 { |
| // Root writes into a read-only directory, so it gets no faults, and |
| // none of I8, I9 and I10: its book stays in memory. |
| @@ -632,7 +683,10 @@ func (s *sim) failureOf(build, host string, stamp time.Time, heldDown bool, star |
| if reconciled { |
| kind = "an intent the restart reconciled" |
| } |
| - if s.recFailed(build, host, heldDown) || reconciled { |
| + switch changed, created := s.recFailed(build, host, heldDown); { |
| + case created: |
| + s.facts = append(s.facts, simFact{build: build, host: host, kind: kind, start: start, app: true}) |
| + case changed || reconciled: |
| s.facts = append(s.facts, simFact{build: build, host: host, kind: kind, start: start}) |
| } |
| s.checkI8(build, host, heldDown) |
| @@ -662,34 +716,39 @@ func (s *sim) stale(build, host string, start time.Time) bool { |
| } |
| |
| // recFailed is a failure of build on host, not stale, in the model's own |
| -// record of build (grec), and reports whether it changed what restricts |
| -// host (I9): a new record (which restricts every host), host's retry spent, |
| -// or host newly held down. A failure that changes none of these moves no |
| -// generation (rule 1 in FailureBook's doc), so it is no fact; a newer |
| -// failure stamp alone is not counted either, the permissive direction. |
| -func (s *sim) recFailed(build, host string, heldDown bool) bool { |
| +// record of build (grec). It reports whether the failure made a new record |
| +// (created), which restricts every host of the app, and otherwise whether it |
| +// changed what restricts host (changed): host's retry spent, or host newly |
| +// held down (I9). A failure that changes none of these moves no generation |
| +// (rule 1 in FailureBook's doc), so it is no fact; a newer failure stamp |
| +// alone is not counted either, the permissive direction. |
| +func (s *sim) recFailed(build, host string, heldDown bool) (changed, created bool) { |
| r := s.grec[build] |
| - changed := r == nil || r.retried[host] || (heldDown && !r.down[host]) |
| if r == nil { |
| r = &simRec{retried: map[string]bool{}, down: map[string]bool{}} |
| s.grec[build] = r |
| + created = true |
| + } else { |
| + changed = r.retried[host] || (heldDown && !r.down[host]) |
| } |
| delete(r.retried, host) |
| if heldDown { |
| r.down[host] = true |
| } |
| - return changed |
| + return changed, created |
| } |
| |
| -// answered reports whether a clear the spec allows to lift f was issued |
| -// after f's execution began (I9): a grant for its (build, host), a pointer |
| -// naming its build for a record, a lift of its host's hold-ran restriction |
| -// (a good write, a signed rollback) for a hold run. |
| -func (s *sim) answered(f simFact) bool { |
| +// answered reports whether a clear the spec allows to lift f, for host, was |
| +// issued after f's execution began (I9): a grant for its build on host, a |
| +// pointer naming its build for a record, a lift of its host's hold-ran |
| +// restriction (a good write, a signed rollback) for a hold run. host is f's |
| +// own host but for a new record (f.app), which holds every host: there it |
| +// is the host of the grant I9 checks, and only that host's grant answers it. |
| +func (s *sim) answered(f simFact, host string) bool { |
| after := func(ts []time.Time) bool { |
| return slices.ContainsFunc(ts, func(t time.Time) bool { return t.After(f.start) }) |
| } |
| - if after(s.issued[[2]string{f.build, f.host}]) { |
| + if after(s.issued[[2]string{f.build, host}]) { |
| return true |
| } |
| if f.marker { |
| @@ -809,11 +868,21 @@ func (f simAct) Owns(host string) bool { |
| return h != nil && h.phase != simAsleep |
| } |
| |
| -// Swap parks the attempt until a complete step releases it. |
| +// Swap parks the attempt until a complete step releases it. When the seed |
| +// ends first (sim.dead), the goroutine exits from here: Swap never returns, |
| +// so done() never runs and the attempt's Manager never touches the book. |
| func (f simAct) Swap(_ context.Context, a activator.Attempt, newest func(string) uint64) activator.SwapResult { |
| sw := &simSwap{a: a, newest: newest, release: make(chan struct{})} |
| - f.s.events <- simEvent{host: a.Host, swap: sw} |
| - <-sw.release |
| + select { |
| + case f.s.events <- simEvent{host: a.Host, swap: sw}: |
| + case <-f.s.dead: |
| + runtime.Goexit() |
| + } |
| + select { |
| + case <-sw.release: |
| + case <-f.s.dead: |
| + runtime.Goexit() |
| + } |
| return f.s.finishSwap(sw) |
| } |
| |
| @@ -881,8 +950,12 @@ func (s *sim) finishSwap(sw *simSwap) activator.SwapResult { |
| // An adoption onto a build the instance served once, and serves no |
| // more: not a failure the invariants count, but the code records |
| // it (done's transition), so the model's own record follows it |
| - // (I9), as no fact. |
| - s.recFailed(a.Build, a.Host, res.Outcome == activator.SwapDown) |
| + // (I9). It is no fact for its own host, unless it made a new |
| + // record: that holds every host of the app, so it is the app's |
| + // fact (simFact.app). |
| + if _, created := s.recFailed(a.Build, a.Host, res.Outcome == activator.SwapDown); created { |
| + s.facts = append(s.facts, simFact{build: a.Build, host: a.Host, kind: "an adoption failure", start: sw.started, app: true}) |
| + } |
| } |
| if res.Outcome == activator.SwapFailed && a.Mode == activator.ModeHold && !sw.serves { |
| // A hold adoption that ran, failed and was not held down. |
| @@ -937,7 +1010,11 @@ func (s *sim) launch(d dispatch) { |
| if r := recover(); r != nil { |
| ev.crash, ev.stack = r, debug.Stack() |
| } |
| - s.events <- ev |
| + select { |
| + case s.events <- ev: |
| + case <-s.dead: |
| + // The seed ended (Swap's Goexit, or no one left to await). |
| + } |
| }() |
| s.sched.execute(context.Background(), d) |
| }() |
| @@ -1405,12 +1482,14 @@ func (s *sim) checkPlanned(h *simHost, build string) { |
| // outcome reported through the real OnGen. |
| func (s *sim) wakeExec() { |
| if h := s.pick(func(h *simHost) bool { return h.phase == simWaking }); h != nil { |
| - s.wakeExecOn(h) |
| + s.wakeExecOn(h, "") |
| } |
| } |
| |
| -// wakeExecOn is wakeExec for h, which is waking. |
| -func (s *sim) wakeExecOn(h *simHost) { |
| +// wakeExecOn is wakeExec for h, which is waking. A first wake of failBuild |
| +// that runs fails its health check at once, as a flaky build may (rewakeRun's |
| +// scenario); "" fails nothing more than the build itself does. |
| +func (s *sim) wakeExecOn(h *simHost, failBuild string) { |
| rt, plan := h.planRoute, h.plan |
| if s.r.Intn(20) == 0 { |
| // The lease was refused (a previous incarnation still holds it): the |
| @@ -1498,8 +1577,9 @@ func (s *sim) wakeExecOn(h *simHost) { |
| return |
| } |
| b := s.builds[boot.Version] |
| - fails := b.bad || (b.flaky && s.r.Intn(2) == 0) |
| - if fails && s.r.Intn(2) == 0 { |
| + forced := boot.Version == failBuild |
| + fails := forced || b.bad || (b.flaky && s.r.Intn(2) == 0) |
| + if fails && !forced && s.r.Intn(2) == 0 { |
| // It passed its health check and exits inside its observation. |
| fails, h.failLater = false, true |
| } else { |
| @@ -1804,7 +1884,17 @@ func (s *sim) retryHeld() { |
| // before another restriction of its app moves the generation; here the two |
| // halves are one step apart. |
| // |
| -// rewakeArm issues the grant and remembers the host. |
| +// When X has no failure record yet, half the time the wake is instead |
| +// another asleep instance's on the same channel, one that never served X, |
| +// and its first wake of X, if it runs, fails its health check (rewakeFail, |
| +// as a flaky build may: a build with a marker and no record has seldom |
| +// failed anywhere). That failure writes X's first record, which holds every |
| +// host of the app, the granted one too (spec §8.4): a fact of every host |
| +// after the grant was issued (M17's witness: a new record that moved no |
| +// generation). The book is read here only to pick the scenario, never for |
| +// a verdict. |
| +// |
| +// rewakeArm issues the grant and remembers the host to wake. |
| func (s *sim) rewakeArm() { |
| var c []*simHost |
| for _, h := range s.hosts { |
| @@ -1817,23 +1907,35 @@ func (s *sim) rewakeArm() { |
| return |
| } |
| h := c[s.gr.Intn(len(c))] |
| - s.issueGrant(h, s.installed[h.ch]) |
| + x := s.installed[h.ch] |
| + s.issueGrant(h, x) |
| s.rewake = h.name |
| - s.logf("rewake %s: retry pressed under its hold-ran marker", h.name) |
| + if _, recorded := s.book.Lookup("bab", "blog", x); !recorded { |
| + var o []*simHost |
| + for _, h2 := range s.hosts { |
| + if h2 != h && h2.ch == h.ch && h2.phase == simAsleep && !h2.served[x] { |
| + o = append(o, h2) |
| + } |
| + } |
| + if len(o) > 0 && s.gr.Intn(2) == 0 { |
| + s.rewake, s.rewakeFail = o[s.gr.Intn(len(o))].name, x |
| + } |
| + } |
| + s.logf("rewake %s: retry pressed under its hold-ran marker; %s wakes next", h.name, s.rewake) |
| } |
| |
| -// rewakeRun wakes the host rewakeArm armed, if it is still asleep: its plan |
| +// rewakeRun wakes the host rewakeArm chose, if it is still asleep: its plan |
| // and exec, then its stop while the first wake is under observation. Then |
| // the agent reads its grants. |
| func (s *sim) rewakeRun() { |
| - h := s.byName[s.rewake] |
| - s.rewake = "" |
| + h, fail := s.byName[s.rewake], s.rewakeFail |
| + s.rewake, s.rewakeFail = "", "" |
| if h.phase != simAsleep { |
| return |
| } |
| s.wakeBeginOn(h) |
| if h.phase == simWaking { |
| - s.wakeExecOn(h) |
| + s.wakeExecOn(h, fail) |
| } |
| if _, inSwap := s.swaps[h.name]; h.phase == simObserving && !inSwap { |
| s.sleepOn(h) |
| @@ -1917,14 +2019,26 @@ func (s *sim) deliver(g simGrant) { |
| |
| // checkI9 is I9 at a grant that has just cleared something: no failure fact |
| // of its (build, host) began after the grant was issued, unless a clear the |
| -// spec allows has answered it since. |
| +// spec allows has answered it since. A new record is a fact of every host of |
| +// the app (simFact.app), so one made by a failure on another host counts |
| +// too, answered only by a grant for g's build on g's host or a pointer for |
| +// the build. A fact that began in the grant's own step (a start not after |
| +// g.at) is skipped: the model's times are per step, so it cannot order the |
| +// two by time. A rewake's execution in that step began before the grant |
| +// (rewakeRun runs before retryHeld), and is rightly skipped; one the step's |
| +// own action started began after it, and skipping it is the permissive |
| +// direction. |
| func (s *sim) checkI9(g simGrant) { |
| for _, f := range s.facts { |
| - if f.build != g.build || f.host != g.host || !f.start.After(g.at) || s.answered(f) { |
| + if f.build != g.build || (!f.app && f.host != g.host) || !f.start.After(g.at) || s.answered(f, g.host) { |
| continue |
| } |
| - violate("I9", "a grant for %s on %s issued at step %d (%s, book %.8s, generation %d) cleared it after %s of %s on %s in an execution begun at %s, which nothing has answered", |
| - g.build, g.host, g.step, g.at.Sub(t0), g.book, g.epoch, f.kind, f.build, f.host, f.start.Sub(t0)) |
| + what := "" |
| + if f.app { |
| + what = " (a new record, which holds every host)" |
| + } |
| + violate("I9", "a grant for %s on %s issued at step %d (%s, book %.8s, generation %d) cleared it after %s of %s on %s%s in an execution begun at %s, which nothing has answered", |
| + g.build, g.host, g.step, g.at.Sub(t0), g.book, g.epoch, f.kind, f.build, f.host, what, f.start.Sub(t0)) |
| } |
| } |
| |
| @@ -1984,11 +2098,12 @@ func (s *sim) diskBook() (failureFile, error) { |
| } |
| |
| // agentRestart is the agent process ending and starting again, on a seed |
| -// whose book is on disk (I9). Everything in flight goes with it: each |
| -// attempt parked in Swap is left there for good (its goroutine never runs |
| -// again, so nothing of the old Manager touches the book), a healthy event |
| -// on its way is lost, and every instance stops, the agent's children with |
| -// it. A window of failing saves closes first: an agent that cannot save |
| +// whose book is on disk (I9). Everything in flight goes with it. Each |
| +// attempt parked in Swap stays parked until the seed ends, when its |
| +// goroutine exits from Swap (sim.dead) without returning, so nothing of the |
| +// old Manager touches the book again, as a killed process cannot. A healthy |
| +// event on its way is lost, and every instance stops, the agent's children |
| +// with it. A window of failing saves closes first: an agent that cannot save |
| // its book at startup refuses managed starts (internal/host), so the box |
| // runs again only once the disk takes a save. The new agent opens the book |
| // from disk (OpenFailureBook, which reconciles the intents left there), |
amadan