fix(desktop): bound Runtime Host handler waits - #3795
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no blocking issues.
The reconnect handler wait now correctly times out after 5s (configurable) instead of hanging indefinitely when the Host is unavailable, with retry handling. No new authority or state.
Minor note: timeoutMs=0 edge (remaining 0ms) lacks a direct test, but the <=0 branch exists.
No P0-P2. Checks on 32fbe34c pending — code GO.
简体中文
该头未发现阻断。32fbe34 to
5581c57
Compare
|
Rebased onto current Local verification on
Please re-review the refreshed head. |
5581c57 to
070ff68
Compare
070ff68 to
21f3afa
Compare
21f3afa to
f668156
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The hang is real and the fix is at the right seam. I traced the symptom chain to confirm it actually closes: #waitForHandler with no timeoutMs registers no timer, so an invocation against an active epoch whose handler was removed only settles when a replacement registers. Downstream, useTaskSubmissionReadiness already wraps checkNow in try/catch and sendOwningItsTarget releases newTaskSendPending in a finally — so a rejection does release the composer guard, and an unsettled promise was the only thing keeping it latched. Bounding at the IPC ownership boundary rather than giving the composer its own timer is the right call: one authority for the wait.
Approving. One question and three notes inline, none blocking.
AI use: Claude Code assisted with source investigation; the analysis and conclusions are my own.
简体中文
卡死是真实的,修复位置也对。我顺着症状链确认它确实闭合:#waitForHandler 不传 timeoutMs 时不会注册定时器,所以针对一个 handler 已被摘除但 epoch 仍 active 的调用,只有等替代 handler 注册才会 settle。下游 useTaskSubmissionReadiness 的 checkNow 本来就有 try/catch,sendOwningItsTarget 也在 finally 里释放 newTaskSendPending——所以 rejection 确实能放开 composer 守卫,而「promise 不 settle」是唯一把它锁死的原因。把界限设在 IPC 所有权边界、而不是给 composer 自己加定时器,是对的:等待只有一个权威。
Approve。行内一个问题、三条说明,都不阻塞。
| type ReconciliationUnavailableIpcHandler = ReconcileIpcHandler; | ||
|
|
||
| const DEFAULT_RECONCILIATION_WAIT_TIMEOUT_MS = 15_000; | ||
| const DEFAULT_HANDLER_WAIT_TIMEOUT_MS = 5_000; |
There was a problem hiding this comment.
Where does 5s come from, and why is it a third of the reconciliation window? These bound the same underlying event — a Host candidate that is restarting — and this one is the harsher of the two: reconciliation expiry degrades to unavailable(), while this one rejects the call outright. Past whatever the real restart time is, this turns a slow success into a visible failure, which is a different regression from the one being fixed.
I have not measured Host restart time, so this is a question rather than a finding: if 5s came from a measurement, please put the number in the comment; if it was chosen to be "clearly shorter than reconciliation", I would rather see it match the 15s already in the file until there is a reason to differ.
| epoch: string, | ||
| previous?: BoundHandler, | ||
| timeoutMs?: number, | ||
| timeoutError: () => Error = () => new ReconciliationWaitExpiredError(), |
There was a problem hiding this comment.
[P3] This default is the opposite of what a caller that forgets the argument should get. The two errors are not interchangeable: :282 swallows ReconciliationWaitExpiredError and returns undefined, which routes the invocation to unavailable(); RuntimeHostHandlerUnavailableError propagates to the renderer. So a future caller that omits the fifth argument does not get a noisy wrong error — it silently takes the graceful reconciliation path for a wait that had nothing to do with reconciliation.
Every current call site is correct, which is why this is P3 rather than higher. But #waitForHandler is no longer reconciliation-specific, so it should not default to the reconciliation error. Make the parameter required, or pair the deadline with its error so the two cannot be set apart:
#waitForHandler(slot, epoch, previous?, deadline?: { ms: number; error: () => Error })That also removes the undefined positional at :256 that exists only to reach past previous.
| slot, | ||
| epoch, | ||
| undefined, | ||
| this.#handlerWaitTimeoutMs, |
There was a problem hiding this comment.
[P3] The bound is per wait, not per invocation. waitForReplacement is called from inside the retry loop, and each call starts a fresh window, so a candidate that flaps faster than the timeout keeps one invocation alive indefinitely. Each cycle does make progress, so this is not the reported hang and I would not hold the PR for it — but the title's promise ("bound handler waits") is the accurate one, and the composer's unsettled-promise symptom still has this residual path. An invocation-scoped deadline would close it.
| RuntimeHostHandlerUnavailableError, | ||
| ); | ||
| router.close(); | ||
| }); |
There was a problem hiding this comment.
"Preserve same-epoch reconnect routing when a replacement arrives within the window" is the property this change must not break, and it is the one bullet without a new test. The existing replacement test now runs against the 5s default, so it passes without ever approaching the boundary. A case where the replacement registers just inside a small configured window would lock the behaviour rather than rely on the default being generous. Not a finding, just the coverage I would want.
|
#3917 has now merged. It preserves this PR's bounded handler-wait behavior while consolidating initial handler absence, replacement retries, and reconciliation into one invocation-wide monotonic deadline. The merged implementation also removes the upstream reconnect loop and fixes quiescence across reconnect gaps. The authorship from this PR is preserved in #3917's commit trailers. No further action is needed here because this PR is already merged. 简体中文#3917 已经合并。它保留了本 PR 对 handler 等待时间的限制,同时把初始 handler 缺席、替换重试与 reconciliation 收敛到一个覆盖整次调用的单调 deadline。合并后的实现还删除了上游重连循环,并修复了重连空档期间的 quiescence。 本 PR 的作者署名已保留在 #3917 的提交 trailer 中。由于本 PR 已经合并,这里不需要继续操作。 Posted by an automated review agent operated by @M4n5ter. This is not an 简体中文本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md |
Problem
When a Runtime Host candidate exits, its channel handlers are removed while the target epoch remains active so a replacement candidate can reconnect. Calls arriving in that interval waited indefinitely for a handler.
The composer performs a reconnectable task-readiness read before sending. If the Host never returned, that promise never settled, so the composer kept its submission guard latched and both the Send button and Enter stopped working.
Root cause
RuntimeHostReconnectingIpcMain.#waitForHandler()supported a timeout only for reconciled controls. Initial dispatches and ordinary reconnectable-read replacement waits called it without a deadline.Fix
RuntimeHostHandlerUnavailableErrorafter the deadline so callers release pending interaction state and retain retryable input.Tests
npm --workspace @maka/desktop run build:mainnode --test apps/desktop/dist/main/__tests__/runtime-host-reconnecting-ipc-main.test.js(15/15, repeated 10 times)npx biome check apps/desktop/src/main/runtime-host-reconnecting-ipc-main.ts apps/desktop/src/main/__tests__/runtime-host-reconnecting-ipc-main.test.tsgoal-dialogDOM-environment failures (getComputedStyle is not defined) and do not touch this path.Repository baseline note
The full workspace dependency build on current
mainis independently blocked by three staleRuntimeHostConnection.queryTurn/stopTurn/startTurncalls inexecution-host-queue.test.tsafter #3784. This PR does not include that unrelated migration.UI evidence
No visual styling or layout changes. The behavior change is covered at the IPC ownership boundary where the indefinite promise originated.