diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 5958d18..77a9a7c 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -76,11 +76,22 @@ type Error struct { Resource string Err error Done Report + // Others is how many more resources also failed. Named rather than folded into the message, + // because "one thing failed" and "eleven things failed" are different machines and the first + // line is what somebody reads. + Others int } func (e *Error) Error() string { - return fmt.Sprintf("applying %q: %v\n\n%d resource(s) were applied before this and remain; "+ - "the machine is in whatever state that left it.", e.Resource, e.Err, len(e.Done.Outcomes)) + also := "" + if e.Others == 1 { + also = ", and one other resource also failed" + } 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)) } func (e *Error) Unwrap() error { return e.Err } @@ -129,11 +140,32 @@ func Apply( // restarting for it every time would make a steady machine restart its services for ever. changed := map[string]bool{} + // Everything is attempted, and every failure is reported. + // + // **It used to stop at the first one**, and that made one broken resource hold the whole + // machine hostage: a module declaring a package that does not exist meant every module + // ordered after it was never applied, for ever, and the mesh reported "failed" without + // saying that the rest had not been tried. A machine with one bad module and nine good ones + // ran none of the nine. + // + // The argument for stopping was that a resource may depend on an earlier one. It still may — + // and it will then fail its own check and be reported, which is more information than not + // attempting it. A service started against a file that was never written does not verify, and + // this host reads back after every write precisely so that is caught rather than assumed. + // + // What does not change: **a declaration that cannot be parsed is still refused whole.** That + // is a different thing — one is "this machine could not do it", the other is "this was never + // a declaration", and they are fixed in different places. + var failures []*Error for _, resource := range d.Resources { was, _ := known.Find(resource.Identity()) outcome, err := applyOne(ctx, sys, resource, run, changed, was, unseal) if err != nil { - return report, known, &Error{Resource: resource.Identity(), Err: err, Done: report} + failures = append(failures, &Error{ + Resource: resource.Identity(), Err: err, Done: report, + }) + log(fmt.Sprintf(" failed %s (%s): %v", resource.Identity(), outcome.Target, err)) + continue } // Only now. The record follows the fact, never leads it. @@ -149,6 +181,16 @@ func Apply( log(fmt.Sprintf(" %s %s (%s)", outcome.Action, outcome.ID, outcome.Target)) } } + + if len(failures) > 0 { + // The first, carrying everything that did happen. One error is what the caller reports + // and what a person reads first; the rest are in the report, which is what the mesh + // keeps. + first := failures[0] + first.Done = report + first.Others = len(failures) - 1 + return report, known, first + } return report, known, nil } diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index 3415849..acfcbe3 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -178,9 +178,16 @@ func TestARenameToTheSamePathDoesNotDeleteTheNewFile(t *testing.T) { } } -func TestAFailedStepFailsTheApply(t *testing.T) { - // novox/hq ADR 0010. And the error carries what HAD been done, because the machine is in - // whatever state the apply reached and the only honest thing to hand back is that list. +func TestAFailedStepFailsTheApplyAndTheRestIsStillAttempted(t *testing.T) { + // The apply fails, names the resource, and carries what did happen — because the machine is + // in whatever state the apply reached and the only honest thing to hand back is that list. + // + // **And everything is attempted.** It used to stop at the first failure, which made one + // broken resource hold the whole machine hostage: a module declaring a package that does not + // exist meant every module after it was never applied, for ever + // (novox/hq 04-ISSUES/011). The case for stopping was that a later resource may depend on an + // earlier one — and it still may, and it then fails its own check and is reported, which is + // more information than not attempting it. dir := t.TempDir() blocker := filepath.Join(dir, "blocker") if err := os.WriteFile(blocker, []byte("i am a file\n"), 0o644); err != nil { @@ -190,7 +197,7 @@ func TestAFailedStepFailsTheApply(t *testing.T) { d := parse(t, `{"declaration":1,"resources":[ {"id":"fine","type":"file","path":"`+filepath.Join(dir, "fine.conf")+`","content":"a\n"}, {"id":"doomed","type":"directory","path":"`+blocker+`"}, - {"id":"never","type":"file","path":"`+filepath.Join(dir, "never.conf")+`","content":"b\n"} + {"id":"after","type":"file","path":"`+filepath.Join(dir, "after.conf")+`","content":"b\n"} ]}`) _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil) @@ -205,12 +212,43 @@ func TestAFailedStepFailsTheApply(t *testing.T) { if applyErr.Resource != "doomed" { t.Errorf("the failure names %q, not the resource that failed", applyErr.Resource) } - if len(applyErr.Done.Outcomes) != 1 { - t.Errorf("the error does not carry what was already applied: %+v", applyErr.Done.Outcomes) + // Everything that worked is in the report, before and after the failure. + if len(applyErr.Done.Outcomes) != 2 { + t.Errorf("the error does not carry what was applied: %+v", applyErr.Done.Outcomes) } - // And nothing after the failure ran. - if _, err := os.Stat(filepath.Join(dir, "never.conf")); !errors.Is(err, os.ErrNotExist) { - t.Error("the apply continued past a failure") + if _, err := os.Stat(filepath.Join(dir, "after.conf")); err != nil { + t.Error("a resource after the failing one was never attempted, so one broken module " + + "still blocks every module after it") + } +} + +func TestEveryFailureIsCountedNotJustTheFirst(t *testing.T) { + // "One thing failed" and "eleven things failed" are different machines, and the first line is + // what somebody reads. + dir := t.TempDir() + for _, name := range []string{"one", "two"} { + if err := os.WriteFile(filepath.Join(dir, name), []byte("a file\n"), 0o644); err != nil { + t.Fatal(err) + } + } + d := parse(t, `{"declaration":1,"resources":[ + {"id":"first","type":"directory","path":"`+filepath.Join(dir, "one")+`"}, + {"id":"second","type":"directory","path":"`+filepath.Join(dir, "two")+`"} + ]}`) + + _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil) + if err == nil { + t.Fatal("two impossible resources did not fail the apply") + } + var applyErr *Error + if !errors.As(err, &applyErr) { + t.Fatalf("got %T", err) + } + if applyErr.Others != 1 { + t.Errorf("the failure says %d others also failed, and one did", applyErr.Others) + } + if !strings.Contains(err.Error(), "one other resource also failed") { + t.Errorf("the message does not say others failed: %v", err) } }