From a47b7ef5d3a052c40aaaf2d9a1b342915b7f4aee Mon Sep 17 00:00:00 2001 From: Dax Raad Date: Tue, 15 Sep 2026 21:27:46 -0400 Subject: [PATCH 1/2] fix(core): reuse model catalog across credential changes Every credential change reached each open Location and, because the provider snapshot was keyed on every connection of every integration, produced a new snapshot even when the same providers stayed available. Model treated that as a catalog change and rebuilt from scratch: deep copying every model, replaying every transform, re-merging provider settings, and publishing model.updated so clients refetched. With many Locations open, one /connect did all of that per Location, twice. Key the snapshot on which provider definitions are available, so a credential switch that changes nothing returns the same snapshot and publishes nothing. Let Model share provider definition maps and copy a provider's map and a model only on their first edit, and reuse merged output for models whose definition and provider record are unchanged, so the rebuild for a genuinely new provider costs only that provider. Provider's index now gives foreign definitions the registering provider's identity, which Model previously did on every rebuild. --- packages/core/src/model.ts | 130 ++++++++++++++++------------- packages/core/src/provider.ts | 35 +++++--- packages/core/test/catalog.test.ts | 126 +++++++++++++++++++++++++++- 3 files changed, 220 insertions(+), 71 deletions(-) diff --git a/packages/core/src/model.ts b/packages/core/src/model.ts index 515f97c0899e..652c481a067b 100644 --- a/packages/core/src/model.ts +++ b/packages/core/src/model.ts @@ -68,7 +68,7 @@ export interface Interface extends State.Transformable { export class Service extends Context.Service()("@opencode/Model") {} type Data = { - models: Map> + models: Map> defaultModel?: { providerID: Provider.ID; modelID: ID } } @@ -82,59 +82,70 @@ const layer = Layer.effect( const state: State.Interface = State.create({ name: "model", initial: () => ({ - models: new Map( - (input?.available ?? []).map((record) => [ - record.provider.id, - new Map( - Array.from(record.models, ([id, model]) => [ - id, - { - ...structuredClone(model), - id, - providerID: record.provider.id, - } as MutableInfo, - ]), - ), - ]), - ), + models: new Map((input?.available ?? []).map((record) => [record.provider.id, record.models])), }), - editor: (data) => ({ - list: (providerID) => - providerID === undefined - ? Array.from(data.models.values()).flatMap((models) => Array.from(models.values())) - : Array.from(data.models.get(providerID)?.values() ?? []), - get: (providerID, modelID) => data.models.get(providerID)?.get(modelID), - update: (providerID, modelID, update) => { - // Model edits cannot create/enable a provider or bypass its availability decision. - const models = data.models.get(providerID) - if (!models) return - const model = models.get(modelID) ?? (Info.default(providerID, modelID) as MutableInfo) - update(model) - model.id = modelID - model.providerID = providerID - const provider = input?.records.get(providerID)?.provider - AISDKNative.rewrite(model, { - specifier: model.package ?? provider?.package, - providerID, - canonical: model.canonical ?? provider?.canonical, - modelID: model.modelID ?? modelID, - }) - models.set(modelID, model) - }, - remove: (providerID, modelID) => { - data.models.get(providerID)?.delete(modelID) - }, - default: { - get: () => data.defaultModel, - set: (providerID, modelID) => { - data.defaultModel = { providerID, modelID } + editor: (data) => { + // Definitions are shared across Locations; a provider's map and a model are copied before their first edit. + const owned = new WeakSet>() + const drafts = new WeakSet() + const writable = (providerID: Provider.ID) => { + const current = data.models.get(providerID) + if (!current) return undefined + if (owned.has(current)) return current as Map + const copy = new Map(current) + owned.add(copy) + data.models.set(providerID, copy) + return copy + } + const draft = (providerID: Provider.ID, modelID: ID) => { + const models = writable(providerID) + const current = models?.get(modelID) + if (!models || !current) return undefined + if (drafts.has(current)) return current as MutableInfo + const copy = structuredClone(current) as MutableInfo + drafts.add(copy) + models.set(modelID, copy) + return copy + } + return { + list: (providerID) => + (providerID === undefined ? Array.from(data.models.keys()) : [providerID]).flatMap((id) => + Array.from(data.models.get(id)?.keys() ?? []).flatMap((modelID) => draft(id, modelID) ?? []), + ), + get: draft, + update: (providerID, modelID, update) => { + // Model edits cannot create/enable a provider or bypass its availability decision. + const models = writable(providerID) + if (!models) return + const model = draft(providerID, modelID) ?? (Info.default(providerID, modelID) as MutableInfo) + update(model) + model.id = modelID + model.providerID = providerID + const provider = input?.records.get(providerID)?.provider + AISDKNative.rewrite(model, { + specifier: model.package ?? provider?.package, + providerID, + canonical: model.canonical ?? provider?.canonical, + modelID: model.modelID ?? modelID, + }) + drafts.add(model) + models.set(modelID, model) }, - }, - provider: { - list: () => Array.from(input?.records.values() ?? []), - get: (providerID) => input?.records.get(providerID), - }, - }), + remove: (providerID, modelID) => { + writable(providerID)?.delete(modelID) + }, + default: { + get: () => data.defaultModel, + set: (providerID, modelID) => { + data.defaultModel = { providerID, modelID } + }, + }, + provider: { + list: () => Array.from(input?.records.values() ?? []), + get: (providerID) => input?.records.get(providerID), + }, + } + }, // read() also refreshes dependencies changed inside a State.batch before notification. notify: () => notify, }) @@ -147,6 +158,8 @@ const layer = Layer.effect( byProvider: ReadonlyMap> } | undefined + // An unedited model keeps its shared definition object across rebuilds, so its merged output is reusable. + const merged = new WeakMap() const read = Effect.fn("Model.snapshot")(function* () { while (true) { const current = yield* providers.snapshot() @@ -165,9 +178,10 @@ const layer = Layer.effect( return [ providerID, new Map( - Array.from(models, ([id, model]) => [ - id, - { + Array.from(models, ([id, model]) => { + const reusable = merged.get(model) + if (reusable && reusable.provider === provider) return [id, reusable.model] + const value = { ...model, ...(provider?.canonical === undefined ? {} : { canonical: provider.canonical }), package: model.package ?? provider?.package, @@ -176,8 +190,10 @@ const layer = Layer.effect( settings: Provider.mergeOverlay(provider?.settings, model.settings), headers: Provider.mergeHeaders(provider?.headers, model.headers), body: Provider.mergeOverlay(provider?.body, model.body), - } satisfies Info, - ]), + } satisfies Info + merged.set(model, { provider, model: value }) + return [id, value] + }), ), ] }), diff --git a/packages/core/src/provider.ts b/packages/core/src/provider.ts index f1bfe668aa66..8b350a4ad65c 100644 --- a/packages/core/src/provider.ts +++ b/packages/core/src/provider.ts @@ -228,12 +228,18 @@ export interface Interface extends State.Transformable { export class Service extends Context.Service()("@opencode/Provider") {} // Every location references the same index for a shared immutable definition array. -const definitions = new WeakMap>() -function index(models: readonly Model.Info[]) { - const cached = definitions.get(models) +const definitions = new WeakMap>>() +function index(providerID: ID, models: readonly Model.Info[]) { + const indexes = definitions.get(models) ?? new Map>() + definitions.set(models, indexes) + const cached = indexes.get(providerID) if (cached) return cached - const result = freeze(new Map(models.map((model) => [model.id, model])), true) - definitions.set(models, result) + // Model shares these definitions without copying, so a foreign definition takes this provider's identity here. + const result = freeze( + new Map(models.map((model) => [model.id, model.providerID === providerID ? model : { ...model, providerID }])), + true, + ) + indexes.set(providerID, result) return result } @@ -281,7 +287,7 @@ const layer = Layer.effect( add: (definition) => { records.set(definition.info.id, { provider: structuredClone(definition.info) as MutableInfo, - models: index(definition.models), + models: index(definition.info.id, definition.models), sourceConnection: definition.sourceConnection, }) }, @@ -300,7 +306,7 @@ const layer = Layer.effect( }, models: { set: (id, values) => { - entry(id).models = index(values) + entry(id).models = index(id, values) }, update: (providerID, modelID, update) => { const record = entry(providerID) @@ -338,7 +344,7 @@ const layer = Layer.effect( // Registrations may outlive a borrowed service layer; their later disposal must // not query dependencies that have already closed. yield* Effect.addFinalizer(() => State.shutdown(state.reload())) - let cached: { records: Snapshot["records"]; access: string; value: Snapshot } | undefined + let cached: { records: Snapshot["records"]; value: Snapshot } | undefined const snapshot = Effect.fn("Provider.snapshot")(function* () { while (true) { const revision = integrations.revision() @@ -347,10 +353,6 @@ const layer = Layer.effect( const connections = yield* integrations.list() // Either fold can disable a plugin that also contributed to the other domain. if (revision !== integrations.revision() || records !== state.get()) continue - const access = JSON.stringify( - connections.map((integration) => [integration.id, integration.connections.map(IntegrationConnection.key)]), - ) - if (cached?.records === records && cached.access === access) return cached.value const byID = new Map(connections.map((integration) => [integration.id, integration])) const available = Array.from(records.values()).filter((record) => { if (record.provider.activation === "disabled") return false @@ -366,8 +368,15 @@ const layer = Layer.effect( if (integration?.connections.length) return true return record.provider.integrationID === undefined && !integration }) + // A credential change that leaves the same definitions available is not a catalog change. + if ( + cached?.records === records && + cached.value.available.length === available.length && + cached.value.available.every((record, index) => record === available[index]) + ) + return cached.value const value = freeze({ records, available, providers: available.map((record) => record.provider) }, true) - cached = { records, access, value } + cached = { records, value } return value } }) diff --git a/packages/core/test/catalog.test.ts b/packages/core/test/catalog.test.ts index 61fc7e75ff7a..695f97b7f696 100644 --- a/packages/core/test/catalog.test.ts +++ b/packages/core/test/catalog.test.ts @@ -1,7 +1,7 @@ import { describe, expect } from "bun:test" import { LanguageModel } from "@opencode/ai" import { OpenAIChat } from "@opencode/ai/protocols" -import { Effect, Fiber, Layer, Stream } from "effect" +import { Effect, Fiber, Layer, Ref, Stream } from "effect" import { Integration } from "@opencode/core/integration" import { Credential } from "@opencode/core/credential" import { AppNodeBuilder } from "@opencode/core/effect/app-node-builder" @@ -148,6 +148,121 @@ describe("Provider and Model", () => { }).pipe(Effect.scoped, Effect.provide(localProviderLayer)) }) + it.effect("reuses the model catalog across credential switches", () => + Effect.gen(function* () { + const providers = yield* Provider.Service + const models = yield* Model.Service + const integrations = yield* Integration.Service + const credentials = yield* Credential.Service + const bus = yield* Bus.Service + const providerID = Provider.ID.make("switchable") + const integrationID = Integration.ID.make(providerID) + yield* integrations.transform((editor) => editor.update(integrationID, () => {})) + yield* providers.transform((editor) => + editor.add({ + info: Provider.Info.empty(providerID), + models: [Model.Info.default(providerID, Model.ID.make("chat"))], + }), + ) + expect(yield* models.available()).toEqual([]) + const log = yield* Ref.make([]) + yield* bus.subscribe().pipe( + Stream.runForEach((event) => Ref.update(log, (types) => [...types, event.type])), + Effect.forkScoped({ startImmediately: true }), + ) + yield* Effect.yieldNow + + const first = yield* credentials.create({ + integrationID, + value: Credential.Key.make({ type: "key", key: "first" }), + }) + const materialized = yield* models.available() + expect(materialized).toHaveLength(1) + + const second = yield* credentials.create({ + integrationID, + value: Credential.Key.make({ type: "key", key: "second" }), + }) + expect(yield* models.available()).toBe(materialized) + yield* credentials.activate(first.id) + expect(yield* models.available()).toBe(materialized) + yield* credentials.remove(first.id) + expect(yield* models.available()).toBe(materialized) + + // Disconnecting is a real change whose model.updated follows every earlier one in the log, + // so once it has arrived the total shows whether any switch above published as well. + yield* credentials.remove(second.id) + expect(yield* models.available()).toEqual([]) + yield* settle( + Ref.get(log).pipe( + Effect.map( + (types) => + types.lastIndexOf(Model.Event.Updated.type) > types.lastIndexOf(Credential.Event.Updated.type), + ), + ), + ) + expect((yield* Ref.get(log)).filter((type) => type === Model.Event.Updated.type)).toHaveLength(2) + }), + ) + + it.effect("gives foreign definitions the registering provider's identity", () => + Effect.gen(function* () { + const providers = yield* Provider.Service + const models = yield* Model.Service + const source = Provider.ID.make("source") + const mirror = Provider.ID.make("mirror") + const modelID = Model.ID.make("chat") + const definitions = [Model.Info.default(source, modelID)] + yield* providers.transform((editor) => { + editor.add({ info: { ...Provider.Info.empty(source), activation: "enabled" }, models: definitions }) + editor.add({ info: { ...Provider.Info.empty(mirror), activation: "enabled" }, models: definitions }) + }) + + expect((yield* models.available()).map((model) => model.providerID).toSorted()).toEqual([mirror, source]) + expect(yield* models.get(mirror, modelID)).toMatchObject({ id: modelID, providerID: mirror }) + expect((yield* providers.snapshot()).records.get(source)?.models.get(modelID)).toBe(definitions[0]) + }), + ) + + it.effect("keeps materialized models when another provider becomes available", () => + Effect.gen(function* () { + const providers = yield* Provider.Service + const models = yield* Model.Service + const integrations = yield* Integration.Service + const credentials = yield* Credential.Service + const existing = Provider.ID.make("existing") + const added = Provider.ID.make("added") + const edited = Model.ID.make("edited") + const untouched = Model.ID.make("untouched") + yield* integrations.transform((editor) => editor.update(Integration.ID.make(added), () => {})) + yield* providers.transform((editor) => { + editor.add({ + info: { ...Provider.Info.empty(existing), activation: "enabled" }, + models: [Model.Info.default(existing, edited), Model.Info.default(existing, untouched)], + }) + editor.add({ info: Provider.Info.empty(added), models: [Model.Info.default(added, Model.ID.make("chat"))] }) + }) + yield* models.transform((editor) => + editor.update(existing, edited, (model) => { + model.limit.context = 1 + }), + ) + const before = required(yield* models.get(existing, untouched)) + + yield* credentials.create({ + integrationID: Integration.ID.make(added), + value: Credential.Key.make({ type: "key", key: "secret" }), + }) + expect((yield* models.available()).map((model) => model.providerID).toSorted()).toEqual([ + added, + existing, + existing, + ]) + expect(yield* models.get(existing, untouched)).toBe(before) + expect(yield* models.get(existing, edited)).toMatchObject({ limit: { context: 1 } }) + }), + ) + it.effect("derives availability from a provider's integration", () => { const integrationID = Integration.ID.make("gateway") const providerID = Provider.ID.make("remote") @@ -511,3 +626,12 @@ describe("Provider and Model", () => { }), ) }) + +// Bus subscribers run on their own fibers, so give them turns until the condition holds. +const settle = Effect.fnUntraced(function* (condition: Effect.Effect) { + for (let attempt = 0; attempt < 100; attempt++) { + if (yield* condition) return + yield* Effect.yieldNow + } + return yield* Effect.die("Timed out waiting for catalog events") +}) From 666187a939648a06902aa728348aa8410ce6b8ad Mon Sep 17 00:00:00 2001 From: Dax Raad Date: Tue, 15 Sep 2026 22:20:24 -0400 Subject: [PATCH 2/2] test(core): harden catalog event test and cover direct model edits Wait for the connect's model.updated before switching credentials so the event count no longer depends on subscriber fibers keeping pace, and add a test that direct edits to models returned by list and get persist into the catalog without touching shared definitions. Also tidy the definition index write, draft's early returns, and the provider selection in list. --- packages/core/src/model.ts | 13 +++++++---- packages/core/src/provider.ts | 2 +- packages/core/test/catalog.test.ts | 37 +++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/packages/core/src/model.ts b/packages/core/src/model.ts index 652c481a067b..7f5328a57c82 100644 --- a/packages/core/src/model.ts +++ b/packages/core/src/model.ts @@ -99,8 +99,9 @@ const layer = Layer.effect( } const draft = (providerID: Provider.ID, modelID: ID) => { const models = writable(providerID) - const current = models?.get(modelID) - if (!models || !current) return undefined + if (!models) return undefined + const current = models.get(modelID) + if (!current) return undefined if (drafts.has(current)) return current as MutableInfo const copy = structuredClone(current) as MutableInfo drafts.add(copy) @@ -108,10 +109,12 @@ const layer = Layer.effect( return copy } return { - list: (providerID) => - (providerID === undefined ? Array.from(data.models.keys()) : [providerID]).flatMap((id) => + list: (providerID) => { + const ids = providerID === undefined ? Array.from(data.models.keys()) : [providerID] + return ids.flatMap((id) => Array.from(data.models.get(id)?.keys() ?? []).flatMap((modelID) => draft(id, modelID) ?? []), - ), + ) + }, get: draft, update: (providerID, modelID, update) => { // Model edits cannot create/enable a provider or bypass its availability decision. diff --git a/packages/core/src/provider.ts b/packages/core/src/provider.ts index 8b350a4ad65c..f309ca4171c6 100644 --- a/packages/core/src/provider.ts +++ b/packages/core/src/provider.ts @@ -231,7 +231,6 @@ export class Service extends Context.Service()("@opencode/Pr const definitions = new WeakMap>>() function index(providerID: ID, models: readonly Model.Info[]) { const indexes = definitions.get(models) ?? new Map>() - definitions.set(models, indexes) const cached = indexes.get(providerID) if (cached) return cached // Model shares these definitions without copying, so a foreign definition takes this provider's identity here. @@ -240,6 +239,7 @@ function index(providerID: ID, models: readonly Model.Info[]) { true, ) indexes.set(providerID, result) + definitions.set(models, indexes) return result } diff --git a/packages/core/test/catalog.test.ts b/packages/core/test/catalog.test.ts index 695f97b7f696..658ff679ed59 100644 --- a/packages/core/test/catalog.test.ts +++ b/packages/core/test/catalog.test.ts @@ -171,6 +171,9 @@ describe("Provider and Model", () => { Effect.forkScoped({ startImmediately: true }), ) yield* Effect.yieldNow + const updates = Ref.get(log).pipe( + Effect.map((types) => types.filter((type) => type === Model.Event.Updated.type).length), + ) const first = yield* credentials.create({ integrationID, @@ -178,6 +181,8 @@ describe("Provider and Model", () => { }) const materialized = yield* models.available() expect(materialized).toHaveLength(1) + // Credential events reach Model on other fibers; let the connect land before switching. + yield* settle(updates.pipe(Effect.map((count) => count >= 1))) const second = yield* credentials.create({ integrationID, @@ -201,7 +206,37 @@ describe("Provider and Model", () => { ), ), ) - expect((yield* Ref.get(log)).filter((type) => type === Model.Event.Updated.type)).toHaveLength(2) + expect(yield* updates).toBe(2) + }), + ) + + it.effect("persists direct edits to models returned by list and get", () => + Effect.gen(function* () { + const providers = yield* Provider.Service + const models = yield* Model.Service + const providerID = Provider.ID.make("direct") + const listed = Model.ID.make("listed") + const fetched = Model.ID.make("fetched") + const definitions = [Model.Info.default(providerID, listed), Model.Info.default(providerID, fetched)] + yield* providers.transform((editor) => + editor.add({ info: { ...Provider.Info.empty(providerID), activation: "enabled" }, models: definitions }), + ) + yield* models.transform((editor) => { + editor.list(providerID).forEach((model) => { + model.limit.context = 4096 + }) + required(editor.get(providerID, fetched)).capabilities.input.push("pdf") + }) + + expect(yield* models.get(providerID, listed)).toMatchObject({ + limit: { context: 4096 }, + capabilities: { input: ["text", "image"] }, + }) + expect(yield* models.get(providerID, fetched)).toMatchObject({ + limit: { context: 4096 }, + capabilities: { input: ["text", "image", "pdf"] }, + }) + expect(definitions.map((model) => model.limit.context)).toEqual([200_000, 200_000]) }), )