From e7888024a8264f5d1e6ac46a2e9cc0d112ff60be Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 28 Oct 2026 20:39:50 +0100 Subject: [PATCH] Checker: delete a check's asks key at its end, so the state asks holds only the checks that run Re-review of fix/lost-check: writing an empty list at the end kept one key for every verdict key ever checked. Tests: every row of the lost-ask table proves the key gone at the end; one row more (deaf after its build module replaced it, asked again by hand); counts unchanged. --- .../cmd/mesh-delivery-checker/bus.go | 8 ++++++++ .../internal/check/askagain.go | 19 +++++++++++++++---- .../internal/check/checker.go | 4 ++-- .../internal/check/fakes_test.go | 8 ++++++++ .../internal/check/lostask_test.go | 4 ++++ 5 files changed, 37 insertions(+), 6 deletions(-) diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/bus.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/bus.go index 13f2089..a5453a6 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/bus.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/bus.go @@ -175,3 +175,11 @@ func (a awaitedKV) Keys(ctx context.Context) ([]string, error) { } return s.Keys(ctx) } + +func (a awaitedKV) Delete(ctx context.Context, key string) error { + s, err := a.kv() + if err != nil { + return err + } + return s.Delete(ctx, key) +} diff --git a/modules/mesh-delivery-checker/internal/check/askagain.go b/modules/mesh-delivery-checker/internal/check/askagain.go index e4a3004..7540bbe 100644 --- a/modules/mesh-delivery-checker/internal/check/askagain.go +++ b/modules/mesh-delivery-checker/internal/check/askagain.go @@ -26,20 +26,31 @@ const ( AsksBucket = delivery.CheckerModule + "_" + AsksState ) -// CheckAsks are the asks a check put, in order, as the state asks keeps them; none once it ended. +// CheckAsks are the asks a check put, in order, as the state asks keeps them while it runs; its end deletes the key. type CheckAsks struct { Asks []string `json:"asks"` } +// AsksKV is the state asks: a KV whose key a check's end deletes, so the state holds only the checks that run. +type AsksKV interface { + KV + Delete(ctx context.Context, key string) error +} + +// forgetAsks deletes the asks a check put at its end; one that cannot be deleted is said, and the next check of the same +// heads writes the key anew. +func (ch *Checker) forgetAsks(ctx context.Context, key string) { + if err := ch.Asks.Delete(ctx, key); err != nil { + ch.logf("%s: the asks it put could not be forgotten at its end: %v", key, err) + } +} + // recordAsks keeps the asks a check put under its verdict key; nil Asks keeps nothing. A record that cannot be written // is said: checker_ask_again then refuses the check, as it cannot tell whether its ask waits in a stream. func (ch *Checker) recordAsks(ctx context.Context, key string, asks []string) { if ch.Asks == nil { return } - if asks == nil { - asks = []string{} - } data, err := json.Marshal(CheckAsks{Asks: asks}) if err == nil { _, err = putOnce(ctx, ch.Asks, key, data, func(cur []byte) bool { return string(cur) == string(data) }) diff --git a/modules/mesh-delivery-checker/internal/check/checker.go b/modules/mesh-delivery-checker/internal/check/checker.go index 2dd21ce..da952d3 100644 --- a/modules/mesh-delivery-checker/internal/check/checker.go +++ b/modules/mesh-delivery-checker/internal/check/checker.go @@ -80,7 +80,7 @@ type Checker struct { // Bound is how long one check may run; none when 0 (DefaultBound). Bound time.Duration // Asks keeps the asks each running check put, by verdict key (AsksState), for checker_ask_again; nil keeps none. - Asks KV + Asks AsksKV // Link is the base of the address a status points at; the verdict key follows. Link string // Announce says a status was written (EventOwedWritten); nil says nothing. @@ -723,7 +723,7 @@ func (ch *Checker) try(ctx, run context.Context, key string, cs *Case) { ch.recordAsks(ctx, cs.Key, cs.Asks()) } } - defer ch.recordAsks(ctx, cs.Key, nil) + defer ch.forgetAsks(ctx, cs.Key) } ch.writeOwed(ctx, cs, delivery.Verdict{State: delivery.VerdictRunning}, Outcome{}) out := ch.judge(run, cs) diff --git a/modules/mesh-delivery-checker/internal/check/fakes_test.go b/modules/mesh-delivery-checker/internal/check/fakes_test.go index 2965b72..d53f2a1 100644 --- a/modules/mesh-delivery-checker/internal/check/fakes_test.go +++ b/modules/mesh-delivery-checker/internal/check/fakes_test.go @@ -60,3 +60,11 @@ func (m *memKV) Keys(context.Context) ([]string, error) { sort.Strings(keys) return keys, nil } + +func (m *memKV) Delete(_ context.Context, key string) error { + m.mu.Lock() + defer m.mu.Unlock() + delete(m.vals, key) + delete(m.revs, key) + return nil +} diff --git a/modules/mesh-delivery-checker/internal/check/lostask_test.go b/modules/mesh-delivery-checker/internal/check/lostask_test.go index 2df4431..ddf4d33 100644 --- a/modules/mesh-delivery-checker/internal/check/lostask_test.go +++ b/modules/mesh-delivery-checker/internal/check/lostask_test.go @@ -265,6 +265,7 @@ func TestACheckWhoseAskJudgedNothingIsAskedAgainOrRead(t *testing.T) { {name: "replaced under every ask", restarts: LostAskTries, state: delivery.VerdictFail, asks: LostAskTries}, {name: "its watch deaf, a step of its build module ends", deaf: 1, how: "nudge", state: delivery.VerdictPass, asks: 1}, {name: "its watch deaf, asked again by hand", deaf: 1, how: "hand", state: delivery.VerdictPass, asks: 2}, + {name: "its watch deaf, asked again by hand after its build module replaced it", deaf: 1, restarts: 1, how: "hand", state: delivery.VerdictPass, asks: 2}, } { t.Run(row.name, func(t *testing.T) { b := newBench(t) @@ -329,6 +330,9 @@ func TestACheckWhoseAskJudgedNothingIsAskedAgainOrRead(t *testing.T) { if v.State != row.state || asked() != row.asks { t.Errorf("verdict %s (%s) after %d asks, want %s after %d", v.State, v.Failed.Words, asked(), row.state, row.asks) } + if _, _, kept, _ := asksKept.Get(ctx, key); kept { + t.Errorf("the asks of the ended check are still kept under %s", key) + } if row.state == delivery.VerdictFail && (!IsLostAsk(v) || !IsNodesFailure(v)) { t.Errorf("a check that judged nothing every time = %q, want lost and judged again when the nodes change", v.Failed.Words) }