amadan
activator: a spell of being held ends when the host serves a request
Review caught the comment claiming a lifecycle the code does not have. The guard block is nested inside the idle-eligible check, so a held host that takes one request skips the guard entirely — it never hibernates, the entry survives, and heldForTick stays stuck true. Every later hold was then silent, which is the opposite of what the line is for. Clear the flag on the not-idle-eligible path, and test the held -> busy -> held cycle: one line per spell, two spells, two lines. Verified the test fails with the fix removed (logs 1, wants 2). Also renames the local `heldForTick` to `hold` so it stops shadowing the field two lines below it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NbP6qc2gf7BFZkQ8QLcm4C
2 files changed,
+56
−5
internal/activator/activator.go+14 −4internal/activator/activator_test.go+42 −1
CI passed — run details
diff --git a/internal/activator/activator.go b/internal/activator/activator.go| index f4a40eca7..a3c2ac95a 100644 |
| --- a/internal/activator/activator.go |
| +++ b/internal/activator/activator.go |
| @@ -271,8 +271,10 @@ type actEntry struct { |
| // TRANSITION rather than on every sweep. A held host trips the guard on |
| // every pass — every 30s at the 15m window, every 22s at 90s — and a line |
| // per pass per host would bury the events an operator actually reads. |
| - // It is only ever cleared by the entry going away, because the sweep's |
| - // only exit from "held" is the hibernate below it. |
| + // Cleared whenever the host is not idle-eligible (busy, or recently |
| + // active), because the sweep skips the guard entirely on that path: a |
| + // held host that took one request would otherwise stay flagged for the |
| + // life of the entry and never log again. |
| heldForTick bool |
| rep Rep |
| @@ -982,8 +984,8 @@ func (a *Service) sweepOnce() { |
| // constant of its own: it is already the deployment's stated |
| // opinion about how long residency is worth holding. |
| next, ok := earliest[e.host] |
| - heldForTick := ok && time.Unix(next, 0).Sub(now) <= a.cfg.Idle |
| - if !heldForTick { |
| + hold := ok && time.Unix(next, 0).Sub(now) <= a.cfg.Idle |
| + if !hold { |
| log.Printf("activator: %s idle %s — hibernating", e.host, now.Sub(e.lastActive).Truncate(time.Second)) |
| a.beginStopLocked(e) |
| continue |
| @@ -1002,6 +1004,14 @@ func (a *Service) sweepOnce() { |
| // for its schedule never reaches a "next wake" to converge on, so |
| // it is now one of the always-awake hosts that arm was written |
| // for — the same class as a sidecar host that never sleeps. |
| + } else { |
| + // Busy, or recently active: this host is not idle-eligible, so |
| + // the guard above did not run and whatever spell of being held it |
| + // was in has ended. Clearing here is what makes the NEXT hold log |
| + // again — without it, a held host that takes a single request |
| + // stays flagged for the life of the entry and every later hold is |
| + // silent, which is the opposite of the point. |
| + e.heldForTick = false |
| } |
| // Converge onto a shipped build. A running child never re-reads |
| // live/<host>, so once a ship repoints the symlink this awake child is |
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go| index 019315536..686a3d40e 100644 |
| --- a/internal/activator/activator_test.go |
| +++ b/internal/activator/activator_test.go |
| @@ -4029,7 +4029,48 @@ func TestGuardLogsHeldOnceNotEverySweep(t *testing.T) { |
| t.Error("entry does not record that it is being held for a tick") |
| } |
| // And the host is still resident: the guard did its job, quietly. |
| - if buf.String() != "" && strings.Contains(buf.String(), "hibernating") { |
| + if strings.Contains(buf.String(), "hibernating") { |
| t.Errorf("host hibernated while held:\n%s", buf.String()) |
| } |
| } |
| + |
| +// A spell of being held ENDS when the host serves a request. The next spell |
| +// is a new event and must log again — otherwise a host that takes one visitor |
| +// goes quiet for the life of its entry, which is the opposite of the point. |
| +func TestGuardLogsAgainAfterABusyInterruption(t *testing.T) { |
| + const host = "interrupted.bab.oncarlos.com" |
| + a, _, clk, reg := testActivator(t, 15*time.Minute) |
| + rt := testRoute(host) |
| + arrangeAwake(t, a, rt) |
| + armTimer(t, reg, host, clk.now().Add(21*time.Minute)) |
| + clk.advance(16 * time.Minute) |
| + |
| + var buf bytes.Buffer |
| + old := log.Writer() |
| + log.SetOutput(&buf) |
| + defer log.SetOutput(old) |
| + |
| + a.sweepOnce() // first spell: one line |
| + if n := strings.Count(buf.String(), "held awake"); n != 1 { |
| + t.Fatalf("first spell logged %d lines, want 1:\n%s", n, buf.String()) |
| + } |
| + |
| + // A visitor lands. The host is no longer idle-eligible, so the sweep |
| + // skips the guard entirely — this is the path that used to leave the |
| + // flag stuck true. |
| + release, err := a.beginRequest(context.Background(), rt) |
| + if err != nil { |
| + t.Fatalf("beginRequest: %v", err) |
| + } |
| + a.sweepOnce() // in flight: not idle-eligible |
| + release() |
| + a.sweepOnce() // idle clock reset by the release: still not eligible |
| + |
| + // Idle again, and still held: a new spell, so a second line. |
| + clk.advance(16 * time.Minute) |
| + a.sweepOnce() |
| + |
| + if n := strings.Count(buf.String(), "held awake"); n != 2 { |
| + t.Errorf("logged %d held-awake lines across two spells, want 2:\n%s", n, buf.String()) |
| + } |
| +} |