amadan
host: a canary group is settled only by its own pointer read, at the generation and age the gate asks (#336)
Final review finding 6. canaryHeads keeps, per (account, app, channel), the head, the app scope's generation captured before the read, and its time; canarySettled requires the group's own generation to equal the one Due returned and its own read to be younger than the gate's maximum age (ReadGate.MaxAge). Another channel of the same app marking the app's generation no longer settles a group whose restart failed and rolled back.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+81
−13
internal/host/agent.go+25 −13internal/host/readgate_test.go+51 −0internal/release/changewatch.go+5 −0
diff --git a/internal/host/agent.go b/internal/host/agent.go| index 0c1357ab9..a27428a0a 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -2191,7 +2191,17 @@ type fastDeps struct { |
| // backstop's age, or a route of the group is not live on the head that |
| // read returned (canaryHeads). nil: every pointer is read, as before. |
| canaryGate *release.ReadGate |
| - canaryHeads map[[3]string]string // (account, app, channel) -> the head its last good read returned |
| + canaryHeads map[[3]string]canaryRead // (account, app, channel) -> its group's last good read |
| +} |
| + |
| +// canaryRead is one canary group's last good pointer read: the head it |
| +// returned, the app scope's generation captured before it, and when. The |
| +// gate's mark is per app scope, so another channel of the same app can mark |
| +// it; a group is settled only on its own read (final review R1, finding 6). |
| +type canaryRead struct { |
| + head string |
| + gen uint64 |
| + at time.Time |
| } |
| // restartFailure is one host's cross-pass memo of a version its restart |
| @@ -2771,9 +2781,10 @@ func fastTick(ctx context.Context, d *fastDeps, now time.Time) { |
| // canaryPointerGate is the sweep's release.PointerGate at now. Only canary |
| // groups are gated; every other group is read as before. A canary group is |
| -// skipped when its app scope has not moved since its last good read, that |
| -// read is younger than the backstop, and every route of the group is live on |
| -// the head it returned: then a read could only repeat what the box already |
| +// skipped when its app scope has not moved since the group's own last good |
| +// read (another channel's read of the same app does not count), that read is |
| +// younger than the backstop, and every route of the group is live on the |
| +// head it returned: then a read could only repeat what the box already |
| // runs. Anything unsettled (an unstaged route, a failed restart rolled back, |
| // a proof wait, a build the update settings chose over the head) keeps |
| // today's read on every tick. |
| @@ -2788,33 +2799,34 @@ func (d *fastDeps) canaryPointerGate(now time.Time) release.PointerGate { |
| } |
| k := [3]string{acct, app, channel} |
| gen, due := d.canaryGate.Due(scope, now) |
| - if !due && d.canarySettled(k, routes) { |
| + if !due && d.canarySettled(k, routes, gen, now) { |
| return true, nil |
| } |
| return false, func(ptr *release.ChannelPointer, err error) { |
| if d.canaryHeads == nil { |
| - d.canaryHeads = map[[3]string]string{} |
| + d.canaryHeads = map[[3]string]canaryRead{} |
| } |
| if err != nil || ptr == nil { |
| delete(d.canaryHeads, k) |
| d.canaryGate.Mark(scope, gen, now, false) |
| return |
| } |
| - d.canaryHeads[k] = ptr.Version |
| + d.canaryHeads[k] = canaryRead{head: ptr.Version, gen: gen, at: now} |
| d.canaryGate.Mark(scope, gen, now, true) |
| } |
| } |
| } |
| -// canarySettled reports whether every route of group k is live on the head |
| -// the group's last good read returned. |
| -func (d *fastDeps) canarySettled(k [3]string, routes []registry.Route) bool { |
| - head, ok := d.canaryHeads[k] |
| - if !ok || head == "" || len(routes) == 0 { |
| +// canarySettled reports whether group k's own last good read is at gen (the |
| +// generation Due returned) and younger than the gate's maximum age, and every |
| +// route of the group is live on the head that read returned. |
| +func (d *fastDeps) canarySettled(k [3]string, routes []registry.Route, gen uint64, now time.Time) bool { |
| + r, ok := d.canaryHeads[k] |
| + if !ok || r.head == "" || len(routes) == 0 || r.gen != gen || now.Sub(r.at) >= d.canaryGate.MaxAge() { |
| return false |
| } |
| for _, rt := range routes { |
| - if v, err := release.LiveVersion(d.optDir, rt.Host); err != nil || v != head { |
| + if v, err := release.LiveVersion(d.optDir, rt.Host); err != nil || v != r.head { |
| return false |
| } |
| } |
diff --git a/internal/host/readgate_test.go b/internal/host/readgate_test.go| index 1e3b321e6..34bec6863 100644 |
| --- a/internal/host/readgate_test.go |
| +++ b/internal/host/readgate_test.go |
| @@ -296,6 +296,57 @@ func TestCanaryPointerReadWhenTheTicksIndexPollFails(t *testing.T) { |
| } |
| } |
| +// Final review R1 finding 6 (amadan #336): two canary channels of one app. |
| +// canary/dev's promote fails its restart and rolls back, so its route sits |
| +// out the sweep in its restart hold; canary/beta reads its own pointer |
| +// meanwhile, which marks the app's generation. When dev's hold ends its |
| +// cached head still matches its rolled-back live version, but its own last |
| +// read predates the app's generation, so it reads again and retries the |
| +// promote. Before, beta's read settled dev until the backstop. |
| +func TestCanaryGroupFreshnessIsPerChannel(t *testing.T) { |
| + t.Parallel() |
| + k := newCanaryKit(t, true) |
| + ctx := context.Background() |
| + const betaHost = "beta.example.com" |
| + if err := release.Promote(ctx, k.ds, k.env.priv, "bab", "fastapp", "canary/beta", "v1"); err != nil { |
| + t.Fatal(err) |
| + } |
| + k.env.addRoute(t, betaHost, "canary/beta") |
| + devKey := release.ChannelKey("bab", "fastapp", "canary/dev") |
| + betaKey := release.ChannelKey("bab", "fastapp", "canary/beta") |
| + tick := func(d time.Duration) (dev, beta int) { |
| + k.clock.add(d) |
| + fastTick(ctx, k.d, k.clock.now()) |
| + return k.cs.count("Get", devKey), k.cs.count("Get", betaKey) |
| + } |
| + if dev, beta := tick(0); dev != 1 || beta != 1 { |
| + t.Fatalf("first tick: dev %d beta %d pointer reads, want 1 each", dev, beta) |
| + } |
| + if dev, beta := tick(2 * time.Second); dev != 1 || beta != 1 { |
| + t.Fatalf("a quiet tick read a pointer: dev %d beta %d", dev, beta) |
| + } |
| + |
| + k.ship(t, "v2") // canary/dev -> v2; the restart fails and rolls back: |
| + k.d.restartHold = map[string]*restartFailure{canaryHost: {ver: "v2", step: time.Minute, notBefore: k.clock.now().Add(time.Minute)}} |
| + if dev, beta := tick(2 * time.Second); dev != 1 || beta != 2 { |
| + t.Fatalf("with dev held: dev %d beta %d pointer reads, want dev untouched and beta read", dev, beta) |
| + } |
| + if k.live(t) != "v1" { |
| + t.Fatalf("fixture: the held route moved to %q", k.live(t)) |
| + } |
| + delete(k.d.restartHold, canaryHost) // the hold ends |
| + dev, beta := tick(2 * time.Second) |
| + if dev != 2 { |
| + t.Fatalf("dev's pointer read %d time(s) after its hold ended, want 2: beta's read settled dev's group", dev) |
| + } |
| + if beta != 2 { |
| + t.Fatalf("beta's pointer read again (%d) with nothing moved", beta) |
| + } |
| + if k.live(t) != "v2" { |
| + t.Fatalf("dev did not retry the promote: live %q", k.live(t)) |
| + } |
| +} |
| + |
| // A stable route keeps today's path: the gate never skips its pointer. |
| func TestCanaryGateLeavesStableRoutes(t *testing.T) { |
| t.Parallel() |
diff --git a/internal/release/changewatch.go b/internal/release/changewatch.go| index df0257711..73dbb9e1b 100644 |
| --- a/internal/release/changewatch.go |
| +++ b/internal/release/changewatch.go |
| @@ -198,6 +198,11 @@ func NewReadGate(w *ChangeWatch, maxAge time.Duration) *ReadGate { |
| return &ReadGate{w: w, maxAge: maxAge, marks: map[string]gateMark{}} |
| } |
| +// MaxAge is the longest a successful read stands in for the next one. A |
| +// caller keeping finer marks of its own under one scope (the agent's canary |
| +// groups, one per channel) ages them by it too. |
| +func (g *ReadGate) MaxAge() time.Duration { return g.maxAge } |
| + |
| // Due reports whether scope must be read now and returns the generation to |
| // pass to Mark once the read is done. Capture it BEFORE the read: a write that |
| // lands during the read then leaves the scope due on the next check instead |