Skip to content

🐛 Run scope destructors in reverse order of registration#1215

Open
taras wants to merge 1 commit into
v4from
fix/lifo-scope-destructors
Open

🐛 Run scope destructors in reverse order of registration#1215
taras wants to merge 1 commit into
v4from
fix/lifo-scope-destructors

Conversation

@taras

@taras taras commented Jul 15, 2026

Copy link
Copy Markdown
Member

Motivation

I noticed this problem in TestKit using resources that wrap Playwright and using daemon from @effectionx/process. I didn't have a chance to figure out what it was while I was working on Effection, but I was able to narrow it down for a problem I had with executable.md.

The problem occurs when async teardown expect another value to be present that was setup by an earlier resource. This dependency chain requires that the dependant resource is torn down before the dependency. This seems to work correctly when the scope runs to completion, but not when the scope is destroyed explicitly.

Approach

Invoke teardown operations in reverse (last first) to ensure that dependent teardown are invoked before the dependency. This makes explicit destroy() consistent with TaskGroup.halt() and normal task unwinding, which already tear down last-in, first-out.

@pkg-pr-new

pkg-pr-new Bot commented Jul 15, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/effection@1215

commit: 362c879

@taras

taras commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

Scope of the bug — this is a v4 regression from v3, and it affects the current stable release (4.0.3 = npm latest), not just the 4.1 alphas.

Both regression scenarios from this PR, run against the published packages:

version teardown order drain protocol
effection@3 ["third","second","first"] — reverse, correct destroy() completes
effection@4.0.3 (latest) ["first","second","third"] — forward destroy() deadlocks
effection@4.1.0-alpha.9 (next) forward ❌ deadlocks

v3's frame-based teardown ran finalizers in reverse; the forward-order destructor set came in with the v4 scope architecture. Given 4.0.3 is latest, this fix is probably worth a backport to the 4.0.x line as well.

taras added a commit to taras/executable.md that referenced this pull request Jul 15, 2026
…rop vendor fork

Upgrade the @effectionx/* packages to the coordinated 0.8-era release set,
pinned exact in both package.json and deno.json so Deno and pnpm resolve
identical versions (this also removes the @effectionx/node drift where pnpm
resolved 0.2.3 but Deno 0.2.2):

  converge 0.1.4, fetch 0.2.0, fs 0.3.0, middleware 0.1.1, node 0.2.4,
  process 0.8.1, scope-eval 0.1.3, stream-helpers 0.8.3, test-adapter 0.7.4,
  timebox 0.4.3

Replace the vendored @effectionx/context-api fork (0.5.0-vendored) with the
published 0.6.0, which ships the nested-scope .around() middleware behavior
({ at: "min" } innermost-wins) that the fork was created for. Remove
vendor/context-api from every workspace list, import map, and tsconfig path,
and stop passing vendor/ to oxfmt.

effection stays pinned at 4.1.0-alpha.7: alpha.9 has a scope-teardown
regression (scope destructors run in registration order instead of reverse,
deadlocking @effectionx/process's kill+drain teardown) that is fixed upstream
by thefrontside/effection#1215 but not yet released.
The other alpha.9 blocker — effect.enter() errors becoming uncatchable — is
already fixed on the v4-1-alpha branch (#1179). Once a release ships with
both, bumping the pin is a one-line change.
@taras
taras force-pushed the fix/lifo-scope-destructors branch from 2407841 to 5dddf39 Compare July 15, 2026 13:04
@taras
taras changed the base branch from v4-1-alpha to v4 July 15, 2026 13:04
@taras
taras force-pushed the fix/lifo-scope-destructors branch from 5dddf39 to 881b83c Compare July 15, 2026 13:18
@codspeed-hq

codspeed-hq Bot commented Jul 15, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 6 untouched benchmarks


Comparing fix/lifo-scope-destructors (362c879) with v4 (e4ab04d)

Open in CodSpeed

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🚀 Deploy Preview Ready!

@taras
taras force-pushed the fix/lifo-scope-destructors branch from 881b83c to 542c633 Compare July 15, 2026 13:37
Scope teardown had two pathways with opposite orders: TaskGroup.halt()
halts tasks in reverse order of creation (and resources are documented
to be released in reverse order of acquisition), but a scope's
destructor set ran forward, in insertion order. Which order you got
depended on which pathway reached the task first.

The forward order breaks any teardown protocol where a later-registered
finalizer needs earlier-registered tasks to still be alive. The
real-world casualty is @effectionx/process: its kill+drain ensure()
(registered last, so that it runs first) waits on resolvers that only
the stdout/stderr reader tasks (registered earlier) can resolve. When
teardown arrives via the scope-destroy cascade, the readers were
destroyed *before* the drain ran, the resolvers could never resolve,
and destroy() deadlocked with "Promise resolution is still pending but
the event loop has already resolved."

Iterate the destructor set in reverse so teardown is always the mirror
image of setup, consistent with TaskGroup.halt().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant