Skip to content

refactor(daemon): move singletons into per-daemon layers - #57

Open
FreshlyBrewedCode wants to merge 13 commits into
36-agent-runtime-servicefrom
38-move-singletons-into-layers
Open

FreshlyBrewedCode wants to merge 13 commits into
36-agent-runtime-servicefrom
38-move-singletons-into-layers

Conversation

@FreshlyBrewedCode

@FreshlyBrewedCode FreshlyBrewedCode commented Sep 20, 2026 •

Copy link
Copy Markdown
Owner

Part of #32 · Closes #38

Four maps lived at module scope — server/runs.ts's active-run registry, server/pubsub.ts's subscribers, lib/dedupe.ts's exported dedupeRegistry, and lib/workspace.ts's refreshGates — which meant two daemons could not coexist in one process and every feature since #13 paid rent on it as another optional test-seam field (dedupeRegistry?, registry?, now?, beforeStart?). #36 gave the daemon a composition root to put them behind; this PR moves them there and retires the seams, without touching the one thing that must not move: the synchronous check-then-set that reserves a registry slot and claims a dedupe key (D24/D29, #15).

What changed

  • createRunRegistry(), createPubSub(), createDedupeRegistry() (already existed) and the new createRefreshGates() replace the four module-level singletons; each is a plain closure over a Map, not an Effect service.
  • DaemonServices (registry, pubsub, dedupeRegistry, refreshGates) is built once per startDaemon() call and threaded explicitly through startTrackedRun, dispatchChildRun, makeScheduleFire, and every HTTP handler in server/http.ts — no daemon reads another daemon's state.
  • dedupeRegistry?, registry?, now?, and beforeStart? are gone; tests build a DaemonServices bag (createTestServices()-style helpers) instead of injecting optional fields.
  • The scheduler's clock is now a Clock.Clock value (SchedulerDeps.clock), sourced from a createLiveClock() in daemon.ts for production and Effect's real TestClock (effect/testing) in scheduler.test.ts, rather than a bespoke now?: () => number.
  • src/server/two-daemons.test.ts is new: starts two full daemons (startDaemon) in one process, asserts independent run lists, cross-daemon 404s, independent registry.activeRunIds(), and that the same dedupe key succeeds on both daemons at once — proving the dedupe registries don't share state either.
  • Along the way: ConcurrencyLimitError/DispatchCapError picked up a mid-stack regression back to plain Error subclasses and were restored to Schema.TaggedError (matching Represent domain failures as Schema.TaggedError #34/base already), and DedupeKeyError gained a message getter plus a DispatchCollision.error field so the collision reason is readable from the event log, not just the typed fields.
  • Review follow-ups: dropped a 0-byte sandbox artifact (data/src/factory/.factory/workspaces/.../.tanstack-projected-...) that had leaked into an earlier commit, and switched scheduler.test.ts from a hand-rolled fake Clock.Clock to Effect's real TestClock.

Notes for reviewers

  • Atomicity of slot reservation + dedupe claim: in startTrackedRun (src/server/runs.ts), registry.get(runId), dedupeRegistry.claim(...), admitRun(...), and registry.reserve(runId) are all plain synchronous calls on plain Map-backed objects, with the first await (allocateWorkspace) only reached afterward, inside the try. No microtask boundary was introduced between check and set — this held for the pre-refactor module-level version and still holds now that the same Maps live behind services.registry/services.dedupeRegistry instead of at module scope, and still holds at this PR's current head (src/server/runs.ts was untouched by the review follow-ups). Verified by reading the resulting code path, not just the diff hunks.
  • TestClock: scheduler.test.ts now uses effect/testing's real TestClock instead of a hand-rolled fake. tickOnce/createSchedulerState stay deliberately plain async (ADR 0009 §5) and only ever read the clock synchronously via currentTimeMillisUnsafe() — they never suspend on Effect.sleep, so none of TestClock's fiber-coordination machinery (scheduled sleeps, the hung-test warning fiber) actually gets exercised. But building the clock once per fixture via Effect.runPromise(Effect.scoped(TestClock.make())) and bridging setTime back into the plain-async fixture (the same pattern ManagedRuntime uses for bridging Effect services into imperative code) composes cleanly with no contortion of tickOnce or production code, so there was no reason to keep the fake.
  • The stray committed artifact noted in an earlier revision of this PR (data/src/factory/.factory/workspaces/run-69780571-.../.tanstack-projected-1e95f9272dfe038f, leaked in 2f97451, same failure mode refactor(runtime): move headless permission setup into the agent adapter #55 hit) has been removed via git rm; .gitignore already covered data/ and .tanstack-projected-*, and no other stray data/ or .factory/workspaces/ paths are tracked on this branch.
  • fix: add port guard in two-daemon test for typecheck is a real narrowing check (port1/port2 can be undefined per Bun.serve's type), not a weakened assertion.

Verification

  • New test: src/server/two-daemons.test.ts (two-daemon isolation, the acceptance criterion's own proof).
  • Existing concurrency (concurrency.test.ts), dedupe (dedupe.test.ts, server/dedupe.test.ts), and nested-run (nested-runs.test.ts) tests were updated to thread DaemonServices through but keep the same assertions/intent, including the M1 "reserved before any await" race test in runs.test.ts, which now holds the window open via real allocateWorkspace timing instead of the removed beforeStart? hook.
  • bun run check (format:check + lint + typecheck + bun test) run locally at this PR's head: 311 pass, 0 fail, typecheck clean, lint clean (pre-existing effecttsgo style warnings only, no errors).

Stack

  1. refactor(cli): replace hand-rolled parsers with effect/unstable/cli #52 — Parse CLI with Effect CLI (issue Parse CLI arguments with effect/unstable/cli #33)
  2. refactor(errors): represent domain failures as Schema.TaggedError #53 — Domain errors as tagged errors (issue Represent domain failures as Schema.TaggedError #34)
  3. refactor(runtime): move chunk interpretation into the agent adapter #54 — Move chunk interpretation into adapter (issue Move chunk interpretation into the agent adapter #35)
  4. refactor(runtime): move headless permission setup into the agent adapter #55 — Move headless permission setup into the agent adapter (issue Move headless permission setup out of the workspace allocator #37)
  5. feat(runtime): add Effect composition root and agent runtime service #56 — Agent runtime service (issue Add an Effect composition root and make the agent runtime a service #36)
  6. refactor(daemon): move singletons into per-daemon layers #57 — Move singletons into per-daemon layers (issue Move the daemon's remaining singletons into layers #38) ← this PR

Stack created with GitHub Stacks CLI • Give Feedback 💬

The dedupe registry is now always created via createDedupeRegistry()
and passed to consumers. This is the first step in moving the four
module-level singletons (dedupe, pubsub, active registry, refresh
gates) into per-daemon instances so two daemons can coexist in one
process.

Part of #38
…bSub()

Subscribers are now per-instance, so two daemons in one process do not
share event fan-out state. Consumers receive a PubSub instance rather
than calling module-level publish/subscribe.

Part of #38
Refresh gates are now per-instance via createRefreshGates() and passed
to allocateWorkspace. This allows two daemons to maintain independent
mirror-refresh queues.

Part of #38
…t seams

The active-run registry is now per-daemon via createRunRegistry().
startTrackedRun and dispatchChildRun accept DaemonServices (registry,
pubsub, dedupeRegistry, refreshGates) as required parameters.

Removed optional test-seam fields:
- dedupeRegistry? from StartTrackedRunOptions and DispatchEnv
- beforeStart? from StartTrackedRunOptions

The slot reservation and dedupe-key claim remain synchronous
check-then-set with no await between, preserving concurrency safety.

Part of #38
SchedulerDeps now requires dedupeRegistry and clock as mandatory fields.
The clock comes from Effect's Clock service (currentTimeMillisUnsafe),
replacing the bespoke now?: () => number test seam. Tests provide a
Clock instance instead of injecting a now function.

Part of #38
startDaemon creates DaemonServices (registry, pubsub, dedupe, refresh
gates) and passes them to serve() and the scheduler. http.ts receives
services as a required ServerOptions field and uses them throughout.

This completes the core refactor - two daemons can now coexist in one
process with independent state. Tests still need updating to provide
services instead of using test seams.

Part of #38
Tests now create their own DaemonServices instances instead of using
module-level singletons or test seams. The beforeStart test seam is
gone - tests use slow workspace allocation or direct registry
manipulation to test the reserved-but-not-started window.

Scheduler tests use Effect's Clock interface instead of the bespoke
now?: () => number test seam.

All 250 tests pass.

Part of #38
Proves that two daemons can run in one process without sharing run
state. Each daemon has its own registry, pubsub, dedupe registry, and
refresh gates. Tests verify:
- Independent run registries (each daemon sees only its own runs)
- Independent pubsub (events don't cross between daemons)
- Independent dedupe state (same key can be claimed in both daemons)

Part of #38
A 0-byte .tanstack-projected-* file under a stray data/src/factory/...
path leaked into 2f97451 from a sandboxed run writing through an
absolute path that mirrored the checkout. .gitignore already covers
.tanstack-projected-* and data/ (added in PR #55), which is why
nothing flagged it since; git rm drops it and its now-empty parent
dirs. No other stray data/ or .factory/workspaces/ paths are tracked
on this branch.

Part of #32
Replace the hand-rolled fake Clock.Clock with effect/testing's
TestClock, per issue #38's "tests use TestClock rather than an
injected now". tickOnce only ever reads the clock synchronously
(currentTimeMillisUnsafe()) and never suspends on Effect.sleep, so
none of TestClock's fiber-coordination machinery is exercised here —
but building it via Effect.runPromise(Effect.scoped(TestClock.make()))
and bridging setTime back into the plain-async fixture (ADR 0009 §5)
composes cleanly, so there was no need to keep the fake.

Part of #32
The #34 exhaustiveness test landed while `fixture` was still synchronous;
#38 made it async for `TestClock`. Rebasing the stack put the two together,
which typecheck caught.
@FreshlyBrewedCode
FreshlyBrewedCode force-pushed the 38-move-singletons-into-layers branch from 5a8c2a0 to 0bbf3f1 Compare September 23, 2026 07:08

This branch has not been deployed

No deployments
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.

Move the daemon's remaining singletons into layers

1 participant