Version
1.64.0-next, d1ead3ecc. Regressed in #42676 (3b346c8df).
Steps to reproduce
A single tool call that runs longer than the idle timeout has its own browser closed underneath it, then fails.
const { client } = await startClient({ args: ['--idle-timeout=1000'] });
await client.callTool({ name: 'browser_navigate', arguments: { url: server.HELLO_WORLD } });
// One call, 3x the idle timeout.
const waited = await client.callTool({ name: 'browser_wait_for', arguments: { time: 3 } });Expected
The call completes. An idle timeout should measure time with nothing running, so a call in flight is by definition not idle.
At dc0f85227, the parent of the regressing commit, that is what happens:
wait isError: false | "### Result\nWaited for 3 ..."
log: {"create browser (persistent)":1,"create context":1}
Actual
At d1ead3ecc the same probe closes the browser mid-call and the call errors:
wait isError: true | "### Error\nError: No open pages available."
log: {"create browser (persistent)":1,"create context":1,"close browser":1}
The navigated page is gone too, so the next browser_snapshot comes back as about:blank and the client silently loses its state.
Cause
#42663 gave IdleTimer a refcount so the timer was cancelled for the duration of a call and only re-armed once the last one finished:
callStarted() { ++this._running; this.dispose(); }
callFinished() { if (!--this._running) this._timer = setTimeout(this._onIdle, this._timeout).unref(); }#42676 replaced both with a single poke() that BrowserBackend.callTool calls when a call starts:
poke() {
this.dispose();
this._timer = setTimeout(this._onIdle, this._timeout);
}so the clock now runs during the call rather than around it.
What made this easy to miss is that #42663 shipped a test for exactly this, does not close the browser while a tool call is running. #42666 removed it while consolidating to one test per scenario, and #42676 landed after that, so nothing was left to catch it. Re-adding that test would pin the behaviour.
One smaller thing in the same three lines: the re-armed timer used to be .unref()ed and no longer is. Now that the timer is armed by default with defaultIdleTimeout of an hour, a pending one will hold the event loop open. I did not chase that far enough to say whether anything else keeps the process alive anyway, so treat it as a note rather than a claim.
The default of an hour makes this hard to hit by accident, but any explicitly lowered --idle-timeout combined with a slow navigation or a long browser_wait_for runs into it.
I am a freshman in college trying to be genuinely useful on real projects, so if the new semantics are intentional and the timer is meant to bound total call time too, I am happy to be told so.
Version
1.64.0-next,
d1ead3ecc. Regressed in #42676 (3b346c8df).Steps to reproduce
A single tool call that runs longer than the idle timeout has its own browser closed underneath it, then fails.
Expected
The call completes. An idle timeout should measure time with nothing running, so a call in flight is by definition not idle.
At
dc0f85227, the parent of the regressing commit, that is what happens:Actual
At
d1ead3eccthe same probe closes the browser mid-call and the call errors:The navigated page is gone too, so the next
browser_snapshotcomes back asabout:blankand the client silently loses its state.Cause
#42663 gave
IdleTimera refcount so the timer was cancelled for the duration of a call and only re-armed once the last one finished:#42676 replaced both with a single
poke()thatBrowserBackend.callToolcalls when a call starts:so the clock now runs during the call rather than around it.
What made this easy to miss is that #42663 shipped a test for exactly this,
does not close the browser while a tool call is running. #42666 removed it while consolidating to one test per scenario, and #42676 landed after that, so nothing was left to catch it. Re-adding that test would pin the behaviour.One smaller thing in the same three lines: the re-armed timer used to be
.unref()ed and no longer is. Now that the timer is armed by default withdefaultIdleTimeoutof an hour, a pending one will hold the event loop open. I did not chase that far enough to say whether anything else keeps the process alive anyway, so treat it as a note rather than a claim.The default of an hour makes this hard to hit by accident, but any explicitly lowered
--idle-timeoutcombined with a slow navigation or a longbrowser_wait_forruns into it.I am a freshman in college trying to be genuinely useful on real projects, so if the new semantics are intentional and the timer is meant to bound total call time too, I am happy to be told so.