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

CHANGELOG.md

@@ -8,6 +8,17 @@ 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.
 ### A flag or a family emoji in a channel preview is no longer cut in half
 
 The one line a roster draws is cut to a cap, and the cut walked code points -- right for a plain

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;
 }
 
 /**

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;

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;
+    }
+  });
 });

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);
+  });
+});