From 153398a6da47c3fc7496dc3fed1e32d5b1dbfe59 Mon Sep 17 00:00:00 2001 From: kevin9327 <5299031+kevin9327@users.noreply.github.com> Date: Wed, 9 Sep 2026 07:34:57 +0900 Subject: [PATCH] Read COMPUTER_BROWSER_IDLE_MS=0 as the documented switch-off, not as 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. --- CHANGELOG.md | 12 +++++ agent-computer/src/env.ts | 21 ++++++++- agent-computer/src/profiles.ts | 8 +++- agent-computer/tests/browser-eviction.test.ts | 27 +++++++++++ agent-computer/tests/number-from-env.test.ts | 45 +++++++++++++++++++ 5 files changed, 110 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dba57a687..6360f7b8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### `COMPUTER_BROWSER_IDLE_MS=0` now keeps browsers resident, as it says it does + +Zero is the documented way to switch off the sweep that closes a Bot's browser after it has sat +untouched, and the sweep itself reads a timeout of zero as being switched off. The value never got +that far. It was read the way the cap on running browsers is, where zero would close every browser +the moment it opened and so has to be refused, and an operator who typed zero got the thirty-minute +default handed back instead. Their browsers went on being closed, which is a Bot signed out of a site +that only issues session cookies and a cold Chromium on its next turn. Zero is now kept for this one +setting. A blank variable, which is what an unset variable declared in a compose file arrives as, is +still not zero: it means "not set" and takes the default, as do a negative and anything that is not a +number. + ### One command to stop what `start.sh` started Stopping the local stack meant four commands read off the end of a successful start, and the one diff --git a/agent-computer/src/env.ts b/agent-computer/src/env.ts index 37565f588..96eaacf10 100644 --- a/agent-computer/src/env.ts +++ b/agent-computer/src/env.ts @@ -5,14 +5,31 @@ * compose file arrives as an empty string rather than as absent, so `??` never fires and the parse * yields `NaN`. Empty, absent, non-numeric and non-positive all mean "not set" and take the fallback. * + * `zeroSwitchesItOff` is for the one setting where zero is an answer rather than a mistake. + * `COMPUTER_BROWSER_IDLE_MS=0` is documented as "keeps them resident", and `chooseIdle` reads a + * timeout of zero as the sweep being switched off — but the value never reached it, because zero is + * not greater than zero, so an operator who switched the sweep off got the thirty-minute default and + * their browsers were closed anyway. It stays off by default: a cap of zero closes every browser the + * moment it opens, and a timeout of zero would be the same mistake if it were read from a blank + * variable rather than from an operator who typed it. + * + * Empty, absent, non-numeric and negative still take the fallback either way. That is what keeps the + * empty string a compose file passes for an unset variable from switching a sweep off by accident, + * which is the whole reason this function exists rather than a bare `Number`. + * * Its own module, free of the `playwright` import `profiles.ts` carries, so a test can reach it * without loading a browser driver that is not installed where the tests run. */ -export function numberFromEnv(name: string, fallback: number): number { +export function numberFromEnv( + name: string, + fallback: number, + { zeroSwitchesItOff = false }: { zeroSwitchesItOff?: boolean } = {}, +): number { const raw = process.env[name]?.trim(); if (!raw) return fallback; const value = Number(raw); - return Number.isFinite(value) && value > 0 ? value : fallback; + if (!Number.isFinite(value)) return fallback; + return (zeroSwitchesItOff ? value >= 0 : value > 0) ? value : fallback; } /** diff --git a/agent-computer/src/profiles.ts b/agent-computer/src/profiles.ts index 0d55cfc47..53714e281 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -204,8 +204,14 @@ const MAX_LIVE_BROWSERS = numberFromEnv("COMPUTER_MAX_BROWSERS", 8); * * The other half. A deployment under the cap still holds a browser per Bot that was used once last * Tuesday, and that memory is doing nothing for anybody. + * + * Zero is the documented way to say "keep them resident", which is why it is read as a value here + * rather than as a value that is not set. Read like the cap, an operator who wrote it got the + * default back and the sweep they had switched off carried on closing their browsers. */ -const IDLE_TIMEOUT_MS = numberFromEnv("COMPUTER_BROWSER_IDLE_MS", 30 * 60_000); +const IDLE_TIMEOUT_MS = numberFromEnv("COMPUTER_BROWSER_IDLE_MS", 30 * 60_000, { + zeroSwitchesItOff: true, +}); /** How often the idle sweep looks. Cheap: it walks a map of at most `MAX_LIVE_BROWSERS`. */ const IDLE_SWEEP_MS = 60_000; diff --git a/agent-computer/tests/browser-eviction.test.ts b/agent-computer/tests/browser-eviction.test.ts index 218178c9a..8f7efd827 100644 --- a/agent-computer/tests/browser-eviction.test.ts +++ b/agent-computer/tests/browser-eviction.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { chooseEvictions, chooseIdle } from "../src/browser-eviction"; +import { numberFromEnv } from "../src/env"; /** * How many browsers one computer holds, and for how long. @@ -126,4 +127,30 @@ describe("reading the limits an operator set", () => { [], ); }); + + test("a deployment that switched the sweep off keeps the browser it asked to keep", () => { + /* + * The two halves, read together, because separately both were right and the pair was not. + * `COMPUTER_BROWSER_IDLE_MS=0` is the documented way to keep browsers resident and the timeout + * of zero above switches this off — but the value never arrived as zero. It was read the way the + * cap is, where zero is a mistake, so it took the thirty-minute default and this Bot's browser + * was closed under an operator who had said not to. + */ + const previous = process.env.COMPUTER_BROWSER_IDLE_MS; + process.env.COMPUTER_BROWSER_IDLE_MS = "0"; + try { + const idleTimeoutMs = numberFromEnv( + "COMPUTER_BROWSER_IDLE_MS", + 30 * 60_000, + { zeroSwitchesItOff: true }, + ); + const now = Date.now(); + expect( + chooseIdle(running(now - 45 * 60_000), idleTimeoutMs, now), + ).toEqual([]); + } finally { + if (previous === undefined) delete process.env.COMPUTER_BROWSER_IDLE_MS; + else process.env.COMPUTER_BROWSER_IDLE_MS = previous; + } + }); }); diff --git a/agent-computer/tests/number-from-env.test.ts b/agent-computer/tests/number-from-env.test.ts index 6233aa0f1..00aa36da3 100644 --- a/agent-computer/tests/number-from-env.test.ts +++ b/agent-computer/tests/number-from-env.test.ts @@ -36,3 +36,48 @@ describe("numberFromEnv", () => { expect(numberFromEnv(NAME, 10000)).toBe(10000); }); }); + +/** + * The setting where zero is an answer rather than a mistake. + * + * `COMPUTER_BROWSER_IDLE_MS=0` is documented as keeping browsers resident, and `chooseIdle` reads a + * timeout of zero as the sweep being switched off. Neither was reachable: zero is not greater than + * zero, so the operator who typed it got the thirty-minute default and the sweep they had switched + * off went on closing their browsers. + */ +describe("numberFromEnv where zero switches the setting off", () => { + test("keeps a zero an operator typed", () => { + process.env[NAME] = "0"; + expect(numberFromEnv(NAME, 10000, { zeroSwitchesItOff: true })).toBe(0); + }); + + test("keeps a zero with whitespace around it, as a hand-edited .env has", () => { + process.env[NAME] = " 0 "; + expect(numberFromEnv(NAME, 10000, { zeroSwitchesItOff: true })).toBe(0); + }); + + test("still reads the empty string a compose file passes as unset, not as zero", () => { + // The trap this whole function exists for, and the reason zero stays off by default: a variable + // declared and left blank must not be read as an operator switching a sweep off. + process.env[NAME] = ""; + expect(numberFromEnv(NAME, 10000, { zeroSwitchesItOff: true })).toBe(10000); + delete process.env[NAME]; + expect(numberFromEnv(NAME, 10000, { zeroSwitchesItOff: true })).toBe(10000); + }); + + test("still falls back on a negative and on a value that is not a number", () => { + // Off is spelled zero. Anything else that is not a length of time is a mistake, and a mistake + // takes the default rather than switching something off on an operator's behalf. + for (const bad of ["-5", "soon", "1e999"]) { + process.env[NAME] = bad; + expect(numberFromEnv(NAME, 10000, { zeroSwitchesItOff: true })).toBe( + 10000, + ); + } + }); + + test("leaves an ordinary value alone", () => { + process.env[NAME] = "5000"; + expect(numberFromEnv(NAME, 10000, { zeroSwitchesItOff: true })).toBe(5000); + }); +});