Skip to content

fix(web): prevent browser crash from timer leaks, DOM growth, SSE buffer - #2433

Closed
henrypark133 wants to merge 2 commits into
stagingfrom
fix/2406-web-ui-crash
Closed

henrypark133 wants to merge 2 commits into
stagingfrom
fix/2406-web-ui-crash

Conversation

@henrypark133

Copy link
Copy Markdown
Collaborator

Summary

Fixes #2406 — the web UI became unresponsive and triggered Chrome's "Pages Unresponsive" dialog during extended bug bash sessions with heavy bot interactions and parallel chats.

Root cause: The browser accumulated resources that were never cleaned up — leaked setInterval timers across SSE reconnects, unbounded DOM node growth, unbounded in-memory job event storage, and a too-small SSE broadcast buffer causing reconnect cascades.

Changes:

  • Timer cleanup (cleanupConnectionState()): Clears _streamDebounceTimer, _streamBuffer, _connectionLostTimer, and jobListRefreshTimer on SSE reconnect, tab-hide, and page unload. Intentionally excludes _doneWithoutResponseTimer which is a turn-level concern (fix: web UI messages stuck until refresh — SSE event ordering bug #2079).
  • DOM pruning (pruneOldMessages()): Caps #chat-messages at 200 elements. Streaming-aware — skips data-streaming="true" elements. Called at turn boundaries (after response event) and after loadHistory(), not per-addMessage() to avoid O(N²) during bulk loads.
  • Job events cap: Limits jobEvents Map to 50 entries with LRU eviction by last event timestamp. Excludes the current job from the eviction scan to prevent self-eviction.
  • SSE broadcast buffer: Increased from 256 to 1024 events (configurable via SSE_BROADCAST_BUFFER env var). Includes .filter(|&n| n > 0) guard to prevent broadcast::channel(0) panic from misconfigured env.

Test plan

  • cargo fmt — clean
  • cargo clippy --all --all-features — zero warnings
  • cargo test --lib — passes (pre-existing file_history SIGABRT unrelated)
  • 3 new Playwright E2E tests pass (test_dom_resource_limits.py):
    • test_dom_pruned_after_many_messages — injects 250 messages, asserts DOM <= 200
    • test_no_timer_leak_across_reconnects — 5 reconnect cycles, asserts no interval leak
    • test_prune_preserves_streaming_message — streaming element survives pruning
  • Manual browser testing on staging: extended session with heavy tool use, tab switching, parallel chats

🤖 Generated with Claude Code

…fer (#2406)

Extended sessions with heavy bot interactions caused Chrome's "Pages
Unresponsive" dialog due to accumulated browser resources that were
never cleaned up.

Fixes:
- Add cleanupConnectionState() to clear leaked setInterval/setTimeout
  timers on SSE reconnect, tab visibility change, and page unload
- Cap DOM at 200 message nodes via pruneOldMessages() with streaming-
  aware pruning (skips data-streaming elements, called at turn
  boundaries and after loadHistory)
- Cap jobEvents Map at 50 entries with LRU eviction (excludes current
  job from eviction scan)
- Increase SSE broadcast buffer from 256 to 1024 (configurable via
  SSE_BROADCAST_BUFFER env var, with zero-guard to prevent panic)

Closes #2406

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 13, 2026 22:37
@github-actions github-actions Bot added scope: channel/web Web gateway channel size: M 50-199 changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels Apr 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Addresses long-running web UI instability (Chrome “Pages Unresponsive”) by bounding frontend resource growth (timers, DOM nodes, in-memory job events) and reducing SSE reconnect cascades via a larger broadcast buffer.

Changes:

  • Add connection-level cleanup for SSE reconnect/tab-hide/unload and prune the chat DOM to a fixed max.
  • Cap tracked job event history to bound in-memory growth.
  • Increase backend SSE broadcast buffer (env-configurable) and add Playwright E2E coverage for resource limits.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
crates/ironclaw_gateway/static/app.js Adds connection cleanup, DOM pruning, and jobEvents map eviction logic to prevent long-session resource leaks.
src/channels/web/sse.rs Makes SSE broadcast buffer size larger and configurable via SSE_BROADCAST_BUFFER.
tests/e2e/scenarios/test_dom_resource_limits.py Adds E2E tests targeting DOM pruning and timer leak regression scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ironclaw_gateway/static/app.js
Comment thread tests/e2e/scenarios/test_dom_resource_limits.py
Comment thread src/channels/web/sse.rs

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements several resource management improvements to prevent memory leaks and DOM bloat in the web UI, alongside server-side SSE buffer optimizations. Key additions include a connection state cleanup routine, a mechanism to prune old chat messages from the DOM while preserving active streams, and a cap on tracked job events. Feedback suggests that the current placement of the DOM pruning logic may interfere with message pagination and that the job eviction logic should be updated to protect the job currently being viewed by the user.

Comment thread crates/ironclaw_gateway/static/app.js Outdated
Comment thread crates/ironclaw_gateway/static/app.js Outdated
- Reset _connectionLostAt in cleanupConnectionState() to prevent stale
  "disconnected >10s" timestamp across reconnects (Copilot)
- Clamp SSE_BROADCAST_BUFFER to MAX_BROADCAST_BUFFER (65536) to prevent
  OOM from misconfigured env var (Copilot)
- Only prune DOM on fresh history loads, not pagination — prevents
  removing messages the user just scrolled to (Gemini)
- Protect currentJobId from job eviction so the viewed job's activity
  stream stays live (Gemini)
- Trigger synthetic stream_chunk in timer leak test so it exercises the
  actual _streamDebounceTimer code path (Copilot)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Prevent browser crash from timer leaks, DOM growth, SSE buffer (Risk: Medium)

Good defensive fixes for a real browser crash — timer cleanup, DOM pruning, job events LRU eviction, and SSE buffer increase. A few items to address.

Concerning: SSE_BROADCAST_BUFFER env var read in wrong module

File: src/channels/web/sse.rs
CLAUDE.md convention: all gateway env vars flow through src/config/channels.rs via GatewayConfig. This PR reads std::env::var("SSE_BROADCAST_BUFFER") directly inside sse.rs, bypassing config resolution, .env.example documentation, and structured config dumps. Move to GatewayConfig as broadcast_buffer: usize, resolved via parse_optional_env, then passed to SseManager::with_max_connections_and_buffer().

Concerning: Verify MAX_BROADCAST_BUFFER clamp exists

File: src/channels/web/sse.rs
Patch 2 claims to add const MAX_BROADCAST_BUFFER: usize = 65_536 and .min(MAX_BROADCAST_BUFFER). Without this clamp, SSE_BROADCAST_BUFFER=18446744073709551615 causes OOM on startup. Verify the clamp is present in the final commit — one reviewer found it missing from the worktree.

Concerning: Verify Patch 2 fixes landed

File: crates/ironclaw_gateway/static/app.js
Patch 2 claims to add _connectionLostAt = null to cleanupConnectionState() and protect currentJobId in LRU eviction (if (k === jobId || k === currentJobId) continue). One reviewer found discrepancies between the diff and the actual file state. Confirm both fixes are present in the final commit.

Concerning: pruneOldMessages during pagination breaks scroll

File: crates/ironclaw_gateway/static/app.js
The pagination path saves/restores scrollHeight, then pruneOldMessages() invalidates the restore. Patch 2 adds if (!isPaginating) pruneOldMessages() — confirm this guard is applied.

Minor: Timer leak test methodology

File: tests/e2e/scenarios/test_dom_resource_limits.py
The setInterval monkey-patch is installed after initApp() has already run, so timers created during initialization (e.g., gatewayStatusInterval from startGatewayStatusPolling()) are invisible to the counter. Consider using page.add_init_script() to install the patch before app JS runs.

Minor: pruneOldMessages counts heterogeneous elements

The selector .message, .activity-group, .time-separator counts all three toward the 200-item cap. A conversation heavy on activity-groups could cause aggressive message pruning. Consider counting .message nodes only for the budget.

Minor: No Rust-side tests for SSE buffer config

No unit test for the buffer size configuration, the clamp, or slow-consumer behavior. Consider a test asserting the env var is parsed and clamped correctly.

Convention notes:

  • E2E test file correctly placed in tests/e2e/scenarios/
  • Should be added to tests/e2e/CLAUDE.md scenario table
  • debug! logging used correctly throughout

🤖 Generated with Claude Code

henrypark133 added a commit that referenced this pull request Apr 14, 2026
… fix E2E timer test

Move SSE_BROADCAST_BUFFER env var from direct std::env::var() in sse.rs
to GatewayConfig in config/channels.rs, following the convention that all
gateway env vars flow through structured config. Add MAX_BROADCAST_BUFFER
(65,536) clamp to prevent OOM from misconfiguration.

Fix E2E timer leak test to install setInterval monkey-patch via
page.add_init_script() before navigation so initialization timers are
tracked. Add test_dom_resource_limits.py to E2E CLAUDE.md scenario table.

Add unit test for buffer config parsing, zero-rejection, and clamp.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@henrypark133

Copy link
Copy Markdown
Collaborator Author

Review follow-up (faa0f60)

All concerns from the review have been addressed:

# Concern Status Commit
1 SSE_BROADCAST_BUFFER env var read in wrong module Fixed — moved to GatewayConfig in src/config/channels.rs via parse_optional_env faa0f60
2 MAX_BROADCAST_BUFFER clamp missing Fixed — added MAX_BROADCAST_BUFFER = 65_536 with .min() clamp and zero-rejection faa0f60
3 Timer leak test monkey-patches after initApp() Fixed — now uses page.add_init_script() before navigation faa0f60
4 Missing from E2E CLAUDE.md scenario table Fixed — added entry faa0f60
5 No Rust-side tests for SSE buffer config Fixed — added broadcast_buffer_defaults_and_clamps test faa0f60
6 _connectionLostAt = null missing in cleanupConnectionState Already fixed 236948a
7 currentJobId not protected in LRU eviction Already fixed 236948a
8 pruneOldMessages during pagination breaks scroll Already fixed 236948a
9 pruneOldMessages counts heterogeneous elements Accepted as-is — simpler, edge case risk is low —

henrypark133 added a commit that referenced this pull request Apr 14, 2026
… fix E2E timer test

Move SSE_BROADCAST_BUFFER env var from direct std::env::var() in sse.rs
to GatewayConfig in config/channels.rs, following the convention that all
gateway env vars flow through structured config. Add MAX_BROADCAST_BUFFER
(65,536) clamp to prevent OOM from misconfiguration.

Fix E2E timer leak test to install setInterval monkey-patch via
page.add_init_script() before navigation so initialization timers are
tracked. Add test_dom_resource_limits.py to E2E CLAUDE.md scenario table.

Add unit test for buffer config parsing, zero-rejection, and clamp.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
henrypark133 added a commit that referenced this pull request Apr 14, 2026
…fer (#2406) (#2441)

* fix(web): prevent browser crash from timer leaks, DOM growth, SSE buffer (#2406)

Extended sessions with heavy bot interactions caused Chrome's "Pages
Unresponsive" dialog due to accumulated browser resources that were
never cleaned up.

Fixes:
- Add cleanupConnectionState() to clear leaked setInterval/setTimeout
  timers on SSE reconnect, tab visibility change, and page unload
- Cap DOM at 200 message nodes via pruneOldMessages() with streaming-
  aware pruning (skips data-streaming elements, called at turn
  boundaries and after loadHistory)
- Cap jobEvents Map at 50 entries with LRU eviction (excludes current
  job from eviction scan)
- Increase SSE broadcast buffer from 256 to 1024 (configurable via
  SSE_BROADCAST_BUFFER env var, with zero-guard to prevent panic)

Closes #2406

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address PR #2433 review — move SSE buffer to GatewayConfig, fix E2E timer test

Move SSE_BROADCAST_BUFFER env var from direct std::env::var() in sse.rs
to GatewayConfig in config/channels.rs, following the convention that all
gateway env vars flow through structured config. Add MAX_BROADCAST_BUFFER
(65,536) clamp to prevent OOM from misconfiguration.

Fix E2E timer leak test to install setInterval monkey-patch via
page.add_init_script() before navigation so initialization timers are
tracked. Add test_dom_resource_limits.py to E2E CLAUDE.md scenario table.

Add unit test for buffer config parsing, zero-rejection, and clamp.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address review — clean gatewayStatusInterval, prune user msgs, use constants

- Add gatewayStatusInterval to cleanupConnectionState() so it is cleared
  on reconnect/tab-hide/unload; add guard in startGatewayStatusPolling()
  to prevent double-start; restart polling on tab visibility restore
- Call pruneOldMessages() after addMessage('user', ...) in sendMessage()
  so DOM stays bounded even during rapid user input
- Replace hardcoded broadcast_buffer: 1024 with DEFAULT_BROADCAST_BUFFER
  in all test construction sites (5 occurrences across 4 files)
- Document in from_sender() doc comment why broadcast_buffer is absent
- Tighten E2E timer leak assertion from baseline+1 to baseline

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address PR #2441 review — prune/timer/assert/doc fixes

- Remove pruneOldMessages() from loadHistory() pagination path to avoid
  immediately evicting just-prepended older messages
- Move MAX_DOM_MESSAGES constant to top-level constants block
- Add _loadThreadsTimer to cleanupConnectionState() for consistency
- Add assert!(broadcast_buffer > 0) to SseManager constructor with
  panic doc (tokio broadcast channel requires capacity > 0)
- Use Set-based interval tracking in E2E test to prevent counter
  underflow from double-clear
- Update CLAUDE.md broadcast buffer docs (256 → 1024, SSE_BROADCAST_BUFFER)

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): remove assert! from SseManager to pass no-panics CI check

Replace assert!(broadcast_buffer > 0) with a doc comment noting the
precondition. GatewayConfig already rejects 0 at the config layer.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address ilblackdragon review — correctness, e2e tests, docs (#2406)

Correctness:
- pruneOldMessages: clean up orphaned leading time-separators after pruning
- jobEvents LRU: replace O(n) scan with O(1) Map insertion-order eviction
- Document degenerate all-streaming under-prune case

Playwright e2e tests:
- Tab hide/restore: no duplicate gateway status polling intervals
- DOM cap + streaming: 260 elements prune to ≤200, streaming preserved, no orphan separators
- jobEvents bounded: 60 jobs stay capped at ≤50 via LRU eviction
- Fix assertion selector to match pruneOldMessages superset, tighten lower bound

Rust:
- Unit test: SseManager buffer size parameter actually controls lag behavior
- Document MAX_BROADCAST_BUFFER memory impact (65K×100×200B ≈ 1.3 GB)
- Move "capacity baked into tx" comment from from_sender to rebuild_state

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): protect currentJobId from LRU eviction in jobEvents map (#2441)

The O(1) LRU eviction skips the job that just received an event (moved
to end via delete+set), but did not protect the job the user is actively
viewing in the detail panel (currentJobId). If the user views a quiet
job while 50+ other jobs fire events, the viewed job's events would be
evicted and the activity tab would appear empty.

Add a currentJobId guard to the eviction loop and a Playwright e2e test
that verifies the actively-viewed job survives LRU pressure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add real-flow Playwright tests for DOM resource limits (#2406)

Add 4 E2E tests that exercise pruning and timer cleanup through actual
UI interactions (mock LLM round-trips, real SSE reconnects) instead of
page.evaluate() injection. Also fix the existing timer leak test which
failed due to execution context destruction from add_init_script.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: serrrfirat <f@nuff.tech>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
…fer (nearai#2406) (nearai#2441)

* fix(web): prevent browser crash from timer leaks, DOM growth, SSE buffer (nearai#2406)

Extended sessions with heavy bot interactions caused Chrome's "Pages
Unresponsive" dialog due to accumulated browser resources that were
never cleaned up.

Fixes:
- Add cleanupConnectionState() to clear leaked setInterval/setTimeout
  timers on SSE reconnect, tab visibility change, and page unload
- Cap DOM at 200 message nodes via pruneOldMessages() with streaming-
  aware pruning (skips data-streaming elements, called at turn
  boundaries and after loadHistory)
- Cap jobEvents Map at 50 entries with LRU eviction (excludes current
  job from eviction scan)
- Increase SSE broadcast buffer from 256 to 1024 (configurable via
  SSE_BROADCAST_BUFFER env var, with zero-guard to prevent panic)

Closes nearai#2406

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address PR nearai#2433 review — move SSE buffer to GatewayConfig, fix E2E timer test

Move SSE_BROADCAST_BUFFER env var from direct std::env::var() in sse.rs
to GatewayConfig in config/channels.rs, following the convention that all
gateway env vars flow through structured config. Add MAX_BROADCAST_BUFFER
(65,536) clamp to prevent OOM from misconfiguration.

Fix E2E timer leak test to install setInterval monkey-patch via
page.add_init_script() before navigation so initialization timers are
tracked. Add test_dom_resource_limits.py to E2E CLAUDE.md scenario table.

Add unit test for buffer config parsing, zero-rejection, and clamp.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address review — clean gatewayStatusInterval, prune user msgs, use constants

- Add gatewayStatusInterval to cleanupConnectionState() so it is cleared
  on reconnect/tab-hide/unload; add guard in startGatewayStatusPolling()
  to prevent double-start; restart polling on tab visibility restore
- Call pruneOldMessages() after addMessage('user', ...) in sendMessage()
  so DOM stays bounded even during rapid user input
- Replace hardcoded broadcast_buffer: 1024 with DEFAULT_BROADCAST_BUFFER
  in all test construction sites (5 occurrences across 4 files)
- Document in from_sender() doc comment why broadcast_buffer is absent
- Tighten E2E timer leak assertion from baseline+1 to baseline

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address PR nearai#2441 review — prune/timer/assert/doc fixes

- Remove pruneOldMessages() from loadHistory() pagination path to avoid
  immediately evicting just-prepended older messages
- Move MAX_DOM_MESSAGES constant to top-level constants block
- Add _loadThreadsTimer to cleanupConnectionState() for consistency
- Add assert!(broadcast_buffer > 0) to SseManager constructor with
  panic doc (tokio broadcast channel requires capacity > 0)
- Use Set-based interval tracking in E2E test to prevent counter
  underflow from double-clear
- Update CLAUDE.md broadcast buffer docs (256 → 1024, SSE_BROADCAST_BUFFER)

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): remove assert! from SseManager to pass no-panics CI check

Replace assert!(broadcast_buffer > 0) with a doc comment noting the
precondition. GatewayConfig already rejects 0 at the config layer.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address ilblackdragon review — correctness, e2e tests, docs (nearai#2406)

Correctness:
- pruneOldMessages: clean up orphaned leading time-separators after pruning
- jobEvents LRU: replace O(n) scan with O(1) Map insertion-order eviction
- Document degenerate all-streaming under-prune case

Playwright e2e tests:
- Tab hide/restore: no duplicate gateway status polling intervals
- DOM cap + streaming: 260 elements prune to ≤200, streaming preserved, no orphan separators
- jobEvents bounded: 60 jobs stay capped at ≤50 via LRU eviction
- Fix assertion selector to match pruneOldMessages superset, tighten lower bound

Rust:
- Unit test: SseManager buffer size parameter actually controls lag behavior
- Document MAX_BROADCAST_BUFFER memory impact (65K×100×200B ≈ 1.3 GB)
- Move "capacity baked into tx" comment from from_sender to rebuild_state

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): protect currentJobId from LRU eviction in jobEvents map (nearai#2441)

The O(1) LRU eviction skips the job that just received an event (moved
to end via delete+set), but did not protect the job the user is actively
viewing in the detail panel (currentJobId). If the user views a quiet
job while 50+ other jobs fire events, the viewed job's events would be
evicted and the activity tab would appear empty.

Add a currentJobId guard to the eviction loop and a Playwright e2e test
that verifies the actively-viewed job survives LRU pressure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add real-flow Playwright tests for DOM resource limits (nearai#2406)

Add 4 E2E tests that exercise pruning and timer cleanup through actual
UI interactions (mock LLM round-trips, real SSE reconnects) instead of
page.evaluate() injection. Also fix the existing timer leak test which
failed due to execution context destruction from add_init_script.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: serrrfirat <f@nuff.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: medium Business logic, config, or moderate-risk modules scope: channel/web Web gateway channel size: M 50-199 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[QA] Pages Unresponsive dialog and black screen crashes

2 participants