diff --git a/go/buildstep/conformance/conformance_test.go b/go/buildstep/conformance/conformance_test.go index 3ee2367..e6380e4 100644 --- a/go/buildstep/conformance/conformance_test.go +++ b/go/buildstep/conformance/conformance_test.go @@ -27,8 +27,8 @@ func TestTheFailureContract(t *testing.T) { LeftRunning(t, func(ctx context.Context, scan buildstep.Scan, node string, before time.Time, logf func(string, ...any)) { buildstep.EndStaleAtStart(ctx, scan, node, before, logf) }) - if p := JudgeLeftRunning(func(context.Context, buildstep.Scan, string, time.Time, func(string, ...any)) {}); len(p) != 13 { - t.Errorf("a sweep that ends nothing: %d problems, want 13 (five steps not ended, each unsaid, three owners unnamed):\n%s", len(p), strings.Join(p, "\n")) + if p := JudgeLeftRunning(func(context.Context, buildstep.Scan, string, time.Time, func(string, ...any)) {}); len(p) != 11 { + t.Errorf("a sweep that ends nothing: %d problems, want 11 (three steps not ended and unsaid, one owner unnamed, two asks not deleted and unsaid):\n%s", len(p), strings.Join(p, "\n")) } wrongNode := func(ctx context.Context, scan buildstep.Scan, _ string, before time.Time, logf func(string, ...any)) { buildstep.EndStaleAtStart(ctx, scan, "kitother", before, logf) diff --git a/go/buildstep/conformance/leftrunning.go b/go/buildstep/conformance/leftrunning.go index 3860d96..a8812c4 100644 --- a/go/buildstep/conformance/leftrunning.go +++ b/go/buildstep/conformance/leftrunning.go @@ -18,7 +18,8 @@ import ( // **A step left running by a previous process** (novox/hq issue 1890, note 21): when a delivery replaced mesh-build-go // on a node while a check ask ran, the new process's start sweep let the ask's key stand as running, and it held one // of the node's build slots until a hand end. Every build module runs this case (LeftRunning) through the start sweep -// its main runs: the steps a previous process left are ended at once and loud, what this process runs is kept. A +// its main runs: the hand-started steps a previous process left are ended at once and loud, its asks' keys deleted (the +// stream redelivers the ask, novox/hq ADR 0525 §6, and its asker keeps waiting for that), what this process runs kept. A // catalogue check (mesh-catalog checks/failurekit) refuses a build module whose tests do not call LeftRunning. // Sweep is a build module's own start sweep, as its main runs it once at its start: it ends node's steps in scan that @@ -35,6 +36,7 @@ type leftCase struct { node string age time.Duration // started this long before the process began (negative: after it) ended bool + deleted bool // a gone process's ask: its key is deleted, not ended } // gone is a previous process's owner on the same node. @@ -42,8 +44,8 @@ var gone = "b00t-of-the-kit.4242.mvp0000" func leftCases() []leftCase { return []leftCase{ - {id: "mvpwlkzi7e6ca8", tool: "check ask", owner: gone, age: 5 * time.Minute, ended: true}, // note 21's ghost - {id: "mvpwlkzi7e6ca9", tool: "delivery ask", owner: gone, age: -time.Second, ended: true}, + {id: "mvpwlkzi7e6ca8", tool: "check ask", owner: gone, age: 5 * time.Minute, deleted: true}, // note 21's ghost + {id: "mvpwlkzi7e6ca9", tool: "delivery ask", owner: gone, age: -time.Second, deleted: true}, {id: "gonehand1", tool: "build_x_test", owner: gone, age: time.Minute, ended: true}, {id: "oldhand1", tool: "build_x_fetch", age: time.Hour, ended: true}, // no owner: an older build's {id: "mvpwlkzi7e6caa", tool: "check ask", age: time.Hour, ended: true}, // no owner, past the bound @@ -107,7 +109,22 @@ func JudgeLeftRunning(sweep Sweep) []string { node = LeftNode } key, _ := delivery.StepKey(node, c.id) - s, err := delivery.DecodeStep(st.get(key)) + raw := st.get(key) + if c.deleted { + if raw != nil { + problems = append(problems, fmt.Sprintf("left running: the ask %s of the gone owner %q is still held after the sweep (%s); "+ + "its key is deleted, never ended: the stream redelivers it (ADR 0525 §6)", key, c.owner, raw)) + } + if !strings.Contains(log, key) || !strings.Contains(log, c.owner) { + problems = append(problems, "left running: the delete of "+key+" is not said with its owner "+c.owner) + } + continue + } + if raw == nil { + problems = append(problems, "left running: the step "+key+" is gone after the sweep, want it kept or ended") + continue + } + s, err := delivery.DecodeStep(raw) if err != nil { problems = append(problems, key+": "+err.Error()) continue @@ -168,6 +185,16 @@ func (s *steps) scan() buildstep.Scan { defer s.mu.Unlock() return s.vals[key], s.revs[key], nil }, + Delete: func(key string, revision uint64) error { + s.mu.Lock() + defer s.mu.Unlock() + if s.revs[key] != revision { + return stdio.ErrChanged + } + delete(s.vals, key) + s.revs[key]++ + return nil + }, Update: func(key string, value any, revision uint64) (uint64, error) { raw, err := json.Marshal(value) if err != nil { diff --git a/go/buildstep/stale.go b/go/buildstep/stale.go index 1156253..988c04e 100644 --- a/go/buildstep/stale.go +++ b/go/buildstep/stale.go @@ -30,46 +30,68 @@ var Boot = time.Now() // Owner is this process as a step's owner (delivery.Step.Owner): `..`, the node's boot id // (/proc/sys/kernel/random/boot_id), the pid and Boot in milliseconds, base 36. The start keeps two processes apart // that a reboot or a pid namespace gave the same pid. A running step whose owner is another is a previous process's. -var Owner = ownerOf(readBootID(), os.Getpid(), Boot) +var Owner = ownerOf(bootID, os.Getpid(), Boot) + +// bootID is the node's boot id as Owner holds it; noBootID when it could not be read, which EndStaleAtStart says once. +var bootID = readBootID() + +const noBootID = "no-boot-id" func ownerOf(bootID string, pid int, start time.Time) string { return bootID + "." + strconv.Itoa(pid) + "." + strconv.FormatInt(start.UnixMilli(), 36) } -// readBootID is the node's boot id, or "no-boot-id" where it cannot be read (the pid and the start then tell the -// processes apart). +// readBootID is the node's boot id, or noBootID where it cannot be read (the pid and the start then tell the processes +// apart). func readBootID() string { b, err := os.ReadFile("/proc/sys/kernel/random/boot_id") if id := strings.TrimSpace(string(b)); err == nil && id != "" { return id } - return "no-boot-id" + return noBootID } // Scan reads and writes a steps state with its revisions: its keys, a key's stored value and revision (nil and 0 for -// none), and a write that lands only while the key is still at the revision read (KeptState.Update), failing with -// stdio.ErrChanged when another writer was first. +// none), a write that lands only while the key is still at the revision read (KeptState.Update), and a delete of a key +// still at the revision read; both fail with stdio.ErrChanged when another writer was first. type Scan struct { Keys func() ([]string, error) Get func(key string) (json.RawMessage, uint64, error) Update func(key string, value any, revision uint64) (uint64, error) + Delete func(key string, revision uint64) error } // ScanOf is the Scan of a state the process keeps. +// +// Its Delete reads the key again and deletes it only while it is still at the revision read: the runtime's +// mesh/state.delete takes no revision yet, so a write landing between that read and the delete (a few milliseconds) is +// deleted with it. For a step key that is a redelivered ask begun on this node in that moment: its running key leaves +// the steps state until its run writes its end, which its asker still reads (a watch takes puts only). func ScanOf(s stdio.KeptState) Scan { - return Scan{Keys: s.Keys, Update: s.Update, Get: func(key string) (json.RawMessage, uint64, error) { + get := func(key string) (json.RawMessage, uint64, error) { e, err := s.Get(key) if err != nil || e == nil { return nil, 0, err } return e.Value, e.Revision, nil + } + return Scan{Keys: s.Keys, Update: s.Update, Get: get, Delete: func(key string, revision uint64) error { + _, rev, err := get(key) + if err != nil { + return err + } + if rev != revision { + return stdio.ErrChanged + } + return s.Delete(key) }} } // isAsk says a step is a pulled ask's: its key is . and its tool is " ask" (a hand-started step // has the tool's name). An ask's key is the stream's to settle, never a restart's: the puller leaves an ask it was // running unacknowledged so the stream redelivers it, and the asker's wait takes the first end it reads on the key. -// Only once the asker's bound has passed (delivery.StepBound) is the key nobody's, and then it is ended. +// A key a gone process owns is deleted, never ended (EndStale); one with no owner is ended only once the asker's bound +// has passed (delivery.StepBound), when it is nobody's. func isAsk(key string, s delivery.Step) bool { _, _, err := delivery.ParseAskStepKey(key) return err == nil && strings.HasSuffix(s.Tool, " ask") @@ -117,20 +139,24 @@ func endRunning(scan Scan, key string, step delivery.Step, revision uint64, why return err } -// EndStale ends this node's running steps that no process of this module runs any more, as failed with RestartReason, -// and answers their ids. Only keys of node are read; a value that is no step is left alone. -// - A step with an owner (delivery.Step.Owner) that is not this process (Owner) is ended at once, whatever its age and -// kind: its process is gone, so nothing will write its end (issue 1890, note 21: a check ask left by a replaced -// mesh-build-go held one of shanks' two slots). A pulled ask the stream gives this node again is begun under a fresh -// owner, so a redelivery is never ended; its asker, which takes the first end it reads, hears the restart at once -// instead of at its bound. -// - A step owned by this process is left: it may run now. +// EndStale ends this node's running steps that no process of this module runs any more, and answers their ids. Only keys +// of node are read; a value that is no step is left alone. +// - A hand-started step with an owner (delivery.Step.Owner) that is not this process (Owner) is ended failed with +// RestartReason at once, whatever its age: its process is gone and nothing redelivers it, so nothing else would +// write its end (issue 1890, note 21). +// - A pulled ask's step with an owner that is not this process is deleted, not ended (novox/hq ADR 0525 §6): the +// stream redelivers an ask whose node stopped (the puller neither ends nor acknowledges one at its stop), and its +// asker takes the first end it reads, so an end written here would fail a check the redelivery would still run. Its +// asker's watch takes puts only and keeps waiting for the redelivery's key, its bound the backstop; the stale running +// key leaves the steps state, and with it the delivery view's and the checker's count of what runs (note 21: it +// held one of shanks' two slots). +// - A step owned by this process is left: it may run now. A redelivered ask begun on this node is owned by it. // - A step with no owner (written by an older build) keeps the earlier rule: a hand-started step started before // `before` (the start of this process) is ended; a pulled ask is left to the stream's redelivery (isAsk) until its // start is older than delivery.StepBound, and then ended "past the step bound: its asker stopped waiting at