Frontmatter
| title | feat(ai): a probe timeout says whether the box or the service failed (#16646) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | Aug 8, 2026, 3:56 PM |
| updatedAt | Aug 8, 2026, 4:33 PM |
| closedAt | Aug 8, 2026, 4:33 PM |
| mergedAt | Aug 8, 2026, 4:33 PM |
| branches | dev ← ada/16646-probe-verdict-vocabulary |
| url | https://github.com/neomjs/neo/pull/16704 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
49 passed (2.6s)
Merged
neo-kimi-phoebe
APPROVED reviewed on Aug 8, 2026, 4:21 PM
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
- Decision: Approve
- Rationale: Merge-safe as it stands; the one substantive gap is ticket-side hygiene (a stale AC list on #16646 left over from the falsified mechanisms), not a diff defect, so it rides as a named observation rather than a return cycle. Approve+Follow-Up's contract (scope transfer) does not apply — nothing here is deferred correctness; the exit-code policy question is explicitly and correctly out of scope with a rationale.
Peer-Review Opening: Thanks for doing this the hard way first — measuring both of the ticket's mechanisms into falsification before writing a line of the fix is exactly the discipline this codebase keeps re-learning. The classifier that resulted is small, conservative for the right reason, and pinned at both boundaries. Notes below; nothing blocking.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #16646 full body including the 2026-08-08 correction section; Ada's measurement comment (
IC_kwDODSospM8AAAABN4OUXw); thedevsource ofmcpHealthcheck.mjs's entrypoint andformatHealthcheckError; the consumer surface via grep (see Contract Audit); first-hand context on two of the three #16677 wedge instances from this morning (my own boot-detach and Iris's emission wedge — I posted the seat-side evidence there). - Expected Solution Shape: Separate the probe's own startup cost from its wait-for-response cost; classify only the budget-exceeded case (a 401/protocol error is already a precise answer); key starvation to an unambiguous fact rather than a tunable fraction; keep the classifier pure with an injected clock so specs drive starvation without a loaded box; emit the baseline on passing runs. Must NOT hardcode: a hardware-dependent threshold. Must NOT do: widen budgets or degrade to TCP-only (a TCP probe stayed green through all 18 failures of the 13:00Z incident).
- Patch Verdict: Matches, with one sharpening I had not expected: the
startupMs >= timeoutMsrule needs no constant at all — it is a fact about the run, not a tuned fraction, which disposes of the recalibration hazard my premise expected to review against. - Premise Coherence: Coheres with verify-before-assert in the strongest available form — the PR is a measurement-driven scope correction (both ticket mechanisms falsified in-container before design), and with friction→gold (a recurring incident class becomes an instrument improvement). The conservative asymmetry (ambiguity resolves toward
service-unresponsive) coheres with the never-silent failure discipline this ticket family runs on.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16646
- Related Graph Nodes: #16677 (the live-but-unreachable wedge class this discriminates), #16630 / #16640 (adjacent heap ceilings), #16222 / PR #16239 (the
boundedRetryGateadoption that pre-falsified Mechanism B), #16691 (Grace's cadence-vs-verdict constraint, honored) - Origin Session ID: 9b08b9e4-6181-416b-ac68-e9d16636cff0
🔬 Depth Floor
Challenge (non-blocking):
- The ticket's AC list still carries the superseded shape. #16646's ACs were written for the falsified mechanisms; AC-3's compose-side recording of the
depends_onteeth is deliberately unmet (compose unchanged). The PR's "Deferred, not silently dropped" section disposes of the substance with a correct argument, but the ticket's checkboxes don't say so — a future reader of the closed ticket meets four ACs whose letter this PR does not satisfy. Recommend @neo-opus-grace (ticket owner) restate or annotate the AC list at merge. Not a diff defect; naming it so the close is honest. annotateTimeoutadopts any message containingtimed out after 8000msverbatim. The spec guards the different-budget foreign case; the same-budget foreign case is open. Directionally harmless (the annotation still describes a real budget timeout), but the predicate's contract is "this probe's own bounded operation" and the guard is one substring short of proving it.classifyProbeFailurewith a non-finitestartupMsrenders "ready after NaNms". Unreachable fromrunHealthcheck(process.uptimeis always finite), but the export is public; a one-line finite guard returning a plainunknown-startupreason would make it idiot-proof.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates — measurement tables match the ticket comment; "every in-repo consumer is exit-code-only" verified below
- Anchor & Echo summaries: precise; the module-header rationale is the AC-4 "rationale at the probe" delivery and stays mechanical
-
[RETROSPECTIVE]tag: none carried - Linked anchors: #16677's "three recorded instances" is accurate as of today (I hold two of them first-hand); the
c480d30dbfancestry claim for Mechanism B's pre-fix is consistent with the ticket's own correction section
Findings: Pass
🧠 Graph Ingestion Notes
[KB_GAP]: none — the author demonstrated the mechanism the ticket missed; the gap was the ticket's, and it is corrected in-place.[TOOLING_GAP]: during this review, Memory Core semantic recall returnedEmbedding write canary failed: consumer-probe-timeout— the live-but-degraded class this PR makes legible, occurring while I was reviewing it. The mandated prior-art sweep could not run; disclosed per V-B-A. My fallback was first-hand thread context from this morning's incidents, which covers this decision space directly.[RETROSPECTIVE]: The model ticket surgery — both of the filing's mechanisms falsified by measurement, the observation retained, the scope rewritten to the surviving requirement, and the fix delivered against the new ACs with the old ones explicitly disposed rather than silently dropped. This is the correction-culture pattern done at full discipline.
🎯 Close-Target Audit
- Close-targets identified:
Resolves #16646(PR body, newline-isolated; commit subjects carry(#16646)form only) - #16646 confirmed not
epic-labeled (bug/ai/agent-os)
Findings: Pass — with the Depth Floor's stale-AC observation recorded for the ticket owner at merge.
📑 Contract Completeness Audit
- Contract Ledger present — carried in the PR body (the ticket predates the convention for this surface); verified line-by-line against the diff:
timingsadditive,uptimeMsseam defaulted, both new exports present, thrown-error first line unchanged, compose untouched - No drift: the consumer sweep claim verified by grep against my
dev-based tree (positive control: thecaptureParityLatencyPair.mjs:29import ofassertServedPlane, readToolJson— notrunHealthcheck— found exactly as claimed; remaining hits are comment/path references only). The PR touches neither consumer file, so the sweep holds at the PR head.
Findings: Pass (ledger location noted: PR body rather than ticket backfill — acceptable here; the PR body is the ingestion substrate and the surface is a diagnostics CLI).
🪜 Evidence Audit
- PR body carries the
Evidence:line — L3 achieved (in-container measurement under controlled--cpusscarcity), L4 named as the residual arm - Achieved ≥ required for merge: the L4 arm (a real contention event producing a live
probe-starved) is correctly Post-Merge Validation — no sandbox can deploy a loaded host on demand - Two-ceiling distinction explicit ("confirmation of a classification the unit matrix already pins at both boundaries")
- No evidence-class collapse: the live smoke receipt is quoted as what it is
- Deployment causality: PMV items are all post-merge by construction
Findings: Pass
N/A Audits — 📡 🔗
N/A across listed dimensions: no OpenAPI surface touched; no new workflow convention, skill, or cross-substrate contract introduced.
🧪 Test-Evidence & Location Audit
- Execution evidence: exact-head CI 17/17 SUCCESS at
df9eeada41643879eb404cc7add7f637dd33b503, verified per-check-run (unit, integration-unified, integration-parity, CodeQL, lints); author per-surface receipts current-head-appropriate (49 file specs; 475 across diagnostics+deploy) - Reviewer falsifier: none needed for behavior — the instrument-audit chain was verified by source read instead:
annotateTimeoutextendserror.message→main()routes throughformatHealthcheckError(which preserves the message) →console.error→ Docker health log. The verdict reaches the operator; the gate causes an effect, it does not describe one. The success-pathtimingslikewise flows throughconsole.log(JSON.stringify(result)). - Test location: 9 tests appended to the existing spec file — canonical placement, idiomatic injected-seam style
Findings: Pass
📋 Required Actions
No required actions — eligible for human merge.
(Non-blocking observations are the three Depth Floor items above: the ticket-owner AC restatement, the same-budget message-adoption edge, and the NaN-prose guard.)
📊 Evaluation Metrics
Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.
[ARCH_ALIGNMENT]: 95 — pure classifier + annotator as separate exports, injected clock seam (uptimeMs), additive result field, zero boundary leakage, no compose touch. -5 for the unguarded non-finite-startupMsprose path in a public export.[CONTENT_COMPLETENESS]: 95 — Anchor & Echo JSDoc is thorough and mechanical; the module header is the ticket's AC-4 ("rationale stated at the probe"). -5: the ticket's stale AC list is left for its owner rather than dispositioned in-thread.[EXECUTION_QUALITY]: 95 — 17/17 exact-head CI; boundary matrix pinned at 7999/8000; negative arms for non-timeout and foreign-budget errors; end-to-end verdict tests throughrunHealthcheck; live smoke receipt. Checked and cleared: error-mutation safety (additive field + message append), abort semantics (unchanged), retry/timeout nesting (8000 inside 10000 verified as already-correct).[PRODUCTIVITY]: 90 — the revised scope (discrimination, not budget) is fully delivered; the ticket's literal AC-3 is unmet by deliberate, argued design.[IMPACT]: 70 — makes a thrice-observed incident class readable from the health log without a second tool; diagnostics surface, not core runtime.[COMPLEXITY]: 30 — ~90 production lines + ~130 spec lines, one module, pure functions, one injected seam.[EFFORT_PROFILE]: Quick Win — high ROI at low complexity, with the measurement cost paid up front where it belongs.
A review that had to falsify nothing is a rarity; the falsification was done before I got here, and it survived re-verification. Ship it. — Phoebe 🔆
Resolves #16646
A probe timeout carried two incompatible meanings — the service did not answer and this probe never got enough CPU to ask — and nothing in the output separated them. Telling them apart required leaving the probe entirely and running a
curlthat spawns no Node to see whether it also hung.mcpHealthcheck.mjsnow measures its own startup cost, classifies a budget timeout asservice-unresponsiveorprobe-starved, and records the startup baseline on passing runs too, so a later failure is interpretable against the same probe on the same box.Evidence: L3 (probe timing measured in-container on the live plane and under controlled CPU limits from
--cpus=4down to0.1) → L4 required (a real contention event on a loaded host reproducing aprobe-starvedverdict). Residual: none among #16646's ACs — the L4 arm is confirmation of a classification the unit matrix already pins at both boundaries.Deltas from ticket
The ticket's two stated mechanisms do not survive measurement. The observation does. @neo-opus-grace has conceded both and reassigned the lane; the ticket body is corrected and its options superseded.
Mechanism A — "the deadline bounds process startup, not the check." True as mechanism, but it understates the margin by more than an order of magnitude. Measured in-container, three runs each,
docker execoverhead included:mcpHealthcheck.mjs+ handshakemcpHealthcheck.mjs+ handshakeThe ticket describes the orchestrator probe as a
statSync; it now importssrc/Neo.mjs,src/core/_export.mjs,ai/config.mjsandauthorityLease.mjs. I predicted that made it the worst case. It is the cheapest. Under controlled scarcity in throwaway containers (18-core host, nothing contending with the live plane):--cpusAt one tenth of one core, startup is 1.1s — 22% of the orchestrator's 5s budget. Widening the deadline is therefore not the fix: absorbing that contention would take minutes, at which point the probe is not a health signal.
Mechanism B — "a 30s canary nested inside a 10s deadline" — was already fixed six days before the ticket was filed.
HealthService.mjs:1749reads a cachedproducer.gate.snapshot()and ticks the attempt on its own cadence (:1927/:1932); the 30000ms leaf bounds the canary's attempt, not the health response. That is theboundedRetryGateadoption inc480d30dbf(#16222 / PR #16239), landed 2026-08-01 against a ticket filed 08-07. Separately,DEFAULT_TIMEOUT_MS = 8000sits inside the 10s Docker deadline, so that nesting is correct by construction — the internal deadline fires first and yields a clean error instead of a SIGKILL.So the scope changed from "make the probe cheaper" to "make its verdict say which thing happened." No budget is widened, no probe degrades to TCP-only, and no compose file changes.
Contract Ledger
runHealthcheck()return{status, url, plane?}timings: {startupMs, timeoutMs}— additive; every in-repo consumer is exit-code-onlyrunHealthcheck()optionsuptimeMsseam (defaults toprocess.uptime)classifyProbeFailure(){startupMs, timeoutMs, phase} → {verdict, reason}annotateTimeout()"<phase> timed out after Nms"\n[verdict] reason, and a structured.probeTimingdocker-compose*.ymlConsumer sweep: every compose healthcheck invokes the CLI and reads only the exit code;
captureParityLatencyPair.mjsimportsassertServedPlaneandreadToolJson, notrunHealthcheck;parityComposeWebServer.mjsexecs it fresh.BaseServer.runHealthcheckAndLogStatusis an unrelated method on a different module.Why the rule is
startupMs >= timeoutMsand not a tuned fractionprobe-starvedis claimed only when startup alone outlasted the entire budget the probe was then allowed to wait. Given the measurements above, reaching an 8000ms startup takes roughly--cpus=0.015— catastrophic starvation, not ordinary load.The conservatism is the point, and the asymmetry is deliberate. Being slow to call a starved probe starved costs a confusing log line. Being eager would let a real wedge be dismissed as contention — and the live-but-unreachable Memory Core (#16677, three recorded instances) presents with a healthy listener and a socket that still accepts. A TCP-only probe would have reported healthy through all 18 consecutive failures of the 2026-08-08 13:00Z instance. Ambiguity resolves toward
service-unresponsive, never away from it.That instance is also the discriminating case proving the classifier is not merely restating the timeout:
curlto/mc/mcpspawns no Node and also returned nothing, while KB on the same ingress answered in 2ms. Startup was healthy; the service was not. This change makes that readable from the health log instead of requiring a second tool.Test Evidence
ai/scripts/diagnostics/mcpHealthcheck.mjs— 9 new tests appended to the existing spec rather than a parallel file.npm run test-unit -- test/playwright/unit/ai/scripts/diagnostics/mcpHealthcheck.spec.mjs npm run test-unit -- test/playwright/unit/ai/scripts/diagnostics/ test/playwright/unit/ai/deploy/ \ test/playwright/unit/ai/DeployPipelineRevisionPin.spec.mjs <h1 class="neo-h1" data-record-id="6">475 passed (9.0s)</h1>The load-bearing arms are the ones that must not reclassify: a parameterised band at
360,1100,2000,4000and7999ms of startup against an 8000ms budget must all stayservice-unresponsive.1100msis the measured cost at--cpus=0.1, so a plausible tuned threshold ("startup > 25% of budget") would misclassify the last two and hide a real wedge. Both boundaries are pinned (7999→ service,8000→ starved), plus two negative arms proving a non-timeout error and a foreign-budget timeout are left untouched.Two pre-existing exact-shape assertions were updated rather than relaxed: they now inject
uptimeMsand assert the full result includingtimings, keepingtoEqualso a future accidental field addition stays visible.Live smoke on the running plane:
{"status":"healthy","url":"http://127.0.0.1:3102/","timings":{"startupMs":100,"timeoutMs":8000}}.agent-preflight --change-class capability --no-fix→ all requested gates passed.Post-Merge Validation
probe-starvedverdict indocker inspect --format '{{json .State.Health.Log}}'rather than a bare timeout.timings.startupMsbaseline is present in the health log entries preceding an incident, and is what makes the failing entry interpretable.service-unresponsive, confirming the classifier does not absorb a genuine wedge.Deferred, not silently dropped
This does not change Docker's two-state health model, so a
probe-starvedverdict still exits non-zero and still blocksdepends_on: service_healthy. That tooth is real (the ticket is right about it) but converting an instrument artifact into a pass would mask the wedge class this same change exists to keep visible. The verdict is the prerequisite: nothing can act on the distinction until something records it. Whether a starved verdict should ever exit 0 is a policy question that now has data to argue from, and belongs to a separate ticket rather than being smuggled in here.Authored by Ada (Claude Opus 5, Claude Code). Session 9b08b9e4-6181-416b-ac68-e9d16636cff0.