diff --git a/modules/mesh-knowledge/README.md b/modules/mesh-knowledge/README.md index d3576f92..4657b39c 100644 --- a/modules/mesh-knowledge/README.md +++ b/modules/mesh-knowledge/README.md @@ -104,8 +104,12 @@ as text. `get` serves each, and any other field a record of hq wrote under `fiel Its author revises it by proposing it again, and that version is reviewed again. - A judgement that cannot be had (the judge unreachable, or an answer that is not `approve`, or `changes` naming no finding) leaves the record proposed and says so on it (`get`'s `review`). It is never taken as an - approval. It is asked again after 10, then 20 minutes; after 3 tries the record says its author proposes it - again to have it reviewed again, and the module is unhealthy, naming it, so the mesh raises a condition. + approval. It is owed: its try count and when it is asked again (10, then 20 minutes later) are kept with the + version in the store and said in its history, so the background work takes it up at that time, also after a + restart of the tool runner (hq issue 497). After 3 tries the record says it was the last and its author + proposes it again to have it reviewed again; the operator is told once on the operator's channel (keyed by + record and version, and cleared once the record is proposed again, accepted, withdrawn or superseded), and + the module is unhealthy, naming it, so the mesh raises a condition. An ask of the operator that cannot be made counts as a try the same way. - A review whose outcome the database refused to keep is held back for that record alone (10, then 20 minutes, 3 tries, logged as given up), so it never blocks the records behind it. diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go b/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go index c32ed260..db2fe8fb 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/answers.go @@ -770,11 +770,6 @@ func reviewSays(r *Record) string { case JudgingUnproven: return "not reviewed: " + j.Reason } - s := fmt.Sprintf("not reviewed: %s (try %d of %d); it stays proposed, and no judgement that could not be had counts as an approval", j.Reason, j.Tries, MostReviewTries) - if j.Tries >= MostReviewTries { - s += "; its author proposes it again to have it reviewed again" - } else { - s += "; asked again after a pause" - } - return s + // When it is asked again is said, so a pause is not taken for a review lost (novox/hq issue 497). + return fmt.Sprintf("not reviewed: %s (%s; no judgement that could not be had counts as an approval", j.Reason, j.owedSays()) } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/draft.go b/modules/mesh-knowledge/cmd/mesh-knowledge/draft.go index 4bcbe82e..7e1bc2bb 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/draft.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/draft.go @@ -356,7 +356,7 @@ func (k *KB) tellTopic(ctx context.Context, topic string) { summary := fmt.Sprintf("The clauses of %d prose decisions on %s are drafted and checked: %d confirmed, %d still drafted.", records, topic, confirmed, drafted) explanation := "A second judgement confirmed each clause against its record; a clause still drafted is never served as in force. " + "You may send any record back with the knowledge base's return, and its clauses are drafted again. Nothing needs you otherwise." - if err := k.Notify("knowledge.clauses."+strings.ReplaceAll(topic, " ", "-"), summary, "Clauses drafted for "+topic, explanation); err != nil { + if err := k.Notify("knowledge.clauses."+strings.ReplaceAll(topic, " ", "-"), summary, "Clauses drafted for "+topic, explanation, ""); err != nil { k.logf("[mesh-knowledge] the operator could not be told about %s's clauses: %v", topic, err) } } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go b/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go index 64b12530..9e51e68d 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/kb.go @@ -64,8 +64,12 @@ type Reserver func(kind, title string) (int, error) // the structured answer and who judged. type Judgement func(question string, schema map[string]any, model string) (json.RawMessage, string, error) -// Notifier tells the operator's channel something (operator-channel.notify). -type Notifier func(key, summary, headline, explanation string) error +// Notifier tells the operator's channel something (operator-channel.notify). Resolved is the line its message says +// once it is cleared: the channel keeps the one given when it is raised (novox/hq issue 497); "" for none. +type Notifier func(key, summary, headline, explanation, resolved string) error + +// Clearer tells the operator's channel that what it was told under key is over, in one resolved line. +type Clearer func(key, resolved string) error // KB is the knowledge base. type KB struct { @@ -75,6 +79,7 @@ type KB struct { Judge Judgement Asks AskBus Notify Notifier + Clear Clearer Emit func(event string, body any) error Settings func() (Settings, error) Now func() time.Time diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go index 944b54bb..1d0ad616 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/kit_test.go @@ -397,7 +397,11 @@ type world struct { router *router said *said told []string - clock time.Time + // toldKeys are the keys told under, and cleared what was cleared: key, then its resolved line. + toldKeys []string + cleared [][2]string + resolved []string // the resolved line given with each notice raised + clock time.Time } func newWorld(t *testing.T, s Store, records Records) *world { @@ -406,8 +410,12 @@ func newWorld(t *testing.T, s Store, records Records) *world { clock: time.Date(2026, 10, 11, 9, 0, 0, 0, time.UTC)} w.k = &KB{Store: s, Records: records, Reserve: w.tracker.reserve, Judge: w.judge.judge, Asks: w.router, Emit: w.said.emit, Settings: func() (Settings, error) { return Settings{}, nil }, Now: func() time.Time { return w.clock }, - Logf: func(f string, a ...any) { t.Logf(f, a...) }, - Notify: func(key, summary, headline, explanation string) error { w.told = append(w.told, summary); return nil }} + Logf: func(f string, a ...any) { t.Logf(f, a...) }, + Notify: func(key, summary, headline, explanation, resolved string) error { + w.told, w.toldKeys, w.resolved = append(w.told, summary), append(w.toldKeys, key), append(w.resolved, resolved) + return nil + }, + Clear: func(key, resolved string) error { w.cleared = append(w.cleared, [2]string{key, resolved}); return nil }} return w } diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/main.go b/modules/mesh-knowledge/cmd/mesh-knowledge/main.go index dd33011b..b2836798 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/main.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/main.go @@ -87,6 +87,7 @@ func newKB(store Store) *KB { Judge: busJudgement(os.Getenv("MESH_NODE")), Asks: sdkAskBus{}, Notify: busNotifier, + Clear: busClearer, Emit: func(event string, body any) error { return stdio.Emit(event, body) }, Settings: func() (Settings, error) { return readSettings(file) }, Logf: logf, @@ -224,9 +225,21 @@ func busJudgement(node string) Judgement { } // busNotifier tells the operator's channel, as a warning: it is the one severity below urgent. -func busNotifier(key, summary, headline, explanation string) error { +func busNotifier(key, summary, headline, explanation, resolved string) error { + body := map[string]any{"key": key, "subject": "the knowledge base", + "severity": "warning", "summary": summary, "headline": headline, "explanation": explanation} + if resolved != "" { + body["resolved"] = resolved + } + _, err := stdio.Ask("seat:operator-channel.notify", body) + return err +} + +// busClearer tells the operator's channel that what it was told under key is over: the message is edited to say +// resolved (novox/hq issue 497). +func busClearer(key, resolved string) error { _, err := stdio.Ask("seat:operator-channel.notify", map[string]any{"key": key, "subject": "the knowledge base", - "severity": "warning", "summary": summary, "headline": headline, "explanation": explanation}) + "clear": true, "resolved": resolved}) return err } @@ -329,6 +342,20 @@ func (l *lazyStore) Told(ctx context.Context, topic string) (bool, error) { } return s.Told(ctx, topic) } +func (l *lazyStore) ToldOf(ctx context.Context, prefix string) ([]string, error) { + s, err := l.store(ctx) + if err != nil { + return nil, err + } + return s.ToldOf(ctx, prefix) +} +func (l *lazyStore) Untold(ctx context.Context, key string) error { + s, err := l.store(ctx) + if err != nil { + return err + } + return s.Untold(ctx, key) +} // ---- prepare, check, dry-run -------------------------------------------------------------------------------- diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go b/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go index 11c5798c..bc0df747 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/operator.go @@ -141,13 +141,8 @@ func (k *KB) askOperator(ctx context.Context, held *Entry, w *Record, by string, } unasked := func(why string) (string, error) { w.Ask = nil - w.Judging = &Judging{State: JudgingFailed, Tries: tries + 1, Reason: why, At: k.now()} - said := fmt.Sprintf("approved by the knowledge base's review, and %s (try %d of %d); it stays proposed", why, tries+1, MostReviewTries) - if tries+1 >= MostReviewTries { - said += ", and its author proposes it again to have it reviewed again" - } else { - said += " and is reviewed again after a pause" - } + w.Judging = failedJudging(tries+1, why, k.now()) + said := fmt.Sprintf("approved by the knowledge base's review, and %s (%s", why, w.Judging.owedSays()) e, err := k.Store.Get(ctx, w.Key()) if err != nil || e == nil { return "", fmt.Errorf("%s: %s, and that could not be kept (%v)", w.Key(), said, err) diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/pgstore.go b/modules/mesh-knowledge/cmd/mesh-knowledge/pgstore.go index c5c55204..6f141c8f 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/pgstore.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/pgstore.go @@ -549,6 +549,29 @@ func (s *PgStore) Findings(ctx context.Context) ([]Finding, error) { return out, rows.Err() } +func (s *PgStore) ToldOf(ctx context.Context, prefix string) ([]string, error) { + // left() and not like: a prefix may hold an underscore, which like would take for any character. + rows, err := s.pool.Query(ctx, `select topic from told where left(topic, length($1)) = $1 order by topic`, prefix) + if err != nil { + return nil, err + } + defer rows.Close() + var out []string + for rows.Next() { + var k string + if err := rows.Scan(&k); err != nil { + return nil, err + } + out = append(out, k) + } + return out, rows.Err() +} + +func (s *PgStore) Untold(ctx context.Context, key string) error { + _, err := s.pool.Exec(ctx, `delete from told where topic = $1`, key) + return err +} + func (s *PgStore) Told(ctx context.Context, topic string) (bool, error) { tag, err := s.pool.Exec(ctx, `insert into told (topic) values ($1) on conflict do nothing`, topic) if err != nil { diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go index 3b63fe73..4a58c4ce 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/review.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review.go @@ -9,8 +9,11 @@ package main // - On changes the record stays proposed, the findings kept with the version they judged, and its author revises // it with a new version, which is reviewed again. // - A judgement that cannot be had — the judge unreachable, or its answer unreadable — leaves the record proposed -// and says so on it. It is never taken as an approval. It is asked again after a pause, up to MostReviewTries; -// after that the record says its author proposes it again to have it reviewed again. +// and says so on it. It is never taken as an approval. It is owed: the try count and when the next try is due +// are kept with the version in the store, so the background work takes it up at that time, also after a restart +// of the tool runner (novox/hq issue 497), up to MostReviewTries. After the last try the record says so, its +// author proposes it again to have it reviewed again, the module's health names it, and the operator is told +// once on the operator's channel. // - A record with a version whose writer the runtime did not prove is not reviewed: who wrote it cannot be told, // and no one but its proven author revises it, so its author proposes it again under a new number. // - On approve, a record that does not need the operator is accepted at once; one its author marked @@ -35,6 +38,41 @@ type Judging struct { Tries int `json:"tries,omitempty"` Reason string `json:"reason,omitempty"` At time.Time `json:"at,omitempty"` + // Due is when a failed review is asked again; zero after its last try. It is kept in the store with the + // version, so a restart of the tool runner neither loses the try nor asks it early (novox/hq issue 497). + Due time.Time `json:"due,omitempty"` +} + +// ReviewPause is the pause after a failed try of a review, times the tries so far. +const ReviewPause = 10 * time.Minute + +// failedJudging is a review that could not be had, the tries'th time: owed again at a time it keeps, unless that +// was its last try. +func failedJudging(tries int, reason string, now time.Time) *Judging { + j := &Judging{State: JudgingFailed, Tries: tries, Reason: reason, At: now} + if tries < MostReviewTries { + j.Due = now.Add(time.Duration(tries) * ReviewPause) + } + return j +} + +// due is when a failed review is asked again. A version kept before the time was kept (novox/hq issue 497) has +// none, and is due after the same pause from its last try. +func (j *Judging) due() time.Time { + if !j.Due.IsZero() { + return j.Due + } + return j.At.Add(time.Duration(j.Tries) * ReviewPause) +} + +// owedSays is what a failed try says of what follows it: when it is asked again, or that it was the last. +func (j *Judging) owedSays() string { + if j.Tries >= MostReviewTries { + return fmt.Sprintf("try %d of %d, the last); it stays proposed and nothing of it is in force, the operator is told, "+ + "and its author proposes it again to have it reviewed again", j.Tries, MostReviewTries) + } + return fmt.Sprintf("try %d of %d); it stays proposed, and it is asked again at %s", j.Tries, MostReviewTries, + j.due().Local().Format("2006-01-02 15:04 MST")) } // The states of Judging. @@ -162,7 +200,7 @@ func (k *KB) reviewDue(e *Entry) bool { case JudgingDue: return true case JudgingFailed: - return w.Judging.Tries < MostReviewTries && k.now().Sub(w.Judging.At) >= time.Duration(w.Judging.Tries)*10*time.Minute + return w.Judging.Tries < MostReviewTries && !k.now().Before(w.Judging.due()) } return false } @@ -217,7 +255,12 @@ func (k *KB) ReviewStep() (string, error) { if err != nil { return "", err } + k.clearUnreviewed(all) for _, e := range all { + if ranOutOfTries(e.Working()) { + k.tellUnreviewed(e) + continue + } if !k.reviewDue(e) || k.heldBack(e) { continue } @@ -240,13 +283,119 @@ func Unreviewed(all []*Entry) []string { var out []string for _, e := range all { w := e.Working() - if w.Status == StatusProposed && w.Judging != nil && w.Judging.State == JudgingFailed && w.Judging.Tries >= MostReviewTries { + if ranOutOfTries(w) { out = append(out, fmt.Sprintf("%s (%s)", w.Key(), w.Judging.Reason)) } } return out } +func ranOutOfTries(w *Record) bool { + return w.Status == StatusProposed && w.Judging != nil && w.Judging.State == JudgingFailed && w.Judging.Tries >= MostReviewTries +} + +// unreviewedPrefix starts the key of every notice of a proposal that ran out of review tries. The notice's key is +// unreviewedPrefix, the record and the version that recorded its last try (knowledge.unreviewed.decision-294.v9), so +// a later version's notice is its own; what the store keeps as told is that key, a space and the last failed try's +// time, which names the run of tries whatever versions are written after it (a link change writes one). +const unreviewedPrefix = "knowledge.unreviewed." + +func unreviewedOf(k Key) string { return fmt.Sprintf("%s%s-%d.", unreviewedPrefix, k.Kind, k.Number) } + +// tellUnreviewed tells the operator, once for each run of tries, that a proposal ran out of review tries: a history +// line alone is read by nobody, and a proposal never reviewed must not wait in silence (novox/hq issue 497). +func (k *KB) tellUnreviewed(e *Entry) { + if k.Notify == nil { + return + } + ctx := context.Background() + w := e.Working() + at := w.Judging.At.UTC().Format(time.RFC3339Nano) + told, err := k.Store.ToldOf(ctx, unreviewedOf(w.Key())) + if err != nil { + k.logf("[mesh-knowledge] whether the operator was told that %s was never reviewed cannot be read: %v", w.Key(), err) + return + } + for _, t := range told { + if strings.HasSuffix(t, " "+at) { + return + } + } + version := e.CurrentVersion + if e.Pending != nil { + version = e.PendingVersion + } + key := fmt.Sprintf("%sv%d", unreviewedOf(w.Key()), version) + // Kept as told before the notice is sent, as for drafted clauses: a notice that fails is logged and not sent + // again, and the module's health still names the record, so the mesh's condition stands for it. + if first, err := k.Store.Told(ctx, key+" "+at); err != nil || !first { + return + } + summary := fmt.Sprintf("%s was never reviewed: its review could not be had %d times (last: %s). It stays proposed, and nothing of it is in force.", + w.Key(), w.Judging.Tries, w.Judging.Reason) + explanation := "No review that could not be had counts as an approval. Its author proposes it again to have it reviewed again; " + + "until then the knowledge base's health names it. The knowledge base's get and history show it." + // The channel edits a cleared message with the resolved line given here, not the one sent with the clear. + resolved := w.Key().String() + " no longer waits for its review" + if err := k.Notify(key, summary, "A proposal was never reviewed: "+w.Key().String(), explanation, resolved); err != nil { + k.logf("[mesh-knowledge] the operator could not be told that %s was never reviewed: %v; the module's health still names it", w.Key(), err) + } +} + +// clearUnreviewed clears every notice of a proposal that ran out of review tries once the record has left that +// state — proposed again, accepted, withdrawn or superseded — so the operator's message never keeps saying it waits +// (novox/hq issue 497). A clear that fails is kept as told and sent again at the next step. +func (k *KB) clearUnreviewed(all []*Entry) { + if k.Clear == nil { + return + } + ctx := context.Background() + told, err := k.Store.ToldOf(ctx, unreviewedPrefix) + if err != nil { + k.logf("[mesh-knowledge] the notices of proposals never reviewed cannot be read: %v", err) + return + } + byKey := map[string]*Entry{} + for _, e := range all { + byKey[unreviewedOf(e.Current.Key())] = e + } + for _, t := range told { + key, at, ok := strings.Cut(t, " ") + if !ok { + continue + } + var e *Entry + for prefix, x := range byKey { + if strings.HasPrefix(key, prefix) { + e = x + break + } + } + resolved := strings.TrimPrefix(key, unreviewedPrefix) + " is no longer held by the knowledge base" + if e != nil { + w := e.Working() + if ranOutOfTries(w) && w.Judging.At.UTC().Format(time.RFC3339Nano) == at { + continue + } + switch { + case w.Status == StatusProposed: + resolved = w.Key().String() + " was proposed again and is reviewed again" + case w.Accepted(): + resolved = w.Key().String() + " was reviewed and is " + w.Status + default: + resolved = w.Key().String() + " no longer waits for its review: it is " + w.Status + } + } + if err := k.Clear(key, resolved); err != nil { + k.logf("[mesh-knowledge] the notice that %s was never reviewed could not be cleared: %v; it is cleared again at the next step", key, err) + continue + } + if err := k.Store.Untold(ctx, t); err != nil { + k.logf("[mesh-knowledge] the notice %s was cleared, and that could not be kept: %v", key, err) + } + } +} + // reviewOne asks the judgement on one record's working version, and acts on its verdict. The write lock is held // only to read and to write, never across the judgement; a record revised while it was judged is judged again. func (k *KB) reviewOne(key Key) (string, error) { @@ -312,12 +461,8 @@ func (k *KB) reviewOne(key Key) (string, error) { } if err != nil { // No verdict: the record stays proposed and says so. Never an approval. - w.Judging.State, w.Judging.Reason, w.Judging.At = JudgingFailed, err.Error(), k.now() - w.Judging.Tries++ - why := fmt.Sprintf("not reviewed: %v (try %d of %d); it stays proposed", err, w.Judging.Tries, MostReviewTries) - if w.Judging.Tries >= MostReviewTries { - why += ", and its author proposes it again to have it reviewed again" - } + w.Judging = failedJudging(w.Judging.Tries+1, err.Error(), k.now()) + why := fmt.Sprintf("not reviewed: %v (%s", err, w.Judging.owedSays()) by = ReviewBy if serr := save(w, why); serr != nil { return "", fmt.Errorf("%s: %s, and that could not be kept (%v)", key, why, serr) diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go b/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go new file mode 100644 index 00000000..c094a35f --- /dev/null +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/review_owed_test.go @@ -0,0 +1,199 @@ +package main + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + "time" +) + +// novox/hq issue 497: a review that could not be had is owed. What is owed is kept in the knowledge base's store, +// with its try count and when its next try is due, so the background work takes it up after a restart of the tool +// runner as it would have without one; the history says when it is asked again, so nobody takes a pause for a +// loss; and after its last try the history says so and the operator is told once. It is never an approval. +func TestAReviewOwedOutlivesARestartAndItsLastTryIsSaid(t *testing.T) { + eachStore(t, func(t *testing.T, s Store) { + inZone(t) + w := imported(t, s) + n := proposed(t, w, "author@g14", false) + key := Key{KindDecision, n} + lastWhy := func() string { + vs, err := s.Versions(context.Background(), key) + if err != nil || len(vs) == 0 { + t.Fatalf("versions of %s: %v", key, err) + } + return vs[len(vs)-1].Why + } + var due time.Time + for try := 1; try <= MostReviewTries; try++ { + if try > 1 { + // The tool runner restarts: a new knowledge base over the same store, nothing of the old one's memory. + w = newWorld(t, s, fixture()) + w.clock = due.Add(-time.Second) + if said := w.reviewAll(t); len(said) != 0 { + t.Fatalf("try %d asked before it was due: %v", try, said) + } + w.clock = due + } + w.judge.reviewFail = true + if said := w.reviewAll(t); len(said) != 1 || !strings.Contains(said[0], "not reviewed") { + t.Fatalf("try %d after a restart was not taken up: %v", try, said) + } + e := mustGet(t, s, key) + if e.Current.Status != StatusProposed || e.Current.Judging == nil || e.Current.Judging.State != JudgingFailed || + e.Current.Judging.Tries != try || len(e.Current.Reviews) != 0 { + t.Fatalf("try %d is not counted in the store: %s, %+v", try, e.Current.Status, e.Current.Judging) + } + why := lastWhy() + if !strings.Contains(why, fmt.Sprintf("try %d of %d", try, MostReviewTries)) { + t.Fatalf("try %d: the history does not count it: %q", try, why) + } + if try < MostReviewTries { + due = w.clock.Add(time.Duration(try) * 10 * time.Minute) + when := "asked again at " + due.Local().Format("2006-01-02 15:04 MST") + if utc := due.UTC().Format("15:04"); strings.Contains(when, utc) || !strings.Contains(when, "CEST") { + t.Fatalf("the time is not said locally: %q (UTC %s)", when, utc) + } + if !strings.Contains(why, when) { + t.Fatalf("try %d: the history does not say when it is asked again (%q): %q", try, when, why) + } + if g, _ := w.k.Get(GetAsk{Number: n}); !strings.Contains(g.Review, when) { + t.Fatalf("try %d: get does not say when it is asked again (%q): %q", try, when, g.Review) + } + if len(w.told) != 0 { + t.Fatalf("try %d: the operator was told before the last try: %v", try, w.told) + } + } + } + // After its last try: the history says it was the last, and the operator is told, once. + if why := lastWhy(); !strings.Contains(why, "the last") || !strings.Contains(why, "the operator is told") { + t.Fatalf("the last try is not said in the history: %q", why) + } + if len(w.told) != 1 || !strings.Contains(w.told[0], key.String()) || !strings.Contains(w.told[0], "never reviewed") { + t.Fatalf("the operator is not told once of the last try: %v", w.told) + } + w.clock = w.clock.Add(24 * time.Hour) + if said := w.reviewAll(t); len(said) != 0 || len(w.told) != 1 { + t.Fatalf("after the last try: asked %v, told %v", said, w.told) + } + // Another restart neither asks it again nor tells the operator again. + clock := w.clock + w = newWorld(t, s, fixture()) + w.clock = clock + if said := w.reviewAll(t); len(said) != 0 || len(w.told) != 0 { + t.Fatalf("after a restart past the last try: asked %v, told %v", said, w.told) + } + // Never an approval. + if e := mustGet(t, s, key); e.Current.Status != StatusProposed || len(w.said.of(EventAccepted)) != 0 { + t.Fatalf("a review never had approved it: %s", e.Current.Status) + } + }) +} + +// novox/hq issue 497: the notice of a proposal never reviewed says it stays proposed, so it is cleared once that is +// no longer so; and a later version's run of tries is a notice of its own, told once whatever else is written. +func TestProposedAgainAfterTheLastTryClearsTheNotice(t *testing.T) { + eachStore(t, func(t *testing.T, s Store) { + w := imported(t, s) + n := proposed(t, w, "author@g14", false) + key := Key{KindDecision, n} + runOut := func(from int) { + t.Helper() + w.judge.reviewFail = true + for try := from; try <= MostReviewTries; try++ { + if said := w.reviewAll(t); len(said) != 1 { + t.Fatalf("try %d: %v", try, said) + } + w.clock = w.clock.Add(time.Duration(try) * ReviewPause) + } + } + runOut(1) + e := mustGet(t, s, key) + first := fmt.Sprintf("knowledge.unreviewed.decision-%d.v%d", n, e.CurrentVersion) + if len(w.toldKeys) != 1 || w.toldKeys[0] != first || len(w.cleared) != 0 || + w.resolved[0] != key.String()+" no longer waits for its review" { + t.Fatalf("after the last try: told %v, cleared %v", w.toldKeys, w.cleared) + } + // Another version written while it waits (as a link change on it writes one) tells nothing again. + r := e.Current.Clone() + if err := s.Save(context.Background(), Version{By: "test", At: w.clock, Why: "a link changed", Record: r}, true, false, e.Revision); err != nil { + t.Fatal(err) + } + if w.reviewAll(t); len(w.toldKeys) != 1 || len(w.cleared) != 0 { + t.Fatalf("a version written while it waits: told %v, cleared %v", w.toldKeys, w.cleared) + } + // Proposed again: the notice is cleared, saying so. + if _, err := w.k.Propose("author@g14", valid(n)); err != nil { + t.Fatal(err) + } + w.clock = w.clock.Add(time.Second) + w.reviewAll(t) // its try 1, which fails, and the clearing + w.clock = w.clock.Add(ReviewPause) + if len(w.cleared) != 1 || w.cleared[0][0] != first || !strings.Contains(w.cleared[0][1], "proposed again") { + t.Fatalf("proposed again: cleared %v", w.cleared) + } + // Its own run of tries out is a notice of its own, under its own version. + runOut(2) + if len(w.toldKeys) != 2 || w.toldKeys[1] == first || len(w.cleared) != 1 { + t.Fatalf("a later version ran out: told %v, cleared %v", w.toldKeys, w.cleared) + } + // Proposed again and accepted at last: that notice is cleared too, and nothing is cleared twice, also after + // a restart. + w.judge.reviewFail = false + w.accepted(t, "author@g14", valid(n)) + if len(w.cleared) != 2 || w.cleared[1][0] != w.toldKeys[1] || !strings.Contains(w.cleared[1][1], "proposed again") { + t.Fatalf("accepted: cleared %v", w.cleared) + } + clock := w.clock + w = newWorld(t, s, fixture()) + w.clock = clock + if w.reviewAll(t); len(w.cleared) != 0 || len(w.told) != 0 { + t.Fatalf("after a restart: told %v, cleared %v", w.told, w.cleared) + } + }) +} + +// inZone sets the local zone to one that is not UTC for one test, so a time said locally is proven to be, wherever +// the test runs. +func inZone(t *testing.T) { + t.Helper() + was := time.Local + time.Local = time.FixedZone("CEST", 2*60*60) + t.Cleanup(func() { time.Local = was }) +} + +// novox/hq issue 497: a clear the operator's channel refused keeps its notice as told, and is sent again at the next +// step until it is taken. +func TestAClearThatFailsIsSentAgain(t *testing.T) { + eachStore(t, func(t *testing.T, s Store) { + w := imported(t, s) + n := proposed(t, w, "author@g14", false) + w.judge.reviewFail = true + for try := 1; try <= MostReviewTries; try++ { + w.reviewAll(t) + w.clock = w.clock.Add(time.Duration(try) * ReviewPause) + } + if len(w.toldKeys) != 1 { + t.Fatalf("after the last try: told %v", w.toldKeys) + } + prefix := fmt.Sprintf("knowledge.unreviewed.decision-%d.", n) + refused := 0 + w.k.Clear = func(key, resolved string) error { refused++; return errors.New("the channel is down") } + if _, err := w.k.Propose("author@g14", valid(n)); err != nil { + t.Fatal(err) + } + w.reviewAll(t) + if told, err := s.ToldOf(context.Background(), prefix); err != nil || len(told) != 1 || refused == 0 { + t.Fatalf("a refused clear: told %v (%v), refused %d", told, err, refused) + } + w.k.Clear = func(key, resolved string) error { w.cleared = append(w.cleared, [2]string{key, resolved}); return nil } + w.clock = w.clock.Add(time.Second) + w.reviewAll(t) + if told, err := s.ToldOf(context.Background(), prefix); err != nil || len(told) != 0 || len(w.cleared) != 1 || + w.cleared[0][0] != w.toldKeys[0] { + t.Fatalf("the next step: told %v (%v), cleared %v", told, err, w.cleared) + } + }) +} diff --git a/modules/mesh-knowledge/cmd/mesh-knowledge/store.go b/modules/mesh-knowledge/cmd/mesh-knowledge/store.go index bbba2dbe..9dcc30e1 100644 --- a/modules/mesh-knowledge/cmd/mesh-knowledge/store.go +++ b/modules/mesh-knowledge/cmd/mesh-knowledge/store.go @@ -63,8 +63,12 @@ type Store interface { // SetFindings replaces the whole-set findings; Findings reads them. SetFindings(ctx context.Context, f []Finding) error Findings(ctx context.Context) ([]Finding, error) - // Told records that the operator was told about a topic's clauses, once; false when it was already. + // Told records that the operator was told of something, once, by its key: a topic's clauses drafted, or a + // version that ran out of review tries (novox/hq issue 497); false when it was already. Told(ctx context.Context, topic string) (bool, error) + // ToldOf is every key told of that starts with prefix; Untold takes one away once what it told of is over. + ToldOf(ctx context.Context, prefix string) ([]string, error) + Untold(ctx context.Context, key string) error } // memStore is the Store in memory. @@ -271,6 +275,26 @@ func (s *memStore) Findings(context.Context) ([]Finding, error) { return append([]Finding(nil), s.findings...), nil } +func (s *memStore) ToldOf(_ context.Context, prefix string) ([]string, error) { + s.mu.Lock() + defer s.mu.Unlock() + var out []string + for k := range s.told { + if strings.HasPrefix(k, prefix) { + out = append(out, k) + } + } + sort.Strings(out) + return out, nil +} + +func (s *memStore) Untold(_ context.Context, key string) error { + s.mu.Lock() + defer s.mu.Unlock() + delete(s.told, key) + return nil +} + func (s *memStore) Told(_ context.Context, topic string) (bool, error) { s.mu.Lock() defer s.mu.Unlock()