fix(browser-control): stop idle sessions, and stop close-all from bricking the browser

[SandroHub013]

Issue for this PR

Closes #240
Closes #241

Type of change

• Bug fix
• New feature
• Refactor / code improvement
• Documentation

What does this PR do?

Two ways a browser outlived its use.

#240 — a forgotten session never lets go. A running session holds its browser (11 chrome.exe
processes on Windows) for as long as the daemon lives, and the daemon's idle shutdown cannot help:
it waits for zero running sessions, which is exactly what a forgotten start is not. Nothing put
an upper bound on it. Sessions now record when they were last driven — require() in
SessionManager is the single place every operation on a session goes through — and the daemon's
existing sweep stops the ones untouched for 30 minutes. That is what finally lets the daemon reach
its own idle shutdown and take the browser with it. A session streaming a live view or sampling a
recording is never reaped, since neither shows up as an operation.
NIKCLI_BROWSER_IDLE_MINUTES overrides the window and 0 disables reaping.

#241 — close-all bricked the browser. SessionManager.closeAll() ended with
Bun.WebView.closeAll(), which SIGKILLs the browser subprocess the whole Bun process shares. Bun
does not recover from it: every later view fails with Chrome process closed the pipe, then
Failed to spawn Chrome, even from a fresh SessionManager. In a daemon — which is the entire
nikcli session when hosted in-process — one close-all left the browser tool dead until a restart.
Closing each view is already enough, and the subprocess goes away with its parent.

The two are in one PR because the second blocks the first: with closeAll() poisoning the process,
no test could open a second session, so the reaper could not be covered.

Two things I got wrong on the way, both now encoded in tests:

• My first version logged each reap to stderr. ensureDaemon spawns the daemon with
  stderr: "pipe" and the CLI exits immediately after, closing the read end — so the first reap
  killed the daemon on a broken pipe, and the browser processes went away only because the daemon
  had died. The sweep is silent now, and the comment says why.
• Marking a session as used in require() caught too much: the status reads a client makes while
  watching a session (info, isRecording, recordingData, rawConsole) counted as use, so
  anything polling on a timer would have pinned a browser forever — the very leak this closes.
  require() is a lookup again; drive() marks use, and only the calls that actually drive the
  page go through it.

How did you verify your code works?

Windows 11 / Bun 1.4.0, Chrome 151.

Counting real OS processes through the CLI, with the window shortened via the env var:

chrome before:            0
chrome with live session: 12
chrome after idle window: 0      <- nobody called stop
list:                     [{ "name": "page-1", "status": "closed", ... }]

The daemon is still answering there — the session is stopped, not lost with it. Watching the whole
chain with the daemon's own idle shutdown temporarily shortened to 5s (untouched, because polling
/health resets its idle clock): session reaped → daemon exits → chrome 19 → 6.

• bun test in packages/browser-control: 17 pass, 41 expect() calls, no skips
• bun typecheck at the repo root: 35/35 packages
• prettier and oxlint clean on the touched files

Each new test was checked against the unfixed code, not just the fixed code:

Reintroduced defect	Result
Bun.WebView.closeAll() back in closeAll()	(fail) leaves the process able to open another session — Chrome process closed the pipe
reapIdle neutered to a no-op	(fail) both the manager and the daemon reaping tests
stderr log back in the sweep	(fail) survives reaping a session after the client process has exited — ECONNRESET
touch() back in require()	(fail) watching a session does not keep it alive

The last one is why that test drives the real cli.ts: a test that spawns the daemon itself keeps
the stderr pipe open and passes even with the bug present.

Screenshots / recordings

Not a UI change.

Checklist

• I have tested my changes locally
• I have not included unrelated changes in this PR

Notes for reviewers

• 30 minutes is a guess at "gave up on it" rather than "paused". Say the word and I will change it.
• Out of scope, but noticed while measuring: any client polling /health resets the daemon's idle
  clock, so a caller that health-checks on a timer keeps the daemon alive indefinitely. The session
  reaper here is independent of that, so the browser is still bounded. Happy to file it separately.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[nikomatt69]

/nikcli review and be sure all is legit and work

[github-actions]

Unable to connect. Is the computer able to access the url?

github run

[nikomatt69]

@cursoragent review pr and be sure all is legit and work

[cursor]

Reviewed #242 against #240/#241. The two bugs are real and the approach is sound: drive() vs require() is the right idle split, and dropping Bun.WebView.closeAll() is what stops close-all from bricking Chrome.

I found two problems in the posted diff and fixed them on this branch:

• UTF-8 BOM plus mangled em dashes in daemon.ts / manager.ts
• A finished live view could pin the browser: GET /screencast stopped the stream object but left isBusy() true after the client disconnected

bun test in packages/browser-control: 19 pass, 47 expect() calls, including the new live-view and recording cases. Pre-push typecheck: 35/35 packages. Review is on the PR.

  

[nikomatt69]

Review of #242 — the two bugs are real, the approach is sound, and the suite now passes here.

Checked against #240 / #241, the daemon/manager/session path, and a full bun test in packages/browser-control.

What holds up

• #241 close-all: dropping Bun.WebView.closeAll() is the right fix. CLI close-all goes through shutdownDaemon → manager.closeAll(); the regression test opens a second session in the same process and it stays running.
• #240 idle sessions: drive() vs require() is the correct split. Status reads (info, isRecording, recordingData, rawConsole, list) do not keep a session alive. NIKCLI_BROWSER_IDLE_MINUTES=0 disables reaping; garbage values fall back to 30 instead of disabling it. The sweep is silent, which is load-bearing (stderr: "pipe" + closed client).
• Tests actually ran (Chrome is present here, so they did not skip): 19 pass, 47 expect() calls, including the CLI-left-behind daemon test and the daemon self-reap test.
• Pre-push typecheck: 35/35 packages.

What I changed on this branch

Two real problems in the posted diff:

1. UTF-8 BOM + mojibake em dashes in daemon.ts / manager.ts (the review comment on line 1). Stripped; diffs now show — correctly.
2. A finished live view could pin the browser. GET /screencast called Screencast.stop() but left session.screencast set, so isBusy() stayed true after the client disconnected. isScreencasting() now requires an active stream, and finish() detaches that object (without stopScreencast(name), which would kill a replacement view). Covered by a new test.

Also fixed a stale SESSION_IDLE_MS reference (that identifier does not exist) and documented the 30-minute session window in the CLI help.

Out of scope, still true

• --record / an in-progress recording is never reaped (isBusy). Intentional, called out in the PR.
• Polling /health still resets the daemon idle clock. Session reaping is independent, so the browser is still bounded.

I would merge this.

[nikomatt69]

Review fatta. I due bug della PR sono veri e il fix regge.

Cosa è ok

• browser-control: close-all bricks the browser for the rest of the process #241 — togliere Bun.WebView.closeAll() è la mossa giusta. Un close-all non bricka più Chrome per il resto del processo; il test riapre una seconda sessione nello stesso process e resta running.
• browser-control: a forgotten session keeps its headless browser alive indefinitely #240 — lo split drive() / require() è corretto: goto/click/ecc. tengono viva la sessione, le letture di stato (info, list, isRecording, …) no. Sweep silenzioso (necessario: stderr: "pipe" + client uscito = broken pipe che uccideva il daemon). NIKCLI_BROWSER_IDLE_MINUTES=0 disabilita; un valore sporco torna a 30, non spegne il reaper.

Cosa ho trovato e sistemato su questo branch (ff8a57a3)

1. BOM UTF-8 + em dash mojibake in daemon.ts e manager.ts (â€" al posto di —). Ora senza BOM, come il resto di src/.
2. Live view finita che teneva il browser in vita. GET /screencast chiamava Screencast.stop() ma lasciava session.screencast settato, quindi isBusy() restava true dopo il disconnect del client. Ora isScreencasting() richiede uno stream attivo, e finish() fa detachScreencast (non stopScreencast(name), che ucciderebbe una view di rimpiazzo). Coperto da test.
3. Commento morto SESSION_IDLE_MS (quell'identificatore non esisteva) e help CLI aggiornato sulla finestra da 30 minuti.

Verifica
bun test in packages/browser-control: 19 pass, 47 expect(), Chrome presente quindi i test non sono skippati. Typecheck pre-push: 35/35 package.

Fuori scope, ma vero

• --record / recording in corso non viene reaped (isBusy) — voluto.
• Un client che polla /health tiene vivo il daemon; il reaper delle sessioni è indipendente, quindi il browser resta comunque bounded.

Per me è mergeabile.

[nikomatt69]

/nikcli analyze and validate pr

[github-actions]

Transport

nikcli session  |  github run