Conversation
…lure A candidate that loses the launch election proves another candidate owns the root and is still coming up. The client reported that loss as the Host's own failure instead, and stopped probing. With a transcript store large enough to keep the winner out of registration past the moment it is asked about, the desktop saw a failure about a second after it asked, and every retry saw the same thing - the permanent unavailability in apache#5843. A lost election is now evidence to keep waiting on: the election runs its window and keeps reconnecting to the candidate that won. The launch throttle is left alone, so a root whose winner dies is still rescued by the next attempt rather than stranded, and the loser's startup diagnostic stays on disk because that trace is what made the storm visible in the first place. Generated-by: Qoder (AI assistant)
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 807a9b4da79d71e1c56c23e308771a903c7d818b (2 files, +142/−7, a single commit).
P2 — the head's test job is red because this PR's own new test fails against this PR's own code.
Run 36977438495, @maka/runtime-host test:dist, 1 failure of 2218:
✖ a candidate that loses the launch election is waited on, not replaced (310ms)
test at dist/__tests__/managed-deployment.test.js:299:1
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected
+ 'launch_election_lost'
- 'startup_timeout'
at …/dist/__tests__/managed-deployment.test.js:328:12
That test comes from the managed-deployment.test.ts this PR adds (+122). So the change and its test disagree about the outcome of exactly the scenario the PR exists to fix: the test expects the waiting path to end as startup_timeout, while the code leaves launch_election_lost as the terminal reason. Either the waiting behaviour does not take effect on that path, or the test asserts a contract the implementation does not hold — but as it stands the PR cannot land, because the gate it added fails on the revision that adds it.
What the change does, and why its intent is sound. connect-or-spawn.ts:459-484 no longer returns { kind: 'failed', reason: … } for a candidate whose permanent startup failure is launch_election_lost. Losing the election is evidence that another candidate owns the root and is still coming up, so the code clears startupFailure and lets the wait continue, keeping the loser's on-disk diagnostic (deliberately non-terminal, per the comment). The branch is a proper if/else: every other permanent-failure reason still selects the diagnostic and returns failed, so genuine startup failures are not swallowed — which is the thing I checked first, and it holds.
Gate on this head: test is red (the failure above); package and label are green; mergeable is true and the state is blocked.
What I could not judge
- I did not run the suite myself; the above is read from the failing run's output plus the commit's diff.
- I did not reproduce the scenario beyond that: whether the correct resolution is to change the loop or to correct the test is a decision for the author, and I have not formed a view on which reading the maintainers intend.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
… too CI on 807a9b4 failed exactly one test, and it was the new one: "a candidate that loses the launch election is waited on, not replaced". The in-loop guard is not the only exit — once the window is spent, the end-of-election path returns whatever startup failure was recorded without asking what it was. On Linux the loser's report resolves after the deadline, so the in-loop guard never sees it and the terminal path still reported launch_election_lost. Both exits now go through one predicate, and the test asserts only the property that must hold on either: a lost election never decides the outcome. Three local repeats exercised just the in-loop exit; this case is CI's, not mine. Generated-by: Qoder (AI assistant)
|
Thanks — that review is against |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 49fb76691ce4b7567be6024154e635276a21638e.
The P2 from my previous pass is resolved, and it was resolved the right way round. No new findings.
I reported that this PR's own new test failed against its own code: the test expected the waiting path to end as startup_timeout, while the implementation left launch_election_lost as the terminal reason. The test's expectation is unchanged at this head (managed-deployment.test.ts:591 still asserts startup_timeout), so the implementation was corrected rather than the assertion relaxed — which is the stronger resolution.
The correction is complete rather than partial. The earlier fix only covered the branch that breaks on a permanent failure; this head also covers the second place the loss could decide the outcome, where the startup window simply runs out:
if (startupFailure && !isLaunchElectionLoss(startupFailure)) {and the new named predicate carries the principle in its own comment — a lost election is evidence about another candidate that holds the root and is still coming up, so it "must never decide how this election ends, whether the loop breaks on it or the window simply runs out", with the issue reference attached. That is the same reading I took when I said the intent was sound and that the test and the loop disagreed about where the wait ends; the loop is now the side that moved.
Genuine failures remain surfaced: the non-election reasons still select the candidate diagnostic and return failed, which the earlier branch already guaranteed and this change does not disturb.
Gate on this head: test is green (and package), which is exactly the check that was failing; mergeable is true and the state is blocked.
What I could not judge
- I did not run the suite myself; the above is read from the failing-then-passing check on this head plus the commit's diff.
- No live Runtime Host, so the slow-first-startup scenario the fix targets is not reproduced end to end.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
…ndidate The Desktop gave up on a starting Host at 45s while the client elects for 75s (connect-or-spawn.ts DEFAULT_ELECTION_DEADLINE_MS, raisable through MAKA_RUNTIME_HOST_ELECTION_DEADLINE_MS). With a 45s manager-side window under a 75s election window the manager can declare a candidate dead while the election it lost is still inside its legitimate window, which is the replace-and- reshuffle amplifier in apache#5843: a large transcript store keeps the winner out of registration past 45s, the manager swaps, and every replacement loses again. The 45_000 arrived with the parity sweep apache#2216 as a plain fallback carrying no alignment argument. Rather than add a second magic number, the Desktop fallback now resolves through the channel the client already honours - caller value, then MAKA_RUNTIME_HOST_ELECTION_DEADLINE_MS, then the client's own default, which is exported for that purpose - so both windows are derived from one constant and cannot drift apart again. Verified on this host (Windows, scoped suites, no Electron runtime launched): - tsc -b packages/runtime-host and tsc -p apps/desktop/tsconfig.main.json: rc=0, zero errors. - dist/main/__tests__/runtime-host-desktop-candidate.test.js: 27 pass / 0 fail. Baseline measured before the change was 26 pass / 0 fail. - Counterfactual: putting `electionDeadlineMs ?? 45_000` back in the resolver body makes the new test fail with `45000 !== 75000` at 1 fail / 26 pass, so it discriminates on the value, not on a missing symbol. - packages/runtime-host scoped set (connect-or-spawn-env, managed-deployment, wait-for-ready, startup-error, host-profile, candidate-startup-failure): 60 pass / 0 fail. - biome check on all four changed files: exit 0, no diagnostics. - Hook payloads run by hand, because spawnSync on node_modules\.bin\biome.cmd dies with EINVAL here (errno -4071): asf-license-headers check-staged rc=0, protocol-epoch-check --staged rc=0 at epoch 202, git diff --cached --check rc=0. biome-staged-check.mjs crashes on that spawn rather than reporting a finding; its substance is the biome check above. Deliberately unchanged, and measured rather than assumed: - host-profile.ts:595 and wsl-environment.ts:57-58 keep their own 45s defaults. connectRuntimeHostProfile has exactly one production caller, the Desktop candidate at :501, and it now always supplies readyTimeoutMs, so neither fallback is on the Desktop path any more. Widening a shared library default for callers that do not exist today is a bigger call than this issue. - candidate-startup-failure.ts:27 still classifies launch_election_lost as permanent, per the reasoning on apache#5843. - startup-diagnostic.test.js fails `54 !== 0` here both with and without this change, proven by rebuilding from HEAD's copies of the two client files, so it is a pre-existing host failure and not a regression. The full runtime-host suite was not run and no full-suite number should be read from this commit. Generated-by: Qoder (AI assistant)
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 49fb7669 (my last reviewed head, clean after the P2 fix) → 000b70e1 (one new commit, 000b70e1, "wait the client's election window before replacing a candidate"). The merge-base with main is unchanged (57964425), so the delta is exactly 49fb7669..000b70e1. GitHub reports MERGEABLE (merge state BLOCKED). CI test and package both pass on this head.
No P0-P2. One P3 inline.
What the commit does: the Desktop profile candidate's readyTimeoutMs: input.electionDeadlineMs ?? 45_000 becomes resolveDesktopCandidateReadyTimeoutMs(...), which falls back to the client's MAKA_RUNTIME_HOST_ELECTION_DEADLINE_MS env channel and then to the newly exported DEFAULT_ELECTION_DEADLINE_MS (75s). Three symbols are newly exported from @maka/runtime-host/client.
What I checked:
- Correctness of the helper: precedence is caller value, then env, then default.
electionDeadlineMsFromEnvironmentuses the samedurationMsFromEnvironment(..., 1)parser as connect-or-spawn, so invalid env values behave the same way on both sides. The test covers default, env override, and caller value. - Exports: the additions are purely additive and the new
exporton the constant has no other side effects. - Earlier fix intact: the
isLaunchElectionLosshandling that resolved my previous P2 is not touched by this commit. - Scope (the P3): the changed line sits only on the remote/WSL profile path. The local election that #5843 targets already used 75s through
connectOrSpawnRuntimeHost. See the inline comment.
What I could not judge: I did not run the suite, and I had no live remote or WSL Host to measure how the slower failure surfacing feels in practice.
| ? {} | ||
| : { handshakeTimeoutMs: input.handshakeTimeoutMs }), | ||
| readyTimeoutMs: input.electionDeadlineMs ?? 45_000, | ||
| readyTimeoutMs: resolveDesktopCandidateReadyTimeoutMs(input.electionDeadlineMs), |
There was a problem hiding this comment.
P3: the new value goes to the remote/WSL profile path, not the local election that #5843 is about. startProfileDesktopRuntimeHostCandidate is only reached when input.profileTarget is set (:383). It calls connectRuntimeHostProfile, which serves remote SSH/WebSocket profiles (host-profile.ts:592-596) and WSL environment profiles (wsl-environment.ts:135-139). In both, readyTimeoutMs is the post-handshake waitForRuntimeHostReady on a Host that is already connected. It is not a launch-election window. The local candidate path already passes electionDeadlineMs to connectOrSpawnRuntimeHost (:1046), which resolves caller value → MAKA_RUNTIME_HOST_ELECTION_DEADLINE_MS → 75s by itself (connect-or-spawn.ts:345-348). So the change does not touch the replace-and-reshuffle loop described in the commit message. What it does change is narrower and unstated: (a) remote/WSL Hosts that connect but never become ready now take 75s instead of 45s to fail, and (b) an env var named ELECTION_DEADLINE now also sets the remote/WSL readiness wait. For WSL, the in-distro election (if any) runs before the handshake, and that is still capped by the 45s DEFAULT_RUNTIME_HOST_WSL_STARTUP_TIMEOUT_MS handshake default unless the caller supplies handshakeTimeoutMs. Suggest either dropping this commit from the PR, or restating its rationale as "align remote/WSL readiness with the election window" with an explicit argument for that coupling. The helper and test are fine as code; the issue is what they are justified by.
Summary
A lost launch election is evidence about another candidate — it won, it holds the root, and it is still coming up. The client was reporting that loss as this Host's own failure and stopping its probes, so with a transcript store big enough to keep the winner out of registration for longer than the caller waits, the desktop saw a failure about a second after it asked and every retry saw the same thing. That is the permanent-unavailability half of #5843.
A lost election is now non-terminal on both exits of the election: the in-loop check and the end-of-window path that returns whatever failure was recorded.
The second half landed in
000b70e18, after the exchange on #5843: the Desktop waited 45 s for a Host while the client elects for 75 s, so the manager could declare a candidate dead while the election it lost was still inside its legitimate window — the replace-and-reshuffle amplifier. That fallback now resolves through the channel the client already honours, off one exported constant, so the two windows cannot drift apart again.Deliberately not changed: the launch throttle. I first added a flag that suppressed further launches for the rest of the election, then removed it — the client cannot tell "winner still initializing" from "winner died" without reading the launch lease, and suppressing on that ambiguity would strand a root whose winner died mid-election instead of letting the next attempt rescue it. Bounding the storm itself needs that lease observer, which lives in
@maka/storage; better as its own change.launch_election_lostalso stays classified permanent incandidate-startup-failure.ts:27: per candidate the loser exits so the manager can spawn a fresh attempt, and the storm came from the window mismatch, not the classification.Root cause
connectOrSpawnRuntimeHostWithDependencieshad two places that end the election on a recorded startup failure: the in-loop permanent-failure check, and theif (startupFailure)return after the window is spent. Both classifiedlaunch_election_lost— reported by the loser itself since #5845 — as terminal, so the first iteration that observed one returned{ kind: 'failed', reason: 'launch_election_lost' }.Window alignment (
000b70e18)runtime-host-desktop-candidate.tspassedreadyTimeoutMs: input.electionDeadlineMs ?? 45_000intoconnectRuntimeHostProfile, whileconnect-or-spawn.tsresolved the election window as caller value, thenMAKA_RUNTIME_HOST_ELECTION_DEADLINE_MS, thenDEFAULT_ELECTION_DEADLINE_MS(75 s, with a comment explaining why: the Windows named-pipe ACL helper has a 60 s fail-closed ceiling). The45_000arrived with the parity sweep #2216 as a plain fallback carrying no alignment argument.So instead of a second magic number, the Desktop fallback calls
resolveDesktopCandidateReadyTimeoutMs, which runs the same three steps, andDEFAULT_ELECTION_DEADLINE_MSis exported from the client barrel to make that reachable.Left alone, and measured rather than assumed:
host-profile.ts:595andwsl-environment.ts:57-58keep their own 45 s defaults.connectRuntimeHostProfilehas exactly one production caller — this Desktop candidate, at:501— and it now always suppliesreadyTimeoutMs, so neither fallback is on the Desktop path. Widening a shared library default for callers that do not exist today is a bigger call than this issue.Verification
On
49fb76691, base57964425a:807a9b4da) rantests 2218 / fail 1and the single failure was my own new test —AssertionError: Expected values to be strictly equalat 310 ms,actual: 'launch_election_lost'. Three local repeats had passed, because they only exercised the in-loop exit; on Linux the loser's report resolves after the deadline, so the end-of-window exit was the one that fired. That CI run is also the counterfactual for the second exit: without the guard it reports the loss, with it the election ends on its own window.node --test dist/__tests__/managed-deployment.test.js→ 10 pass / 0 fail, three consecutive repeats, including a new test that a candidate dying without reporting a loss still gets replaced, so a root with no owner is not stranded. The loser test now asserts only the timing-independent property (a loss must not decide the outcome), since which exit it reaches depends on when the diagnostic resolves.actual: 'launch_election_lost', expected: 'startup_timeout'.owned-candidate,execution-host-queue,peer-native,connection-session): 50 pass / 2 fail on this branch and 50 pass / 2 fail on a pristineorigin/mainworktree, same two names (shares one endpoint…,rejects an incomplete endpoint API…) — both need native/bundle artifacts this environment lacks.tscclean for@maka/core,storage,mcp,runtime,runtime-host.On
000b70e18— Windows, scoped suites, and no Electron runtime was launched:tsc -b packages/runtime-hostandtsc -p apps/desktop/tsconfig.main.json: rc=0, zero errors each.apps/desktopdist/main/__tests__/runtime-host-desktop-candidate.test.js: 27 pass / 0 fail. The same suite measured on this tree immediately before the change was 26 pass / 0 fail, so the new case is the only addition and no existing case moved.electionDeadlineMs ?? 45_000into the resolver body fails the new test with45000 !== 75000at 1 fail / 26 pass. It discriminates on the value, not on a missing symbol.packages/runtime-hostscoped set, counted individually:connect-or-spawn-env5/5,managed-deployment10/10,wait-for-ready1/1,startup-error6/6,host-profile32/32,candidate-startup-failure6/6 — 60 pass / 0 fail.startup-diagnostic.test.jsfails on this host with54 !== 0both with and without this change: measured by rebuildingpackages/runtime-hostfrom HEAD's copies of the two client files and re-running. Pre-existing host failure, not a regression.biome checkon all four files in this commit: exit 0, no diagnostics.asf-license-headers.mjs check-stagedrc=0,protocol-epoch-check.mjs --stagedrc=0 at epoch 202,git diff --cached --checkrc=0.biome-staged-check.mjsitself dies withspawnSyncEINVAL (errno -4071) on the.cmdshim innode_modules\.bin— an environment fault, not a finding; the directbiome checkabove is its substance.Not run, and why:
npm run build:testandscripts/run-workspace-tests-parallel.mjs, the sanctioned full run. I builtpackages/uiandpackages/computer-usein addition, because the Desktop typecheck needs theirdist, but the full 215-fileruntime-hostsuite is not green on this host — Windows SQLite teardown EBUSY, plus a sibling-plugin bundle--ignore-scriptsnever produced. No full-suite number here should be read as a regression signal. Desktop main-process tests were run, specifically: the 27 above, and no otherapps/desktopsuite.Also disclosing: husky's pre-commit hook cannot run on this machine at all — it dies in
spawnSyncon thebiome.cmdshim innode_modules\.bin(Node 24 refuses.cmdwithout a shell), an environment fault, not a finding. I ran the hook's own payloads instead, all four listed above, and committed withcore.hooksPathpointed at an empty directory for those commands. No--no-verify.Editing note, so this is not a silent revision: the description was updated with
000b70e18because two claims in it had become false — that main-process tests were never run, and that there was no public API change. The exports listed below are additive; nothing was renamed or removed.AI use
Select exactly one:
Tool(s) and scope: Qoder (AI assistant) authored the diff, the tests, the counterfactual runs and this description; the contributor of record directs the work and owns its accuracy, provenance, licensing and the merge decision. The tip commit carries
Generated-by: Qoder (AI assistant), which is what a squash merge retains.Checklist
45000 !== 75000)managed-deployment.test.js10/10 andruntime-host-desktop-candidate.test.js26/26, both including the cases that were there before this PR)@maka/runtime-host/client(DEFAULT_ELECTION_DEADLINE_MS,ELECTION_DEADLINE_MS_ENV_VAR,electionDeadlineMsFromEnvironment) and of@maka/desktopmain (resolveDesktopCandidateReadyTimeoutMs). Nothing existing was renamed or removed.