Skip to content

Defer memoized Layer state installation until Effect execution - #6946

Open
fubhy wants to merge 1 commit into
mainfrom
audit/repro-core-layer-memo-laziness
Open

Defer memoized Layer state installation until Effect execution#6946
fubhy wants to merge 1 commit into
mainfrom
audit/repro-core-layer-memo-laziness

Conversation

@fubhy

@fubhy fubhy commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

Merely constructing a memoized layer build Effect inserts an entry into the memo map. If that Effect is abandoned, a later build can wait forever on its never-completed deferred and the child scope and entry are leaked.

Important

This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.

Memoized layer construction installs state eagerly

Module: Layer
Audit ID: core-g-r-layer-memo-entry-installed-eagerly
Severity / confidence: medium / high

What happens

Merely constructing a memoized layer build Effect inserts an entry into the memo map. If that Effect is abandoned, a later build can wait forever on its never-completed deferred and the child scope and entry are leaked.

Why it happens

getOrElseMemoize invokes memoMapBuild immediately. memoMapBuild creates a scope and deferred and inserts the memo entry before returning the Effect that would perform acquisition and install cleanup.

Expected behavior

Layer.buildWithMemoMap returns a lazy Effect; memo entries and acquisition state are established when that Effect executes, not when it is constructed.

Relevant implementation

These links and excerpts are pinned to audit base c9b56ab507f224426ee8388dc450da447ec4715f.

View problematic code at packages/effect/src/Layer.ts:380-418
export const fromBuildMemo = <ROut, E, RIn>(
  build: (
    memoMap: MemoMap,
    scope: Scope.Scope
  ) => Effect<Context.Context<ROut>, E, RIn>
): Layer<ROut, E, RIn> => {
  const self: Layer<ROut, E, RIn> = fromBuild((memoMap, scope) => memoMap.getOrElseMemoize(self, scope, build))
  return self
}

const memoMapBuild = <RIn, E, ROut>(
  memoMap: MemoMapImpl,
  layer: Layer<ROut, E, RIn>,
  scope: Scope.Scope,
  build: (memoMap: MemoMap, scope: Scope.Scope) => Effect<Context.Context<ROut>, E, RIn>
): Effect<Context.Context<ROut>, E, RIn> => {
  const layerScope = Scope.makeUnsafe()
  const deferred = Deferred.makeUnsafe<Context.Context<ROut>, E>()
  const entry: MemoMapEntry = {
    observers: 1,
    effect: Deferred.await(deferred),
    finalizer: (exit: Exit.Exit<unknown, unknown>) =>
      internalEffect.suspend(() => {
        entry.observers--
        if (entry.observers === 0) {
          memoMap.map.delete(layer)
          return Scope.close(layerScope, exit)
        }
        return internalEffect.void
      })
  }
  memoMap.map.set(layer, entry)
  return internalEffect.scopeAddFinalizerExit(scope, entry.finalizer).pipe(
    internalEffect.flatMap(() => build(memoMap, layerScope)),
    internalEffect.onExit((exit) => {
      entry.effect = exit
      return Deferred.done(deferred, exit)
    })
  )

View exact lines on GitHub

View problematic code at packages/effect/src/Layer.ts:445-455
  getOrElseMemoize<RIn, E, ROut>(
    layer: Layer<ROut, E, RIn>,
    scope: Scope.Scope,
    build: (memoMap: MemoMap, scope: Scope.Scope) => Effect<Context.Context<ROut>, E, RIn>
  ): Effect<Context.Context<ROut>, E, RIn> {
    const existing = this.get(layer, scope)
    if (existing) {
      return existing
    }
    return memoMapBuild(this, layer, scope, build)
  }

View exact lines on GitHub

Reproduction

pnpm vitest run packages/effect/test/Layer.test.ts -t "does not memoize a build before its Effect executes"

Observed failure: The intended failure was reproduced: the map size was 1 instead of 0; an independent public-API probe also observed a second build timing out.

Implementation handoff

The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.

  1. Start with the pinned implementation excerpts and the Why it happens analysis above.
  2. Change the implementation so it satisfies the stated Expected behavior; do not weaken or remove the reproduction assertions.
  3. Run the focused reproduction command(s) and confirm the observed failures become passing tests:
pnpm vitest run packages/effect/test/Layer.test.ts -t "does not memoize a build before its Effect executes"
  1. Run the affected package's existing tests, then the repository lint and type checks before requesting review.

Audit provenance

  • Audit base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Reproduction base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Findings: core-g-r-layer-memo-entry-installed-eagerly
  • Initial patch: focused reproduction tests; implementation fix pending

@fubhy fubhy added the audit Findings originating from the Effect runtime correctness audit label Aug 4, 2026
@github-project-automation github-project-automation Bot moved this to Discussion Ongoing in PR Backlog Aug 4, 2026
@changeset-bot

changeset-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: afea745

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@effect-slopcop effect-slopcop Bot added 4.0 bug Something isn't working labels Aug 4, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

This PR currently adds only the regression test; the implementation fix announced in the title is missing. CI will fail until that fix is included.

Reviewed changes

Reviewed packages/effect/test/Layer.test.ts (commit afea745). It adds one regression test under the MemoMap describe block that asserts Layer.buildWithMemoMap does not install a memo-map entry until the returned Effect is executed.

  • packages/effect/test/Layer.test.ts:455-463 — regression test for eager memo-entry installation.

⚠️ Implementation fix is missing

The regression test is well-targeted and confirmed failing locally (expected 1 to equal +0), which matches the PR description. However, the PR title claims the fix is included, while the branch only contains the failing test.

To resolve this before merging, add the deferred-installation implementation in packages/effect/src/Layer.ts so that memoMapBuild constructs the scope, deferred, and memo-map entry inside the returned Effect rather than during Layer build construction.

Pullfrog  | ⚠️ 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) | 𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.0 audit Findings originating from the Effect runtime correctness audit bug Something isn't working

Projects

Status: Discussion Ongoing

Development

Successfully merging this pull request may close these issues.

1 participant