Fix catchCause does not deliver interrupt causes when fiber is interruptible - #7140
Fix catchCause does not deliver interrupt causes when fiber is interruptible#7140fubhy wants to merge 2 commits into
Conversation
fubhy
added
bug
350dc4d to
94e6012
Compare
🦋 Changeset detectedLatest commit: e0ffce5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR documents a real bug with a failing reproduction test, but the runtime fix described in the title and changeset is not present. The new test fails because packages/effect/src/internal/core.ts was not modified.
Reviewed changes
- Added a changeset claiming
catchCausenow delivers interrupt causes in interruptible fibers. - Added a new test in
packages/effect/test/Effect.test.tsthat reproduces the bug.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
The latest commit (94e6012) is identical in content to the previously reviewed commit and still does not contain the runtime fix in packages/effect/src/internal/core.ts. The prior feedback stands: please implement the exitFailCause loop change so the new test can pass.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
94e6012 to
2b176b1
Compare
effect-slopcop
Bot
added
3.0
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
- Removed the
.changeset/tricky-bugs-check.mdthat claimed a runtime fix was shipped. - Updated the PR description to clarify that this is a test-only regression repro with no implementation fix.
- Added the
"receives interrupt causes on interruption"regression test inside thecatchCausedescribe block.
The prior CHANGES_REQUESTED feedback is addressed by narrowing the scope to a failing regression test. One small formatting fix is still needed before merge.
Note: the PR title still says "Fix ..." while the body now says the change is test-only. Consider aligning the title with the actual scope (e.g., "Add regression test for ...").
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
The fix delivers the interrupt cause to catchCause/matchCauseEffect legacy handlers, but in the pop loop the handler's returned Effect is discarded (only its synchronous closure runs). So the most common handler idiom — catchCause(cause => Effect.log(cause)) — constructs the effect and never evaluates it; only inline mutations (like the new test's caught = cause) are observed. Please confirm this matches what EFF-571 needs: if handlers should actually react to the interruption, the continuation's result must flow back into the run loop instead of being dropped.
Reviewed changes
- Implemented the runtime fix in
exitFailCause(packages/effect/src/internal/core.ts): the interrupt-unwind pop loop now invokes each skipped failure continuation soOnFailure/OnSuccessAndFailurehandlers receive the interruption cause. - Added the
catch-cause-interruptspatch changeset foreffect. - Rewrote the regression test in
packages/effect/test/Effect.test.tsto capture and assert the delivered cause (Cause.hasInterruptsOnly), replacing the earlier boolean flag. - Updated
packages/effect/test/cluster/ClusterWorkflowEngine.test.ts:catchCauseis now expected to run on the suspension interrupt.
Prior Pullfrog threads (test assertion form and dprint) are resolved by this commit.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| } | ||
| let cont = fiber.getCont(contE) | ||
| while (fiber.interruptible && fiber._interruptedCause && cont) { | ||
| cont[contE](cause, fiber, annotated ? undefined : this) |
There was a problem hiding this comment.
During interruption, this loop invokes each delivery target's [contE] but throws away its return value, then pops the next continuation. For a bare catchCause/matchCauseEffect (no contAll finalizer on top), fiber.interruptible stays true and the loop calls the handler, then discards the Effect it returns and ends in fiber.yieldWith(this). So Effect.catchCause(cause => Effect.log(cause)) — the common idiom — has its closure run but the log effect is never evaluated; only inline synchronous mutations (like the new test's caught = cause) are observable. If EFF-571 is about letting users observe/record the interruption, this doesn't deliver that for effect-returning handlers. If instead handlers must not act at all during interruption, consider whether invoking them at all (with side-effect-free result) is the intended contract, and document it.
| expect(flags.get("ensuring")).toBeTruthy() | ||
| expect(flags.get("catchCause")).toBeFalsy() | ||
| // catchCause receives the suspension interrupt | ||
| expect(flags.get("catchCause")).toBeTruthy() |
There was a problem hiding this comment.
This flip (and the one at line 75) is the observable contract change this PR ships. It's consistent with the fix (the workflow's catchCause handler at line 518 does a synchronous flags.set, which survives the discarded-return semantics), just noting it since a behavior-sensitive reviewer may want it called out in the changeset as more than a one-line fix description.
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
Effect.catchCauseeffectTesting
pnpm test --run --project effect --silent=passed-only --reporter=dotpnpm --filter effect checkpnpm dprint check packages/effect/src/internal/core.ts packages/effect/test/Effect.test.ts packages/effect/test/cluster/ClusterWorkflowEngine.test.ts .changeset/catch-cause-interrupts.mdpnpm oxlint packages/effect/src/internal/core.ts packages/effect/test/Effect.test.ts packages/effect/test/cluster/ClusterWorkflowEngine.test.tsCloses EFF-571