Let an operator actually reach the browser limits - #96
Merged
Conversation
Found by driving them rather than by reading, and the second half is the reason it was worth driving.
`COMPUTER_MAX_BROWSERS` and `COMPUTER_BROWSER_IDLE_MS` were read by the code and declared nowhere, so
setting either on `docker compose up` did nothing at all: the container never saw them. They are in
the compose environment now and in the configuration reference.
Declaring them exposed the real bug. An unset variable in a compose file arrives as an empty string
rather than as absent, and `Number("")` is zero, so `?? 8` never fired and every deployment that had
not set a cap would have got a cap of zero — every browser closed the moment it opened, a computer
that looks broken rather than misconfigured. Anything that is not a positive number now falls back,
because "I typed this wrong" and "I did not set it" both mean the default.
Driven against the real container. With the cap at one, opening a second Bot's browser closed the
first and logged why, the closed Bot came straight back on its next request because the profile is on
the volume, and with the variable empty three browsers stayed up.
davidmckayv
requested review from
MikeRyanDev,
guidovizoso and
tylerslaton
as code owners
August 21, 2026 19:08
4 tasks
davidmckayv
pushed a commit
that referenced
this pull request
Aug 23, 2026
…r limits use (#114) * Read the computer's port and timeouts through the fallback the browser limits use #96 found that an unset variable declared in a compose file arrives as an empty string, so `Number.parseInt(process.env.X ?? "default")` never falls back: `??` sees "" rather than undefined, and the parse is NaN. It moved the browser limits onto `numberFromEnv`, which treats empty, absent, non-numeric and non-positive alike as "not set". The three values alongside them in `agent-computer/src/index.ts` kept the raw parse: `PORT`, `NAVIGATION_TIMEOUT_MS` and `ACTION_TIMEOUT_MS`. `ACTION_TIMEOUT_MS` is a documented variable (`.env.example:173`), so a deployment that sets it the way the compose file already sets the browser limits gets a NaN timeout, Playwright waiting on NaN instead of the 10s the default promises, a computer that looks broken rather than misconfigured. Export `numberFromEnv` and read the three through it. Adds a test for the helper, including the empty-string case that is the whole point. This is in agent-computer, which the root typecheck does not reach (that gap is #112). Verified in the package: `bunx tsc --noEmit` clean, and `bun test tests/number-from-env.test.ts` is 5/5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Move numberFromEnv to a playwright-free module so the test runs in CI The test imported `numberFromEnv` from `profiles.ts`, which hard-imports `playwright` at module load. CI installs only the root workspace, not `agent-computer`, so that import throws `Cannot find package 'playwright'`, drops the file's tests, and fails the `tests` and `verify` jobs. `numberFromEnv` moves to its own `agent-computer/src/env.ts`, which imports nothing; `profiles.ts` re-exports it so `index.ts` and the browser-limit constants are untouched, and the test imports it from `./env`. Verified with `agent-computer/node_modules` moved aside — a runner without playwright — where `bun test` now passes 5/5 instead of failing to import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: kevin9327 <kevin9327@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Two problems with the browser cap #90 added, both found by driving it against the real container
rather than by reading the diff.
The settings were unreachable.
COMPUTER_MAX_BROWSERSandCOMPUTER_BROWSER_IDLE_MSare read byprofiles.tsand were declared nowhere, soCOMPUTER_MAX_BROWSERS=1 docker compose updid nothing:the container never saw the variable. A limit an operator cannot set is not a limit.
Declaring them exposed the real bug. An unset variable in a compose file arrives as an empty
string rather than as absent, so
process.env.X ?? 8never fires andNumber("")is zero. Everydeployment that had not set a cap would have got a cap of zero: every browser closed the moment it
opened, which reads as a broken computer rather than a misconfigured one. Anything that is not a
positive number now falls back, because "I typed this wrong" and "I did not set it" both mean the
default.
This is the second time today an empty-string environment value has been the bug worth catching, the
first being
OPENBOT_SINGLE_USER. Worth remembering when adding a setting.Proof
Against the real container, rebuilt:
COMPUTER_MAX_BROWSERS=1: openingbeta's browser closedalpha's, loggedcomputer-browser-closedwiththe cap on running browsers was reached, and left the Bot thatjust asked running.
alphacame straight back on its next request, because the profile is on the volume. Closing onecosts a relaunch and nothing else.
Two more tests, one of which states the failure as the behaviour it would have caused.
bun run test:ci: 1010 tests, 0 fail.Where it runs
the right scope: the memory being bounded is that container's.
unrefed.