amadan
moves: the held render's intended set is pruned to the file on disk (Task 9a fix round 4)
The 32-hash cap could block a dirty return for ever: returns have no
deadline, so thirty-two distinct renders while readiness was blocked, then
one more config change, refused the current content even after readiness
recovered.
notePreRender now prunes to disk instead. Before each attempt it hashes
the file now at <host>.config.env (Lstat'ed, never followed) and journals
intended = {that hash, only if the set already names it} plus {the new
content's hash}, then the render writes. A file whose content the set does
not name (the earlier state, or foreign content) contributes nothing, and
the pre_render earlier-state record is left as first seen. The renderer
replaces the file by an atomic rename (appconfig syncexec.go:1416-1420,
temp then rename; nested rows instancefs.WriteAtomic, :1410), so the file
on disk is always one whole content and a pruned hash is never on disk.
The set never holds more than two hashes: no cap, no refusal.
Tests: TestReturnRenderRetriesStayBounded (a return: forty failed or
changed attempts, half written, the set at most two and always naming the
file on disk; then a successful render completes the return with that
content kept and named), TestRenderCrashAfterThePrunedJournal (A then B
written; C prunes to [B C] and dies before writing; the cleaning parks B),
TestRenderReplayKeepsEveryIntendedHash unchanged ([A B]). The cap test is
gone. RED: "intended [<A> <B> <C>]; want [B C]" and "attempt 2: 3 intended
hashes".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+133
−52
internal/host/movescarries_test.go+91 −22internal/host/movesinstall.go+39 −30internal/host/movesinstall_test.go+3 −0
diff --git a/internal/host/movescarries_test.go b/internal/host/movescarries_test.go| index 48ba2f828..e59db67c9 100644 |
| --- a/internal/host/movescarries_test.go |
| +++ b/internal/host/movescarries_test.go |
| @@ -351,31 +351,100 @@ func TestRenderReplayKeepsEveryIntendedHash(t *testing.T) { |
| } |
| } |
| -// TestRenderIntentsAreBounded (fix round 2): the set of intended hashes is |
| -// capped at maxRenderIntents distinct contents; a hash already recorded adds |
| -// nothing, and a render past the cap is refused before it writes. |
| -func TestRenderIntentsAreBounded(t *testing.T) { |
| - h := newLandingHarness(t) |
| +// renderAttempt is one fake held render: it journals sum through the |
| +// installer's before-write hook, writes body to path when write is true, |
| +// and then dies (no ack, no answer) unless done is true. |
| +func renderAttempt(path string, body []byte, write, done bool, held appconfig.HeldRender) func(func(string) error) (appconfig.HeldRender, error) { |
| + return func(before func(string) error) (appconfig.HeldRender, error) { |
| + sum := sha256.Sum256(body) |
| + if err := before(hex.EncodeToString(sum[:])); err != nil { |
| + return appconfig.HeldRender{}, err |
| + } |
| + if !write { |
| + return appconfig.HeldRender{}, errors.New("the write failed") |
| + } |
| + if err := os.WriteFile(path, body, 0o600); err != nil { |
| + return appconfig.HeldRender{}, err |
| + } |
| + if !done { |
| + return appconfig.HeldRender{}, errors.New("the agent died after the write") |
| + } |
| + held.Path, held.SHA256 = path, hex.EncodeToString(sum[:]) |
| + return held, nil |
| + } |
| +} |
| + |
| +// TestRenderCrashAfterThePrunedJournal (fix round 4): render A is written, |
| +// then B is written; render C prunes the set to what is on disk, B, plus C, |
| +// journals it, and dies before writing. The file on disk is B, which the set |
| +// still names, so the cleaning parks it. |
| +func TestRenderCrashAfterThePrunedJournal(t *testing.T) { |
| + h := &rollbackHarness{landingHarness: newLandingHarness(t)} |
| h.flip(t) |
| - var refused error |
| - h.renderHook = func(before func(string) error) (appconfig.HeldRender, error) { |
| - for i := 0; i <= maxRenderIntents; i++ { |
| - sum := sha256.Sum256([]byte(fmt.Sprintf("content %d", i))) |
| - if err := before(hex.EncodeToString(sum[:])); err != nil { |
| - refused = err |
| - return appconfig.HeldRender{}, err |
| - } |
| - if err := before(hex.EncodeToString(sum[:])); err != nil { // the same hash again |
| - t.Fatalf("a recorded hash was refused: %v", err) |
| - } |
| + path := filepath.Join(h.tdir, p1Host+".config.env") |
| + a, b, c := []byte("SECRET=a\n"), []byte("SECRET=b\n"), []byte("SECRET=c\n") |
| + for _, step := range []func(func(string) error) (appconfig.HeldRender, error){ |
| + renderAttempt(path, a, true, false, appconfig.HeldRender{}), |
| + renderAttempt(path, b, true, false, appconfig.HeldRender{}), |
| + renderAttempt(path, c, false, false, appconfig.HeldRender{}), |
| + } { |
| + h.renderHook = step |
| + h.land(t) |
| + } |
| + sumB, sumC := sha256.Sum256(b), sha256.Sum256(c) |
| + pre := journalledPreRender(t, h.landJournal(t)) |
| + if want := []string{"sha256:" + hex.EncodeToString(sumB[:]), "sha256:" + hex.EncodeToString(sumC[:])}; len(pre.Intended) != 2 || pre.Intended[0] != want[0] || pre.Intended[1] != want[1] { |
| + t.Fatalf("intended %v; want [B C]", pre.Intended) |
| + } |
| + if pre.Before != "absent" { |
| + t.Fatalf("the earlier-state record changed: %q", pre.Before) |
| + } |
| + h.cleanAfterRollback(t) |
| + if _, err := os.Lstat(path); !errors.Is(err, os.ErrNotExist) { |
| + t.Fatalf("B is still live: %v", err) |
| + } |
| + if got, ok := parkedLocally(t, h.tdir)[p1Host+".config.env"]; !ok || !bytes.Equal(got, b) { |
| + t.Fatal("B was not parked") |
| + } |
| +} |
| + |
| +// TestReturnRenderRetriesStayBounded (fix round 4): a dirty return has no |
| +// deadline, so its held render may be retried any number of times. Forty |
| +// failed or changed attempts, half written and half not, keep the intended |
| +// set at most two hashes, always naming what is on disk; the next render |
| +// succeeds and the return completes with that content kept as the source's |
| +// config, still named. |
| +func TestReturnRenderRetriesStayBounded(t *testing.T) { |
| + h := newReturnHarness(t, false) |
| + path := filepath.Join(h.sdir, p1Host+".config.env") |
| + for i := 0; i < 40; i++ { |
| + h.renderHook = renderAttempt(path, []byte(fmt.Sprintf("SECRET=%d\n", i)), i%2 == 0, false, appconfig.HeldRender{}) |
| + if _, err := h.install(t); err == nil { |
| + t.Fatalf("attempt %d: the render answered", i) |
| + } |
| + pre := journalledPreRender(t, h.returnJournal(t)) |
| + if len(pre.Intended) > 2 { |
| + t.Fatalf("attempt %d: %d intended hashes", i, len(pre.Intended)) |
| + } |
| + if got, err := fileSHA256(path); err == nil && !containsString(pre.Intended, got) { |
| + t.Fatalf("attempt %d: the file on disk (%s) is not named by %v", i, got, pre.Intended) |
| } |
| - return appconfig.HeldRender{}, errors.New("unreachable") |
| } |
| - h.land(t) |
| - if refused == nil { |
| - t.Fatal("no render was refused past the cap") |
| + final := []byte("SECRET=final\n") |
| + h.renderHook = renderAttempt(path, final, true, true, h.render) |
| + if _, err := h.install(t); err != nil { |
| + t.Fatalf("the successful render: %v", err) |
| + } |
| + if rb := h.read(t, p1Host).Move.Rollback; rb.SourcePhase != provision.RbSourceRestored { |
| + t.Fatalf("the return did not complete: %+v", rb) |
| + } |
| + got, err := os.ReadFile(path) |
| + if err != nil || !bytes.Equal(got, final) { |
| + t.Fatalf("the source's config is %q %v, want the final render", got, err) |
| } |
| - if pre := journalledPreRender(t, h.landJournal(t)); len(pre.Intended) != maxRenderIntents { |
| - t.Fatalf("%d intended hashes recorded, want %d", len(pre.Intended), maxRenderIntents) |
| + sum := sha256.Sum256(final) |
| + pre := journalledPreRender(t, h.returnJournal(t)) |
| + if len(pre.Intended) > 2 || !containsString(pre.Intended, "sha256:"+hex.EncodeToString(sum[:])) { |
| + t.Fatalf("intended %v after the final render", pre.Intended) |
| } |
| } |
diff --git a/internal/host/movesinstall.go b/internal/host/movesinstall.go| index dc2d0bcf3..972297786 100644 |
| --- a/internal/host/movesinstall.go |
| +++ b/internal/host/movesinstall.go |
| @@ -1120,25 +1120,26 @@ func (ir *installRun) configEnvPath() string { |
| // preRender is the landStepPreRender record: <host>.config.env's state |
| // before the first held render ("absent", "sha256:<hex>" of a regular file, |
| -// or "other"), and the sha256 of every distinct content a render of this op |
| -// was about to place, each journalled before its write (fix round 2). The |
| -// set only grows: a retry that renders other content adds its hash and |
| -// never replaces an earlier one, whose file may be the one on disk. |
| +// or "other"), and the intended set: the sha256 of each content a render of |
| +// this op may have placed, journalled before its write. The set is pruned |
| +// to the file on disk (fix round 4): before each attempt it keeps the hash |
| +// of the content now on disk, only when it is already one of the set's, and |
| +// adds the new content's. So it never holds more than two hashes, and every |
| +// content this op placed that can still be on disk stays named. |
| type preRender struct { |
| Before string `json:"before"` |
| Intended []string `json:"intended"` |
| } |
| -// maxRenderIntents caps the distinct contents one op's held renders may |
| -// journal. A render with a hash already recorded adds nothing; past the cap |
| -// the render refuses before it writes, so the record stays bounded and |
| -// every file a render placed stays accounted for. |
| -const maxRenderIntents = 32 |
| - |
| -// notePreRender is the held render's before-write hook: it adds the |
| -// intended content's sha256 (hex) to the journal's set ahead of the write, |
| -// keeping the first look at the file's earlier state (Lstat'ed, never |
| -// followed). |
| +// notePreRender is the held render's before-write hook. It hashes the file |
| +// now on disk (Lstat'ed, never followed) and journals the intended set as |
| +// {that hash, if the set already names it} plus {the new content's hash}, |
| +// before the write. A file whose content the set does not name (the earlier |
| +// state, or foreign content) contributes nothing, and the earlier-state |
| +// record is kept as first seen. The renderer replaces the file by an atomic |
| +// rename (appconfig's renderExecConfigBefore), so the file on disk is always |
| +// one whole content: a hash pruned here is never on disk again unless this |
| +// op renders it again, when it is journalled again first. |
| func (ir *installRun) notePreRender(sha string) error { |
| if sha == "" { |
| return errors.New("moves: the held render names no content") |
| @@ -1147,6 +1148,18 @@ func (ir *installRun) notePreRender(sha string) error { |
| if err != nil { |
| return err |
| } |
| + path := ir.configEnvPath() |
| + disk := "" |
| + fi, err := os.Lstat(path) |
| + switch { |
| + case errors.Is(err, os.ErrNotExist): |
| + case err != nil: |
| + return err |
| + case fi.Mode().IsRegular(): |
| + if disk, err = fileSHA256(path); err != nil { |
| + return err |
| + } |
| + } |
| var pre preRender |
| if raw, ok := j.Steps[landStepPreRender]; ok { |
| if err := json.Unmarshal([]byte(raw), &pre); err != nil { |
| @@ -1154,29 +1167,25 @@ func (ir *installRun) notePreRender(sha string) error { |
| } |
| } else { |
| pre.Before = "other" |
| - path := ir.configEnvPath() |
| - fi, err := os.Lstat(path) |
| switch { |
| - case errors.Is(err, os.ErrNotExist): |
| + case errors.Is(err, os.ErrNotExist) || fi == nil: |
| pre.Before = "absent" |
| - case err != nil: |
| - return err |
| - case fi.Mode().IsRegular(): |
| - if pre.Before, err = fileSHA256(path); err != nil { |
| - return err |
| - } |
| + case disk != "": |
| + pre.Before = disk |
| } |
| } |
| want := "sha256:" + sha |
| - for _, h := range pre.Intended { |
| - if h == want { |
| - return nil // this content's ownership is already recorded |
| - } |
| + var next []string |
| + if disk != "" && containsString(pre.Intended, disk) { |
| + next = append(next, disk) |
| + } |
| + if want != disk || len(next) == 0 { |
| + next = append(next, want) |
| } |
| - if len(pre.Intended) >= maxRenderIntents { |
| - return fmt.Errorf("moves: %s: %d distinct held renders of %s were journalled; the render waits (the landing's deadline rolls it back)", ir.rec.Host, len(pre.Intended), ir.g.ID) |
| + if strings.Join(next, " ") == strings.Join(pre.Intended, " ") { |
| + return nil |
| } |
| - pre.Intended = append(pre.Intended, want) |
| + pre.Intended = next |
| body, err := json.Marshal(pre) |
| if err != nil { |
| return err |
diff --git a/internal/host/movesinstall_test.go b/internal/host/movesinstall_test.go| index 2143b5c2b..5f29a618d 100644 |
| --- a/internal/host/movesinstall_test.go |
| +++ b/internal/host/movesinstall_test.go |
| @@ -212,6 +212,9 @@ func (h *returnHarness) sourceCfg(t *testing.T) moveCfg { |
| c.ParkView = recordView(h.s, "ie", roleSource) |
| c.RenderHeld = func(_ context.Context, _ release.Store, rt registry.Route, op string, before func(string) error) (appconfig.HeldRender, error) { |
| h.renders++ |
| + if h.renderHook != nil { |
| + return h.renderHook(before) |
| + } |
| if err := before(h.render.SHA256); err != nil { |
| return appconfig.HeldRender{}, err |
| } |