Keep a failed toolchain probe a loud refusal; only the command directory is fixed (issue 734, review round 1)

Declining an ask whose probe failed would turn a truly broken toolchain into a
wait of up to the decline bound that then fails with an empty "needs a node
with" reason. With the node-engine no longer deleting a running build and
commands started in a directory that exists, the getwd cause is gone, so the
guard's refusal stays as it was. The decline refactor goes with it.

go/buildstep/guard.go

@@ -27,13 +27,11 @@ type Guard struct {
 // Check probes the installed version, writes it to <node>.<language> 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
+// naming its failure, and nothing is written. A write that fails is an error that is not a Refusal.
-// 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.
 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 {

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
+// 1473/AC3: a probe that fails: a Refusal naming the failure, and nothing put.
-// toolchain known to be wrong.
 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 {
+	if !errors.As(err, &ref) || !strings.Contains(ref.Text, "go: not found") {
-		t.Fatalf("Check = %+v, want a Refusal to ask again naming the probe's failure", err)
+		t.Fatalf("Check = %v, want a Refusal naming the probe's failure", err)
 	}
 	if len(k) != 0 {
 		t.Errorf("put %v", k)

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).

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
+// dropped for the asker to put again), and runs on a node that has it.
-// ask again (a toolchain that could not be read) is declined the same way, not refused for good.
 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.

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 }