From 7ae060fe3e875d55a09929a14ff79cf18e6ac937 Mon Sep 17 00:00:00 2001 From: Victor Vogel Date: Sat, 24 Oct 2026 08:52:53 -0500 Subject: [PATCH 1/3] fix(server): settings saves no longer hand out a closed provider instance Reconcile closed a replaced instance's scope, then awaited the replacement's creation before swapping the entries map. Until the swap, getInstance still returned the closed runtime. Drop retired ids from the map before closing their scopes. Co-Authored-By: Claude Opus 5.5 --- .../provider/ProviderInstanceRegistry.test.ts | 75 +++++++++++++++++++ .../src/provider/ProviderInstanceRegistry.ts | 12 ++- 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/apps/server/src/provider/ProviderInstanceRegistry.test.ts b/apps/server/src/provider/ProviderInstanceRegistry.test.ts index 48711552b43c0..df1814d533a95 100644 --- a/apps/server/src/provider/ProviderInstanceRegistry.test.ts +++ b/apps/server/src/provider/ProviderInstanceRegistry.test.ts @@ -16,12 +16,17 @@ import { } from "@t3tools/contracts"; import * as ProviderLatestVersions from "@t3tools/provider-core/server/ProviderLatestVersions"; import * as McpProviderSessions from "@t3tools/provider-core/server/McpProviderSessions"; +import type { AnyProviderDriver, ProviderInstance } from "@t3tools/provider-core/server/driver"; import * as HostProcess from "@t3tools/shared/HostProcess"; import * as DateTime from "effect/DateTime"; +import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Path from "effect/Path"; +import * as Ref from "effect/Ref"; +import * as Schema from "effect/Schema"; import * as Stream from "effect/Stream"; import * as TestClock from "effect/testing/TestClock"; import { HttpClient, HttpClientResponse } from "effect/http"; @@ -646,3 +651,73 @@ describe("ProviderInstanceRegistry — multi-instance codex slice", () => { }).pipe(Effect.provide(layerTest)), ); }); + +describe("ProviderInstanceRegistry — reconcile", () => { + const reconcileDriverKind = ProviderDriverKind.make("reconcileTest"); + const reconcileInstanceId = ProviderInstanceId.make("reconcile_test"); + + const makeReconcileInstance = (displayName: string | undefined): ProviderInstance => ({ + instanceId: reconcileInstanceId, + driverKind: reconcileDriverKind, + continuationIdentity: { + driverKind: reconcileDriverKind, + continuationKey: `reconcileTest:${displayName}`, + }, + displayName, + enabled: true, + snapshot: {} as ProviderInstance["snapshot"], + orchestrationAdapter: {} as ProviderInstance["orchestrationAdapter"], + textGeneration: {} as ProviderInstance["textGeneration"], + }); + + const configWithName = (displayName: string): ProviderInstanceConfigMap => ({ + [reconcileInstanceId]: { driver: reconcileDriverKind, displayName, config: {} }, + }); + + it.effect("stops returning a replaced instance before closing its scope", () => + Effect.scoped( + Effect.gen(function* () { + const closedNames = yield* Ref.make>([]); + const replacementStarted = yield* Deferred.make(); + const releaseReplacement = yield* Deferred.make(); + + const driver: AnyProviderDriver = { + driverKind: reconcileDriverKind, + metadata: { displayName: "Reconcile test" }, + configSchema: Schema.Struct({}), + defaultConfig: () => ({}), + create: ({ displayName }) => + Effect.gen(function* () { + yield* Effect.addFinalizer(() => + Ref.update(closedNames, (names) => [...names, displayName]), + ); + if (displayName === "Second") { + yield* Deferred.succeed(replacementStarted, undefined); + yield* Deferred.await(releaseReplacement); + } + return makeReconcileInstance(displayName); + }), + }; + + const { registry, mutator } = yield* makeProviderInstanceRegistry({ + drivers: [driver], + configMap: configWithName("First"), + }); + expect((yield* registry.getInstance(reconcileInstanceId))?.displayName).toBe("First"); + + const reconcileFiber = yield* mutator + .reconcile(configWithName("Second")) + .pipe(Effect.forkChild({ startImmediately: true })); + yield* Deferred.await(replacementStarted); + + expect(yield* Ref.get(closedNames)).toEqual(["First"]); + expect(yield* registry.getInstance(reconcileInstanceId)).toBeUndefined(); + expect(yield* registry.listInstances).toEqual([]); + + yield* Deferred.succeed(releaseReplacement, undefined); + yield* Fiber.join(reconcileFiber); + expect((yield* registry.getInstance(reconcileInstanceId))?.displayName).toBe("Second"); + }), + ), + ); +}); diff --git a/apps/server/src/provider/ProviderInstanceRegistry.ts b/apps/server/src/provider/ProviderInstanceRegistry.ts index 1659e8f8fa144..4f1ef4bf9cf47 100644 --- a/apps/server/src/provider/ProviderInstanceRegistry.ts +++ b/apps/server/src/provider/ProviderInstanceRegistry.ts @@ -192,7 +192,17 @@ const makeReconcile = (input: { replacedIds.add(instanceId); } } - for (const id of [...removedIds, ...replacedIds]) { + // Drop the ids before closing so `getInstance` never returns a closed + // runtime while replacements build. + const retiredIds = [...removedIds, ...replacedIds]; + if (retiredIds.length > 0) { + yield* Ref.update(state.entries, (entries) => { + const remaining = new Map(entries); + for (const id of retiredIds) remaining.delete(id); + return remaining; + }); + } + for (const id of retiredIds) { const live = previousEntries.get(id); if (live) { yield* Scope.close(live.scope, Exit.void).pipe(Effect.ignore); From 791ebb0a7df60f16bdb9c4612d4bbb1e89dd8af2 Mon Sep 17 00:00:00 2001 From: Victor Vogel Date: Sat, 24 Oct 2026 10:51:25 -0500 Subject: [PATCH 2/3] test(server): reconcile test replaces the instance under any equality rule The test renamed the instance without changing its config, which only counts as a replacement while reconcile compares whole entries. Change the config too, and give the fake instance real snapshot effects so the registry can wrap them. Co-Authored-By: Claude Opus 5.5 --- .../provider/ProviderInstanceRegistry.test.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/apps/server/src/provider/ProviderInstanceRegistry.test.ts b/apps/server/src/provider/ProviderInstanceRegistry.test.ts index df1814d533a95..7607c58818e9a 100644 --- a/apps/server/src/provider/ProviderInstanceRegistry.test.ts +++ b/apps/server/src/provider/ProviderInstanceRegistry.test.ts @@ -665,13 +665,24 @@ describe("ProviderInstanceRegistry — reconcile", () => { }, displayName, enabled: true, - snapshot: {} as ProviderInstance["snapshot"], + // Real effects, so the registry can wrap the snapshot when it builds an entry. + snapshot: { + getSnapshot: Effect.never, + refresh: Effect.never, + streamChanges: Stream.empty, + } as Partial as ProviderInstance["snapshot"], orchestrationAdapter: {} as ProviderInstance["orchestrationAdapter"], textGeneration: {} as ProviderInstance["textGeneration"], }); + // Change runtime config, not only the display name, so the edit always + // replaces the instance rather than counting as cosmetic. const configWithName = (displayName: string): ProviderInstanceConfigMap => ({ - [reconcileInstanceId]: { driver: reconcileDriverKind, displayName, config: {} }, + [reconcileInstanceId]: { + driver: reconcileDriverKind, + displayName, + config: { revision: displayName }, + }, }); it.effect("stops returning a replaced instance before closing its scope", () => @@ -684,8 +695,8 @@ describe("ProviderInstanceRegistry — reconcile", () => { const driver: AnyProviderDriver = { driverKind: reconcileDriverKind, metadata: { displayName: "Reconcile test" }, - configSchema: Schema.Struct({}), - defaultConfig: () => ({}), + configSchema: Schema.Struct({ revision: Schema.String }), + defaultConfig: () => ({ revision: "" }), create: ({ displayName }) => Effect.gen(function* () { yield* Effect.addFinalizer(() => From d5039d89c0ba5a21acc34ab8c19bbe32eed6d213 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Fri, 30 Oct 2026 04:41:11 +0000 Subject: [PATCH 3/3] fix(server): simplify retired provider lookup regression --- .../provider/ProviderInstanceRegistry.test.ts | 140 +++++++----------- .../src/provider/ProviderInstanceRegistry.ts | 10 +- 2 files changed, 58 insertions(+), 92 deletions(-) diff --git a/apps/server/src/provider/ProviderInstanceRegistry.test.ts b/apps/server/src/provider/ProviderInstanceRegistry.test.ts index 2807f3c96adfe..a9408c099814e 100644 --- a/apps/server/src/provider/ProviderInstanceRegistry.test.ts +++ b/apps/server/src/provider/ProviderInstanceRegistry.test.ts @@ -16,7 +16,6 @@ import { } from "@t3tools/contracts"; import * as ProviderLatestVersions from "@t3tools/provider-core/server/ProviderLatestVersions"; import * as McpProviderSessions from "@t3tools/provider-core/server/McpProviderSessions"; -import type { AnyProviderDriver, ProviderInstance } from "@t3tools/provider-core/server/driver"; import * as HostProcess from "@t3tools/shared/HostProcess"; import * as DateTime from "effect/DateTime"; import * as Deferred from "effect/Deferred"; @@ -25,9 +24,6 @@ import * as Fiber from "effect/Fiber"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Path from "effect/Path"; -import * as Ref from "effect/Ref"; -import * as Schema from "effect/Schema"; -import * as Stream from "effect/Stream"; import * as TestClock from "effect/testing/TestClock"; import { HttpClient, HttpClientResponse } from "effect/http"; @@ -198,6 +194,61 @@ describe("ProviderInstanceRegistry — multi-instance codex slice", () => { Layer.provideMerge(layerBase), ); + it.effect.each(["replacement", "removal"] as const)( + "hides retired instances during reconcile (%s)", + (scenario) => + Effect.gen(function* () { + let closed = 0; + const replacementStarted = yield* Deferred.make(); + const releaseReplacement = yield* Deferred.make(); + const driver: typeof CodexDriver = { + ...CodexDriver, + create: (input) => + Effect.gen(function* () { + if (input.displayName === "Second") { + yield* Deferred.succeed(replacementStarted, undefined); + yield* Deferred.await(releaseReplacement); + } + const instance = yield* CodexDriver.create(input); + yield* Effect.addFinalizer(() => Effect.sync(() => closed++)); + return instance; + }), + }; + const retiredId = ProviderInstanceId.make("retired"); + const siblingId = ProviderInstanceId.make("sibling"); + const nextId = scenario === "replacement" ? retiredId : ProviderInstanceId.make("new"); + const entry = { + driver: CodexDriver.driverKind, + enabled: false, + config: makeCodexConfig({}), + }; + const { registry, mutator } = yield* makeProviderInstanceRegistry({ + drivers: [driver], + configMap: { [retiredId]: entry, [siblingId]: entry }, + }); + const sibling = yield* registry.getInstance(siblingId); + const reconcile = yield* mutator + .reconcile({ + [siblingId]: entry, + [nextId]: { + ...entry, + displayName: "Second", + config: makeCodexConfig({ customModels: ["replacement-model"] }), + }, + }) + .pipe(Effect.forkChild({ startImmediately: true })); + yield* Deferred.await(replacementStarted); + expect(closed).toBe(1); + expect(yield* registry.getInstance(retiredId)).toBeUndefined(); + expect(yield* registry.listInstances).toEqual([sibling]); + yield* Deferred.succeed(releaseReplacement, undefined); + yield* Fiber.join(reconcile); + expect((yield* registry.getInstance(nextId))?.displayName).toBe("Second"); + expect(yield* registry.getInstance(siblingId)).toBe(sibling); + expect(closed).toBe(1); + }).pipe(Effect.provide(layerTest)), + ); + it.live("treats an explicit in-config enabled:false as disabling despite the envelope", () => Effect.gen(function* () { const staleId = ProviderInstanceId.make("codex_stale"); @@ -649,84 +700,3 @@ describe("ProviderInstanceRegistry — multi-instance codex slice", () => { }).pipe(Effect.provide(layerTest)), ); }); - -describe("ProviderInstanceRegistry — reconcile", () => { - const reconcileDriverKind = ProviderDriverKind.make("reconcileTest"); - const reconcileInstanceId = ProviderInstanceId.make("reconcile_test"); - - const makeReconcileInstance = (displayName: string | undefined): ProviderInstance => ({ - instanceId: reconcileInstanceId, - driverKind: reconcileDriverKind, - continuationIdentity: { - driverKind: reconcileDriverKind, - continuationKey: `reconcileTest:${displayName}`, - }, - displayName, - enabled: true, - // Real effects, so the registry can wrap the snapshot when it builds an entry. - snapshot: { - getSnapshot: Effect.never, - refresh: Effect.never, - streamChanges: Stream.empty, - } as Partial as ProviderInstance["snapshot"], - orchestrationAdapter: {} as ProviderInstance["orchestrationAdapter"], - textGeneration: {} as ProviderInstance["textGeneration"], - }); - - // Change runtime config, not only the display name, so the edit always - // replaces the instance rather than counting as cosmetic. - const configWithName = (displayName: string): ProviderInstanceConfigMap => ({ - [reconcileInstanceId]: { - driver: reconcileDriverKind, - displayName, - config: { revision: displayName }, - }, - }); - - it.effect("stops returning a replaced instance before closing its scope", () => - Effect.scoped( - Effect.gen(function* () { - const closedNames = yield* Ref.make>([]); - const replacementStarted = yield* Deferred.make(); - const releaseReplacement = yield* Deferred.make(); - - const driver: AnyProviderDriver = { - driverKind: reconcileDriverKind, - metadata: { displayName: "Reconcile test" }, - configSchema: Schema.Struct({ revision: Schema.String }), - defaultConfig: () => ({ revision: "" }), - create: ({ displayName }) => - Effect.gen(function* () { - yield* Effect.addFinalizer(() => - Ref.update(closedNames, (names) => [...names, displayName]), - ); - if (displayName === "Second") { - yield* Deferred.succeed(replacementStarted, undefined); - yield* Deferred.await(releaseReplacement); - } - return makeReconcileInstance(displayName); - }), - }; - - const { registry, mutator } = yield* makeProviderInstanceRegistry({ - drivers: [driver], - configMap: configWithName("First"), - }); - expect((yield* registry.getInstance(reconcileInstanceId))?.displayName).toBe("First"); - - const reconcileFiber = yield* mutator - .reconcile(configWithName("Second")) - .pipe(Effect.forkChild({ startImmediately: true })); - yield* Deferred.await(replacementStarted); - - expect(yield* Ref.get(closedNames)).toEqual(["First"]); - expect(yield* registry.getInstance(reconcileInstanceId)).toBeUndefined(); - expect(yield* registry.listInstances).toEqual([]); - - yield* Deferred.succeed(releaseReplacement, undefined); - yield* Fiber.join(reconcileFiber); - expect((yield* registry.getInstance(reconcileInstanceId))?.displayName).toBe("Second"); - }), - ), - ); -}); diff --git a/apps/server/src/provider/ProviderInstanceRegistry.ts b/apps/server/src/provider/ProviderInstanceRegistry.ts index 6a9bad6fde3cb..02d769f5cfd45 100644 --- a/apps/server/src/provider/ProviderInstanceRegistry.ts +++ b/apps/server/src/provider/ProviderInstanceRegistry.ts @@ -179,15 +179,11 @@ const makeReconcile = (input: { replacedIds.add(instanceId); } } - // Drop the ids before closing so `getInstance` never returns a closed - // runtime while replacements build. const retiredIds = [...removedIds, ...replacedIds]; if (retiredIds.length > 0) { - yield* Ref.update(state.entries, (entries) => { - const remaining = new Map(entries); - for (const id of retiredIds) remaining.delete(id); - return remaining; - }); + const remaining = new Map(previousEntries); + for (const id of retiredIds) remaining.delete(id); + yield* Ref.set(state.entries, remaining); } for (const id of retiredIds) { const live = previousEntries.get(id);