Skip to content

fix(api): start new IPNS entries at seq 1 to match client expectation#255

Merged
FSM1 merged 2 commits into
mainfrom
fix/e2e-conflict-persist
Mar 3, 2026
Merged

fix(api): start new IPNS entries at seq 1 to match client expectation#255
FSM1 merged 2 commits into
mainfrom
fix/e2e-conflict-persist

Conversation

@FSM1

@FSM1 FSM1 commented Mar 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • Phase 16 added conflict detection via expectedSequenceNumber on folder IPNS publishes
  • upsertFolderIpns created new DB entries with sequenceNumber='0' but the client computes newSeq = 0+1 = 1 and stores that
  • On the next publish to the same folder, the client sends expectedSeq='1', server has '0'409 Conflict
  • The retry mechanism also fails because resolveRecord prefers the network IPNS record (signed by the client with seq=1) over the DB value (seq=0), so resync returns the same wrong value
  • Fix: Initialize new IPNS entries at sequenceNumber='1' to match the IPNS record the client signed for first publish

Root Cause

The sequence number mismatch was invisible before Phase 16 because expectedSequenceNumber wasn't sent. Phase 16 exposed the off-by-one between DB storage and client state.

Note: Root folder entries created during vault initialization (seq='0', latestCid=null) are unaffected — their first actual publish goes through the UPDATE path which correctly increments.

Test plan

  • All 44 IPNS service unit tests pass (updated expectations for new entries)
  • All 121 IPNS + conflict-related tests pass
  • Vault service tests unaffected (36 pass)
  • CI E2E tests pass (sharing-workflow, invite-link-workflow, full-workflow)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Corrected initial sequence number tracking for newly published IPNS folders so initial publishes start at 1.
  • Tests

    • Updated test assertions to reflect the corrected sequence number initialization across folder publishing scenarios.
  • Chores

    • Added a data migration to backfill existing folder sequence numbers from 0 to 1 where appropriate.

Phase 16 added conflict detection via expectedSequenceNumber, but
upsertFolderIpns created new entries with sequenceNumber='0' while
the client computes newSeq = 0+1 = 1 and stores that. On the next
publish the client sends expectedSeq='1', server has '0' → 409.

The retry also fails because resolveRecord prefers the network IPNS
record (which the client signed with seq=1) over the DB value (seq=0),
so resync returns the same stale value and the retry gets 409 again.

Fix: initialize new IPNS entries at seq='1' to match the IPNS record
the client actually signed for first publish.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 6374ba359bf4
@coderabbitai

coderabbitai Bot commented Mar 3, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 17c7088 and 98b9e32.

📒 Files selected for processing (1)
  • apps/api/src/migrations/1740500000000-BackfillIpnsSequenceNumbers.ts

Walkthrough

Initial FolderIpns sequenceNumber initialization changed from '0' to '1'; tests updated accordingly; and a data migration added to backfill existing non-root FolderIpns records from sequence_number '0' to '1'. package.json was also modified.

Changes

Cohort / File(s) Summary
Service Implementation
apps/api/src/ipns/ipns.service.ts
Changed upsertFolderIpns to initialize new FolderIpns entries with sequenceNumber = '1' (was '0'); added explanatory comments about client-signed IPNS sequences.
Service Tests
apps/api/src/ipns/ipns.service.spec.ts
Updated mocked repository returns and assertions across multiple new-folder publish scenarios (phase 6 optional fields, keyEpoch handling, batch flows) to expect sequenceNumber = '1' instead of '0'.
Migration
apps/api/src/migrations/1740500000000-BackfillIpnsSequenceNumbers.ts
Added a new TypeORM migration that updates non-root folder_ipns rows: set sequence_number from '0' to '1' where latest_cid IS NOT NULL. Includes non-reversible down placeholder.
Manifest
package.json
Manifest modified (lines changed +10/-10) — likely to register or reference the new migration or related metadata.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 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 accurately describes the main change: initializing new IPNS entries with sequence number 1 instead of 0 to align with client expectations, which is the core fix for the conflict detection issue.
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 (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/e2e-conflict-persist

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

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

@codecov

codecov Bot commented Mar 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.68%. Comparing base (ef90514) to head (98b9e32).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
+ Coverage   46.31%   46.68%   +0.37%     
==========================================
  Files         106      106              
  Lines        8266     8255      -11     
  Branches      591      594       +3     
==========================================
+ Hits         3828     3854      +26     
+ Misses       4271     4233      -38     
- Partials      167      168       +1     
Flag Coverage Δ
api 84.55% <ø> (+0.68%) ⬆️
crypto 84.55% <ø> (+0.68%) ⬆️

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

Files with missing lines Coverage Δ
apps/api/src/ipns/ipns.service.ts 87.09% <ø> (+17.01%) ⬆️
🚀 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 227-233: Rows seeded previously with sequenceNumber = '0' and
latestCid != null will conflict with new publishes; add a one-time DB migration
or reconciliation to bump those rows to '1'. Implement a migration script (or DB
migration file) that runs the provided SQL to UPDATE folder_ipns SET
sequence_number = '1' WHERE sequence_number = '0' AND latest_cid IS NOT NULL AND
is_root = false, and/or add a reconciliation path in the service (where
folderIpnsRepository.create and any publish/update logic runs) that detects
existing records with sequenceNumber === '0' and latestCid !== null and
atomically updates sequenceNumber to '1' before proceeding. Ensure the migration
is idempotent and referenced in deployment/migration tooling so existing users
are unblocked.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f864e50 and 17c7088.

📒 Files selected for processing (2)
  • apps/api/src/ipns/ipns.service.spec.ts
  • apps/api/src/ipns/ipns.service.ts

Comment thread apps/api/src/ipns/ipns.service.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an off-by-one sequence number mismatch between the API’s DB cache and the web client’s first IPNS publish, preventing false 409 Conflict errors after conflict detection (expectedSequenceNumber) was introduced.

Changes:

  • Initialize newly created (non-root) FolderIpns DB entries with sequenceNumber: '1' instead of '0'.
  • Update IPNS service unit tests to expect the new initial sequence number for first-time publishes.

Reviewed changes

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

File Description
apps/api/src/ipns/ipns.service.ts Sets initial sequenceNumber for new IPNS tracking entries to '1' to align with the client’s first signed publish.
apps/api/src/ipns/ipns.service.spec.ts Updates unit test expectations for new-entry publishing to reflect the new initial sequence number.

Entries created before the seq='1' fix have seq='0' in the DB but
the client stored seq=1n. This migration bumps non-root entries that
have published content (latestCid IS NOT NULL) from '0' to '1'.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: b5da120786f5
@FSM1 FSM1 enabled auto-merge (squash) March 3, 2026 18:10
@FSM1 FSM1 merged commit 493111d into main Mar 3, 2026
20 checks passed
@FSM1 FSM1 deleted the fix/e2e-conflict-persist branch March 4, 2026 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants