diff --git a/go/buildstep/guard.go b/go/buildstep/guard.go index 6d2678b..a9dc49c 100644 --- a/go/buildstep/guard.go +++ b/go/buildstep/guard.go @@ -27,13 +27,11 @@ type Guard struct { // Check probes the installed version, writes it to . of the toolchains state every time it runs, // and answers nil when it is the recipe's. Otherwise it answers a *Refusal naming both versions; the installed // version is written even then, so the indexer sees that builders disagree. A probe that fails is a *Refusal -// naming its failure with Again set, and nothing is written: a toolchain that could not be read is not a toolchain -// known to be wrong, so the ask goes to another node or a later pull instead of ending refused (novox/hq issue 734). -// A write that fails is an error that is not a Refusal. +// naming its failure, and nothing is written. A write that fails is an error that is not a Refusal. func (g Guard) Check(ctx context.Context) error { installed, err := g.Probe(ctx) if err != nil { - return &Refusal{Text: fmt.Sprintf("the installed %s version could not be read: %v", g.Language, err), Want: g.Want, Again: true} + return &Refusal{Text: fmt.Sprintf("the installed %s version could not be read: %v", g.Language, err), Want: g.Want} } key, err := delivery.ToolchainKey(g.Node, g.Language) if err != nil { diff --git a/go/buildstep/guard_test.go b/go/buildstep/guard_test.go index a22d11b..30514b1 100644 --- a/go/buildstep/guard_test.go +++ b/go/buildstep/guard_test.go @@ -46,7 +46,7 @@ func TestCheckRefusesAnotherVersionAndWritesTheInstalledOne(t *testing.T) { if !errors.As(err, &ref) { t.Fatalf("Check = %v, want a Refusal", err) } - if ref.Text != "go 1.27.1 installed, the recipe builds with go 1.27.0" || ref.Want != "1.27.0" || ref.Have != "1.27.1" || ref.Again { + if ref.Text != "go 1.27.1 installed, the recipe builds with go 1.27.0" || ref.Want != "1.27.0" || ref.Have != "1.27.1" { t.Errorf("refusal = %+v", ref) } if k["ace.go"].Version != "1.27.1" { @@ -54,14 +54,13 @@ func TestCheckRefusesAnotherVersionAndWritesTheInstalledOne(t *testing.T) { } } -// 1473/AC3: a probe that fails: a Refusal naming the failure, and nothing put. Issue 734: one to ask again (Again), not a -// toolchain known to be wrong. +// 1473/AC3: a probe that fails: a Refusal naming the failure, and nothing put. func TestCheckRefusesAFailedProbeAndPutsNothing(t *testing.T) { k := toolchainKeys{} err := guardFor(k, "", errors.New("go: not found")).Check(context.Background()) var ref *Refusal - if !errors.As(err, &ref) || !strings.Contains(ref.Text, "go: not found") || !ref.Again { - t.Fatalf("Check = %+v, want a Refusal to ask again naming the probe's failure", err) + if !errors.As(err, &ref) || !strings.Contains(ref.Text, "go: not found") { + t.Fatalf("Check = %v, want a Refusal naming the probe's failure", err) } if len(k) != 0 { t.Errorf("put %v", k) diff --git a/go/buildstep/pull.go b/go/buildstep/pull.go index 587d3cf..960d02e 100644 --- a/go/buildstep/pull.go +++ b/go/buildstep/pull.go @@ -320,33 +320,22 @@ func (p *Puller) next(ctx context.Context, wait time.Duration) (*Pulled, error) // back, as one whose start cannot be. func (p *Puller) decline(msg *Pulled, ask delivery.StepAsk, lacks []string) { step, key, err := begin(p.Put, p.Node, ask.Kind+" ask", ask.ID, AskWhat(ask)) - if err != nil { - p.logf("the ask %s cannot be declined on %s, so it is given back: %v", ask.ID, p.Node, err) - _ = msg.Nak() - return - } - why := fmt.Sprintf("the ask %s needs %s, and %s lacks %s: it is left to a node that has it", ask.ID, - strings.Join(ask.Needs, ", "), p.Node, strings.Join(lacks, ", ")) - p.endDeclined(msg, ask, key, step, why, lacks) -} - -// endDeclined ends a begun step of an ask refused as declined (delivery.StepDeclined) and drops its message, for its -// asker to put again; the worker it came from is not looked at again until the next pull's wait. A message whose -// decline cannot be written is handed back. -func (p *Puller) endDeclined(msg *Pulled, ask delivery.StepAsk, key string, step delivery.Step, why string, lacks []string) { - step.Ended = now().UTC() - step.State, step.Reason = delivery.StepRefused, why - var err error - step.Result, err = json.Marshal(delivery.StepDeclined{Declined: true, Needs: ask.Needs, Lacks: lacks}) if err == nil { - err = writeEnd(p.Put, key, step) + why := Refuse("the ask %s needs %s, and %s lacks %s: it is left to a node that has it", ask.ID, + strings.Join(ask.Needs, ", "), p.Node, strings.Join(lacks, ", ")) + step.Ended = now().UTC() + step.State, step.Reason = delivery.StepRefused, why.Error() + step.Result, err = json.Marshal(delivery.StepDeclined{Declined: true, Needs: ask.Needs, Lacks: lacks}) + if err == nil { + err = writeEnd(p.Put, key, step) + } } if err != nil { p.logf("the ask %s cannot be declined on %s, so it is given back: %v", ask.ID, p.Node, err) _ = msg.Nak() return } - p.logf("the ask %s is declined on %s: %s", ask.ID, p.Node, why) + p.logf("the ask %s is declined on %s: it needs %s, and this node lacks %s", ask.ID, p.Node, strings.Join(ask.Needs, ", "), strings.Join(lacks, ", ")) _ = msg.Term() p.mu.Lock() if p.declined == nil { @@ -442,12 +431,6 @@ func (p *Puller) run(ctx context.Context, msg *Pulled, ask delivery.StepAsk, key // The process stops: the ask is neither ended nor acknowledged here, so the stream gives it to a node that runs. return } - if ref := (*Refusal)(nil); errors.As(err, &ref) && ref.Again { - // The node cannot run it at this moment (its toolchain could not be read): declined, not refused, so the - // asker puts it again for another node or a later pull (issue 734). - p.endDeclined(msg, ask, key, step, fmt.Sprintf("%s cannot run the ask %s now: %v", p.Node, ask.ID, err), nil) - return - } if ferr := finish(p.Put, key, step, result, err, p.Logf); ferr != nil { // No end was written: the ask is not acknowledged as done, so the stream gives it again and a run that can // write its end (or the poison rule, issue 1907) ends it where its asker sees it (issue 1930). diff --git a/go/buildstep/pull_test.go b/go/buildstep/pull_test.go index 72e2cb4..902a460 100644 --- a/go/buildstep/pull_test.go +++ b/go/buildstep/pull_test.go @@ -139,8 +139,7 @@ func TestPullHoldsAtMostItsCapacity(t *testing.T) { // An ask that is not one, or not this seat's, is dropped and runs nowhere. And issue 890/AC1: a check ask needing // node-build-node is declined on a node that lacks it (its step refused as declined, naming what it lacks, the message -// dropped for the asker to put again), and runs on a node that has it. And issue 734: an ask whose run is refused to -// ask again (a toolchain that could not be read) is declined the same way, not refused for good. +// dropped for the asker to put again), and runs on a node that has it. func TestPullDropsWhatIsNoAsk(t *testing.T) { src, st := NewFakeAsks(), newSteps() src.Publish(delivery.AskDelivery, []byte(`{"v":1}`)) @@ -174,21 +173,6 @@ func TestPullDropsWhatIsNoAsk(t *testing.T) { Run: func(ctx context.Context, a delivery.StepAsk) (any, error) { return nil, nil }} go q.Pull(ctx) waitFor(t, "the placed ask acked where its need is met", func() bool { a, _, _ := has.Fates(); return len(a) == 1 }) - - probe, probeSteps := NewFakeAsks(), newSteps() - unready := testAsk(delivery.AskCheck) - _ = probe.PublishAsk(unready) - r := &Puller{Source: probe, Language: delivery.AskGo, Node: "ace", Put: probeSteps.put, Capacity: func() int { return 1 }, - PullWait: 10 * time.Millisecond, Run: func(ctx context.Context, a delivery.StepAsk) (any, error) { - return nil, &Refusal{Text: "the installed go version could not be read: getwd: no such file or directory", Again: true} - }} - go r.Pull(ctx) - waitFor(t, "the unready ask termed", func() bool { _, tm, _ := probe.Fates(); return len(tm) == 1 }) - key, _ = delivery.AskStepKey("ace", unready.ID) - s, _ = probeSteps.get(key) - if _, ok := delivery.DeclinedOf(s); !ok || !strings.Contains(s.Reason, "getwd") { - t.Fatalf("the step of an ask its node could not run now = %+v, want it declined naming why", s) - } } // load reads the recorded steps state's raw values: the pause key is not a step, so it is kept apart. diff --git a/go/buildstep/refusal.go b/go/buildstep/refusal.go index c5256a7..dcf0c48 100644 --- a/go/buildstep/refusal.go +++ b/go/buildstep/refusal.go @@ -10,11 +10,6 @@ type Refusal struct { Text string // Want is the version the recipe asks for and Have the one found; either may be empty. Want, Have string - // Again says the node cannot run the step at this moment, not that it never should: its toolchain could not even - // be read (novox/hq issue 734: a probe started in a directory deleted under it). A puller declines such an ask - // rather than refusing it (delivery.StepDeclined), so its asker puts it again for another node or a later pull; a - // step run as a tool ends refused, as any Refusal. - Again bool } func (r *Refusal) Error() string { return r.Text }