Skip to content

fix(desktop): bound Runtime Host handler waits #3795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Astro-Han merged 1 commit into from
Aug 26, 2026
+87 −4
Merged

fix(desktop): bound Runtime Host handler waits #3795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions apps/desktop/src/main/__tests__/runtime-host-reconnecting-ipc-main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
} from "../ipc-reconnect-policy.js";
import * as ipcReconnectPolicy from "../ipc-reconnect-policy.js";
import {
RuntimeHostHandlerUnavailableError,
RuntimeHostReconnectingIpcMain,
RuntimeHostTargetChangedError,
} from "../runtime-host-reconnecting-ipc-main.js";
Expand Down Expand Up @@ -338,6 +339,56 @@ test("holds an invocation across a Runtime Host candidate replacement", async ()
assert.equal(ipc.size, 0);
});

test("bounds invocation while an active Runtime Host has no handler", async () => {
const ipc = ipcHarness();
const router = new RuntimeHostReconnectingIpcMain(ipc, {
handlerWaitTimeoutMs: 5,
});
const target = router.createTarget("target-a");
target.handle("sessions:send", async () => "sent");
router.activate("target-a");
target.removeHandler("sessions:send");

await assert.rejects(
() => ipc.invoke("sessions:send", scope("target-a")),
RuntimeHostHandlerUnavailableError,
);
target.handle("sessions:send", async () => "retried");
assert.equal(
await ipc.invoke("sessions:send", scope("target-a")),
"retried",
);
router.close();
});

test("bounds reconnectable reads when no replacement handler becomes available", async () => {
const ipc = ipcHarness();
const router = new RuntimeHostReconnectingIpcMain(ipc, {
handlerWaitTimeoutMs: 5,
});
const target = router.createTarget("target-a");
const failRead = deferred();
target.handleReconnectableRead?.("taskReadiness:getSnapshot", async () => {
await failRead.promise;
throw new RuntimeHostOperationError(
"session.catalog.query",
"host_draining",
"Runtime Host is draining",
);
});
router.activate("target-a");

const reading = ipc.invoke("taskReadiness:getSnapshot", scope("target-a"));
target.removeHandler("taskReadiness:getSnapshot");
failRead.resolve();

await assert.rejects(
() => reading,
RuntimeHostHandlerUnavailableError,
);
router.close();
});

@Astro-Han Astro-Han Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.


test("does not return a late read from a replaced Runtime Host candidate", async () => {
const ipc = ipcHarness();
const router = new RuntimeHostReconnectingIpcMain(ipc);
Expand Down
40 changes: 36 additions & 4 deletions apps/desktop/src/main/runtime-host-reconnecting-ipc-main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,11 @@ type ReconcileIpcHandler = (
type ReconciliationUnavailableIpcHandler = ReconcileIpcHandler;

const DEFAULT_RECONCILIATION_WAIT_TIMEOUT_MS = 15_000;
const DEFAULT_HANDLER_WAIT_TIMEOUT_MS = 5_000;

@Astro-Han Astro-Han Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


export interface RuntimeHostReconnectingIpcMainOptions {
readonly reconciliationWaitTimeoutMs?: number;
readonly handlerWaitTimeoutMs?: number;
}

class ReconciliationWaitExpiredError extends Error {
Expand Down Expand Up @@ -81,6 +83,13 @@ export class RuntimeHostTargetChangedError extends Error {
}
}

export class RuntimeHostHandlerUnavailableError extends Error {
constructor() {
super("Runtime Host handler remained unavailable after the reconnection window");
this.name = "RuntimeHostHandlerUnavailableError";
}
}

/**
* Keeps Electron IPC registration stable across reconnects while fencing each
* target generation. Reconnectable reads may move to a replacement candidate,
Expand All @@ -91,6 +100,7 @@ export class RuntimeHostReconnectingIpcMain {
readonly #slots = new Map<string, HandlerSlot>();
readonly #activeEpochs = new Set<string>();
readonly #reconciliationWaitTimeoutMs: number;
readonly #handlerWaitTimeoutMs: number;
#closed = false;

constructor(
Expand All @@ -104,6 +114,12 @@ export class RuntimeHostReconnectingIpcMain {
throw new TypeError("Runtime Host reconciliation wait timeout must be positive");
}
this.#reconciliationWaitTimeoutMs = reconciliationWaitTimeoutMs;
const handlerWaitTimeoutMs =
options.handlerWaitTimeoutMs ?? DEFAULT_HANDLER_WAIT_TIMEOUT_MS;
if (!Number.isSafeInteger(handlerWaitTimeoutMs) || handlerWaitTimeoutMs <= 0) {
throw new TypeError("Runtime Host handler wait timeout must be positive");
}
this.#handlerWaitTimeoutMs = handlerWaitTimeoutMs;
}

createTarget(epoch: string): RuntimeHostTargetIpcMain {
Expand Down Expand Up @@ -233,14 +249,29 @@ export class RuntimeHostReconnectingIpcMain {
): Promise<unknown> {
const epoch = this.#requireTargetEpoch(args[0]);
let handler: BoundHandler =
slot.handlers.get(epoch) ?? await this.#waitForHandler(slot, epoch);
slot.handlers.get(epoch) ??
await this.#waitForHandler(
slot,
epoch,
undefined,
this.#handlerWaitTimeoutMs,

@Astro-Han Astro-Han Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

() => new RuntimeHostHandlerUnavailableError(),
);
let reconciliationContext: unknown;
let reconciling = false;
let reconciliationDeadline: number | undefined;
const waitForReplacement = async (
previous: BoundHandler,
): Promise<BoundHandler | undefined> => {
if (!reconciling) return this.#waitForHandler(slot, epoch, previous);
if (!reconciling) {
return this.#waitForHandler(
slot,
epoch,
previous,
this.#handlerWaitTimeoutMs,
() => new RuntimeHostHandlerUnavailableError(),
);
}
const remainingMs = Math.max(
0,
(reconciliationDeadline ?? Date.now()) - Date.now(),
Expand Down Expand Up @@ -314,6 +345,7 @@ export class RuntimeHostReconnectingIpcMain {
epoch: string,
previous?: BoundHandler,
timeoutMs?: number,
timeoutError: () => Error = () => new ReconciliationWaitExpiredError(),

@Astro-Han Astro-Han Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

): Promise<BoundHandler> {
try {
this.#assertActive(epoch);
Expand All @@ -325,7 +357,7 @@ export class RuntimeHostReconnectingIpcMain {
return Promise.resolve(current);
}
if (timeoutMs !== undefined && timeoutMs <= 0) {
return Promise.reject(new ReconciliationWaitExpiredError());
return Promise.reject(timeoutError());
}
return new Promise((resolve, reject) => {
let timeout: ReturnType<typeof setTimeout> | undefined;
Expand All @@ -344,7 +376,7 @@ export class RuntimeHostReconnectingIpcMain {
if (timeoutMs !== undefined) {
timeout = setTimeout(() => {
if (!slot.waiters.delete(waiter)) return;
waiter.reject(new ReconciliationWaitExpiredError());
waiter.reject(timeoutError());
}, timeoutMs);
}
});
Expand Down