CODAP-1408: Coalesce streamed create-item requests into batched model changes#2631
CODAP-1408: Coalesce streamed create-item requests into batched model changes#2631kswenson wants to merge 17 commits into
Conversation
…to remove before PR) Restores the request-latency logging ([REQ-DBG]) and prf.measure blocks from 292809a (stripped from the CODAP-1404 branch in 9111557), plus the syncMs/asyncMs split in the REQ-DBG log used for the graph-open/closed bisection. Establishes the measurement baseline for coalescing streamed create-item requests. Not for merge — strip before the PR is ready. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… changes
Plugin requests arrive one per postMessage event and were drained synchronously,
so each streamed create triggered its own model change and full tile-reactivity
cascade — O(N) per add, O(N²) per stream — tripping the plugin's ~2s request
timer at scale (a 200-case Simmer stream with an open graph timed out from
n≈118). The request queue processor now defers its drain by a macrotask so
already-delivered requests accumulate, then processes ONE work unit per drain
tick: a run of consecutive single-item creates (capped at 10) executes as one
batched create — one applyModelChange, one addCases, one notification round —
while the browser paints between ticks, preserving the visible sense of
streaming. Measured: 200 streamed creates ≈ 60ms total processing (was ~2,900ms
and climbing), zero plugin timeouts.
- request-coalescer.ts: takeNextWorkUnit() identifies the coalescable run (or
single request) at the head of the queue, up to a max run length
- item-handler.ts: createItemsInSegments() executes a multi-segment batch and
returns per-segment results (positional itemIDs; new cases attributed to the
segment of their earliest contributing item, matching sequential semantics);
handler create() is the single-segment case
- request processor: deferred one-unit-per-tick drain with fallback to
individual processing if a batched create cannot proceed; tableModified is
now checked after processing completes (previously it was checked before the
async processors could set it, so interruption counting never fired here)
- request-queue.ts: adds items getter and takeItems(); processItems() removed;
RequestPair exported
- DEBUG_NO_COALESCE ("noCoalesce" debug flag) processes requests individually
for A/B comparison
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the fixed batch cap of 10 with adaptive sizing driven by two signals: - Backlog-proportional: each drain tick takes up to 1/10 of the queued requests (floor of 5), so small backlogs stream in fine-grained batches while large ones grow the batch to keep up. Machine speed is adapted to implicitly: slower ticks let more requests accumulate, growing the batch. - Queue-wait urgency: as the head request's wait approaches 1s, the batch ramps up to the entire backlog — timely acknowledgment (plugin request timers are conventionally ~2s) outranks streaming granularity. Since a flood's tail requests have been waiting since the flood began, this also accelerates the tail of a large drain instead of dribbling it out at the minimum batch size (500-case streams into a large dataset timed out with backlog-proportional sizing alone). Promotes the request queue's enqueuedAt timestamp from temporary diagnostic to a real field consumed by the processor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CODAP-1403 (#2628) suppressed point animation on every addCases to fix the streaming animation freeze, but as review noted, animation is desirable for individual case additions — user-entered rows, plugin creates at user-visible rates. Whether the request processor coalesced multiple create requests into one batch is exactly the "high-speed stream" discriminator, so suppression is now opt-in via a new addCases suppressAnimation option that the item handler sets for multi-segment (coalesced) batches only. A single create request — even one carrying many items — animates as an ordinary bulk add. removeCases suppression is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Strip the temporary request-latency logging ([REQ-DBG]) and prf.measure blocks re-added under 4042f0d to measure the coalescing work. The request queue's enqueuedAt timestamp remains — it now drives queue-wait-aware batch sizing. Also drops an unnecessary optional call on the handler function in processAction, which is guarded above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## CODAP-1403-streaming-points-animation #2631 +/- ##
=========================================================================
+ Coverage 87.33% 87.37% +0.04%
=========================================================================
Files 798 799 +1
Lines 45468 45577 +109
Branches 11523 11545 +22
=========================================================================
+ Hits 39708 39825 +117
+ Misses 5747 5741 -6
+ Partials 13 11 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
codap-v3
|
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
CODAP-1408-coalesce-streamed-creates
|
| Run status |
|
| Run duration | 08m 51s |
| Commit |
|
| Committer | null |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
1
|
|
|
82
|
|
|
0
|
|
|
380
|
| View all changes introduced in this branch ↗︎ | |
Adds doc/plugin-request-processing.md describing the request pipeline (queueing, deferred drain, coalescing, adaptive batch sizing, response slicing), plugin-visible semantics, the plugin traffic patterns that shaped the design, and known limits/future extensions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes CODAP v3’s data-interactive/plugin request pipeline to prevent O(N²) slowdown and timeouts when plugins stream many single-item create … item requests by batching/coalescing them into fewer model changes while preserving sequential semantics.
Changes:
- Defer request-queue draining by a macrotask and process one “work unit” per tick, enabling consecutive create-item requests to coalesce into batched creates with adaptive batch sizing.
- Add
suppressAnimationplumbing so high-speed coalesced adds snap (no point animation restart), while ordinary adds continue to animate. - Add targeted unit tests and documentation describing the new queue/coalescing behavior and semantics.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/src/models/data/data-set-types.ts | Adds suppressAnimation option to addCases options for downstream consumers (graph/data-display). |
| v3/src/lib/embedded-mode/embedded-server.ts | Updates disposer typing to match new request-processor return type. |
| v3/src/lib/debug.ts | Adds DEBUG_NO_COALESCE debug flag for A/B comparison. |
| v3/src/data-interactive/request-coalescer.ts | Introduces logic to form “single” vs “coalesced” work units from queued requests. |
| v3/src/data-interactive/request-coalescer.test.ts | Adds unit tests for coalescing rules and max-run-length behavior. |
| v3/src/data-interactive/handlers/item-handler.ts | Adds createItemsInSegments() to execute coalesced creates as one model change and slice per-request results. |
| v3/src/data-interactive/handlers/item-handler.test.ts | Adds tests for segment slicing, case attribution semantics, and animation suppression behavior. |
| v3/src/data-interactive/data-interactive-request-processor.ts | Reworks queue draining to be deferred/serial and to execute coalesced create runs via createItemsInSegments(). |
| v3/src/data-interactive/data-interactive-request-processor.test.ts | Adds tests for deferred draining, batching size behavior, ordering, and fallback-to-individual processing. |
| v3/src/components/web-view/request-queue.ts | Adds enqueue timestamps and replaces processItems() with items + takeItems() to support the new drain model. |
| v3/src/components/graph/models/graph-data-configuration-model.test.ts | Updates expectations: suppress-on-add is now opt-in via suppressAnimation, remove always suppresses. |
| v3/src/components/data-display/models/data-configuration-model.ts | Applies suppression on remove always, and on add only when requested via addCases option. |
| v3/doc/plugin-request-processing.md | New internal documentation for the end-to-end request queuing/coalescing pipeline and semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…iew) - A throwing handler no longer drops the request: processSingleRequest catches the exception and responds with an errorResult, so the plugin gets an error instead of waiting out its request timer (new V3.DI.Error.exceptionProcessingRequest string). - Response callbacks are invoked through a guard so one throwing consumer can't prevent later responses (e.g. the remaining members of a coalesced batch) from being delivered. - New-case attribution now uses a precomputed itemIndex→segmentIndex lookup instead of a per-case linear scan of the segment ranges, avoiding O(segments × cases) behavior in large coalesced batches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Copilot review) Captures the dataset's name after creation and uses it in the test request resource strings, so the tests stay correct if createDataSet ever renames the dataset for uniqueness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
createItemsInSegments unwrapped the Collaborative `{ values: {...} }`
shape via `typeof item.values === "object"`, which also matched an item
whose own `values` attribute held an object (dropping its other fields)
or null (`typeof null === "object"` -> crash on the __id__ access). Only
unwrap a non-null, non-array object so a malformed or "values"-named
attribute can't corrupt the item or throw.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Explain why the request-queue processor needs no "disposed" flag: at most one drain timer is ever pending (always held in drainTimer), takeItems re-fires the reaction to schedule the next timer before the await, and the disposer clears that one timer -- so a mid-await drain can't leak a follow-up timer. Comment only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
createItemsInSegments identifies new cases by an identity-based before/ after diff, which correctly excludes pre-existing parent/middle cases that a streamed item merely joins. Add a hierarchical case guarding that invariant: one segment joining an existing (a1,a2) group reports only its new leaf case, while a segment introducing a fresh hierarchy reports its new parent, middle, and leaf cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collapse the three parallel structures used to attribute new cases back to their originating request -- segmentIndexByItemIndex, itemIndexMap, and segmentRanges -- down to segmentRanges plus a single id->segment map. Since items are appended in segment order, a new case's earliest contributing item is simply the one in its lowest-numbered segment, so the reduce finds the min segment directly rather than min item-index then an index->segment lookup. Add a characterization test covering attribution to a non-zero earliest segment (segment 2 joins a parent created by segment 1): the prior tests all had segment 0 as the earliest contributor, so this guards the exact behavior the simplification touches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new-case attribution snippet wrote caseInfoMap.get(caseId).childItemIds, but the code uses optional chaining (caseInfoMap.get(caseId)?.childItemIds ?? []) to handle the no-contributing-item edge case the same paragraph describes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bfinzer
left a comment
There was a problem hiding this comment.
👍🏻LGTM
Wow! Quite a subtle and important piece of work. Claude did not come up with any blockers.
processCoalescedRun fell back to processing members individually whenever the batched createItemsInSegments did not cleanly succeed. Because applyModelChange is not transactional on throw, a failure after addCases leaves cases committed, so the fallback re-added them — silent data duplication. Only fall back when the dataContext was unresolvable (nothing mutated); otherwise respond with an error to each member. Also make the per-member debug log lazy instead of eagerly stringifying on the hot path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… path createItemsInSegments built an itemId->segment Map and ran a per-case childItemIds.reduce even for an ordinary (non-coalesced) create, where every new case trivially belongs to segment 0. Guard the attribution machinery behind segments.length > 1 and take a direct segment-0 path otherwise. Behavior-preserving: a single segment always attributed every case to segment 0 anyway. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code review — remaining items (non-blocking)A high-effort review (8 finder angles, independent verification) produced the fixes already landed on this branch (see the PR description's Post-review hardening section). The items below are deliberate-design tradeoffs and judgment calls, not bugs — recording them here so reviewers can weigh in and so the risk surface is documented. Behavior changes worth a conscious call
Tradeoffs already noted in the PR / docs
Suggested manual test plan before merge
|
…g, doc) - processSingleRequest debug log no longer eagerly JSON.stringifies the request when DEBUG_PLUGINS is off (matches the coalesced path). - onProcessingStart fires once before the batched create rather than per-member after it, so it accurately signals when processing starts. - Update plugin-request-processing.md "Failure handling" to match the implemented semantics: only an unresolvable dataContext falls back to per-request processing; a post-resolve failure responds with per-request errors without retrying (avoids re-adding cases already committed by the non-transactional applyModelChange). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The single-timer invariant alone did not make teardown safe. When takeItems() empties the queue, the reaction does not re-arm a timer before drain()'s await, so drainTimer is null at that point. If the processor is disposed mid-await and a request then arrives (e.g. an in-flight postMessage before the endpoint disconnects), the trailing scheduleDrain() in drain()'s finally would arm a new timer and process that request after disposal. Add an explicit `disposed` flag, checked in scheduleDrain() and at the top of drain() and set by the disposer (which also nulls drainTimer), so no drain runs after teardown. Correct the now-inaccurate "no disposed guard needed" comments. Add a regression test that holds a drain mid-await, disposes, pushes a request, and asserts it is not processed (verified red before the fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
When a plugin streams cases one at a time (e.g. Simmer emitting ~200 items), each create request triggered its own model change and full tile-reactivity cascade — O(N) per add, O(N²) per stream — and the plugin's ~2s request timers started timing out around n≈118 with an open graph.
The request-queue processor now:
create … itemrequests into one batched create — oneapplyModelChange, oneaddCases, one notification round per batch — responding to each original request with its own per-segment result (positionalitemIDs; new cases attributed to the segment of their earliest contributing item, matching sequential semantics).If a batched create cannot proceed, the run's members are processed individually (exact sequential semantics). The
noCoalescedebug flag disables coalescing for A/B comparison.Measured (Simmer streams, graph + table open)
Semantics notes
createCasesnotifications.tableModifiedwas checked before the async processors could set it, soincrementInterruptionCountnever fired from this path; it now works.Post-review hardening (high-effort code review)
applyModelChange, which is not transactional on throw, so a failure afteraddCasescan leave cases committed. The fallback that re-processed members individually would then add those cases a second time (silent duplication). Now the individual fallback runs only when the dataContext was unresolvable (nothing mutated); any post-resolve failure responds with an error to each member instead. Covered by a new regression test.createItemsInSegmentsno longer builds the itemId→segment map or runs the per-casereduceon the ordinary (non-coalesced) create path, where every new case trivially belongs to segment 0. Behavior-preserving.JSON.stringifys each request on the streaming hot path whenDEBUG_PLUGINSis off.Stacked on #2628 (CODAP-1403); retarget to
mainafter it merges.Fixes CODAP-1408.
🤖 Generated with Claude Code