fix(browser): close tracked tabs after gateway restart#110797
Conversation
openclaw-barnacle
Bot
added
docs
clawsweeper
Bot
added
proof: sufficient
|
Codex review: found issues before merge. Reviewed July 19, 2026, 1:15 AM ET / 05:15 UTC. Summary PR surface: Source +1790, Tests +2841, Docs +48. Total +4679 across 39 files. Reproducibility: yes. from source, with high confidence: a manually closed durable CDP target can return a new protocol error string that does not match the existing ignorable-close classification, and identity-unavailable durable records have no demonstrated convergence path. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Keep ownership verification fail-closed, but make unrecoverable or confirmed-missing durable rows converge to removal under a narrowly specified, tested retention policy so stale records cannot exhaust the plugin store. Do we have a high-confidence way to reproduce the issue? Yes from source, with high confidence: a manually closed durable CDP target can return a new protocol error string that does not match the existing ignorable-close classification, and identity-unavailable durable records have no demonstrated convergence path. Is this the best way to solve the issue? Unclear. The ownership-verification approach is the right safety boundary, but indefinite retention is not the best complete fix because the fixed-capacity store can become unavailable to future opens. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2f7da3057c05. Label changesLabel justifications:
Evidence reviewedPR surface: Source +1790, Tests +2841, Docs +48. Total +4679 across 39 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (2 earlier review cycles) |
Yigtwxx
left a comment
Yigtwxx
left a comment
There was a problem hiding this comment.
Traced the cleanup convergence path. The design is careful — CAS claim tokens, restart-stable browser fingerprints from the /devtools/browser/<uuid> websocket path, no adoption of user tabs — and the restart tests are genuinely end-to-end rather than unit-level shape (reopening both the SQLite DB and the module, proving aliases are not recovered, covering cross-instance claim dedup and the 5001st-record rejection). That's above the usual bar.
My concern is that orphaned rows appear to have no retirement path, which turns a bounded reject-new store into a one-way ratchet. Two specifics inline.
One smaller thing, worth its own commit rather than a change here: cdp.helpers.ts:533 adds AbortSignal.any([ctrl.signal, init.signal]) inside fetchCdpChecked, which changes abort semantics for every CDP HTTP fetch in the extension, not just the new ownership probes. It looks correct and the new callers need it, but it's the one hunk in this diff whose blast radius is wider than tracked tabs. Otherwise the XL size reads honest to me — the bulk is tests and the source files all sit on the tracked-tab path.
| return await runCleanupWithLifecyclePriority({ ...params, candidate }); | ||
| } | ||
|
|
||
| export function isIgnorableTabCloseError(error: unknown): boolean { |
There was a problem hiding this comment.
This list came over from main's isIgnorableCloseError, where the only producer was BrowserTabNotFoundError (errors.ts:73, message starts "tab not found: "). This PR adds a second close route — session-tab-registry.ts → closeCdpTargetById → Target.closeTarget — and the CDP sender rejects with the protocol message verbatim (p.reject(new Error(parsed.error.message))).
For an unknown target that string is No target with given id found, which this repo emits itself in extension-relay/relay-bridge.ts:690,745,842,858. None of the four substrings match it.
So the most common real case — the user closes an OpenClaw tab by hand — leaves a durable row forever: every sweep re-claims it, probes /json/version, calls Target.closeTarget, gets a non-ignorable error, warns, and keeps the row. The tests don't catch it because the fixtures fabricate the string: session-tab-registry.sqlite.test.ts:516 and :966 both throw new Error("target not found"), which is in this list, so "converges on not-found" passes on a message the new path never actually produces.
Adding no target with given id would cover both producers.
|
|
||
| /** Opens and publishes Browser's canonical durable tab store during plugin registration. */ | ||
| export function initializeBrowserSessionTabStore(runtime: BrowserSessionTabStoreRuntime): void { | ||
| const sessionTabs = runtime.state.openSyncKeyedStore<unknown>({ |
There was a problem hiding this comment.
Related, and the reason the error-string gap above isn't just log noise: this namespace opens with maxEntries: 5_000, overflowPolicy: "reject-new" and no defaultTtlMs, though OpenKeyedStoreOptions supports one (src/plugin-state/plugin-state-store.types.ts:52). Combined with the non-durable / browser-identity-lookup-failed branch in session-tab-registry.ts — which returns without deleting whenever /json/version fails — a browser that never comes back at the same cdpUrl leaves its rows immortal. The only sweep is config-gated off by default (session-tab-cleanup.ts:41).
At the cap the failure isn't graceful: reject-new throws PLUGIN_STATE_LIMIT_EXCEEDED (plugin-state-store.sqlite.ts:497), which propagates out of trackSessionBrowserTab into the compensation path in browser-tool-session-tabs.ts:80-97 — that closes the tab the user just opened and rethrows. So once 5000 dead rows accumulate, every browser open on a durable profile opens a tab, closes it again, and errors, with no self-healing path.
A defaultTtlMs on the namespace, or an age-based retire in performDurableCleanup when identity lookup has been failing since trackedAt, would bound it.
clawsweeper
Bot
added
status: ⏳ waiting on author
|
Merged via squash.
|
* fix(browser): preserve tab cleanup across restarts * fix(browser): disambiguate restart tab aliases * fix(browser): keep untrack selection type private --------- Co-authored-by: Peter Steinberger <[email protected]>
3 participants
What Problem This Solves
After a Gateway restart, the in-memory browser-tab registry was empty.
/new, session lifecycle cleanup, and idle/cap sweeps therefore could not close tabs that OpenClaw had opened before the restart. The bug is most visible with sharedexisting-sessionbrowsers, where cleanup must recover OpenClaw ownership without ever adopting or closing user-created tabs.Why the Diff Is Large
This cannot safely be fixed by serializing the old
Map. A restart-safe cleanup path needs to prove both target ownership and browser-instance continuity before it closes anything. The production changes therefore establish a durable ownership boundary, while much of the diff is adversarial regression coverage for restart, alias, race, and mismatch cases.The reviewed version keeps the design deliberately narrow:
autoConnect, browser-bridge, and otherwise non-durable targets remain process-local.User Impact
Tabs created by OpenClaw through managed, remote-CDP, and explicit-
cdpUrlexisting-session profiles can now be cleaned after a Gateway restart. User-created tabs remain untouched. Tabs created before this version, and profiles without a stable browser identity, still require manual cleanup after restart.Release note: Fix browser-tab leaks across Gateway restarts while preserving shared-browser user tabs.
AI-assisted: yes. The implementation was deeply reviewed, refactored, and tested by a maintainer. A redacted agent transcript was intentionally not included at the requester's direction.
Evidence
Head:
24226ec9fee3da36f4d5157da348086f49740875pnpm buildandpnpm checkpass on the reviewed tree, including TypeScript, lint, formatting, import-cycle, database-first, SDK-surface, and policy guards. Testbox proof: Actions run 29673821849.git diff --checkpasses.Real behavior proof
A real headless Chrome stayed alive while two separate Node/OpenClaw processes shared the plugin SQLite state, simulating a Gateway restart. The harness created one user tab, one tracked OpenClaw tab, and one tracked tab that disappeared before cleanup. Process B reopened the registry and ran the production lifecycle cleanup path.
Observed result: the OpenClaw tab closed, the already-missing row retired, the durable store emptied, and the user tab survived.
What was not live-tested: durable restart cleanup for
existing-session --autoConnect, because that mode intentionally has no restart-stable native ownership; cross-platform browser behavior. Those boundaries are covered by focused regression tests.Made with Cursor