Repository navigation
Perf/avoid shell snapshot for thread detail - #33
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
vouch:trusted
ronak-guliani
left a comment
ronak-guliani
left a comment
There was a problem hiding this comment.
Review
The core perf change (thread-detail route now reads getSnapshotSequence() instead of building the full getShellSnapshot()) is a clean, well-targeted win — nice. The new durable workflow-coordinator machinery is generally solid: stable idempotency keys for crash recovery, event-store/projector/read-model all threaded through, and good test coverage.
I focused on bugs, reliability, performance, and maintainability. One high-severity correctness bug in the decider (the missing-parent finalize path is unreachable and leaves a run stuck forever), plus a few medium/low items around the review-snapshot behavior change and N+1 query cost. Details inline.
| !node || | ||
| (node.status !== "completed" && node.status !== "failed" && node.status !== "pending") || | ||
| (node.status === "pending" && command.status !== "failed") || | ||
| command.status !== node.status || |
There was a problem hiding this comment.
Bug (high): the pending → failed finalize path is unreachable, so "parent deleted before worker started" runs get stuck forever.
WorkflowCoordinatorReactor.startWorker dispatches workflow.run.finalize with status: "failed" while the node is still pending (the finalize-missing-parent command). The check just above at lines 1378–1379 explicitly allows node.status === "pending" as long as command.status === "failed", and the projector at projector.ts (workflow.run-finalized) has dedicated logic to flip a pending node to failed. But this line unconditionally requires command.status === node.status, so for a pending node + failed command it evaluates "failed" !== "pending" → true → invariant error.
Because the engine records a permanent rejected receipt for invariant failures (OrchestrationEngine.processEnvelope), the finalize can never succeed on retry. The run stays pending/incomplete, listIncomplete() keeps returning it, and every subsequent workflow/thread domain event re-triggers reconcile → startWorker → dispatch finalize → PreviouslyRejected → logWarning — a permanently stuck run plus recurring log noise.
Suggested fix: scope the equality check to non-pending nodes, e.g.
(node.status !== "pending" && command.status !== node.status) ||This path isn't covered by tests today — the reactor test stubs the engine dispatch, so it verifies the command is emitted but not that the decider accepts it. Worth a decider-level test for pending+failed.
There was a problem hiding this comment.
Addressed in the local follow-up. The finalize invariant now permits the intentional pending -> failed transition while still requiring non-pending node status to match the command. A decider-level regression exercises the emitted finalize-missing-parent command against the real decider and verifies the run reaches failed. Pending commit/push to the PR branch.
| const bytes = Buffer.from(fullDiff); | ||
| const truncated = bytes.byteLength > REVIEW_SNAPSHOT_MAX_BYTES; | ||
| return { | ||
| if (Buffer.byteLength(fullDiff) > REVIEW_SNAPSHOT_MAX_BYTES) { |
There was a problem hiding this comment.
Behavior change (medium): silent truncation replaced by a hard failure. Previously a diff over the limit was truncated (truncated: true) and review continued; now anything over 4MB fails with a GitCommandError and truncated is always false.
Raising the cap to 4MB and reviewing the full diff is a good correctness improvement, but please confirm the two consumers degrade gracefully:
ws.tsreview-changes workflow start — a >4MB repo diff now fails the RPC/workflow instead of producing a (truncated) review. Is that the intended UX vs. e.g. aworkflowSkippedreason?ReviewSnapshotVerifier.isCurrent— see the note inProviderRuntimeIngestion.ts.
Also, ReviewSnapshot.truncated is now hard-coded false everywhere. If nothing can ever set it true, consider removing the field and any UI that renders a "truncated" indicator so it doesn't become dead/misleading code.
There was a problem hiding this comment.
Confirmed intentional: reviewing a partial diff is correctness-unsafe, so snapshots over 4 MiB fail explicitly rather than producing a misleading partial review. The local follow-up aligns each review-diff command output ceiling with the 4 MiB aggregate limit, removes the impossible ReviewSnapshot.truncated field, makes verification failures discard the result without aborting turn completion, and uses NUL for native Windows untracked diffs. Pending commit/push.
| }); | ||
| if ( | ||
| cwd === null || | ||
| !(yield* reviewSnapshotVerifier.isCurrent({ cwd, snapshot: thread.reviewSnapshot })) |
There was a problem hiding this comment.
Reliability (medium/low): a git failure in isCurrent is not the same as "worktree changed". reviewSnapshotVerifier.isCurrent now calls resolveReviewChangesContext, which can fail (git error, or the diff having grown past the new 4MB cap). Only the boolean-false result is handled here (log + discard). A failure instead propagates and is swallowed by processInputSafely's catchCause as a generic "provider runtime ingestion failed to process event" — the turn.completed handling for that event is aborted and the review result is permanently dropped (the event isn't retried).
Consider catching the verifier's error and treating it as "could not confirm snapshot is current → discard" with the specific warning, so a transient git hiccup doesn't masquerade as an ingestion failure.
There was a problem hiding this comment.
Addressed in the local follow-up. isCurrent failures are logged as snapshot-verification failures and converted to false; the review result is discarded fail-closed, but turn.completed ingestion continues instead of being aborted by the generic catch. Pending commit/push.
| Effect.mapError(toPersistenceSqlError("ProjectionWorkflowRepository.listIncomplete:query")), | ||
| ); | ||
|
|
||
| const listAll: ProjectionWorkflowRepositoryShape["listAll"] = () => |
There was a problem hiding this comment.
Performance (low/medium): N+1 node queries. Both listAll and listIncomplete fetch runs, then issue one listNodeRows per run with concurrency: 1 (sequential). listAll() runs in the full-snapshot path (ProjectionSnapshotQuery.getSnapshot/getShellSnapshot), which is exactly the hot path this PR is trying to keep cheap. With many runs this is O(runs) sequential round-trips.
Consider a single SELECT ... FROM projection_workflow_nodes WHERE run_id IN (...) (or a JOIN) and group in memory, matching how the other projections avoid per-row fan-out.
There was a problem hiding this comment.
Addressed in the local follow-up. listAll and listIncomplete now fetch all node rows for the selected run IDs in one IN (...) query and group them in memory. The reconnect shell path also uses this batched query through a bounded lightweight snapshot. Pending commit/push.
| listIncompleteRunRows(undefined).pipe( | ||
| Effect.flatMap((rows) => | ||
| Effect.forEach( | ||
| rows.filter((row) => isIncompleteWorkflowRun(row.status)), |
There was a problem hiding this comment.
Minor: this .filter(isIncompleteWorkflowRun(...)) is redundant with the SQL WHERE status IN ('pending','running') in listIncompleteRunRows. Keep one source of truth (either the SQL predicate or the code filter) to avoid the two drifting.
There was a problem hiding this comment.
Addressed in the local follow-up. The redundant application-level incomplete-status filter was removed; the SQL predicate is now the single source of truth. Pending commit/push.
| typeof value.body !== "string" || | ||
| typeof value.confidence_score !== "number" | ||
| ) { | ||
| return value; |
There was a problem hiding this comment.
Behavior change (confirm intent): a single non-normalizable finding now invalidates the entire review. Previously non-matching findings were dropped (flatMap → []); now the raw value is returned, which won't match the ReviewFinding schema, so decodeReviewModelOutput throws and the whole result becomes invalid-output. Combined with returning null at line 27 for non-Codex output, this makes parsing much stricter.
That's defensible (don't silently swallow findings), but it's a meaningful semantic shift — one malformed finding discards an otherwise-valid review. Please confirm this is intended rather than per-finding filtering.
There was a problem hiding this comment.
The local follow-up retains per-finding tolerance: non-normalizable findings are filtered while the overall envelope still must satisfy the review schema. One malformed finding therefore no longer discards otherwise valid findings. Pending commit/push.
| createWorkflowRuntimeState(), | ||
| ref.threadId, | ||
| ) | ||
| : []; |
There was a problem hiding this comment.
Nit: the : [] fallback returns a fresh array each call, which is the exact footgun the new AGENTS.md scar ("selectors must return a referentially stable snapshot… fresh arrays can trigger React pingdotgg#185") warns about. It's currently safe because the only consumer wraps it in useShallow, but a shared frozen EMPTY constant would make it robust regardless of how future callers subscribe.
There was a problem hiding this comment.
Addressed in the local follow-up. The selector now returns a shared frozen EMPTY_WORKFLOW_RUNS value, so referential stability no longer depends on every caller remembering useShallow. Pending commit/push.
- decider: allow the pending -> failed finalize path so runs whose parent was deleted before the worker started can be finalized instead of getting stuck on a permanent invariant rejection - provider runtime ingestion: treat a git failure while verifying the review snapshot as "cannot confirm current -> discard" instead of letting it surface as a generic ingestion failure that drops the event without retry - projection workflows: batch node lookups for listAll/listIncomplete with a single WHERE run_id IN (...) query and drop the redundant status filter now covered by SQL - review result: drop non-normalizable Codex findings per-finding instead of invalidating the entire review - web store: return a referentially stable frozen EMPTY_WORKFLOW_RUNS snapshot - tests: cover pending+failed finalize decider path and batched workflow node grouping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Consolidated follow-up reviewStatus: the fixes below are implemented and validated in the local follow-up working tree, but are not yet committed or pushed to the PR branch. I replied to each of the seven existing inline threads with its concrete disposition. Existing review feedback
Additional issues found in the fresh review
Local validation
The current GitHub Windows Smoke failure is against the existing remote head and should be rerun after the Windows follow-up is pushed. |
- reconcile recovered workflows via activeTurnId/latest-turn state instead of runtime liveness so alive-but-idle provider runtimes no longer stay stuck - finalize tool-only completed turns with a deterministic fallback worker body - cascade thread archival across the full active subtree (including active descendants beneath already-archived intermediaries) and stop every affected provider runtime/terminal; extract collectActiveThreadSubtree - populate parentThreadId on thread.created so subtree traversal finds children - align git command output limits with the 4 MiB aggregate review-snapshot cap and use NUL instead of /dev/null for untracked-diff capture on Windows - remove the now-dead ReviewSnapshot.truncated field - mirror worker-start and failed-finalization workflow transitions client-side - limit sidebar agent-run dismissal to terminal runs, persisted by stable keys - bound reconnect shell snapshots to all active runs plus the latest 20 terminal runs per parent and load only result/final artifacts - collapse subtree session mapping into a single flatMap pass - target the rendered table header directly in the ChatMarkdown browser test - wait for the asserted ordering side effect in the provider-reactor test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…shot-for-thread-detail # Conflicts: # apps/web/src/components/ChatMarkdown.browser.tsx
1 participant
What Changed
Why
UI Changes
Checklist