fix(client-runtime): busy threads no longer freeze clients - #8309
fix(client-runtime): busy threads no longer freeze clients#8309lnieuwenhuis wants to merge 4 commits into
Conversation
Thread detail and shell subscriptions applied every stream item individually, publishing a full state per event. On long-running threads a turn emits thousands of deltas and activities, so clients (most visibly the mobile JS thread, pingdotgg#8118, pingdotgg#4596) fell behind reprocessing the whole thread per event and taps went dead while native UI stayed responsive. Both subscriptions now run behind Stream.buffer + Stream.chunks: whatever accumulated while the previous batch applied folds through the pure reducer as one run and publishes once. Batches form adaptively, so an idle or keeping-up client still applies items one at a time with no added latency. Thread batches are sliced at 128 items per publication so a huge replay backlog keeps repainting and the apply lock stays available.
Review finding on the batching change: when one batch folds a turn's settle and the next turn's start, the single end-of-run publication ends on a running session and setThread skips the cache offer the old per-item path made at the settle moment. Track the last cache-acceptable folded state during the run and offer it directly when the run ends unsettled.
Three findings from the concurrency review of the batching change: - The unbounded buffer silently removed the transport's end-to-end backpressure (the client acked as fast as the server pushed, moving the un-applied backlog into client heap). A 4096-item suspend buffer keeps adaptive batching while the overflow waits on the server as before. - shell.ts had no lock, so a draining batch's read-modify-write could clobber the new session's HTTP seed after afterSequence was computed from it, losing the shell rows in between. Folds and the seed+cursor read now share a semaphore, mirroring threads.ts. - The threads !supportsPagination reset mutated state outside applyLock; a mid-fold reset would be written right back. It now takes the lock.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e053232. Configure here.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This change substantially alters the production client-runtime synchronization pipeline across RPC, shell, and thread state, introducing buffering, batch reduction, generation-based filtering, and lock-serialized resume behavior. The freeze scenario is clearly targeted and well tested, but the changed scheduling, backpressure, and stale-item semantics carry cross-cutting runtime risk beyond a limited bug fix. You can add or adjust custom eligibility rules. Learn more. |
|
For anyone seeing macroscope didn't approve, it detected a false positive which I don't have the access rights to correct it on: #8309 (comment) |
|
Quick question on this one: are you using the legacy stream token-by-token setting? I have had some pretty brutal, long-running threads for even days at a time and haven't noticed any performance regressions. |
|
No — legacy token streaming makes it much worse (one persisted event per provider chunk), but it isn't required, and I had it off. The numbers in #4596 include a thread sampled from a real database with streaming off: 4,830 events, 4,170 of them |
|
And yes, that write up was written by Fable, after I provided what I thought was up-to-date hardware you (Theo) own, which is an M4 (or similar, modern) Mac. |

Long-running threads make clients unresponsive (#8118, #4596): the thread-detail and shell subscriptions applied every stream item individually — one full reducer pass and one state publication (React commit) per event — so a busy turn's flood of deltas and activities saturated the JS thread, most visibly on mobile where taps went dead while native context menus stayed alive.
Both subscriptions now apply items in adaptive batches. The stream runs through
Stream.buffer+Stream.chunks, so each batch is whatever accumulated while the previous batch applied: a keeping-up client still applies items one at a time with zero added latency, while a backlogged client folds the whole backlog through the pure reducer and publishes once per run. Thread batches are sliced at 128 items per publication so a huge replay keeps repainting, and the buffer is bounded (4096, suspend) so the transport's end-to-end backpressure is preserved — overflow waits on the server exactly as before.Two adjacent races surfaced during review are also handled: a turn that settles mid-batch (with the next turn starting in the same batch) still reaches the thread cache under its own sequence, and shell folds are now serialized with the subscription's HTTP seed (the threads pre-pagination reset runs under the apply lock too) so a draining backlog cannot clobber a freshly seeded snapshot after
afterSequencewas computed from it.In the new regression test, a 400-event backlog folds into 6 publications (was 402). Deterministic tests cover fold ordering (mixed snapshot/stale/live/delete batches), mid-batch settle persistence, and the publication bound for both threads and shell.
This addresses the client half of #8118; the server-side pieces (interrupt latency / a dedicated Stop lane, per-thread ingestion partitioning) are separate concerns and remain open.
Built by Claude Fable 5 via Claude Code.
Note
Medium Risk
Touches the core shell/thread sync pipeline with new batching, buffering, and generation-based staleness rules; behavior is heavily tested but incorrect folding or filtering could cause missed updates or stale UI.
Overview
Shell and thread subscriptions no longer publish one React update per WebSocket event. Both paths now run through a bounded
Stream.buffer+Stream.chunkspipeline and fold consecutive items into fewerSubscriptionRefupdates, so large backlogs (busy turns, replay) don’t saturate the JS thread.RPC subscriptions gain generation tagging via
subscribeDynamicWithGeneration: each emitted value carries session + subscription generation, and appliers ignore buffered items from an old session or resubscribe. Thread batches are also capped at 128 items per slice with yields between slices so the UI can repaint during huge replays and older-page loads can interleave.Concurrency fixes serialize batch folds with HTTP snapshot seeding and resume cursor reads (
applyLock), including shell seed/afterSequenceand thread pagination-reset paths. Thread caching now persists a turn that settles mid-batch even when the batch ends on a running session.Regression tests cover 400-event publication bounds, dropped stale buffers on session replace, mixed snapshot/delete ordering, and mid-batch persistence.
Reviewed by Cursor Bugbot for commit fd3a6cd. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix client freezes under busy threads with adaptive state batching
subscribeDynamicWithGenerationto client.ts, tagging each dynamic subscription attempt with a uniqueDynamicSubscriptionGenerationsymbol and session so downstream consumers can reject stale items.capacity: 4096,strategy: "suspend"), chunk it, and fold consecutive items into a single publication (capped atMAX_STREAM_ITEMS_PER_PUBLISH = 128for threads), cutting publication frequency under heavy event load.applyLockSemaphorein both state factories to serialize HTTP snapshot seeding, fold application, pagination-capability downgrades, and synchronized markers so concurrent operations cannot interleave or lose updates.Macroscope summarized fd3a6cd.