Skip to content

refactor effect runtime#18336

Open
tim-smart wants to merge 2 commits intoanomalyco:devfrom
tim-smart:effect-runtime
Open

refactor effect runtime#18336
tim-smart wants to merge 2 commits intoanomalyco:devfrom
tim-smart:effect-runtime

Conversation

@tim-smart
Copy link
Contributor

No description provided.

@github-actions github-actions bot added contributor needs:compliance This means the issue will auto-close after 2 hours. needs:title labels Mar 20, 2026
@github-actions
Copy link
Contributor

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

  • PR description is missing required template sections. Please use the PR template.

Please edit this PR description to address the above within 2 hours, or it will be automatically closed.

If you believe this was flagged incorrectly, please let a maintainer know.

@github-actions
Copy link
Contributor

Hey! Your PR title refactor effect runtime doesn't follow conventional commit format.

Please update it to start with one of:

  • feat: or feat(scope): new feature
  • fix: or fix(scope): bug fix
  • docs: or docs(scope): documentation changes
  • chore: or chore(scope): maintenance tasks
  • refactor: or refactor(scope): code refactoring
  • test: or test(scope): adding or updating tests

Where scope is the package name (e.g., app, desktop, opencode).

See CONTRIBUTING.md for details.

@tim-smart
Copy link
Contributor Author

@kitlangton fyi was playing with the legos

@github-actions
Copy link
Contributor

The following comment was made by an LLM, it may be inaccurate:

Based on my search, I found several related PRs that are part of the same refactoring initiative:

Related PRs (not duplicates, but part of the same effort):

  1. PR effectify LSP service #18321 - "effectify LSP service" - effectify LSP service #18321
  2. PR effectify Pty service #18319 - "effectify Pty service" - effectify Pty service #18319
  3. PR effectify SessionStatus service #18313 - "effectify SessionStatus service" - effectify SessionStatus service #18313
  4. PR effectify Installation service, drop Effect suffix from namespaces #18266 - "effectify Installation service, drop Effect suffix from namespaces" - effectify Installation service, drop Effect suffix from namespaces #18266
  5. PR feat(bus): migrate Bus to Effect service with PubSub #18173 - "feat(bus): migrate Bus to Effect service with PubSub" - feat(bus): migrate Bus to Effect service with PubSub #18173

These PRs are all focused on migrating services to use the Effect library, which appears to be a coordinated refactoring effort related to the effect runtime changes in PR #18336. However, none of these are duplicates - they each address specific services being effectified.

@github-actions
Copy link
Contributor

This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window.

Feel free to open a new pull request that follows our guidelines.

@github-actions github-actions bot removed the needs:compliance This means the issue will auto-close after 2 hours. label Mar 20, 2026
@github-actions github-actions bot closed this Mar 20, 2026
@kitlangton
Copy link
Contributor

Vouch @tim-smart

@kitlangton kitlangton reopened this Mar 20, 2026
@kitlangton
Copy link
Contributor

kitlangton commented Mar 20, 2026

Ah. The problem with this is that we have two tiers of services:

  1. Global services that should only ever be instantiated once per app.
  2. Instance (per directory) services that should be instantiated and shared once per instance (until that instance is disposed, at which point they'd be shutdown/reacquired until the next disposal).

ManagedRuntime does what we need for the Global services and LayerMap basically does what we need for the instance services (minus the forceful interruption on invalidation -- hence my RcMap PR -- but I was able to implement a forceful interruption in a draft PR I have here).

The problem with this approach is that requirement numero 2 is not met, as it will potentially instantiate an instance service multiple times, say if it's depended upon by a second instance service layer.

I added these tests to this branch locally and alas "A reuses the already-running B from the same directory" fails.

import { afterEach, describe, expect, test } from "bun:test"
import { Effect, Layer } from "effect"
import * as ServiceMap from "effect/ServiceMap"
import {
  disposeAllRuntimes,
  disposeInstanceRuntimes,
  makeRuntimeGlobal,
  makeRuntimeInstance,
} from "../../src/effect/runtime"
import { Instance } from "../../src/project/instance"
import { tmpdir } from "../fixture/fixture"

/**
 * These tests describe the DESIRED lifecycle behaviour of the runtime system.
 *
 * We model three services:
 *
 * - `Global`: process-wide, shared across all directories
 * - `B`: instance-scoped, depends on `Global`
 * - `A`: instance-scoped, depends on both `B` and `Global`
 *
 * Every layer increments an acquire counter when it is built and a release
 * counter when its runtime is disposed. That lets each test describe not just
 * the final value, but the lifecycle path that got us there.
 *
 * The key invariant: within a single directory, instance services that appear
 * as dependencies of other instance services must be instantiated ONCE and
 * shared — never rebuilt inside a second runtime.
 */
function mk() {
  const seen = {
    globalStart: 0,
    globalEnd: 0,
    bStart: 0,
    bEnd: 0,
    aStart: 0,
    aEnd: 0,
  }

  let id = 0
  const next = () => {
    id += 1
    return id
  }

  class Global extends ServiceMap.Service<Global, { readonly id: number }>()("@test/runtime/global") {}
  class B extends ServiceMap.Service<B, { readonly id: number; readonly global: number }>()("@test/runtime/b") {}
  class A extends ServiceMap.Service<A, { readonly id: number; readonly b: number; readonly global: number }>()(
    "@test/runtime/a",
  ) {}

  const globalLayer = Layer.effect(
    Global,
    Effect.acquireRelease(
      Effect.sync(() => {
        seen.globalStart += 1
        return Global.of({ id: next() })
      }),
      () =>
        Effect.sync(() => {
          seen.globalEnd += 1
        }),
    ),
  )

  const bCore = Layer.effect(
    B,
    Effect.gen(function* () {
      const global = yield* Global
      return yield* Effect.acquireRelease(
        Effect.sync(() => {
          seen.bStart += 1
          return B.of({ id: next(), global: global.id })
        }),
        () =>
          Effect.sync(() => {
            seen.bEnd += 1
          }),
      )
    }),
  )

  const bLayer = bCore.pipe(Layer.provide(globalLayer))

  const aCore = Layer.effect(
    A,
    Effect.gen(function* () {
      const global = yield* Global
      const b = yield* B
      return yield* Effect.acquireRelease(
        Effect.sync(() => {
          seen.aStart += 1
          return A.of({ id: next(), b: b.id, global: global.id })
        }),
        () =>
          Effect.sync(() => {
            seen.aEnd += 1
          }),
      )
    }),
  )

  const aLayer = aCore.pipe(Layer.provide(bLayer), Layer.provide(globalLayer))
  const memo = Layer.makeMemoMapUnsafe()

  return {
    seen,
    global: makeRuntimeGlobal(Global, globalLayer, memo),
    b: makeRuntimeInstance(B, bCore.pipe(Layer.fresh, Layer.provide(globalLayer)), memo),
    a: makeRuntimeInstance(A, aCore.pipe(Layer.fresh, Layer.provide(bLayer), Layer.provide(globalLayer)), memo),
  }
}

describe("effect/runtime", () => {
  afterEach(async () => {
    await disposeAllRuntimes()
  })

  test("global services are instantiated exactly once", async () => {
    const rt = mk()

    const first = await rt.global.runPromise((svc) => Effect.succeed(svc.id))
    const second = await rt.global.runPromise((svc) => Effect.succeed(svc.id))

    expect(first).toBe(second)
    expect(rt.seen.globalStart).toBe(1)
    expect(rt.seen.globalEnd).toBe(0)

    await disposeAllRuntimes()

    expect(rt.seen.globalEnd).toBe(1)
  })

  test("instance services are reused within a directory but isolated across directories", async () => {
    const rt = mk()
    await using one = await tmpdir()
    await using two = await tmpdir()

    const first = await Instance.provide({
      directory: one.path,
      fn: async () => {
        const a = await rt.b.runPromise((svc) => Effect.succeed({ id: svc.id, global: svc.global }))
        const b = await rt.b.runPromise((svc) => Effect.succeed({ id: svc.id, global: svc.global }))
        expect(a).toEqual(b)
        return a
      },
    })

    expect(rt.seen.bStart).toBe(1)
    expect(rt.seen.globalStart).toBe(1)

    // Different directory → new B instance, but Global is still shared.
    const second = await Instance.provide({
      directory: two.path,
      fn: () => rt.b.runPromise((svc) => Effect.succeed({ id: svc.id, global: svc.global })),
    })

    expect(second.id).not.toBe(first.id)
    expect(second.global).toBe(first.global)
    expect(rt.seen.bStart).toBe(2)
    expect(rt.seen.globalStart).toBe(1)

    await disposeAllRuntimes()

    expect(rt.seen.bEnd).toBe(2)
    expect(rt.seen.globalEnd).toBe(1)
  })

  test("A reuses the already-running B from the same directory", async () => {
    const rt = mk()
    await using tmp = await tmpdir()

    await Instance.provide({
      directory: tmp.path,
      fn: async () => {
        // Step 1: build B's standalone runtime.
        const b = await rt.b.runPromise((svc) => Effect.succeed({ id: svc.id, global: svc.global }))

        expect(rt.seen.bStart).toBe(1)
        expect(rt.seen.globalStart).toBe(1)

        // Step 2: build A, which depends on B and Global.
        //
        // A should pick up the SAME B instance that is already running in this
        // directory — not rebuild it. This is the critical invariant.
        const a = await rt.a.runPromise((svc) => Effect.succeed({ id: svc.id, b: svc.b, global: svc.global }))

        expect(a.b).toBe(b.id)
        expect(a.global).toBe(b.global)
        expect(rt.seen.bStart).toBe(1) // B was NOT rebuilt
        expect(rt.seen.aStart).toBe(1)
        expect(rt.seen.globalStart).toBe(1)

        // Step 3: standalone B is still the same cached instance.
        const b2 = await rt.b.runPromise((svc) => Effect.succeed({ id: svc.id, global: svc.global }))

        expect(b2).toEqual(b)
      },
    })

    await disposeAllRuntimes()

    // One instance of each service, one release of each.
    expect(rt.seen.globalEnd).toBe(1)
    expect(rt.seen.bEnd).toBe(1)
    expect(rt.seen.aEnd).toBe(1)
  })

  test("instance disposal does not tear down global services", async () => {
    const rt = mk()
    await using tmp = await tmpdir()

    // Ensure global runtime is initialised first (mirrors real startup order).
    const globalId = await rt.global.runPromise((svc) => Effect.succeed(svc.id))

    expect(rt.seen.globalStart).toBe(1)

    await Instance.provide({
      directory: tmp.path,
      fn: async () => {
        const first = await rt.b.runPromise((svc) => Effect.succeed({ id: svc.id, global: svc.global }))

        expect(first.global).toBe(globalId)
        expect(rt.seen.bStart).toBe(1)
        expect(rt.seen.bEnd).toBe(0)

        // Disposing instance runtimes releases B but leaves Global alive.
        await disposeInstanceRuntimes(tmp.path)

        expect(rt.seen.bEnd).toBe(1)
        expect(rt.seen.globalEnd).toBe(0)

        // Re-entering rebuilds B; Global is still the same instance.
        const second = await rt.b.runPromise((svc) => Effect.succeed({ id: svc.id, global: svc.global }))

        expect(second.id).not.toBe(first.id)
        expect(second.global).toBe(first.global)
        expect(rt.seen.bStart).toBe(2)
        expect(rt.seen.globalStart).toBe(1)
      },
    })
  })
})

Were you mainly trying to reduce circular imports? Is there a possible issue with the LayerMap/ManagedRuntime approach that I'm missing? 😅

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants