Mark/kimi_agent_kin_app_sync

Fork 0

[Review] migrateSensitiveFieldsToVault is not actually idempotent — duplicate-write on crash, data-loss on partial run #175

New issue

Closed

opened 2026-09-14 14:07:04 +00:00 by Mark · 2 comments

Mark commented

2026-09-14 14:07:04 +00:00

Owner

Finding

VaultStore.migrateSensitiveFieldsToVault (src/vault/VaultStore.ts:503-575) ships with a test at src/vault/tests/vaultMigration.test.ts:95-129 claiming "is idempotent across repeated runs." The test passes only because the first run strips the source data; the migration itself has no dedup logic.

Repro

// VaultStore.ts:518-572 (simplified)
for (const i of interactions) {
  if (i.id == null) continue;
  const updates: Partial<Interaction> = {};
  if (i.mood) {
    await db.vaultMood.add({                // (1) write to vault
      interactionId: i.id, personId: i.personId, mood: i.mood, date: i.date, ...
    });
    updates.mood = undefined;               // (2) mark for strip
  }
  // ...
  if (Object.keys(updates).length > 0) {
    await mainDb.updateInteraction(i.id, updates);   // (4) strip from main
  }
}

The window between (1) and (4) is unbounded. If the process dies between those awaits (refresh, OOM, OS kill), the second run sees the main row still has mood, copies it again, and writes a second vaultMood row with the same interactionId. There is no vaultMood.where({interactionId}).first() check before add().

Failure modes

Duplicate vault entries per interactionId on any crash mid-loop or on cold-boot re-run before the loop completes.
No resume token: every cold start re-runs the full migration. With many interactions the loop is for await over Dexie writes on the main thread, with no progress UI and no "are we done?" flag.
Direction of idempotency is data-loss-shaped: idempotency is achieved by deleting the source on first run, not by checking the destination. If the vault write fails (Dexie quota, IndexedDB transaction abort) but the main strip succeeds, the user has lost the field with no vault copy.

Suggested fix shape

Add vaultMood.where({interactionId}).first() check before add() (and same for intimacy/conflicts/quality).
Add a per-person migrationMarker row in vaultIntimateSettings or a dedicated vaultMigrationState table; only migrate rows for persons where the marker is absent.
Wrap the (1)→(4) pair in a Dexie transaction so a crash rolls back the vault write too.
Add a unit test that simulates a crash between vault-write and main-strip (e.g. throw after add, assert second run produces no duplicate).

Severity

High. This is the privacy-boundary migration. Data duplication or loss here directly affects the vault's correctness guarantee.

Discovered by

Manual code review pass over the last ~10 merged commits (intimate-state feature #168/#169 + recent hardening), 2026-09-14.

## Finding `VaultStore.migrateSensitiveFieldsToVault` (src/vault/VaultStore.ts:503-575) ships with a test at src/vault/__tests__/vaultMigration.test.ts:95-129 claiming "is idempotent across repeated runs." The test passes only because the first run strips the source data; the migration itself has no dedup logic. ## Repro ```ts // VaultStore.ts:518-572 (simplified) for (const i of interactions) { if (i.id == null) continue; const updates: Partial<Interaction> = {}; if (i.mood) { await db.vaultMood.add({ // (1) write to vault interactionId: i.id, personId: i.personId, mood: i.mood, date: i.date, ... }); updates.mood = undefined; // (2) mark for strip } // ... if (Object.keys(updates).length > 0) { await mainDb.updateInteraction(i.id, updates); // (4) strip from main } } ``` The window between (1) and (4) is unbounded. If the process dies between those awaits (refresh, OOM, OS kill), the second run sees the main row still has `mood`, copies it again, and writes a second `vaultMood` row with the same `interactionId`. There is no `vaultMood.where({interactionId}).first()` check before `add()`. ## Failure modes 1. Duplicate vault entries per `interactionId` on any crash mid-loop or on cold-boot re-run before the loop completes. 2. No resume token: every cold start re-runs the full migration. With many interactions the loop is `for await` over Dexie writes on the main thread, with no progress UI and no "are we done?" flag. 3. Direction of idempotency is data-loss-shaped: idempotency is achieved by deleting the source on first run, not by checking the destination. If the vault write fails (Dexie quota, IndexedDB transaction abort) but the main strip succeeds, the user has lost the field with no vault copy. ## Suggested fix shape - Add `vaultMood.where({interactionId}).first()` check before `add()` (and same for intimacy/conflicts/quality). - Add a per-person `migrationMarker` row in `vaultIntimateSettings` or a dedicated `vaultMigrationState` table; only migrate rows for persons where the marker is absent. - Wrap the (1)→(4) pair in a Dexie transaction so a crash rolls back the vault write too. - Add a unit test that simulates a crash between vault-write and main-strip (e.g. throw after `add`, assert second run produces no duplicate). ## Severity High. This is the privacy-boundary migration. Data duplication or loss here directly affects the vault's correctness guarantee. ## Discovered by Manual code review pass over the last ~10 merged commits (intimate-state feature #168/#169 + recent hardening), 2026-09-14.

Mark added the

labels

2026-09-14 14:07:04 +00:00

Mark referenced this issue from a commit

2026-09-14 14:24:29 +00:00

fix(#175): make migrateSensitiveFieldsToVault crash-safe

Mark commented

2026-09-14 14:24:45 +00:00

Author

Owner

Fixed in commit a32d000 (fix(#175): make migrateSensitiveFieldsToVault crash-safe), now on origin/main.

The kimi_agent_kin_app_sync runner originally landed the fix locally but its git push was rejected because origin/main had advanced between commit and push (other kilo-gsd runs had landed in the same window). Watcher rebased onto the new origin/main, re-pushed both #175 and the unrelated #179 commit.

Three hardening changes per the commit message:

Per-interaction dedup — before each vaultMood/vaultIntimacy/vaultConflicts/vaultQuality write, look up the existing row by interactionId. Skip the add if it already exists. Protects against duplicate rows after a crash between vault-write and main-strip.
Per-person vaultMigrationState marker — once all four sensitive tables for a personId are processed, the marker records it. Persons with a marker are skipped entirely on subsequent runs. Crash mid-person leaves the marker absent, so the next run reprocesses (and the dedup checks above prevent duplicates).
Vault-only rw transaction — each person's four sensitive tables plus the marker commit atomically. Main-DB strip runs after; idempotent (Dexie.update() on the same value is a no-op).

Two regression tests in src/vault/__tests__/vaultMigration.test.ts:

crash simulation (vault rows pre-populated, main row still has fields; second run → 0 new writes, no duplicates, main row still stripped)
marker verification (successful run writes the per-person marker with correct counts)

Verified locally: npx vitest run src/vault/__tests__/vaultMigration.test.ts → 5/5 PASS (934ms).

**Fixed in commit a32d000** (`fix(#175): make migrateSensitiveFieldsToVault crash-safe`), now on `origin/main`. The kimi_agent_kin_app_sync runner originally landed the fix locally but its `git push` was rejected because `origin/main` had advanced between commit and push (other kilo-gsd runs had landed in the same window). Watcher rebased onto the new `origin/main`, re-pushed both #175 and the unrelated #179 commit. Three hardening changes per the commit message: 1. **Per-interaction dedup** — before each `vaultMood/vaultIntimacy/vaultConflicts/vaultQuality` write, look up the existing row by `interactionId`. Skip the add if it already exists. Protects against duplicate rows after a crash between vault-write and main-strip. 2. **Per-person `vaultMigrationState` marker** — once all four sensitive tables for a `personId` are processed, the marker records it. Persons with a marker are skipped entirely on subsequent runs. Crash mid-person leaves the marker absent, so the next run reprocesses (and the dedup checks above prevent duplicates). 3. **Vault-only rw transaction** — each person's four sensitive tables plus the marker commit atomically. Main-DB strip runs after; idempotent (`Dexie.update()` on the same value is a no-op). Two regression tests in `src/vault/__tests__/vaultMigration.test.ts`: - crash simulation (vault rows pre-populated, main row still has fields; second run → 0 new writes, no duplicates, main row still stripped) - marker verification (successful run writes the per-person marker with correct counts) Verified locally: `npx vitest run src/vault/__tests__/vaultMigration.test.ts` → 5/5 PASS (934ms).

Mark closed this issue

2026-09-14 14:24:45 +00:00

Mark commented

2026-09-14 14:36:58 +00:00

Author

Owner

Closing as already-fixed.

Commit a32d000 (fix(#175): make migrateSensitiveFieldsToVault crash-safe) landed on main at 2026-09-14 (today, before this triage session reviewed the file).

The chat-agent code review read against a stale HEAD and reported this issue as open. The fix was already in place when the review started — the agent failed to re-check HEAD between reads.

No work to do.

Closing as already-fixed. Commit `a32d000` (`fix(#175): make migrateSensitiveFieldsToVault crash-safe`) landed on `main` at 2026-09-14 (today, before this triage session reviewed the file). The chat-agent code review read against a stale HEAD and reported this issue as open. The fix was already in place when the review started — the agent failed to re-check HEAD between reads. No work to do.

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Mark/kimi_agent_kin_app_sync#175

No description provided.

Rows
Columns