amadan
fix(activator): a seed that exits 0 is a success, even if it leaks its output pipe
cmd.WaitDelay reports on the success path too: a seed that exits 0 after backgrounding a child that inherits stderr makes Run return ErrWaitDelay instead of nil. Reading that as failure failed the wake AND deleted the database the seed had just correctly built, so the next wake was fresh again — a permanently un-wakeable host. The exit status is the verdict; the leaked pipe is a log line. The cleanup's guard is also fail-safe now: seedCreatedTheDB arms removal only on a definite os.IsNotExist, so a stat that merely failed reads as "there might be data here" rather than "nothing was there".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+77
−4
internal/activator/activator_test.go+51 −0internal/activator/backend_exec.go+26 −4
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go| index 3f24130df..ff981a8f1 100644 |
| --- a/internal/activator/activator_test.go |
| +++ b/internal/activator/activator_test.go |
| @@ -24,6 +24,7 @@ import ( |
| "strings" |
| "sync" |
| "sync/atomic" |
| + "syscall" |
| "testing" |
| "time" |
| @@ -2995,3 +2996,53 @@ func TestExecBackendSeedFailureKeepsPreexistingDB(t *testing.T) { |
| t.Fatalf("pre-existing database = %q, %v; want it untouched by a failed seed", got, err) |
| } |
| } |
| + |
| +// A seed that exits 0 but backgrounds a child holding its stderr pipe open is |
| +// a SUCCESS. os/exec's WaitDelay fires on the success path too — Run returns |
| +// ErrWaitDelay instead of nil — and treating that as a failure would fail the |
| +// wake and delete the database the seed had just correctly built, leaving the |
| +// host fresh again on the next wake: permanently un-wakeable. |
| +func TestExecBackendSeedSucceedsDespiteLingeringChild(t *testing.T) { |
| + b := testExecBackend(t) |
| + host := "leaky-seed.example.com" |
| + // argv is: seed --db <path> → $3 is the database path. The backgrounded |
| + // sleeper inherits stderr and holds the pipe open past the drain grace. |
| + stageSeedScript(t, b, host, "v1", "printf seeded > \"$3\"\nsleep 5 &\nexit 0\n") |
| + |
| + prevTimeout, prevGrace := seedTimeout, seedDrainGrace |
| + seedTimeout, seedDrainGrace = 10*time.Second, 200*time.Millisecond |
| + t.Cleanup(func() { seedTimeout, seedDrainGrace = prevTimeout, prevGrace }) |
| + |
| + if err := b.Seed(context.Background(), registry.Route{Host: host, Kind: "instance", Seed: `["seed"]`}); err != nil { |
| + t.Fatalf("Seed = %v, want nil — the seed exited 0; a leaked output pipe is not a failed seed", err) |
| + } |
| + got, err := os.ReadFile(b.defaultDBPath(host)) |
| + if err != nil || string(got) != "seeded" { |
| + t.Fatalf("seeded database = %q, %v; want it kept — deleting it would make the host fresh again next wake", got, err) |
| + } |
| +} |
| + |
| +// The delete guard is fail-safe by construction: it arms ONLY on a definite |
| +// "not there". Asserted on the rule itself, because the interesting inputs — |
| +// a stat that failed for a reason other than absence — are exactly the ones an |
| +// integration test can't stage reliably. |
| +func TestSeedCreatedTheDB(t *testing.T) { |
| + for _, tc := range []struct { |
| + name string |
| + statErr error |
| + want bool |
| + }{ |
| + {"database is there", nil, false}, |
| + {"definitely absent", &os.PathError{Op: "stat", Path: "/data/x.db", Err: syscall.ENOENT}, true}, |
| + {"bare ErrNotExist", os.ErrNotExist, true}, |
| + {"unreadable", &os.PathError{Op: "stat", Path: "/data/x.db", Err: syscall.EACCES}, false}, |
| + {"path not traversable", &os.PathError{Op: "stat", Path: "/data/x.db", Err: syscall.ENOTDIR}, false}, |
| + {"i/o error", &os.PathError{Op: "stat", Path: "/data/x.db", Err: syscall.EIO}, false}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + if got := seedCreatedTheDB(tc.statErr); got != tc.want { |
| + t.Fatalf("seedCreatedTheDB(%v) = %v, want %v — an inconclusive stat must never arm the removal of a database", tc.statErr, got, tc.want) |
| + } |
| + }) |
| + } |
| +} |
diff --git a/internal/activator/backend_exec.go b/internal/activator/backend_exec.go| index 29d931837..517ff8369 100644 |
| --- a/internal/activator/backend_exec.go |
| +++ b/internal/activator/backend_exec.go |
| @@ -203,10 +203,10 @@ func (b *ExecBackend) Seed(ctx context.Context, rt registry.Route) error { |
| } |
| // Whether the database existed BEFORE the seed ran decides whether a |
| - // failed seed may clean up after itself (below). On the wake path it never |
| - // does — Restore reported fresh — so the normal answer is false. |
| + // failed seed may clean up after itself (below). On the wake path it is |
| + // always absent — Restore reported fresh. |
| _, statErr := os.Stat(dbPath) |
| - dbPreexisting := statErr == nil |
| + seedOwnsDB := seedCreatedTheDB(statErr) |
| ctx, cancel := context.WithTimeout(ctx, seedTimeout) |
| defer cancel() |
| @@ -228,6 +228,18 @@ func (b *ExecBackend) Seed(ctx context.Context, rt registry.Route) error { |
| cmd.WaitDelay = seedDrainGrace |
| log.Printf("activator: seeding %s: %s %s", host, b.liveLink(host), strings.Join(argv, " ")) |
| if err := cmd.Run(); err != nil { |
| + // WaitDelay reports on the SUCCESS path too: a seed that exits 0 after |
| + // backgrounding something (a helper, a stray `&`) leaves the inherited |
| + // stderr pipe open, and Run then returns ErrWaitDelay INSTEAD of nil |
| + // even though the process exited 0. Treating that as a failure would |
| + // fail the wake AND delete the database the seed had just correctly |
| + // built — leaving the host fresh again next wake, i.e. permanently |
| + // un-wakeable. The exit status is the verdict; the leaked pipe is a |
| + // log line. |
| + if errors.Is(err, exec.ErrWaitDelay) && cmd.ProcessState != nil && cmd.ProcessState.Success() { |
| + log.Printf("activator: seeded %s (it exited 0 but left a child holding its output pipe open for more than %s)", host, seedDrainGrace) |
| + return nil |
| + } |
| if errors.Is(ctx.Err(), context.DeadlineExceeded) { |
| err = fmt.Errorf("timed out after %s and was killed: %w", seedTimeout, err) |
| } |
| @@ -238,7 +250,7 @@ func (b *ExecBackend) Seed(ctx context.Context, rt registry.Route) error { |
| // half-seeded instance this whole path exists to prevent. Only a |
| // database that wasn't there when we started is ours to remove, and |
| // under the lease nothing else can have written it. |
| - if !dbPreexisting { |
| + if seedOwnsDB { |
| for _, p := range []string{dbPath, dbPath + "-wal", dbPath + "-shm"} { |
| _ = os.Remove(p) |
| } |
| @@ -252,6 +264,16 @@ func (b *ExecBackend) Seed(ctx context.Context, rt registry.Route) error { |
| return nil |
| } |
| +// seedCreatedTheDB decides, from the stat taken just BEFORE a seed ran, |
| +// whether the database at that path can only be the seed's own work — the |
| +// single condition under which a failed seed's cleanup may delete it. |
| +// |
| +// Fail-safe by construction: ONLY a definite "not there" says yes. A stat that |
| +// merely failed — a permission error, an I/O error, a path that stopped being |
| +// traversable — means "there might be data here", and a guard on deleting live |
| +// data must never arm itself because we couldn't look. |
| +func seedCreatedTheDB(statErr error) bool { return os.IsNotExist(statErr) } |
| + |
| // seedTimeout bounds a first-boot seed run — a wedged seed must fail the wake |
| // (which releases the lease) rather than sit on it. seedDrainGrace is how long |
| // Run then waits for the killed seed's output pipes to close before giving up |