Fix failed ResourceMap lookups leak acquisition scopes by fubhy · Pull Request #7066 · Effect-TS/effect

fubhy · 2026-08-05T21:42:45Z

Summary

Close each keyed resource's private acquisition scope when its lookup fails, so finalizers registered before the failure run immediately. Includes a focused regression test and patch changeset.

Failed ResourceMap lookups leak acquisition scopes

Module: effect/unstable/cluster/internal/resourceMap
Audit ID: effect-fb55d74159c877b4
Severity / confidence: medium / high

What happens

When a new keyed resource acquisition fails, its entry is removed from the map but its private acquisition scope is left open. Finalizers registered before the failure do not run.

Why it happens

For a missing key, get creates a closeable scope and stores it with a deferred. Its failure onExit branch deletes the entry and completes the deferred but never calls Scope.close. Because the parent finalizer closes only scopes still present in the backing map, deleting the failed entry also removes the only later cleanup path.

Expected behavior

Every per-key scope created by ResourceMap.get must be closed exactly once: by removal or map shutdown after a successful acquisition, or immediately when acquisition fails before the entry becomes usable.

Relevant implementation

These links and excerpts are pinned to audit base 17f0b91a243ccfe4a38d27debdc983adf434e738.

packages/effect/src/unstable/cluster/internal/resourceMap.ts:68-77

View problematic code at packages/effect/src/unstable/cluster/internal/resourceMap.ts:68-77

      const scope = Effect.runSync(Scope.make())
      const deferred = Deferred.makeUnsafe<A, E>()
      backingSet(this.entries, key, { scope, deferred })
      return Effect.onExit(this.lookup(key, scope), (exit) => {
        if (exit._tag === "Success") {
          return Deferred.done(deferred, exit)
        }
        backingDelete(this.entries, key)
        return Deferred.done(deferred, exit)
      })

View exact lines on GitHub

Reproduction

pnpm test --run packages/effect/test/cluster/ResourceMap.test.ts

Observed failure: Focused contract assertion failed against 17f0b91, demonstrating: Failed ResourceMap lookups leak acquisition scopes.

Implementation

The failure branch now completes the shared deferred and closes the per-key scope with the acquisition exit. The regression test verifies that finalizers registered before a failed acquisition run immediately.

Validation

pnpm test --run packages/effect/test/cluster/ResourceMap.test.ts
pnpm test --run packages/effect/test/cluster
pnpm lint
pnpm check

Audit provenance

Audit base: 17f0b91a243ccfe4a38d27debdc983adf434e738
Reproduction base: 17f0b91a243ccfe4a38d27debdc983adf434e738
Findings: effect-fb55d74159c877b4
Initial patch: focused reproduction test; implementation fix included in this PR

Closes EFF-486

changeset-bot · 2026-08-05T21:42:49Z

🦋 Changeset detected

Latest commit: bd93b0b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages

Name	Type
effect	Patch
@effect/ai-anthropic	Patch
@effect/ai-openai	Patch
@effect/ai-openai-compat	Patch
@effect/ai-openrouter	Patch
@effect/atom-react	Patch
@effect/atom-solid	Patch
@effect/atom-vue	Patch
@effect/docgen	Patch
@effect/doctest	Patch
@effect/openapi-generator	Patch
@effect/opentelemetry	Patch
@effect/platform-browser	Patch
@effect/platform-bun	Patch
@effect/platform-deno	Patch
@effect/platform-node	Patch
@effect/platform-node-shared	Patch
@effect/sql-clickhouse	Patch
@effect/sql-d1	Patch
@effect/sql-libsql	Patch
@effect/sql-mssql	Patch
@effect/sql-mysql2	Patch
@effect/sql-pg	Patch
@effect/sql-pglite	Patch
@effect/sql-sqlite-bun	Patch
@effect/sql-sqlite-do	Patch
@effect/sql-sqlite-node	Patch
@effect/sql-sqlite-react-native	Patch
@effect/sql-sqlite-wasm	Patch
@effect/vitest	Patch

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

Important

The PR title and commit message say the bug is fixed, but the diff currently adds only the regression test. Either the implementation fix should be included before merge, or the title/description should be updated to reflect that this is the test-only first step.

Reviewed changes

The PR adds a single failing regression test in packages/effect/test/cluster/ResourceMap.test.ts that demonstrates leaked acquisition scopes when a ResourceMap lookup fails after registering a finalizer. Running pnpm test --run packages/effect/test/cluster/ResourceMap.test.ts fails as expected with finalized being 0 instead of 1.

packages/effect/test/cluster/ResourceMap.test.ts:1-15 — new regression test

⚠️ PR scope / title mismatch

The diff is test-only, while the title and commit subject read as a complete fix. If the intent is to land the fix in a follow-up commit on this branch, update the PR title/body so reviewers and CI don't treat this as a finished fix. If the fix is meant to be here, it is missing.

⚠️ Reproduction command points to the wrong file

The PR body tells reviewers to run:

pnpm test --run packages/effect/test/cluster/ResourceMapFailedLookupScope.test.ts

but the committed file is ResourceMap.test.ts. Update the command in the PR description.

ℹ️ Nitpicks

The new test file is not grouped under a describe(...) block; the repo convention is to group related tests with describe.
The lookup parameter is named _key, which is fine, but key is also acceptable.

^{｜ ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏}

pullfrog

✅ No new issues found.

Reviewed changes

The latest commit adds the missing implementation fix, resolves the prior review's scope/title mismatch, and addresses the test organization nit.

Implemented scope cleanup on failed lookups — packages/effect/src/unstable/cluster/internal/resourceMap.ts:75-79 now closes the per-key acquisition scope in the failure branch after completing the deferred and removing the map entry.
Added a patch changeset — .changeset/close-failed-resource-map-scopes.md describes the runtime fix.
Organized the regression test — packages/effect/test/cluster/ResourceMap.test.ts is now wrapped in describe("ResourceMap", ...) per repo convention.

Validation run against e8c3549d:

pnpm --filter effect test --run test/cluster/ResourceMap.test.ts passed
pnpm lint-fix passed
pnpm check passed

^{｜ ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏}

tim-smart · 2026-08-05T23:50:10Z

This should use Scope.makeUnsafe too

pullfrog

✅ No new issues found.

Reviewed changes

The latest commit replaces the synchronous effect runner used to create per-key scopes with the direct unsafe scope constructor.

Switched to Scope.makeUnsafe() — packages/effect/src/unstable/cluster/internal/resourceMap.ts:68 now creates the per-key acquisition scope directly instead of through Effect.runSync(Scope.make()), which is equivalent behavior without the synchronous effect execution overhead.

Validation run against bd93b0b:

pnpm --filter effect test --run test/cluster/ResourceMap.test.ts passed
pnpm lint-fix passed
pnpm check passed

^{｜ ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏}

github-actions · 2026-08-06T00:58:20Z

Bundle Size Analysis

Generated from PR build output; treat the content below as untrusted.

File Name	Current Size	Previous Size	Difference
`basic.ts`	7.06 KB	7.06 KB	0.00 KB (0.00%)
`batching.ts`	9.86 KB	9.86 KB	0.00 KB (0.00%)
`brand.ts`	6.34 KB	6.34 KB	0.00 KB (0.00%)
`cache.ts`	10.71 KB	10.71 KB	0.00 KB (0.00%)
`config.ts`	20.73 KB	20.73 KB	0.00 KB (0.00%)
`differ.ts`	20.31 KB	20.31 KB	0.00 KB (0.00%)
`http-client.ts`	21.53 KB	21.53 KB	0.00 KB (0.00%)
`logger.ts`	10.84 KB	10.84 KB	0.00 KB (0.00%)
`metric.ts`	8.98 KB	8.98 KB	0.00 KB (0.00%)
`optic.ts`	7.18 KB	7.18 KB	0.00 KB (0.00%)
`pubsub.ts`	14.99 KB	14.99 KB	0.00 KB (0.00%)
`queue.ts`	11.66 KB	11.66 KB	0.00 KB (0.00%)
`schedule.ts`	10.83 KB	10.83 KB	0.00 KB (0.00%)
`schema-class.ts`	19.27 KB	19.27 KB	0.00 KB (0.00%)
`schema-fromJsonSchemaDocument.ts`	29.09 KB	29.09 KB	0.00 KB (0.00%)
`schema-representation-roundtrip.ts`	25.40 KB	25.40 KB	0.00 KB (0.00%)
`schema-string-transformation.ts`	13.42 KB	13.42 KB	0.00 KB (0.00%)
`schema-string.ts`	10.95 KB	10.95 KB	0.00 KB (0.00%)
`schema-template-literal.ts`	15.21 KB	15.21 KB	0.00 KB (0.00%)
`schema-toArbitraryLazy.ts`	22.02 KB	22.05 KB	-0.03 KB (-0.12%)
`schema-toCodeDocument.ts`	24.45 KB	24.45 KB	0.00 KB (0.00%)
`schema-toCodecJson.ts`	19.28 KB	19.28 KB	0.00 KB (0.00%)
`schema-toEquivalence.ts`	19.11 KB	19.11 KB	0.00 KB (0.00%)
`schema-toFormatter.ts`	18.97 KB	18.97 KB	0.00 KB (0.00%)
`schema-toJsonSchemaDocument.ts`	22.69 KB	22.69 KB	0.00 KB (0.00%)
`schema-toRepresentation.ts`	19.60 KB	19.60 KB	0.00 KB (0.00%)
`schema.ts`	18.52 KB	18.52 KB	0.00 KB (0.00%)
`stm.ts`	12.63 KB	12.63 KB	0.00 KB (0.00%)
`stream.ts`	9.80 KB	9.80 KB	0.00 KB (0.00%)

Fix failed ResourceMap lookups leak acquisition scopes

08d32eb

fubhy added the audit Findings originating from the Effect runtime correctness audit label Aug 5, 2026

pullfrog Bot requested changes Aug 5, 2026

View reviewed changes

Fix failed ResourceMap lookup scope cleanup

e8c3549

pullfrog Bot approved these changes Aug 5, 2026

View reviewed changes

tim-smart reviewed Aug 5, 2026

View reviewed changes

Use unsafe scope constructor in ResourceMap

bd93b0b

tim-smart enabled auto-merge (squash) August 6, 2026 00:22

pullfrog Bot approved these changes Aug 6, 2026

View reviewed changes

tim-smart merged commit 24e22d2 into main Aug 6, 2026
20 checks passed

tim-smart deleted the audit/repro-17f0b91a-resource-map-failed-lookup-scope branch August 6, 2026 00:48

Navigation Menu

Uh oh!

Uh oh!

Fix failed ResourceMap lookups leak acquisition scopes - #7066

Fix failed ResourceMap lookups leak acquisition scopes#7066
tim-smart merged 3 commits into
mainfrom
audit/repro-17f0b91a-resource-map-failed-lookup-scope

fubhy commented Aug 5, 2026 •

edited by tim-smart

Loading

Uh oh!

changeset-bot Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

pullfrog Bot left a comment

Uh oh!

pullfrog Bot left a comment

Uh oh!

tim-smart Aug 5, 2026

Uh oh!

pullfrog Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

fubhy commented Aug 5, 2026 • edited by tim-smart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Failed ResourceMap lookups leak acquisition scopes

What happens

Why it happens

Expected behavior

Relevant implementation

Reproduction

Implementation

Validation

Audit provenance

Uh oh!

changeset-bot Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

⚠️ PR scope / title mismatch

⚠️ Reproduction command points to the wrong file

ℹ️ Nitpicks

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

tim-smart Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Aug 6, 2026

Bundle Size Analysis

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fubhy commented Aug 5, 2026 •

edited by tim-smart

Loading

changeset-bot Bot commented Aug 5, 2026 •

edited

Loading