fix(browser): preserve debugger attachment across relay disconnects during navigation reattach#28725
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae7c2fcc04
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // When relay is down, still attach the debugger but skip sending the | ||
| // relay event. reannounceAttachedTabs() will notify the relay once it | ||
| // reconnects, so the tab stays tracked across transient relay drops. | ||
| await attachTab(tabId, { skipAttachedEvent: !relayUp }) |
There was a problem hiding this comment.
Preserve previous session identity on offline reattach
When the extension reconnects within the relay grace window, connectedTargets in src/browser/extension-relay.ts is intentionally retained and keyed by sessionId; this new path reattaches with skipAttachedEvent while offline, which always creates a new session ID in attachTab but never delivers a Target.detachedFromTarget for the old session. After reannounceAttachedTabs() runs, the relay can hold both stale and fresh sessions for the same tab, and CDP flows like Target.attachToTarget can select the stale session first, leading to follow-up command failures (tab not found / invalid session) despite a successful reconnect.
Useful? React with 👍 / 👎.
Greptile SummaryFixed critical bug where tabs were permanently lost when Chrome navigates during relay disconnect. The solution elegantly preserves debugger attachment with Key changes:
The fix is minimal, targeted, and backward compatible. Code follows existing patterns, properly handles edge cases, and includes clear explanatory comments. Confidence Score: 5/5
Last reviewed commit: ae7c2fc |
ae7c2fc to
b2af6b8
Compare
|
Landed via temp rebase onto main.
Checking formatting... All matched files use the correct format.
Found 0 warnings and 0 errors.
OK apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift
A2UI bundle up to date; skipping.
[copy-hook-metadata] Copied boot-md/HOOK.md
RUN v4.0.18 /Users/steipete/Projects/clawdbot3 RUN v4.0.18 /Users/steipete/Projects/clawdbot3 ✓ src/cli/program/preaction.test.ts (10 tests) 777ms Test Files 17 passed (17) ✓ src/slack/actions.read.test.ts (2 tests) 12ms Test Files 718 passed | 1 skipped (719)
Thanks @stone-jin! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2af6b8823
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // When relay is down, still attach the debugger but skip sending the | ||
| // relay event. reannounceAttachedTabs() will notify the relay once it | ||
| // reconnects, so the tab stays tracked across transient relay drops. | ||
| await attachTab(tabId, { skipAttachedEvent: !relayUp }) |
There was a problem hiding this comment.
Recheck relay state after reattach before skipping announce
relayUp is computed before the asynchronous attachTab(...) work, so if the relay reconnects while chrome.debugger.attach/Target.getTargetInfo is still running, this call still uses skipAttachedEvent: true and suppresses Target.attachedToTarget even though the socket is now open. Since tab re-announcement only happens during reconnect (scheduleReconnect -> reannounceAttachedTabs), a reconnect that already finished before this tab was reinserted can leave the tab permanently unannounced, causing continued tab not found-style failures until another disconnect/reconnect cycle or manual reattach.
Useful? React with 👍 / 👎.
Summary
onDebuggerDetachin the Chrome extension immediately gives up reattaching the debugger because the relay is down. The tab is permanently lost from tracking — even after the relay reconnects,reannounceAttachedTabs()finds nothing to announce because the tab was already removed from thetabsMap.tab not founderrors that require manual user intervention (clicking the extension icon again). Agents operating via Browser Relay lose the ability to interact with any page until the user manually re-attaches.onDebuggerDetachno longer bails out when the relay WebSocket is down. Instead, it attaches the debugger withskipAttachedEvent: true, keeping the tab in thetabsMap. When the relay reconnects,reannounceAttachedTabs()discovers the tab and sends the announce event — restoring full connectivity automatically.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
…) instead of "error" (red!) when the debugger is attached but relay is temporarily down.Security Impact (required)
NoNoNoNoNoRepro + Verification
Environment
Steps
Expected
listTabs()after the relay reconnectsActual (before fix)
Error: tab not foundEvidence
restores tabs after extension reconnects and re-announces— validates the full disconnect → grace period cleanup → reconnect → re-announce cyclepreserves tab across a fast extension reconnect within grace period— validates tabs survive brief relay dropsHuman Verification (required)
Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
background.jsassets/chrome-extension/background.jsreannounceAttachedTabsflow may need investigationRisks and Mitigations
reannounceAttachedTabssomehow doesn't fire after reconnectinitPromise.then(...)startup hook both trigger reconnect + re-announce, providing multiple recovery paths