amadan
release: bake clock Save writes atomically, migration lands in one pass — review fixes (#13)
Two problems in the previous commit, both from the same root cause: the bake clock now has two writers (carlos-edge as `carlos`, the root adopt one-shot as root) sharing /data/channel-first-seen.json. CRITICAL: FirstSeen.Save was an in-place os.WriteFile(..., O_TRUNC), which needs write permission on the existing file. Whichever uid creates the file first (0644, owned by that uid) locks the other uid out with no repair — reproducing issue #13 at the new location. Save now writes a temp file next to the target and renames over it, the same co-written-file pattern internal/appconfig/sync.go and syncexec.go already use: rename(2) only needs write permission on the directory, never on the file being replaced. IMPORTANT: Save is changed-gated, so migration off the legacy path was not durable — a box whose fleet saw no new channel head could run for days with the new path still absent, still reading the legacy file every pass, while looking migrated. LoadFirstSeenMigrating now marks the clock `migrated`, and PassFilter forces that first Save regardless of whether anything new was observed, so one completed pass with BakeDir set fully populates the new path and the legacy file is safe to delete. Covering tests: TestFirstSeenSaveReplacesFileItCannotWriteInPlace (bake_test.go) and TestPassMigratesBakeClockDurably (adopt_test.go), both shown red before their fixes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 files changed,
+130
−3
internal/release/adopt.go+12 −1internal/release/adopt_test.go+39 −0internal/release/bake.go+42 −2internal/release/bake_test.go+37 −0
diff --git a/internal/release/adopt.go b/internal/release/adopt.go| index 037bf260d..a5420dc5a 100644 |
| --- a/internal/release/adopt.go |
| +++ b/internal/release/adopt.go |
| @@ -69,6 +69,12 @@ func NagExpiredCanaries(w io.Writer) PassOption { |
| // /opt/carlos/channel-first-seen.json keeps every observation (issue #13). |
| // Unset, the clock stays at its legacy path — existing tests and callers |
| // are unaffected. |
| +// |
| +// The migration is durable after the FIRST pass this option is set on a box |
| +// still on the legacy path: PassFilter forces a Save even when no new |
| +// channel head was observed, so the new path is fully populated (and the |
| +// legacy file provably safe to delete) one pass later, not whenever the |
| +// fleet next happens to see a new head. |
| func BakeDir(dir string) PassOption { |
| return func(c *passConfig) { c.bakeDir = dir } |
| } |
| @@ -203,7 +209,12 @@ func PassFilter(ctx context.Context, reg *registry.Registry, s Store, pub ed2551 |
| if cfg.bakeDir != "" { |
| bake = LoadFirstSeenMigrating(filepath.Join(cfg.bakeDir, "channel-first-seen.json"), FirstSeenPath(optDir)) |
| } |
| - changed := false |
| + // bake.migrated forces this pass's Save even if nothing new is observed |
| + // below: otherwise a changed-gated Save could leave the new path absent |
| + // for days on a fleet that happens to see no new channel head, and the |
| + // legacy file would look safe to delete long before it actually is (see |
| + // LoadFirstSeenMigrating and BakeDir's doc comments, 2026-08-03 review). |
| + changed := bake.migrated |
| for k, ptr := range pointers { |
| if ptr != nil && bake.Observe(k.acct, k.app, k.channel, ptr.Version, now) { |
| changed = true |
diff --git a/internal/release/adopt_test.go b/internal/release/adopt_test.go| index ecc1f7c63..f7ffb98e1 100644 |
| --- a/internal/release/adopt_test.go |
| +++ b/internal/release/adopt_test.go |
| @@ -104,6 +104,45 @@ func TestPassBakeDirMovesTheClock(t *testing.T) { |
| } |
| } |
| +// TestPassMigratesBakeClockDurably proves the migration off the legacy path |
| +// completes in ONE pass even when nothing new was observed (2026-08-03 |
| +// review, IMPORTANT): Save is changed-gated, so with the head already |
| +// recorded in the legacy clock, Observe reports no change this pass and the |
| +// ordinary "tick and save" path would never write the new location — a box |
| +// could run for days with the new path absent, and deleting the legacy file |
| +// in that window would silently reset the channel's bake window. The legacy |
| +// clock here already has this exact head, so the ONLY way the new path ends |
| +// up populated is the migration forcing its own first Save. |
| +func TestPassMigratesBakeClockDurably(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, pub := testKeys(t) |
| + opt := t.TempDir() |
| + bakeDir := t.TempDir() |
| + reg := testRegistry(t) |
| + |
| + shipRelease(t, s, "bab", "myapp", "v1", adoptPlatform, []byte("build one")) |
| + if err := Promote(ctx, s, priv, "bab", "myapp", "canary/x", "v1"); err != nil { |
| + t.Fatalf("Promote: %v", err) |
| + } |
| + addRoute(t, reg, "awake.example.com", "myapp", "canary/x", false) |
| + |
| + legacy := LoadFirstSeen(FirstSeenPath(opt)) |
| + legacy.Observe("bab", "myapp", "canary/x", "v1", time.Now()) |
| + if err := legacy.Save(); err != nil { |
| + t.Fatalf("seed legacy clock: %v", err) |
| + } |
| + |
| + if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }, BakeDir(bakeDir)); err != nil { |
| + t.Fatalf("Pass: %v", err) |
| + } |
| + |
| + st := LoadFirstSeen(filepath.Join(bakeDir, "channel-first-seen.json")) |
| + if _, ok := st.EligibleAt("bab", "myapp", "canary/x", "v1", DefaultHolds); !ok { |
| + t.Fatal("migrated content did not land in the new path after one pass with no new head observed") |
| + } |
| +} |
| + |
| func TestPassPoisonedBuildSkipsRemainingHosts(t *testing.T) { |
| s := NewDirStore(t.TempDir()) |
| ctx := context.Background() |
diff --git a/internal/release/bake.go b/internal/release/bake.go| index d58cd4585..ad1c8b189 100644 |
| --- a/internal/release/bake.go |
| +++ b/internal/release/bake.go |
| @@ -78,6 +78,13 @@ type FirstSeen struct { |
| Channels map[string]map[string]time.Time `json:"channels"` |
| path string // where Save persists; set by LoadFirstSeen |
| + |
| + // migrated marks a clock LoadFirstSeenMigrating built from its |
| + // legacy-fallback branch (the new path didn't exist yet). PassFilter |
| + // checks this to force an immediate Save even when Observe reports no |
| + // change this pass — see LoadFirstSeenMigrating's doc comment for why |
| + // a changed-gated Save alone isn't enough to make the migration durable. |
| + migrated bool |
| } |
| // channelClockKey identifies one channel head's clock. The ACCOUNT is part of |
| @@ -136,10 +143,22 @@ func LoadFirstSeen(path string) *FirstSeen { |
| // dir). Save always writes path. The legacy file is never written or |
| // deleted: an older binary still running against it keeps a working clock, |
| // and this binary simply stops advancing it. |
| +// |
| +// The returned clock is marked migrated, which its caller (PassFilter) uses |
| +// to force that first Save even when nothing new was observed this pass — |
| +// Save is otherwise changed-gated (bake.go/adopt.go), and a box whose fleet |
| +// happens to see no new channel head could otherwise run for days with path |
| +// still absent, still re-reading legacyPath every pass. That window matters |
| +// because it invites deleting the legacy file as soon as this binary ships, |
| +// which — before the migrated Save lands — would silently wipe every stable |
| +// channel's observations and freeze the fleet on a fresh 72h window. Forcing |
| +// the first Save makes migration complete (path fully populated, |
| +// legacyPath now provably disposable) after exactly one pass. |
| func LoadFirstSeenMigrating(path, legacyPath string) *FirstSeen { |
| if _, err := os.Stat(path); os.IsNotExist(err) { |
| st := LoadFirstSeen(legacyPath) |
| st.path = path |
| + st.migrated = true |
| return st |
| } |
| return LoadFirstSeen(path) |
| @@ -182,13 +201,34 @@ func (st *FirstSeen) Observe(acct, app, channel, version string, now time.Time) |
| return changed |
| } |
| -// Save persists the clock back to the path it was loaded from. |
| +// Save persists the clock back to the path it was loaded from, via a |
| +// temp-file-in-the-same-dir + rename — the same co-written-file pattern |
| +// internal/appconfig/sync.go and syncexec.go already use for /data files |
| +// more than one uid writes (2026-08-03 review, issue #13): carlos-edge.service |
| +// runs as `carlos`, the root adopt one-shot runs as root, and BakeDir makes |
| +// both write this same path. An in-place O_TRUNC write needs write |
| +// permission on the EXISTING file, so whichever uid creates it first (0644, |
| +// owned by that uid) locks the other one out — EACCES with nothing to |
| +// repair the ownership (bootstrap only chowns /data once, at bootstrap). |
| +// rename(2), by contrast, only requires write permission on the containing |
| +// directory, which both uids have on /data, so either side can always |
| +// replace the file — and a crash mid-write leaves the old content intact |
| +// rather than a torn, half-written file. |
| func (st *FirstSeen) Save() error { |
| raw, err := json.MarshalIndent(st, "", " ") |
| if err != nil { |
| return err |
| } |
| - return os.WriteFile(st.path, append(raw, '\n'), 0o644) |
| + raw = append(raw, '\n') |
| + tmp := st.path + ".tmp" |
| + if err := os.WriteFile(tmp, raw, 0o644); err != nil { |
| + return err |
| + } |
| + if err := os.Rename(tmp, st.path); err != nil { |
| + os.Remove(tmp) // don't leave a stray temp file behind |
| + return err |
| + } |
| + return nil |
| } |
| // EligibleAt is the moment a version first seen at a channel head may be |
diff --git a/internal/release/bake_test.go b/internal/release/bake_test.go| index 485fb9a60..082f0720f 100644 |
| --- a/internal/release/bake_test.go |
| +++ b/internal/release/bake_test.go |
| @@ -314,3 +314,40 @@ func TestLoadFirstSeenMigrating(t *testing.T) { |
| } |
| }) |
| } |
| + |
| +// TestFirstSeenSaveReplacesFileItCannotWriteInPlace stands in for the real |
| +// production race (2026-08-03 review, CRITICAL): carlos-edge.service writes |
| +// this path as user `carlos`, the root adopt one-shot writes it as root — |
| +// whichever creates the file first locks the other uid out, because an |
| +// in-place O_TRUNC write needs write permission on the FILE, and a 0644 file |
| +// owned by the other uid has none for anyone else. A file this process |
| +// cannot open for writing (0o444, no write bit for anyone, including its own |
| +// owner) reproduces the same "cannot write in place" shape without needing a |
| +// second uid: rename(2) only requires write permission on the containing |
| +// directory, never on the file being replaced, so a Save implemented as |
| +// temp-file+rename must still succeed here where a truncating write cannot. |
| +func TestFirstSeenSaveReplacesFileItCannotWriteInPlace(t *testing.T) { |
| + path := filepath.Join(t.TempDir(), "clock.json") |
| + if err := os.WriteFile(path, []byte("{}"), 0o444); err != nil { |
| + t.Fatal(err) |
| + } |
| + st := LoadFirstSeen(path) |
| + st.Observe("acct", "app", "stable", "v1", time.Now()) |
| + if err := st.Save(); err != nil { |
| + t.Fatalf("Save must replace via rename, not an in-place write: %v", err) |
| + } |
| + if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) { |
| + t.Fatalf("Save left a temp file behind (stat err: %v)", err) |
| + } |
| + raw, err := os.ReadFile(path) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var reloaded FirstSeen |
| + if err := json.Unmarshal(raw, &reloaded); err != nil { |
| + t.Fatalf("Save produced invalid/partial content: %v", err) |
| + } |
| + if _, ok := reloaded.Channels[channelClockKey("acct", "app", "stable")]["v1"]; !ok { |
| + t.Fatal("observation missing after Save") |
| + } |
| +} |