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 <josh@martian.engineering>
2026-07-23 02:51:13 +00:00 · 2026-07-18 06:55:16 +08:00
parent 6caf637163
commit ccaaa42f52
2 changed files with 118 additions and 12 deletions
--- 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);
--- 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]}`,
        ],
      };
    }