authorgravatar for julius0216@outlook.comJulius Marminge <julius0216@outlook.com> 2026-09-18 11:20:06-07:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-09-18 11:20:06-07:00
log6d68677ff90915c17de06d78bb498d41a1adfe6a
treefdb73c3497c9aa9d7e3ac9dfb33048a431972552
parentc557bb10aa7cbb78a50df230d1e8e24af79cb662
signature Signed by PGP key B5690EEEBB952194

fix(server): restore secrets when settings persistence fails (#12487)


2 files changed, 372 insertions(+), 97 deletions(-)

apps/server/src/serverSettings.test.ts+203
...@@ -1184,6 +1184,34 @@ it.layer(NodeServices.layer)("server settings", (it) => {...@@ -1184,6 +1184,34 @@ it.layer(NodeServices.layer)("server settings", (it) => {
1184 );1184 );
1185 }1185 }
11861186
1187 for (const sensitiveLast of [true, false]) {
1188 it.effect(`preserves duplicate secret operation order (sensitive last: ${sensitiveLast})`, () =>
1189 Effect.gen(function* () {
1190 const service = yield* ServerSettingsModule.ServerSettingsService;
1191 const instanceId = ProviderInstanceId.make("codex_duplicate");
1192 const secret = { name: "API_TOKEN", value: "secret-last", sensitive: true };
1193 const plain = { name: "API_TOKEN", value: "plain-last", sensitive: false };
1194 const next = yield* service.updateSettings({
1195 providerInstances: {
1196 [instanceId]: {
1197 driver: ProviderDriverKind.make("codex"),
1198 environment: sensitiveLast ? [plain, secret] : [secret, plain],
1199 config: {},
1200 },
1201 },
1202 });
1203 assert.equal(
1204 next.providerInstances[instanceId]?.environment?.at(-1)?.value,
1205 sensitiveLast ? "secret-last" : "plain-last",
1206 );
1207 assert.equal(
1208 next.providerInstances[instanceId]?.environment?.find((v) => v.sensitive)?.value,
1209 sensitiveLast ? "secret-last" : "",
1210 );
1211 }).pipe(Effect.provide(makeServerSettingsLayer())),
1212 );
1213 }
1214
1187 it.effect("stores sensitive provider instance environment values outside settings.json", () =>1215 it.effect("stores sensitive provider instance environment values outside settings.json", () =>
1188 Effect.gen(function* () {1216 Effect.gen(function* () {
1189 const serverSettings = yield* ServerSettingsModule.ServerSettingsService;1217 const serverSettings = yield* ServerSettingsModule.ServerSettingsService;
...@@ -1283,6 +1311,181 @@ it.layer(NodeServices.layer)("server settings", (it) => {...@@ -1283,6 +1311,181 @@ it.layer(NodeServices.layer)("server settings", (it) => {
1283 }).pipe(Effect.provide(makeServerSettingsLayer())),1311 }).pipe(Effect.provide(makeServerSettingsLayer())),
1284 );1312 );
12851313
1314 it.effect("rolls back provider secret changes when the settings file commit fails", () =>
1315 Effect.gen(function* () {
1316 const fileSystem = yield* FileSystem.FileSystem;
1317 let failRename = false;
1318 let settingsPathToFail: string | undefined;
1319 const writeFailure = PlatformError.systemError({
1320 _tag: "PermissionDenied",
1321 module: "FileSystem",
1322 method: "rename",
1323 description: "Forced settings write failure.",
1324 });
1325 const failingFileSystem = FileSystem.FileSystem.of({
1326 ...fileSystem,
1327 rename: (fromPath, toPath) =>
1328 failRename && toPath === settingsPathToFail
1329 ? Effect.fail(writeFailure)
1330 : fileSystem.rename(fromPath, toPath),
1331 });
1332 const instanceId = ProviderInstanceId.make("codex_write_failure");
1333 const settingsLayer = makeServerSettingsLayer().pipe(
1334 Layer.provideMerge(Layer.succeed(FileSystem.FileSystem, failingFileSystem)),
1335 );
1336
1337 yield* Effect.gen(function* () {
1338 const serverSettings = yield* ServerSettingsModule.ServerSettingsService;
1339 settingsPathToFail = (yield* ServerConfig.ServerConfig).settingsPath;
1340 yield* serverSettings.updateSettings({
1341 providerInstances: {
1342 [instanceId]: {
1343 driver: ProviderDriverKind.make("codex"),
1344 environment: [{ name: "OPENROUTER_API_KEY", value: "sk-kept", sensitive: true }],
1345 config: {},
1346 },
1347 },
1348 });
1349
1350 failRename = true;
1351 const failedUpdate = yield* serverSettings
1352 .updateSettings({
1353 providerInstances: {
1354 [instanceId]: {
1355 driver: ProviderDriverKind.make("codex"),
1356 environment: [{ name: "OPENROUTER_API_KEY", value: "sk-new", sensitive: true }],
1357 config: {},
1358 },
1359 },
1360 })
1361 .pipe(Effect.result);
1362 assert.equal(failedUpdate._tag, "Failure");
1363 assert.equal(
1364 (yield* serverSettings.getSettings).providerInstances[instanceId]?.environment?.[0]
1365 ?.value,
1366 "sk-kept",
1367 );
1368
1369 const failed = yield* serverSettings
1370 .updateSettings({ providerInstances: {} })
1371 .pipe(Effect.result);
1372 assert.equal(failed._tag, "Failure");
1373 assert.equal(
1374 (yield* serverSettings.getSettings).providerInstances[instanceId]?.environment?.[0]
1375 ?.value,
1376 "sk-kept",
1377 );
1378 }).pipe(Effect.provide(settingsLayer));
1379 }),
1380 );
1381
1382 for (const failure of ["response materialization", "partially committed write"] as const) {
1383 it.effect(`rolls back provider secret changes after ${failure} fails`, () => {
1384 const textDecoder = new TextDecoder();
1385 const secrets = new Map<string, Uint8Array>();
1386 let rejectNewSecret = false;
1387 const secretStoreLayer = Layer.succeed(
1388 ServerSecretStore.ServerSecretStore,
1389 ServerSecretStore.ServerSecretStore.of({
1390 get: (name) =>
1391 Effect.suspend(() => {
1392 const value = secrets.get(name);
1393 if (
1394 failure === "response materialization" &&
1395 rejectNewSecret &&
1396 value !== undefined &&
1397 textDecoder.decode(value) === "sk-new"
1398 ) {
1399 return Effect.fail(
1400 new ServerSecretStore.SecretStoreReadError({
1401 resource: `secret ${name}`,
1402 cause: "Forced response materialization failure.",
1403 }),
1404 );
1405 }
1406 return Effect.succeed(
1407 value === undefined ? Option.none() : Option.some(Uint8Array.from(value)),
1408 );
1409 }),
1410 set: (name, value) =>
1411 Effect.suspend(() => {
1412 secrets.set(name, Uint8Array.from(value));
1413 return failure === "partially committed write" &&
1414 rejectNewSecret &&
1415 textDecoder.decode(value) === "sk-new"
1416 ? Effect.fail(
1417 new ServerSecretStore.SecretStorePersistError({
1418 resource: `secret ${name}`,
1419 cause: "chmod failed after rename",
1420 }),
1421 )
1422 : Effect.void;
1423 }),
1424 create: (name, value) =>
1425 Effect.sync(() => {
1426 secrets.set(name, Uint8Array.from(value));
1427 }),
1428 getOrCreateRandom: (name, bytes) =>
1429 Effect.sync(() => {
1430 const value = secrets.get(name) ?? new Uint8Array(bytes);
1431 secrets.set(name, value);
1432 return Uint8Array.from(value);
1433 }),
1434 remove: (name) =>
1435 Effect.sync(() => {
1436 secrets.delete(name);
1437 }),
1438 }),
1439 );
1440 const settingsLayer = ServerSettingsModule.layer.pipe(
1441 Layer.provideMerge(Layer.fresh(SqlitePersistenceMemory)),
1442 Layer.provide(secretStoreLayer),
1443 Layer.provideMerge(
1444 Layer.fresh(
1445 ServerConfig.layerTest(process.cwd(), {
1446 prefix: "t3code-server-settings-materialization-failure-test-",
1447 }),
1448 ),
1449 ),
1450 );
1451 const instanceId = ProviderInstanceId.make("codex_materialization_failure");
1452
1453 return Effect.gen(function* () {
1454 const serverSettings = yield* ServerSettingsModule.ServerSettingsService;
1455 yield* serverSettings.updateSettings({
1456 providerInstances: {
1457 [instanceId]: {
1458 driver: ProviderDriverKind.make("codex"),
1459 environment: [{ name: "OPENROUTER_API_KEY", value: "sk-kept", sensitive: true }],
1460 config: {},
1461 },
1462 },
1463 });
1464
1465 rejectNewSecret = true;
1466 const failedUpdate = yield* serverSettings
1467 .updateSettings({
1468 providerInstances: {
1469 [instanceId]: {
1470 driver: ProviderDriverKind.make("codex"),
1471 environment: [{ name: "OPENROUTER_API_KEY", value: "sk-new", sensitive: true }],
1472 config: {},
1473 },
1474 },
1475 })
1476 .pipe(Effect.result);
1477
1478 assert.equal(failedUpdate._tag, "Failure");
1479 rejectNewSecret = false;
1480 assert.equal(
1481 (yield* serverSettings.getSettings).providerInstances[instanceId]?.environment?.[0]
1482 ?.value,
1483 "sk-kept",
1484 );
1485 }).pipe(Effect.provide(settingsLayer));
1486 });
1487 }
1488
1286 it.effect("folds legacy project overrides into projectSettingsOverrides once", () =>1489 it.effect("folds legacy project overrides into projectSettingsOverrides once", () =>
1287 Effect.gen(function* () {1490 Effect.gen(function* () {
1288 const serverConfig = yield* ServerConfig.ServerConfig;1491 const serverConfig = yield* ServerConfig.ServerConfig;
apps/server/src/serverSettings.ts+169-97
...@@ -733,14 +733,21 @@ const make = Effect.gen(function* () {...@@ -733,14 +733,21 @@ const make = Effect.gen(function* () {
733 Stream.map(resolveTextGenerationProvider),733 Stream.map(resolveTextGenerationProvider),
734 );734 );
735735
736 const persistProviderEnvironmentSecrets = (736 type SecretChange = {
737 current: ServerSettings,737 readonly secretName: string;
738 next: ServerSettings,738 readonly providerInstanceId?: string;
739 ): Effect.Effect<ServerSettings, ServerSettingsError> =>739 readonly environmentVariable?: string;
740 Effect.gen(function* () {740 } & (
741 | { readonly kind: "write"; readonly value: Uint8Array }
742 | { readonly kind: "remove"; readonly operation: "remove-secret" | "remove-stale-secret" }
743 );
744
745 const persistProviderEnvironmentSecrets = (current: ServerSettings, next: ServerSettings) =>
746 Effect.sync(() => {
741 const providerInstances: Record<string, ProviderInstanceConfig> = {747 const providerInstances: Record<string, ProviderInstanceConfig> = {
742 ...next.providerInstances,748 ...next.providerInstances,
743 };749 };
750 const changes: SecretChange[] = [];
744751
745 const nextSecretKeys = new Set<string>();752 const nextSecretKeys = new Set<string>();
746 for (const [instanceId, instance] of Object.entries(next.providerInstances)) {753 for (const [instanceId, instance] of Object.entries(next.providerInstances)) {
...@@ -749,18 +756,13 @@ const make = Effect.gen(function* () {...@@ -749,18 +756,13 @@ const make = Effect.gen(function* () {
749 for (const variable of instance.environment) {756 for (const variable of instance.environment) {
750 const secretName = providerEnvironmentSecretName({ instanceId, name: variable.name });757 const secretName = providerEnvironmentSecretName({ instanceId, name: variable.name });
751 if (!variable.sensitive) {758 if (!variable.sensitive) {
752 yield* secretStore.remove(secretName).pipe(759 changes.push({
753 Effect.mapError(760 kind: "remove",
754 (cause) =>761 secretName,
755 new ServerSettingsError({762 operation: "remove-secret",
756 settingsPath,763 providerInstanceId: instanceId,
757 operation: "remove-secret",764 environmentVariable: variable.name,
758 providerInstanceId: instanceId,765 });
759 environmentVariable: variable.name,
760 cause,
761 }),
762 ),
763 );
764 environment.push(redactProviderEnvironmentVariable(variable));766 environment.push(redactProviderEnvironmentVariable(variable));
765 continue;767 continue;
766 }768 }
...@@ -779,32 +781,22 @@ const make = Effect.gen(function* () {...@@ -779,32 +781,22 @@ const make = Effect.gen(function* () {
779 const value = inlineValue ?? variable.value;781 const value = inlineValue ?? variable.value;
780 if (!variable.valueRedacted || inlineValue !== undefined) {782 if (!variable.valueRedacted || inlineValue !== undefined) {
781 if (value.length > 0) {783 if (value.length > 0) {
782 yield* secretStore.set(secretName, textEncoder.encode(value)).pipe(784 changes.push({
783 Effect.mapError(785 kind: "write",
784 (cause) =>786 secretName,
785 new ServerSettingsError({787 value: textEncoder.encode(value),
786 settingsPath,788 providerInstanceId: instanceId,
787 operation: "write-secret",789 environmentVariable: variable.name,
788 providerInstanceId: instanceId,790 });
789 environmentVariable: variable.name,
790 cause,
791 }),
792 ),
793 );
794 environment.push({ ...variable, value: "", valueRedacted: true });791 environment.push({ ...variable, value: "", valueRedacted: true });
795 } else {792 } else {
796 yield* secretStore.remove(secretName).pipe(793 changes.push({
797 Effect.mapError(794 kind: "remove",
798 (cause) =>795 secretName,
799 new ServerSettingsError({796 operation: "remove-secret",
800 settingsPath,797 providerInstanceId: instanceId,
801 operation: "remove-secret",798 environmentVariable: variable.name,
802 providerInstanceId: instanceId,799 });
803 environmentVariable: variable.name,
804 cause,
805 }),
806 ),
807 );
808 const { valueRedacted: _omit, ...rest } = variable;800 const { valueRedacted: _omit, ...rest } = variable;
809 environment.push(rest);801 environment.push(rest);
810 }802 }
...@@ -824,18 +816,13 @@ const make = Effect.gen(function* () {...@@ -824,18 +816,13 @@ const make = Effect.gen(function* () {
824 if (!variable.sensitive) continue;816 if (!variable.sensitive) continue;
825 const secretName = providerEnvironmentSecretName({ instanceId, name: variable.name });817 const secretName = providerEnvironmentSecretName({ instanceId, name: variable.name });
826 if (nextSecretKeys.has(secretName)) continue;818 if (nextSecretKeys.has(secretName)) continue;
827 yield* secretStore.remove(secretName).pipe(819 changes.push({
828 Effect.mapError(820 kind: "remove",
829 (cause) =>821 secretName,
830 new ServerSettingsError({822 operation: "remove-stale-secret",
831 settingsPath,823 providerInstanceId: instanceId,
832 operation: "remove-stale-secret",824 environmentVariable: variable.name,
833 providerInstanceId: instanceId,825 });
834 environmentVariable: variable.name,
835 cause,
836 }),
837 ),
838 );
839 }826 }
840 }827 }
841828
...@@ -843,51 +830,151 @@ const make = Effect.gen(function* () {...@@ -843,51 +830,151 @@ const make = Effect.gen(function* () {
843 for (const [sourceId, source] of Object.entries(next.usageLimitSources)) {830 for (const [sourceId, source] of Object.entries(next.usageLimitSources)) {
844 const secretName = usageLimitSourceSecretName(sourceId);831 const secretName = usageLimitSourceSecretName(sourceId);
845 if (source.managementKey === USAGE_LIMIT_SOURCE_KEY_REDACTED) {832 if (source.managementKey === USAGE_LIMIT_SOURCE_KEY_REDACTED) {
846 // Unchanged from the client's point of view; the store already has it.
847 usageLimitSources[sourceId] = source;833 usageLimitSources[sourceId] = source;
848 continue;834 continue;
849 }835 }
850 if (source.managementKey.length === 0) {836 if (source.managementKey.length === 0) {
851 yield* secretStore837 changes.push({ kind: "remove", secretName, operation: "remove-secret" });
852 .remove(secretName)
853 .pipe(
854 Effect.mapError(
855 (cause) =>
856 new ServerSettingsError({ settingsPath, operation: "remove-secret", cause }),
857 ),
858 );
859 usageLimitSources[sourceId] = source;838 usageLimitSources[sourceId] = source;
860 continue;839 continue;
861 }840 }
862 yield* secretStore841 changes.push({
863 .set(secretName, textEncoder.encode(source.managementKey))842 kind: "write",
864 .pipe(843 secretName,
865 Effect.mapError(844 value: textEncoder.encode(source.managementKey),
866 (cause) =>845 });
867 new ServerSettingsError({ settingsPath, operation: "write-secret", cause }),
868 ),
869 );
870 usageLimitSources[sourceId] = { ...source, managementKey: USAGE_LIMIT_SOURCE_KEY_REDACTED };846 usageLimitSources[sourceId] = { ...source, managementKey: USAGE_LIMIT_SOURCE_KEY_REDACTED };
871 }847 }
872 for (const sourceId of Object.keys(current.usageLimitSources)) {848 for (const sourceId of Object.keys(current.usageLimitSources)) {
873 if (sourceId in next.usageLimitSources) continue;849 if (sourceId in next.usageLimitSources) continue;
874 yield* secretStore850 changes.push({
875 .remove(usageLimitSourceSecretName(sourceId))851 kind: "remove",
876 .pipe(852 secretName: usageLimitSourceSecretName(sourceId),
877 Effect.mapError(853 operation: "remove-stale-secret",
878 (cause) =>854 });
879 new ServerSettingsError({ settingsPath, operation: "remove-stale-secret", cause }),
880 ),
881 );
882 }855 }
883856
884 return {857 return {
885 ...next,858 settings: {
886 providerInstances: providerInstances as ServerSettings["providerInstances"],859 ...next,
887 usageLimitSources: usageLimitSources as ServerSettings["usageLimitSources"],860 providerInstances: providerInstances as ServerSettings["providerInstances"],
861 usageLimitSources: usageLimitSources as ServerSettings["usageLimitSources"],
862 },
863 changes,
888 };864 };
889 });865 });
890866
867 const rollbackProviderEnvironmentSecretWrites = (
868 writes: ReadonlyArray<{
869 readonly secretName: string;
870 readonly previousValue: Option.Option<Uint8Array>;
871 readonly providerInstanceId?: string;
872 readonly environmentVariable?: string;
873 }>,
874 ) =>
875 Effect.forEach(
876 writes.toReversed(),
877 (write) =>
878 (Option.isSome(write.previousValue)
879 ? secretStore.set(write.secretName, write.previousValue.value)
880 : secretStore.remove(write.secretName)
881 ).pipe(
882 Effect.catch((cause) =>
883 Effect.logWarning("failed to roll back provider environment secret", {
884 providerInstanceId: write.providerInstanceId,
885 environmentVariable: write.environmentVariable,
886 cause,
887 }),
888 ),
889 ),
890 { discard: true },
891 );
892
893 const applyProviderEnvironmentSecretChanges = (changes: ReadonlyArray<SecretChange>) => {
894 const applied: Array<{
895 readonly secretName: string;
896 readonly previousValue: Option.Option<Uint8Array>;
897 readonly providerInstanceId?: string;
898 readonly environmentVariable?: string;
899 }> = [];
900 const rollback = Effect.suspend(() => rollbackProviderEnvironmentSecretWrites(applied));
901 return Effect.forEach(
902 changes,
903 (change) =>
904 Effect.gen(function* () {
905 const previousValue = yield* secretStore.get(change.secretName).pipe(
906 Effect.mapError(
907 (cause) =>
908 new ServerSettingsError({
909 settingsPath,
910 operation: "read-secret",
911 providerInstanceId: change.providerInstanceId,
912 environmentVariable: change.environmentVariable,
913 cause,
914 }),
915 ),
916 );
917 // A store operation may mutate before reporting an error (for example chmod after rename).
918 applied.push({ ...change, previousValue });
919 yield* (
920 change.kind === "write"
921 ? secretStore.set(change.secretName, change.value)
922 : secretStore.remove(change.secretName)
923 ).pipe(
924 Effect.mapError(
925 (cause) =>
926 new ServerSettingsError({
927 settingsPath,
928 operation: change.kind === "write" ? "write-secret" : change.operation,
929 providerInstanceId: change.providerInstanceId,
930 environmentVariable: change.environmentVariable,
931 cause,
932 }),
933 ),
934 );
935 }),
936 { discard: true },
937 ).pipe(
938 Effect.tapError(() => rollback),
939 Effect.as(rollback),
940 );
941 };
942
943 const updateSettings = (
944 patch: ServerSettingsPatch,
945 ): Effect.Effect<ServerSettings, ServerSettingsError> =>
946 writeSemaphore.withPermits(1)(
947 Effect.gen(function* () {
948 const current = yield* getSettingsFromCache;
949 const updated = applyServerSettingsPatch(current, patch);
950 const persisted = yield* persistProviderEnvironmentSecrets(current, updated);
951 const next = yield* normalizeServerSettings(persisted.settings);
952 const materialized = yield* Effect.uninterruptibleMask(() =>
953 Effect.gen(function* () {
954 const rollbackSecretChanges = yield* applyProviderEnvironmentSecretChanges(
955 persisted.changes,
956 );
957 const materializedExit = yield* Effect.exit(
958 materializeProviderEnvironmentSecrets(next),
959 );
960 if (Exit.isFailure(materializedExit)) {
961 yield* rollbackSecretChanges;
962 return yield* Effect.failCause(materializedExit.cause);
963 }
964 const writeExit = yield* Effect.exit(writeSettingsAtomically(next));
965 if (Exit.isFailure(writeExit)) {
966 yield* rollbackSecretChanges;
967 return yield* Effect.failCause(writeExit.cause);
968 }
969 return materializedExit.value;
970 }),
971 );
972 yield* Cache.set(settingsCache, cacheKey, next);
973 yield* emitChange(next);
974 return resolveTextGenerationProvider(materialized);
975 }),
976 );
977
891 const revalidateAndEmit = writeSemaphore.withPermits(1)(978 const revalidateAndEmit = writeSemaphore.withPermits(1)(
892 Effect.gen(function* () {979 Effect.gen(function* () {
893 yield* Cache.invalidate(settingsCache, cacheKey);980 yield* Cache.invalidate(settingsCache, cacheKey);
...@@ -963,22 +1050,7 @@ const make = Effect.gen(function* () {...@@ -963,22 +1050,7 @@ const make = Effect.gen(function* () {
963 Effect.flatMap(materializeProviderEnvironmentSecrets),1050 Effect.flatMap(materializeProviderEnvironmentSecrets),
964 Effect.map(resolveTextGenerationProvider),1051 Effect.map(resolveTextGenerationProvider),
965 ),1052 ),
966 updateSettings: (patch) =>1053 updateSettings,
967 writeSemaphore.withPermits(1)(
968 Effect.gen(function* () {
969 const current = yield* getSettingsFromCache;
970 const nextPersisted = yield* persistProviderEnvironmentSecrets(
971 current,
972 applyServerSettingsPatch(current, patch),
973 );
974 const next = yield* normalizeServerSettings(nextPersisted);
975 yield* writeSettingsAtomically(next);
976 yield* Cache.set(settingsCache, cacheKey, next);
977 yield* emitChange(next);
978 const materialized = yield* materializeProviderEnvironmentSecrets(next);
979 return resolveTextGenerationProvider(materialized);
980 }),
981 ),
982 get streamChanges() {1054 get streamChanges() {
983 return materializeChanges(Stream.fromPubSub(changesPubSub));1055 return materializeChanges(Stream.fromPubSub(changesPubSub));
984 },1056 },