test(runtime-host): attribute Windows startup stalls - #3454
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for strengthening the Runtime Host startup diagnostics and keeping the wire/lifecycle behavior unchanged. The exact-head checks are green, but three parts of the new diagnostic/stress contract still lose or overstate the evidence they are meant to provide. I left focused P2 comments inline; none require a parallel startup path.\n\nAI-assisted review disclosure: Codex performed an independent exact-head review and reproduction; I verified the reported paths and live PR state before posting.
| return { endpointConnected: true, registration }; | ||
| } | ||
| if (result.kind !== 'unavailable') { | ||
| return { endpointConnected: false, ...(registration ? { registration } : {}) }; |
There was a problem hiding this comment.
[P2] draining is returned only after a Host accepted the connection and completed a handshake, but this branch records endpointConnected: false. A timeout then reports sawEndpointConnected:false, removing the key distinction this PR is trying to diagnose. Please derive this flag from whether a handshake was reached (including draining, and any equivalent incompatible-handshake result), and add a regression assertion for the resulting diagnostic.
There was a problem hiding this comment.
Fixed in 8a5a275. Election-only unavailable results now carry an internal endpointConnected phase fact written by connectResolvedRuntimeHost instead of deriving it from an ambiguous reason. Registration, read, and connect failures write false; draining, non-blocking incompatible, and post-connect handshake failures write true. finalizeConnectRuntimeHostResult strips this field, so the public result contract is unchanged. The consumer tests cover the phase variants, including registration root mismatch false and accepted wrong-root handshake true.
| "windows:inventory:write": "node scripts/windows-test-inventory.mjs --write", | ||
| "smoke:windows": "npm run build && npm run smoke:windows:dist", | ||
| "smoke:windows:dist": "node scripts/windows-smoke.mjs", | ||
| "stress:windows-runtime-host-startup": "npm --workspace @maka/runtime-host run build && node scripts/stress-windows-runtime-host-startup.mjs", |
There was a problem hiding this comment.
[P2] This new public stress entry cannot run from a clean checkout because building only @maka/runtime-host leaves referenced workspace dist artifacts absent and tsc fails before the runner starts. Please make the command self-contained using the existing root/reference build seam (as smoke:windows does), or explicitly split it into a documented :dist command whose prerequisite is enforced by the caller.
There was a problem hiding this comment.
Fixed in 8a5a275. The exploratory Node stress command was removed from the permanent PR surface, including its scripts, package entry, and CI wiring. The packaged boundary remains Release Windows check, which owns the real Electron path.
| { | ||
| launchCandidate(input) { | ||
| if (launch) return launch; | ||
| launch = launchOwnedRuntimeHostCandidate({ ...input, idleGraceMs: 0 }); |
There was a problem hiding this comment.
[P2] This stress path launches the candidate with the default process.execPath; under npm run, that is Node, not the packaged Electron executable implicated by the Windows startup stall. The result therefore demonstrates a Node fresh-root launch, not the advertised packaged path. Please either pass the packaged executable and run this in the Windows/package lane, or name/document it as Node-only and add a separate packaged smoke boundary. That keeps the evidence claim aligned with what the harness actually exercises.
There was a problem hiding this comment.
Fixed in 8a5a275. This PR no longer ships or claims a Node stress harness. It retains bounded production startup diagnostics only. Packaged Electron evidence is provided by Release Windows check, which passed on the exact head.
b2315f2 to
8ab1ce2
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for this — and worth saying up front, because the CI state makes it look worse than it is: the failing package job is this PR working, not this PR breaking.
Verify automatic update end to end failed because the packaged app genuinely stalled during Runtime Host startup. Before this PR that failure would have read Runtime Host did not become ready and nothing else. Instead it produced:
{"deadlineMs":45000,"elapsedMs":45174,"candidateLaunches":69,
"sawEndpointConnected":true,
"observations":{"notRegistered":73,"connectFailed":0,"handshakeFailed":0,
"connected":1,"readyWaitFailed":1,"deadlineElapsed":0},
"lastRegistration":{"pid":8268,"state":"recovering","lifecycleMode":"ephemeral"}}That is exactly the evidence the PR set out to capture: 69 candidate launches in 45s, zero connect/handshake failures, one successful connection followed by a ready-wait failure, and a timeout that missed by 174ms. It says the problem is retry convergence, not a deadline that's slightly too short — which is a materially different repair than the one you'd guess from the old error message.
Four questions: the problem is that startup stalls were unattributable; the fix instruments the existing election loop with counters and a candidate/registration snapshot and appends a bounded JSON diagnostic on failure. Right layer — evidence before repair, and the instrumentation is pure observation with no behavioural branch. Counters reuse the existing connection/election authority rather than standing up a second view of startup state.
We checked the counters rather than trusting them, since the conclusions above depend on them: candidateLaunches de-duplicates via a Set (so coalesced launches count once), and notRegistered counts poll observations rather than launches — so 73 > 69 is definitional, not double-counting. Two reviewers verified this independently.
On the CI gate: we can't approve while package is red — approval means "safe to merge," and merging red turns main red, regardless of whose fault it is. This isn't a mark against the PR. Once the underlying stall is fixed (or the job is green on this head for another reason), the review conclusion is otherwise clean: the three earlier P2s are all addressed at this head.
One finding inline.
AI-assisted review. Two reviewers worked this independently and converged on the same finding and the same counter verification.
| ...(registration ? { registration } : {}), | ||
| }; | ||
| } | ||
| switch (result.reason) { |
There was a problem hiding this comment.
[P3] Four unavailable reasons fall through the switch without landing in any bucket, and there's no other or total to reveal it.
recordElectionResult counts not_registered, connect_failed, and handshake_failed. RuntimeHostUnavailableReason also includes invalid_registration, root_mismatch, composition_mismatch, and epoch_mismatch — those keep the election going but increment nothing. The early return above for result.kind !== 'unavailable' is silent in the same way.
Why it matters more than a missing counter usually would: this diagnostic exists to tell someone where to look, and the failure mode is that it points somewhere wrong. If a run stalls mainly on epoch_mismatch, the JSON reports notRegistered: 0, connectFailed: 0, handshakeFailed: 0 — and the person reading it concludes the connection layer is healthy and goes hunting elsewhere. A diagnostic that quietly omits the actual cause is worse than no diagnostic, because it carries authority.
The loop above already shows how easy the fix is: add the missing cases, or an other bucket plus a totalAttempts field so the buckets can be reconciled against it. The total is the part worth having — it makes any future omission visible instead of silent, including reasons added after this PR.
There was a problem hiding this comment.
Fixed in 8a5a275. The diagnostic now records totalResults and otherResults. For every election result, totalResults equals notRegistered plus connectFailed plus handshakeFailed plus connected plus deadlineElapsed plus otherResults. readyWaitFailed remains a separate post-connection phase counter. The new test covers draining plus an unclassified unavailable reason and asserts the reconciliation.
Generated-by: Codex
Generated-by: Codex
8ab1ce2 to
8a5a275
Compare
|
Final status for the current exact head 8a5a275: #3512 is merged and owns Candidate single-flight behavior; this PR is now diagnostic-only. The exploratory Node stress harness was removed, the existing operator deadline guidance was preserved, endpoint phase evidence is internal and bounded, and totalResults/otherResults close the remaining counter-completeness finding. Core CI and Release Windows check both pass on this exact head, including packaged automatic update. The PR is ready for normal human review; no behavior or timeout change is being proposed here. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving 8a5a275926f7abef0109b496acd84dc86cae535a. Required test is completed / success bound to that exact SHA, alongside package. No P0–P2.
Re-review at the current head; the earlier findings were re-derived rather than carried forward.
All three earlier findings are closed, and two of them by deletion rather than repair. The endpoint phase attribution [P2] is fixed. The two [P2]s about the Windows stress harness — that it could not run from a clean checkout, and that it launched process.execPath (Node) rather than the packaged Electron binary implicated in the stall — are closed by removing the harness and its entry point. That is the right resolution: those findings said the harness could not produce the evidence it claimed to produce, and a harness that cannot support its own conclusion is worth less than no harness. The unknown-unavailable reason [P3] is closed by a default bucket.
The branch was rebased, and the review handled that correctly: git range-diff was used to separate the real increment from main-side ASF-header and error-copy noise. The genuine change is confined to connect-or-spawn.ts, with no new P0–P2.
Three [P3]s, none blocking:
readyWaitFailedis a sub-count ofconnected, but that relationship exists only in the discussion — nothing in the code or the tests states it.- Nothing asserts
readyWaitFailed <= connected, which is the cheap invariant that would keep the previous point from drifting. - The title is still
test(runtime-host), but the final content ships production diagnostics (connect-or-spawn.ts,connection.ts,startup-error.ts) and the stress harness is gone. Since the repository squash-merges, this title becomes the commit message onmain— worth a rename before merge.
One procedural disclosure, made by the reviewing line rather than discovered afterwards: the reviewer's task claim omitted --reviewer-isolation. The technical brief arrived after the claim and the public task carried no technical judgement, and the first-stage judgement was sealed before any GitHub review thread was read — so the evidence stands. It is recorded here rather than quietly dropped, because a disclosed imperfection is worth more than an undisclosed clean claim.
Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.
Summary
Preserves bounded structured evidence for Runtime Host election timeout failures.
The diagnostic records the observed startup cut with the exact Candidate PID/attempt identity and exit state, final election elapsed time, endpoint-connected observation, reconcilable election result counters, and the last safe registration summary. Desktop and CLI startup errors include the same bounded JSON.
This PR is diagnostic-only. The Candidate single-flight behavior fix from #3512 is already merged into
main; this PR does not change that behavior, the 45-second election deadline, retry/respawn policy, candidate backoff, endpoint behavior, public wire protocol, failure classification, or cleanup ownership.Refs #3279.
Diagnostic contract
false; draining, incompatible, and post-connect handshake failures recordtrue. The internal phase field is stripped from the existing public connection result.totalResultsis reconciled bynotRegistered + connectFailed + handshakeFailed + connected + deadlineElapsed + otherResults.readyWaitFailedis a separate post-connection phase counter and is intentionally outside that sum.Release Windows checklane.Scope decisions
The merged #3512 behavior fix owns per-election Candidate single-flight. This PR retains only its diagnostic observation and regression evidence; it does not reimplement or broaden the behavior fix. Cross-client coalescing, full LaunchLease architecture, detailed phase timestamps, and retry/deadline policy changes remain deferred to #2142 or #3279 follow-up work.
Verification
Exact head
8a5a275926f7abef0109b496acd84dc86cae535a, rebased ontomainf19eede03a829c226be39f001b2684860a3aa1d5:npm run lint: 2,583 files, 0 errors.npm run format:check: 1,591 files, 0 changes.git diff --checkpassed.Local limitations:
read_eoftiming noise; the new root-mismatch writer tests pass 2/2.L3 gate
Fresh exact-head L3 evidence is green. The remaining gate is normal human review.
AI use
Select exactly one:
Tool(s) and scope: Codex implemented the bounded diagnostic, addressed review findings, rebased it after the #3512 behavior fix, removed the exploratory Node stress harness, ran local validation, and used independent author-gate reviews. The commits include
Generated-by: Codextrailers.Checklist
Does this PR entail a change in behavior?