Merge pull request #3342 from code-yeongyu/fix/migration-sidecar-transaction-order

fix(migration): write config before sidecar updates

src/shared/migration/config-migration.test.ts

@@ -0,0 +1,121 @@
+/// <reference types="bun-types" />
+
+import { afterEach, describe, expect, test } from "bun:test"
+import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "fs"
+import { tmpdir } from "os"
+import { join } from "path"
+import { migrateConfigFile } from "./config-migration"
+import { getSidecarPath } from "./migrations-sidecar"
+
+const createdDirectories: string[] = []
+const MIGRATION_KEY = "model-version:anthropic/claude-opus-4-5->anthropic/claude-opus-4-6"
+
+function createWorkdir(): string {
+  const workdir = mkdtempSync(join(tmpdir(), "omo-config-migration-"))
+  createdDirectories.push(workdir)
+  return workdir
+}
+
+function createLegacyConfig(): Record<string, unknown> {
+  return {
+    agents: {
+      prometheus: { model: "anthropic/claude-opus-4-5" },
+    },
+  }
+}
+
+afterEach(() => {
+  for (const directory of createdDirectories.splice(0)) {
+    rmSync(directory, { recursive: true, force: true })
+  }
+})
+
+describe("migrateConfigFile sidecar write ordering", () => {
+  test("writes the migrated config before recording the sidecar when both writes succeed", () => {
+    // given
+    const workdir = createWorkdir()
+    const configPath = join(workdir, "oh-my-opencode.json")
+    const rawConfig = createLegacyConfig()
+
+    writeFileSync(configPath, JSON.stringify(rawConfig, null, 2) + "\n")
+
+    // when
+    const needsWrite = migrateConfigFile(configPath, rawConfig)
+
+    // then
+    expect(needsWrite).toBe(true)
+    expect(rawConfig._migrations).toBeUndefined()
+    expect((rawConfig.agents as Record<string, Record<string, unknown>>).prometheus.model).toBe(
+      "anthropic/claude-opus-4-6",
+    )
+
+    const persistedConfig = JSON.parse(readFileSync(configPath, "utf-8")) as Record<string, unknown>
+    expect(persistedConfig._migrations).toBeUndefined()
+    expect((persistedConfig.agents as Record<string, Record<string, unknown>>).prometheus.model).toBe(
+      "anthropic/claude-opus-4-6",
+    )
+
+    const sidecar = JSON.parse(readFileSync(getSidecarPath(configPath), "utf-8")) as {
+      appliedMigrations: string[]
+    }
+    expect(sidecar.appliedMigrations).toEqual([MIGRATION_KEY])
+  })
+
+  test("skips the sidecar when the config write fails so the migration retries on next startup", () => {
+    // given
+    const workdir = createWorkdir()
+    const configPath = join(workdir, "missing-parent", "oh-my-opencode.json")
+    const firstAttemptConfig = createLegacyConfig()
+
+    // when
+    const firstAttemptNeedsWrite = migrateConfigFile(configPath, firstAttemptConfig)
+
+    // then
+    expect(firstAttemptNeedsWrite).toBe(true)
+    expect(existsSync(getSidecarPath(configPath))).toBe(false)
+    expect(firstAttemptConfig._migrations).toEqual([MIGRATION_KEY])
+
+    // given
+    mkdirSync(join(workdir, "missing-parent"), { recursive: true })
+    writeFileSync(configPath, JSON.stringify(createLegacyConfig(), null, 2) + "\n")
+    const retriedConfig = createLegacyConfig()
+
+    // when
+    const retriedNeedsWrite = migrateConfigFile(configPath, retriedConfig)
+
+    // then
+    expect(retriedNeedsWrite).toBe(true)
+    expect(retriedConfig._migrations).toBeUndefined()
+    expect((retriedConfig.agents as Record<string, Record<string, unknown>>).prometheus.model).toBe(
+      "anthropic/claude-opus-4-6",
+    )
+    expect(existsSync(getSidecarPath(configPath))).toBe(true)
+  })
+
+  test("preserves _migrations in the config when the sidecar write fails after the config write succeeds", () => {
+    // given
+    const workdir = createWorkdir()
+    const configPath = join(workdir, "oh-my-opencode.json")
+    const rawConfig = createLegacyConfig()
+
+    writeFileSync(configPath, JSON.stringify(rawConfig, null, 2) + "\n")
+    mkdirSync(getSidecarPath(configPath))
+
+    // when
+    const needsWrite = migrateConfigFile(configPath, rawConfig)
+
+    // then
+    expect(needsWrite).toBe(true)
+    expect(rawConfig._migrations).toEqual([MIGRATION_KEY])
+    expect((rawConfig.agents as Record<string, Record<string, unknown>>).prometheus.model).toBe(
+      "anthropic/claude-opus-4-6",
+    )
+
+    const persistedConfig = JSON.parse(readFileSync(configPath, "utf-8")) as Record<string, unknown>
+    expect(persistedConfig._migrations).toEqual([MIGRATION_KEY])
+    expect((persistedConfig.agents as Record<string, Record<string, unknown>>).prometheus.model).toBe(
+      "anthropic/claude-opus-4-6",
+    )
+    expect(statSync(getSidecarPath(configPath)).isDirectory()).toBe(true)
+  })
+})

src/shared/migration/config-migration.ts

@@ -1,4 +1,4 @@
-import * as fs from "fs"
+import * as fs from "node:fs"
 import { log } from "../logger"
 import { writeFileAtomically } from "../write-file-atomically"
 import { AGENT_NAME_MAP, migrateAgentNames } from "./agent-names"
@@ -10,7 +10,7 @@ export function migrateConfigFile(
   configPath: string,
   rawConfig: Record<string, unknown>
 ): boolean {
-  const copy = structuredClone(rawConfig)
+  const copy = JSON.parse(JSON.stringify(rawConfig)) as Record<string, unknown>
   let needsWrite = false
 
   // Load previously applied migrations from BOTH the legacy in-config
@@ -74,14 +74,11 @@ export function migrateConfigFile(
   // the first place. The in-memory `rawConfig` never re-exposes
   // `_migrations` to downstream schema validation.
   const newMigrationsToRecord = allNewMigrations.filter(mKey => !existingMigrations.has(mKey))
-  let sidecarWriteSucceeded = false
   const fullMigrationSet = new Set<string>([
     ...existingMigrations,
     ...newMigrationsToRecord,
   ])
-  if (newMigrationsToRecord.length > 0 || hadLegacyInConfigMigrations) {
+  const shouldWriteSidecar = newMigrationsToRecord.length > 0 || hadLegacyInConfigMigrations
-    sidecarWriteSucceeded = writeAppliedMigrations(configPath, fullMigrationSet)
-  }
   if (newMigrationsToRecord.length > 0) {
     needsWrite = true
   }
@@ -89,10 +86,9 @@ export function migrateConfigFile(
     // Migrating state out of the config body is itself a config write.
     needsWrite = true
   }
-  if (sidecarWriteSucceeded && "_migrations" in copy) {
+  if (shouldWriteSidecar) {
-    delete copy._migrations
+    // Keep `_migrations` in the first config write so a later sidecar failure
-  } else if (!sidecarWriteSucceeded && newMigrationsToRecord.length > 0) {
+    // does not strand the config with migrated state missing from disk.
-    // Sidecar write failed — persist migration tracking in-config as fallback
     ;(copy as Record<string, unknown>)._migrations = Array.from(fullMigrationSet)
     needsWrite = true
   }
@@ -158,17 +154,32 @@ export function migrateConfigFile(
     }
 
     let writeSucceeded = false
+    let finalConfig = JSON.parse(JSON.stringify(copy)) as Record<string, unknown>
     try {
-      writeFileAtomically(configPath, JSON.stringify(copy, null, 2) + "\n")
+      writeFileAtomically(configPath, JSON.stringify(finalConfig, null, 2) + "\n")
       writeSucceeded = true
     } catch (err) {
       log(`Failed to write migrated config to ${configPath}:`, err)
     }
 
+    if (writeSucceeded && shouldWriteSidecar) {
+      const sidecarWriteSucceeded = writeAppliedMigrations(configPath, fullMigrationSet)
+      if (sidecarWriteSucceeded && "_migrations" in finalConfig) {
+        const configWithoutLegacyMigrations = JSON.parse(JSON.stringify(finalConfig)) as Record<string, unknown>
+        delete configWithoutLegacyMigrations._migrations
+        try {
+          writeFileAtomically(configPath, JSON.stringify(configWithoutLegacyMigrations, null, 2) + "\n")
+          finalConfig = configWithoutLegacyMigrations
+        } catch (err) {
+          log(`Failed to remove legacy _migrations fallback from ${configPath}:`, err)
+        }
+      }
+    }
+
     for (const key of Object.keys(rawConfig)) {
       delete rawConfig[key]
     }
-    Object.assign(rawConfig, copy)
+    Object.assign(rawConfig, finalConfig)
 
     if (writeSucceeded) {
       const backupMessage = backupSucceeded ? ` (backup: ${backupPath})` : ""