fix: prevent duplicate folder_ipns row on vault init

[FSM1]

Summary

• Fix duplicate key value violates unique constraint "UQ_folder_ipns_user_ipns" error on POST /vault/init
• The new vault init flow (PR fix: separate vault key blob from root folder IPNS name #349) publishes IPNS records before calling /vault/init. The IPNS publish endpoint creates a folder_ipns row on first publish. Then /vault/init tried to blindly insert a second row for the same root IPNS name, hitting the unique constraint.
• Fix: check if the folder_ipns entry already exists (created by the publish step) and update isRoot=true instead of inserting a duplicate.

Root Cause

After PR #349, the vault initialization sequence is:

1. Publish vault key blob to vault key IPNS (creates folder_ipns row via publish endpoint)
2. Publish root folder metadata to root folder IPNS (creates another folder_ipns row)
3. Call POST /vault/init — tries to insert a folder_ipns row for root IPNS name → duplicate

Test plan

• Desktop E2E tests pass (linux, macos) — these were consistently failing
• Web E2E test 3.7 (page reload) passes
• Fresh vault creation via wallet login works

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Vault initialization now detects and handles existing root IPNS records to avoid duplicate entries and ensure resilient reinitialization.

• Tests

  • Added coverage for handling duplicate-root scenarios and for propagating non-duplicate errors during vault initialization.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0e07a446-abaf-415f-bba8-67a39ba81f20

📥 Commits

Reviewing files that changed from the base of the PR and between 6475c87 and 34b3527.

📒 Files selected for processing (1)

• apps/api/src/vault/vault.service.spec.ts

---

Walkthrough

VaultService.initializeVault now tries to insert a root FolderIpns row and, if the insert fails with a duplicate-key error (code === '23505'), updates the existing (userId, ipnsName) row to set isRoot: true; other errors are rethrown.

Changes

Cohort / File(s)	Summary
Vault service apps/api/src/vault/vault.service.ts	Replace unconditional FolderIpns creation with try/save that catches duplicate-key (23505) and calls update on the existing (userId, ipnsName) row to set isRoot: true; retains previous defaults for new rows.
Tests for initializeVault apps/api/src/vault/vault.service.spec.ts	Add tests covering duplicate-key on folder_ipns.save (asserts update called with { userId, ipnsName } and { isRoot: true }) and non-duplicate error propagation; add mockFolderIpnsRepo.update mock.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix(api): save IPNS record to DB before delegated routing publish #151 — Modifies FolderIpns persistence/duplicate-key handling and tests covering save/update error paths.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: prevent duplicate folder_ipns row on vault init' directly and concisely describes the main change—implementing an upsert pattern to handle duplicate IPNS records during vault initialization.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/vault-init-folder-ipns-duplicate

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

apps/api/src/vault/vault.service.ts (1)

80-101: Consider atomic upsert to eliminate TOCTOU race window.

The check-then-write pattern has a narrow race window: if a concurrent request (e.g., the publish endpoint) inserts a row between findOne returning null and save executing, the unique constraint violation will bubble up as an unhandled 500 error.

An atomic upsert using TypeORM's orUpdate would be more robust and follows the pattern already used by recordPin (line 183):

♻️ Suggested atomic upsert

-    const existingFolderIpns = await this.folderIpnsRepository.findOne({
-      where: { userId, ipnsName: dto.rootIpnsName },
-    });
-
-    if (existingFolderIpns) {
-      // Mark as root if not already (publish endpoint creates with isRoot=false)
-      if (!existingFolderIpns.isRoot) {
-        existingFolderIpns.isRoot = true;
-        await this.folderIpnsRepository.save(existingFolderIpns);
-      }
-    } else {
-      const rootFolderIpns = this.folderIpnsRepository.create({
-        userId,
-        ipnsName: dto.rootIpnsName,
-        latestCid: null,
-        sequenceNumber: '0',
-        encryptedIpnsPrivateKey: null,
-        keyEpoch: null,
-        isRoot: true,
-      });
-      await this.folderIpnsRepository.save(rootFolderIpns);
-    }
+    await this.folderIpnsRepository
+      .createQueryBuilder()
+      .insert()
+      .into(FolderIpns)
+      .values({
+        userId,
+        ipnsName: dto.rootIpnsName,
+        latestCid: null,
+        sequenceNumber: '0',
+        encryptedIpnsPrivateKey: null,
+        keyEpoch: null,
+        isRoot: true,
+      })
+      .orUpdate(['is_root'], ['user_id', 'ipns_name'])
+      .execute();

This atomically inserts a new row or updates is_root=true on conflict, eliminating the race window entirely.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/vault/vault.service.ts` around lines 80 - 101, The current
check-then-write using folderIpnsRepository.findOne followed by save has a
TOCTOU race; replace it with an atomic upsert like the pattern used in recordPin
so the insert-or-update happens in one DB statement: use the
repository.create(...) or repository.createQueryBuilder() and call orUpdate (or
the equivalent TypeORM upsert) to insert the row with isRoot=true or update the
existing row to set isRoot=true on conflict, removing the findOne/save flow
(remove existingFolderIpns and rootFolderIpns branching) and ensure the unique
key (ipnsName+userId) is used as the conflict target.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/api/src/vault/vault.service.ts`:
- Around line 80-101: The current check-then-write using
folderIpnsRepository.findOne followed by save has a TOCTOU race; replace it with
an atomic upsert like the pattern used in recordPin so the insert-or-update
happens in one DB statement: use the repository.create(...) or
repository.createQueryBuilder() and call orUpdate (or the equivalent TypeORM
upsert) to insert the row with isRoot=true or update the existing row to set
isRoot=true on conflict, removing the findOne/save flow (remove
existingFolderIpns and rootFolderIpns branching) and ensure the unique key
(ipnsName+userId) is used as the conflict target.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7930a12f-51ad-4db0-a54d-9c2243a31a4a

📥 Commits

Reviewing files that changed from the base of the PR and between 1a873de and 86a8d1a.

📒 Files selected for processing (1)

• apps/api/src/vault/vault.service.ts

[Copilot]

Pull request overview

Fixes a vault initialization failure in the API when the root folder IPNS row is already created earlier in the init flow (via /ipns/publish), preventing a unique constraint violation during POST /vault/init.

Changes:

• Updates VaultService.initializeVault() to check for an existing folder_ipns row for (userId, rootIpnsName).
• If the row exists, flips isRoot to true instead of inserting a duplicate; otherwise inserts a new root tracking row.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.23%. Comparing base (1a873de) to head (34b3527).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
+ Coverage   51.21%   51.23%   +0.02%     
==========================================
  Files         115      115              
  Lines        9256     9260       +4     
  Branches      747      748       +1     
==========================================
+ Hits         4740     4744       +4     
  Misses       4337     4337              
  Partials      179      179              

Flag	Coverage Δ
api	82.88% <100.00%> (+0.01%)	⬆️
api-client	82.88% <100.00%> (+0.01%)	⬆️
core	82.88% <100.00%> (+0.01%)	⬆️
crypto	82.88% <100.00%> (+0.01%)	⬆️
sdk	82.88% <100.00%> (+0.01%)	⬆️
sdk-core	82.88% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
apps/api/src/vault/vault.service.ts	90.16% <100.00%> (+0.69%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[FSM1]

@coderabbitai please review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the diff for the PR
gh pr diff 351 --repo FSM1/cipher-box

echo "---FILE CONTENT---"
# Also get the full file content
gh api repos/FSM1/cipher-box/contents/apps/api/src/vault/vault.service.ts \
  --jq '.content' | base64 -d

Length of output: 11463

---

@FSM1 The fix is clean and well-reasoned. Here's the full review:

---

apps/api/src/vault/vault.service.ts

Overall approach — ✅ Correct

The optimistic insert pattern is the right call here. A findOne + conditional save would have reintroduced the TOCTOU window (and cost an extra round-trip on every new vault). The try/catch on 23505 is idiomatic for this pattern.

Fallback update only patches isRoot: true — ✅ Correct

The pre-existing row (created by the publish step) already holds the actual latestCid and sequenceNumber written by the client. Patching only isRoot: true and leaving those fields intact is exactly right.

One minor observation — affected count not verified

In the catch block the update() result is not inspected:

await this.folderIpnsRepository.update(
  { userId, ipnsName: dto.rootIpnsName },
  { isRoot: true }
);

TypeORM's update() returns UpdateResult; if affected === 0 it silently no-ops. This is theoretically reachable only if the row is deleted between the failed insert and this update — practically impossible during vault init, and the 23505 error already proves the row existed. The risk is negligible, but logging a warning on affected === 0 would make a future debugging session easier:

const result = await this.folderIpnsRepository.update(
  { userId, ipnsName: dto.rootIpnsName },
  { isRoot: true }
);
if (result.affected === 0) {
  // Should never happen: row existed (caused 23505) but was gone by the time we updated
  this.logger.warn(`folder_ipns update affected 0 rows for userId=${userId} ipnsName=${dto.rootIpnsName}`);
}

Happy to leave it as-is if you consider it noise — it's not a correctness issue.

Hardcoded '23505' — acceptable

Project is PostgreSQL-only, so this is fine.

Pre-existing: no transaction around vaultRepository.save + folderIpnsRepository upsert

If the folderIpnsRepository.update in the catch block throws, the vault row is already persisted. This was true before the PR and is out of scope here.

---

apps/api/src/vault/vault.service.spec.ts — ✅ Solid

Both new test cases are well-targeted:

• Verifies update is called with the correct compound key and { isRoot: true } on a 23505 error.
• Verifies non-23505 errors propagate and update is never called.

The mockFolderIpnsRepo.update mock addition at the top is correctly scoped.

---

Summary: The fix correctly resolves the duplicate constraint violation introduced by the pre-publish-then-init sequence from PR #349. One non-blocking suggestion on logging the affected === 0 edge case, but the change is otherwise ready to merge.

---

🧠 Learnings used

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 296
File: packages/sdk/src/client.ts:286-360
Timestamp: 2026-03-20T23:50:52.646Z
Learning: In `packages/sdk/src/client.ts` (`CipherBoxClient`): Concurrent mutations to the same folder (e.g., `createFolder`, `renameItem`, `moveItem`, `deleteItem`, `deleteToBin`) are intentionally not serialized with a per-folder mutex in the SDK. Concurrent safety is provided at the UI layer — buttons/actions are disabled while an operation is in flight. A keyed async mutex/queue per folder IPNS name (and a separate one for bin state) is a known deferred improvement and should not be re-flagged as a regression in this or future PRs (first flagged in PR `#296`).

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 344
File: .planning/phases/20-vault-migration/20-02-PLAN.md:21-25
Timestamp: 2026-03-24T03:38:06.629Z
Learning: In FSM1/cipher-box `apps/api` and `.planning/`: As of Phase 20 (PR `#344`), the server stores ZERO crypto material by design. Both `encryptedRootFolderKey` and `encryptedRootIpnsPrivateKey` are NULLed on migration via `POST /vault/migrate`. The previously documented permanent DB fallback for `encryptedRootFolderKey` was intentionally removed in gap-closure plans 20-05 and 20-06 after verification confirmed zero non-migrated vaults exist in any environment (staging DB was nuked during Phase 19.2). `REQUIREMENTS.md` and all login/recovery flows have been updated to reflect the zero-crypto-material server state. Do NOT flag nulling of `encryptedRootFolderKey` in migration logic as a missing fallback.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 280
File: .planning/research/ARCHITECTURE.md:0-0
Timestamp: 2026-03-07T03:58:54.388Z
Learning: In `apps/api` and `.planning/research/ARCHITECTURE.md`: The `vaults.encryptedRootFolderKey` column is intentionally retained as a permanent disaster-recovery fallback even after the v1.1 migration moves the primary copy into the IPFS vault blob v2 format. The IPFS blob v2 copy is the primary source; the DB copy ensures login works if IPNS resolution fails. This is documented in `.planning/SCOPING_RATIONALE.md` Decision 3. The post-migration `vaults` table therefore holds `ownerPublicKey`, `rootIpnsName`, and `encryptedRootFolderKey (permanent fallback)` — the server's role shifts to coordination relay but it still holds this one ECIES-wrapped key for resilience.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 296
File: apps/web/src/hooks/useDropUpload.ts:63-74
Timestamp: 2026-03-20T14:24:39.034Z
Learning: In `apps/web/src/hooks/useDropUpload.ts`, the divergent parent-folder resolution between the collision-check path (uses `useFolderStore.getState().folders[folderId]` directly) and the upload path (uses `folderId === 'root' ? getRootFolderState(vault, folders) : folders[folderId]`) is pre-existing code moved during the Phase 19.1 SDK extraction refactor (PR `#296`). Consolidating both into a single resolution call is a known deferred improvement — do not flag this as a new regression introduced by the PR.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 344
File: apps/api/src/migrations/1740600000000-AddVaultMigratedAt.ts:32-35
Timestamp: 2026-03-24T03:38:02.176Z
Learning: In `apps/api/src/migrations/1740600000000-AddVaultMigratedAt.ts`: the `down()` method intentionally omits restoring NOT NULL constraints on `encrypted_root_folder_key` and `encrypted_root_ipns_private_key`. This migration is superseded by `apps/api/src/migrations/1740700000000-DropVaultCryptoColumns.ts`, which drops all three columns entirely. The `down()` here is unreachable in practice because `DropVaultCryptoColumns` must be reverted first, and its own `down()` re-adds the columns as nullable. Do not flag the incomplete rollback in `AddVaultMigratedAt.down()` as a defect.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 262
File: apps/web/src/services/bin.service.ts:320-341
Timestamp: 2026-03-04T13:09:02.427Z
Learning: In `apps/web/src/services/bin.service.ts` `restoreFromBin()` / `restoreFromBinInternal()`: The `resolveTargetFolder` helper only returns `FolderNode` objects that are already resident in the in-memory `useFolderStore` (fully decrypted, valid folderKey/ipnsPrivateKey). The root-folder fallback (`getRootFolder()`) uses real keys from the vault store — never zeroed/synthesized keys. Therefore no guard for `isLoaded === false` or zeroed crypto fields is needed on the restore publish path in v1. On-demand folder loading during restore (fetching encrypted metadata and resolving the full key chain) is an enhancement deferred to v2 due to engineering complexity.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 344
File: apps/web/src/hooks/useAuth.ts:110-129
Timestamp: 2026-03-24T03:56:16.584Z
Learning: In `apps/web/src/hooks/useAuth.ts` (`initializeOrLoadVault`): the locally derived `ipnsName` (from `deriveVaultIpnsKeypair(userKeypair.privateKey)` → `deriveIpnsName(ipnsKeypair.publicKey)`) and `existingVault.rootIpnsName` (stored at vault init time via the same derivation chain) are always identical by construction. Both are deterministic HKDF outputs from the same private key, so they cannot diverge. Do NOT flag the use of `existingVault.rootIpnsName` in `setVaultKeys()` as a divergence risk.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 281
File: apps/api/src/ipns/ipns.service.ts:0-0
Timestamp: 2026-03-07T06:13:57.127Z
Learning: In apps/api/src/ipns/ipns.service.ts, `publishBatch()` delegates to `publishRecord()` for each entry (via `Promise.allSettled`), so batch publishes correctly emit per-record `cipherbox_ipfs_ipns_duration_seconds{operation="publish"}` histogram observations. The previous inline duplication of `upsertFolderIpns + delegatedRouting.publish` inside `publishBatch()` was eliminated in PR `#281` commit 0a41da5. Do not add per-batch-level timing instrumentation on top of the per-record observations already provided by `publishRecord()`.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 262
File: apps/desktop/src-tauri/src/fuse/mod.rs:544-585
Timestamp: 2026-03-04T03:09:12.729Z
Learning: In `apps/desktop/src-tauri/src/fuse/mod.rs` `spawn_bin_entry_publish()`: The bin IPNS publish conflict handling (line ~574) is intentionally fire-and-forget. On a publish conflict, the new bin entry is dropped from the IPNS record but the deleted file/folder CID remains pinned (no data loss). A retry-with-merge strategy (similar to `spawn_metadata_publish`) is a known enhancement deferred to v2 due to engineering complexity for a background non-blocking path. The web app bin UI is expected to be able to reconstruct state from pinned CIDs if needed.

---

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 262
File: apps/desktop/src-tauri/src/fuse/write_ops.rs:949-967
Timestamp: 2026-03-04T13:08:51.542Z
Learning: In `apps/desktop/src-tauri/src/fuse/write_ops.rs` `build_folder_path()`: The 20-iteration safety cap on the parent-chain walk is intentional for v1. The breadcrumb path stored as `original_path` in `BinEntry` is informational (displayed in the Bin UI); restore correctness relies on `originalParentIpnsName`, not on this path. CipherBox vaults rarely exceed 5-6 nesting levels in practice, so silent truncation beyond 20 levels is an accepted trade-off to guard against infinite loops on corrupted inode trees.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.