amadan
memory drop-in under a lock; the restart watch keeps wall time and a present counter (#284)
Summary
Astra's third pass on slice A: three P2s. - writeMemoryDropIn runs under a flock on the drop-in's directory, so a run by hand overlapping a timer's cannot remove the reload marker the other just left. - The restart watch strips monotonic readings (now.Round(0)): every time it holds is one it could have loaded, so a clock step is measured the same way either side of an agent restart, and a watch it wrote always passes its own validation. - A saved unit history with a missing or null "last" is discarded. Census overlay regenerated.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8 files changed,
+154
−10
internal/host/restartwatch.go+20 −0internal/host/restartwatch_test.go+37 −0internal/ops/flock_unix.go+15 −0internal/ops/flock_windows.go+18 −0internal/ops/litestream_memory.go+17 −3internal/ops/ops_test.go+40 −0internal/provision/plan14_root_reads_test.go+1 −1internal/provision/testdata/plan14-root-reads-ls284.tsv+6 −6
CI cancelled — superseded by a newer push — run details
CI log
=== gate === cd launchctx && go vet ./... && go test ./... -count=1 ok amadan.net/carlos/platform.git/launchctx 0.011s ? amadan.net/carlos/platform.git/launchctx/controlname [no test files] ? amadan.net/carlos/platform.git/launchctx/wireid [no test files] go vet ./... gofmt -l .: clean go test ./... -count=1 -timeout 30m amadan: job cancelled — a newer push superseded it
diff --git a/internal/host/restartwatch.go b/internal/host/restartwatch.go| index 028cfbc06..5f0141b8c 100644 |
| --- a/internal/host/restartwatch.go |
| +++ b/internal/host/restartwatch.go |
| @@ -91,6 +91,11 @@ func (w *restartWatch) tick(now time.Time, units map[string]unitRestartState) ma |
| if units == nil { |
| return nil |
| } |
| + // Wall time only. A time from time.Now carries a monotonic reading that |
| + // a saved-and-loaded one does not, and comparing the two kinds measures a |
| + // clock step differently depending on which side of a restart each time |
| + // came from. Every time this watch holds is one it could have loaded. |
| + now = now.Round(0) |
| if w.Units == nil || now.Before(w.At) || (!w.At.IsZero() && now.Sub(w.At) > restartWindow) { |
| w.Units = map[string]*unitRestartHistory{} |
| } |
| @@ -154,6 +159,21 @@ func loadRestartWatch(path string) restartWatch { |
| if err != nil || json.Unmarshal(b, &w) != nil || !w.wellFormed() { |
| return restartWatch{} |
| } |
| + // A missing or null "last" decodes as 0, which is a real counter value, |
| + // so presence is checked on its own. |
| + var present struct { |
| + Units map[string]struct { |
| + Last *int64 `json:"last"` |
| + } `json:"units"` |
| + } |
| + if json.Unmarshal(b, &present) != nil { |
| + return restartWatch{} |
| + } |
| + for name := range w.Units { |
| + if present.Units[name].Last == nil { |
| + return restartWatch{} |
| + } |
| + } |
| return w |
| } |
diff --git a/internal/host/restartwatch_test.go b/internal/host/restartwatch_test.go| index abdbe2979..1dea08fc7 100644 |
| --- a/internal/host/restartwatch_test.go |
| +++ b/internal/host/restartwatch_test.go |
| @@ -166,6 +166,8 @@ func TestRestartWatchDiscardsASavedWatchItDidNotWrite(t *testing.T) { |
| "another version": `{"v":2,"at":"` + at + `","units":{}}`, |
| "no time": `{"v":1,"units":{"carlos-litestream.service":{"last":5,"quiet_since":"` + at + `"}}}`, |
| "empty unit history": `{"v":1,"at":"` + at + `","units":{"carlos-litestream.service":{}}}`, |
| + "no counter": `{"v":1,"at":"` + at + `","units":{"carlos-litestream.service":{"quiet_since":"` + at + `"}}}`, |
| + "null counter": `{"v":1,"at":"` + at + `","units":{"carlos-litestream.service":{"last":null,"quiet_since":"` + at + `"}}}`, |
| "null unit history": `{"v":1,"at":"` + at + `","units":{"carlos-litestream.service":null}}`, |
| "negative counter": `{"v":1,"at":"` + at + `","units":{"carlos-litestream.service":{"last":-3,"quiet_since":"` + at + `"}}}`, |
| "negative rise": `{"v":1,"at":"` + at + `","units":{"carlos-litestream.service":{"last":5,"quiet_since":"` + at + `","bumps":[{"at":"` + at + `","n":-900}]}}}`, |
| @@ -240,3 +242,38 @@ func TestParseUnitRestarts(t *testing.T) { |
| t.Fatalf("no loaded units = %v, want an empty map, not nil", got) |
| } |
| } |
| + |
| +// The loop hands tick times from time.Now, which carry a monotonic reading; |
| +// what it loads back does not. The watch keeps wall time only, so a watch it |
| +// wrote is one it will accept, and a clock step is measured the same way on |
| +// both sides of an agent restart. |
| +func TestRestartWatchKeepsWallTimeOnly(t *testing.T) { |
| + path := filepath.Join(t.TempDir(), "restarts.json") |
| + var w restartWatch |
| + start := time.Now() |
| + w.tick(start, oneUnit(3)) |
| + w.tick(start.Add(time.Minute), oneUnit(7)) |
| + if err := saveRestartWatch(path, w); err != nil { |
| + t.Fatal(err) |
| + } |
| + loaded := loadRestartWatch(path) |
| + if loaded.Units == nil { |
| + t.Fatal("a watch ticked with time.Now was discarded on load") |
| + } |
| + if got := loaded.tick(start.Add(2*time.Minute), oneUnit(7)); got[lsUnit].Count != 4 { |
| + t.Fatalf("after load: %+v, want the 4 restarts it saved", got[lsUnit]) |
| + } |
| + for _, h := range w.Units { |
| + if h.QuietSince != h.QuietSince.Round(0) || w.At != w.At.Round(0) { |
| + t.Fatalf("the watch kept a monotonic reading: at=%v quiet_since=%v", w.At, h.QuietSince) |
| + } |
| + } |
| + |
| + // A backward step with nothing loaded: still a rebaseline. |
| + var live restartWatch |
| + live.tick(start, oneUnit(10)) |
| + live.tick(start.Add(time.Minute), oneUnit(14)) |
| + if got := live.tick(start.Add(-time.Hour), oneUnit(14)); got[lsUnit].Count != 0 || got[lsUnit].Settled { |
| + t.Fatalf("a live backward step kept %+v", got[lsUnit]) |
| + } |
| +} |
diff --git a/internal/ops/flock_unix.go b/internal/ops/flock_unix.go| new file mode 100644 |
| index 000000000..29c48345a |
| --- /dev/null |
| +++ b/internal/ops/flock_unix.go |
| @@ -0,0 +1,15 @@ |
| +//go:build !windows |
| + |
| +package ops |
| + |
| +import ( |
| + "os" |
| + "syscall" |
| +) |
| + |
| +// lockDir takes an exclusive advisory lock on an open directory, and |
| +// unlockDir drops it. syscall.Flock is on both linux and darwin, which |
| +// covers every platform that runs an ops verb. |
| +func lockDir(d *os.File) error { return syscall.Flock(int(d.Fd()), syscall.LOCK_EX) } |
| + |
| +func unlockDir(d *os.File) { syscall.Flock(int(d.Fd()), syscall.LOCK_UN) } //nolint:errcheck |
diff --git a/internal/ops/flock_windows.go b/internal/ops/flock_windows.go| new file mode 100644 |
| index 000000000..7889789b7 |
| --- /dev/null |
| +++ b/internal/ops/flock_windows.go |
| @@ -0,0 +1,18 @@ |
| +//go:build windows |
| + |
| +package ops |
| + |
| +import ( |
| + "errors" |
| + "os" |
| +) |
| + |
| +// lockDir refuses on Windows rather than pretending to lock. The ops verbs |
| +// run on a Linux box; the Windows build exists only so the carlos CLI |
| +// compiles for workstation use. Refusing is the safe failure: the caller |
| +// writes nothing when the lock cannot be taken. |
| +func lockDir(d *os.File) error { |
| + return errors.New("ops verbs lock box-side (linux) only") |
| +} |
| + |
| +func unlockDir(d *os.File) {} |
diff --git a/internal/ops/litestream_memory.go b/internal/ops/litestream_memory.go| index 55f359c0d..6a8e9fd78 100644 |
| --- a/internal/ops/litestream_memory.go |
| +++ b/internal/ops/litestream_memory.go |
| @@ -90,9 +90,26 @@ func LitestreamMemoryDropIn(n int) string { |
| // to rewrite, and would never reload a cap an earlier run failed to load. |
| // The marker is written first and removed last, so every way of stopping in |
| // between leaves it for the next run. systemd ignores it (no .conf suffix). |
| +// |
| +// The whole of it runs under a lock on the drop-in's directory. The verb is |
| +// run by hand and may be run by a timer, and two runs interleaved could have |
| +// one remove the marker the other had just left for a reload that failed. |
| func writeMemoryDropIn(path string, n int, reload func() error) (changed bool, err error) { |
| want := LitestreamMemoryDropIn(n) |
| pending := path + ".reload-pending" |
| + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { |
| + return false, err |
| + } |
| + dir, err := os.Open(filepath.Dir(path)) |
| + if err != nil { |
| + return false, err |
| + } |
| + defer dir.Close() |
| + if err := lockDir(dir); err != nil { |
| + return false, err |
| + } |
| + defer unlockDir(dir) |
| + |
| prior, readErr := os.ReadFile(path) |
| if readErr != nil && !os.IsNotExist(readErr) { |
| return false, readErr |
| @@ -101,9 +118,6 @@ func writeMemoryDropIn(path string, n int, reload func() error) (changed bool, e |
| if _, err := os.Lstat(pending); current && os.IsNotExist(err) { |
| return false, nil |
| } |
| - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { |
| - return false, err |
| - } |
| if err := safefile.WriteFile(pending, nil, 0o644); err != nil { |
| return false, err |
| } |
diff --git a/internal/ops/ops_test.go b/internal/ops/ops_test.go| index 7e9d89a79..3262997e1 100644 |
| --- a/internal/ops/ops_test.go |
| +++ b/internal/ops/ops_test.go |
| @@ -998,3 +998,43 @@ func TestWriteMemoryDropInRetriesAfterAFailedReload(t *testing.T) { |
| t.Fatalf("stray marker: changed=%v err=%v reloads=%d, want true, nil, 4", changed, err, reloads) |
| } |
| } |
| + |
| +// Two runs must not interleave. If a second run could start while the first |
| +// is between its reload and removing its marker, the first would go on to |
| +// remove the marker the second had just left for a reload that failed, and |
| +// the second's cap would sit on disk unloaded for good. |
| +func TestWriteMemoryDropInRunsOneAtATime(t *testing.T) { |
| + path := filepath.Join(t.TempDir(), "d", "50-memory.conf") |
| + second := make(chan error, 1) |
| + firstReload := func() error { |
| + go func() { |
| + _, err := writeMemoryDropIn(path, 300, func() error { return errors.New("no systemd here") }) |
| + second <- err |
| + }() |
| + select { |
| + case err := <-second: |
| + t.Errorf("a second run finished (%v) while the first was still reloading", err) |
| + case <-time.After(300 * time.Millisecond): |
| + } |
| + return nil |
| + } |
| + if changed, err := writeMemoryDropIn(path, 100, firstReload); !changed || err != nil { |
| + t.Fatalf("first run: changed=%v err=%v, want true, nil", changed, err) |
| + } |
| + select { |
| + case err := <-second: |
| + if err == nil { |
| + t.Fatal("the second run's reload was meant to fail") |
| + } |
| + case <-time.After(10 * time.Second): |
| + t.Fatal("the second run never ran after the first finished") |
| + } |
| + // The second run's debt is still on record, and the next run pays it. |
| + if _, err := os.Stat(path + ".reload-pending"); err != nil { |
| + t.Fatalf("the second run's marker is gone: %v", err) |
| + } |
| + reloads := 0 |
| + if changed, err := writeMemoryDropIn(path, 300, func() error { reloads++; return nil }); !changed || err != nil || reloads != 1 { |
| + t.Fatalf("the run after: changed=%v err=%v reloads=%d, want true, nil, 1", changed, err, reloads) |
| + } |
| +} |
diff --git a/internal/provision/plan14_root_reads_test.go b/internal/provision/plan14_root_reads_test.go| index d3c0f085a..00669f8d0 100644 |
| --- a/internal/provision/plan14_root_reads_test.go |
| +++ b/internal/provision/plan14_root_reads_test.go |
| @@ -667,7 +667,7 @@ func TestPlan14RootReadCensus(t *testing.T) { |
| // Census ruling ls284 (#284): the litestream memory drop-in, the unit's |
| // restart backoff and the agent's restart-counter probe and its saved state add seven frozen |
| // declarations. |
| - seal = "4501bab20e84943df805b7727eba4aa64e21da84d56e80da5847187ddc6561c8" |
| + seal = "13bc746ce6afb179c887894b36e18cbc0c1e7852e32317c6811aa8a8d3253c56" |
| errs := append(compareRootReads(got, inv), checkRootReadLegacy(inv, seal)...) |
| errs = append(errs, checkRootReadDeclarations(got, inv)...) |
| if len(errs) > 0 { |
diff --git a/internal/provision/testdata/plan14-root-reads-ls284.tsv b/internal/provision/testdata/plan14-root-reads-ls284.tsv| index 06ecb0245..8a7611406 100644 |
| --- a/internal/provision/testdata/plan14-root-reads-ls284.tsv |
| +++ b/internal/provision/testdata/plan14-root-reads-ls284.tsv |
| @@ -2,21 +2,21 @@ |
| # The ruling is requested from the census owner on amadan #280; these rows state the branch's own account. Earlier overlays are immutable. |
| @legacy f942faf6b1bda4b171de3930c7a9f5627d4db32a2f802c5ecb10ef26410e0af9 internal/console/templates/metrics.html external legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=The alerts table shows a rule's detail beside its name. Same alert state objects, read by the same console sweep; no new source or authority. file:external@0 8732b0e3a4e60029b6c13109be12b15f0a48f51f0fb3c7578e6a52fc26e93140 |
| internal/console/templates/metrics.html external legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=The alerts table shows a rule's detail beside its name. Same alert state objects, read by the same console sweep; no new source or authority. file:external@0 8732b0e3a4e60029b6c13109be12b15f0a48f51f0fb3c7578e6a52fc26e93140 |
| -@legacy 4d3f9d6b930b32c1fd002d7a831aaf4c7317afd20972ba356031c135ffef8a1b internal/host/restartwatch.go loadRestartWatch legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=The agent, as carlos, reads back its own restart-watch state from the data directory's monitor folder, beside the alert state it already keeps there. Feeds an alert only; never read by root and no root decision. file:ReadFile@81 1507415bd6d74eb96c6e23de214e9da98cfa6be25a4888d3ecf95b56c45d508c |
| -internal/host/restartwatch.go loadRestartWatch legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=The agent, as carlos, reads back its own restart-watch state from the data directory's monitor folder, beside the alert state it already keeps there. Feeds an alert only; never read by root and no root decision. file:ReadFile@81 1507415bd6d74eb96c6e23de214e9da98cfa6be25a4888d3ecf95b56c45d508c |
| +@legacy 6d9ea00f5c39150015f9d150c7886e01cd9472ed5835b6279122b8cfd30b781e internal/host/restartwatch.go loadRestartWatch legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=The agent, as carlos, reads back its own restart-watch state from the data directory's monitor folder, beside the alert state it already keeps there. Feeds an alert only; never read by root and no root decision. file:ReadFile@81 d1c0460e2692dfa99a1a5f2454c6d47d5678edcdf5830ed495e323c126fab26a |
| +internal/host/restartwatch.go loadRestartWatch legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=The agent, as carlos, reads back its own restart-watch state from the data directory's monitor folder, beside the alert state it already keeps there. Feeds an alert only; never read by root and no root decision. file:ReadFile@81 d1c0460e2692dfa99a1a5f2454c6d47d5678edcdf5830ed495e323c126fab26a |
| @legacy bc86270eeccc35765f5ecd7771202ed99605065e5e703c23bc14934aa79ddb3d internal/host/restartwatch.go platformUnitRestarts legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=The agent, as carlos, asks systemd for the restart counters of loaded carlos-*.service units; a fixed command with no registry or tenant input, the same class as internal/host/probes.go adoptTimerActive under J4 root-input. Feeds an alert only; no root decision. file:CommandContext@166 fde6a651c0a8a66c324124a2bd3085959c9b575cae08e2d65d2eb666761b0dcc |
| internal/host/restartwatch.go platformUnitRestarts legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=The agent, as carlos, asks systemd for the restart counters of loaded carlos-*.service units; a fixed command with no registry or tenant input, the same class as internal/host/probes.go adoptTimerActive under J4 root-input. Feeds an alert only; no root decision. file:CommandContext@166 fde6a651c0a8a66c324124a2bd3085959c9b575cae08e2d65d2eb666761b0dcc |
| @legacy e39e5045ef43883a78c327f56fbd90e501ff3c012c8ddf512b0f7585b75623c1 internal/host/units/carlos-litestream.service external legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Memory cap default 512M with GOMEMLIMIT, and restart backoff (RestartSteps, RestartMaxDelaySec). User, paths, command and sandboxing unchanged; no new source or authority. file:external@0 2fc82f963202103a16a89aaecdcaeb89c6b9fee8e9d0e09cb76389a2326c1714 |
| internal/host/units/carlos-litestream.service external legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Memory cap default 512M with GOMEMLIMIT, and restart backoff (RestartSteps, RestartMaxDelaySec). User, paths, command and sandboxing unchanged; no new source or authority. file:external@0 2fc82f963202103a16a89aaecdcaeb89c6b9fee8e9d0e09cb76389a2326c1714 |
| -@legacy db8b810137dce7ad80730610211e0a8a2262242dacedf00621e2cca9b3818dcf internal/ops/litestream_memory.go writeMemoryDropIn legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Root reads the root-owned drop-in this verb itself wrote, and lstats its own reload-pending marker beside it, to decide whether to rewrite and reload; the same compare-before-write as writeIfChanged under J4 root-input. The path is fixed or an operator-supplied flag. file:Lstat@342,file:ReadFile@183 32b3e20e28898170b36664fe67715582425888f7e90aeaa9c2afb561799b28fe |
| -internal/ops/litestream_memory.go writeMemoryDropIn legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Root reads the root-owned drop-in this verb itself wrote, and lstats its own reload-pending marker beside it, to decide whether to rewrite and reload; the same compare-before-write as writeIfChanged under J4 root-input. The path is fixed or an operator-supplied flag. file:Lstat@342,file:ReadFile@183 32b3e20e28898170b36664fe67715582425888f7e90aeaa9c2afb561799b28fe |
| +@legacy 8d4ad62d122c3adc99b4ac55ff45d965ec1982a024245825a2a37dd82acec4e1 internal/ops/litestream_memory.go writeMemoryDropIn legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Root opens the drop-in's own directory to hold a lock, reads the root-owned drop-in this verb itself wrote, and lstats its own reload-pending marker beside it, to decide whether to rewrite and reload; the same compare-before-write as writeIfChanged under J4 root-input. The path is fixed or an operator-supplied flag. file:Lstat@613,file:Open@264,file:ReadFile@454 9b0fb46e224f97d4740165498abe45a1c25004a955c6176bce20b08398b46f7c |
| +internal/ops/litestream_memory.go writeMemoryDropIn legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Root opens the drop-in's own directory to hold a lock, reads the root-owned drop-in this verb itself wrote, and lstats its own reload-pending marker beside it, to decide whether to rewrite and reload; the same compare-before-write as writeIfChanged under J4 root-input. The path is fixed or an operator-supplied flag. file:Lstat@613,file:Open@264,file:ReadFile@454 9b0fb46e224f97d4740165498abe45a1c25004a955c6176bce20b08398b46f7c |
| @legacy dc2175dd0445552045146497dd473327ec0ddca2de728f6b8a2c5c755b82420d internal/ops/ops.go runLitestream legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Same registry open and prior-config read. Adds one root-owned write, the memory drop-in beside the unit, sized from the database count this run computed, and a daemon-reload; no new input is trusted. data-literal@214,file:Open@1383,file:ReadFile@2357 380a01961cc02f2a18673bc4ad0a0bc1112905d4726f8ab9e7b370da36686306 |
| internal/ops/ops.go runLitestream legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Same registry open and prior-config read. Adds one root-owned write, the memory drop-in beside the unit, sized from the database count this run computed, and a daemon-reload; no new input is trusted. data-literal@214,file:Open@1383,file:ReadFile@2357 380a01961cc02f2a18673bc4ad0a0bc1112905d4726f8ab9e7b370da36686306 |
| @legacy cb8030cd13f53803aca45d166742f194f1cb00e1d881263a776b79a334a37189 internal/ops/ops.go systemdDaemonReload legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Root runs a fixed systemctl daemon-reload with no caller input, the same class as internal/host/bootstrap.go systemctl under J4 root-input. file:Command@48 3edadda9e71b806a78d14160f2da0b6091212c4ee55933a327856be811290a74 |
| internal/ops/ops.go systemdDaemonReload legacy accepted-legacy accepted-legacy;rev3=census-ruling-ls284;basis=Root runs a fixed systemctl daemon-reload with no caller input, the same class as internal/host/bootstrap.go systemctl under J4 root-input. file:Command@48 3edadda9e71b806a78d14160f2da0b6091212c4ee55933a327856be811290a74 |
| @coverage internal/console 85d4f9521a2acc08aa1276f414be41cea32aa4275f761546e77e951d0d58b558 |
| @coverage internal/console/templates e622e1e901447dc067aba32e4e520eef4279ffcdd807695f012fd38f646d48b4 |
| -@coverage internal/host 563e0efbe9bd9de3e4d6844cb6d650d5cd221681fed5594ecd502d45cef68826 |
| +@coverage internal/host 8bf2f3b97227361f378bbd8943d2d0e157a66f6f90baa5291fb8a13649d4e7de |
| @coverage internal/host/units e06e4a1a0d9d45f8e7494f0419834bf23e14fb2cd4861b94ef905bfaf795a7d1 |
| @coverage internal/metrics 1adc51fc485ba895970c22277b7bff997c24ebededbd19f48a10e75cf531323a |
| -@coverage internal/ops 31e815c0a685f7ef7013c9295a1f570faff3b6846901c215617832b436ffffc1 |
| +@coverage internal/ops 1bd2f9d768a424e8b1fbd1744bba475985b49f43e7ecca836bbadf593fe91ad0 |