From fc4f57c4b3a3d98cbbc62962afb3206a92aad458 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 23 Oct 2026 16:36:46 +0200 Subject: [PATCH] =?UTF-8?q?Restart=20an=20ask's=20bound=20at=20each=20run'?= =?UTF-8?q?s=20start,=20also=20on=20the=20same=20node,=20so=20a=20step=20r?= =?UTF-8?q?edelivered=20after=20its=20build=20module=20was=20replaced=20is?= =?UTF-8?q?=20not=20cut=20by=20the=20killed=20run's=20time=20(issue=201887?= =?UTF-8?q?,=20review:=20issue=20734=20path;=20ADR=200525=20=C2=A76)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- go/delivery/buildrun/asks.go | 9 +++++---- go/delivery/buildrun/asks_test.go | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/go/delivery/buildrun/asks.go b/go/delivery/buildrun/asks.go index 97607fe..59d99d5 100644 --- a/go/delivery/buildrun/asks.go +++ b/go/delivery/buildrun/asks.go @@ -75,7 +75,7 @@ func (k Asker) Wait(ctx context.Context, a delivery.StepAsk) (delivery.Step, err timer.Stop() } }() - running := "" + running, since := "", time.Time{} for { select { case <-ctx.Done(): @@ -89,11 +89,12 @@ func (k Asker) Wait(ctx context.Context, a delivery.StepAsk) (delivery.Step, err if step.State != delivery.StepRunning { return step, nil } - if k.Bound <= 0 || step.Node == running { + if k.Bound <= 0 || (step.Node == running && step.Started.Equal(since)) { continue } - // A node took the ask (or another took it after the first stopped): its step is bounded from its start. - running = step.Node + // A node took the ask, or took it again after its run stopped (another node, or the same node's build module + // replaced mid-run: a new start): its step is bounded from its own start. + running, since = step.Node, step.Started left := k.Bound if !step.Started.IsZero() { left = time.Until(step.Started.Add(k.Bound)) diff --git a/go/delivery/buildrun/asks_test.go b/go/delivery/buildrun/asks_test.go index 9432242..3365202 100644 --- a/go/delivery/buildrun/asks_test.go +++ b/go/delivery/buildrun/asks_test.go @@ -49,3 +49,23 @@ func TestAnAskerPutsOnceAndReadsTheStep(t *testing.T) { t.Fatal("a wait put an ask") } } + +// Review (issue 734 path): an ask taken again by the same node (its build module replaced mid-run) is bounded from the +// new run's start, so its pass is answered after the first run's bound would have ended the wait. +func TestARedeliveryToTheSameNodeRestartsTheBound(t *testing.T) { + f := &FakeAsks{} + k := Asker{Publisher: f, Waiter: f, Bound: 60 * time.Millisecond} + const id = "mvl0frv18d1962" + first := time.Now() + f.Write(delivery.Step{ID: id, Node: "ace", State: delivery.StepRunning, Started: first}) + go func() { + time.Sleep(40 * time.Millisecond) + f.Write(delivery.Step{ID: id, Node: "ace", State: delivery.StepRunning, Started: time.Now()}) + time.Sleep(50 * time.Millisecond) // past the first run's bound, inside the second's + f.Write(delivery.Step{ID: id, Node: "ace", State: delivery.StepPassed, Started: time.Now()}) + }() + step, err := k.Wait(context.Background(), goAsk(id)) + if err != nil || step.State != delivery.StepPassed { + t.Fatalf("step %+v, %v, want the redelivered run's pass", step, err) + } +}