perf: optimize IPFS upload with concurrent pins and pebbleds datastore#342
Merged
Conversation
Entire-Checkpoint: cd99e3a83c8b
Entire-Checkpoint: 7865dece39f3
- Archive upload-throughput and mixed-workload metrics as pre-19.2 baseline JSON - Document uploadFile p50/p95/p99, server-side latency breakdown, optimization targets - Source: existing Phase 19 load test results (local API not running at capture time) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 577280bff1b1
- Test 1: Verify batchPublishIpnsRecords and updateFolderMetadataAndPublish run concurrently - Test 2: Non-critical batch publish failure logs warning, upload succeeds - Test 3: Critical folder metadata failure throws (already passes) - Test 4: Both reject, folder error takes priority over batch error Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 285632b23158
- Replace sequential batchPublishIpnsRecords + updateFolderMetadataAndPublish with Promise.allSettled for concurrent execution - Folder metadata failure is critical (throws) - IPNS batch publish failure is non-critical (logs warning, upload succeeds) - Saves ~1.73s per upload by overlapping independent pin operations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 25989fb4017a
…tion plan - SUMMARY.md with pre-optimization baselines and Promise.allSettled implementation - STATE.md updated with plan progress and metrics - ROADMAP.md updated with phase 19.2 plan progress (1/2 complete) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2350295ee724
- Add IPFS_PROFILE=server,pebbleds to local dev docker-compose.yml - Update IPFS_PROFILE to server,pebbleds in staging docker-compose.staging.yml - Add comments explaining volume recreation requirement for datastore migration Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 98dfa783348e
…comparison - Run upload throughput test (5 clients): p50=1.5s, 0 errors, 3.12 ops/s - Kubo pebbleds datastore confirmed active via API - Before/after comparison table with percentage changes - Success criteria analysis (SC1 likely met, SC3 pending staging validation) - Document environment caveats (local vs staging baseline mismatch) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 5104e672a608
- SUMMARY.md with before/after comparison and success criteria analysis - STATE.md updated: phase 19.2 complete, 158 plans total - ROADMAP.md updated with plan progress Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 42767be6f2e2
…ging validation Entire-Checkpoint: 9617cd6191ba
Replace staging-dependent validation plan with local three-point comparison: (1) no opts, (2) SDK concurrent pins, (3) SDK + pebbleds. Same environment + client count across all runs for valid comparison. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 91c9d9b0bf22
- Add PERF-09 definition to Performance Baselines section - Add PERF-09 to traceability table mapped to Phase 19.2 (Complete) - Update coverage count from 36 to 37 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4889ad36c596
- Add 19.2-03-SUMMARY.md documenting requirement registration - Update STATE.md position to plan 3 of 4 - Update ROADMAP.md with plan progress (3/4 complete) - Include 19.2-VERIFICATION.md from verifier Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2fc8ea0ce5dc
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 03e68cad730e
…meguy routing - Add Run 0 concurrency probe to find stable client count ceiling - Specify inline env vars (LOAD_TEST_SECRET, THROTTLE_BYPASS_SECRET) since vitest does not load dotenv - Use someguy (192.168.133.114:8190) instead of mock-ipns-routing - Document exact API .env config requirements Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 93e1ea74f465
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: c660dc8f2284
…tion baselines - Added concurrency probe table (Run 0) showing 50-client ceiling - Added three-point results table with SDK delta, pebbleds delta, and total delta - Documented optimization attribution: concurrent pins alone hurt at 50 clients, pebbleds rescues - Added definitive SC analysis: SC4 met, SC2 observable, SC1/SC3 not met at 50 clients - Updated caveats to reflect three-point findings - Archived all three-point JSON data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 0a7e10cbd575
- Created 19.2-04-SUMMARY.md documenting concurrency probe and three-point comparison findings - Updated STATE.md: Phase 19.2 complete (4/4), added synergistic optimization decision - Updated ROADMAP.md: marked plans 03 and 04 complete, 4/4 plans, phase status Complete Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 566c46040c36
Pebbleds eliminates the failure mode at 75 clients that flatfs exhibited (10 errors, timeout). Throughput +43.9%, p95 -52.8%, zero errors. SC3 (>15% throughput) met at 75-client level. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: f95972856ab6
Re-verification: 6/6 must-haves verified. Both gaps closed (PERF-09 registration + three-point local baselines). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2004db778750
|
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 (2)
WalkthroughAdds Phase 19.2 artifacts and changes: concurrent IPNS/file-folder publish orchestration in Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CipherBoxClient
participant IPNS as sdkCore.batchPublishIpnsRecords
participant Folder as sdkCore.updateFolderMetadataAndPublish
participant State as InternalState
rect rgba(200,150,255,0.5)
Note over Client,State: BEFORE — Sequential execution
Client->>IPNS: publish file IPNS record
activate IPNS
IPNS-->>Client: filePublishResult
deactivate IPNS
Client->>Folder: update folder metadata & publish
activate Folder
Folder-->>Client: folderPublishResult
deactivate Folder
Client->>State: apply results
end
rect rgba(150,200,150,0.5)
Note over Client,State: AFTER — Concurrent execution (Promise.allSettled)
par Parallel calls
Client->>IPNS: publish file IPNS record
activate IPNS
Client->>Folder: update folder metadata & publish
activate Folder
and
IPNS-->>Client: filePublishResult (fulfill/reject)
deactivate IPNS
Folder-->>Client: folderPublishResult (fulfill/reject)
deactivate Folder
end
alt Folder rejects
Client->>Client: throw folder error (critical)
else Folder fulfills, IPNS rejects
Client->>Client: log warning + emit ipns:batchPublishFailed (non-critical)
Client->>State: apply folderPublishResult
else Both fulfill
Client->>State: apply both results
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 #342 +/- ##
==========================================
+ Coverage 50.73% 50.84% +0.11%
==========================================
Files 115 114 -1
Lines 9227 9252 +25
Branches 727 734 +7
==========================================
+ Hits 4681 4704 +23
- Misses 4367 4369 +2
Partials 179 179
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ailed event - Extract createTestConfig and setupFolder to shared __tests__/helpers.ts (was duplicated across 3 test files) - Add ipns:batchPublishFailed typed event for consumer observability (consistent with share:reWrapFailed pattern) - Replace setTimeout(10) with Promise.resolve() in concurrency test - Add comment about benign orphaned IPNS record on folder-update failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 70577f6deb2d
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/sdk/src/__tests__/client-upload-concurrency.test.ts (1)
220-221: Minor: Mock return value doesn't match actual signature.
batchPublishIpnsRecordsreturnsPromise<{ totalSucceeded: number; totalFailed: number }>per the real signature, but the mock resolves withundefined. This works here because the test validates the rejection path, but for consistency:🔧 Suggested fix for type consistency
- vi.mocked(sdkCore.batchPublishIpnsRecords).mockResolvedValue(undefined); + vi.mocked(sdkCore.batchPublishIpnsRecords).mockResolvedValue({ totalSucceeded: 1, totalFailed: 0 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/__tests__/client-upload-concurrency.test.ts` around lines 220 - 221, The mock for batchPublishIpnsRecords uses mockResolvedValue(undefined) but the real signature returns Promise<{ totalSucceeded: number; totalFailed: number }>, so update the test to resolve with an object matching that shape (e.g., { totalSucceeded: 0, totalFailed: 0 }) when calling vi.mocked(sdkCore.batchPublishIpnsRecords) so the mock matches the actual function signature; keep vi.mocked(sdkCore.updateFolderMetadataAndPublish).mockRejectedValue(folderError) as-is since it’s testing the rejection path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md:
- Around line 174-179: Update the Pitfall 1 text to reflect the actual
asymmetric failure policy used in implementation: state that folder metadata
publish is treated as a critical failure (causes upload to fail) while batch
IPNS/file IPNS publish failures are treated as non-critical warnings or retried
according to the logic in packages/sdk/src/client.ts; replace the current
recommendation that "either Promise outcome should fail the upload" with
guidance to use Promise.allSettled for observation but explicitly document that
only folder metadata failure is fatal and batch IPNS failures are tolerated per
the existing prioritized partial-failure handling.
In `@docker/docker-compose.staging.yml`:
- Around line 66-69: The staging compose change enables
IPFS_PROFILE=server,pebbleds but the deploy-staging workflow still runs docker
compose up -d without removing the old ipfs_staging volume, which will break
Kubo on first deploy; update the deploy-staging workflow to run a one-time
migration step that executes docker compose -f docker-compose.staging.yml down
-v (or docker compose down -v) before docker compose up -d when deploying this
change (or add a gated boolean/migration flag like STAGING_IPFS_MIGRATE to run
the down -v only once), and/or add a clear operator note in deployment docs
referencing the ipfs_staging volume and the IPFS_PROFILE change so operators
know to reset the volume before first deploy.
---
Nitpick comments:
In `@packages/sdk/src/__tests__/client-upload-concurrency.test.ts`:
- Around line 220-221: The mock for batchPublishIpnsRecords uses
mockResolvedValue(undefined) but the real signature returns Promise<{
totalSucceeded: number; totalFailed: number }>, so update the test to resolve
with an object matching that shape (e.g., { totalSucceeded: 0, totalFailed: 0 })
when calling vi.mocked(sdkCore.batchPublishIpnsRecords) so the mock matches the
actual function signature; keep
vi.mocked(sdkCore.updateFolderMetadataAndPublish).mockRejectedValue(folderError)
as-is since it’s testing the rejection path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d5f753d-5d20-45b1-81c8-272899157128
📒 Files selected for processing (24)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/baselines/19.2-post-optimization-baselines.md.planning/baselines/19.2-pre-optimization-baselines.md.planning/phases/19.2-ipfs-upload-performance-optimization/.gitkeep.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-01-PLAN.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-01-SUMMARY.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-02-PLAN.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-02-SUMMARY.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-03-PLAN.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-03-SUMMARY.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-04-PLAN.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-04-SUMMARY.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-VALIDATION.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-VERIFICATION.md.planning/todos/pending/2026-03-23-investigate-removal-of-mock-ipns-routing-layer.md.planning/todos/pending/2026-03-23-load-tests-should-handle-401s-with-token-refresh.mddocker/docker-compose.staging.ymldocker/docker-compose.ymlpackages/sdk/src/__tests__/client-upload-concurrency.test.tspackages/sdk/src/client.tstests/load/metrics-baseline-pre-19.2.json
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets improved uploadFile performance by parallelizing independent IPNS/folder publish steps in the SDK and increasing Kubo write throughput via the pebbleds datastore profile, with supporting baselines, planning artifacts, and tests.
Changes:
- Update
CipherBoxClient.uploadFile()to run file IPNS batch publish concurrently with folder metadata update usingPromise.allSettled. - Switch local + staging Docker Compose IPFS nodes to
IPFS_PROFILE=server,pebbleds(fresh repo required). - Add unit tests and capture/report pre/post performance baselines and Phase 19.2 planning/verification docs.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/load/metrics-baseline-pre-19.2.json | Adds archived pre-optimization load-test metrics JSON for comparison. |
| packages/sdk/src/events.ts | Adds ipns:batchPublishFailed SDK event type. |
| packages/sdk/src/client.ts | Implements concurrent IPNS batch publish + folder publish in uploadFile() with partial-failure handling. |
| packages/sdk/src/tests/helpers.ts | Adds shared test helpers (config + folder setup). |
| packages/sdk/src/tests/client.test.ts | Refactors tests to use shared helpers. |
| packages/sdk/src/tests/client-upload-concurrency.test.ts | Adds new tests covering concurrency + partial failure behavior for uploadFile(). |
| packages/sdk/src/tests/client-extended.test.ts | Refactors extended tests to use shared helpers. |
| docker/docker-compose.yml | Enables IPFS_PROFILE=server,pebbleds for local IPFS service with migration note. |
| docker/docker-compose.staging.yml | Enables IPFS_PROFILE: server,pebbleds for staging IPFS service with migration note. |
| .planning/todos/pending/2026-03-23-load-tests-should-handle-401s-with-token-refresh.md | Adds a todo for improving load-test auth refresh handling. |
| .planning/todos/pending/2026-03-23-investigate-removal-of-mock-ipns-routing-layer.md | Adds a todo to evaluate removing the mock IPNS routing layer. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-VERIFICATION.md | Adds Phase 19.2 verification report + remaining human checks. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-VALIDATION.md | Adds Phase 19.2 validation strategy document. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md | Adds detailed research write-up + architecture/pitfalls for the perf work. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-04-SUMMARY.md | Adds summary of matched-environment three-point comparison. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-04-PLAN.md | Adds execution plan for three-point local baseline methodology. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-03-SUMMARY.md | Adds summary for PERF-09 requirements registration gap closure. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-03-PLAN.md | Adds plan to register PERF-09 into REQUIREMENTS.md. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-02-SUMMARY.md | Adds summary of pebbleds adoption + post-optimization baselines capture. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-02-PLAN.md | Adds plan to switch Kubo datastore + capture baselines. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-01-SUMMARY.md | Adds summary of concurrent pins change + baseline capture. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-01-PLAN.md | Adds plan for baseline capture + concurrent pins implementation. |
| .planning/phases/19.2-ipfs-upload-performance-optimization/.gitkeep | Ensures phase directory exists in git. |
| .planning/baselines/19.2-pre-optimization-baselines.md | Adds pre-optimization baseline report. |
| .planning/baselines/19.2-post-optimization-baselines.md | Adds post-optimization baseline report including three-point comparison. |
| .planning/STATE.md | Updates current planning state to reflect Phase 19.2 completion. |
| .planning/ROADMAP.md | Inserts Phase 19.2 and updates progress table. |
| .planning/REQUIREMENTS.md | Registers PERF-09 and updates traceability + coverage counts. |
- Normalize batchResult.reason to Error before emitting event (Copilot) - Check batchResult.value.totalFailed for partial publish failures (Copilot) - Fix ROADMAP.md progress table column alignment (Copilot) - Fix post-optimization baselines SDK description accuracy (Copilot) - Update RESEARCH.md pitfall text to match asymmetric failure policy (CodeRabbit) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2c144ac654c4
- Return { totalSucceeded, totalFailed } from batchPublishIpnsRecords
mocks to match real function signature
- Add test for batchResult.value.totalFailed > 0 partial failure path
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 9d83ce8ea294
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md (1)
117-130:⚠️ Potential issue | 🟡 MinorUnify examples with the documented
Promise.allSettledfailure contract.Line 117 and Line 227 still show
Promise.all-style guidance, which conflicts with the same file’s pitfall policy (folder failure = critical, batch IPNS failure = non-critical). This can reintroduce the wrong implementation pattern.🛠️ Suggested doc fix
-// AFTER: concurrent -const [, publishResult] = await Promise.all([ - sdkCore.batchPublishIpnsRecords([uploadResult.ipnsRecord], this.ctx), - sdkCore.updateFolderMetadataAndPublish({ +// AFTER: concurrent with prioritized failure handling +const [batchResult, folderResult] = await Promise.allSettled([ + sdkCore.batchPublishIpnsRecords([uploadResult.ipnsRecord], this.ctx), + sdkCore.updateFolderMetadataAndPublish({ children: updatedChildren, folderKey: folder.folderKey, ipnsPrivateKey: folder.ipnsKeypair.privateKey, ipnsName: folderIpnsName, sequenceNumber: folder.sequenceNumber, ctx: this.ctx, }), ]); -const { newSequenceNumber } = publishResult; +if (folderResult.status === 'rejected') throw folderResult.reason; +if (batchResult.status === 'rejected') { + console.warn('[SDK] File IPNS batch publish failed:', batchResult.reason); +} +const { newSequenceNumber } = folderResult.value;#!/bin/bash # Verify implementation and docs are aligned on allSettled + prioritized handling. set -euo pipefail echo "== Locate CipherBoxClient.uploadFile implementation ==" fd client.ts packages sdk src 2>/dev/null || true rg -n -C3 'uploadFile\s*\(' packages/sdk/src/client.ts echo echo "== Check Promise API used in implementation ==" rg -n -C4 'Promise\.allSettled|Promise\.all' packages/sdk/src/client.ts echo echo "== Check research doc examples for Promise API usage ==" rg -n -C3 'Promise\.allSettled|Promise\.all' .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md echo echo "== Check prioritized failure handling language in research doc ==" rg -n -C2 'folder metadata publish failure.*critical|batch publish failure.*non-critical|warn \+ emit event' \ .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.mdBased on learnings,
packages/sdk/src/client.tsintentionally uses prioritized partial-failure handling (folder failure critical, batch IPNS failure non-critical).Also applies to: 227-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md around lines 117 - 130, The doc examples still show Promise.all usage; change them to Promise.allSettled for the concurrent publish so the code and pitfall policy align: call Promise.allSettled on [sdkCore.batchPublishIpnsRecords(...), sdkCore.updateFolderMetadataAndPublish(...)], inspect the results array by status, treat the updateFolderMetadataAndPublish result (the folder publish) as critical—if it is rejected, surface/throw the error and stop; treat the batchPublishIpnsRecords result as non‑critical—log/warn and emit the non‑fatal event if it failed but continue and use the folder publish output (newSequenceNumber) when fulfilled (refer to sdkCore.batchPublishIpnsRecords, sdkCore.updateFolderMetadataAndPublish, and publishResult/batchResult in the example).
🧹 Nitpick comments (2)
packages/sdk/src/__tests__/helpers.ts (1)
28-50: HardensetupFolderagainst shared-object aliasing and time nondeterminism.Right now, returned
childcan alias the same object stored inchildren, and repeatedDate.now()calls reduce reproducibility. A tiny refactor makes test fixtures more stable.Proposed refactor
-export function setupFolder(client: CipherBoxClient, ipnsName = 'folder-ipns') { +export function setupFolder( + client: CipherBoxClient, + ipnsName = 'folder-ipns', + now = Date.now() +) { const child = { type: 'file' as const, id: 'file1', name: 'test.txt', fileMetaIpnsName: 'k51file', ipnsPrivateKeyEncrypted: 'abc', - createdAt: Date.now(), - modifiedAt: Date.now(), + createdAt: now, + modifiedAt: now, }; client.getFolderTree().set(ipnsName, { ipnsName, folderKey: new Uint8Array(32).fill(1), ipnsKeypair: { publicKey: new Uint8Array(32).fill(2), privateKey: new Uint8Array(64).fill(3), }, sequenceNumber: 1n, - children: [child], + children: [{ ...child }], metadata: null, - lastLoadedAt: Date.now(), + lastLoadedAt: now, }); - return child; + return { ...child }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/__tests__/helpers.ts` around lines 28 - 50, The setupFolder helper currently stores the same child object into client.getFolderTree().set(...) and returns it and makes multiple Date.now() calls; to harden against aliasing and nondeterminism create a single timestamp const (e.g. const now = Date.now()) and use it for createdAt, modifiedAt and lastLoadedAt, and insert a defensive clone of the child into the folder's children (e.g. copy primitive fields and new Uint8Array copies for folderKey/ipnsKeypair) while returning a separate instance (or clone) so the returned object does not alias the stored children; update setupFolder, references to child, folderKey and ipnsKeypair accordingly..planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md (1)
13-14: Refresh recommendation text to reflect final Phase 19.2 rollout guidance.Line 13 and Line 305 still frame pebbleds as conditional (“if needed”), but this phase’s validated outcome is that concurrent pins alone can regress under high concurrency and should be paired with pebbleds for net benefit.
As per coding guidelines in
.planning/ROADMAP.md, success criteria should stay aligned with “concurrent pin orchestration + pebbleds” (not concurrent pins alone).Also applies to: 305-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md around lines 13 - 14, Update the Phase 19.2 recommendation text so the primary recommendation reads that the validated rollout requires "concurrent pin orchestration + pebbleds" (i.e., pair concurrent pin orchestration in the SDK client with swapping the Kubo datastore to pebbleds) rather than presenting pebbleds as conditional; edit the block currently at Line 13 and the repeated guidance at Lines 305–306 in .planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md to reflect this final rollout guidance and ensure the success criteria align with the ROADMAP.md requirement of "concurrent pin orchestration + pebbleds."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md:
- Around line 117-130: The doc examples still show Promise.all usage; change
them to Promise.allSettled for the concurrent publish so the code and pitfall
policy align: call Promise.allSettled on [sdkCore.batchPublishIpnsRecords(...),
sdkCore.updateFolderMetadataAndPublish(...)], inspect the results array by
status, treat the updateFolderMetadataAndPublish result (the folder publish) as
critical—if it is rejected, surface/throw the error and stop; treat the
batchPublishIpnsRecords result as non‑critical—log/warn and emit the non‑fatal
event if it failed but continue and use the folder publish output
(newSequenceNumber) when fulfilled (refer to sdkCore.batchPublishIpnsRecords,
sdkCore.updateFolderMetadataAndPublish, and publishResult/batchResult in the
example).
---
Nitpick comments:
In @.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md:
- Around line 13-14: Update the Phase 19.2 recommendation text so the primary
recommendation reads that the validated rollout requires "concurrent pin
orchestration + pebbleds" (i.e., pair concurrent pin orchestration in the SDK
client with swapping the Kubo datastore to pebbleds) rather than presenting
pebbleds as conditional; edit the block currently at Line 13 and the repeated
guidance at Lines 305–306 in
.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.md to
reflect this final rollout guidance and ensure the success criteria align with
the ROADMAP.md requirement of "concurrent pin orchestration + pebbleds."
In `@packages/sdk/src/__tests__/helpers.ts`:
- Around line 28-50: The setupFolder helper currently stores the same child
object into client.getFolderTree().set(...) and returns it and makes multiple
Date.now() calls; to harden against aliasing and nondeterminism create a single
timestamp const (e.g. const now = Date.now()) and use it for createdAt,
modifiedAt and lastLoadedAt, and insert a defensive clone of the child into the
folder's children (e.g. copy primitive fields and new Uint8Array copies for
folderKey/ipnsKeypair) while returning a separate instance (or clone) so the
returned object does not alias the stored children; update setupFolder,
references to child, folderKey and ipnsKeypair accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69c7110d-b0bf-4cba-9e7e-4471eef26b6b
📒 Files selected for processing (9)
.planning/ROADMAP.md.planning/baselines/19.2-post-optimization-baselines.md.planning/phases/19.2-ipfs-upload-performance-optimization/19.2-RESEARCH.mdpackages/sdk/src/__tests__/client-extended.test.tspackages/sdk/src/__tests__/client-upload-concurrency.test.tspackages/sdk/src/__tests__/client.test.tspackages/sdk/src/__tests__/helpers.tspackages/sdk/src/client.tspackages/sdk/src/events.ts
✅ Files skipped from review due to trivial changes (5)
- packages/sdk/src/tests/client-extended.test.ts
- packages/sdk/src/events.ts
- .planning/ROADMAP.md
- packages/sdk/src/client.ts
- packages/sdk/src/tests/client-upload-concurrency.test.ts
- Update RESEARCH.md examples from Promise.all to Promise.allSettled with prioritized failure handling (CodeRabbit duplicate) - Update recommendation text to reflect validated finding: concurrent pins require pebbleds for net benefit (CodeRabbit nitpick) - Harden setupFolder helper against object aliasing and timestamp nondeterminism (CodeRabbit nitpick) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 31f194efd531
Merged
This was referenced Mar 25, 2026
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
uploadFilenow usesPromise.allSettledto parallelize file IPNS publish and folder metadata update steps, reducing the sequential pin chainIPFS_PROFILE=server,pebbleds) for faster concurrent write throughputKey findings
At 75 clients, pebbleds eliminates the failure mode entirely (10 errors → 0, throughput +43.9%).
Critical finding: SDK concurrent pins alone cause a regression at high concurrency — both optimizations must be deployed together.
Staging migration note
Switching to pebbleds requires a fresh IPFS repo. Staging migration plan: parallel cutover with CID validation against DB before re-pinning to new node.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Reliability
Tests
Documentation