T5: getStatus, relaunch, getLaunchOptions, stop->launch leak fixes by krassx · Pull Request #41 · bugsee/bugsee-javascript

krassx · 2026-10-12T04:04:31Z

Review-only PR. This change is already on main (the repo works directly on main; see CLAUDE.md). The base branch is the commit it forked from, so the diff below is exactly this task. Merging this PR changes nothing. Review findings become follow-up commits on main. Plan: docs/design/mobile-api-alignment.md.

Mobile parity: getStatus, relaunch, getLaunchOptions, getHostLaunchOptions.

Also fixes real leaks found in launch -> stop -> launch: browser instance Web Lock never released, three IndexedDB connections never closed, stop ordering vs the recovery scan, stale stop() evicting a relaunched client, node double teardown, and node clean-stop subtrees never recovered while the pid lives (now marked .stopped).

Review focus / known gaps

Nothing run on real Bun/Deno/Electron; edge/worker clients return {} from the options methods and have no relaunch.
Two concurrent launch() calls can still race (pre-existing).
relaunch() without options uses the globalThis carrier; a custom carrier must be passed again.
Merge fix on main: Stopped is emitted after stopping clears.

🤖 Generated with Claude Code

Adds the mobile launch-state surface: BugseeStatus (Stopped/Launching/Launched/Stopping), client and process-level getStatus(), relaunch(options?) on the browser, node and umbrella entries, and getLaunchOptions()/getHostLaunchOptions() keyed by canonical com.bugsee.option.* (registered options only, never the token or transport). Fixes found while verifying launch -> stop -> launch: - browser: the instance Web Lock and the three IndexedDB connections were never released on stop; stop now flushes the capture store, waits out the launch-time recovery scan, then releases them - browser: dead-sibling recovery left its generation cleanup unawaited - browser/node: a repeat or late stop() cleared the process slot unconditionally and could evict a relaunched client; node also ran its teardown twice - node: a cleanly stopped instance's subtree was never recovered while its pid lived (one stranded subtree of capture files per cycle); stop now marks it .stopped so the next launch recovers and removes it 🤖 Generated with [Claude Code](https://claude.com/claude-code)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

cursor

T5 (getStatus / relaunch / getLaunchOptions, plus the launch→stop→launch leak fixes) is the right slice, and the sequential browser/node cycle work is real: identity-based clearCarrierClient, process status around async browser launch, canonical-only option views, and the node .stopped marker for a still-live pid all match the intended contract.

The new stop/relaunch paths still have a few concrete failure modes. The browser killed/!isLaunched early-return undoes the lock/IDB release this PR is here to add; relaunch(partial) replaces the previous launch bag rather than merging a delta; node recovery can miss a subtree when launch() overlaps an in-flight drain; and a deadline-bound stop(timeout) reintroduces the IDB reopen leak the new wait was meant to prevent.

Findings

P1 — Browser `stop()` after KILL_SDK never releases the Web Lock or IDB connections

Location: packages/browser/src/launch.ts:815-817
Problem: publicClient.stop() snapshots client.isLaunched() (the core client) and, when it is false, returns before the finally that flushes, awaits recovery, and calls coexistence.release().
Impact: After collector KILL_SDK (core sets launched = false without running platform teardown), stop() / relaunch() leave the instance lock held and the three IndexedDB connections open. The next launch in this tab cannot recover that instance (lock still looks live). Same-page relaunch with a new token also leaks connections until navigation.
Scenario: persist: true launch → upload returns 99099 → onError / relaunch(). Core halt already made isLaunched() false, so the new stop wrapper skips release().
Fix: Always run the flush/wait/coexistence.release() cleanup. Gate only the drain on wasLaunched, not lock/IDB release. Drive the existing test through the core client’s isLaunched() (the spy on the spread publicClient never runs).

P1 — `relaunch(options)` replaces the entire previous launch bag

Location: packages/core/src/relaunch.ts:26
Problem: launch(last.appToken, options ?? last.options) uses the new object as the full options bag. Design §lifecycle is relaunch(opts?: Partial<O>); T5 is mobile parity. Tests call relaunch(opts({ maxRecordingTime: 15 })), so the harness re-supplies transport / window / process and hides the loss.
Impact: A natural delta (relaunch({ captureLogs: false })) resets persist, endpoint, dataDir, onError, and every other omitted field to platform defaults. Staging traffic can silently move to api.bugsee.com; a disk-backed node session becomes in-memory; crash persistence turns off.
Scenario: launch(token, { persist: true, endpoint: 'https://staging', captureLogs: true }) then relaunch({ captureLogs: false }).
Fix: Merge { ...last.options, ...options } (and document it). Keep replace only if it is an explicit, tested, user-facing contract — not just a PR comment.

P2 — Node `.stopped` is written after drain, so overlapping `stop()`+`launch()` misses recovery

Location: packages/node/src/launch.ts:1151-1154 (consulted at recover-instances.ts:225)
Problem: Slot clear, heartbeat stop, and writer dispose are synchronous at the start of stop(). .stopped is written only in stopCore().then(markStopped) after the upload drain. The next launch() runs recoverInstances immediately and treats a live-pid subtree without .stopped as still owned.
Impact: The stranded-subtree leak this PR fixes still happens for one cycle when launch() is not sequenced behind a finished drain. Sequential await stop(); launch() and relaunch() (which awaits stop()) are fine. A later launch recovers the files; a process that only ever does one overlapping pair keeps them until exit.
Scenario: In-flight logException → const p = client.stop(); launch(token, { dataDir }) without awaiting p — the same overlap the status tests already exercise.
Fix: Mark the subtree recoverable as soon as this instance has stopped mutating it (after dispose/heartbeat stop), and rely on upload/server dedup for overlap with an in-flight drain; or have the new launch wait for in-process drains on the same dataDir before scanning.

P2 — `stop(timeout)` can leak the IndexedDB connections `release()` was added to close

Location: packages/browser/src/launch.ts:832-835
Problem: The comment at 829–831 states that closing under an in-flight recovery scan reopens the connection and leaks it, so stop waits on recoveryDone. A finite timeout Promise.races that wait, then still calls coexistence.release(). The scan continues, open() creates a new dbPromise, and nothing closes it. The timer is also never cleared.
Impact: Deadline-bound stop during a slow dead-sibling replay (or whenReady hydration) leaks one connection set per call. SPA logout stop(100) + persist: true is enough.
Scenario: persist: true launch while a sibling recovery upload is in flight; await client.stop(20).
Fix: If the race loses, do not release() (or detach a closer that runs when recoveryDone actually settles). Clear the timeout timer. The existing deadline test should assert open connection count, not only that stop returns quickly.

Related (out of this diff, still the same leak class)

@bugsee/webworker still does setCarrierClient(undefined) on every stop() and never calls coexistence.release(). A stale worker stop() can evict a relaunched client; locks/IDB stay held for the activation.

Overall

Risk: Medium
Merge recommendation: Request changes
Fix first: browser killed-path release(); then relaunch merge vs replace; then node overlapping .stopped and timed stop/IDB close
What looks solid: clearCarrierClient identity check; Launching/Stopping process status; canonical-only option views (no token/transport); .stopped + live-pid recovery for the sequential path; umbrella relaunch going through platform launch so extensions re-wire; 3-cycle listener/lock/IDB tests on the happy path

_{Sent by Cursor Automation: Bugsee code review}

cursor · 2026-10-12T08:48:59Z

+    async stop(timeout?: number): Promise<boolean> {
+      const wasLaunched = client.isLaunched();
+      // Release the process slot, but only if it is still OURS: a repeat/late stop() must not evict a
+      // client that has since been relaunched.
+      const owned = getCarrierClient(carrier) === publicClient;
+      if (owned) {
+        setCarrierStatus(wasLaunched ? BugseeStatus.Stopping : BugseeStatus.Stopped, carrier);
+        clearCarrierClient(publicClient, carrier);
+      }
      uninstallPageHideFlush();
-      return stopCore(timeout);
+      if (!wasLaunched) {
+        return stopCore(timeout);


P1: This early return skips the finally that flushes, waits for recovery, and calls coexistence.release().

enterKillState in client.ts sets launched = false without running platform teardown. The next stop() / relaunch() therefore leaves the instance Web Lock held and the IndexedDB connections open. A later launch in this tab cannot recover that instance (the lock still looks live).

wasLaunched is client.isLaunched() on the core client closed over here, not the spread publicClient. launch-lifecycle.test.ts spies publicClient.isLaunched, so it never takes this branch and still runs release().

Always run the lock/IDB cleanup. Only the upload drain needs wasLaunched. Pin the test by killing the core client (or calling the real KILL_SDK path) with persist: true and asserting the lock is free / open connection count is 0.

cursor · 2026-10-12T08:48:59Z

+    throw new Error('Bugsee.relaunch() called before launch(); there is no app token to reuse');
+  }
+  await getCarrierClient<Client>(globalObj)?.stop();
+  return launch(last.appToken, options ?? (last.options as Options));


P1: options ?? last.options treats a provided object as the entire next launch bag, not a delta.

Design §lifecycle is relaunch(opts?: Partial<O>), and T5 is mobile parity. Lifecycle tests call relaunch(opts({ maxRecordingTime: 15 })), so the harness re-supplies transport / window / process and never shows the loss.

A real caller that does relaunch({ captureLogs: false }) after launch(token, { persist: true, endpoint: 'https://staging' }) silently drops persistence and sends to https://api.bugsee.com. Same for node dataDir.

Merge { ...last.options, ...options } when options is provided (empty {} then still means “same token, default options” only if that is an intentional, documented behavior).

cursor · 2026-10-12T08:48:59Z

+      return stopCore(timeout)
+        .then((drained) => {
+          markStopped(drained);


P2: .stopped is written only after stopCore (the upload drain) settles, but the carrier slot is already cleared and the heartbeat/writer are already gone.

recoverInstances treats a live-pid subtree without .stopped as still owned (recover-instances.ts:225-234). So:

void client.logException(err); const stopping = client.stop(); // drain in flight, no .stopped yet launch(token, { dataDir }); // scan skips this subtree await stopping; // .stopped appears too late

That is the overlap the status tests already allow. Sequential await stop(); launch() and relaunch() (which awaits stop()) are fine; a process that only ever does one overlapping pair keeps the files until exit — the leak this PR is meant to close.

Mark the subtree recoverable as soon as this instance has stopped mutating it (after dispose / heartbeat stop), and let upload/server dedup handle overlap with an in-flight drain.

cursor · 2026-10-12T08:49:00Z

+        // The launch-time dead-sibling scan may still be reading IndexedDB (a quick launch -> stop); closing
+        // under it would just make it reopen the connection and leak it. Let it settle first — bounded by
+        // the caller's own timeout, since a long recovery upload must not hold up a deadline-bound stop.
+        await (timeout === undefined
+          ? recoveryDone
+          : Promise.race([recoveryDone, new Promise<void>((r) => setTimeout(r, timeout))]));
+        coexistence.release();


P2: The comment correctly states that closing under an in-flight recovery scan reopens the connection and leaks it. The timeout branch then does exactly that: Promise.race can win, coexistence.release() runs, the scan calls open() again, and nothing closes the new dbPromise. The timeout timer is also never cleared.

stop(20) during a slow dead-sibling replay (or a whenReady that never settles) leaks one connection set per call. The deadline test asserts only that stop returns quickly, not that open() returns to 0.

If the race loses, skip release() here and close when recoveryDone actually settles (or attach a closer to that promise). Clear the timer. Extend the deadline test to count IDB connections.

…tem; document node .stopped timing - PR #41/#43 reviews 🤖 Generated with [Claude Code](https://claude.com/claude-code)

krassx · 2026-10-12T11:05:33Z

Review findings triaged against current main (this PR branch is a frozen snapshot, so its diff does not show the fixes): Findings were fixed on main earlier (browser lock release after KILL_SDK, relaunch merge semantics, IDB release on stop(timeout)); node .stopped timing documented as a kept difference (a1caa99).

The overview PR #46 (head = main) was re-reviewed at ba608c4: no remaining findings, recommendation Approve.

🤖 Generated with Claude Code

krassx · 2026-10-12T12:16:57Z

Closing instead of merging: this review-only branch is a frozen snapshot that now conflicts with the review base branch after #37/#40/#43/#46 were merged. Its content is already on main, and every finding was addressed there (see the earlier triage comment; #46 was approved at the final review).

🤖 Generated with Claude Code

krassx added 2 commits October 12, 2026 08:16

ci: retrigger checks and automated review

cf059a8

🤖 Generated with [Claude Code](https://claude.com/claude-code)

cursor Bot reviewed Oct 12, 2026

View reviewed changes

krassx added a commit that referenced this pull request Oct 12, 2026

fix(core): a failed urgent notify at the queue cap keeps the newest i…

a1caa99

…tem; document node .stopped timing - PR #41/#43 reviews 🤖 Generated with [Claude Code](https://claude.com/claude-code)

krassx closed this Oct 12, 2026

Navigation Menu

Repository navigation

T5: getStatus, relaunch, getLaunchOptions, stop->launch leak fixes - #41

T5: getStatus, relaunch, getLaunchOptions, stop->launch leak fixes#41
krassx wants to merge 2 commits into
review/base-1from
review/t5

krassx commented Oct 12, 2026

Uh oh!

cursor Bot left a comment

Uh oh!

cursor Bot Oct 12, 2026

Uh oh!

cursor Bot Oct 12, 2026

Uh oh!

cursor Bot Oct 12, 2026

Uh oh!

cursor Bot Oct 12, 2026

Uh oh!

krassx commented Oct 12, 2026

Uh oh!

krassx commented Oct 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

krassx commented Oct 12, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Findings

P1 — Browser stop() after KILL_SDK never releases the Web Lock or IDB connections

P1 — relaunch(options) replaces the entire previous launch bag

P2 — Node .stopped is written after drain, so overlapping stop()+launch() misses recovery

P2 — stop(timeout) can leak the IndexedDB connections release() was added to close

Related (out of this diff, still the same leak class)

Overall

Uh oh!

cursor Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

krassx commented Oct 12, 2026

Uh oh!

krassx commented Oct 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

P1 — Browser `stop()` after KILL_SDK never releases the Web Lock or IDB connections

P1 — `relaunch(options)` replaces the entire previous launch bag

P2 — Node `.stopped` is written after drain, so overlapping `stop()`+`launch()` misses recovery

P2 — `stop(timeout)` can leak the IndexedDB connections `release()` was added to close