Skip to content

fix(ipns): prefer DB cache over stale network IPNS records#203

Merged
FSM1 merged 1 commit into
mainfrom
fix/e2e-staging-resilience
Feb 25, 2026
Merged

fix(ipns): prefer DB cache over stale network IPNS records#203
FSM1 merged 1 commit into
mainfrom
fix/e2e-staging-resilience

Conversation

@FSM1

@FSM1 FSM1 commented Feb 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • API fix: resolveRecord() now compares sequence numbers from both the delegated routing network and DB cache, preferring whichever is newer. The DB is always authoritative (written synchronously during publish) while delegated-ipfs.dev can serve stale cached records — this caused post-reload IPNS sync to return old metadata on staging.
  • E2E resilience: Increased timeouts for staging IPNS latency in the page-reload test (3.7) and added retry-with-re-navigation logic for the shared folder visibility test (7.2).
  • Unit tests: Added 2 new tests for the sequence number comparison behavior.

Test plan

  • All 48 IPNS service unit tests pass (including 2 new ones)
  • E2E full-workflow and sharing-workflow pass tests 1-16 and 1-29 against staging
  • Deploy to staging and re-run full E2E suite to confirm the API fix resolves the stale IPNS issue for tests 3.7 and 7.2

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved IPNS resolution to prefer the freshest data based on sequence number comparison.
    • Added retry mechanism for IPNS propagation delays in shared content scenarios.
  • Tests

    • Added test coverage for sequence number comparison in IPNS resolution.
    • Extended E2E test timeouts to accommodate IPNS resolution delays.
  • Documentation

    • Added learning notes on IPNS stale resolution behavior.

resolveRecord() now compares sequence numbers from both the network
(delegated-ipfs.dev) and DB cache, returning whichever is newer. The DB
is always up-to-date (written synchronously during publish) while the
network resolver can serve stale cached records — causing post-reload
sync to return old metadata on staging.

Also increases E2E test timeouts and adds retry logic for staging IPNS
latency in the page-reload and shared-folder-visibility tests.

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

coderabbitai Bot commented Feb 25, 2026

Copy link
Copy Markdown

Walkthrough

The PR implements sequence number comparison in IPNS resolution to prefer the fresher data source between network and database cache, adds corresponding unit tests, documents staging environment issues, and updates E2E tests with extended timeouts and retry mechanisms to handle IPNS propagation delays on staging.

Changes

Cohort / File(s) Summary
IPNS Resolution Core Logic
apps/api/src/ipns/ipns.service.ts
Added DB cache lookup and sequence number comparison in resolveRecord. When both network and cached results exist, the fresher source (higher sequence number as BigInt) is preferred; removed redundant later DB cache fetch.
Unit Tests
apps/api/src/ipns/ipns.service.spec.ts
Added two test cases: one verifying DB cache preference when sequence number is higher than network result, another verifying network preference when sequence number is equal or higher.
E2E Test Timeouts
tests/e2e/tests/full-workflow.spec.ts
Extended IPNS resolution timeouts from 90s to 240s and 60s to 120s in page reload test to accommodate slower staging resolution.
E2E Test Retry Logic
tests/e2e/tests/sharing-workflow.spec.ts
Introduced retry mechanism with up to three attempts, 120-second overall timeout, and 5-second wait between retries in shared folder file visibility test to handle IPNS propagation delays.
Documentation
.learnings/2026-02-25-ipns-stale-resolution-staging.md
Added markdown documenting IPNS stale resolution findings from staging, including observed behavior, proposed fixes, affected code locations, and improvement recommendations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 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 and concisely summarizes the main change: implementing logic to prefer the DB cache over stale network IPNS records through sequence number comparison.
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-staging-resilience

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.

@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.

🧹 Nitpick comments (2)
apps/api/src/ipns/ipns.service.ts (1)

387-398: Consider handling corrupted sequence numbers defensively.

BigInt(result.sequenceNumber) and BigInt(cached.sequenceNumber) will throw a SyntaxError if either value is an empty string or non-numeric. While both values are controlled (network defaults to '0', DB starts at '0'), a corrupted DB row could crash the resolve path.

A try/catch around the BigInt comparison that falls back to the network result would make this more resilient.

🛡️ Defensive BigInt comparison
     if (result && cached?.latestCid) {
       // Both sources available — prefer the one with the higher sequence number
-      const networkSeq = BigInt(result.sequenceNumber);
-      const dbSeq = BigInt(cached.sequenceNumber);
-      if (dbSeq > networkSeq) {
-        this.logger.log(
-          `DB cache has newer sequence (${dbSeq} > ${networkSeq}) for ${ipnsName}, using DB: ${cached.latestCid}`
-        );
-        return { cid: cached.latestCid, sequenceNumber: cached.sequenceNumber };
+      try {
+        const networkSeq = BigInt(result.sequenceNumber);
+        const dbSeq = BigInt(cached.sequenceNumber);
+        if (dbSeq > networkSeq) {
+          this.logger.log(
+            `DB cache has newer sequence (${dbSeq} > ${networkSeq}) for ${ipnsName}, using DB: ${cached.latestCid}`
+          );
+          return { cid: cached.latestCid, sequenceNumber: cached.sequenceNumber };
+        }
+      } catch {
+        this.logger.warn(
+          `Failed to compare sequence numbers for ${ipnsName}, using network result`
+        );
       }
       return result;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/ipns/ipns.service.ts` around lines 387 - 398, The BigInt
conversions of result.sequenceNumber and cached.sequenceNumber can throw on
corrupted/non-numeric strings; wrap the comparison logic (the block that creates
networkSeq and dbSeq and compares them) in a try/catch inside the resolve path
so that any error during BigInt(result.sequenceNumber) or
BigInt(cached.sequenceNumber) is caught and you fall back to returning result
(the network value). Keep the existing log when DB is newer, but in the catch
log a warning including ipnsName and the offending cached.sequenceNumber/value,
and then return result to avoid crashing the resolve flow.
apps/api/src/ipns/ipns.service.spec.ts (1)

900-949: Good coverage of the two primary comparison branches.

The two new tests correctly validate DB-preferred and network-preferred paths. One minor gap: there's no test where network sequence is strictly greater than DB (e.g., network=15, DB=10). While the implementation handles it via the same code path as the equal case, an explicit test would document the expected behavior and prevent regressions if the comparison operator changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/ipns/ipns.service.spec.ts` around lines 900 - 949, Add a third
unit test in ipns.service.spec.ts that exercises service.resolveRecord when the
network IPNS record has a strictly greater sequence than the DB record (e.g.,
network sequence 15 vs DB sequence '10') to ensure the network result is chosen;
reuse the same mocks (mockFetch, mockParseIpnsRecord,
mockFolderIpnsRepo.findOne) but set mockParseIpnsRecord to return sequence 15n
and mockFolderIpnsRepo.findOne to return sequenceNumber '10', then assert result
is not null and that result.cid and result.sequenceNumber match the network
values (verifying resolveRecord prefers the higher network sequence).
🤖 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/ipns/ipns.service.spec.ts`:
- Around line 900-949: Add a third unit test in ipns.service.spec.ts that
exercises service.resolveRecord when the network IPNS record has a strictly
greater sequence than the DB record (e.g., network sequence 15 vs DB sequence
'10') to ensure the network result is chosen; reuse the same mocks (mockFetch,
mockParseIpnsRecord, mockFolderIpnsRepo.findOne) but set mockParseIpnsRecord to
return sequence 15n and mockFolderIpnsRepo.findOne to return sequenceNumber
'10', then assert result is not null and that result.cid and
result.sequenceNumber match the network values (verifying resolveRecord prefers
the higher network sequence).

In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 387-398: The BigInt conversions of result.sequenceNumber and
cached.sequenceNumber can throw on corrupted/non-numeric strings; wrap the
comparison logic (the block that creates networkSeq and dbSeq and compares them)
in a try/catch inside the resolve path so that any error during
BigInt(result.sequenceNumber) or BigInt(cached.sequenceNumber) is caught and you
fall back to returning result (the network value). Keep the existing log when DB
is newer, but in the catch log a warning including ipnsName and the offending
cached.sequenceNumber/value, and then return result to avoid crashing the
resolve flow.

ℹ️ 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 a97734a and d741c1d.

📒 Files selected for processing (5)
  • .learnings/2026-02-25-ipns-stale-resolution-staging.md
  • apps/api/src/ipns/ipns.service.spec.ts
  • apps/api/src/ipns/ipns.service.ts
  • tests/e2e/tests/full-workflow.spec.ts
  • tests/e2e/tests/sharing-workflow.spec.ts

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.

1 participant