fix(api): save IPNS record to DB before delegated routing publish#151
Conversation
When publishToDelegatedRouting() failed (e.g. rate-limited 429 or network error), upsertFolderIpns() never ran, so per-file IPNS names were never stored in the DB. This caused resolve to return 404 for files uploaded from the desktop FUSE mount under load, since both the DHT and DB fallback had no record. Fix: save to DB first, then attempt delegated routing as best-effort. If routing fails, log a warning but return success since the DB cache ensures resolve will find the record. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThe changes modify IPNS publishing to prioritize database persistence over delegated routing publication. Delegated routing failures transition from fatal exceptions to non-fatal logged warnings, enabling the service to save state even if external routing publication fails. Test cases are updated to validate successful database persistence occurs regardless of routing outcomes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/ipns/ipns.service.ts (1)
56-73:⚠️ Potential issue | 🟡 Minor
success: truesemantics silently changed — document the new contractBefore this PR:
success: true← DHT publication succeeded and DB was saved.
After this PR:success: true← DB was saved; DHT publication may have silently failed.Any caller that uses
success: trueas a signal that the record is immediately resolvable from delegated routing (e.g., waiting to share a link) will now receive a false positive when routing is down. The DB-fallback inresolveRecord()provides eventual consistency, but not the immediate DHT reachability thatsuccesspreviously implied.Consider adding a field to
PublishIpnsResponseDtoto distinguish the two outcomes:💡 Proposed DTO addition
export interface PublishIpnsResponseDto { success: boolean; ipnsName: string; sequenceNumber: string; + /** true when the record was also published to the delegated routing DHT */ + routingPublished?: boolean; }Then at the call site:
+ let routingPublished = false; try { await this.publishToDelegatedRouting(dto.ipnsName, recordBytes); + routingPublished = true; } catch (error) { this.logger.warn(...); } - return { success: true, ipnsName: dto.ipnsName, sequenceNumber: folder.sequenceNumber }; + return { success: true, ipnsName: dto.ipnsName, sequenceNumber: folder.sequenceNumber, routingPublished };If enriching the response is too noisy for now, at minimum update the JSDoc on
publishRecordandPublishIpnsResponseDtoto state thatsuccessreflects DB persistence only.🤖 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 56 - 73, The PR changed success semantics so PublishIpnsResponseDto now may return success: true even if delegated routing publish failed; update the contract by either adding a boolean field (e.g., delegatedRoutingPublished or routedToDelegatedRouting) to PublishIpnsResponseDto and set it from the result of publishToDelegatedRouting (called in ipns.service.ts after upsertFolderIpns), or at minimum update the JSDoc/comments on publishRecord (or the method that returns PublishIpnsResponseDto) and the PublishIpnsResponseDto declaration to explicitly state that success indicates DB persistence only and does not guarantee immediate delegated routing/DHT publication.
🧹 Nitpick comments (2)
apps/api/src/ipns/ipns.service.ts (1)
111-128: Batch fix is consistent with single-record fix — but lacks test coverageThe same DB-first / best-effort-routing pattern is correctly applied inside the
Promise.allSettledmap.upsertFolderIpnsfailures still surface as rejected promises and are handled by the settled loop (resulting insuccess: falseper entry). Routing failures are swallowed and only warn-logged, returningsuccess: trueper entry.However, the new
"delegated routing failures are non-fatal"describe block in the spec file covers onlypublishRecord. There are no corresponding tests forpublishBatchwith non-fatal routing failures (500, network error, string rejection). Given that the fix applies to both paths andpublishBatchis independently callable, this is a test-coverage gap for code changed in this PR.🤖 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 111 - 128, Add unit tests for publishBatch mirroring the existing "delegated routing failures are non-fatal" spec for publishRecord: exercise publishBatch with entries where upsertFolderIpns succeeds but publishToDelegatedRouting rejects with (a) an HTTP 500-like error, (b) a network/error object, and (c) a string rejection, and assert that each batch entry returns success: true while the logger.warn is invoked (i.e., routing failures are non-fatal); target the publishBatch method and stub/mocks for upsertFolderIpns and publishToDelegatedRouting to simulate the failure modes used in publishRecord tests.apps/api/src/ipns/ipns.service.spec.ts (1)
901-960: New suite validates non-fatal behavior forpublishRecord— add parallel coverage forpublishBatchThe three cases (string rejection, 400, 503) are well-chosen and cover the distinct code paths in
publishToDelegatedRouting(network-error backoff path, non-retryable HTTP path). However,publishBatchhas the identical try/catch wrapping added in this PR (lines 121–128 of the service) and is not exercised in any routing-failure scenario.Suggested additions to the describe block (or a sibling
"publishBatch — delegated routing failures are non-fatal"block):it('should return success for all entries even when delegated routing fails (500)', async () => { mockFetch.mockResolvedValue({ ok: false, status: 500, text: () => Promise.resolve('Internal Server Error'), }); mockFolderIpnsRepo.create.mockReturnValue({ ...mockFolderEntity, sequenceNumber: '0' }); const result = await service.publishBatch(testUserId, { records: [ { ipnsName: testIpnsName, record: testRecord, metadataCid: testMetadataCid }, ], }); expect(result.totalSucceeded).toBe(1); expect(result.totalFailed).toBe(0); expect(result.results[0].success).toBe(true); expect(mockFolderIpnsRepo.save).toHaveBeenCalled(); });🤖 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 901 - 960, Add tests mirroring the delegated-routing failure cases already covering publishRecord to also exercise publishBatch: create a new describe/it block that stubs mockFetch to reject with a string or return non-OK responses (e.g., status 400, 500/503), mockFolderIpnsRepo.create to return an entity (e.g., sequenceNumber: '0'), call service.publishBatch(...) with one or more records, and assert the batch response reports successes (totalSucceeded === number of records, totalFailed === 0, each results[i].success === true) and that mockFolderIpnsRepo.save was called; this exercises the same try/catch in publishBatch and validates delegated routing failures are non-fatal (referencing publishBatch, publishRecord, publishToDelegatedRouting, mockFetch, mockFolderIpnsRepo.create, and mockFolderIpnsRepo.save).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 56-73: The PR changed success semantics so PublishIpnsResponseDto
now may return success: true even if delegated routing publish failed; update
the contract by either adding a boolean field (e.g., delegatedRoutingPublished
or routedToDelegatedRouting) to PublishIpnsResponseDto and set it from the
result of publishToDelegatedRouting (called in ipns.service.ts after
upsertFolderIpns), or at minimum update the JSDoc/comments on publishRecord (or
the method that returns PublishIpnsResponseDto) and the PublishIpnsResponseDto
declaration to explicitly state that success indicates DB persistence only and
does not guarantee immediate delegated routing/DHT publication.
---
Nitpick comments:
In `@apps/api/src/ipns/ipns.service.spec.ts`:
- Around line 901-960: Add tests mirroring the delegated-routing failure cases
already covering publishRecord to also exercise publishBatch: create a new
describe/it block that stubs mockFetch to reject with a string or return non-OK
responses (e.g., status 400, 500/503), mockFolderIpnsRepo.create to return an
entity (e.g., sequenceNumber: '0'), call service.publishBatch(...) with one or
more records, and assert the batch response reports successes (totalSucceeded
=== number of records, totalFailed === 0, each results[i].success === true) and
that mockFolderIpnsRepo.save was called; this exercises the same try/catch in
publishBatch and validates delegated routing failures are non-fatal (referencing
publishBatch, publishRecord, publishToDelegatedRouting, mockFetch,
mockFolderIpnsRepo.create, and mockFolderIpnsRepo.save).
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 111-128: Add unit tests for publishBatch mirroring the existing
"delegated routing failures are non-fatal" spec for publishRecord: exercise
publishBatch with entries where upsertFolderIpns succeeds but
publishToDelegatedRouting rejects with (a) an HTTP 500-like error, (b) a
network/error object, and (c) a string rejection, and assert that each batch
entry returns success: true while the logger.warn is invoked (i.e., routing
failures are non-fatal); target the publishBatch method and stub/mocks for
upsertFolderIpns and publishToDelegatedRouting to simulate the failure modes
used in publishRecord tests.
1 participant
Summary
publishRecord()calledpublishToDelegatedRouting()beforeupsertFolderIpns(). If delegated routing failed (429 rate limit, network error, DHT issues), the DB save never ran — leaving per-file IPNS names unresolvable.publishRecord()andpublishBatch()are fixed.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests