Skip to content

fix(workflows): avoid cloning payloads for graph updates - #2144

Merged
flora131 merged 4 commits into
fix/2100-overlay-graph-tui-perffrom
fix/2100-payload-free-store-observation
Aug 3, 2026
Merged

fix(workflows): avoid cloning payloads for graph updates#2144
flora131 merged 4 commits into
fix/2100-overlay-graph-tui-perffrom
fix/2100-payload-free-store-observation

Conversation

@flora131

@flora131 flora131 commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Carries Shreyasd10/atomic#1 by @ashkanvg. Part of #2100.

This is @ashkanvg's work. 66c5db4 is their commit; git log shows Ashkan Vedadi Gargary <70602836+ashkanvg@users.noreply.github.com> as author and GitHub attributes it to them.

Top of stack #2145. Base is #2143 by @Shreyasd10. Review only the diff for this layer; #2143 carries the viewport work.

Why this is a separate half of the same problem

#2143 makes painting topology-bounded — cards, edges, and hit targets cost what is on screen rather than what is in the graph. Testing that branch against payload-heavy nested workflows surfaced a second bottleneck that sits before rendering: every store mutation still built a full JSON.stringify / JSON.parse snapshot, so a small question or status update traversed complete workflow inputs, authored stage result bodies, child output values, and tool bodies before the optimized renderer ever ran.

Measured by @ashkanvg on a real GraphView with 480 nodes and a 100 MiB workflow input:

Branch Store mutation median Viewport render median
#2143 alone 147.71 ms 3.58 ms
#2143 + this 2.85 ms 4.46 ms

Neither change is sufficient alone. One removes cost that scales with stage count, the other cost that scales with payload size.

What changed

  • An optional synchronous invalidation-only store channel that constructs no snapshot.
  • One immutable, memoized graph projection per store version.
  • Graph-irrelevant payloads excluded: run inputs, authored stage result bodies, child output values, tool input/output bodies and unbounded tool history, model-attempt and MCP-scope details.
  • Preserved: topology, status, timing, prompts, attachment state, notices, bounded returned-status fields, durable tool summaries, child output counts.
  • Migrated graph, overlay, attach pane, stage chat, widget, lifecycle and HIL notifications, resume picker, send admission, and graph-only control helpers to the compact channel.
  • The existing Store.snapshot() / Store.subscribe(snapshot) contract is unchanged; the opt-in full status-file writer stays on it deliberately, since its external JSON schema is unchanged.

Rebase notes

This commit was written against 9505c1b and needed rebasing onto current main. Two conflicts, both from #2140 reshaping run cards afterwards:

node-card.ts — mechanical. #2140 moved the child run id onto its own wrapped row, so the meta line no longer carries run <shortRunId> · . Kept main's shorter line, applied this commit's completed.outputCount ?? Object.keys(completed.outputs).length.

session-overlays.ts — a behavioral resolution worth a reviewer's attention. Main had begun caching a snapshot inside the store.subscribe callback to feed the resume-candidate lookup. The invalidation channel carries no snapshot, so taking this commit's line as written would have frozen that cache at its initial value and newly started runs would never have appeared in the picker. The picker now reads one memoized projection per render:

const selectRows = () => {
  const snapshot = readGraphStoreSnapshot(store);
  const resumeCandidateLookup = intent === "resume" ? resumeCandidateCache(snapshot) : undefined;
  return selectRunsForPicker(snapshot.runs, state.query, state.includeAll, Date.now(), intent, resumeCandidateLookup);
};
unsubscribe = subscribeStoreInvalidation(store, () => tui.requestRender?.());

That keeps the payload-free path — graphSnapshot() is memoized per store version and resumeCandidateCache already keys on snapshot.version — without the staleness. Flagged to @ashkanvg here.

The changelog entry was also split: rather than rewriting #2143's [Unreleased] bullet, this layer adds its own, so each PR carries its own release note.

Verification on the stacked tree

Command Outcome
npm run check exit 0
npm run test:unit exit 0 — 620 files, 5806 passed, 2 pre-existing skips

Review status

Reviewed. Five fresh-context verifiers drove the real store, executor, and TUI consumers against this layer alone (base fix/2100-overlay-graph-tui-perf). Render output is byte-identical between the legacy snapshot and the compact projection across GraphView, WorkflowAttachPane in both modes, the status widget, session lists, node cards, and all four lifecycle notification kinds. Memoization freshness held at 31 checkpoints across every mutating store method with zero stale reads, and two independent live-executor probes could not turn the latent version-key gap into a user-visible stale repaint (polls: 68575 divergences: 0).

One claim was falsified, and it is a good one:

The projection was not payload-free. compactResultField bounded a run result with value.slice(0, 1024). V8 backs a slice of a flat string of 13+ characters with a SlicedString pointing at its parent, so the projected field reported length === 1024 while keeping the whole original alive. Because graphSnapshot() memoizes, the store itself held that retention after the run left state.runs, as did every long-lived projection holder. A session with large workflow results grew the heap this change exists to reclaim.

Fixed in b7f077a with a calibrated regression test. I re-verified it independently: reverting only the flatten call fails exactly the projection arm with retained 33571744 bytes, while both control arms still pass.

Also worth recording, because it was checked rather than assumed: the edit to test/integration/overlay-entrypoints-animation.test.ts is a repair, not an accommodation. The base branch shipped that test red — a run with stages: [] leaves hasAnimatingStages === false, so no tick ever fired. The animation gate is byte-identical on both branches and all three original assertions survive verbatim.

Before merge, from the full review above:

  1. Resolve the resume-intent divergence — the projection deletes exactly the fields workflowRunHasArtifactReference scans, so artifactsIntact degrades to undefined and the resumability guard never fires. Unreachable today because no shipped surface passes "resume" to openSessionPicker, so either fix it or delete the unreachable intent.
  2. Close four coverage gaps in one pass on store-payload-observation.test.ts: the outputCount round trip, memo identity, both size bounds, and the deep freeze. Roughly four short tests; kills six mutations that currently survive the entire 5808-test unit project.
  3. Wrap the two notify() loops in per-listener try/catch — one throwing invalidation listener currently starves every later listener and the whole legacy snapshot channel.

Cheap in the same pass: make graphSnapshot / subscribeInvalidation required rather than optional (there is exactly one Store, so the fallbacks never run and they make one helper return two different data shapes under one type), document the version-bump invariant, and qualify the changelog headline — statusFile: true sessions still pay the full traversal.

Notes

  • No dependency or lockfile changes.
  • No DBOS, checkpoint, persistence, or workflow authoring schema changes.

Greptile Summary

This change adds a compact, memoized workflow graph snapshot and synchronous invalidation notifications so interactive graph and overlay updates avoid cloning or retaining full run payloads.

Focused workflow-store suites and a targeted mutation test passed. The executed paths disproved concerns that a graph snapshot could remain stale after an invalidation, that the compact projection could omit graph-facing prompts, input requests, tool summaries, notices, or child-output metadata, that legacy subscribers could lose complete payloads, or that one throwing observer could prevent later observers from receiving updates.

Confidence Score: 5/5

Safe to merge; no blocking failure remains in the validated store projection, notification, and compatibility behavior.

The focused and targeted tests exercised store mutation through both compact and legacy observation paths, including synchronous invalidation and observer failure isolation, without reproducing a defect.

T-Rex T-Rex Logs

What T-Rex did

  • Ran baseline focused test suite and confirmed 3 files with 13 tests passed.
  • Executed targeted mutation path; a single file with 1 test passed, and the graph version was read within both synchronous invalidation callbacks after mutation, including after the first callback threw.
  • Performed regression rerun after temporary test cleanup; 3 files with 13 tests passed.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (2): Last reviewed commit: "refactor(workflows): drop unreachable un..." | Re-trigger Greptile

@flora131 flora131 changed the title fix/2100 payload free store observation fix(workflows): avoid cloning payloads for graph updates Aug 2, 2026
Comment thread packages/workflows/src/shared/graph-store-snapshot.ts Fixed

@flora131 flora131 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.

Adversarial review of this layer only, run against the stacked tree with origin/fix/2100-overlay-graph-tui-perf as the base. Five fresh-context verifiers plus deterministic npm run check / npm run test:unit gates. The one blocking defect was repaired in this branch as b7f077a; I independently re-confirmed the regression test by reverting the fix and watching exactly one arm fail.

Review

The change is sound and it does what it claims. Five reviewers drove the real store, the real executor and the real TUI consumers, and render output is byte-identical between the legacy snapshot and the compact projection across GraphView, WorkflowAttachPane in both modes, the status widget, session lists, node cards and all four lifecycle notification kinds. The payload-exclusion core (stage.result, run.inputs) and the invalidation channel are well covered by the new tests: seven of fifteen mutations died on the diff's own three files, and the countFullSnapshots + payload-getter pattern in workflow-large-payload-interaction.test.ts is the strongest test work here, because it asserts the absence of expensive work rather than the presence of a value.

One claim was falsified: the projection was not payload-free. That defect is fixed in this branch, along with a regression test. What remains is one real but currently unreachable behaviour divergence, and a cluster of coverage gaps in the parts of the projection that are bounds and derived values rather than exclusions — the memo identity, the 1024-char clamp, the tool-event cap, outputCount, the deep freeze. Every one of those survives the full 5808-test unit project when mutated.


Major — fixed in this branch

compactResultField's slice() pinned the untruncated run result

packages/workflows/src/shared/graph-store-snapshot.ts:14-32, retained via store-internal.ts:143-158.

compactResultField bounded a field with value.slice(0, 1024). V8 backs a slice of a flat string of 13+ chars with a SlicedString holding a pointer to the parent, so the projected result.summary reported length === 1024 while keeping the whole original alive. graphSnapshot() memoizes the projection, so the store itself held that retention after the run left state.runs, and every long-lived holder of a projection — GraphViewState.currentSnapshot, WorkflowAttachPane, the overlay adapter — held it for whatever version it captured. inputs was correctly freed (inputs: {}), so the leak was specific to run.result. A session with large workflow results grew the heap this change was written to reclaim.

Heap-snapshot reachability, with the scanner validated first so it can only report a retained parent:

$ npx vitest --run --project unit test/unit/__probe-i-scanner.test.ts
I1 flat string held      -> []
I2 slice-of-12MB held    -> ["string 12582928B head=ZZZZZZ"]
I3 flattened-slice held  -> []

$ npx vitest --run --project unit test/unit/__probe-k-leak-control.test.ts   # after removeRun("r1")
K1 (no reader)   []
K2 (legacy)      []
K3 (projection)  ["string 12582928B head=rrrrrr","string 12582928B head=ssssss"]
K4 (re-read)     []

I reproduced the mechanism independently before judging the fix: node --expose-gc on a 1024-char slice of a 16 MB flat string retained 16.8 MB after the parent left scope; the same slice through split("").join("") retained 0.0 MB, and the round trip is code-unit exact.

Fix applied (graph-store-snapshot.ts:14-32): truncated fields now pass through flattenTruncatedField, with a comment naming the SlicedString reason. This is the only truncation site in packages/workflows/src/shared.

Regression test added: test/unit/graph-projection-result-retention.test.ts. It is calibrated, not one-sided — its "payload deliberately held" and "no projection taken" arms pass under both the fixed and the defective source, so only the projection arm moves. I reverted just the source line and re-ran it: the projection arm failed with retained 33571744 bytes. With the fix restored it passes 3/3 on five consecutive runs. It also asserts the projected summary is still a 1024-char prefix of the right field, so a fix that emptied the field could not pass.

Gates re-run on the restored tree: npm run check exited 0; npm run test:unit exited 0 — 621 files, 5809 passed, 2 skipped, the new file included under full-suite parallelism. git diff HEAD -- '*test*' is empty: no existing test was weakened or deleted.

Out of scope, not introduced here: summarizeToolResult (packages/workflows/src/durable/tool-primitive.ts:615) builds ${serialized.slice(0, 237)}..., whose ConsString still points at the SlicedString and so at the whole JSON.stringify output — 8.4 MB pinned by a 240-char summary. It exists identically on HEAD and sits on the durable path, not the memoized projection. Worth a separate issue.


Minor

1. The resume intent feeds the projection into the artifact-integrity probe

packages/workflows/src/tui/session-overlays.ts:107-118

selectRows builds resumeCandidateCache(readGraphStoreSnapshot(store)). Downstream, workflowRunHasArtifactReference (workflow-artifacts.ts:53-64) detects run-scoped artifacts by serialising { result: run.result, stages: run.stages } and searching for /runs/<runId>/ — text that lives exactly in the fields the projection deletes: stage.result, toolEvents[].input/output, and non-canonical keys of run.result. Against the projection the scan finds nothing, workflowRunArtifactsIntact degrades from false to undefined, and isWorkflowRunResumable's artifactsIntact === false guard never fires.

Two reviewers, different angles, production functions with only hasDurableCheckpoint stubbed, one paused run whose stage.result names a nonexistent artifact dir:

$ npx vitest --run --project unit test/unit/__probe-artifacts.test.ts
full hasArtifactReference: true    / graph: false
full artifactsIntact:      false   / graph: undefined
full isWorkflowRunResumable: false / graph: true
resume rows (full): []             / (graph): ['wf-artifact-run-00000000']

End to end through the real overlay, the picker rendered empty on the store.snapshot() fallback and a row on the projection.

This is minor rather than major because no shipped surface takes this path. I read workflow-run-control-command.ts:236-312: the action === "resume" branch returns before the openSessionPicker(...) call on line 313, and the only other call site (line 79) passes "connect". A reachability probe driving the real handleRunControlCommand agrees — action=resume mounted titles=[], while pause and attach do mount. The live resume surface (collectResumePickerLiveRuns, workflow-resume-picker-rows.ts:41-45) still reads runStore.runs().

Please pick one: build the resume-candidate lookup from live runs — resumeCandidateCache({ ...snapshot, runs: store.runs() }), still one artifact scan per store version since the cache invalidates on snapshot.runs identity — or compute hasArtifactReference as a boolean inside createGraphStoreSnapshot from live state. If neither, delete the unreachable resume intent from openSessionPicker rather than shipping a filter that silently over-offers. Add a test asserting resume eligibility is identical between the full run and the projected run for a run whose artifact path appears only in stage.result.

2. The outputCount round trip has zero coverage on both halves

packages/workflows/src/shared/graph-store-snapshot.ts:60-66 and node-card.ts:141-150

outputCount exists for one reason: the projection blanks outputs to {}, so node-card.ts:148 can no longer count keys. Producer and consumer are both new, both load-bearing, and neither is asserted anywhere. grep -rn outputCount test/ packages/workflows returns only node-card.ts:146,148, graph-store-snapshot.ts:64, store-types.ts:157 — no test file. I re-ran that grep in the working tree.

Mutation 3 (delete the outputCount: line) and mutation 4 (drop the ?? Object.keys(...) fallback) each survived the diff's three test files and the full project:

=== MUTATION 3 vs FULL unit project ===
 Test Files  620 passed (620)
      Tests  5806 passed | 2 skipped (5808)

Today's code is correct — both reviewers rendered 3 outs through the real projection — but the failure mode is a silently wrong number on every workflow-child boundary card, not a crash:

legacyPath=3 outs | projectedPath=3 outs | regressedProjection=0 outs

One case in store-payload-observation.test.ts — record a 3-output workflowChild, assert outputs is {} and outputCount === 3 through store.graphSnapshot(), render the card and assert "3 outs" — kills both mutations.

3. One throwing invalidation listener starves everything downstream

packages/workflows/src/shared/store-internal.ts:160-166

notify() iterates state.invalidationListeners in a bare for loop with no per-listener isolation, and the legacy full-snapshot loop is strictly downstream. A throw from any invalidation listener skips every later invalidation listener, skips snapshot() entirely, skips every subscribe consumer (status-writer.ts:165 is the last one), and escapes the mutating store call into the executor.

$ npx vitest --run --project unit test/unit/__probe-b-channel.test.ts
B4 threw out of the store mutation: listener boom
B4 listeners actually called: ["thrower"]
   expected ["thrower","second-invalidation-listener","legacy-full-listener"]

Notifier fragility pre-dates the change, but the two channels are now ordered, so a throwing TUI listener deterministically starves the full-snapshot channel regardless of registration order. Wrap each dispatch in try { fn(); } catch {} in both loops.

4. The projection cache is keyed only on state.version

packages/workflows/src/shared/store-internal.ts:152-158

snapshot() re-serialized live rows on every call; graphSnapshot() returns the memoized object while state.version is unchanged. The store hands out live objects (store-run-methods.ts:39-41 returns state.runs itself), and the executor mutates them without a store method: executor-stage-call.ts:154/263, executor-scheduler.ts:55 (a graph edge change), executor-lifecycle.ts:107. Pull-based readers now go through the cache — workflow-attach-pane.ts:456, stage-chat-view-state.ts:350, postmortem-deps.ts:37/65, workflow-targets.ts:119/170, quit.ts:104/307 — all called on keypress.

$ npx vitest --run --project unit test/unit/__probe-e-stale.test.ts
E1 legacy snapshot() sees: running ["s0"]
E1 graph projection sees : pending []
E1 versions: 1 1

Two reviewers demonstrated the mechanism and neither could turn it into a user-visible stale repaint. Live-executor probes polled a live/projection fingerprint on every macrotask and microtask across real runs including parallel stages, a ctx.tool node and a failing stage: D status: completed polls: 68575 divergences: 0, and E2 polls: 3366 divergences: 0. Every direct write is closely followed by a bumping store call.

So: latent, undocumented, untested. State the invariant on graphSnapshot() — graph-visible state may only change through a version-bumping store method — and at those four executor sites prefer the store method or bump after the write. Do not add polling or version-free rebuilds.

5. The memoization contract — the point of the change — has no assertion

packages/workflows/src/shared/store-internal.ts:143-158

store-public-types.ts:232 documents graphSnapshot() as one immutable projection per store version. workflow-attach-pane.ts alone calls readGraphStoreSnapshot from eight methods, so without the cache one keystroke rebuilds the projection eight times at O(runs × stages). Removing the memo restores exactly the per-mutation cost this change removes, and nothing detects it:

=== MUTATION 6 (unconditional rebuild) vs FULL unit project ===
 Test Files  620 passed (620)
      Tests  5806 passed | 2 skipped (5808)

WITH MUTATION 6 -> PROBE same-version identical = false
AT HEAD         -> PROBE same-version identical = true / after-bump identical = false

The inverse mutation — a cache that never invalidates — is caught, but only as × question preview, input, and submission avoid full snapshots and payload traversal 30003ms, which under the AGENTS.md flaky-suite gate burns the whole budget instead of failing in milliseconds. assert.equal(store.graphSnapshot!(), first) plus assert.notEqual after a recordStageStart bump fixes both.

6. graphSnapshot? / subscribeInvalidation? are optional with no second implementation

packages/workflows/src/shared/store-observation.ts:1-12, store-public-types.ts:231,235

There is exactly one Store:

$ grep -rn "): Store\b\|satisfies Store\b\|implements Store\b" packages/workflows/src packages/coding-agent/src --include=*.ts
packages/workflows/src/authoring.ts:341:export declare function createStore(): Store;
packages/workflows/src/shared/store-factory.ts:8:export function createStore(): Store {

It always supplies both members, so the : store.snapshot() / : store.subscribe(listener) fallbacks never run — not in production, not in any test. They are not inert dead code: they make one helper return a different data shape depending on the receiver, under the same StoreSnapshot type, with no type-level help:

legacyInputsKeys=["big"] | graphInputsKeys=[] | legacyStageResult=authored stage result |
graphStageResult=undefined | legacyFrozen=false | graphFrozen=true

node-card.ts:148 already carries one ?? compensating for the divergence. The fallback also breaks the identity contract the resume-candidate memo relies on — store.snapshot() mints a new runs array per call, so createSessionPickerResumeCandidateCache (session-picker.ts:73) thrashes on every render:

legacy store: probes after 1 render=2, after 3 renders=6
real store:   first render=2, after 4 renders=2, after mutation+render=5

Make both members required, delete the two ternaries, keep readGraphStoreSnapshot as a named seam. authoring.ts:326-339 — the declaration external authors compile against — does not declare snapshot/subscribe either, so this breaks no published contract. If the optionality must stay, memoize the fallback by version in a per-store WeakMap.

7. Both projection size bounds survive the entire unit project when removed

packages/workflows/src/shared/graph-store-snapshot.ts:14-19, 55-58

compactStage drops stage result outright, so the 1024-char clamp is the only thing bounding run.result strings entering the projection; compactToolEvents narrowing an unbounded ToolEvent[] to one name-only entry is the only thing bounding per-stage growth for a long-running stage. Neither is asserted in either direction.

MUTATION 13 (compactResultField returns value unchanged) -> full project: 620 files, 5808 green
MUTATION 14 (return events.map((event) => ({ ...event }))) -> identical result

This compounds the retention finding above: the clamp is the code whose implementation leaked. One test asserting both sides of the clamp (result?.summary?.length === 1024 for a 5000-char summary, a short status surviving verbatim) and one asserting three recorded tool events project to exactly [{ name: "third" }].


Nits

Deep freeze untestedgraph-store-snapshot.ts:115-139. Unlike snapshot(), which handed each caller a private clone, the memoized projection is one object shared by every consumer in a version. The freeze is what stops one view corrupting another's data, and store-public-types.ts:232 promises it. Mutation 15 (remove the deepFreezeGraphValue call): full project green. Assert Object.isFrozen on the snapshot, on snap.runs, and on snap.runs[0].stages[0], so the freeze is pinned as deep rather than shallow.

Invalidation ordering untestedstore-internal.ts:160-171. The invalidation channel carries no snapshot, so listeners call graphSnapshot() themselves; that only yields fresh data because bumpAndNotify increments state.version before notify(). Mutation 9 (invalidation loop moved ahead of the bump, legacy path untouched) passed all three diff files. With a warmed cache: AT HEAD listener saw stages = 2 version = 2; WITH MUTATION 9 stages = 1 version = 1. The wider suite does catch it, as 67 diffuse failures across 18 unrelated files rather than a diagnosis. Extend the existing "notifies synchronously…" test — warm the memo with a graphSnapshot?.() call first, since without one the mutation is invisible.

countFullSnapshots is a blind seamtest/unit/workflow-large-payload-interaction.test.ts:25-33,100,157. The helper patches the public store.snapshot method, but notify() builds the legacy snapshot through the module-local closure: patchedStoreSnapshotCalls=0 | legacySubscriberDeliveries=1. A regression that rebuilt a full snapshot on every mutation leaves the counter at 0. The neighbouring payloadReads.count === 0 is what holds the line, and only because the fixture registers no subscribe listener. The file is otherwise sound — injected now, hand-advanced clock, no timers, no wall-clock bound, so it will not flake on a loaded runner. Either drop the fullSnapshots assertion as decorative or assert payloadReads.count === 0 with a legacy subscriber registered.

Changelog headline overstates scopepackages/workflows/CHANGELOG.md:12. notify() skips the full snapshot only when state.listeners.size === 0. status-writer.ts:165 is the last store.subscribe consumer and is installed unconditionally at extension-runtime-state.ts:83. The default statusFile is false, so default sessions get the full win, but a user with statusFile: true still pays the whole traversal plus a second JSON.stringify per mutation. Qualify with "in the default configuration", or migrate the status writer onto subscribeInvalidation plus its own snapshot() call.

unknown where a specific type existsgraph-store-snapshot.ts:16,127-135. compactResultField(value: unknown) is only ever called with result.status/summary/remaining_work/result, all typed WorkflowSerializableValue | undefined at authoring-contract-stage.ts:8-20. A reviewer compiled the identical body with the specific signature and the repo typecheck was clean — it is a drop-in. deepFreezeGraphValue(value: unknown) is the more defensible of the two as a generic walker; a one-line comment would settle it.

compactStage preserves undefined-valued keysgraph-store-snapshot.ts:68-96. The legacy JSON.parse(JSON.stringify(...)) stripped own keys whose value is undefined; the object spread keeps them (observed: stages[3].skippedReason ADDED as an own key with value undefined). No migrated consumer enumerates stage keys or uses in, and every render diff was identical, so this is inert — but Object.keys(stage).length now differs between the two shapes, which matters if the optional fallback stays.

Re-entrant mutation skips a version for legacy subscribersstore-internal.ts:160-166. B5 before 1 full-listener versions [3,3] where [2,3] was expected. The last delivered value is correct, and the only remaining subscriber is level-triggered. No code change needed; note it in the subscribeInvalidation doc comment if the contract should be explicit.

Two animation tests still pass with zero tickstest/integration/overlay-entrypoints-animation.test.ts:118-192. Worth being clear that the fixture change is a repair, not an accommodation: the base branch shipped test 1 red, because a run with stages: [] leaves hasAnimatingStages === false so no tick ever fired. Running the old fixture against the new code:

✓ suppresses ... while hidden        ✓ tick stops after ... disposed
FAIL expected tui.requestRender to fire on the animation tick (got 0)
Tests  1 failed | 2 passed (3)

The animation gate in graph-view-state.ts is byte-identical on both branches. All three original assertions survive verbatim. But tests 2 and 3 assert only that a count did not grow, and neither observes a tick before its negative window — test 3 captures afterDispose and never asserts it non-zero. Add assert.ok(afterDispose > 0) in test 3 and observe one tick before setHidden(true) in test 2.


Probed and clean

So the coverage of this review is auditable:

  • Render fidelity. A 21-difference field-level diff of snapshot() vs graphSnapshot() was traced to every consumer, and every render came back byte-identical: GraphView.render(120), WorkflowAttachPane in graph and stage-chat mode (nested child run, blocked stage, durable tool node, an inputRequest with question header and two option labels), buildThemedWidgetLines, renderSessionList, statusRuns (deepEqual), all four lifecycle notification kinds, HIL answer notifications, and node cards at 0/1/3 child outputs.
  • The freeze does not leak onto live state. Three reviewers independently: after graphSnapshot(), live run/stage/inputs/toolEvent are all frozen: false, a subsequent mutation is reflected, and deepFreezeGraphValue reaches only objects the compactors newly allocated. No shared subobject references between store and projection.
  • The memo does not freeze the UI. installReactiveWidget compares rendered lines and re-reads now() per refresh, so stable snapshot identity cannot freeze elapsed time — driven end to end the widget updated 0/1 0s -> 1/2 0s. The picker re-reads the projection per render and a run started after mount appears for all three intents. The counterfactual (caching the snapshot in the subscribe callback) does freeze the list at cached rows=1 live rows=2, so the merge resolution is necessary, not cosmetic.
  • Memoization freshness. 31 checkpoints across every mutating store method: after each, graphSnapshot() deep-equalled a freshly built projection. 0 stale. No store method notifies without bumping.
  • Not migrated, so unaffected. inspectRun (run-inspect.ts:84) and snapshotTranscriptEntries still read full data.
  • Structural rules. No dist/, outDir, tsconfig.build or bundling in the diff; .js import extensions throughout; the changelog entry appends to the existing ### Fixed under ## [Unreleased] with released sections untouched; neither new test file declares an explicit per-test timeout.
  • Docs. Checked and refuted rather than assumed: packages/coding-agent/docs/workflows.md:3723-3748 documents the lean Store contract and explicitly excludes the snapshot/subscription surface, and authoring.ts omits snapshot/subscribe too. Two more runtime-only members create no doc gap.

Three reported items were discarded. The complaint that store-payload-observation.test.ts tests the store primitive rather than the migrated call sites rested on an un-run thought experiment; the probe that was run supports the opposite conclusion. The bare }, 15_000) in the animation test comes from the base branch, is untouched here, and does not restate the 30 000 ms default, so it is not the case AGENTS.md forbids.


Recommended action

Request changes. The blocking defect is already fixed and tested in this branch — please review graph-store-snapshot.ts:14-32, the new test/unit/graph-projection-result-retention.test.ts, and the changelog entry, and keep them.

Before merge:

  1. Resolve the resume-intent divergence (minor 1) — fix it or delete the unreachable intent.
  2. Close the four coverage gaps in one pass on store-payload-observation.test.ts: outputCount round trip, memo identity, both size bounds, deep freeze. That is roughly four short tests and it kills six surviving mutations.
  3. Wrap the two notify() loops in per-listener try/catch (minor 3).

Cheap and worth doing in the same pass: make graphSnapshot/subscribeInvalidation required (minor 6), document the version-bump invariant on graphSnapshot() (minor 4), and qualify the changelog headline. The remaining nits are optional.

@flora131

flora131 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Review items closed — ready for review

All three before-merge items and the cheap follow-ups are done, verified under an automated kill-matrix: each new assertion was scored by applying the exact regression it targets and requiring a test to fail. 9/9 mutations killed, verifier approved with no weakened tests and no unaddressed items.

1. Resume-intent divergence — the resume-candidate lookup now probes live runs via { ...snapshot, runs: store.runs() }, so a run whose artifact path appears only in stage.result is detected and isWorkflowRunResumable stops silently over-offering. Still one artifact scan per store version, because the cache invalidates on snapshot.runs identity.

2. Four coverage gaps closed. Each previously survived the entire 5808-test unit project:

Gap Regression that now fails
outputCount round trip deleting the producer line; dropping the card's ?? fallback
Memo identity rebuilding the projection on every read
Result clamp returning the value unclamped; dropping short-value passthrough
Tool-event bound projecting all events; keeping event payloads
Deep freeze removing the freeze; making it shallow

3. notify() listener isolation — both loops wrap each dispatch, so one throwing invalidation listener no longer starves later listeners or the legacy snapshot channel.

Plus: graphSnapshot and subscribeInvalidation are now required Store members with the fallback ternaries deleted (there was exactly one Store, so they never ran, and they made one helper return two different data shapes under one type); the version-bump invariant is documented; the changelog headline is qualified for statusFile: true; and the two test-fidelity items are fixed.

Rebased onto the hardened base. npm run check and npm run test:unit green — 5833 passed, 2 pre-existing skips.

@ashkanvg — the session-overlays.ts resume divergence I flagged as coming from my rebase resolution is resolved in item 1. Worth a look if you want to confirm the approach matches what you had in mind.

@flora131

flora131 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto the CI fix, and automated review addressed

Review feedback. cloneInputRequest was flagged for an unreachable undefined guard — correct: its sole call site already spreads behind inputRequest !== undefined. Fixed.

clonePrompt has the identical shape and all three of its call sites are guarded the same way. It was not flagged, but leaving the twin of a defect we just removed is worse than not having found either, so both are fixed. Both now take and return a concrete value; the optional signatures were not merely redundant, they widened the return type to T | undefined for callers that can never receive it.

Rebase note. The integration-fixture repair that was in @ashkanvg's commit has moved down to #2143, because that is the layer whose animation gate breaks the fixture, and a lower layer has to be green on its own. It is unchanged and co-authored there. Everything else in this layer is untouched.

All four suites green: 5836 unit, 484 integration, 38 ci-contracts, check clean.

@flora131
flora131 marked this pull request as ready for review August 3, 2026 05:42
Comment thread packages/workflows/src/shared/graph-store-snapshot.ts
ashkanvg and others added 4 commits August 2, 2026 22:57
Add an invalidation-only store channel and one memoized payload-free graph
projection per store version, so a small status or question update no longer
walks complete workflow inputs, authored stage result bodies, child output
values, and tool bodies before the overlay renders.

Rebased onto current main. Two conflicts resolved against #2140, which
reshaped run cards after this commit was written:

- node-card.ts: kept main's meta line, which no longer carries the short run
  id because #2140 moved the full id onto its own wrapped row, and applied
  this commit's payload-free `outputCount ?? Object.keys(outputs).length`.
- session-overlays.ts: main had begun caching a snapshot from the subscribe
  callback for the resume-candidate lookup. The invalidation channel carries
  no snapshot, so that cache would have gone stale and newly-started runs
  would never have appeared in the picker. The picker now reads one memoized
  projection per render via readGraphStoreSnapshot().

Co-authored-by: Ashkan Vedadi Gargary <70602836+ashkanvg@users.noreply.github.com>
…ults

`compactResultField` bounded a run result with `value.slice(0, 1024)`. V8 backs
a slice of a flat string of 13 characters or more with a SlicedString that
points at its parent, so the projected field reported `length === 1024` while
keeping the whole original alive.

That matters more here than at a normal truncation site, because
`graphSnapshot()` memoizes the projection: the store itself held the retention
after the run left `state.runs`, and so did every long-lived holder of a
projection — GraphViewState, WorkflowAttachPane, the overlay adapter. A session
with large workflow results grew the heap this change exists to reclaim.
`inputs` was already freed, so the leak was specific to `run.result`.

Copy truncated fields into fresh flat strings via `split("").join("")`, which is
code-unit exact and leaves no parent pointer.

The regression test is calibrated rather than one-sided: its "payload
deliberately held" and "no projection taken" arms pass under both the fixed and
the defective source, so only the projection arm moves. Reverting just the
flatten call fails that arm alone with `retained 33571744 bytes`, and the test
also asserts the projected summary is still a 1024-char prefix of the right
field, so a fix that emptied the field could not pass.

Not fixed here: `summarizeToolResult` in durable/tool-primitive.ts has the same
shape on the durable path, exists identically on the base branch, and deserves
its own issue.

Assistant-model: Claude Opus 5
Closes the coverage gaps an adversarial review found, where the source could be
broken while the suite stayed green. Each added assertion was scored by applying
the specific mutation it targets and requiring the covering tests to fail.

Gates: npm run check exited 0; npm run test:unit exited 0; mutations killed 9/9.

Assistant-model: Claude Opus 5
…jection cloners

Automated review flagged the guard in `cloneInputRequest` as dead. It is: the
sole call site already spreads conditionally behind `inputRequest !== undefined`.

`clonePrompt` has the identical shape and all three of its call sites are
guarded the same way, so it is fixed here too rather than left as the twin of a
defect we just removed.

Both now take and return a concrete value. The optional signatures were not
merely redundant — they widened the return type to `T | undefined` for callers
that can never receive it.

No behavior change; `npm run check` and the payload-observation suites pass.

Assistant-model: Claude Opus 5
@flora131
flora131 force-pushed the fix/2100-payload-free-store-observation branch from 1aa7026 to dcd9b84 Compare August 3, 2026 06:06
@flora131
flora131 merged commit 19ba078 into main Aug 3, 2026
18 checks passed
@flora131
flora131 deleted the fix/2100-payload-free-store-observation branch August 14, 2026 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants