From 4695dfc0a27bd0593b03892a68d15c21e9cd57b9 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 18:51:04 +0200 Subject: [PATCH] Say both a gated module and a machine stop, and count skipped apart (review of #83) A module-less action that stopped the apply after a module was gated was reported as everything else attempted. The failure now names the gated modules and the stop, counts skipped resources apart from applied ones, and the tests cover an archive below a refused path, a gated module tried again by the next apply, and a refused file among the failures. --- internal/apply/apply.go | 51 ++++++++--- internal/apply/refused_directory_test.go | 104 ++++++++++++++++++++++- 2 files changed, 142 insertions(+), 13 deletions(-) diff --git a/internal/apply/apply.go b/internal/apply/apply.go index f3cd989..2e64757 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -135,11 +135,16 @@ type Error struct { // 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 + // Gated is true when what failed was a step (an action, or a run-once container or process). 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 + // Stopped is true when that step belongs to no module, so it gated the whole machine and + // nothing after it was attempted. A module's step gates only its module (novox/hq ADR 0136), + // which Modules names. + Stopped bool + // Modules are the modules whose rest was not attempted, behind a step that did not complete // (novox/hq ADR 0136) or a directory the engine refused (issue 495). Named, because "everything // was attempted" is then untrue, and a report that says so after skipping a module lies. @@ -153,17 +158,40 @@ func (e *Error) Error() string { } else if e.Others > 1 { also = fmt.Sprintf(", and %d other resources also failed", e.Others) } - rest := "everything was attempted, so what is not listed as failed was done." + // Gated with no module named is an Error made before Stopped existed: a step that stopped the machine. + stopped := e.Stopped || (e.Gated && len(e.Modules) == 0) + // What the apply did not attempt, said whole (novox/hq issue 495): a module gated earlier in an apply + // that a machine-wide step then stopped is both, and saying either alone is untrue. + gatedRest := "" if len(e.Modules) > 0 { - rest = fmt.Sprintf("the rest of %s was not attempted and is listed as skipped; everything else "+ - "was attempted, so what is not listed as failed or skipped was done.", strings.Join(e.Modules, ", ")) - } else 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." + gatedRest = fmt.Sprintf("the rest of %s was not attempted and is listed as skipped", strings.Join(e.Modules, ", ")) } - 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) + rest := "everything was attempted, so what is not listed as failed was done." + switch { + case stopped && gatedRest != "": + rest = gatedRest + "; and this step gates the whole machine, so nothing after it was attempted — " + + "the machine is in whatever state that left it." + case stopped: + // An action is a gate: it exists to make something true before the next thing needs it. + rest = "this step gates the whole machine, so nothing after it was attempted — the machine is in " + + "whatever state that left it." + case gatedRest != "": + rest = gatedRest + "; everything else was attempted, so what is not listed as failed or skipped was done." + } + // Skipped is not applied: counted apart, so the count says what is on the machine. + applied, skipped := 0, 0 + for _, o := range e.Done.Outcomes { + if o.Action == "skipped" { + skipped++ + } else { + applied++ + } + } + count := fmt.Sprintf("%d resource(s) were applied and remain", applied) + if skipped > 0 { + count += fmt.Sprintf(", %d were skipped", skipped) + } + return fmt.Sprintf("applying %q: %v%s\n\n%s; %s", e.Resource, e.Err, also, count, rest) } func (e *Error) Unwrap() error { return e.Err } @@ -746,6 +774,7 @@ func ApplyMindingWindows( "of it was not attempted", module)) continue } + failed.Stopped = true failed.Done = report failed.Others = len(failures) - 1 failed.Modules = gatedModules() diff --git a/internal/apply/refused_directory_test.go b/internal/apply/refused_directory_test.go index 13065ff..997cf28 100644 --- a/internal/apply/refused_directory_test.go +++ b/internal/apply/refused_directory_test.go @@ -3,6 +3,8 @@ package apply import ( "context" "errors" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" @@ -116,7 +118,105 @@ func TestAFileIsJudgedByTheSameGuardAsADirectory(t *testing.T) { if _, statErr := os.Stat(filepath.Join(home, "data", "notes.conf")); statErr == nil { t.Error("a file was written below a directory the engine refused") } - if o := outcomeOf(report, "notes.conf"); o.ID != "" && o.Action != "skipped" { - t.Errorf("the file below a refused directory is reported %q", o.Action) + // A refused file has no outcome: it is a failure. Declared first, it is the failure the apply reports, + // and the directory the other. + var applyErr *Error + if !errors.As(err, &applyErr) || applyErr.Resource != "notes.conf" || applyErr.Others != 1 { + t.Errorf("the file below a refused directory is not among the failures: %v", err) + } + if o := outcomeOf(report, "notes.conf"); o.ID != "" { + t.Errorf("a refused file has an outcome as if applied: %+v", o) + } +} + +// An archive below a refused path is refused with it, whichever module declares it and in whatever order. +func TestAnArchiveBelowARefusedPathIsRefused(t *testing.T) { + dir := t.TempDir() + home := filepath.Join(dir, "home", "alice") + if err := os.MkdirAll(home, 0o755); err != nil { + t.Fatal(err) + } + passwd := filepath.Join(dir, "passwd") + if err := os.WriteFile(passwd, []byte("alice:x:4242:4242::"+home+":/bin/sh\n"), 0o644); err != nil { + t.Fatal(err) + } + before := passwdFile + passwdFile = passwd + t.Cleanup(func() { passwdFile = before }) + body, digest := anArchive(t, map[string]string{"theme.css": "x\n"}) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write(body) })) + t.Cleanup(server.Close) + + _, _, err := applyOnce(t, + `{"id":"theme.files","type":"archive","path":"`+home+`/data/theme","source":"`+server.URL+`/t.tar.gz",`+ + `"digest":"`+digest+`"},`+ + `{"id":"notes.data","type":"directory","path":"`+home+`/data"}`, nil, store.State{}) + refusedFor339(t, "an archive below a refused directory", err) + var applyErr *Error + if !errors.As(err, &applyErr) || applyErr.Resource != "theme.files" || applyErr.Others != 1 { + t.Errorf("the archive below a refused directory is not among the failures: %v", err) + } + if _, statErr := os.Stat(filepath.Join(home, "data")); statErr == nil { + t.Error("an archive was unpacked below a directory the engine refused") + } +} + +// Gated is per apply: a module gated behind its refused directory is tried again by the next apply, gated +// again while the directory is refused, and applied whole once it is not. +func TestAGatedModuleIsTriedAgainByTheNextApply(t *testing.T) { + root := aRuntimeRoot(t) + b := newStepBed(t) + declared := func() string { + return `{"id":"mod.state","type":"directory","path":"` + root + `"},` + + `{"id":"mod.rotation","type":"file","path":"` + root + `/log-rotation.conf","content":"max-size=10m\n"},` + + `{"id":"mod.check","type":"process","name":"mod-check","source":"` + b.body + `","digest":"` + b.digest + `",` + + `"run":["./check"],"run-once":true,"restart-on":["mod.rotation"]}` + } + apply := func(known store.State) (Report, store.State, error) { + return Apply(context.Background(), archHost(t), declare(t, declared()), known, store.OriginDeclared, b.run, nil, nil) + } + report, known, err := apply(store.State{}) + if err == nil || !skippedIn(report, "mod.check") || b.starts != 0 { + t.Fatalf("apply 1: err %v, skipped %v, starts %d", err, skippedIn(report, "mod.check"), b.starts) + } + report, known, err = apply(known) + if err == nil || !skippedIn(report, "mod.check") || b.starts != 0 { + t.Fatalf("apply 2, still refused: err %v, skipped %v, starts %d", err, skippedIn(report, "mod.check"), b.starts) + } + // The runtime moves its data elsewhere: the directory is no longer refused, and the module is applied. + elsewhere := filepath.Join(t.TempDir(), "docker") + if err := os.WriteFile(runtimeConfigs.daemonJSON, []byte(`{"data-root":"`+elsewhere+`"}`), 0o644); err != nil { + t.Fatal(err) + } + report, _, err = apply(known) + if err != nil || skippedIn(report, "mod.check") || b.starts != 1 { + t.Fatalf("apply 3, no longer refused: err %v, skipped %v, starts %d", err, skippedIn(report, "mod.check"), b.starts) + } + if _, statErr := os.Stat(filepath.Join(root, "log-rotation.conf")); statErr != nil { + t.Errorf("the module's file was not written once its directory was placed: %v", statErr) + } +} + +// A module gated behind its refused directory, then a step of no module that stops the machine: the report +// names both, and says nothing after the step was attempted (novox/hq issue 495). +func TestAMachineStopAfterAGatedModuleSaysBoth(t *testing.T) { + root := aRuntimeRoot(t) + after := filepath.Join(t.TempDir(), "after.conf") + _, _, err := Apply(context.Background(), archHost(t), parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"mod.state","type":"directory","path":"`+root+`"}, + {"id":"gate","type":"action","command":["false"],"verify":["false"]}, + {"id":"other.after","type":"file","path":"`+after+`","content":"b\n"} + ]}`), store.State{}, store.OriginCarried, ExecRunner, nil, nil) + var applyErr *Error + if !errors.As(err, &applyErr) || !applyErr.Stopped { + t.Fatalf("the failed action did not stop the apply: %v", err) + } + if _, statErr := os.Stat(after); statErr == nil { + t.Error("the apply continued past a failed action") + } + msg := err.Error() + if !strings.Contains(msg, "the rest of mod") || !strings.Contains(msg, "nothing after it was attempted") || + strings.Contains(msg, "everything else was attempted") { + t.Errorf("the report does not say both what was gated and that the machine stopped: %s", msg) } }