From 0da0c19545e2aa73cb9666a02530f912cdad0f59 Mon Sep 17 00:00:00 2001 From: Michael Yankelev Date: Sat, 21 Mar 2026 18:16:38 +0100 Subject: [PATCH 1/3] fix(api): make delegated routing publish fire-and-forget 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) Entire-Checkpoint: 2c05abce85b4 --- apps/api/src/ipns/ipns.service.spec.ts | 10 +++++++ apps/api/src/ipns/ipns.service.ts | 37 ++++++++++++++------------ 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/apps/api/src/ipns/ipns.service.spec.ts b/apps/api/src/ipns/ipns.service.spec.ts index 214df9219c..5b437c33c2 100644 --- a/apps/api/src/ipns/ipns.service.spec.ts +++ b/apps/api/src/ipns/ipns.service.spec.ts @@ -936,6 +936,10 @@ describe('IpnsService', () => { metadataCid: testMetadataCid, }); + // Delegated routing publish is fire-and-forget; flush microtask queue + // so the detached .then() callback that records metrics can run. + await new Promise(process.nextTick); + expect(mockStartTimer).toHaveBeenCalledWith({ operation: 'publish', source: '' }); expect(mockEndTimer).toHaveBeenCalledWith({ result: 'success' }); expect(mockMetricsService.ipnsPublishDuration.observe).toHaveBeenCalledWith( @@ -974,6 +978,9 @@ describe('IpnsService', () => { metadataCid: testMetadataCid, }); + // Delegated routing publish is fire-and-forget; flush microtask queue + await new Promise(process.nextTick); + expect(mockMetricsService.ipnsPublishDuration.observe).toHaveBeenCalledWith( { outcome: 'error' }, expect.any(Number) @@ -995,6 +1002,9 @@ describe('IpnsService', () => { metadataCid: testMetadataCid, }); + // Delegated routing publish is fire-and-forget; flush microtask queue + await new Promise(process.nextTick); + expect(mockMetricsService.ipnsPublishDuration.observe).toHaveBeenCalledWith( { outcome: 'error' }, expect.any(Number) diff --git a/apps/api/src/ipns/ipns.service.ts b/apps/api/src/ipns/ipns.service.ts index 2a2c71845e..f1841a4083 100644 --- a/apps/api/src/ipns/ipns.service.ts +++ b/apps/api/src/ipns/ipns.service.ts @@ -72,24 +72,27 @@ export class IpnsService { dto.expectedSequenceNumber ); - // Publish to delegated routing API (best-effort — DB is the reliable source) + // Publish to delegated routing API (fire-and-forget — DB is the reliable source). + // DHT propagation via someguy takes ~10-30s per record. Since the DB record + // is already saved and resolveRecord() always checks/prefers DB data, there is + // no need to block the API response on DHT propagation. Metrics are still + // collected via the detached promise's finally block. const publishStart = process.hrtime.bigint(); - let publishOutcome = 'success'; - try { - await this.delegatedRouting.publish(dto.ipnsName, recordBytes); - } catch (error) { - result = 'delegated_error'; - publishOutcome = 'error'; - this.logger.warn( - `Delegated routing publish failed for ${dto.ipnsName}, DB record saved: ${error instanceof Error ? error.message : String(error)}` - ); - } finally { - const publishElapsed = Number(process.hrtime.bigint() - publishStart) / 1e9; - this.metricsService.ipnsPublishDuration.observe( - { outcome: publishOutcome }, - publishElapsed - ); - } + 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 + ); + }); return { success: true, From 783ba8c76004b1ef7e2d685b8ad8cc14aae07ecf Mon Sep 17 00:00:00 2001 From: Michael Yankelev Date: Sat, 21 Mar 2026 19:16:06 +0100 Subject: [PATCH 2/3] =?UTF-8?q?fix(api):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20fix=20comment=20text=20and=20add=20trailing=20catch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Entire-Checkpoint: 4f5e70eaac60 --- apps/api/src/ipns/ipns.service.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/api/src/ipns/ipns.service.ts b/apps/api/src/ipns/ipns.service.ts index f1841a4083..5b79f4a988 100644 --- a/apps/api/src/ipns/ipns.service.ts +++ b/apps/api/src/ipns/ipns.service.ts @@ -76,7 +76,7 @@ export class IpnsService { // DHT propagation via someguy takes ~10-30s per record. Since the DB record // is already saved and resolveRecord() always checks/prefers DB data, there is // no need to block the API response on DHT propagation. Metrics are still - // collected via the detached promise's finally block. + // collected via the detached promise chain (catch + then). const publishStart = process.hrtime.bigint(); this.delegatedRouting .publish(dto.ipnsName, recordBytes) @@ -92,6 +92,11 @@ export class IpnsService { { outcome: outcome === 'error' ? 'error' : 'success' }, publishElapsed ); + }) + .catch((error) => { + this.logger.error( + `Failed to record IPNS publish metrics for ${dto.ipnsName}: ${error instanceof Error ? error.message : String(error)}` + ); }); return { From 632ce03b359b90492a60fd7cf59b7dce636c5a9f Mon Sep 17 00:00:00 2001 From: Michael Yankelev Date: Sat, 21 Mar 2026 19:23:45 +0100 Subject: [PATCH 3/3] test(api): add coverage for metrics observe() failure in fire-and-forget 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) Entire-Checkpoint: fc32db80737c --- apps/api/src/ipns/ipns.service.spec.ts | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/apps/api/src/ipns/ipns.service.spec.ts b/apps/api/src/ipns/ipns.service.spec.ts index 5b437c33c2..f0bcd9c39b 100644 --- a/apps/api/src/ipns/ipns.service.spec.ts +++ b/apps/api/src/ipns/ipns.service.spec.ts @@ -1010,6 +1010,33 @@ describe('IpnsService', () => { expect.any(Number) ); }); + + it('should log error and not crash when metrics observe() throws', async () => { + mockFolderIpnsRepo.findOne.mockResolvedValue(null); + mockFolderIpnsRepo.create.mockReturnValue({ ...mockFolderEntity, sequenceNumber: '1' }); + mockFolderIpnsRepo.save.mockResolvedValue({ ...mockFolderEntity, sequenceNumber: '1' }); + mockMetricsService.ipnsPublishDuration.observe.mockImplementation(() => { + throw new Error('metrics explosion'); + }); + + const loggerSpy = jest.spyOn(service['logger'], 'error').mockImplementation(); + + const result = await service.publishRecord(testUserId, { + ipnsName: testIpnsName, + record: testRecord, + metadataCid: testMetadataCid, + }); + + // Flush fire-and-forget promise chain + await new Promise(process.nextTick); + + expect(result.success).toBe(true); + expect(loggerSpy).toHaveBeenCalledWith( + expect.stringContaining('Failed to record IPNS publish metrics') + ); + + loggerSpy.mockRestore(); + }); }); describe('conflict detection', () => {