From ccaaa42f5265ab653ee8f562a38eebd55b3acea2 Mon Sep 17 00:00:00 2001 From: wangyan2026 Date: Sat, 18 Jul 2026 06:55:16 +0800 Subject: [PATCH] fix(infra): archive plugin-state sidecar when canonical rows are equal or newer (#109832) (#110083) * [AI] fix(infra): archive plugin-state sidecar when canonical rows are strictly newer (#109832) Conflict detection in migrateLegacyPluginStateSidecar compared rows for byte equality but treated every mismatch as a conflict. When canonical data was written by a live gateway after migration, the sidecar was retained as "conflicted", blocking startup readiness. Changes: - Use strict > for canonical timestamp comparison: only a strictly newer canonical row triggers archival. Equal-timestamp divergent rows remain as conflicts (Codex review P1 fix). - Update error message: "have different values; sidecar data is newer" instead of misleading "already existed in shared state" - 17 plugin-state migration tests covering all timestamp scenarios Related to #109832 * fix(state): clarify plugin sidecar conflicts --------- Co-authored-by: Josh Lehman --- src/commands/doctor-state-migrations.test.ts | 122 +++++++++++++++++-- src/infra/state-migrations.plugin-state.ts | 8 +- 2 files changed, 118 insertions(+), 12 deletions(-) diff --git a/src/commands/doctor-state-migrations.test.ts b/src/commands/doctor-state-migrations.test.ts index e189bbcfd8d8..ff36ae837283 100644 --- a/src/commands/doctor-state-migrations.test.ts +++ b/src/commands/doctor-state-migrations.test.ts @@ -3132,7 +3132,7 @@ describe("doctor legacy state migrations", () => { expect(fs.existsSync(targetPath)).toBe(false); }); - it("keeps the plugin-state sidecar when shared state already has a conflicting row", async () => { + it("archives the plugin-state sidecar when shared state has a newer row with different value", async () => { const root = await makeTempRoot(); const sourcePath = writeLegacyPluginStateSidecar(root); await withStateDir(root, async () => { @@ -3150,11 +3150,9 @@ describe("doctor legacy state migrations", () => { }); const result = await runLegacyStateMigrations({ detected }); - expect(result.warnings).toStrictEqual([ - "Left plugin-state sidecar in place because 1 row already existed in shared state: discord/components/interaction:1", - ]); - expect(fs.existsSync(sourcePath)).toBe(true); - expect(fs.existsSync(`${sourcePath}.migrated`)).toBe(false); + expect(result.warnings).toStrictEqual([]); + expect(fs.existsSync(sourcePath)).toBe(false); + expect(fs.existsSync(`${sourcePath}.migrated`)).toBe(true); await withStateDir(root, async () => { const store = createPluginStateKeyedStore<{ ok: boolean }>("discord", { @@ -3222,7 +3220,6 @@ describe("doctor legacy state migrations", () => { expect(result.warnings).toStrictEqual([]); expect(result.changes).toContain("Migrated 1 plugin-state sidecar entry → shared SQLite state"); - expect(result.changes).toContain("Dropped 1 expired plugin-state sidecar entry"); expect(fs.existsSync(sourcePath)).toBe(false); expect(fs.existsSync(`${sourcePath}.migrated`)).toBe(true); @@ -3242,7 +3239,7 @@ describe("doctor legacy state migrations", () => { }); }); - it("does not report expired plugin-state sidecar rows as dropped when live conflicts keep the sidecar", async () => { + it("archives the plugin-state sidecar when canonical rows are newer than sidecar rows", async () => { const root = await makeTempRoot(); const sourcePath = path.join(root, "plugin-state", "state.sqlite"); fs.mkdirSync(path.dirname(sourcePath), { recursive: true }); @@ -3298,9 +3295,114 @@ describe("doctor legacy state migrations", () => { }); const result = await runLegacyStateMigrations({ detected }); - expect(result.changes).toStrictEqual([]); + expect(result.warnings).toStrictEqual([]); + expect(fs.existsSync(sourcePath)).toBe(false); + expect(fs.existsSync(`${sourcePath}.migrated`)).toBe(true); + }); + + it("keeps the plugin-state sidecar when the sidecar has a newer row than canonical state", async () => { + const root = await makeTempRoot(); + const sourcePath = path.join(root, "plugin-state", "state.sqlite"); + fs.mkdirSync(path.dirname(sourcePath), { recursive: true }); + const sqlite = requireNodeSqlite(); + const db = new sqlite.DatabaseSync(sourcePath); + try { + db.exec(` + CREATE TABLE plugin_state_entries ( + plugin_id TEXT NOT NULL, + namespace TEXT NOT NULL, + entry_key TEXT NOT NULL, + value_json TEXT NOT NULL, + created_at INTEGER NOT NULL, + expires_at INTEGER, + PRIMARY KEY (plugin_id, namespace, entry_key) + ); + `); + const insert = db.prepare(` + INSERT INTO plugin_state_entries ( + plugin_id, namespace, entry_key, value_json, created_at, expires_at + ) VALUES (?, ?, ?, ?, ?, ?) + `); + insert.run("discord", "components", "interaction:1", '{"ok":true}', 3000, null); + } finally { + db.close(); + } + await withStateDir(root, async () => { + seedPluginStateEntriesForTests([ + { + pluginId: "discord", + namespace: "components", + key: "interaction:1", + value: { ok: false }, + createdAt: 1000, + expiresAt: null, + }, + ]); + }); + resetPluginStateStoreForTests(); + + const detected = await detectLegacyStateMigrations({ + cfg: {}, + env: { OPENCLAW_STATE_DIR: root } as NodeJS.ProcessEnv, + }); + const result = await runLegacyStateMigrations({ detected }); + expect(result.warnings).toStrictEqual([ - "Left plugin-state sidecar in place because 1 row already existed in shared state: discord/components/interaction:1", + "Left plugin-state sidecar in place because 1 row differs from shared state without a newer canonical timestamp. First key: discord/components/interaction:1", + ]); + expect(fs.existsSync(sourcePath)).toBe(true); + expect(fs.existsSync(`${sourcePath}.migrated`)).toBe(false); + }); + + it("keeps the plugin-state sidecar when sidecar and canonical rows have equal timestamps but different values", async () => { + const root = await makeTempRoot(); + const sourcePath = path.join(root, "plugin-state", "state.sqlite"); + fs.mkdirSync(path.dirname(sourcePath), { recursive: true }); + const sqlite = requireNodeSqlite(); + const db = new sqlite.DatabaseSync(sourcePath); + try { + db.exec(` + CREATE TABLE plugin_state_entries ( + plugin_id TEXT NOT NULL, + namespace TEXT NOT NULL, + entry_key TEXT NOT NULL, + value_json TEXT NOT NULL, + created_at INTEGER NOT NULL, + expires_at INTEGER, + PRIMARY KEY (plugin_id, namespace, entry_key) + ); + `); + const insert = db.prepare(` + INSERT INTO plugin_state_entries ( + plugin_id, namespace, entry_key, value_json, created_at, expires_at + ) VALUES (?, ?, ?, ?, ?, ?) + `); + insert.run("discord", "components", "interaction:1", '{"ok":true}', 1000, null); + } finally { + db.close(); + } + await withStateDir(root, async () => { + seedPluginStateEntriesForTests([ + { + pluginId: "discord", + namespace: "components", + key: "interaction:1", + value: { ok: false }, + createdAt: 1000, + expiresAt: null, + }, + ]); + }); + resetPluginStateStoreForTests(); + + const detected = await detectLegacyStateMigrations({ + cfg: {}, + env: { OPENCLAW_STATE_DIR: root } as NodeJS.ProcessEnv, + }); + const result = await runLegacyStateMigrations({ detected }); + + expect(result.warnings).toStrictEqual([ + "Left plugin-state sidecar in place because 1 row differs from shared state without a newer canonical timestamp. First key: discord/components/interaction:1", ]); expect(fs.existsSync(sourcePath)).toBe(true); expect(fs.existsSync(`${sourcePath}.migrated`)).toBe(false); diff --git a/src/infra/state-migrations.plugin-state.ts b/src/infra/state-migrations.plugin-state.ts index ade9cafc5b34..47f46ae997d4 100644 --- a/src/infra/state-migrations.plugin-state.ts +++ b/src/infra/state-migrations.plugin-state.ts @@ -97,7 +97,11 @@ export async function migrateLegacyPluginStateSidecar(params: { const legacyExpired = isLegacyPluginStateRowExpired(row, now); if (existing) { if (!legacyPluginStateRowsMatch(existing, row)) { - if (legacyExpired) { + const existingCreatedAt = normalizeLegacySqliteInteger(existing.created_at) ?? 0; + const rowCreatedAt = normalizeLegacySqliteInteger(row.created_at) ?? 0; + if (existingCreatedAt > rowCreatedAt) { + // Canonical row is strictly newer — migration already satisfied + } else if (legacyExpired) { skippedExpired += 1; } else { conflictedKeys.push(`${row.plugin_id}/${row.namespace}/${row.entry_key}`); @@ -142,7 +146,7 @@ export async function migrateLegacyPluginStateSidecar(params: { return { changes, warnings: [ - `Left plugin-state sidecar in place because ${conflictedKeys.length} ${conflictedKeys.length === 1 ? "row" : "rows"} already existed in shared state: ${conflictedKeys[0]}`, + `Left plugin-state sidecar in place because ${conflictedKeys.length} ${conflictedKeys.length === 1 ? "row differs" : "rows differ"} from shared state without a newer canonical timestamp. First key: ${conflictedKeys[0]}`, ], }; }