Resolves #17172

🌿 A percentile exists to forgive the slow tail. Over five samples it forgives nothing, and still answers to the name.

assertSustainedHealth now refuses to assert a statistic its sample cannot support. Below twenty observations a nearest-rank p95 is the maximum, so the helper says so instead of returning the max wearing a p95 label.

Evidence: L4 (18 unit arms over the pure index function and the helper's own control flow, red-proved three independent ways) → L4 required (every AC is a local, in-process observable; the integration plane is not needed to decide any of them). Residual: none.

What happened

CI reddened PR #17166 with p95 latency should be <= 500ms / Received: 501. That PR touches ai/services/github-workflow/** and nothing else; the failing spec probes the Dockerized KB/MC plane. The diff and the failure share no surface, and I spent a good part of a turn establishing that before I could look at the actual cause.

Deltas from ticket

I authored #17172 an hour before this PR, so there is no age-decay to report — but three things did move between ticket and implementation, and one of them is a correction to my own filing.

1. The ticket's first version called the formula wrong. It is not. I wrote that the index "does not select a 95th percentile"; ceil(p·n) - 1 is textbook nearest-rank and the original author implemented it faithfully. I corrected the ticket before starting work rather than defend the framing, because the accusation matters to whoever reads it next: the defect is applying a correct formula to a sample too small for the statistic, which is a different and more interesting bug than bad arithmetic.

2. The AC asked for the index table at n = 5/10/19/20/30/60; the spec also pins n = 1. A one-sample window is the degenerate-degenerate case and Math.max(0, …) is the only thing standing between it and index -1. Cheap arm, real boundary.

3. The AC offered "either pass an explicit generous budget with a comment, or let the latency assertion be opt-in". The implementation does both, because they answer different questions: p95Ms: null records that this check has no opinion on latency, and maxMs records the ceiling it does hold. Collapsing them would have made the call site say less than it knows.

One AC deliberately discharged in prose rather than code: "the two HeartbeatPropagation call sites are reviewed under the same lens and either kept deliberately or adjusted — a decision, not an omission." They are correct as written, so the decision lives in this body rather than as two comments explaining why nothing changed. A permanent comment recording a transient review is the bloat that discipline exists to prevent.

The defect

The formula is correct and the original author implemented it faithfully. ceil(p·n) - 1 is textbook nearest-rank. The defect is where it is applied:

ceil(0.95n) - 1 === n - 1 for every n ≤ 19. healthcheck.spec.mjs runs a 5s/1s window — five samples — so the assertion labelled "p95 latency should be ≤ 500ms" actually demanded that every probe be ≤ 500ms. A budget whose entire purpose is tolerating a slow tail was forbidding one, and a single 501ms probe failed the build for the whole team.

Interpolating instead would not have rescued it: a linear p95 of five samples still lands against the largest of them. Five samples do not contain a 95th percentile under any definition — which is the thing the helper has to say, rather than paper over.

Why the call site makes it sharper

The test is named Sustained liveness composability check (Lane B helper) — 5s/1s. Its stated job is proving the helper composes — two probes under Promise.all, both return. It is not a latency benchmark, yet it inherited the full default budget and gated the merge queue on a 500ms tail it never meant to measure.

The shape

• MIN_PERCENTILE_SAMPLES = 20, derived from the 19→20 transition rather than chosen for roundness, and asserted from both sides — that 20 works and that 19 does not. A constant justified from one side drifts.
• p95Ms below that count throws, naming the sample count and both remedies. A guard that refuses without saying what to do instead gets routed around — that is the same lesson the failure text in #17151 encodes.
• maxMs bounds the slowest probe and is valid at any n: the statistic a short window genuinely supports, named for what it is.
• percentileIndex is exported, so the degeneracy is assertable as arithmetic. An implementation that quietly returns n - 1 cannot satisfy the pinned table, and no CI-runner luck can make it pass.
• The 5s/1s call site now passes p95Ms: null, maxMs: <generous liveness ceiling>, with the reason at the call site rather than in the ticket.
• summary.actualP95 is null below the floor, not a number — the refusal covers the reported value, not only the assertion. See the round-2 section.

Deliberately not done

Raising 500 → 600. That buys quiet until the next noisy runner and leaves the label lying. The number is not the defect.

Touching the two HeartbeatPropagation call sites. They run at the n=30 default where the p95 is genuine (index 28, one sample tolerated), so they are correct as written. Reviewed under the same lens and left alone — a decision, recorded here rather than an omission.

One thing I checked rather than assumed, because the refusal could otherwise break those specs in CI: NEO_INTEGRATION_SUSTAINED_WINDOW_MS and its siblings are read only inside this helper and set nowhere in the repo (grep across yml/yaml/json/mjs/env). No environment shortens those windows into the refusal path.

Round 2 — @neo-gpt: the summary republished the defect as data

I fixed the assertion and left the same conflation in the reported value. actualP95 was computed unconditionally, so with p95Ms: null and n=5 the summary returned actualP95 === actualMax. Reproduced before repairing, using one slow probe among fast ones so the two statistics can differ at all:

n= 5   actualP95=40  actualMax=40   CONFLATED
n=20   actualP95=0   actualMax=41   separated

A consumer charting summary.actualP95 over short windows would have been charting the maximum under a p95 label — the original defect, one layer out. Below MIN_PERCENTILE_SAMPLES, actualP95 is now null; null rather than 0 because zero is a plausible latency and a reader cannot tell it from a fast probe.

My own spec arm certified the defect. It asserted actualMax >= actualP95 at n=3, where the two are equal — so it passed on exactly the conflation it was written to catch, which is worse than no arm because it reads as coverage. The replacement uses strict toBeLessThan, the only form that can fail when the guard regresses:

old:  actualMax >= actualP95   ->  true  on (40, 40)   passes on the conflation
new:  actualP95 <  actualMax   ->  false on (40, 40)   fails on it

Consumer sweep before changing the return shape: no integration spec and no module outside test/ reads the summary — all three call sites discard the return value — so the null breaks nothing. @returns now states the nullability rather than leaving it to be found.

Test Evidence

npx playwright test -c test/playwright/playwright.config.unit.mjs \
  test/playwright/unit/test/ --workers=1
→ 41 passed (2.6s)     [20 of them new]

Red-proved three ways — each mutation is a plausible wrong implementation, not a strawman:

The third matters most: it is what stops the constant drifting later to whatever number makes a future red go away.

Arms cover the index table at n = 1/5/10/19/20/30/60, the exhaustive claim that every n below the boundary degenerates to the max, both sides of the boundary itself, the empty sample, the refusal and its message content, the positive control at exactly MIN_PERCENTILE_SAMPLES (without which the refusal could be unconditional and every other arm would still pass), p95Ms: null opting out, maxMs passing and failing in the max's own words, and the summary reporting both statistics.

Precedent

#10918 was this same helper failing this same way: toBeGreaterThan on consecutive process.uptime() samples, broken by a CI runner fast enough to return identical fractional seconds. Fixed in PR #10920 by loosening the comparison.

Same family, same root class — an assertion tuned to a developer's intuition about a real environment and violated by CI's, without the system being unhealthy. One instance is a fix; two is the template, which is why this went to a ticket rather than a re-run.

Honest sizing

This is not a major flake source and the ticket says so. Over the last 60 Tests runs there were 7 failures and all 7 were unit — mine is the only integration-unified red in that window, ~1.6%.

What earns the lane is that the defect is structural rather than statistical, provable with node -e and independent of how often it fires, and that it fails in the most expensive direction: an unrelated author debugging a plane they never touched.

Post-Merge Validation

Nothing is owed. Every AC is a local observable and each is armed above. The integration suite exercises the new call-site configuration on its next run, which is confirmation rather than validation debt.

Evolution

The re-run is what unblocked #17166, and re-running is not a fix — it converts a wrong assertion into a coin flip and teaches the team to read integration reds as noise, which is how a real regression eventually gets waved through. The cost of not filing this is not the 1.6%; it is the next person who assumes their own red is this one.

Related: #17166 · #10896 · #10918 Refs #17171 — same shape, found the same afternoon: an artifact whose label claims more than its mechanism delivers.

Authored by Vega (Claude Opus 5, Claude Code). Session 5cd926fa-77e1-4309-8bbf-ca563ab07403.