Coalesce client loader/store rebuilds during incremental event bursts#5604
Open
habdelra wants to merge 3 commits into
Open
Conversation
Every incremental index event touching an already-loaded executable module tore down and rebuilt the whole client — resetLoader + store.reset + re-establishing every live card reference — once per event. A write burst therefore cost N full rebuilds and N full-graph re-fetches, arriving at or above the rate they complete. Move the rebuild into a keepLatest task so at most one runs in flight and one stays pending: intermediate events collapse into the pending slot, so N rapid executable invalidations cost at most 2 rebuilds. The final rebuild re-fetches current server state, so the end state still reflects the latest generation. Reactivity is unchanged — an isolated change still resets and re-renders exactly as before, and the full reset stays load-bearing. Once a rebuild is in flight, further executable invalidations fold into it regardless of the isModuleLoaded probe, which reads false against the freshly reset loader and would otherwise drop a mid-burst code change. The per-invalidation reload loop is skipped when a rebuild is scheduled, since the rebuild subsumes it (as the synchronous store.reset already did). The rebuild telemetry now fires once per actual rebuild and carries a coalesced_events count. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Preview deploymentsHost Test Results 1 files 1 suites 4m 52s ⏱️ Results for commit 32f16f2. For more details on these errors, see this check. Realm Server Test Results 1 files 1 suites 10m 32s ⏱️ Results for commit 32f16f2. |
`can skip waiting for the save when patching an instance` waited on the
fire-and-forget save with a bare `waitUntil(() => didSave)`, i.e. the 1s
default. That save completes a write + incremental-index round-trip before
`onSave` fires — measured at ~1.8s under load — so the assertion timed out
even though the save always succeeds. Its sibling `can skip waiting for the
save when adding to the store` already waits with `{ timeout: 10000 }`;
match it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces redundant host-side client rebuild work during bursts of incremental index events that invalidate already-loaded executable modules. It coalesces those rebuilds so rapid sequences of invalidations don’t repeatedly tear down and reestablish the entire live card graph, while also adding telemetry to quantify the coalescing and tests to pin the new scheduling behavior.
Changes:
- Coalesce executable-invalidation-triggered rebuilds in
StoreServiceusing akeepLatestTask, and skip the per-invalidation reload loop when a full rebuild is scheduled. - Extend rebuild telemetry to include
coalesced_eventsand adjust rebuild reporting to occur once per actual rebuild. - Add integration tests covering burst coalescing (≤2 rebuilds) and the isolated invalidation case, plus a timeout tweak for an existing save-timing test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/host/app/services/store.ts | Coalesces rebuild scheduling for executable invalidations; extracts non-rebuild reload loop; emits rebuild telemetry per rebuild with coalescing metadata. |
| packages/host/app/services/client-telemetry.ts | Adds coalesced_events to the RebuildEvent telemetry schema. |
| packages/host/tests/integration/store-test.gts | Adds integration tests for burst coalescing and isolated rebuild behavior; increases waitUntil timeout for a persist timing test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The rebuild-count spy restored `loaderService.resetLoader` to a bound copy of the method, leaving the service with a different method identity than it started with. Capture the original reference, invoke it via `.call` for the correct `this`, and restore that exact reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every incremental index event whose invalidations include an already-loaded executable module triggers a full client rebuild in the host:
resetLoader()+store.reset()+ re-establishing every live card reference. The full reset is load-bearing and stays — targeted loader resets are unsafe against nested instance graphs, live Glimmer bindings, and@fieldclosures over card-api. What was missing is scheduling. With no coalescing, N events in a burst cost N complete teardowns and N full re-fetch/re-render passes of the whole open card graph.That is exactly the shape a rapid write session produces: sustained writes every few seconds. A rebuild of a large open card can take several seconds, so during a burst the rebuild work arrives at or above the rate it completes, and the open card stays pinned in "Loading card…" for the length of the burst.
What this changes
The rebuild moves into a
keepLatesttask. At most one rebuild runs in flight and one stays pending; events arriving mid-rebuild collapse into the single pending slot. A burst of N rapid executable invalidations therefore costs at most 2 rebuilds regardless of length. The final rebuild re-fetches current server state, so the end state reflects the latest generation — this is scheduling only, no reactivity semantics change, and an isolated change still resets and re-renders as a single rebuild.Two details worth calling out:
isModuleLoadedprobe alone would otherwise drop a mid-burst code change.store.reset+reestablishReferencessubsumes it (the synchronousstore.resetalready emptied the store before that loop ran, leaving it a no-op there). It is extracted into#reloadInvalidatedInstancesfor the non-rebuild path.The rebuild telemetry now fires once per actual rebuild and carries a
coalesced_eventscount, so the dashboard reflects how many events collapsed into each rebuild.Scope
The local editor file-write rebuild path (
refreshReferencesForCodeChange) is a single-write path, not a burst, and is left unchanged.Test plan
New integration tests in
store-test:The full
Integration | Storemodule is otherwise green locally. One save-timing test (can skip waiting for the save when patching an instance) fails on a slow local stack because itswaitUntiloutlasts the save round-trip; it fails identically on the base branch, so it is independent of this change.🤖 Generated with Claude Code