diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 77a9a7c..16ff97f 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -80,6 +80,11 @@ type Error struct { // because "one thing failed" and "eleven things failed" are different machines and the first // line is what somebody reads. Others int + + // Gated is true when what failed was an action, so nothing after it was attempted. A person + // reading a report needs to know the difference between "these things failed" and "these + // things failed and the rest was never tried". + Gated bool } func (e *Error) Error() string { @@ -89,9 +94,14 @@ func (e *Error) Error() string { } else if e.Others > 1 { also = fmt.Sprintf(", and %d other resources also failed", e.Others) } - return fmt.Sprintf("applying %q: %v%s\n\n%d resource(s) were applied and remain; "+ - "everything was attempted, so what is not listed as failed was done.", - e.Resource, e.Err, also, len(e.Done.Outcomes)) + rest := "everything was attempted, so what is not listed as failed was done." + if e.Gated { + // An action is a gate: it exists to make something true before the next thing needs it. + rest = "this is an action, so nothing after it was attempted — the machine is in " + + "whatever state that left it." + } + return fmt.Sprintf("applying %q: %v%s\n\n%d resource(s) were applied and remain; %s", + e.Resource, e.Err, also, len(e.Done.Outcomes), rest) } func (e *Error) Unwrap() error { return e.Err } @@ -161,10 +171,30 @@ func Apply( was, _ := known.Find(resource.Identity()) outcome, err := applyOne(ctx, sys, resource, run, changed, was, unseal) if err != nil { - failures = append(failures, &Error{ - Resource: resource.Identity(), Err: err, Done: report, - }) + failed := &Error{Resource: resource.Identity(), Err: err, Done: report} + failures = append(failures, failed) log(fmt.Sprintf(" failed %s (%s): %v", resource.Identity(), outcome.Target, err)) + + // **A failed action stops what follows. Nothing else does.** + // + // An action is the only shape whose purpose is to make something true *before* the + // next thing needs it — which is why it is the only one with a `verify`. The + // bootstrap is a row of them: the store answers, then its databases exist, then their + // schemas, then the broker. Carrying on past one that did not happen means starting + // things against a machine that is not ready, and on a small machine that is how a + // database still initialising gets its memory taken away and shuts down. Observed, + // in the lab, caused by an earlier version of this loop. + // + // Everything else is independent state. A package that will not install has nothing + // to do with a file on the other side of the declaration, and stopping there is what + // made one broken module hold a whole machine hostage + // (novox/hq 04-ISSUES/011). + if resource.Kind() == declaration.TypeAction { + failed.Done = report + failed.Others = len(failures) - 1 + failed.Gated = true + return report, known, failed + } continue } diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index acfcbe3..086ab4d 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -1189,3 +1189,38 @@ func TestAnUntouchedFileIsStillUnchanged(t *testing.T) { t.Errorf("an untouched file was reported as %q", got) } } + +func TestAFailedActionStopsWhatFollows(t *testing.T) { + // An action is the only shape whose purpose is to make something true BEFORE the next thing + // needs it, which is why it is the only one with a verify. The bootstrap is a row of them: + // the store answers, then its databases exist, then their schemas, then the broker. + // + // Carrying on past one that did not happen starts things against a machine that is not ready + // — and on a small machine that is how a database still initialising has its memory taken + // away and shuts down. Observed in the lab, caused by a version of this loop that continued + // past everything. + dir := t.TempDir() + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"gate","type":"action","command":["false"],"verify":["false"]}, + {"id":"after","type":"file","path":"`+filepath.Join(dir, "after.conf")+`","content":"b\n"} + ]}`) + + _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, + ExecRunner, nil, nil) + if err == nil { + t.Fatal("an action that cannot succeed did not fail the apply") + } + var applyErr *Error + if !errors.As(err, &applyErr) { + t.Fatalf("got %T", err) + } + if !applyErr.Gated { + t.Error("the failure does not say that nothing after it was attempted") + } + if _, statErr := os.Stat(filepath.Join(dir, "after.conf")); statErr == nil { + t.Error("the apply continued past a failed action, which is a gate") + } + if !strings.Contains(err.Error(), "nothing after it was attempted") { + t.Errorf("the message does not say the rest was not tried: %v", err) + } +}