amadan
activator: review fixes — no chmod through a symlink, an hourly re-say, no orphan child
Review B1's four minors.
1. os.Chmod is path-based chmod(2) and FOLLOWS a symlink, and the tightening
now runs on every tick for the host rather than once per instance
lifetime. A same-uid sibling — the residual spec §7.2 documents and
accepts — that swapped <host>.env for a link could aim the agent's chmod
at a file of its choosing. Lstat first and refuse the link, exactly as
backend_exec.go already does before normalizing an instance socket ("a
symlink planted at the route's path must not become a mode change
somewhere else"). The Perm check also keeps the common case down to the
Lstat the guard costs anyway, and the failure line now names the mode it
found so the operator learns 0644 from 0666 without an SSM round trip.
2. The complaint memory is a time.Time, not a bool: the chmod is RETRIED
every tick, so the condition can recur — an operator fixes a 0644 file,
something re-widens it weeks later — and a once-per-process latch would
keep the agent silent about a world-readable instance secret until the
next binary roll, which is weeks away. One hour: quiet enough for a 15s
caller, loud enough that a condition which comes back is heard again.
3. The drain test's child traps TERM and polls a file inside t.TempDir(). On
any t.Fatalf before the release file is written — the failure the test
exists to produce, among others — it spun forever on a directory the
cleanup had deleted, immune to the signal the test is about; the reviewer
collected two real orphans that needed kill -9. t.Cleanup kills it.
Verified by injecting an early t.Fatal: no child survives the run.
4. The drain-window guarantee lived only in a Go comment, and an undocumented
guarantee is the one a refactor removes without noticing — which is how
this bug got in. Spec §7.2 now states the socket's lifetime as a decision
in the app contract: bound before the spawn, closed after the child exits,
up through the SIGTERM drain. The code comment cites that instead of
§7.1, which only ever promised the BOOT half.
Two tests added: the symlink refusal, and the throttle saying it again once
the window has elapsed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 files changed,
+137
−28
docs/superpowers/specs/2026-08-23-scheduled-work-design.md+9 −0internal/activator/activator_test.go+6 −0internal/activator/admintoken.go+54 −26internal/activator/admintoken_test.go+65 −0internal/activator/backend_exec.go+3 −2
diff --git a/docs/superpowers/specs/2026-08-23-scheduled-work-design.md b/docs/superpowers/specs/2026-08-23-scheduled-work-design.md| index 6ad9ec92c..2da60869f 100644 |
| --- a/docs/superpowers/specs/2026-08-23-scheduled-work-design.md |
| +++ b/docs/superpowers/specs/2026-08-23-scheduled-work-design.md |
| @@ -461,6 +461,15 @@ channel: always bound for exec children (the env var is therefore always |
| set), with purge mounted only when the box has a cache (`CARLOS_PURGE=header` |
| keeps its "only when true" rule). |
| +**Lifetime (decision):** the socket brackets the process. It is bound BEFORE |
| +the spawn (§7.1) and closed only AFTER the child has exited — so it stays up |
| +through the instance's own `SIGTERM` drain. An app may therefore register or |
| +re-assert timers on the way down as well as at boot: a shutdown hook that |
| +books a retry for work it could not finish reaches a live channel, not a |
| +connection-refused. The agent used to close it ahead of the signal, which |
| +made the drain window the one moment the channel an app had just been handed |
| +was gone. |
| + |
| | Verb | Body / result | |
| |---|---| |
| | `PUT /schedule/{name}` | `{"at": "2026-09-01T08:00:00Z", "path": "/jobs/remind"}` → `200` with the row. Upsert by name. `409` if `name` is a declared schedule. `at` up to 400 days ahead; a past `at` fires on the next runner tick. At most **1000** once-timers per host (`429`). | |
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go| index 96386e86e..7d0b9a92c 100644 |
| --- a/internal/activator/activator_test.go |
| +++ b/internal/activator/activator_test.go |
| @@ -3658,6 +3658,12 @@ func TestChildProcStopKeepsTheControlSocketUpUntilTheChildExits(t *testing.T) { |
| ctl.Close() |
| t.Fatalf("start the fake child: %v", err) |
| } |
| + // The child traps TERM and polls a file inside t.TempDir(). Any t.Fatalf |
| + // before the release file is written — the failure this test exists to |
| + // produce, among others — leaves it spinning on a directory the cleanup |
| + // then deletes, immune to the very signal this test is about: only |
| + // SIGKILL ends it, and a red CI run would leak one per attempt. |
| + t.Cleanup(func() { _ = cmd.Process.Kill() }) |
| p := &childProc{cmd: cmd, done: make(chan struct{}), ctl: ctl} |
| go func() { |
| p.err = cmd.Wait() |
diff --git a/internal/activator/admintoken.go b/internal/activator/admintoken.go| index 9ee178208..77af6d60f 100644 |
| --- a/internal/activator/admintoken.go |
| +++ b/internal/activator/admintoken.go |
| @@ -28,6 +28,7 @@ import ( |
| "os" |
| "strings" |
| "sync" |
| + "time" |
| ) |
| // adminTokenMu serializes minting across the process. Two callers can find |
| @@ -37,29 +38,37 @@ import ( |
| // first spawn's child holds the first. |
| var adminTokenMu sync.Mutex |
| -// adminTokenLogged remembers which (condition, file) pairs have already had |
| -// their complaint logged, so the two conditions below are diagnoses rather |
| -// than a log flood. Both of them are STICKY — a 0644 file the agent cannot |
| -// chmod, a token the wrong shape — and every one of the callers repeats: the |
| -// runner ensures the token on every tick for the host (every 15s while a |
| -// timer is due), so an un-deduplicated line would be thousands a day saying |
| -// the same thing. Read and written only under adminTokenMu. |
| +// adminTokenLogged remembers when each (condition, file) pair last had its |
| +// complaint logged, so the two conditions below are diagnoses rather than a |
| +// log flood. Both are STICKY — a file the agent cannot tighten, a token the |
| +// wrong shape — and every one of the callers repeats: the runner ensures the |
| +// token on every tick for the host (every 15s while a timer is due), so an |
| +// un-throttled line would be thousands a day saying the same thing. Read and |
| +// written only under adminTokenMu. |
| // |
| -// Per-process, deliberately: an agent restart says it again, which is the |
| -// one moment an operator is reading these logs anyway. |
| -var adminTokenLogged = map[string]bool{} |
| +// A TIME and not a bool, because the conditions can also RECUR: the chmod is |
| +// retried every tick, so an operator can fix a 0644 file and something can |
| +// re-widen it weeks later — and a once-per-process bool would keep the agent |
| +// silent about a world-readable instance secret until the next binary roll, |
| +// which is weeks away. An hour is quiet enough for the 15s caller and loud |
| +// enough that a condition which comes back is heard again. |
| +var adminTokenLogged = map[string]time.Time{} |
| -// logOncePerFile emits msg the first time it is asked for this (path, kind) |
| -// pair and stays quiet afterwards. Keyed by kind as well as path so a file |
| -// that is both un-tightenable AND holding a malformed token still reports |
| -// both — one condition must not swallow the other's only line. Caller holds |
| +// adminTokenLogRepeat is how long one (condition, file) complaint stays |
| +// deduplicated before it is said again. |
| +const adminTokenLogRepeat = time.Hour |
| + |
| +// logPerFile emits msg unless this (kind, path) pair has already said it |
| +// within adminTokenLogRepeat. Keyed by kind as well as path so a file that |
| +// is both un-tightenable AND holding a malformed token still reports both — |
| +// one condition must not swallow the other's only line. Caller holds |
| // adminTokenMu. |
| -func logOncePerFile(kind, path, msg string) { |
| +func logPerFile(kind, path, msg string) { |
| key := kind + "\x00" + path |
| - if adminTokenLogged[key] { |
| + if last, seen := adminTokenLogged[key]; seen && time.Since(last) < adminTokenLogRepeat { |
| return |
| } |
| - adminTokenLogged[key] = true |
| + adminTokenLogged[key] = time.Now() |
| log.Print(msg) |
| } |
| @@ -95,8 +104,10 @@ func wellFormedAdminToken(tok string) bool { |
| // created here, already existed in a looser mode, or already held a token — |
| // that is the same mode the env file carries everywhere else, and the only |
| // thing standing between another UNIX user on the box and the instance's |
| -// secret. An existing token is also shape-checked, and a value that isn't |
| -// the shape this mints is reported and left exactly as it is. |
| +// secret — the one exception being a path that turns out to be a symlink, |
| +// which is reported and never chmodded. An existing token is also |
| +// shape-checked, and a value that isn't the shape this mints is reported and |
| +// left exactly as it is. |
| func EnsureAdminToken(path string) error { |
| adminTokenMu.Lock() |
| defer adminTokenMu.Unlock() |
| @@ -113,12 +124,29 @@ func EnsureAdminToken(path string) error { |
| // First, the mode. The chmod below used to run only on the minting |
| // path, so an env file that was already 0644 when the platform found |
| // a token in it stayed 0644 — holding the very secret the mode is |
| - // there to protect. Tightening is idempotent and costs one syscall |
| - // per ensure; failing it does NOT fail the ensure, because the token |
| - // still works and refusing the wake over a mode would trade a |
| - // hardening for an outage. |
| - if err := os.Chmod(path, 0o600); err != nil { |
| - logOncePerFile("chmod", path, fmt.Sprintf("activator: %s holds CARLOS_ADMIN_TOKEN but could not be tightened to 0600: %v — another UNIX user on this box may be able to read the instance's secret", path, err)) |
| + // there to protect. Failing to tighten it does NOT fail the ensure, |
| + // because the token still works and refusing the wake over a mode |
| + // would trade a hardening for an outage. |
| + // |
| + // Lstat first, and never chmod a symlink: os.Chmod is path-based |
| + // chmod(2), which FOLLOWS one. A same-uid sibling — the residual the |
| + // spec's §7.2 documents and accepts — that replaced <host>.env with a |
| + // link would otherwise aim the agent's chmod at a file of its |
| + // choosing, now once every 15s rather than once per instance |
| + // lifetime. backend_exec.go does exactly this dance before it |
| + // normalizes an instance socket ("a symlink planted at the route's |
| + // path must not become a mode change somewhere else") and the hazard |
| + // is the same one. The Perm check also keeps the common case — a file |
| + // already at 0600 — down to the Lstat the guard costs anyway. |
| + switch fi, statErr := os.Lstat(path); { |
| + case statErr != nil: |
| + logPerFile("chmod", path, fmt.Sprintf("activator: %s holds CARLOS_ADMIN_TOKEN but could not be stat'd to check its mode: %v — another UNIX user on this box may be able to read the instance's secret", path, statErr)) |
| + case fi.Mode()&os.ModeSymlink != 0: |
| + logPerFile("chmod", path, fmt.Sprintf("activator: %s is a SYMLINK, not the instance's own env file — refusing to change its target's mode, and the token in it should be treated as compromised (a same-uid process on this box planted this)", path)) |
| + case fi.Mode().Perm() != 0o600: |
| + if err := os.Chmod(path, 0o600); err != nil { |
| + logPerFile("chmod", path, fmt.Sprintf("activator: %s holds CARLOS_ADMIN_TOKEN at mode %#o and could not be tightened to 0600: %v — another UNIX user on this box may be able to read the instance's secret", path, fi.Mode().Perm(), err)) |
| + } |
| } |
| // Second, the shape. A torn write leaves a value that will 401 every |
| // tick, and the platform must say so rather than let the operator |
| @@ -132,7 +160,7 @@ func EnsureAdminToken(path string) error { |
| // one safe repair — stop the instance, delete the line, let the next |
| // spawn mint it. |
| if !wellFormedAdminToken(tok) { |
| - logOncePerFile("shape", path, fmt.Sprintf("activator: CARLOS_ADMIN_TOKEN in %s is not the shape the platform mints (64 hex characters) — left in place, because the running instance holds this same value and rotating it would lock the platform out of a live instance; if it is corrupt, stop the instance, delete the line and restart it to have one minted", path)) |
| + logPerFile("shape", path, fmt.Sprintf("activator: CARLOS_ADMIN_TOKEN in %s is not the shape the platform mints (64 hex characters) — left in place, because the running instance holds this same value and rotating it would lock the platform out of a live instance; if it is corrupt, stop the instance, delete the line and restart it to have one minted", path)) |
| } |
| return nil |
| } |
diff --git a/internal/activator/admintoken_test.go b/internal/activator/admintoken_test.go| index a4dec130b..1da90a94f 100644 |
| --- a/internal/activator/admintoken_test.go |
| +++ b/internal/activator/admintoken_test.go |
| @@ -6,6 +6,7 @@ import ( |
| "path/filepath" |
| "strings" |
| "testing" |
| + "time" |
| ) |
| func TestEnsureAdminTokenMintsOnce(t *testing.T) { |
| @@ -199,3 +200,67 @@ func TestWellFormedAdminToken(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// os.Chmod is path-based chmod(2) and follows symlinks, and this now runs on |
| +// every tick for the host rather than once per instance lifetime. A same-uid |
| +// sibling — the residual spec §7.2 documents — that swaps <host>.env for a |
| +// link must not turn the agent's tightening into a mode change somewhere |
| +// else on the box. Same guard backend_exec.go puts in front of the instance |
| +// socket's normalization, for the same reason. |
| +func TestEnsureAdminTokenRefusesToChmodThroughASymlink(t *testing.T) { |
| + dir := t.TempDir() |
| + target := filepath.Join(dir, "someone-elses-file") |
| + if err := os.WriteFile(target, []byte("CARLOS_ADMIN_TOKEN="+strings.Repeat("ab", 32)+"\n"), 0o644); err != nil { |
| + t.Fatal(err) |
| + } |
| + link := filepath.Join(dir, "h.env") |
| + if err := os.Symlink(target, link); err != nil { |
| + t.Skipf("this filesystem will not make symlinks: %v", err) |
| + } |
| + buf := captureLog(t) |
| + if err := EnsureAdminToken(link); err != nil { |
| + t.Fatal(err) |
| + } |
| + st, err := os.Stat(target) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if st.Mode().Perm() != 0o644 { |
| + t.Fatalf("mode of the symlink's TARGET = %o, want the untouched 644 — the agent must not chmod through a link", st.Mode().Perm()) |
| + } |
| + if !strings.Contains(buf.String(), "SYMLINK") { |
| + t.Fatalf("log = %q, want the refusal named", buf.String()) |
| + } |
| +} |
| + |
| +// The throttle is a clock, not a latch: the chmod is retried every tick, so |
| +// the condition can come back (an operator fixes a 0644 file, something |
| +// re-widens it weeks later) and a once-per-process bool would keep the agent |
| +// silent about a world-readable secret until the next binary roll. |
| +func TestEnsureAdminTokenComplaintIsThrottledNotSilenced(t *testing.T) { |
| + p := filepath.Join(t.TempDir(), "h.env") |
| + if err := os.WriteFile(p, []byte("CARLOS_ADMIN_TOKEN=deadbe\n"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + buf := captureLog(t) |
| + for i := 0; i < 3; i++ { |
| + if err := EnsureAdminToken(p); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if n := strings.Count(buf.String(), "not the shape"); n != 1 { |
| + t.Fatalf("warning appeared %d times inside the throttle window, want 1", n) |
| + } |
| + // Age the memory past the window — what a real recurrence looks like. |
| + adminTokenMu.Lock() |
| + for k := range adminTokenLogged { |
| + adminTokenLogged[k] = time.Now().Add(-adminTokenLogRepeat - time.Minute) |
| + } |
| + adminTokenMu.Unlock() |
| + if err := EnsureAdminToken(p); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n := strings.Count(buf.String(), "not the shape"); n != 2 { |
| + t.Fatalf("warning appeared %d times after the window elapsed, want it said again (2)", n) |
| + } |
| +} |
diff --git a/internal/activator/backend_exec.go b/internal/activator/backend_exec.go| index 566022c07..ab062ede4 100644 |
| --- a/internal/activator/backend_exec.go |
| +++ b/internal/activator/backend_exec.go |
| @@ -925,8 +925,9 @@ func (p *childProc) Stop(ctx context.Context) error { |
| // The control listener OUTLIVES the drain, and the ordering is the whole |
| // point. Closing it first — which is what this did — unbound and unlinked |
| // the socket before the child had even seen SIGTERM, so an app that |
| - // registers or re-asserts its once-timers on the way down (spec §7.1's |
| - // "re-assert at boot, survive restarts" story; §7.2 is the verb) got |
| + // registers or re-asserts its once-timers on the way down (spec §7.2's |
| + // lifetime decision — the socket brackets the PROCESS, bound before the |
| + // spawn and closed after the exit — serving §7.1's re-assert story) got |
| // connection-refused on the channel the platform had just told it about, |
| // during precisely the window it was told to use. Signal first, close |
| // after the process is gone. |