Skip to content

Read COMPUTER_BROWSER_IDLE_MS=0 as the documented switch-off, not as unset - #456

Open
kevin9327 wants to merge 1 commit into
CopilotKit:mainfrom
kevin9327:fix/idle-timeout-zero
Open

Read COMPUTER_BROWSER_IDLE_MS=0 as the documented switch-off, not as unset#456
kevin9327 wants to merge 1 commit into
CopilotKit:mainfrom
kevin9327:fix/idle-timeout-zero

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

COMPUTER_BROWSER_IDLE_MS=0 is documented as the way to keep a Bot's browser resident, and
chooseIdle reads a timeout of zero as the sweep being switched off. The value never arrived as
zero. numberFromEnv treats anything that is not greater than zero as "not set", which is right for
the cap — a COMPUTER_MAX_BROWSERS of zero would close every browser the moment it opened, and an
unset variable declared in a compose file arrives as "", not as absent — but it means the one
setting 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. 30
    minutes by default; 0 keeps them resident."
  • browser-eviction.ts: "A timeout of zero or less switches this off, so a deployment can keep
    browsers resident if it would rather."
  • browser-eviction.test.ts: "a timeout of zero switches it off".

Reachable through docker-compose.yml, which passes COMPUTER_BROWSER_IDLE_MS: ${COMPUTER_BROWSER_IDLE_MS:-} straight into the computer, and through any deployment that runs
agent-computer directly.

Reproduction on main (06633a4)

$ bun -e '
import { numberFromEnv } from "./agent-computer/src/env.ts";
import { chooseIdle } from "./agent-computer/src/browser-eviction.ts";
process.env.COMPUTER_BROWSER_IDLE_MS = "0";
const idle = numberFromEnv("COMPUTER_BROWSER_IDLE_MS", 30 * 60000);
const now = Date.now();
const running = new Map([["sales", { usedAt: now - 45 * 60000 }]]);
console.log("COMPUTER_BROWSER_IDLE_MS=0 -> IDLE_TIMEOUT_MS =", idle);
console.log("chooseIdle picks:", chooseIdle(running.entries(), idle, now));
'
COMPUTER_BROWSER_IDLE_MS=0 -> IDLE_TIMEOUT_MS = 1800000
chooseIdle picks: [ "sales" ]

The sweep an operator switched off closes the browser they asked to keep.

The change

numberFromEnv gains zeroSwitchesItOff, 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_MS is read with it on. Empty, absent, negative and non-numeric still take the
fallback 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.ts and agent-computer/src/profiles.ts
were reverted to main with git checkout HEAD -- ... and the suites run against the two-argument
function:

$ bun test agent-computer/tests/number-from-env.test.ts agent-computer/tests/browser-eviction.test.ts
(fail) reading the limits an operator set > a deployment that switched the sweep off keeps the browser it asked to keep
(fail) numberFromEnv where zero switches the setting off > keeps a zero an operator typed
(fail) numberFromEnv where zero switches the setting off > keeps a zero with whitespace around it, as a hand-edited .env has
 21 pass
 3 fail
Ran 24 tests across 2 files.

with the failures reading Expected: 0 / Received: 10000 and Expected: [] / Received: ["bot-0"].
With the fix restored:

$ bun test agent-computer/tests/number-from-env.test.ts agent-computer/tests/browser-eviction.test.ts
 24 pass
 0 fail
Ran 24 tests across 2 files.

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 both
directions too; the only tests that change colour are the three above.

Verification

$ bun test agent-computer/tests supervisor/tests
 243 pass / 20 skip / 20 fail          # 237 pass / 20 skip / 20 fail before, same 20 failures
$ cd agent-computer && bunx tsc --noEmit     # clean
$ bunx biome check <the four files>          # clean

The 20 failures are the same on main and on this branch: they are agent-computer/tests/shell.test.ts
spawning /bin/bash and agent-computer/tests/workspace.test.ts creating symlinks, neither of which
this Windows machine can do. Nothing in this change touches either.

Notes

  • The changelog entry is at the top of ## Unreleased and shares that anchor with other open PRs, so
    it may need a one-line rebase.
  • The Helm chart emits this variable through {{- with .Values.computers.browserIdleMs }}, and Go
    templates treat 0 as 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.

…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.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant