Read COMPUTER_BROWSER_IDLE_MS=0 as the documented switch-off, not as unset - #456
Open
kevin9327 wants to merge 1 commit into
Open
Read COMPUTER_BROWSER_IDLE_MS=0 as the documented switch-off, not as unset#456kevin9327 wants to merge 1 commit into
COMPUTER_BROWSER_IDLE_MS=0 as the documented switch-off, not as unset#456kevin9327 wants to merge 1 commit into
Conversation
…unset Zero is documented as keeping browsers resident and chooseIdle reads a timeout of zero as the sweep being switched off, but numberFromEnv discarded it: zero is not greater than zero, so the operator who typed it got the thirty-minute default and their browsers were closed anyway. An empty variable, which is what compose passes for an unset one, still means not set, and so do a negative and anything that is not a number.
kevin9327
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
September 8, 2026 22:36
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
COMPUTER_BROWSER_IDLE_MS=0is documented as the way to keep a Bot's browser resident, andchooseIdlereads a timeout of zero as the sweep being switched off. The value never arrived aszero.
numberFromEnvtreats anything that is not greater than zero as "not set", which is right forthe cap — a
COMPUTER_MAX_BROWSERSof zero would close every browser the moment it opened, and anunset variable declared in a compose file arrives as
"", not as absent — but it means the onesetting where zero is an answer had no way to express it. An operator who switched the sweep off got
the thirty-minute default back, and their browsers went on being closed.
Three places already say zero switches it off:
docs/configuration.md: "COMPUTER_BROWSER_IDLE_MS| How long an untouched browser is kept. 30minutes by default;
0keeps them resident."browser-eviction.ts: "A timeout of zero or less switches this off, so a deployment can keepbrowsers resident if it would rather."
browser-eviction.test.ts: "a timeout of zero switches it off".Reachable through
docker-compose.yml, which passesCOMPUTER_BROWSER_IDLE_MS: ${COMPUTER_BROWSER_IDLE_MS:-}straight into the computer, and through any deployment that runsagent-computerdirectly.Reproduction on
main(06633a4)The sweep an operator switched off closes the browser they asked to keep.
The change
numberFromEnvgainszeroSwitchesItOff, off by default, so every existing call site —PORT,NAVIGATION_TIMEOUT_MS,ACTION_TIMEOUT_MS,COMPUTER_MAX_BROWSERS— reads exactly as it did.COMPUTER_BROWSER_IDLE_MSis read with it on. Empty, absent, negative and non-numeric still take thefallback in both modes, which is what keeps a blank compose variable from switching a sweep off by
accident.
Fail before, pass after
The tests were written first, then
agent-computer/src/env.tsandagent-computer/src/profiles.tswere reverted to
mainwithgit checkout HEAD -- ...and the suites run against the two-argumentfunction:
with the failures reading
Expected: 0 / Received: 10000andExpected: [] / Received: ["bot-0"].With the fix restored:
Guard against widening. The existing "falls back on zero and negatives, so a bad timeout is never
enforced" passes before and after, unchanged: a two-argument call still refuses zero. Added
alongside the new behaviour are the values that must still be refused when zero is accepted —
"",unset,
-5,soon,1e999— and an ordinary value passing through untouched. Those pass in bothdirections too; the only tests that change colour are the three above.
Verification
The 20 failures are the same on
mainand on this branch: they areagent-computer/tests/shell.test.tsspawning
/bin/bashandagent-computer/tests/workspace.test.tscreating symlinks, neither of whichthis Windows machine can do. Nothing in this change touches either.
Notes
## Unreleasedand shares that anchor with other open PRs, soit may need a one-line rebase.
{{- with .Values.computers.browserIdleMs }}, and Gotemplates treat
0as empty, so a chart value of zero is dropped before it reaches the container.That is the same setting failing one layer earlier and is not touched here: I could not render the
chart to check it, so it is left as a separate question rather than guessed at.