From 68fedded8c74dda6204c2760cc1e91df149d22b6 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 23:37:51 +0100 Subject: [PATCH] =?UTF-8?q?F1,=20F7:=20the=20asker=20drops=20an=20ask=20on?= =?UTF-8?q?ly=20on=20a=20withdrawal=20cause=20(superseded,=20taken=20out,?= =?UTF-8?q?=20asked=20again=20by=20hand),=20not=20on=20the=20check=20bound?= =?UTF-8?q?;=20the=20checker's=20drop=20on=20Forget=20is=20asserted;=20a?= =?UTF-8?q?=20step's=20own=20run=20count=20and=20a=20failed=20drop=20are?= =?UTF-8?q?=20said=20(ADR=200664=20=C2=A74,=20issue=201748=20review=201)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01JG73KEq4zoDfM9WtYpcQgZ --- .../mesh-delivery-builder/buildrun/asks.go | 32 +++++++++-- .../buildrun/asks_test.go | 28 +++++++++- .../internal/build/askagain.go | 3 + .../internal/build/run.go | 31 ++++++++++- .../internal/build/run_test.go | 55 +++++++++++++++++++ .../internal/check/askagain.go | 3 +- .../internal/check/checker.go | 4 ++ .../internal/check/checker_test.go | 21 +++++++ 8 files changed, 168 insertions(+), 9 deletions(-) diff --git a/modules/mesh-delivery-builder/buildrun/asks.go b/modules/mesh-delivery-builder/buildrun/asks.go index f96af4d5..55c51534 100644 --- a/modules/mesh-delivery-builder/buildrun/asks.go +++ b/modules/mesh-delivery-builder/buildrun/asks.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log" "strings" "sync" "time" @@ -42,8 +43,18 @@ type Asker struct { // again that the bus did not give within its bound, or a wait ended with a cause its caller names (a superseded // fingerprint). Never a wait ended by a shutdown (a plain cancel): the ask is then taken over by the next run. Dropper AskDropper + // Logf says a drop that failed (the log's Printf when unset): the ask then stays on its stream, and it is said. + Logf func(format string, args ...any) } +// Withdrawn is the cause a caller gives the context of an ask's wait to withdraw the ask (ADR 0664 §4): a build superseded by a +// newer fingerprint, a candidate taken out, an ask asked again by hand. Asker.Wait drops the ask on this cause alone: a +// plain cancel (a shutdown) and any other cause (a bound passing, a check of whether the ask is gone) leave it on its stream +// for the next run. A cause that wraps a Withdrawn (errors.As) withdraws too. +type Withdrawn struct{ Why string } + +func (w Withdrawn) Error() string { return w.Why } + // AskDropper ends an ask: the seat's verb drop, served by any holder of the build seat (no node is named). It writes the // ask in the build module's shared state of pauses, so every node refuses it when the stream gives it, and the node that // runs it ends the step (ADR 0664 §5). @@ -71,14 +82,20 @@ func (d SeatDrops) DropAsk(_ context.Context, a buildstep.StepAsk, why string) e return nil } -// dropped ends a without its end. A failed drop is not the wait's error: the wait says why it ended. +// dropped ends a without its end. A failed drop is not the wait's error, but it is said (Logf). func (k Asker) dropped(a buildstep.StepAsk, why string) { if k.Dropper == nil { return } ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - _ = k.Dropper.DropAsk(ctx, a, why) + if err := k.Dropper.DropAsk(ctx, a, why); err != nil { + say := k.Logf + if say == nil { + say = log.Printf + } + say("the ask %s was not dropped (%s), it stays on its stream: %v", a.ID, why, err) + } } // Put puts a on its stream: the seat's accept subject of its language and kind. @@ -133,8 +150,9 @@ func (k Asker) Wait(ctx context.Context, a buildstep.StepAsk) (buildstep.Step, e for { select { case <-ctx.Done(): - if cause := context.Cause(ctx); cause != nil && !errors.Is(cause, context.Canceled) { - k.dropped(a, cause.Error()) + var w Withdrawn + if errors.As(context.Cause(ctx), &w) { + k.dropped(a, context.Cause(ctx).Error()) } return buildstep.Step{}, ctx.Err() case <-bound: @@ -167,8 +185,10 @@ func (k Asker) Wait(ctx context.Context, a buildstep.StepAsk) (buildstep.Step, e continue } if buildstep.Ended(step) { - if step.Passing && step.State == buildstep.StepFailed && len(agains) > 0 { - step.Reason = fmt.Sprintf("a transport fault on %d runs (%s): %s", len(agains)+1, strings.Join(append(agains, step.Node), ", "), step.Reason) + if step.Passing && step.State == buildstep.StepFailed && (len(agains) > 0 || step.Run > 1) { + // The step's own count of runs (ADR 0664 §2), not only the ones this wait saw: a builder that started again + // reads the key's last value and none of the runs before it. + step.Reason = fmt.Sprintf("a transport fault on %d runs (%s): %s", max(step.Run, len(agains)+1), strings.Join(append(agains, step.Node), ", "), step.Reason) } return step, nil } diff --git a/modules/mesh-delivery-builder/buildrun/asks_test.go b/modules/mesh-delivery-builder/buildrun/asks_test.go index b80fc1ab..29406198 100644 --- a/modules/mesh-delivery-builder/buildrun/asks_test.go +++ b/modules/mesh-delivery-builder/buildrun/asks_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "strings" "sync" "testing" @@ -79,15 +80,33 @@ func TestAnAskerPutsOnceAndReadsTheStep(t *testing.T) { (Asker{Publisher: h, Waiter: h, Bound: 20 * time.Millisecond, Dropper: drops}).Wait(context.Background(), goAsk("mvl0frv18d1974")) named, stop := context.WithCancelCause(context.Background()) h.Write(buildstep.Step{ID: "mvl0frv18d1975", Node: "g14", State: buildstep.StepRunning, Started: time.Now()}) - seenStep := WithStepSeen(named, func(buildstep.Step) { stop(errors.New("superseded by a newer fingerprint")) }) + seenStep := WithStepSeen(named, func(buildstep.Step) { stop(Withdrawn{Why: "superseded by a newer fingerprint"}) }) (Asker{Publisher: h, Waiter: h, Bound: time.Hour, Dropper: drops}).Wait(seenStep, goAsk("mvl0frv18d1975")) shut, off := context.WithCancel(context.Background()) off() (Asker{Publisher: h, Waiter: h, Bound: time.Hour, Dropper: drops}).Wait(shut, goAsk("mvl0frv18d1977")) (Asker{Publisher: &FakeAsks{}, Waiter: &FakeAsks{}, Bound: time.Hour, Dropper: drops}).Wait(named, goAsk("mvl0frv18d1976")) + // Review 1 (finding 1): a cause that is no withdrawal (the builder's check bound passing, which waits on) drops nothing. + bounded, passed := context.WithCancelCause(context.Background()) + passed(errors.New("the check bound passed")) + (Asker{Publisher: &FakeAsks{}, Waiter: &FakeAsks{}, Bound: time.Hour, Dropper: drops}).Wait(bounded, goAsk("mvl0frv18d1978")) if got := strings.Join(drops.got, "|"); !strings.Contains(got, "mvl0frv18d1974") || !strings.Contains(got, "mvl0frv18d1975 superseded by a newer fingerprint") || !strings.Contains(got, "mvl0frv18d1976 superseded by a newer fingerprint") || len(drops.got) != 3 { t.Errorf("drops %q, want the ask never given again, the superseded one and the one no node took, and no other", got) } + // Review 1: a wait that starts after the first runs (a builder started again) reads the last value only, and its end still says + // how many runs; a drop that failed is said, not discarded. + late := &FakeAsks{} + late.Write(buildstep.Step{ID: "mvl0frv18d1979", Node: "shanks", State: buildstep.StepFailed, Passing: true, Run: 3, Reason: "lookup proxy: i/o timeout"}) + if st, err := (Asker{Publisher: late, Waiter: late, Bound: time.Minute}).Wait(context.Background(), goAsk("mvl0frv18d1979")); err != nil || !strings.Contains(st.Reason, "a transport fault on 3 runs") { + t.Errorf("a failed step of run 3 read alone says %q (%v), want it names 3 runs", st.Reason, err) + } + var said []string + bad, off2 := context.WithCancelCause(context.Background()) + off2(Withdrawn{Why: "withdrawn"}) + (Asker{Publisher: &FakeAsks{}, Waiter: &FakeAsks{}, Bound: time.Hour, Dropper: failingDrops{}, Logf: func(f string, a ...any) { said = append(said, fmt.Sprintf(f, a...)) }}).Wait(bad, goAsk("mvl0frv18d1980")) + if len(said) != 1 || !strings.Contains(said[0], "mvl0frv18d1980") || !strings.Contains(said[0], "no holder") { + t.Errorf("a drop that failed said %q, want it named with its cause", said) + } // review round 2 item 1: an ask that waits in its stream for a free slot is not bounded; once a node writes its step // running, the step is bounded from its start, and the error names the node. g := &FakeAsks{} @@ -294,3 +313,10 @@ func (d *seatDrops) DropAsk(_ context.Context, a buildstep.StepAsk, why string) d.got = append(d.got, strings.TrimSpace(a.ID+" "+why)) return nil } + +// failingDrops is an AskDropper whose verb is not answered. +type failingDrops struct{} + +func (failingDrops) DropAsk(context.Context, buildstep.StepAsk, string) error { + return errors.New("no holder answered") +} diff --git a/modules/mesh-delivery-builder/internal/build/askagain.go b/modules/mesh-delivery-builder/internal/build/askagain.go index c7c3a73b..9c3ab4b1 100644 --- a/modules/mesh-delivery-builder/internal/build/askagain.go +++ b/modules/mesh-delivery-builder/internal/build/askagain.go @@ -301,6 +301,9 @@ type byHand struct{ why string } func (h byHand) Error() string { return "asked again by hand: " + h.why } +// Unwrap makes the ask asked again by hand a withdrawal of the old ask: its asker drops it (ADR 0664 §4). +func (h byHand) Unwrap() error { return buildrun.Withdrawn{Why: h.Error()} } + // answer waits for the step of the lane's ask, and asks it again while it can no longer be answered (Gone) or a person // asked it again (AskAgain); it answers the step and the ask that has it. func (b *Builder) answer(ctx context.Context, l *lane, ask sdkstep.StepAsk, asked *askNotes, module, fp string) (sdkstep.Step, sdkstep.StepAsk, error) { diff --git a/modules/mesh-delivery-builder/internal/build/run.go b/modules/mesh-delivery-builder/internal/build/run.go index 4d38f634..4d1b3b51 100644 --- a/modules/mesh-delivery-builder/internal/build/run.go +++ b/modules/mesh-delivery-builder/internal/build/run.go @@ -121,6 +121,9 @@ type Builder struct { waiting map[string]map[string]chan string // hands are the requests to build a key again by hand that wait for the claim and its asks (BuildAgain), by build key. hands map[string]*handWatch + // builds are the ends that withdraw the asks of a key's build in flight, by build key (ADR 0664 §4): a newer fingerprint of + // the module takes its older one's build out, and the asker drops that build's asks. + builds map[string]context.CancelCauseFunc } type planned struct { @@ -139,6 +142,7 @@ func (b *Builder) init() { b.known, b.inflight, b.again, b.plans, b.tried = map[string]meshcatalog.ModulesByModuleValue{}, map[string]bool{}, map[string]meshcatalog.ModulesByModuleValue{}, map[string]planned{}, map[string]bool{} b.published, b.held, b.waiting = map[string]bool{}, map[string]bool{}, map[string]map[string]chan string{} b.hands = map[string]*handWatch{} + b.builds = map[string]context.CancelCauseFunc{} }) } @@ -161,13 +165,25 @@ func (b *Builder) Module(ctx context.Context, key string, value []byte) { return } b.mu.Lock() + old, had := b.known[key] b.known[key] = m + var superseded context.CancelCauseFunc + if had && old.Fp2 != "" && old.Fp2 != m.Fp2 { + if ok, err := builds.BuilderBuildKey(key, old.Fp2); err == nil { + superseded = b.builds[ok] + } + } if bk, err := builds.BuilderBuildKey(key, m.Fp2); err == nil && m.Fp2 != "" { // A fingerprint the trunk carries is built from the trunk's commit, whoever asked for it first. b.trunkFP[bk] = true delete(b.via, bk) } b.mu.Unlock() + if superseded != nil { + // A newer fingerprint of the module: the build of the older one in flight is taken out, and its asks are withdrawn + // from the build seats (ADR 0664 §4). + superseded(buildrun.Withdrawn{Why: "the build of " + key + " at " + old.Fp2 + " was superseded by the fingerprint " + m.Fp2}) + } b.consider(ctx, key, m) } @@ -498,7 +514,20 @@ func (b *Builder) try(ctx context.Context, module string, m meshcatalog.ModulesB // any other, so the controller has a done key to register it from (issue 1613, ADR 0478 clause 1, ADR 0501 clause 1). result = builds.BuilderBuild{State: builds.BuildDone, Step: "nothing to build: the manifest names no artifact"} } else { - result = b.build(ctx, module, m, plan, planErr, prior, only, b.notes(cl)) + run, withdraw := context.WithCancelCause(ctx) + b.mu.Lock() + b.builds[key] = withdraw + b.mu.Unlock() + result = b.build(run, module, m, plan, planErr, prior, only, b.notes(cl)) + b.mu.Lock() + delete(b.builds, key) + b.mu.Unlock() + withdraw(nil) + var w buildrun.Withdrawn + if ctx.Err() == nil && errors.As(context.Cause(run), &w) { + // Taken out, not stopped: the key says so, and the asks are withdrawn (the asker dropped them). + result = builds.BuilderBuild{State: builds.BuildFailed, Error: keyError([]string{"withdrawn: " + w.Why})} + } } if ctx.Err() != nil { return diff --git a/modules/mesh-delivery-builder/internal/build/run_test.go b/modules/mesh-delivery-builder/internal/build/run_test.go index a4034eb8..b4620e5b 100644 --- a/modules/mesh-delivery-builder/internal/build/run_test.go +++ b/modules/mesh-delivery-builder/internal/build/run_test.go @@ -624,6 +624,7 @@ func TestTwoInstancesStartingTogetherEndWithOneDoneKey(t *testing.T) { // 1607/A } func TestAModulesValueThatArrivesWhileItsKeyIsBeingBuiltIsLookedAtAgainWhenItEnds(t *testing.T) { // 1607/AC6 (no event is dropped, no timer) + t.Run("a newer fingerprint withdraws the build of the older one", aNewerFingerprintWithdrawsTheBuildOfTheOlderOne) r := newRig(t, goBundle) r.bm.fails["ace/build_go_build"] = "go build of tool: boom" r.bm.hold, r.bm.entered = make(chan struct{}), make(chan struct{}, 8) @@ -640,6 +641,60 @@ func TestAModulesValueThatArrivesWhileItsKeyIsBeingBuiltIsLookedAtAgainWhenItEnd } } +// ADR 0664 §4 (issue 1748, review 1 finding 2): a newer fingerprint of a module takes the build of its older one out: the key +// says so, and the asker drops the ask the build waits on, carrying the cause. The first ask's step runs and never ends. +func aNewerFingerprintWithdrawsTheBuildOfTheOlderOne(t *testing.T) { + r := newRig(t, goBundle) + drops := &seatDrops{} + var asked int32 + running := make(chan struct{}) + r.asks.Answer = func(a buildstep.StepAsk) buildstep.Step { + if atomic.AddInt32(&asked, 1) == 1 { + defer close(running) + return buildstep.Step{ID: a.ID, Node: "ace", State: buildstep.StepRunning, Started: time.Now()} + } + return r.answer(a) + } + r.b.Asks = buildrun.Asker{Publisher: r.asks, Waiter: r.asks, Bound: 2 * time.Second, Dropper: drops} + r.b.Module(context.Background(), "m", moduleValue(t, "modules/m")) + <-running + fp2 := "src2:" + hex64('2') + data, err := meshcatalogclient.EncodeModule(meshcatalog.ModulesByModuleValue{Repository: "novox/mesh-catalog", Commit: "7a74cee83f88e6585a5511c3cc53bdf0f5303d20", Fp: "src1:" + hex64('f'), Fp2: fp2, + Manifest: mdigest('a'), Path: "modules/m", Toolchains: CatalogToolchains(agreedToolchains)}) + if err != nil { + t.Fatal(err) + } + r.b.Module(context.Background(), "m", data) + r.b.Wait() + old := valueOf(t, r.st, r.key) + if old.State != builds.BuildFailed || !strings.Contains(old.Error, "superseded by the fingerprint "+fp2) { + t.Errorf("the older key = %+v, want failed, naming the newer fingerprint that superseded it", old) + } + first := r.asks.Asks()[0].ID + if got := drops.get(); len(got) != 1 || !strings.HasPrefix(got[0], first+" ") || !strings.Contains(got[0], "superseded by the fingerprint "+fp2) { + t.Errorf("dropped %q, want the first ask withdrawn with the cause", got) + } +} + +// seatDrops keeps the asks an Asker withdrew: " ". +type seatDrops struct { + mu sync.Mutex + got []string +} + +func (d *seatDrops) DropAsk(_ context.Context, a buildstep.StepAsk, why string) error { + d.mu.Lock() + defer d.mu.Unlock() + d.got = append(d.got, a.ID+" "+why) + return nil +} + +func (d *seatDrops) get() []string { + d.mu.Lock() + defer d.mu.Unlock() + return append([]string(nil), d.got...) +} + func TestAKeyIsBuiltOnlyWithTheToolchainItsFingerprintNamesAndNothingIsWrittenOtherwise(t *testing.T) { // 1615/AC4 r := newRig(t, goBundle) old := map[string]builds.ToolchainUsed{"go": {Version: "go1.26.0", Recipe: "1"}} diff --git a/modules/mesh-delivery-checker/internal/check/askagain.go b/modules/mesh-delivery-checker/internal/check/askagain.go index 75b7ddfa..4a9ec8c5 100644 --- a/modules/mesh-delivery-checker/internal/check/askagain.go +++ b/modules/mesh-delivery-checker/internal/check/askagain.go @@ -10,6 +10,7 @@ import ( "git.novox.be/novox/mesh-delivery/contracts/go/clients/meshdeliverycheckerclient" "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshcatalog" "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshdeliverychecker" + "git.novox.be/novox/mesh-delivery/modules/mesh-delivery-builder/buildrun" "git.novox.be/novox/mesh-delivery/sdk/buildstep" ) @@ -70,7 +71,7 @@ const AskAgainTool = "checker_ask_again" const AskedAgainWords = EndedWords + "asked again by hand (" + AskAgainTool + "): " // errAskedAgain is the cause a running check is ended with when its verdict was asked again by hand. -var errAskedAgain = errors.New("its verdict was asked again by hand (" + AskAgainTool + ")") +var errAskedAgain = buildrun.Withdrawn{Why: "its verdict was asked again by hand (" + AskAgainTool + ")"} // IsAskedAgain says whether a verdict was written by AskAgainTool. func IsAskedAgain(v meshdeliverychecker.VerdictsByGroupHeadsValue) bool { diff --git a/modules/mesh-delivery-checker/internal/check/checker.go b/modules/mesh-delivery-checker/internal/check/checker.go index ffe77839..1e583732 100644 --- a/modules/mesh-delivery-checker/internal/check/checker.go +++ b/modules/mesh-delivery-checker/internal/check/checker.go @@ -18,6 +18,7 @@ import ( "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshcatalog" "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshdelivery" "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshdeliverychecker" + "git.novox.be/novox/mesh-delivery/modules/mesh-delivery-builder/buildrun" "git.novox.be/novox/mesh-delivery/sdk/buildstep" "git.novox.be/novox/mesh-sdk/go/keys" ) @@ -177,6 +178,9 @@ func (s superseded) Error() string { return EndedWords + "superseded by " + s.by } +// Unwrap makes a check ended so a withdrawal of its asks: the asker drops what it put on the build nodes (ADR 0664 §4, §5). +func (s superseded) Unwrap() error { return buildrun.Withdrawn{Why: s.Error()} } + func (ch *Checker) init() { ch.once.Do(func() { ch.known, ch.inflight, ch.again, ch.tried = map[string]meshcatalog.CandidatesByGroupValue{}, map[string]bool{}, map[string]bool{}, map[string]bool{} diff --git a/modules/mesh-delivery-checker/internal/check/checker_test.go b/modules/mesh-delivery-checker/internal/check/checker_test.go index e33901e1..f68c1a7b 100644 --- a/modules/mesh-delivery-checker/internal/check/checker_test.go +++ b/modules/mesh-delivery-checker/internal/check/checker_test.go @@ -16,6 +16,7 @@ import ( "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshcatalog" "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshdelivery" "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshdeliverychecker" + "git.novox.be/novox/mesh-delivery/modules/mesh-delivery-builder/buildrun" "git.novox.be/novox/mesh-delivery/sdk/buildstep" "git.novox.be/novox/mesh-sdk/go/keys" ) @@ -371,14 +372,34 @@ func TestARepositoryWithNoTrunkCannotBeJudgedLoudlyUntilItsTrunkIsKnown(t *testi ch.init() run, end := context.WithCancelCause(context.Background()) ch.live["g.heads"] = liveCheck{group: "g", end: end} + // The check's asker is waiting on an ask of its run when the candidate goes (review 1, finding 7). + drops := &checkDrops{} + fake := &buildrun.FakeAsks{} + done := make(chan struct{}) + go func() { + defer close(done) + (buildrun.Asker{Publisher: fake, Waiter: fake, Bound: time.Hour, Dropper: drops}).Wait(run, buildstep.StepAsk{ID: "mvl0frv18d2000", Language: buildstep.AskGo, Kind: buildstep.AskCheck}) + }() ch.Forget("g") + <-done var s superseded if cause := context.Cause(run); !errors.As(cause, &s) || s.by != "" || errors.Is(cause, context.Canceled) { t.Errorf("the check was ended with %v, want superseded with no heads, not a plain cancel", cause) } + if len(drops.got) != 1 || !strings.Contains(drops.got[0], "mvl0frv18d2000") || !strings.Contains(drops.got[0], "no candidate holds these heads") { + t.Errorf("the asker dropped %q, want its ask withdrawn with the check's cause", drops.got) + } }) } +// checkDrops keeps the asks an asker dropped: " ". +type checkDrops struct{ got []string } + +func (d *checkDrops) DropAsk(_ context.Context, a buildstep.StepAsk, why string) error { + d.got = append(d.got, a.ID+" "+why) + return nil +} + func countWith(lines []string, words string) int { n := 0 for _, l := range lines {