fix(doctor): budget the retry envelope in the listener acknowledgement grace

[kyle-sexton]

Summary

Implements #100 / audit finding D4 ("widen grace, keep hard fault").

The root cause turned out to be sharper than "the window is too small", so the
fix is a derivation change rather than a bigger multiplier.

listenerAcknowledgementGrace budgeted one request timeout plus two reconcile
intervals, with no retry allowance at all. Twenty lines further down the same
file, observedFreshnessLimit computes a bound over the same GitHub request
path and budgets a full retry envelope. That asymmetry is the defect: a busy
fleet is precisely when those requests retry, and the acknowledgement window had
no room for even one backoff. Benign busy-fleet lag exceeding it was the
predictable consequence, not a surprise.

The window now budgets the complete retry envelope for each of the two request
paths an acknowledgement crosses — the controller advertising the new
capacity, and a later reconcile reading back the pool state that acknowledges it
— plus two reconcile intervals. One envelope is github.retry.maxAttempts
attempts, each capped at github.requestTimeout, separated by backoff waits of
github.retry.maximum × (1 + github.retry.jitterRatio). On the tested config
shape that takes the window from 4m30s to 26m10s, and to 28m34s at
DefaultBackoffPolicy's 0.2 jitter ratio.

It has to be the complete envelope rather than a sample of it, because nothing
else in the doctor notices a poll that is still legitimately retrying:
Reconciler.pollCheckpoint keeps writing observed state on the reconcile cadence
while a poll is open, and deliberately preserves the pool transition timestamp
this age measures. So the heartbeat stays fresh inside observedFreshnessLimit
while the measured age grows monotonically through the poll. A window shorter
than the retry policy hard-faults a listener mid-retry.

Two supporting changes:

• saturatingFreshnessDuration now takes a single composed retryUnits count
  instead of separate attempt and target multipliers, so a caller composing it
  from a different pair of factors does not have to pass one of them through a
  parameter named for the other. Overflow saturation stays one implementation.
• maxJitteredBackoff is extracted from reconcileStepTimeout (value-identical
  there). Jitter is applied after the backoff base is capped at
  Retry.Maximum, so a policy-compliant wait can reach
  Maximum × (1 + JitterRatio); that bound now has one implementation instead
  of a second copy in doctor.go.

observedFreshnessLimit deliberately still sizes its waits from bare
Retry.Maximum. That gap predates this branch, and widening the
heartbeat-staleness window is a behavior change outside this fix — the extracted
helper makes closing it a one-line change whenever it is taken on deliberately.

The hard fault is unchanged. A wedged listener never acknowledges, so its lag
grows monotonically past any bounded window; widening delays that verdict, it
does not remove it. That is pinned by a dedicated test rather than left to
inspection. The cost of the wider window is detection latency, which now scales
with the configured retry policy.

On sizing, honestly

The issue asks for the new derivation to be picked "against observed busy-fleet
lag distributions, not a guess". There is no distribution — the audit record
(D4) contains exactly one benign observation, 2m9s, ~1.6x the old window. So:

• The derivation is anchored to the configured retry policy, not to a hand-picked
  number of seconds and not to that single observation. That is what keeps it
  from being a guess: the bound answers "how long can a legitimate convergence
  take, given this config", which is answerable from the config alone.
• It contains the one observation on record with wide margin as a consequence of
  that derivation, not as its calibration target. Nothing here is tuned to 2m9s.
• The observation is pinned as a test case so the real number stays a checked
  property rather than a figure in a report.

Flagging that explicitly rather than implying a distribution was consulted.

Test plan

Full suite green on Windows: go build ./..., go vet ./..., gofmt -l ./internal/ (clean), golangci-lint run --config .golangci.yml ./... (0 issues.), go test ./... -count=1 (all packages ok, 0 failures).

Measured values, not hand-derived: grace 4m30s → 26m10s at jitter 0 and
28m34s at jitter 0.2; observedFreshnessLimit unchanged at 13m10s.

New and changed coverage:

• TestListenerAcknowledgementGraceBudgetsAFullRetryEnvelopePerConvergenceLeg
  pins the derivation exactly, then asserts a widened window for each term
  of the retry policy — maxAttempts, retry.maximum, jitterRatio,
  requestTimeout — as separate subtests. The maxAttempts and jitterRatio
  cases are the regression tests for the specific defect: the previous
  derivation ignored both.
• TestDoctorAllowsOnlyBoundedListenerAcknowledgementTransition keeps the
  benign-busy-fleet-lag case at exactly the observed 2m9s, and its
  grace-boundary cases (at-grace, past-grace) now derive from
  listenerAcknowledgementGrace rather than hardcoding seconds, so a future
  re-derivation cannot leave a boundary case silently asserting the wrong side
  of the window.
• TestListenerAcknowledgementStaysAHardFaultForAWedgedListener drives a
  listener at 10x grace and asserts ExitDegraded plus the [FAIL] marker, so
  widening cannot silently turn the check advisory. It scales with the derivation.
• No test asserts an ordering between the grace and observedFreshnessLimit.
  The two bound unrelated quantities — one pending transition spanning several
  reconciles and several retry envelopes, versus heartbeat staleness over a value
  pollCheckpoint refreshes every reconcile interval — so neither constrains the
  other, and an ordering assertion would only pin a coincidence of the current
  leg count.

Revert-proof: restoring the previous derivation fails the exact-value assertion,
the maxAttempts and jitterRatio subtests, and the at-grace case — so the
new coverage genuinely discriminates the fix rather than passing either way.

Not run locally: the race detector (-race requires cgo, unavailable on this
host). CI's go-quality lane covers it.

Related

Fixes #100.

• ci-runner-alignment audit finding D4 (disposition: widen grace, keep the hard
  fault) — the source of the 2m9s observation and of the constraint that
  sustained beyond-grace lag stays non-advisory.
• docs/observability.md restated the old formula in two places; both are
  updated, since a doc that names a derivation goes stale the moment the
  derivation changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01W5KaGv34aWCL1epZMSiupC

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a2ebcf4ed8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".