Delete a gone process's ask key instead of ending it, so its redelivery still answers the asker (issue 1890)

ADR 0525 §6 has the stream redeliver an ask whose node stopped, and the
asker takes the first end it reads: ending the key at a restart failed
checks a redelivery would still run. The sweep now deletes such a key,
guarded by the revision read, so the asker keeps waiting and the stale
key leaves the counts of what runs; a hand-started step of a gone owner
is still ended at once. A missing boot id is said once at start.

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 {
+		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 13 (five steps not ended, each unsaid, three owners unnamed):\n%s", len(p), strings.Join(p, "\n"))
+			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)

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: "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, ended: true},
+		{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 {

go/buildstep/stale.go

@@ -30,46 +30,68 @@ var Boot = time.Now()
 // Owner is this process as a step's owner (delivery.Step.Owner): `<boot id>.<pid>.<start>`, 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
+// readBootID is the node's boot id, or noBootID where it cannot be read (the pid and the start then tell the processes
-// processes apart).
+// 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
+// none), a write that lands only while the key is still at the revision read (KeptState.Update), and a delete of a key
-// stdio.ErrChanged when another writer was first.
+// 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 <node>.<ask id> and its tool is "<kind> 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,
+// EndStale ends this node's running steps that no process of this module runs any more, and answers their ids. Only keys
-// and answers their ids. Only keys of node are read; a value that is no step is left alone.
+// 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
+//   - A hand-started step with an owner (delivery.Step.Owner) that is not this process (Owner) is ended failed with
-//     kind: its process is gone, so nothing will write its end (issue 1890, note 21: a check ask left by a replaced
+//     RestartReason at once, whatever its age: its process is gone and nothing redelivers it, so nothing else would
-//     mesh-build-go held one of shanks' two slots). A pulled ask the stream gives this node again is begun under a fresh
+//     write its end (issue 1890, note 21).
-//     owner, so a redelivery is never ended; its asker, which takes the first end it reads, hears the restart at once
+//   - A pulled ask's step with an owner that is not this process is deleted, not ended (novox/hq ADR 0525 §6): the
-//     instead of at its bound.
+//     stream redelivers an ask whose node stopped (the puller neither ends nor acknowledges one at its stop), and its
-//   - A step owned by this process is left: it may run now.
+//     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 <time>".
 //
-// Each end is logged, naming the step, its owner and why. A key written by another writer since it was read (a
+// Each end and delete is logged, naming the step, its owner and why. A key written by another writer since it was read
-// redelivered ask's start, a real end) is skipped.
+// (a redelivered ask's start, a real end) is skipped.
 func EndStale(scan Scan, node string, before time.Time, logf func(string, ...any)) ([]string, error) {
 	keys, err := scan.Keys()
 	if err != nil {
@@ -157,22 +183,34 @@ func EndStale(scan Scan, node string, before time.Time, logf func(string, ...any
 		if why == "" {
 			continue
 		}
-		if err := endRunning(scan, k, step, rev, why); err != nil {
+		done := "ended"
+		if step.Owner != "" && isAsk(k, step) {
+			done = "deleted, left to the stream's redelivery"
+			if scan.Delete == nil {
+				err = fmt.Errorf("the steps state takes no delete")
+			} else {
+				err = scan.Delete(k, rev)
+			}
+		} else {
+			err = endRunning(scan, k, step, rev, why)
+		}
+		if err != nil {
 			if errors.Is(err, stdio.ErrChanged) {
 				continue
 			}
 			return ended, fmt.Errorf("buildstep: ending the step %s: %w", k, err)
 		}
 		if logf != nil {
-			logf("the step %s (%s, started %s) was left running and is ended: %s; %s", k, step.Tool,
+			logf("the step %s (%s, started %s) was left running and is %s: %s; %s", k, step.Tool,
-				step.Started.UTC().Format(time.RFC3339), why, said)
+				step.Started.UTC().Format(time.RFC3339), done, why, said)
 		}
 		ended = append(ended, step.ID)
 	}
 	return ended, nil
 }
 
-// staleWhy is why a running step is ended by EndStale and what the log says of its owner, or "" to leave it.
+// staleWhy is why a running step is ended (or, for an ask with an owner, deleted) by EndStale and what the log says of
+// its owner, or "" to leave it.
 func staleWhy(key string, step delivery.Step, before time.Time) (why, said string) {
 	switch {
 	case step.Owner == Owner:
@@ -277,14 +315,19 @@ var (
 var StaleServed = stdio.Served
 
 // EndStaleAtStart is what a build module runs once at its start, in a goroutine: it waits for the runtime to serve,
-// then ends this node's steps left running before `before` (EndStale), trying again when the state cannot be read yet.
+// then ends this node's steps a previous process left running (EndStale), trying again when the state cannot be read
-// A step this process starts is never one of them: it starts after `before`.
+// yet. A step names its owner, so `before` (the start of this process) matters only for a step with no owner, written
+// by an older build: such a step started before it is a previous process's. When the node's boot id cannot be read it
+// says so once: Owner then tells processes apart by their pid and start only.
 func EndStaleAtStart(ctx context.Context, scan Scan, node string, before time.Time, logf func(string, ...any)) {
 	select {
 	case <-ctx.Done():
 		return
 	case <-StaleServed():
 	}
+	if bootID == noBootID && logf != nil {
+		logf("the boot id of %s cannot be read (/proc/sys/kernel/random/boot_id): its steps' owner is %s, told apart by pid and start only", node, Owner)
+	}
 	for attempt := 1; ; attempt++ {
 		ended, err := EndStale(scan, node, before, logf)
 		if err == nil {

go/buildstep/stale_test.go

@@ -34,6 +34,15 @@ func scanOf(k *keys, race func(key string)) Scan {
 			raw, err := delivery.EncodeStep(s[len(s)-1])
 			return raw, uint64(len(s)), err
 		},
+		Delete: func(key string, revision uint64) error {
+			k.mu.Lock()
+			defer k.mu.Unlock()
+			if uint64(len(k.puts[key])) != revision {
+				return stdio.ErrChanged
+			}
+			delete(k.puts, key)
+			return nil
+		},
 		Update: func(key string, value any, revision uint64) (uint64, error) {
 			if race != nil {
 				race(key)
@@ -49,7 +58,8 @@ func scanOf(k *keys, race func(key string)) Scan {
 	}
 }
 
-// 1890 and 1890/AC6: EndStale ends this node's running steps of another owner at once, whatever their age or kind; of
+// 1890 and 1890/AC6: EndStale ends this node's hand-started running steps of another owner at once, whatever their age,
+// and deletes its asks of another owner (the stream redelivers them, ADR 0525 §6); of
 // the steps with no owner (an older build's), the hand-started ones left running before the process began and the asks
 // past the step bound (a younger ask is the stream's to redeliver); never a step this process owns or a key written
 // meanwhile. EndOne ends one by hand, never one this process owns or started.
@@ -87,7 +97,10 @@ func TestEndStaleAndEndOne(t *testing.T) {
 	if err != nil || len(ended) != 4 {
 		t.Fatalf("ended %v, %v", ended, err)
 	}
-	for _, key := range []string{"ace.old1", "ace.mjk3l9x0abcded", "ace.gone2"} {
+	if _, held := k.puts["ace.mjk3l9x0abcded"]; held {
+		t.Errorf("the ghost ask's key is still held: %+v", k.last("ace.mjk3l9x0abcded"))
+	}
+	for _, key := range []string{"ace.old1", "ace.gone2"} {
 		if s := k.last(key); s.State != delivery.StepFailed || s.Reason != RestartReason || s.Ended.IsZero() {
 			t.Errorf("%s = %+v", key, s)
 		}
@@ -95,7 +108,7 @@ func TestEndStaleAndEndOne(t *testing.T) {
 	if s := k.last("ace.mjk3l9x0abcdef"); s.State != delivery.StepFailed || !strings.HasPrefix(s.Reason, "past the step bound: its asker stopped waiting at ") {
 		t.Errorf("the old ask = %+v", s)
 	}
-	if log := strings.Join(said, "\n"); !strings.Contains(log, "ace.mjk3l9x0abcded") || !strings.Contains(log, "its owner "+gone+" is not this process "+Owner) {
+	if log := strings.Join(said, "\n"); !strings.Contains(log, "ace.mjk3l9x0abcded") || !strings.Contains(log, "deleted, left to the stream's redelivery") || !strings.Contains(log, "its owner "+gone+" is not this process "+Owner) {
 		t.Errorf("the log does not name the ghost and its owner:\n%s", log)
 	}
 	for key, state := range map[string]string{"ace.mjk3l9x0abcdee": delivery.StepRunning, "ace.mine1": delivery.StepRunning, "ace.raced1": delivery.StepPassed,