Skip to content

Commit edeea8c

Browse files
yordisyordis
authored
fix(server): a settings save no longer reads an unreadable secret again (#31226)
Signed-off-by: Yordis Prieto <[email protected]>
1 parent 9529ee3 commit edeea8c

4 files changed

Lines changed: 140 additions & 9 deletions

File tree

‎apps/server/src/provider/ProviderInstanceRegistry.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -878,7 +878,7 @@ describe("ProviderInstanceRegistry secret environment", () => {
878878
yield* PubSub.take(changes);
879879
return built;
880880
});
881-
return { vault, reads, invalidated, created, configs, make, makeGated, gate };
881+
return { vault, reads, invalidated, created, configs, configMap, make, makeGated, gate };
882882
}
883883

884884
it.effect("starts an instance with the secret its environment points at", () => {
@@ -899,13 +899,19 @@ describe("ProviderInstanceRegistry secret environment", () => {
899899
it.effect("marks an instance unavailable and names the variable it could not read", () => {
900900
const fixture = setup({});
901901
return Effect.gen(function* () {
902-
const { registry } = yield* fixture.make;
902+
const { registry, mutator } = yield* fixture.make;
903+
const readsAtStartup = fixture.reads.length;
903904

904905
expect(yield* registry.getInstance(instanceId)).toBeUndefined();
905906
expect(fixture.created).toEqual([]);
906907
const [unavailable] = yield* registry.listUnavailable;
907908
expect(unavailable?.instanceId).toBe(instanceId);
908909
expect(unavailable?.unavailableReason).toContain("OPENAI_API_KEY");
910+
911+
// A settings save that leaves it alone does not read the vault again.
912+
yield* mutator.reconcile({ ...fixture.configMap });
913+
expect(fixture.reads).toHaveLength(readsAtStartup);
914+
expect(yield* registry.listUnavailable).toHaveLength(1);
909915
}).pipe(Effect.scoped);
910916
});
911917

‎apps/server/src/provider/ProviderInstanceRegistry.ts‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@ interface LiveEntry {
6060
interface RegistryState {
6161
readonly entries: Ref.Ref<ReadonlyMap<ProviderInstanceId, LiveEntry>>;
6262
readonly unavailable: Ref.Ref<ReadonlyMap<ProviderInstanceId, ServerProvider>>;
63+
// Configs parked on an unreadable secret. A settings save that leaves one alone does not read
64+
// its vault again, so a locked vault is not prompted for on every unrelated save.
65+
readonly secretsUnreadable: Ref.Ref<ReadonlyMap<ProviderInstanceId, ProviderInstanceConfig>>;
6366
readonly changes: PubSub.PubSub<void>;
6467
}
6568

@@ -155,7 +158,11 @@ const buildEntry = Effect.fn("ProviderInstanceRegistry.buildEntry")(function* <R
155158
readonly deferSecrets: boolean;
156159
}): Effect.fn.Return<
157160
| { readonly kind: "live"; readonly live: LiveEntry }
158-
| { readonly kind: "unavailable"; readonly snapshot: ServerProvider },
161+
| {
162+
readonly kind: "unavailable";
163+
readonly snapshot: ServerProvider;
164+
readonly secretsUnreadable?: true;
165+
},
159166
never,
160167
R | SecretResolver.SecretResolver
161168
> {
@@ -234,6 +241,7 @@ const buildEntry = Effect.fn("ProviderInstanceRegistry.buildEntry")(function* <R
234241
if (unreadable.length > 0) {
235242
return {
236243
kind: "unavailable" as const,
244+
secretsUnreadable: true as const,
237245
snapshot: yield* buildUnavailableProviderSnapshot({
238246
driverKind: entry.driver,
239247
instanceId,
@@ -304,6 +312,7 @@ const makeReconcile = <R>(input: {
304312
Effect.gen(function* () {
305313
const previousEntries = yield* Ref.get(state.entries);
306314
const previousUnavailable = yield* Ref.get(state.unavailable);
315+
const previousSecretsUnreadable = yield* Ref.get(state.secretsUnreadable);
307316
const nextRaw = Object.entries(configMap);
308317
const nextKeys = new Set<ProviderInstanceId>(
309318
nextRaw.map(([raw]) => ProviderInstanceId.make(raw)),
@@ -338,12 +347,21 @@ const makeReconcile = <R>(input: {
338347
}
339348
}
340349

350+
const parkedSnapshot = (instanceId: ProviderInstanceId, entry: ProviderInstanceConfig) => {
351+
const parked = previousSecretsUnreadable.get(instanceId);
352+
return !rebuild.has(instanceId) && parked !== undefined && entryEqual(parked, entry)
353+
? previousUnavailable.get(instanceId)
354+
: undefined;
355+
};
356+
341357
// Read every secret the builds below need in one go, so a vault unlocks once.
342358
const references = yield* Effect.forEach(nextRaw, ([rawInstanceId, entry]) =>
343359
Effect.gen(function* () {
344360
const instanceId = ProviderInstanceId.make(rawInstanceId);
345361
const kept = previousEntries.has(instanceId) && !replacedIds.has(instanceId);
346-
return kept || !(yield* isEntryEnabled(driversById, entry))
362+
return kept ||
363+
parkedSnapshot(instanceId, entry) !== undefined ||
364+
!(yield* isEntryEnabled(driversById, entry))
347365
? []
348366
: secretReferencesOf(driversById, entry);
349367
}),
@@ -354,6 +372,7 @@ const makeReconcile = <R>(input: {
354372

355373
const builtEntries = new Map<ProviderInstanceId, LiveEntry>();
356374
const builtUnavailable = new Map<ProviderInstanceId, ServerProvider>();
375+
const builtSecretsUnreadable = new Map<ProviderInstanceId, ProviderInstanceConfig>();
357376
let orderChanged = false;
358377
const previousOrder = [...previousEntries.keys()];
359378
const nextOrder: Array<ProviderInstanceId> = [];
@@ -367,6 +386,12 @@ const makeReconcile = <R>(input: {
367386
builtEntries.set(instanceId, existing);
368387
continue;
369388
}
389+
const parked = parkedSnapshot(instanceId, entry);
390+
if (parked !== undefined) {
391+
builtUnavailable.set(instanceId, parked);
392+
builtSecretsUnreadable.set(instanceId, entry);
393+
continue;
394+
}
370395

371396
const result = yield* buildEntry({
372397
driversById,
@@ -381,6 +406,7 @@ const makeReconcile = <R>(input: {
381406
builtEntries.set(instanceId, result.live);
382407
} else {
383408
builtUnavailable.set(instanceId, result.snapshot);
409+
if (result.secretsUnreadable) builtSecretsUnreadable.set(instanceId, entry);
384410
}
385411
}
386412

@@ -410,6 +436,7 @@ const makeReconcile = <R>(input: {
410436

411437
yield* Ref.set(state.entries, builtEntries);
412438
yield* Ref.set(state.unavailable, builtUnavailable);
439+
yield* Ref.set(state.secretsUnreadable, builtSecretsUnreadable);
413440

414441
if (entriesChanged || unavailableChanged) {
415442
yield* PubSub.publish(state.changes, undefined);
@@ -446,10 +473,13 @@ export const makeProviderInstanceRegistry = Effect.fn(
446473

447474
const entries = yield* Ref.make<ReadonlyMap<ProviderInstanceId, LiveEntry>>(new Map());
448475
const unavailable = yield* Ref.make<ReadonlyMap<ProviderInstanceId, ServerProvider>>(new Map());
476+
const secretsUnreadable = yield* Ref.make<
477+
ReadonlyMap<ProviderInstanceId, ProviderInstanceConfig>
478+
>(new Map());
449479
const changes = yield* PubSub.unbounded<void>();
450480
yield* Effect.addFinalizer(() => PubSub.shutdown(changes));
451481

452-
const state: RegistryState = { entries, unavailable, changes };
482+
const state: RegistryState = { entries, unavailable, secretsUnreadable, changes };
453483
const reconcileWithR = makeReconcile({
454484
state,
455485
driversById,
@@ -488,7 +518,10 @@ export const makeProviderInstanceRegistry = Effect.fn(
488518
const rebuild = new Set<ProviderInstanceId>();
489519
for (const [rawInstanceId, entry] of targets) {
490520
const current = live.get(ProviderInstanceId.make(rawInstanceId));
491-
if (current === undefined) continue;
521+
if (current === undefined) {
522+
rebuild.add(ProviderInstanceId.make(rawInstanceId));
523+
continue;
524+
}
492525
const { secrets, unreadable } = yield* readInstanceSecrets(
493526
driversById.get(entry.driver),
494527
entry,

‎apps/server/src/provider/ProviderInstanceRegistryHydration.test.ts‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,27 @@ import {
33
ProjectId,
44
ProviderDriverKind,
55
ProviderInstanceId,
6+
SecretProviderId,
7+
SecretProviderKind,
68
type ServerSettings,
79
} from "@t3tools/contracts";
810
import * as NodeServices from "@effect/platform-node/NodeServices";
911
import { it as effectIt } from "@effect/vitest";
1012
import * as Effect from "effect/Effect";
1113
import * as Layer from "effect/Layer";
14+
import * as Queue from "effect/Queue";
15+
import * as Stream from "effect/Stream";
1216
import { describe, expect, it } from "vite-plus/test";
1317

1418
import * as ServerConfig from "../config.ts";
1519
import { ServerSettingsService } from "../serverSettings.ts";
1620
import {
1721
DEV_REPLAY_INSTANCE_ID,
1822
deriveProviderInstanceConfigMap,
23+
layerSettingsWatcher,
1924
seedDevReplayInstance,
2025
} from "./ProviderInstanceRegistryHydration.ts";
26+
import { ProviderInstanceRegistryMutator } from "./ProviderInstanceRegistryMutator.ts";
2127

2228
const projectId = ProjectId.make("project-a");
2329
const instanceId = ProviderInstanceId.make("codex_work");
@@ -141,3 +147,67 @@ describe("seedDevReplayInstance", () => {
141147
),
142148
);
143149
});
150+
151+
describe("layerSettingsWatcher", () => {
152+
const provider = (label: string) => ({
153+
[SecretProviderId.make("personal")]: {
154+
kind: SecretProviderKind.make("one_password"),
155+
label,
156+
config: { account: "my" },
157+
},
158+
});
159+
160+
const watch = (initial: ServerSettings | undefined) =>
161+
Effect.gen(function* () {
162+
const changes = yield* Queue.unbounded<ServerSettings>();
163+
const reconciled = yield* Queue.unbounded<void>();
164+
let reloads = 0;
165+
const mutator = Layer.succeed(ProviderInstanceRegistryMutator, {
166+
reconcile: () => Queue.offer(reconciled, undefined),
167+
reloadSecrets: () => Effect.sync(() => void reloads++),
168+
});
169+
yield* Layer.build(
170+
layerSettingsWatcher(Stream.fromQueue(changes), initial).pipe(Layer.provide(mutator)),
171+
);
172+
return {
173+
reloads: () => reloads,
174+
// Changes run one at a time, so the repeat's reconcile means the first one's reload decision is done.
175+
change: (settings: ServerSettings) =>
176+
Queue.offerAll(changes, [settings, settings]).pipe(
177+
Effect.andThen(Queue.take(reconciled)),
178+
Effect.andThen(Queue.take(reconciled)),
179+
),
180+
};
181+
});
182+
183+
effectIt.effect(
184+
"retries unreadable secrets when a secret provider changes, not when it is renamed",
185+
() =>
186+
Effect.gen(function* () {
187+
const settings = yield* ServerSettingsService;
188+
const { reloads, change } = yield* watch(yield* settings.getSettings);
189+
190+
yield* change(yield* settings.updateSettings({ secretProviders: provider("Personal") }));
191+
expect(reloads()).toBe(1);
192+
193+
// A label is not read by any vault, so renaming a provider retries nothing.
194+
yield* change(yield* settings.updateSettings({ secretProviders: provider("Renamed") }));
195+
expect(reloads()).toBe(1);
196+
}).pipe(Effect.scoped, Effect.provide(ServerSettingsService.layerTest())),
197+
);
198+
199+
effectIt.effect(
200+
"compares a change queued during startup with the settings the registry started from",
201+
() =>
202+
Effect.gen(function* () {
203+
const settings = yield* ServerSettingsService;
204+
const started = yield* settings.getSettings;
205+
const saved = yield* settings.updateSettings({ secretProviders: provider("Personal") });
206+
const { reloads, change } = yield* watch(started);
207+
208+
yield* change(saved);
209+
210+
expect(reloads()).toBe(1);
211+
}).pipe(Effect.scoped, Effect.provide(ServerSettingsService.layerTest())),
212+
);
213+
});

‎apps/server/src/provider/ProviderInstanceRegistryHydration.ts‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import {
88
type ProviderInstanceConfigMap,
99
ServerSettings,
1010
} from "@t3tools/contracts";
11+
import * as NodeUtil from "node:util";
12+
1113
import * as HostProcess from "@t3tools/shared/HostProcess";
1214
import type * as SecureRandom from "effect/crypto/SecureRandom";
1315
import * as Effect from "effect/Effect";
@@ -122,18 +124,36 @@ export const seedDevReplayInstance = Effect.fn(
122124
});
123125
});
124126

125-
const layerSettingsWatcher = (settingsChanges: Stream.Stream<ServerSettings>) =>
127+
// A label is display-only, so only kind and config can change what a vault returns.
128+
const vaultConfigs = (settings: ServerSettings) =>
129+
Object.entries(settings.secretProviders).map(([id, { kind, config }]) => [id, kind, config]);
130+
131+
export const layerSettingsWatcher = (
132+
settingsChanges: Stream.Stream<ServerSettings>,
133+
// The settings the registry was built from: a change queued during startup differs from these.
134+
initialSettings: ServerSettings | undefined,
135+
) =>
126136
Layer.effectDiscard(
127137
Effect.gen(function* () {
128138
const mutator = yield* ProviderInstanceRegistryMutator.ProviderInstanceRegistryMutator;
129139
const serverSettings = yield* Settings.ServerSettingsService;
140+
let secretProviders = initialSettings && vaultConfigs(initialSettings);
130141
yield* settingsChanges.pipe(
131142
Stream.runForEach(() =>
132143
// The subscription predates the seed read, so a queued change can be
133144
// older than the seed. A failed current read must leave the registry intact.
134145
serverSettings.getSettings.pipe(
135146
Effect.flatMap((current) =>
136-
mutator.reconcile(deriveProviderInstanceConfigMap(current)),
147+
Effect.gen(function* () {
148+
yield* mutator.reconcile(deriveProviderInstanceConfigMap(current));
149+
// Reconcile leaves instances parked on an unreadable secret alone,
150+
// and a corrected secret provider is what can make them readable.
151+
const next = vaultConfigs(current);
152+
if (!NodeUtil.isDeepStrictEqual(secretProviders, next)) {
153+
secretProviders = next;
154+
yield* mutator.reloadSecrets();
155+
}
156+
}),
137157
),
138158
Effect.catchCause((cause) =>
139159
Effect.logError("ProviderInstanceRegistry reconcile failed", cause),
@@ -186,7 +206,9 @@ export const layer: Layer.Layer<
186206
Layer.provide(SecretResolver.layer),
187207
);
188208

189-
return layerSettingsWatcher(settingsChanges).pipe(Layer.provideMerge(layerMutable));
209+
return layerSettingsWatcher(settingsChanges, initialSettings).pipe(
210+
Layer.provideMerge(layerMutable),
211+
);
190212
}),
191213
) as Layer.Layer<
192214
| ProviderInstanceRegistry.ProviderInstanceRegistry

0 commit comments

Comments
 (0)