Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (14)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR raises the global event bus capacity to 1024, adds shared ChangesEvent bus resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The global event bus now retains substantially larger bursts for temporarily stalled subscribers while preserving lag reporting and closed-stream behavior. The change is ready to merge with no identified current production risk. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The Provider-backed E2E check is red for a reason unrelated to this change: the provider account cannot access the model the test uses. The job loads profile Every other check passes, including Coverage, both platform build-and-test jobs, the clippy and structural lint gate, formatting, and CodeRabbit. This will fail the same way on any PR until either the account regains access to |
The global event bus singleton was a 16-slot broadcast ring shared by every subscriber in the process. A receiver that stopped polling while 17 or more events were emitted got RecvError::Lagged and its skipped events were evicted for good: a stalled UI task lost ChatEvents, and parallel test runtimes sharing the one ring produced the steering flakes that commit 1cb981b could only work around by serializing the tests that observe the bus. The ring is now sized by the GLOBAL_BUS_CAPACITY constant (1024), so a subscriber may fall 1024 events behind before anything is evicted. A deterministic test in src/events/global.rs pins the arithmetic: a burst larger than the old 16-slot ring but smaller than capacity arrives in full and in order with no lag. The eleven production poll sites already reported lag: each of the ten presenter loops and the view-command bridge in main_gpui.rs matched RecvError::Lagged and warned with its own wording. That reporting is now one function, handle_recv_error in src/events/bus.rs, which warns with the component name and skipped count and returns whether the loop should keep polling. Consolidating it makes the behaviour uniform and denies a future poll site the chance to omit the warning silently; it does not add reporting that was missing. The lag line stays a warning because the skipped events are already gone when it runs.
1 participant
Fixes #234
The global event bus singleton was
EventBus::new(16): one tokio broadcast ring of sixteen slots, shared by every subscriber in the process. A receiver that stopped polling while seventeen or more events were emitted gotRecvError::Lagged, and the events it missed were evicted for good.What changed
The ring is sized by a named constant,
GLOBAL_BUS_CAPACITY = 1024, whose doc comment states the arithmetic rather than asserting a number: a subscriber resuming afterkemitted events receivesLagged(k - capacity), so nothing is lost while a subscriber stays within capacity of the writer.Receive-error handling moved into one function.
handle_recv_errorinsrc/events/bus.rswarns with the component name and skipped count onLaggedand returns whether the loop should keep polling, marked#[must_use]so a caller cannot ignore closure. The ten presenter event loops and the broadcast-to-mpsc view-command bridge inmain_gpui.rsall delegate to it.What this does not claim
The issue as filed said lag was invisible. That was wrong, and I corrected it in a comment on #234 before writing this. All eleven production poll sites already matched
RecvError::Laggedand warned with the skipped count. Consolidating them makes the wording uniform and denies a future poll site the chance to omit the warning silently, but it adds no reporting that was missing. The capacity is the substantive fix here; the helper is a refactor.One consequence worth knowing: every site now emits the helper's wording,
"{component} lagged: {n} events skipped and lost", replacing per-site phrasing such as"{} bridge lagged: {} commands dropped". Log greps written against the old strings need updating.On #231
#231 reports
concurrent_streams::cancel_emits_event_only_for_targetflaking, and attributes it to a drain loop that stops atLagged. That mechanism is already fixed: bothdiscard_buffered_eventsandcount_stream_cancelledcontinue pastLaggedtoday, breaking only onClosedor their deadline.What survives is that continuing past
Laggeddoes not recover the eventsLaggedrepresents. If theStreamCancelledevent under test was among those evicted from the sixteen-slot ring, the count comes back zero and the assertion fails for real. Capacity 1024 removes that.I ran
cargo test --lib concurrent_streamsfifteen times on this branch and all fifteen passed, but that evidence is weak on its own and I am not closing #231 on it: each run was7 passed; 1264 filtered outin under a second, and a filtered run has almost none of the competing bus traffic the flake needs. The argument for #231 is the arithmetic, not the sample. Leaving it open to be confirmed by ordinary CI runs over time.Verification
cargo fmt --all -- --check; the full CI clippy invocation includingcognitive_complexity/too_many_lines/too_many_arguments/type_complexity/struct_excessive_bools;cargo test --lib --tests(1977 passed, 0 failed, up from 1975 on main);cargo xtask guard;lizard -C 50 -L 100 -w src/; the 1000-line file gate. No lint suppressions, no new dependencies, no existing test weakened.The capacity test was written first against the old value of 16, observed failing, then went green at 1024. It builds a local
EventBusrather than the global singleton, so it cannot itself be perturbed by other tests sharing the process ring, and it asserts the burst arrives in order, not merely that it arrives.The
STEERING_BUS_LOCKtest serialization from #222 is deliberately untouched. It can likely be removed now that the ring is not the constraint, but that is a separate change with its own verification burden.Summary by CodeRabbit
Bug Fixes
Tests