diff --git a/charts/runtime-agent/values.yaml b/charts/runtime-agent/values.yaml index cde8c36..b888413 100644 --- a/charts/runtime-agent/values.yaml +++ b/charts/runtime-agent/values.yaml @@ -183,11 +183,12 @@ controller: memory: 256Mi priorityClassName: "" # How long the controller may take to leave. Its shutdown pass writes the - # journals that accumulated since the last periodic flush, and the ceiling of - # that path is about ten seconds; the default is the Kubernetes default and - # covers it with room. Below 15 the chart refuses: SIGKILL would land before - # the pass runs, and the minute of journals lost with it is lost silently - # (ADR 0068 §4). + # journals that accumulated since the last periodic flush — about ten seconds + # at the ceiling — and then spends up to four more offering the spool to the + # backend, because that volume goes with the pod (ADR 0079). The default is + # the Kubernetes default and covers both with room. Below 15 the chart + # refuses: SIGKILL would land inside that path, and what is lost with it is + # lost silently (ADR 0068 §4). terminationGracePeriodSeconds: 30 nodeSelector: {} tolerations: [] diff --git a/cmd/agent/flush.go b/cmd/agent/flush.go index bb07ae7..0404fda 100644 --- a/cmd/agent/flush.go +++ b/cmd/agent/flush.go @@ -1,5 +1,7 @@ package main +import "context" + // The set of payload writers that run on the agent's own cadence, as data. // // There are two passes over it — the periodic one and the one a shutdown makes @@ -33,3 +35,24 @@ func runFlushers(flushers []flusher, shutdown bool) { f.run() } } + +// finalShipper is the one thing the shutdown pass needs from the shipper. An +// interface so the order below can be tested without a backend, and so an +// installation with no backend configured passes nil rather than a typed nil. +type finalShipper interface { + FinalRound(ctx context.Context) +} + +// runShutdown is the last pass, and the order in it is the decision: write what +// the accumulators still hold, and only then offer the spool to the backend. +// +// The other way round — which is what this replaced — writes those payloads +// after the only thing that could deliver them has already stopped, onto a +// volume that goes with the pod (ADR 0079). +func runShutdown(flushers []flusher, ship finalShipper) { + runFlushers(flushers, true) + if ship == nil { + return // no backend configured: the spool is the whole of the protocol + } + ship.FinalRound(context.Background()) +} diff --git a/cmd/agent/flush_test.go b/cmd/agent/flush_test.go index 023d1d2..7686bc2 100644 --- a/cmd/agent/flush_test.go +++ b/cmd/agent/flush_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "slices" "testing" ) @@ -38,3 +39,50 @@ func TestRunFlushersSkipsPeriodicOnlyWritersOnShutdown(t *testing.T) { t.Fatalf("shutdown pass ran %v, want %v", ran, want) } } + +// recordingShipper notes when the last round happened relative to the writers. +type recordingShipper struct { + ran *[]string + calls int +} + +func (r *recordingShipper) FinalRound(context.Context) { + r.calls++ + *r.ran = append(*r.ran, "ship") +} + +// The order is the whole fix. Shipping used to stop with the watchers, so the +// payloads the shutdown pass wrote were written after the only thing that could +// deliver them had already gone — onto a volume that goes with the pod +// (ADR 0079). +func TestTheShutdownPassShipsAfterItHasWritten(t *testing.T) { + var ran []string + record := func(name string) func() { return func() { ran = append(ran, name) } } + flushers := []flusher{ + {run: record("journals"), onShutdown: true}, + {run: record("metadata")}, + {run: record("inventory"), onShutdown: true}, + } + + runShutdown(flushers, &recordingShipper{ran: &ran}) + + want := []string{"journals", "inventory", "ship"} + if !slices.Equal(ran, want) { + t.Fatalf("the shutdown pass ran %v, want %v", ran, want) + } +} + +// An installation with no backend configured — which is every installation +// today — still writes its last pass and simply has nowhere to send it. +func TestTheShutdownPassWritesWithNoBackendConfigured(t *testing.T) { + var ran []string + flushers := []flusher{ + {run: func() { ran = append(ran, "journals") }, onShutdown: true}, + } + + runShutdown(flushers, nil) + + if !slices.Equal(ran, []string{"journals"}) { + t.Fatalf("the shutdown pass ran %v, want the writers to run regardless", ran) + } +} diff --git a/cmd/agent/main.go b/cmd/agent/main.go index 91e4ce2..5c72dde 100644 --- a/cmd/agent/main.go +++ b/cmd/agent/main.go @@ -1052,13 +1052,16 @@ func run(ctx context.Context, logger *slog.Logger, clientset kubernetes.Interfac }() } wg.Wait() - // A final flush lands whatever arrived since the last periodic write: an - // inventory fact joined, a container restarted, a pod preempted. Without it - // a graceful shutdown silently drops up to one coverage interval of the - // journals, which is exactly the minute a rolling upgrade or a node drain - // tends to be interesting in. The periodic pass is waited for above, so this - // is the only writer; the accumulators serialize regardless. - runFlushers(flushers, true) + // The last pass: write whatever arrived since the periodic one — an + // inventory fact joined, a container restarted, a pod preempted — and then + // give it one bounded chance to reach the backend. The periodic pass is + // waited for above, so this is the only writer; the order between the two + // halves is the decision, and it lives in runShutdown (ADR 0079). + var final finalShipper + if ship != nil { + final = ship + } + runShutdown(flushers, final) return errors.Join(errs...) } diff --git a/docs/adr/0068-the-agent-yields-first.md b/docs/adr/0068-the-agent-yields-first.md index 3283a93..e70275e 100644 --- a/docs/adr/0068-the-agent-yields-first.md +++ b/docs/adr/0068-the-agent-yields-first.md @@ -2,6 +2,7 @@ Date: 2026-09-04 Status: Accepted +Amended by: 0079 ## Context diff --git a/docs/adr/0075-a-payload-leaves-the-spool-only-when-the-backend-has-it.md b/docs/adr/0075-a-payload-leaves-the-spool-only-when-the-backend-has-it.md index f965d37..35e1528 100644 --- a/docs/adr/0075-a-payload-leaves-the-spool-only-when-the-backend-has-it.md +++ b/docs/adr/0075-a-payload-leaves-the-spool-only-when-the-backend-has-it.md @@ -3,7 +3,7 @@ Date: 2026-09-08 Status: Accepted Amends: 0027 §2 -Amended by: 0078 +Amended by: 0078, 0079 Adds the transmit path: one configuration setting, an HTTP POST per payload, and the deletion rule that makes the spool a queue rather than a directory. Closes diff --git a/docs/adr/0079-the-last-pass-writes-then-ships.md b/docs/adr/0079-the-last-pass-writes-then-ships.md new file mode 100644 index 0000000..a559a01 --- /dev/null +++ b/docs/adr/0079-the-last-pass-writes-then-ships.md @@ -0,0 +1,134 @@ +# 0079. The last pass writes, and then ships what it wrote + +Date: 2026-09-11 +Status: Accepted +Amends: 0068 §4, 0075 + +Reverses the order of the two halves of shutdown and adds the second one. The +shutdown flush now runs before a final, time-bounded shipping round instead of +after shipping has already stopped. Adds one constant, `FinalRoundBudget`, which +the chart's grace-period floor is now asserted against. Changes no payload, no +natural key, no cadence and nothing about what is collected. + +## Context + +The controller's shutdown had two halves that could not help each other. + +Shipping was a lifecycle task like the watchers: `tasks["ship"] = ship.Run`, +cancelled by the same context the signal cancels. `Shipper.Run` returns on +`ctx.Done()` and `Round` bails on `ctx.Err()`, so from SIGTERM onward the +shipper does nothing at all. Only after `wg.Wait()` — after the shipper has +returned — did `runFlushers(flushers, true)` write the journals and the +inventory that had accumulated since the last periodic pass. + +So the agent's last act was to write payloads into a directory nothing would +read again. Not "we do not ship on the way out": *we specifically do not ship +the thing we specifically stayed alive to write.* ADR 0068 §4 sized the grace +period around that flush and called it the reason the floor is 15 seconds. + +What makes it loss rather than delay is the volume. The spool is an `emptyDir` +by decision (ADR 0007, ADR 0026), asserted by +`TestTheSpoolIsAlwaysAnEmptyDir`. A container restart keeps it — and since +ADR 0072 and ADR 0077 the next process reads its open windows back. A **pod** +replacement does not: a node drain, a chart upgrade or an eviction takes the +directory with the pod, and everything in it that had not been delivered is +gone. + +How much that is depends entirely on whether shipping was configured and +working. With a healthy backend the exposure is small — payloads are written +every minute and offered every thirty seconds, so the backend already holds the +open hour as of a minute ago, and what is lost is the tail plus the shutdown +flush itself. With no backend configured, which is every installation today +(`security.md` §5), the spool is the only copy and a replaced pod loses all of +it. With a backend that has been down, it loses everything since it went down. + +The reason the second half was not simply added is the constraint in the way. +ADR 0075 states it: `Run` returns only on cancellation, so a backend that is +down, hostile or absent never reaches the agent's `cancel()`. A final round is +by definition a network call on the exit path, and the whole value of that rule +is that a backend cannot influence the agent's lifecycle. A round that could +block would be a backend holding somebody's node drain open. + +## Decision + +**1. The shutdown pass writes first and ships second.** `runShutdown` runs the +flushers and then offers the spool once. The order is the decision and it lives +in one named function, tested directly, rather than in the sequence of two +statements at the end of a long one. + +**2. The round is bounded by a budget that is not negotiable.** +`FinalRoundBudget` is four seconds for the whole round, from a fresh context — +not the cancelled one the signal produced, because this round exists precisely +because everything else has stopped. + +One cap suffices, and this is worth stating because two were considered. `post` +derives its own deadline from the round's, and the earlier of the two wins, so +no request in flight can outlive the budget. A shorter per-request timeout would +buy nothing on top: `Round` already stops at the first payload the backend could +not take, so a stalled request ends the round either way. + +**3. The budget is a constant, not a value.** It is the one knob that, set to +sixty by someone reasoning locally, turns a node drain into a five-minute wait +for a backend that is never coming back. Cadences are constants of the protocol +version for the same reason (ADR 0073); this is the same class. + +**4. Four seconds, because the floor must not move.** The chart refuses a +controller grace period below 15 (`_helpers.tpl`), and installations that set it +explicitly to 15 would break if the floor rose. ADR 0068 §4 measured the flush +path at about ten seconds; ten plus four is fourteen, so the floor stays where +it is and no existing install is affected. + +**5. The floor is asserted against the constant.** ADR 0068 §4 said the grace +period's job was "to be a number that was chosen, and that a later change to the +shutdown path has to be measured against". This is that change, and the +measurement is a test rather than a memory: +`TestTheGracePeriodFloorCoversTheShutdownPath` fails if the budget grows past +what the floor covers. Two numbers that merely agree drift (ADR 0073 §5). + +**6. What the round does not do.** It does not run when the shipper is halted on +a 401 or 403 — nothing retries into a rejected credential, and a fleet's worth of +final rounds arriving at a backend that has already said no is the worst version +of it. It does not change delivery semantics: a payload leaves the spool only on +a 2xx, one request per natural key, undelivered files stay. It does not run at +all when no backend is configured, so today's installations write their last pass +exactly as before. + +**7. Its result can only be logged, and that is a property of the order, not an +omission.** The coverage payload is written by the flush that runs *before* this +round, so it cannot carry the round's own outcome; writing coverage afterwards +would produce a payload guaranteed never to be sent. Nothing scrapes the metrics +endpoint of a process that is exiting. So the log carries what was delivered, +what was deferred, and whether the budget ran out — the last of which is how +anyone would learn that four seconds was the wrong number. + +## Consequences + +**Easier.** A graceful pod replacement — a drain, a rolling upgrade, an +eviction, the three ways a controller pod is normally replaced — now delivers +what it just wrote instead of writing it onto a disappearing disk. The shutdown +flush that ADR 0068 sized the grace period around finally has a consumer on the +same path. + +The exposure that remains is honest and bounded: what a *hard* kill loses (no +pass runs at all), and what a round that ran out of budget or met a down backend +could not deliver. Both were already lost before this; neither is new. + +**Harder / given up.** The agent takes up to four seconds longer to leave, every +time, on installations with a backend configured. That is four seconds of +somebody's node drain, and it is spent whether or not there was anything to +send — a round with an empty spool is one directory listing, so in practice the +cost is only paid when there is something to pay it for. + +The exit path now makes network calls, which it never did. The bound is the +whole answer to that, and the bound is tested against a backend that never +answers rather than argued for. + +**Not changed.** Any payload, any natural key, any cadence, the one-way +protocol, what is collected or filtered, or the rule that a payload leaves the +spool only when the backend has it. The spool is still an `emptyDir` and still +loses what a replaced pod could not deliver; this decision shrinks that set, it +does not make the volume durable, and nothing here should be read as claiming +otherwise. + +This ADR records decisions implemented in the same pull request, per the process +in [`README.md`](README.md). diff --git a/docs/adr/README.md b/docs/adr/README.md index bf9b2d1..59ae8fc 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -136,3 +136,4 @@ line to paste. - [0076. The payload travels encoded, and the encoding is not what it is](0076-the-payload-travels-encoded.md) - [0077. A restart resumes every open window, not only the usage one](0077-a-restart-resumes-every-open-window.md) - [0078. A journal window says whether it is finished](0078-a-journal-window-says-whether-it-is-finished.md) +- [0079. The last pass writes, and then ships what it wrote](0079-the-last-pass-writes-then-ships.md) diff --git a/internal/chartrender/chart_test.go b/internal/chartrender/chart_test.go index 3605168..f4ed5f9 100644 --- a/internal/chartrender/chart_test.go +++ b/internal/chartrender/chart_test.go @@ -8,6 +8,7 @@ import ( "strconv" "strings" "testing" + "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -18,6 +19,7 @@ import ( "github.com/RebuildStackCo/runtime-agent/internal/chartrender" "github.com/RebuildStackCo/runtime-agent/internal/config" + "github.com/RebuildStackCo/runtime-agent/internal/shipper" "github.com/RebuildStackCo/runtime-agent/internal/sink" ) @@ -925,6 +927,26 @@ func TestAGracePeriodTooShortForTheShutdownPassIsRefused(t *testing.T) { } } +// The floor has to cover the whole shutdown path, and the path grew: the flush +// pass, measured at about ten seconds (ADR 0068 §4), is now followed by a +// bounded shipping round (ADR 0079). +// +// Against the constant, not a literal: two numbers that merely agree drift +// (ADR 0073 §5), and this one drifting lands as a SIGKILL mid-flush. +func TestTheGracePeriodFloorCoversTheShutdownPath(t *testing.T) { + const measuredFlushCeiling = 10 * time.Second + required := int((measuredFlushCeiling + shipper.FinalRoundBudget) / time.Second) + + values := map[string]any{ + "profile": "inventory", + "controller": map[string]any{"terminationGracePeriodSeconds": required - 1}, + } + if _, err := chartrender.Manifests(chartDir, chartrender.Options{Values: values}); err == nil { + t.Errorf("the chart accepted %ds, which does not cover a %s flush pass plus a %s shipping round", + required-1, measuredFlushCeiling, shipper.FinalRoundBudget) + } +} + // The memory limit has to reach the process, because the Go runtime does not // read it: it reads the CPU limit and has no memory equivalent (ADR 0068 §5). // The container name matters — a resourceFieldRef naming the wrong container diff --git a/internal/shipper/shipper.go b/internal/shipper/shipper.go index 58a71cf..b35ffb0 100644 --- a/internal/shipper/shipper.go +++ b/internal/shipper/shipper.go @@ -45,6 +45,15 @@ const MaxPayloadBytes = 4 << 20 // through. A round with an empty spool is one directory listing. const DefaultInterval = 30 * time.Second +// FinalRoundBudget is the whole of what shipping may take on the way out, and +// the reason it is small: the spool is an emptyDir, so the round is the last +// chance these bytes have — but a backend that is down, hostile or absent must +// not be able to hold up a pod's termination (ADR 0075, ADR 0079). +// +// The chart's grace-period floor has to cover this plus the flush pass before +// it, which a test in internal/chartrender asserts against this constant. +const FinalRoundBudget = 4 * time.Second + const ( // requestTimeout bounds one delivery. The same 30 seconds the node→ // controller shippers use; nothing here is streaming. @@ -83,10 +92,11 @@ type Shipper struct { failures int counts model.Shipping - // interval and jitter are fields only so a test can compress the first and - // fix the second. Production assigns neither. - interval time.Duration - jitter func(n int64) int64 + // interval, finalBudget and jitter are fields only so a test can compress + // the first two and fix the third. Production assigns none of them. + interval time.Duration + finalBudget time.Duration + jitter func(n int64) int64 } // New returns a shipper for base, or nil when base is empty — the unconfigured @@ -103,8 +113,9 @@ func New(base, dir string, logger *slog.Logger) *Shipper { held: map[string]os.FileInfo{}, // #nosec G404 -- jitter spreads a fleet's retries over a window; it is // not a secret and predicting it buys nothing - jitter: rand.Int64N, - interval: DefaultInterval, + jitter: rand.Int64N, + interval: DefaultInterval, + finalBudget: FinalRoundBudget, } } @@ -132,6 +143,35 @@ func (s *Shipper) Run(ctx context.Context) error { } } +// FinalRound is the one round that happens after the agent has stopped +// collecting, on the volume that goes with the pod (ADR 0079). +// +// ctx is the caller's, not the cancelled one the signal produced — this round +// runs precisely because everything else has stopped. The budget bounds all of +// it, requests included: post derives its deadline from this one, and the +// earlier of the two wins. Undelivered payloads stay in the spool as always; +// what differs is that no next process may read them. +func (s *Shipper) FinalRound(ctx context.Context) { + if s.halted.Load() { + return // nothing retries into a rejected credential, this least of all + } + ctx, cancel := context.WithTimeout(ctx, s.finalBudget) + defer cancel() + + before := s.Coverage() + s.Round(ctx) + after := s.Coverage() + + // The log is the only reader this can have. The coverage payload was + // written by the flush that ran before this, so it cannot carry the result, + // and nothing scrapes the metrics endpoint of a process that is exiting. + s.logger.Info("final shipping round finished", + "delivered", after.Delivered-before.Delivered, + "deferred", after.Deferred-before.Deferred, + "budget", s.finalBudget, + "budget_spent", ctx.Err() != nil) +} + // Round offers every payload the spool holds, oldest first, one at a time. // // Oldest first is the sweep's own eviction order (ADR 0042): what is closest to diff --git a/internal/shipper/shipper_test.go b/internal/shipper/shipper_test.go index 939e4b1..9413195 100644 --- a/internal/shipper/shipper_test.go +++ b/internal/shipper/shipper_test.go @@ -622,6 +622,86 @@ func TestAnInterruptedDeliveryLeavesThePayloadToBeSentAgain(t *testing.T) { // --- helpers --- +// The property the whole final round is bounded for: a backend that never +// answers must not be able to hold up a pod's termination. Everything else +// about this round is best effort; this is not (ADR 0075, ADR 0079). +func TestAHangingBackendCannotHoldUpShutdown(t *testing.T) { + release := make(chan struct{}) + backend := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + <-release + })) + defer backend.Close() + defer close(release) + + s, dir := newShipper(t, backend.URL) + s.finalBudget = 200 * time.Millisecond + payload(t, dir, "collection-coverage.json", "collection_coverage") + + start := time.Now() + s.FinalRound(t.Context()) + elapsed := time.Since(start) + + if elapsed > 2*time.Second { + t.Errorf("the final round took %s against a %s budget; a hung backend delays shutdown", + elapsed, s.finalBudget) + } + // Undelivered is undelivered: the file stays, exactly as in any other round. + if _, err := os.Stat(filepath.Join(dir, "collection-coverage.json")); err != nil { + t.Errorf("an undelivered payload was removed from the spool: %v", err) + } +} + +// The round is the ordinary one under a deadline, so what it delivers it +// deletes and what it does not it leaves. +func TestTheFinalRoundDeliversWhatItCanAndKeepsTheRest(t *testing.T) { + backend := answering(http.StatusOK) + defer backend.Close() + + s, dir := newShipper(t, backend.URL) + payload(t, dir, "collection-coverage.json", "collection_coverage") + payload(t, dir, "usage-1-3600.json", "usage_snapshot") + + s.FinalRound(t.Context()) + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Errorf("the spool still holds %d payloads a 200 acknowledged", len(entries)) + } + if got := s.Coverage().Delivered; got != 2 { + t.Errorf("delivered %d, want 2", got) + } +} + +// Nothing retries into a rejected credential, and the last round least of all: +// it would be a fleet's worth of spools arriving at once at a backend that has +// already said no. +func TestAHaltedShipperRunsNoFinalRound(t *testing.T) { + var requests atomic.Int64 + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusUnauthorized) + })) + defer backend.Close() + + s, dir := newShipper(t, backend.URL) + payload(t, dir, "collection-coverage.json", "collection_coverage") + + s.Round(t.Context()) // the 401 that halts it + if !s.halted.Load() { + t.Fatal("a 401 did not halt the shipper") + } + before := requests.Load() + + s.FinalRound(t.Context()) + + if got := requests.Load(); got != before { + t.Errorf("the final round made %d more requests into a rejected credential", got-before) + } +} + func newShipper(t *testing.T, base string) (*Shipper, string) { t.Helper() dir := t.TempDir()