diff --git a/modules/mesh-issues/README.md b/modules/mesh-issues/README.md index e576850e..bfba6ed0 100644 --- a/modules/mesh-issues/README.md +++ b/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 diff --git a/modules/mesh-issues/cmd/mesh-issues/groom.go b/modules/mesh-issues/cmd/mesh-issues/groom.go index 4dd64a28..8fe4f98c 100644 --- a/modules/mesh-issues/cmd/mesh-issues/groom.go +++ b/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 "+ - "(novox/hq ADR 0278)", notePrefix, v.Checked, by) + most := "" + 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 "+ - "pass %s for its progress", kept.ID, kept.Judged, kept.Issues, kept.ID) + return nil, fmt.Errorf("pass %s is running; one pass runs at a time: ask groom with pass %s for its progress", + 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. -func (t *Tracker) applyPass(p *Pass) (*Groomed, error) { +// applyPass applies exactly a done pass's findings, each checked again against the issues as they stand. One +// 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 diff --git a/modules/mesh-issues/cmd/mesh-issues/groom_test.go b/modules/mesh-issues/cmd/mesh-issues/groom_test.go index 702e7634..f6f6b041 100644 --- a/modules/mesh-issues/cmd/mesh-issues/groom_test.go +++ b/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) + } +}