fix(api): make delegated routing publish fire-and-forget#308
Conversation
DHT propagation via someguy takes 10-30s per IPNS record. Since the DB record is saved before the DHT publish and resolveRecord() always prefers DB data, there is no need to block the API response. This was causing batch operations (e.g. multi-folder delete) to take minutes. Metrics are still collected via the detached promise chain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2c05abce85b4
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughpublishRecord() now invokes delegated routing publish as fire-and-forget (no await); delegated publish errors are logged and recorded asynchronously. Tests were updated to flush the Node.js microtask queue so detached callbacks run before assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IpnsService
participant DelegatedRouting
participant Metrics
Client->>IpnsService: publishRecord(request)
IpnsService->>IpnsService: perform primary publish (blocking)
IpnsService-->>Client: respond (primary result)
IpnsService->>DelegatedRouting: publish(...) (fire-and-forget)
DelegatedRouting-->>IpnsService: resolve / reject (async)
IpnsService->>Metrics: record delegated publish outcome (inside detached chain)
Note right of Metrics: Errors in metrics observation are logged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
==========================================
- Coverage 49.83% 49.82% -0.02%
==========================================
Files 114 114
Lines 9126 9126
Branches 699 701 +2
==========================================
- Hits 4548 4547 -1
Misses 4402 4402
- Partials 176 177 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/ipns/ipns.service.ts (1)
81-95: Consider adding a terminal.catch()for defensive error handling.The fire-and-forget pattern correctly handles delegated routing failures via
.catch(), but any exception thrown inside the.then()callback (e.g., ifmetricsService.ipnsPublishDuration.observe()fails) would surface as an unhandled promise rejection.While unlikely in practice, adding a terminal
.catch()ensures no silent failures in the metrics path:🛡️ Proposed hardening
this.delegatedRouting .publish(dto.ipnsName, recordBytes) .catch((error) => { this.logger.warn( `Delegated routing publish failed for ${dto.ipnsName}, DB record saved: ${error instanceof Error ? error.message : String(error)}` ); return 'error' as const; }) .then((outcome) => { const publishElapsed = Number(process.hrtime.bigint() - publishStart) / 1e9; this.metricsService.ipnsPublishDuration.observe( { outcome: outcome === 'error' ? 'error' : 'success' }, publishElapsed ); - }); + }) + .catch((err) => { + this.logger.warn(`Failed to record IPNS publish metrics: ${err instanceof Error ? err.message : String(err)}`); + });🤖 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 81 - 95, The promise chain starting at delegatedRouting.publish(dto.ipnsName, recordBytes) needs a terminal .catch to handle exceptions thrown inside the .then() (e.g., failures in metricsService.ipnsPublishDuration.observe), so append a final .catch that logs the error (use this.logger.error or this.logger.warn) and, if appropriate, marks the metrics outcome as 'error' using dto.ipnsName and publishStart context; ensure you reference delegatedRouting.publish, the .then callback that computes publishElapsed from publishStart, and metricsService.ipnsPublishDuration.observe when adding the defensive catch.
🤖 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.ts`:
- Around line 81-95: The promise chain starting at
delegatedRouting.publish(dto.ipnsName, recordBytes) needs a terminal .catch to
handle exceptions thrown inside the .then() (e.g., failures in
metricsService.ipnsPublishDuration.observe), so append a final .catch that logs
the error (use this.logger.error or this.logger.warn) and, if appropriate, marks
the metrics outcome as 'error' using dto.ipnsName and publishStart context;
ensure you reference delegatedRouting.publish, the .then callback that computes
publishElapsed from publishStart, and metricsService.ipnsPublishDuration.observe
when adding the defensive catch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0677da66-41f0-4ec1-9622-edd0c754a0fe
📒 Files selected for processing (2)
apps/api/src/ipns/ipns.service.spec.tsapps/api/src/ipns/ipns.service.ts
There was a problem hiding this comment.
Pull request overview
This PR updates the API’s IPNS publish path to avoid blocking responses on slow delegated routing/DHT propagation by converting delegated routing publish into a fire-and-forget operation, while keeping metrics collection asynchronous.
Changes:
- Make
DelegatedRoutingClient.publish(...)fire-and-forget and recordipnsPublishDurationmetrics asynchronously. - Update unit tests to flush the queue so detached metrics callbacks run before assertions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/src/ipns/ipns.service.ts | Converts delegated routing publish to a detached promise chain and keeps publish duration metrics. |
| apps/api/src/ipns/ipns.service.spec.ts | Adds microtask/nextTick flushes in tests that assert async metrics behavior. |
- Fix comment saying "finally block" when code uses catch+then chain - Add trailing .catch() to prevent unhandled rejection if observe() throws Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4f5e70eaac60
…get chain Covers the trailing .catch() that handles errors from ipnsPublishDuration.observe() throwing in the detached promise chain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: fc32db80737c
Summary
resolveRecord()always prefers DB data, so awaiting DHT propagation is unnecessaryawait this.delegatedRouting.publish(...)to a detached promise chainChanges
apps/api/src/ipns/ipns.service.ts— fire-and-forget DHT publish with async metrics collectionapps/api/src/ipns/ipns.service.spec.ts— flush microtask queue in 3 tests that verify async metricsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit