Repository navigation
fix(discord): self-healing CDP scanner — relaxed target match + idle-watchdog pump (#3693) - #4108
Conversation
…inyhumansai#3693) Add CdpConn::pump_events_resilient for long-lived consumers: a background drain task forwards session-matched frames off the fixed-capacity broadcast ring into an unbounded queue (so a burst over EVENT_CHANNEL_CAP is buffered, not silently dropped), and an idle-timeout watchdog returns Ok once no frame arrives within the window so a stale/destroyed page target forces re-attach. Plain pump_events is unchanged (cdp/session.rs has its own watchdog). Core logic split into pump_resilient_core over raw primitives + unit tests (idle-timeout, close-flush, non-lossy burst, session filter).
…targets (tinyhumansai#3693) The Discord scanner is the only long-lived CDP pump consumer. The plain pump returned only when the whole transport closed, so a stale page target (Ctrl+R reload, renderer crash, hard navigation) left it awaiting forever and the outer reconnect loop never re-entered — MITM silently stopped. Switch to pump_events_resilient with PUMP_IDLE_TIMEOUT=90s (>2x Discord's ~41s gateway heartbeat) so a dead session forces re-attach, and bursts buffer instead of dropping frames. Correct the now-inaccurate run_mitm_session + pump doc comments to match the in-process implementation.
|
Warning Review limit reached
More reviews will be available in 7 minutes and 49 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds a resilient CDP event pump with idle-timeout handling and buffered delivery, updates Discord scanner attachment to pin and resolve page targets across reconnects, threads the pin through DOM polling, and adds target-resolution tests. ChangesResilient CDP attachment
Sequence Diagram(s)sequenceDiagram
participant spawn_scanner
participant run_mitm_session
participant attach_account_target
participant resolve_page_target
participant CdpConn
spawn_scanner->>run_mitm_session: pinned_target_id
run_mitm_session->>attach_account_target: attach_account_target(..., pinned_target_id)
attach_account_target->>resolve_page_target: resolve_page_target(...)
resolve_page_target-->>attach_account_target: targetId + strict
run_mitm_session->>CdpConn: pump_events_resilient(session_id, PUMP_IDLE_TIMEOUT, ...)
CdpConn-->>run_mitm_session: return Ok(()) on idle timeout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…hash strip (tinyhumansai#3693) Discord's web client replaceState's to its canonical /channels URL on boot, stripping the #openhuman-account-<id> fragment the webview opens with. Both the MITM attach and the DOM poll matched strictly on that fragment, so attach failed forever once the hash was gone — the scanner never attached after the page settled. Port the Slack scanner's pin→strict→relaxed resolution (shared attach_account_target + pure resolve_page_target) into run_mitm_session and dom_scan_once, persisting a target-id pin across reconnects. Adds 5 resolution unit tests.
…sai#3693) The resilient pump's drain forwarded empty-session (browser/target-level) frames alongside own-session ones. A dead/reloaded page still emits empty-session transport chatter (target lifecycle, crash churn), which reached the consumer and reset the 90s idle timer — so a crashed session never tripped re-attach (caught in live renderer-crash smoke; the watchdog failed to fire for 3+ min). Forward strictly own-session frames; Discord's real traffic (Network.*, incl. ~41s gateway heartbeats) is always page-session-tagged, so nothing useful is dropped. Updates the session-filter test as a regression guard.
oxoxDev
changed the title
coderabbitai
Bot
added
the
rust-core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b220bd806e
ℹ️ 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".
| cdp.pump_events_resilient(&session_id, PUMP_IDLE_TIMEOUT, |method, params| { | ||
| dispatch_event(app, account_id, method, params, &mut ingest_state); | ||
| }) |
There was a problem hiding this comment.
When the new idle watchdog fires while the Discord page target is still alive but quiet (for example a logged-out Discord page, temporary offline state, or a throttled gateway), this .await returns Ok(()) and the outer loop immediately attaches a new session, but the old Target.attachToTarget session is never detached. Because CdpConn is just a handle over the shared in-process transport, dropping it does not send Target.detachFromTarget, so this can accumulate enabled Network sessions and duplicate/extra CDP traffic every 90 seconds until the webview is closed.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src-tauri/src/cdp/conn.rs`:
- Around line 174-229: The drain loop in the async pump is swallowing
RecvError::Lagged and continuing, which breaks the non-lossy contract; update
the tokio::spawn drain path to treat lag as a hard failure by returning an error
or triggering the same re-attach flow used on idle/closed transport, instead of
logging and continuing. Keep the behavior localized around the rx_u receiver
loop and the RecvError::Lagged arm, and add a regression test that intentionally
overflows the queue without yielding to verify the pump fails fast rather than
processing a partial stream.
In `@app/src-tauri/src/discord_scanner/mod.rs`:
- Around line 416-419: The pinned page fast-path in
discord_scanner::mod::find_target currently returns a page target by id even
when it no longer matches url_prefix. Update the pinned_target_id branch to also
verify the candidate target is a page under the current Discord URL prefix
before returning it, matching the same prefix constraint already enforced by the
strict/relaxed lookup paths.
- Around line 454-462: The pin update logic in discord_scanner’s strict-match
path is too restrictive because pinned_target_id.is_none() blocks refreshing an
existing stale pin even after a successful strict fragment match. Update the
pinning condition around the strict recovery branch so a new strict match can
replace an outdated pin, and keep the relaxed-match path from pinning; use the
existing is_strict check and the pinned_target_id assignment/logging in the
pinning block to locate the change.
- Around line 381-384: The idle-reconnect path in discord_scanner::mod::scan
leaves the previous CDP target session attached when pump_events_resilient
returns normally, so clean it up before the outer loop reattaches. Mirror the
DOM scan teardown pattern by detaching the current session in the
watchdog/reconnect path, using the existing session_id and cdp handling around
dispatch_event and pump_events_resilient, so stale sessions do not accumulate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81287ff1-8d96-4bc5-8bf2-4d4123cda3fe
📒 Files selected for processing (3)
app/src-tauri/src/cdp/conn.rsapp/src-tauri/src/discord_scanner/mod.rsapp/src-tauri/src/discord_scanner/mod_tests.rs
…stream (tinyhumansai#3693) CodeRabbit review: RecvError::Lagged means frames were already dropped before the drain could pull, so continuing fed a partial event stream downstream with no re-sync. The drain now forwards Result<EventFrame,String> and signals lag as Err, so the pump returns and the outer loop re-attaches (restarts capture) rather than processing a gap. Adds a regression test that overflows the ring without yielding and asserts the error path. Doc updated to match.
…nnect (tinyhumansai#3693) CodeRabbit review, three target-resolution/lifecycle fixes: - Detach the CDP session after the pump returns, so idle/lag-forced reconnects don't accumulate orphaned sessions on the transport. - Constrain the pinned-id branch to url_prefix — a pinned tab can keep its target id while navigating off Discord; don't keep scanning an off-prefix page. - Re-pin on every strict fragment match (not only when unset) so a stale pin recovers after a renderer-swap gives a new target id, instead of being stuck on relaxed matching forever. Adds an off-prefix pin-rejection test; moves the resolution-hierarchy doc onto attach_account_target.
Summary
Makes the Discord webview scanner actually attach and self-heal. Three coupled fixes, all validated against a live Discord login on a CEF staging build:
pin → strict → relaxedresolution so attach survives Discord stripping the URL fragment (without this, the scanner never attaches at all once the page settles).Problem
#3636 moved the Discord scanner onto the in-process
WebviewCdpTransportlong-livedpump_events. Three issues compound there:pump_eventsonly returns onRecvError::Closed(whole transport dropped). When the attached page target dies (Ctrl+R reload, renderer crash, SPA hard-nav) the session goes stale but the pump awaits forever — MITM silently stops until the webview is torn down.Laggedwith no re-sync, and the Discord pump had no idle watchdog at all.replaceStates to its canonical/channelsURL on boot, stripping the#openhuman-account-<id>fragment the webview opens with. Both the MITM attach and the DOM poll matched strictly on that fragment, so after the 4s settle the strict match failed forever — the scanner loopedno matching page targetindefinitely and never attached. Slack hit the identical SPA-strip problem and fixed it long ago (fix(webview/slack): scanner spawn + notifications + post-pushState target match (#1016) #1028); Discord never got the equivalent.Solution
cdp/conn.rs—pump_events_resilient/pump_resilient_core: a drain task forwards frames off the fixed broadcast ring into an unbounded queue (decoupling slow per-frame work from the ring); the consumer reads under atokio::time::timeoutidle watchdog (PUMP_IDLE_TIMEOUT = 90s, > Discord's ~41s gateway heartbeat). On idle it returnsOk(())so the outer loop re-attaches. The drain forwards only own-session frames — empty-session (browser/target-level) chatter is dropped so it can't falsely reset the idle timer on a dead page.discord_scanner/mod.rs: sharedattach_account_target+ pureresolve_page_targetimplementpin → strict → relaxedtarget selection (mirrorsslack_scanner::scan_once), persisting a target-id pin across reconnects. Wired into bothrun_mitm_session(with the resilient pump) anddom_scan_once. Per-accountdata_directoryisolation makes the relaxed prefix-only fallback safe.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/pr-ci.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.md. (Coverage matrix updated)## Related)docs/RELEASE-MANUAL-SMOKE.md. (Manual smoke checklist)Closes #NNNin the## RelatedsectionImpact
Discord MITM + DOM ingest now attach and recover. Validated live (staging, real Discord account):
attached session=…,Network.enable ok, gatewayws-open,dom scan rows=8). Previously loopedno matching page targetforever.event pump idle for 90s, forcing re-attach) →session ended cleanly→ re-attached to a new session. Before the liveness-scoping fix the watchdog never fired (empty-session transport chatter kept resetting the timer for 3+ min).Known limits (honest): the non-lossy drain still logged a minor
drain lagged skipped≈22during the gateway zlib cold-start burst (near-non-lossy under extreme bursts; DOM poll backfills state). A hard-SIGKILL'd renderer leaves a zombie target with no live renderer, so re-attach'sNetwork.enabletimes out (35s) and retries — a test artifact of the kill; a real reload/crash-recovery has a live renderer and completes immediately.Related
Closes #3693
Follow-up to #3636. Relaxed-match pattern mirrors #1028 (Slack).
AI Authored PR Metadata
app/src-tauri/src/cdp/conn.rs,app/src-tauri/src/discord_scanner/mod.rs,app/src-tauri/src/discord_scanner/mod_tests.rs.