mesh-issues: review — one judgement at a time with new issues first, one verdict applied at a time, a resolved issue left alone on redelivery, the pass's answer a copy (hq ADR 0278)

An independent review found a pass could starve new issues' checks, two writers
could apply one verdict twice, a judged issue resolved by a person could still be
linked, and the dry run's answer raced its pass.

modules/mesh-issues/README.md

@@ -63,6 +63,10 @@ older ones not resolved and not found a duplicate in the same pass. It writes on
 `grooming` (the last pass, one key), and applies that pass's findings on `confirm` — never a fresh judgement.
 A finding whose issues have moved on is skipped and said. It adds no "none" note to the backlog.
 
+The tracker asks one judgement at a time; the pass waits while a new issue's check is waiting, and one verdict is
+applied at a time. A check may take up to a minute (two judgements of at most 25 seconds each), which the tool
+runner allows a handler while keeping its event alive (hq issue 387).
+
 ## The import of hq's issues (slice 3)
 
 `issue-tracker.import` reads every `04-ISSUES/NNN-slug/00-report.md` of hq **through the records module

modules/mesh-issues/cmd/mesh-issues/groom.go

@@ -17,6 +17,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	stdio "git.novox.be/novox/mesh-sdk/go"
@@ -87,8 +88,10 @@ type Verdict struct {
 	Issue   uint64 `json:"issue,omitempty"`
 	Certain bool   `json:"certain,omitempty"`
 	Reason  string `json:"reason,omitempty"`
-	// Checked is how many older issues not resolved it was judged against.
+	// Checked is how many older issues not resolved it was judged against; Older how many there were, more
+	// when their titles did not all fit the question.
 	Checked int `json:"checked"`
+	Older   int `json:"older,omitempty"`
 	// By is who judged: the agent, its node and model.
 	By string `json:"by,omitempty"`
 }
@@ -275,7 +278,7 @@ func (t *Tracker) judge(is *Issue, candidates []*Issue) (*Verdict, error) {
 		}
 	}
 	if len(shortlist) == 0 {
-		return &Verdict{Verdict: VerdictNone, Checked: len(offered), By: by}, nil
+		return &Verdict{Verdict: VerdictNone, Checked: len(offered), Older: len(candidates), By: by}, nil
 	}
 
 	var b strings.Builder
@@ -298,7 +301,7 @@ func (t *Tracker) judge(is *Issue, candidates []*Issue) (*Verdict, error) {
 	if err := json.Unmarshal(raw, &v); err != nil {
 		return nil, fmt.Errorf("the verdict came back outside the shape asked: %.200s", raw)
 	}
-	v.By, v.Checked, v.Reason = by, len(offered), strings.TrimSpace(v.Reason)
+	v.By, v.Checked, v.Older, v.Reason = by, len(offered), len(candidates), strings.TrimSpace(v.Reason)
 	switch v.Verdict {
 	case VerdictNone:
 		v.Issue, v.Certain = 0, false
@@ -337,14 +340,21 @@ func verdictNote(v *Verdict) string {
 	if v.By != "" {
 		by = " Judged by " + v.By + "."
 	}
-	return fmt.Sprintf("%snone of %d older issues not resolved asks the same question, and none is closely related.%s "+
+	most := ""
-		"(novox/hq ADR 0278)", notePrefix, v.Checked, by)
+	if v.Older > v.Checked {
+		most = fmt.Sprintf(" They were the %d most recent of %d; the titles of the rest did not fit the question.", v.Checked, v.Older)
+	}
+	return fmt.Sprintf("%snone of %d older issues not resolved asks the same question, and none is closely related.%s%s "+
+		"(novox/hq ADR 0278)", notePrefix, v.Checked, most, by)
 }
 
 // apply does what a verdict on the newer issue says (ADR 0278 §2, §3), each step only when it is not done
 // yet, through the tracker's own note, link and move. The verdict note is written first. The older issue is
 // noted and linked to, never moved; neither side is touched once resolved or wontfix. Answers what it did.
 func (t *Tracker) apply(newer uint64, v *Verdict) ([]string, error) {
+	// One writer of verdicts at a time: an event's check and a confirm never both find no verdict note.
+	t.groom.applying.Lock()
+	defer t.groom.applying.Unlock()
 	is, _, err := read(t.Store, newer)
 	if err != nil {
 		return nil, err
@@ -371,7 +381,12 @@ func (t *Tracker) apply(newer uint64, v *Verdict) ([]string, error) {
 		}
 		did = append(did, fmt.Sprintf("noted the verdict on %d", newer))
 	} else {
-		// Delivered again, or reached again by the backlog's pass: what was decided stands, in its own words.
+		// Delivered again, or reached again by the backlog's pass: what was decided stands, in its own words —
+		// unless the issue has moved on since (a person resolved it): then it is not touched. The move to
+		// wontfix is always the last step, so an issue the check closed has nothing left to do either.
+		if !unresolved(is) {
+			return nil, nil
+		}
 		v = recorded(is)
 	}
 	if v.Verdict == VerdictNone {
@@ -464,7 +479,12 @@ func (t *Tracker) Check(n uint64) error {
 			older = append(older, c)
 		}
 	}
+	// A new issue goes before the backlog: the pass waits while an event's check waits for the agent.
+	t.groom.waiting.Add(1)
+	t.groom.judging.Lock()
+	t.groom.waiting.Add(-1)
 	v, err := t.judge(is, older)
+	t.groom.judging.Unlock()
 	if err != nil {
 		t.Logf("[mesh-issues] duplicate check of %d failed: %v", n, err)
 		if !noted(is, failedLine) {
@@ -549,6 +569,13 @@ const (
 type grooming struct {
 	mu  sync.Mutex
 	run string
+	// judging is held while a judgement is asked: one at a time from this tracker, and waiting counts the
+	// event checks waiting for it, which the backlog's pass lets go first.
+	judging sync.Mutex
+	waiting atomic.Int32
+	// applying is held while a verdict is applied, and confirming while a pass is.
+	applying   sync.Mutex
+	confirming sync.Mutex
 }
 
 func newPassID() string {
@@ -627,11 +654,11 @@ func (t *Tracker) Groom(a GroomAsk, background func(func())) (*Groomed, error) {
 		if !a.Confirm {
 			return &Groomed{Said: passSaid(kept), Pass: kept}, nil
 		}
-		return t.applyPass(kept)
+		return t.applyPass(id)
 	}
 	if running != "" {
-		return nil, fmt.Errorf("pass %s is running (%d of %d issues judged); one pass runs at a time: ask groom with "+
+		return nil, fmt.Errorf("pass %s is running; one pass runs at a time: ask groom with pass %s for its progress",
-			"pass %s for its progress", kept.ID, kept.Judged, kept.Issues, kept.ID)
+			running, running)
 	}
 	issues, err := t.unresolvedIssues()
 	if err != nil {
@@ -656,8 +683,9 @@ func (t *Tracker) Groom(a GroomAsk, background func(func())) (*Groomed, error) {
 		t.groom.mu.Unlock()
 		return nil, err
 	}
+	answered := *p
 	background(func() { t.runPass(p, issues) })
-	return &Groomed{Pass: p, Said: fmt.Sprintf("Started dry run %s over %d issues not resolved and not yet checked, oldest "+
+	return &Groomed{Pass: &answered, Said: fmt.Sprintf("Started dry run %s over %d issues not resolved and not yet checked, oldest "+
 		"first. It writes nothing on any issue. Ask groom with pass %s for its progress and findings; apply them with "+
 		"confirm and pass %s.", p.ID, p.Issues, p.ID, p.ID)}, nil
 }
@@ -684,7 +712,12 @@ func (t *Tracker) runPass(p *Pass, issues []*Issue) {
 				older = append(older, c)
 			}
 		}
+		for t.groom.waiting.Load() > 0 {
+			t.Sleep(500 * time.Millisecond)
+		}
+		t.groom.judging.Lock()
 		v, err := t.judge(is, older)
+		t.groom.judging.Unlock()
 		p.Judged++
 		switch {
 		case err != nil:
@@ -730,8 +763,27 @@ func passSaid(p *Pass) string {
 		"on any issue; apply the findings with confirm and pass %s.", p.ID, p.Judged, len(p.Findings), len(p.Failures), p.ID)
 }
 
-// applyPass applies exactly a done pass's findings, each checked again against the issues as they stand.
+// applyPass applies exactly a done pass's findings, each checked again against the issues as they stand. One
-func (t *Tracker) applyPass(p *Pass) (*Groomed, error) {
+// confirm at a time; the pass is read again under it and kept only at the revision read, so a confirm never
+// writes over a newer pass.
+func (t *Tracker) applyPass(id string) (*Groomed, error) {
+	if !t.groom.confirming.TryLock() {
+		return nil, errors.New("a pass is being applied at this moment; ask again once it is done")
+	}
+	defer t.groom.confirming.Unlock()
+	p, rev, err := t.readPass()
+	if err != nil {
+		return nil, err
+	}
+	if p == nil || p.ID != id {
+		return nil, fmt.Errorf("there is no pass %s: only the last pass is kept, and a new dry run replaces it", id)
+	}
+	t.groom.mu.Lock()
+	running := t.groom.run
+	t.groom.mu.Unlock()
+	if p.State == PassRunning && p.ID != running {
+		p.State = PassInterrupted
+	}
 	switch p.State {
 	case PassRunning:
 		return nil, fmt.Errorf("pass %s is still running; it is applied once it is done", p.ID)
@@ -751,7 +803,7 @@ func (t *Tracker) applyPass(p *Pass) (*Groomed, error) {
 	}
 	at := t.now()
 	p.AppliedAt, p.State = &at, PassApplied
-	if err := t.savePass(p); err != nil {
+	if _, err := t.Store.Update(GroomingState, passKey, p, rev); err != nil {
 		return nil, fmt.Errorf("the findings were applied (%s) and the pass could not be kept: %v", strings.Join(p.Applied, "; "), err)
 	}
 	return &Groomed{Pass: p, Said: passSaid(p)}, nil

modules/mesh-issues/cmd/mesh-issues/groom_test.go

@@ -441,3 +441,52 @@ func TestConfirmClosesOnlyTheNewerAndKeepsTheOlderOpen(t *testing.T) {
 		t.Fatal("an interrupted pass was applied")
 	}
 }
+
+// An issue the check already judged, then resolved by a person, is not touched when its event comes again.
+func TestAJudgedIssueAPersonResolvedIsNotTouchedAgain(t *testing.T) {
+	h := newHarness(t)
+	older := h.open(t, "the counter skips a number")
+	newer := h.open(t, "a number is skipped")
+	if _, err := h.note(GroomBy, newer, "Duplicate check: duplicate of 365.\n\nWhy: the same."); err != nil {
+		t.Fatal(err)
+	}
+	resolve(t, h, newer)
+	withJudge(h, &stubJudge{err: errors.New("must not be asked")})
+	if err := h.Check(newer); err != nil {
+		t.Fatal(err)
+	}
+	if len(get(t, h, newer).Links) != 0 || len(get(t, h, older).Notes) != 0 {
+		t.Fatal("a resolved issue was linked, or its older noted")
+	}
+}
+
+// The pass runs in the background for real, while its answer is read and a new issue is checked; the race
+// detector watches.
+func TestThePassRunsBesideTheChecksOfNewIssues(t *testing.T) {
+	h := newHarness(t)
+	for i := 0; i < 6; i++ {
+		h.open(t, "issue "+string(rune('a'+i)))
+	}
+	withJudge(h, &stubJudge{})
+	done := make(chan struct{})
+	out, err := h.Groom(GroomAsk{}, func(f func()) { go func() { f(); close(done) }() })
+	if err != nil {
+		t.Fatal(err)
+	}
+	raw, _ := json.Marshal(out) // read while the pass runs
+	newer := h.open(t, "a new one")
+	if err := h.Check(newer); err != nil {
+		t.Fatal(err)
+	}
+	<-done
+	if !strings.Contains(string(raw), out.Pass.ID) {
+		t.Fatal(string(raw))
+	}
+	p, _, _ := h.readPass()
+	if p.State != PassDone || p.Judged != 6 {
+		t.Fatalf("%+v", p)
+	}
+	if nn := checkNotes(get(t, h, newer)); len(nn) != 1 || !strings.HasPrefix(nn[0], "Duplicate check: none of 6 older") {
+		t.Fatalf("%q", nn)
+	}
+}