fix(browser-trace): make snapshot-loop responsive to SIGTERM and validate loop liveness

[Nagendhra-web]

Summary

browser-trace's snapshot-loop.mjs runs a forever-loop that takes a screenshot, dumps the DOM, fetches the URL, then waits interval-seconds before the next tick. The SIGTERM and SIGINT handlers flip a stopping flag that is only checked at the top of the while loop, so a SIGTERM that arrives during the inter-iteration sleep waits the full interval before the loop actually exits.

stop-capture.mjs sends SIGTERM and then falls back to SIGKILL after roughly three seconds. With the default 2 s interval the race is usually invisible, but with a longer interval-seconds (10, 30, ...) the snapshot loop is still asleep when the SIGKILL arrives. The user loses the last DOM and screenshot pair, half-rendered files end up as .partial leftovers that the parent has to sweep, and the run's index.jsonl is missing its final entry.

Two related lifecycle issues come along for the ride: the spawnSync('browse', ...) calls inside the loop have no timeout, so a hung browse CLI can block the loop indefinitely; and start-capture.mjs verifies that the browse cdp child survived its one-second sanity wait but does not apply the same check to the snapshot loop, so a loop that fails to start (missing dep, future syntax error) leaves the run with CDP events but zero DOM/screenshots and the user only notices after stop-capture.

This PR addresses all three under one cohesive scope.

Changes

skills/browser-trace/scripts/snapshot-loop.mjs

• Introduce a createStopSignal() factory that owns the SIGTERM/SIGINT subscription, exposes a boolean stopping view, and provides an abortable sleep(ms) whose promise resolves on either the timer firing or the stop being requested. The factory is exported so the unit test can drive it deterministically without touching the live process's signal handlers.
• Re-check stopping between major work units (screenshot, DOM, URL fetch, index append) so an in-flight tick that just missed the sleep wake-up still bails before starting more work.
• Pass a timeout option (O11Y_SNAPSHOT_TIMEOUT_MS, default 30 s) to every spawnSync('browse', ...) call. A hung browse CLI now skips the tick instead of blocking the loop, which keeps SIGTERM responsive.
• Wrap the CLI entry path in a guard so the module can be imported by tests without booting the sampler loop.

skills/browser-trace/scripts/start-capture.mjs

• Apply the existing one-second liveness check to the snapshot loop child as well, mirroring the existing CDP check. If the loop has died, kill CDP, surface the loop's stderr, and exit non-zero so the user sees the real error instead of a half-running capture.

skills/browser-trace/scripts/snapshot-loop.test.mjs (new)

• Five node --test cases covering createStopSignal:
  • stopping is false until trigger fires.
  • sleep(60_000) aborts in well under a second when the trigger fires mid-wait.
  • sleep(60_000) resolves synchronously after the trigger has already fired.
  • sleep(80) honors its interval when no trigger fires.
  • Repeated triggers are idempotent.

skills/browser-trace/package.json

• Add npm test -> node --test scripts/*.test.mjs so the test runs through the standard tooling and CI can pick it up later if you wire one in.

Verification

cd skills/browser-trace
npm test

> browser-trace@0.1.0 test
> node --test scripts/*.test.mjs

✔ stopping is false until trigger fires
✔ sleep wakes immediately when trigger fires mid-wait
✔ sleep resolves immediately when trigger has already fired
✔ sleep without a trigger waits for the requested interval
✔ repeated triggers are idempotent
ℹ tests 5
ℹ pass  5
ℹ fail  0
ℹ duration_ms 188.0

Net diff: +177 / -19 across four files in skills/browser-trace/. No new dependencies, no public surface removed.

Scope notes

• Behavior is purely additive for the default 2 s interval: the loop already exited within one cycle, and the new abortable sleep just removes the residual lag. Long-interval users see the real win.
• O11Y_SNAPSHOT_TIMEOUT_MS is opt-out (existing setups stay on the 30 s default). The env name follows the existing O11Y_ prefix used for O11Y_ROOT and O11Y_DOMAINS.
• createStopSignal() is exported so a future capture variant (e.g. capturing on multiple targets, or running snapshot logic inside another script) can reuse the same SIGTERM behavior without re-implementing the controller. Marked _trigger is documented as test-only.
• The start-capture.mjs check uses the same isAlive helper and the same one-second wait already in place; it is one symmetric block, not a new mechanism.

---

Note

Medium Risk
Touches capture lifecycle and process/signal handling, so failures could truncate runs or leave orphan processes, but the change is localized and adds unit tests plus timeouts to bound hangs.

Overview
snapshot-loop.mjs is refactored to use an exported createStopSignal() with an abortable sleep() so SIGTERM/SIGINT can break long inter-iteration waits, and each spawnSync('browse', ...) call now has a configurable timeout (O11Y_SNAPSHOT_TIMEOUT_MS, default 30s) to avoid indefinite hangs.

start-capture.mjs now also validates the snapshot loop process is still alive after the initial 1s sanity delay (mirroring the existing CDP check) and surfaces the loop log on early exit.

Adds scripts/snapshot-loop.test.mjs (Node’s built-in test runner) to unit-test stop-signal behavior and wires npm test in package.json to run these tests.

^{Reviewed by Cursor Bugbot for commit 1b958ad. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Nagendhra-web]

Thanks @cursor[bot]. The intermediate stop checks are unreachable in practice and the comment claiming "mid-iteration responsiveness" was misleading. Pushed 2a713a5 to remove them.

You are right about the runtime: between two synchronous spawnSync calls in the iteration body there is no event-loop tick, so Node never drains the SIGTERM callback queue, so stopping stays whatever the top-of-loop check observed. The only yield point that actually matters is await stop.sleep(intervalMs) at the bottom; the next iteration's while (!stop.stopping) then catches the flag.

The fix's actual SIGTERM responsiveness comes from two real mechanisms:

1. The O11Y_SNAPSHOT_TIMEOUT_MS (default 30 s) cap on every spawnSync('browse', ...) call. Without this, a hung browse CLI could pin the loop indefinitely and SIGTERM would never get a chance to land. With it, each iteration body finishes within at most 3 * SNAPSHOT_TIMEOUT_MS + tiny sync work, then the abortable sleep yields control, then the handler fires.

2. The abortable stop.sleep(intervalMs) at the bottom of the iteration. SIGTERM during this sleep resolves the promise immediately, the while-check picks up stopping = true, and the loop exits in well under a second instead of waiting out the full interval-seconds. This is what the unit test exercises.

Net change versus the previous commit on this branch: identical observable behavior, honest comment, and 12 fewer lines of code that suggested guarantees the runtime cannot actually provide. All 5 stop-signal unit tests continue to pass.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 2a713a5. Configure here.}

[Nagendhra-web]

Thanks again. Real leak, fixed in 1b958ad.

You are right on the analysis: every sleep(ms) was calling stopPromise.then(...), and since stopPromise only resolves at SIGTERM, every handler closure (capturing timer and resolve) sat on the promise's reaction list for the lifetime of the capture. At the default 2 s interval that is around 43,000 closures over 24 hours.

Switched the implementation to an explicit pending Set:

• sleep(ms) adds a { resolve, timer } entry. The timer's own callback removes the entry on natural wake-up, so the closure becomes unreachable the moment the sleep resolves.
• trigger() walks the set, clears each timer, calls each resolve, then clears the set in one pass.
• stopPromise and resolveStop are gone; the new design does not need a long-lived promise at all.
• Public surface is unchanged: same stopping getter, same sleep(ms). Added a test-only _pendingCount() accessor alongside the existing _trigger, both documented as test-only.

Two new unit tests guard the fix:

• completed sleeps deregister from the pending set so closures can be GC'd runs 200 back-to-back 1 ms sleeps and asserts _pendingCount() returns to 0 after each one. With the old code this assertion held by accident (the Set itself was empty, the leak was internal to the Promise reaction list), but it now directly exercises the path the regression would have to break.
• trigger drains every still-pending sleep without leaking starts three long sleeps, asserts _pendingCount() is 3, fires the trigger, asserts it drops to 0, and awaits all three for completion.

Verification:

> browser-trace@0.1.0 test
> node --test scripts/*.test.mjs

✔ stopping is false until trigger fires
✔ sleep wakes immediately when trigger fires mid-wait
✔ sleep resolves immediately when trigger has already fired
✔ sleep without a trigger waits for the requested interval
✔ repeated triggers are idempotent
✔ completed sleeps deregister from the pending set so closures can be GC'd (3048 ms)
✔ trigger drains every still-pending sleep without leaking
ℹ tests 7  ℹ pass 7  ℹ fail 0

Net change vs the previous commit: +44 / -8 across the same two files. No behavior change to the snapshot loop itself.

[jcalvin4]

Hi, I just discovered this repo and was wondering how you came to this solution for the loop problem?