diff --git a/apps/server/src/serverSettings.test.ts b/apps/server/src/serverSettings.test.ts index 208d75fb6517951515e39789a22994f5f6c415e0..bd216a0e88a1b34b077fa6cc6b64810beecbf075 100644 --- a/apps/server/src/serverSettings.test.ts +++ b/apps/server/src/serverSettings.test.ts @@ -1184,6 +1184,34 @@ it.layer(NodeServices.layer)("server settings", (it) => { ); } + for (const sensitiveLast of [true, false]) { + it.effect(`preserves duplicate secret operation order (sensitive last: ${sensitiveLast})`, () => + Effect.gen(function* () { + const service = yield* ServerSettingsModule.ServerSettingsService; + const instanceId = ProviderInstanceId.make("codex_duplicate"); + const secret = { name: "API_TOKEN", value: "secret-last", sensitive: true }; + const plain = { name: "API_TOKEN", value: "plain-last", sensitive: false }; + const next = yield* service.updateSettings({ + providerInstances: { + [instanceId]: { + driver: ProviderDriverKind.make("codex"), + environment: sensitiveLast ? [plain, secret] : [secret, plain], + config: {}, + }, + }, + }); + assert.equal( + next.providerInstances[instanceId]?.environment?.at(-1)?.value, + sensitiveLast ? "secret-last" : "plain-last", + ); + assert.equal( + next.providerInstances[instanceId]?.environment?.find((v) => v.sensitive)?.value, + sensitiveLast ? "secret-last" : "", + ); + }).pipe(Effect.provide(makeServerSettingsLayer())), + ); + } + it.effect("stores sensitive provider instance environment values outside settings.json", () => Effect.gen(function* () { const serverSettings = yield* ServerSettingsModule.ServerSettingsService; @@ -1283,6 +1311,181 @@ it.layer(NodeServices.layer)("server settings", (it) => { }).pipe(Effect.provide(makeServerSettingsLayer())), ); + it.effect("rolls back provider secret changes when the settings file commit fails", () => + Effect.gen(function* () { + const fileSystem = yield* FileSystem.FileSystem; + let failRename = false; + let settingsPathToFail: string | undefined; + const writeFailure = PlatformError.systemError({ + _tag: "PermissionDenied", + module: "FileSystem", + method: "rename", + description: "Forced settings write failure.", + }); + const failingFileSystem = FileSystem.FileSystem.of({ + ...fileSystem, + rename: (fromPath, toPath) => + failRename && toPath === settingsPathToFail + ? Effect.fail(writeFailure) + : fileSystem.rename(fromPath, toPath), + }); + const instanceId = ProviderInstanceId.make("codex_write_failure"); + const settingsLayer = makeServerSettingsLayer().pipe( + Layer.provideMerge(Layer.succeed(FileSystem.FileSystem, failingFileSystem)), + ); + + yield* Effect.gen(function* () { + const serverSettings = yield* ServerSettingsModule.ServerSettingsService; + settingsPathToFail = (yield* ServerConfig.ServerConfig).settingsPath; + yield* serverSettings.updateSettings({ + providerInstances: { + [instanceId]: { + driver: ProviderDriverKind.make("codex"), + environment: [{ name: "OPENROUTER_API_KEY", value: "sk-kept", sensitive: true }], + config: {}, + }, + }, + }); + + failRename = true; + const failedUpdate = yield* serverSettings + .updateSettings({ + providerInstances: { + [instanceId]: { + driver: ProviderDriverKind.make("codex"), + environment: [{ name: "OPENROUTER_API_KEY", value: "sk-new", sensitive: true }], + config: {}, + }, + }, + }) + .pipe(Effect.result); + assert.equal(failedUpdate._tag, "Failure"); + assert.equal( + (yield* serverSettings.getSettings).providerInstances[instanceId]?.environment?.[0] + ?.value, + "sk-kept", + ); + + const failed = yield* serverSettings + .updateSettings({ providerInstances: {} }) + .pipe(Effect.result); + assert.equal(failed._tag, "Failure"); + assert.equal( + (yield* serverSettings.getSettings).providerInstances[instanceId]?.environment?.[0] + ?.value, + "sk-kept", + ); + }).pipe(Effect.provide(settingsLayer)); + }), + ); + + for (const failure of ["response materialization", "partially committed write"] as const) { + it.effect(`rolls back provider secret changes after ${failure} fails`, () => { + const textDecoder = new TextDecoder(); + const secrets = new Map(); + let rejectNewSecret = false; + const secretStoreLayer = Layer.succeed( + ServerSecretStore.ServerSecretStore, + ServerSecretStore.ServerSecretStore.of({ + get: (name) => + Effect.suspend(() => { + const value = secrets.get(name); + if ( + failure === "response materialization" && + rejectNewSecret && + value !== undefined && + textDecoder.decode(value) === "sk-new" + ) { + return Effect.fail( + new ServerSecretStore.SecretStoreReadError({ + resource: `secret ${name}`, + cause: "Forced response materialization failure.", + }), + ); + } + return Effect.succeed( + value === undefined ? Option.none() : Option.some(Uint8Array.from(value)), + ); + }), + set: (name, value) => + Effect.suspend(() => { + secrets.set(name, Uint8Array.from(value)); + return failure === "partially committed write" && + rejectNewSecret && + textDecoder.decode(value) === "sk-new" + ? Effect.fail( + new ServerSecretStore.SecretStorePersistError({ + resource: `secret ${name}`, + cause: "chmod failed after rename", + }), + ) + : Effect.void; + }), + create: (name, value) => + Effect.sync(() => { + secrets.set(name, Uint8Array.from(value)); + }), + getOrCreateRandom: (name, bytes) => + Effect.sync(() => { + const value = secrets.get(name) ?? new Uint8Array(bytes); + secrets.set(name, value); + return Uint8Array.from(value); + }), + remove: (name) => + Effect.sync(() => { + secrets.delete(name); + }), + }), + ); + const settingsLayer = ServerSettingsModule.layer.pipe( + Layer.provideMerge(Layer.fresh(SqlitePersistenceMemory)), + Layer.provide(secretStoreLayer), + Layer.provideMerge( + Layer.fresh( + ServerConfig.layerTest(process.cwd(), { + prefix: "t3code-server-settings-materialization-failure-test-", + }), + ), + ), + ); + const instanceId = ProviderInstanceId.make("codex_materialization_failure"); + + return Effect.gen(function* () { + const serverSettings = yield* ServerSettingsModule.ServerSettingsService; + yield* serverSettings.updateSettings({ + providerInstances: { + [instanceId]: { + driver: ProviderDriverKind.make("codex"), + environment: [{ name: "OPENROUTER_API_KEY", value: "sk-kept", sensitive: true }], + config: {}, + }, + }, + }); + + rejectNewSecret = true; + const failedUpdate = yield* serverSettings + .updateSettings({ + providerInstances: { + [instanceId]: { + driver: ProviderDriverKind.make("codex"), + environment: [{ name: "OPENROUTER_API_KEY", value: "sk-new", sensitive: true }], + config: {}, + }, + }, + }) + .pipe(Effect.result); + + assert.equal(failedUpdate._tag, "Failure"); + rejectNewSecret = false; + assert.equal( + (yield* serverSettings.getSettings).providerInstances[instanceId]?.environment?.[0] + ?.value, + "sk-kept", + ); + }).pipe(Effect.provide(settingsLayer)); + }); + } + it.effect("folds legacy project overrides into projectSettingsOverrides once", () => Effect.gen(function* () { const serverConfig = yield* ServerConfig.ServerConfig; diff --git a/apps/server/src/serverSettings.ts b/apps/server/src/serverSettings.ts index 50f8649eaacb061c3d32d9a3c3c3b2b2559b3549..ed52282c02377a2d86304eca155af240420019e7 100644 --- a/apps/server/src/serverSettings.ts +++ b/apps/server/src/serverSettings.ts @@ -733,14 +733,21 @@ const make = Effect.gen(function* () { Stream.map(resolveTextGenerationProvider), ); - const persistProviderEnvironmentSecrets = ( - current: ServerSettings, - next: ServerSettings, - ): Effect.Effect => - Effect.gen(function* () { + type SecretChange = { + readonly secretName: string; + readonly providerInstanceId?: string; + readonly environmentVariable?: string; + } & ( + | { readonly kind: "write"; readonly value: Uint8Array } + | { readonly kind: "remove"; readonly operation: "remove-secret" | "remove-stale-secret" } + ); + + const persistProviderEnvironmentSecrets = (current: ServerSettings, next: ServerSettings) => + Effect.sync(() => { const providerInstances: Record = { ...next.providerInstances, }; + const changes: SecretChange[] = []; const nextSecretKeys = new Set(); for (const [instanceId, instance] of Object.entries(next.providerInstances)) { @@ -749,18 +756,13 @@ const make = Effect.gen(function* () { for (const variable of instance.environment) { const secretName = providerEnvironmentSecretName({ instanceId, name: variable.name }); if (!variable.sensitive) { - yield* secretStore.remove(secretName).pipe( - Effect.mapError( - (cause) => - new ServerSettingsError({ - settingsPath, - operation: "remove-secret", - providerInstanceId: instanceId, - environmentVariable: variable.name, - cause, - }), - ), - ); + changes.push({ + kind: "remove", + secretName, + operation: "remove-secret", + providerInstanceId: instanceId, + environmentVariable: variable.name, + }); environment.push(redactProviderEnvironmentVariable(variable)); continue; } @@ -779,32 +781,22 @@ const make = Effect.gen(function* () { const value = inlineValue ?? variable.value; if (!variable.valueRedacted || inlineValue !== undefined) { if (value.length > 0) { - yield* secretStore.set(secretName, textEncoder.encode(value)).pipe( - Effect.mapError( - (cause) => - new ServerSettingsError({ - settingsPath, - operation: "write-secret", - providerInstanceId: instanceId, - environmentVariable: variable.name, - cause, - }), - ), - ); + changes.push({ + kind: "write", + secretName, + value: textEncoder.encode(value), + providerInstanceId: instanceId, + environmentVariable: variable.name, + }); environment.push({ ...variable, value: "", valueRedacted: true }); } else { - yield* secretStore.remove(secretName).pipe( - Effect.mapError( - (cause) => - new ServerSettingsError({ - settingsPath, - operation: "remove-secret", - providerInstanceId: instanceId, - environmentVariable: variable.name, - cause, - }), - ), - ); + changes.push({ + kind: "remove", + secretName, + operation: "remove-secret", + providerInstanceId: instanceId, + environmentVariable: variable.name, + }); const { valueRedacted: _omit, ...rest } = variable; environment.push(rest); } @@ -824,18 +816,13 @@ const make = Effect.gen(function* () { if (!variable.sensitive) continue; const secretName = providerEnvironmentSecretName({ instanceId, name: variable.name }); if (nextSecretKeys.has(secretName)) continue; - yield* secretStore.remove(secretName).pipe( - Effect.mapError( - (cause) => - new ServerSettingsError({ - settingsPath, - operation: "remove-stale-secret", - providerInstanceId: instanceId, - environmentVariable: variable.name, - cause, - }), - ), - ); + changes.push({ + kind: "remove", + secretName, + operation: "remove-stale-secret", + providerInstanceId: instanceId, + environmentVariable: variable.name, + }); } } @@ -843,51 +830,151 @@ const make = Effect.gen(function* () { for (const [sourceId, source] of Object.entries(next.usageLimitSources)) { const secretName = usageLimitSourceSecretName(sourceId); if (source.managementKey === USAGE_LIMIT_SOURCE_KEY_REDACTED) { - // Unchanged from the client's point of view; the store already has it. usageLimitSources[sourceId] = source; continue; } if (source.managementKey.length === 0) { - yield* secretStore - .remove(secretName) - .pipe( - Effect.mapError( - (cause) => - new ServerSettingsError({ settingsPath, operation: "remove-secret", cause }), - ), - ); + changes.push({ kind: "remove", secretName, operation: "remove-secret" }); usageLimitSources[sourceId] = source; continue; } - yield* secretStore - .set(secretName, textEncoder.encode(source.managementKey)) - .pipe( - Effect.mapError( - (cause) => - new ServerSettingsError({ settingsPath, operation: "write-secret", cause }), - ), - ); + changes.push({ + kind: "write", + secretName, + value: textEncoder.encode(source.managementKey), + }); usageLimitSources[sourceId] = { ...source, managementKey: USAGE_LIMIT_SOURCE_KEY_REDACTED }; } for (const sourceId of Object.keys(current.usageLimitSources)) { if (sourceId in next.usageLimitSources) continue; - yield* secretStore - .remove(usageLimitSourceSecretName(sourceId)) - .pipe( - Effect.mapError( - (cause) => - new ServerSettingsError({ settingsPath, operation: "remove-stale-secret", cause }), - ), - ); + changes.push({ + kind: "remove", + secretName: usageLimitSourceSecretName(sourceId), + operation: "remove-stale-secret", + }); } return { - ...next, - providerInstances: providerInstances as ServerSettings["providerInstances"], - usageLimitSources: usageLimitSources as ServerSettings["usageLimitSources"], + settings: { + ...next, + providerInstances: providerInstances as ServerSettings["providerInstances"], + usageLimitSources: usageLimitSources as ServerSettings["usageLimitSources"], + }, + changes, }; }); + const rollbackProviderEnvironmentSecretWrites = ( + writes: ReadonlyArray<{ + readonly secretName: string; + readonly previousValue: Option.Option; + readonly providerInstanceId?: string; + readonly environmentVariable?: string; + }>, + ) => + Effect.forEach( + writes.toReversed(), + (write) => + (Option.isSome(write.previousValue) + ? secretStore.set(write.secretName, write.previousValue.value) + : secretStore.remove(write.secretName) + ).pipe( + Effect.catch((cause) => + Effect.logWarning("failed to roll back provider environment secret", { + providerInstanceId: write.providerInstanceId, + environmentVariable: write.environmentVariable, + cause, + }), + ), + ), + { discard: true }, + ); + + const applyProviderEnvironmentSecretChanges = (changes: ReadonlyArray) => { + const applied: Array<{ + readonly secretName: string; + readonly previousValue: Option.Option; + readonly providerInstanceId?: string; + readonly environmentVariable?: string; + }> = []; + const rollback = Effect.suspend(() => rollbackProviderEnvironmentSecretWrites(applied)); + return Effect.forEach( + changes, + (change) => + Effect.gen(function* () { + const previousValue = yield* secretStore.get(change.secretName).pipe( + Effect.mapError( + (cause) => + new ServerSettingsError({ + settingsPath, + operation: "read-secret", + providerInstanceId: change.providerInstanceId, + environmentVariable: change.environmentVariable, + cause, + }), + ), + ); + // A store operation may mutate before reporting an error (for example chmod after rename). + applied.push({ ...change, previousValue }); + yield* ( + change.kind === "write" + ? secretStore.set(change.secretName, change.value) + : secretStore.remove(change.secretName) + ).pipe( + Effect.mapError( + (cause) => + new ServerSettingsError({ + settingsPath, + operation: change.kind === "write" ? "write-secret" : change.operation, + providerInstanceId: change.providerInstanceId, + environmentVariable: change.environmentVariable, + cause, + }), + ), + ); + }), + { discard: true }, + ).pipe( + Effect.tapError(() => rollback), + Effect.as(rollback), + ); + }; + + const updateSettings = ( + patch: ServerSettingsPatch, + ): Effect.Effect => + writeSemaphore.withPermits(1)( + Effect.gen(function* () { + const current = yield* getSettingsFromCache; + const updated = applyServerSettingsPatch(current, patch); + const persisted = yield* persistProviderEnvironmentSecrets(current, updated); + const next = yield* normalizeServerSettings(persisted.settings); + const materialized = yield* Effect.uninterruptibleMask(() => + Effect.gen(function* () { + const rollbackSecretChanges = yield* applyProviderEnvironmentSecretChanges( + persisted.changes, + ); + const materializedExit = yield* Effect.exit( + materializeProviderEnvironmentSecrets(next), + ); + if (Exit.isFailure(materializedExit)) { + yield* rollbackSecretChanges; + return yield* Effect.failCause(materializedExit.cause); + } + const writeExit = yield* Effect.exit(writeSettingsAtomically(next)); + if (Exit.isFailure(writeExit)) { + yield* rollbackSecretChanges; + return yield* Effect.failCause(writeExit.cause); + } + return materializedExit.value; + }), + ); + yield* Cache.set(settingsCache, cacheKey, next); + yield* emitChange(next); + return resolveTextGenerationProvider(materialized); + }), + ); + const revalidateAndEmit = writeSemaphore.withPermits(1)( Effect.gen(function* () { yield* Cache.invalidate(settingsCache, cacheKey); @@ -963,22 +1050,7 @@ const make = Effect.gen(function* () { Effect.flatMap(materializeProviderEnvironmentSecrets), Effect.map(resolveTextGenerationProvider), ), - updateSettings: (patch) => - writeSemaphore.withPermits(1)( - Effect.gen(function* () { - const current = yield* getSettingsFromCache; - const nextPersisted = yield* persistProviderEnvironmentSecrets( - current, - applyServerSettingsPatch(current, patch), - ); - const next = yield* normalizeServerSettings(nextPersisted); - yield* writeSettingsAtomically(next); - yield* Cache.set(settingsCache, cacheKey, next); - yield* emitChange(next); - const materialized = yield* materializeProviderEnvironmentSecrets(next); - return resolveTextGenerationProvider(materialized); - }), - ), + updateSettings, get streamChanges() { return materializeChanges(Stream.fromPubSub(changesPubSub)); },