Skip to content

fix(effect-ts): await Effect cleanup during Effection scope teardown #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
taras merged 3 commits into from
Mar 16, 2026
Merged

fix(effect-ts): await Effect cleanup during Effection scope teardown #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
daeef22
fix(effect-ts): await Effect cleanup during Effection scope teardown
taras Mar 6, 2026
e35b091
🐛 fix(effect-ts): resolve runExit with Exit.die instead of rejecting
taras Mar 14, 2026
5bdc517
Merge branch 'main' into fix/effect-ts-cancellation-with-alpha
taras Mar 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 67 additions & 14 deletions effect-ts/effect-runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { type Effect, type Exit, Layer, ManagedRuntime } from "effect";
import { type Operation, action, call, resource } from "effection";
import { type Effect, Exit, Layer, ManagedRuntime } from "effect";
import { type Operation, action, resource, until } from "effection";

/**
* A runtime for executing Effect programs inside Effection operations.
Expand Down Expand Up @@ -103,34 +103,87 @@ export function makeEffectRuntime<R = never>(
layer ?? Layer.empty,
) as ManagedRuntime.ManagedRuntime<R, never>;

interface PendingExecution {
abort: () => void;
settled: Promise<void>;
}

const pending = new Set<PendingExecution>();

function startManaged<T>(runPromise: (signal: AbortSignal) => Promise<T>) {
const controller = new AbortController();
let done = false;

const execution = {
abort: () => {
if (!done) {
controller.abort();
}
},
settled: Promise.resolve(),
} as PendingExecution;

const promise = runPromise(controller.signal);

execution.settled = promise
.then(
() => undefined,
() => undefined,
)
.finally(() => {
done = true;
pending.delete(execution);
});

pending.add(execution);
Comment on lines +126 to +138
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Harden startManaged() against synchronous launcher failures.

The tracking invariant here still depends on the launcher never throwing before pending.add(). Register first and normalize the call through Promise.resolve().then(...) so sync failures stay on the same settled cleanup path.

🛠️ Proposed fix 
-      const promise = runPromise(controller.signal);
+      pending.add(execution);
+      const promise = Promise.resolve().then(() =>
+        runPromise(controller.signal),
+      );

       execution.settled = promise
         .then(
           () => undefined,
           () => undefined,
         )
         .finally(() => {
           done = true;
           pending.delete(execution);
         });

-      pending.add(execution);
-
       return { promise, abort: execution.abort, signal: controller.signal };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const promise = runPromise(controller.signal);
execution.settled = promise
.then(
() => undefined,
() => undefined,
)
.finally(() => {
done = true;
pending.delete(execution);
});
pending.add(execution);
pending.add(execution);
const promise = Promise.resolve().then(() =>
runPromise(controller.signal),
);
execution.settled = promise
.then(
() => undefined,
() => undefined,
)
.finally(() => {
done = true;
pending.delete(execution);
});
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@effect-ts/effect-runtime.ts` around lines 126 - 138, Register the execution
in pending before invoking the launcher and normalize synchronous throws by
wrapping the launcher call in Promise.resolve().then(...): move
pending.add(execution) to before creating the promise, create promise as
Promise.resolve().then(() => runPromise(controller.signal)) (so sync exceptions
become rejections), then set execution.settled = promise.then(() => undefined,
() => undefined).finally(() => { done = true; pending.delete(execution); });
Apply this change inside startManaged so runPromise, execution,
execution.settled, pending, and controller.signal are used as described.


return { promise, abort: execution.abort, signal: controller.signal };
}

const run: EffectRuntime<R>["run"] = <A, E>(
effect: Effect.Effect<A, E, R>,
) => {
return action<A>((resolve, reject) => {
const controller = new AbortController();
managedRuntime
.runPromise(effect, { signal: controller.signal })
.then(resolve, reject);
return () => controller.abort();
const { promise, abort, signal } = startManaged((signal) =>
managedRuntime.runPromise(effect, { signal }),
);

promise.then(resolve, (error) => {
if (!signal.aborted) {
reject(error);
}
});
return abort;
});
};

const runExit: EffectRuntime<R>["runExit"] = <A, E>(
effect: Effect.Effect<A, E, R>,
) => {
return action<Exit.Exit<A, E>>((resolve, reject) => {
const controller = new AbortController();
managedRuntime
.runPromiseExit(effect, { signal: controller.signal })
.then(resolve, reject);
return () => controller.abort();
return action<Exit.Exit<A, E>>((resolve, _reject) => {
const { promise, abort, signal } = startManaged((signal) =>
managedRuntime.runPromiseExit(effect, { signal }),
);

promise.then(resolve, (error) => {
if (!signal.aborted) {
resolve(Exit.die(error) as Exit.Exit<A, E>);
}
});
return abort;
});
};

try {
yield* provide({ run, runExit });
} finally {
yield* call(() => managedRuntime.dispose());
const active = Array.from(pending);
for (const execution of active) {
execution.abort();
}

yield* until(Promise.all(active.map((execution) => execution.settled)));
yield* until(managedRuntime.dispose());
Comment on lines +180 to +186
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Document this teardown guarantee explicitly.

This now makes scope exit wait for interrupted Effect executions to finish cleanup before disposing the ManagedRuntime. Please capture that in the makeEffectRuntime() docs so callers know teardown is cleanup-complete, not just abort-triggered. As per coding guidelines: document lifecycle/cancellation semantics.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@effect-ts/effect-runtime.ts` around lines 180 - 186, The documentation for
makeEffectRuntime() must explicitly state that teardown waits for interrupted
Effect executions to finish cleanup (not merely trigger abort); update the
makeEffectRuntime() docs to describe that when pending executions are aborted
(via execution.abort()) the runtime yields until all execution.settled promises
resolve and only then calls managedRuntime.dispose(), so callers understand
teardown is cleanup-complete and the lifecycle/cancellation semantics guarantee.

}
});
}
2 changes: 1 addition & 1 deletion effect-ts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@effectionx/effect-ts",
"description": "Bidirectional interop between Effect-TS and Effection",
"version": "0.1.2",
"version": "0.1.4",
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check if version 0.1.3 was ever published or tagged

# Check npm registry for published versions
echo "=== Checking npm registry for `@effectionx/effect-ts` versions ==="
npm view `@effectionx/effect-ts` versions --json 2>/dev/null || echo "Package not found in npm registry or not accessible"

# Check git tags for version 0.1.3
echo -e "\n=== Checking git tags for effect-ts@0.1.3 ==="
git tag -l | grep -E 'effect-ts.*0\.1\.3|@effectionx/effect-ts.*0\.1\.3' || echo "No tags found for version 0.1.3"

# Check git log for version bump commits
echo -e "\n=== Checking recent version bump commits in effect-ts/package.json ==="
git log --oneline --all -20 -- effect-ts/package.json

Repository: thefrontside/effectionx

Length of output: 844

Correct the version to 0.1.4 or clarify the double-patch bump.

The version 0.1.3 was never published to npm or tagged in git history. Since this PR contains a single bug fix (race condition in effect-runtime.ts), semantic versioning requires only a single patch bump: 0.1.2 → 0.1.3. Verify that 0.1.4 is intentional (e.g., another version bump is planned in this PR), otherwise change the version to 0.1.3.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@effect-ts/package.json` at line 4, The package.json version was set to 0.1.4
but the PR only fixes a single bug (race condition in effect-runtime.ts) and
0.1.3 was never published, so change the "version" field in package.json from
"0.1.4" to "0.1.3" unless you intentionally planned a double-patch bump; if the
bump to 0.1.4 is deliberate, add a short note in the PR describing why two patch
bumps are needed.

"keywords": ["effection", "effectionx", "interop", "effect-ts", "effect"],
"type": "module",
"main": "./dist/mod.js",
Expand Down
Loading