From e2380404a1fa1e7d474f04b25410e10d36a65279 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 4 Nov 2026 11:36:14 +0100 Subject: [PATCH] Act on a backup answer whatever runs, say why a module stays held, and let the engine's read tool show its want and its hold (issue 1585) nats on novox, 2026-11-04 11:07: after mesh-host #207 the module stayed held although its want carried "Back up again", and the engine said nothing. The engine's composition verb showed it running what its want names (taken from its held have at the start), so when the answer was in a revision of the want read after the hold, settleOne returned on "it runs what the want names" before it looked at the hold: the answer was swallowed, silently. The start path #207 fixed was right but never met; no test ran the live shape. - settleOne: the hold is looked at before what runs. An answer (again or skip) in a want revision after the one the hold took is acted on whatever runs; with no answer the hold is kept and said with both revisions. - baseline: the hold kept at the start is said (it was the silent branch). - composition verb: the want as the engine holds it now carries self, backup, words and its key's revision, beside the engine's attempt (held, answered revision, want and composition revisions, transient) and whether the module was looked at since the start, so which want the engine read is read, never guessed. Quick fix: answer once more by hand (a new want revision), which the guard still swallows while what runs equals the want, so none exists. Thorough fix, chosen: the order of the guards and the read tool, proved by rows of the live shape (a self module of policy record, its held have naming the want's own composition, the answer before the start, and again and skip arriving while held); the two later rows fail with the old guard order. Claude-Session: https://claude.ai/code/session_01JG73KG7AfSdh6FP8VrBqCn --- internal/agent/agent.go | 35 +++++++++++++---- internal/agent/agent_test.go | 72 +++++++++++++++++++++++++++++++++++ internal/agent/composition.go | 39 ++++++++++++++++++- 3 files changed, 136 insertions(+), 10 deletions(-) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 9f63130..baedc59 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -1305,20 +1305,35 @@ func (a *Agent) settleOne(ctx context.Context, module string) { a.mu.Unlock() } a.mu.Lock() - if r, ok := a.running[module]; ok && r.FP == w.Want.FP && r.Composed == w.Want.Composed && r.Phase != delivery.PhaseRemoved { + r, runs := a.running[module] + runsWant := runs && r.FP == w.Want.FP && r.Composed == w.Want.Composed && r.Phase != delivery.PhaseRemoved + att, tried := a.attempt[module] + same := tried && att.FP == w.Want.FP && att.Composed == w.Want.Composed + if same && att.Held { + // Held for a failed backup: looked at before what runs, since a hold may name what the want names (its have kept + // the composition it held, nats on novox, 2026-11-04), and an answer in a later revision of the want would then + // be swallowed by "it runs what its want names" (issue 1585, the second failure). The operator's answer, again or + // skip, in a revision after the one the hold took, is acted on whatever runs; any other new revision of the want + // is the answer only for a move (ADR 0568 §4), as before. + answered := w.Rev > att.AnsweredRev && (w.Want.Backup == delivery.BackupAgain || w.Want.Backup == delivery.BackupSkip) + if !answered && (runsWant || (att.AnsweredRev >= w.Rev && att.CompRev == comp.Rev)) { + a.sayOnce("held:"+module, fmt.Sprintf("%s is held for a failed backup before %s: it waits for the operator's "+ + "answer, and its want (revision %d) carries none since the hold (revision %d)", module, w.Want.FP, w.Rev, att.AnsweredRev)) + a.mu.Unlock() + return + } + if answered { + a.sayOnce("answered:"+module+":"+fmt.Sprint(w.Rev), fmt.Sprintf("%s is held for a failed backup, and "+ + "its want (revision %d) carries the operator's answer %q: acted on now", module, w.Rev, w.Want.Backup)) + } + } else if runsWant { a.mu.Unlock() return } - att, tried := a.attempt[module] - same := tried && att.FP == w.Want.FP && att.Composed == w.Want.Composed if same && !att.Held && att.WantRev == w.Rev && att.CompRev == comp.Rev { a.mu.Unlock() return // failed already for exactly these keys: retried by the next change of either } - if same && att.Held && att.AnsweredRev >= w.Rev && att.CompRev == comp.Rev { - a.mu.Unlock() - return // held for a failed backup, and no answer since - } if module == NodeModule { // Not while a module's composition cannot be read or is refused: what it carries may be what left the node's own, // and the node's own apply would remove it, held out by nothing (issue 1945, the review's gap 2). @@ -1390,8 +1405,12 @@ func (a *Agent) baseline(ctx context.Context, module string, w wantNow, comp com } if h.Phase == string(delivery.PhaseHeld) { // Held for a failed backup, and the operator's answer is still to come: the hold is kept, nothing is - // backed up again or applied, and the next revision of the want is the answer (ADR 0568 §4). + // backed up again or applied, and the next revision of the want is the answer (ADR 0568 §4). Said, so + // why it holds is read from the journal and not guessed (issue 1585: this branch was silent). a.mu.Lock() + a.sayOnce("held:"+module, fmt.Sprintf("%s is held for a failed backup (%s) at this start: it keeps what "+ + "its have names, %s, and waits for the operator's answer; its want (revision %d) carries none", + module, h.Detail, short(h.Composed), w.Rev)) a.attempt[module] = attempt{FP: w.Want.FP, Composed: w.Want.Composed, WantRev: w.Rev, CompRev: comp.Rev, Held: true, AnsweredRev: w.Rev} if h.Composed != "" { diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index e2fed75..f5703a6 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -636,6 +636,78 @@ func TestADataModuleIsBackedUpFirstAndHeldWhenItFails(t *testing.T) { } stopR() } + + // The live shape (nats on novox, 2026-11-04, issue 1585, the second failure): a module the node composes itself, of + // policy record, its want Self with the yes for its composition, and its held have naming that very composition (what + // runs is what the want names). The operator's answer acted on at the start, and in a revision of the want that + // arrives while the module is held; the engine's read tool says the want's answer and the hold. + for _, row := range []struct { + name string + atStart string // the answer the want carries when the engine starts + later string // the answer a later revision carries; "" writes none + backups int + holdsTill bool // it is held, and said held, before the later revision + }{ + {"answered before the start", delivery.BackupAgain, "", 1, false}, + {"answered while held", "", delivery.BackupAgain, 1, true}, + {"skipped while held", "", delivery.BackupSkip, 0, true}, + } { + t.Run(row.name, func(t *testing.T) { + r := newBed(t) + r.env.SelfComposed, r.kept = true, nil + r.env.Policy = func(context.Context, string) string { return catalogue.PolicyRecord } + ck, craw, dig := composition(t, "nats", fp2, true, file("nats", "x", "2")) + r.comps.before(ck, craw) + wk, _ := delivery.WantKey(node, "nats") + selfWant := func(backup string) []byte { + raw, err := delivery.EncodeWant(delivery.Want{FP: fp2, Self: true, Yes: dig, Backup: backup}) + if err != nil { + t.Fatal(err) + } + return raw + } + r.wants.before(wk, selfWant(row.atStart)) + r.haves.stored = map[string]Have{"nats": {Module: "nats", FP: fp2, Composed: dig, Phase: string(delivery.PhaseHeld), + Detail: "the backup failed: sh: exit status 1"}} + stopR := r.run() + defer stopR() + if row.holdsTill { + eventually(t, "the hold at the start is not said", func() bool { + r.mu.Lock() + defer r.mu.Unlock() + for _, s := range r.said { + if strings.Contains(s, "nats is held for a failed backup") && strings.Contains(s, "carries none") { + return true + } + } + return false + }) + view, err := r.ag.Composition(context.Background(), "nats", "", false) + if err != nil || view.Attempt == nil || !view.Attempt.Held || view.Want == nil || view.Want.Backup != "" || !view.Started { + t.Fatalf("the read tool does not say the hold: %+v %+v %v", view.Attempt, view.Want, err) + } + if got := r.appliedList(); len(got) != 0 { + t.Fatalf("applied behind a hold with no answer: %v", got) + } + r.wants.put(wk, selfWant(row.later)) + eventually(t, "the read tool does not say the answer", func() bool { + v, err := r.ag.Composition(context.Background(), "nats", "", false) + return err == nil && v.Want != nil && v.Want.Backup == row.later && v.Want.Revision > view.Want.Revision + }) + } + eventually(t, "the answer was not acted on", func() bool { return len(r.appliedList()) == 1 }) + eventually(t, "not running after the answer", func() bool { + h := r.haves.last("nats") + return h.Phase == string(delivery.PhaseRunning) && h.Composed == dig + }) + r.mu.Lock() + n := len(r.backups) + r.mu.Unlock() + if n != row.backups { + t.Errorf("%d backups for the answer, want %d", n, row.backups) + } + }) + } } // A data module with a good backup is applied after it; one that never ran has nothing to back up. diff --git a/internal/agent/composition.go b/internal/agent/composition.go index 4035b8b..11d00e5 100644 --- a/internal/agent/composition.go +++ b/internal/agent/composition.go @@ -21,6 +21,13 @@ type CompositionView struct { Module string `json:"module"` // Want is the module's want as the agent holds it; nil when it holds none. Want *WantView `json:"want,omitempty"` + // Attempt is what the agent holds of the module's last apply that did not end running: failed, failed for a moment + // of the bus, or held for a failed backup, with the want's revision that hold took as answered; nil when none + // (issue 1585: why a module stays held was not readable from the engine). + Attempt *AttemptView `json:"attempt,omitempty"` + // Started says the agent has looked at the module once since it started (its baseline): what runs was taken from its + // have, or it was applied. + Started bool `json:"started"` // Have is what the module's have key says now; nil when it says nothing. Have *HaveView `json:"have,omitempty"` // Composition is the revision read: the latest of the want's build, or the one whose digest was asked. @@ -55,12 +62,34 @@ type OwnView struct { Held map[string]string `json:"held,omitempty"` } -// WantView is a want as the verb shows it. +// WantView is a want as the verb shows it: as the agent holds it, at the revision of its key it last read. type WantView struct { FP string `json:"fp"` Composed string `json:"composed,omitempty"` Yes string `json:"yes,omitempty"` Removed bool `json:"removed,omitempty"` + // Self says the node composes the module itself (ADR 0547 §2). + Self bool `json:"self,omitempty"` + // Backup is the operator's answer to a failed backup the want carries, again or skip (ADR 0539 §16); empty when none. + Backup string `json:"backup,omitempty"` + Words string `json:"words,omitempty"` + // Revision is the revision of the want's key the agent read last. + Revision uint64 `json:"revision"` +} + +// AttemptView is the agent's record of a module's last apply that did not end running. +type AttemptView struct { + FP string `json:"fp"` + Composed string `json:"composed,omitempty"` + // WantRevision and CompositionRevision are the revisions of the want's and the composition's keys it was tried at. + WantRevision uint64 `json:"want_revision"` + CompositionRevision uint64 `json:"composition_revision"` + // Held says it is held for a failed backup; AnsweredRevision is the want's revision the hold took, so an answer + // (again or skip) in a later revision releases it. + Held bool `json:"held,omitempty"` + AnsweredRevision uint64 `json:"answered_revision,omitempty"` + // Transient says it failed because the bus did not answer for a moment, and is tried again by the clock. + Transient bool `json:"transient,omitempty"` } // HaveView is a have as the verb shows it. @@ -127,10 +156,16 @@ func (a *Agent) Composition(ctx context.Context, module, digest string, diff boo fp := "" if w.Present { fp = w.Want.FP - view.Want = &WantView{FP: w.Want.FP, Composed: w.Want.Composed, Yes: w.Want.Yes, Removed: w.Want.Removed} + view.Want = &WantView{FP: w.Want.FP, Composed: w.Want.Composed, Yes: w.Want.Yes, Removed: w.Want.Removed, + Self: w.Want.Self, Backup: w.Want.Backup, Words: w.Want.Words, Revision: w.Rev} } else if have != nil { fp = have.FP } + if att, ok := a.attempt[module]; ok { + view.Attempt = &AttemptView{FP: att.FP, Composed: att.Composed, WantRevision: att.WantRev, CompositionRevision: att.CompRev, + Held: att.Held, AnsweredRevision: att.AnsweredRev, Transient: att.Transient} + } + view.Started = a.checked[module] latest, haveLatest := a.compositionFor(module, fp) r, ran := a.running[module] from, known := a.runningComposition(module) -- 2.54.0