fix(api,desktop): fix sequence number mismatch in cached IPNS resolves#449
Merged
Conversation
Two fixes for the desktop E2E "Persistent conflict" regression: 1. API: parseCachedRecord now uses the DB column's sequenceNumber as authoritative instead of parsing it from the signedRecord bytes. The record bytes contain the client's pre-increment value (e.g. 0 on first publish), while the DB column is always incremented by upsertFolderIpns (to 1). Using the record's value caused resolve to return seq 0, leading to 409 conflicts on the next publish. 2. Desktop: Seed PublishCoordinator cache with sequence numbers from the IPNS resolves during FUSE mount pre-population. Previously the cache was empty, forcing a redundant resolve on first write that could race with other operations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2b690f04c0ad
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCI now captures staged logs on failure; API and web IPNS handling favor DB-stored CID/sequence numbers when falling back to cached signed-records while retaining parsed signature fields; protobuf group wire types are skipped (not erroring); desktop FUSE seeds initial IPNS sequences and drains upload completions; E2E polling windows were lengthened. Changes
Sequence Diagram(s)sequenceDiagram
actor Mount as "Mount (tauri)"
participant Resolver as "IPNS Resolver / API"
participant Prepop as "Pre-populator"
participant PubCoord as "PublishCoordinator"
Mount->>Resolver: resolve IPNS for root and subfolders
Note right of Resolver: returns (data, path, sequence_number)
Resolver-->>Mount: (data, path, sequence_number)
Mount->>Prepop: collect (ipns_name, sequence_number) pairs
Prepop->>PubCoord: record_publish(name, sequence_number) for each pair
PubCoord-->>Prepop: ack
Prepop-->>Mount: pre-population complete
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 docstrings
🧪 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 |
…workspace upload-artifact@v7 requires files within the repo root. On Windows, the desktop log is in RUNNER_TEMP (D:\a\_temp\) which is outside the repo. Copy all logs into a failure-logs/ directory before uploading. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2d0d04e68084
github-actions
Bot
added
release:api:fix
Contributor
Release Preview
Cascade Details
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #449 +/- ##
==========================================
+ Coverage 62.43% 71.57% +9.13%
==========================================
Files 135 114 -21
Lines 10130 7261 -2869
Branches 1067 1081 +14
==========================================
- Hits 6325 5197 -1128
+ Misses 3584 1840 -1744
- Partials 221 224 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes IPNS sequence-number conflicts caused by cached resolve responses returning a stale sequence number, and improves desktop FUSE mount behavior by capturing sequence numbers during pre-population for later publish coordination.
Changes:
- API:
parseCachedRecordnow treats the DBsequenceNumbercolumn as authoritative when returning cached resolve results. - Desktop: FUSE mount pre-population now parses
sequence_numberfrom resolve responses and seeds thePublishCoordinatorsequence cache.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/api/src/ipns/ipns.service.ts | Overrides cached resolve sequence numbers with the DB column to avoid stale values parsed from signed record bytes. |
| apps/desktop/src-tauri/src/fuse/mod.rs | Tracks sequence numbers during root/subfolder pre-populate resolves and seeds the publish coordinator cache. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/desktop-e2e.yml:
- Line 507: The cp command copying apps/api/*.log is likely a no-op because the
API logs to stdout/stderr rather than files; replace that behavior by
redirecting the API startup output to a known file and copy that file into
failure-logs instead. Concretely, update the workflow step that runs the API to
tee or redirect its stdout/stderr to a persistent path (e.g.,
$RUNNER_TEMP/api.log) and change/remove the existing cp apps/api/*.log
failure-logs/ invocation so the workflow instead copies $RUNNER_TEMP/api.log (or
both) into failure-logs to ensure API-side logs are collected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29e8e072-66c7-4d47-b73f-efb51100a28d
📒 Files selected for processing (3)
.github/workflows/desktop-e2e.ymlapps/api/src/ipns/ipns.service.tsapps/desktop/src-tauri/src/fuse/mod.rs
…eprecated protobuf wire types Signature verification in resolveIpnsRecord now logs warnings instead of throwing, preventing vault init and folder loading from crashing when the API returns signature data that doesn't verify due to protobuf parser inconsistencies. The IPNS record parser now gracefully skips deprecated wire types 3/4 (start/end group) instead of throwing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4b4659a70a7b
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/services/ipns.service.ts (1)
173-210:⚠️ Potential issue | 🟠 MajorDon’t return a usable record after an explicit signature/key mismatch.
At Line 173 this turns
resolveIpnsRecord()into a fail-open path: invalid signatures andpubKey→ipnsNamemismatches are only logged, then the function still returns a non-null record. The current web callers inapps/web/src/services/vault-settings.service.ts(Lines 35-60 and Lines 105-125) andapps/web/src/services/file-metadata.service.ts(Lines 315-335) do not inspectsignatureVerified, so they will silently trust the returnedcid/sequenceNumber. That regresses the client-side authenticity check and also diverges frompackages/sdk-core/src/ipns/index.ts, which still throws on the same conditions. If parser inconsistencies need a temporary workaround, keep the missing-signature case non-fatal, but continue to reject explicit cryptographic failures until all callers are updated.Based on learnings: client verifies Ed25519 signature + pubKey-to-ipnsName binding on
resolveRecordas a current mitigation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/ipns.service.ts` around lines 173 - 210, The resolveIpnsRecord implementation currently logs but returns a usable record on explicit crypto failures; change it so that when verifyIpnsSignature returns false or deriveIpnsName does not match ipnsName the function rejects/throws (or returns null/undefined) instead of returning a record with signatureVerified=false; keep the existing non-fatal behavior only for the "missing signature/pubKey" branch (leave the logger and continue returning null/empty), but for the explicit failure paths in resolveIpnsRecord (references: verifyIpnsSignature, deriveIpnsName, signatureVerified) immediately abort and return an error/value that callers must handle to avoid silently trusting cid/sequenceNumber.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/ipns/ipns-record-parser.ts`:
- Around line 113-134: In the wireType === 3 group-skip block in
ipns-record-parser.ts (the loop that uses variables pos, buf, depth and calls
readVarint), add explicit bounds checks before advancing pos for each inner
branch: ensure pos + 4 and pos + 8 are within buf.length before adding 4/8, and
ensure np2 + Number(len) <= buf.length before setting pos = np2 + Number(len);
throw a parsing error if any check fails. After the while loop, verify depth ===
0 and throw if not (to fail closed on unterminated start-group). Also when
encountering a top-level wireType === 4 (end group) outside a group, do not
silently skip — reject by throwing a parsing error. Use the existing readVarint,
pos, buf, depth, innerWire, np/np2, and len identifiers to locate and implement
these checks.
---
Outside diff comments:
In `@apps/web/src/services/ipns.service.ts`:
- Around line 173-210: The resolveIpnsRecord implementation currently logs but
returns a usable record on explicit crypto failures; change it so that when
verifyIpnsSignature returns false or deriveIpnsName does not match ipnsName the
function rejects/throws (or returns null/undefined) instead of returning a
record with signatureVerified=false; keep the existing non-fatal behavior only
for the "missing signature/pubKey" branch (leave the logger and continue
returning null/empty), but for the explicit failure paths in resolveIpnsRecord
(references: verifyIpnsSignature, deriveIpnsName, signatureVerified) immediately
abort and return an error/value that callers must handle to avoid silently
trusting cid/sequenceNumber.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c36c36fc-5b0a-41d4-9b47-d658eaa729b7
📒 Files selected for processing (3)
apps/api/src/ipns/ipns-record-parser.spec.tsapps/api/src/ipns/ipns-record-parser.tsapps/web/src/services/ipns.service.ts
When rmdir/unlink/rename removes an inode, the publish queue entry for that inode was not cleaned up. If the deleted folder had been queued for publishing, flush_publish_queue would later fail with "Folder inode N not found" because the inode was already removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 55436d4d4c81
The FUSE publish pipeline (debounce + IPFS upload + IPNS publish) can take slightly over 60s in CI. Double the polling attempts from 12 to 24 (keeping 5s intervals) to give sufficient headroom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6f29768846ab
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 (3)
tests/desktop-e2e/scripts/test-round-trip.ps1 (1)
197-209:⚠️ Potential issue | 🟡 MinorFailure message says "24s" but actual polling duration is 48s.
Same inconsistency as the bash version. The loop runs 24 attempts × 2s sleep = 48s.
Proposed fix
- Test-Fail "IPNS resolve did not return expected CID after 24s polling" + Test-Fail "IPNS resolve did not return expected CID after 48s polling"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/desktop-e2e/scripts/test-round-trip.ps1` around lines 197 - 209, The failure message's duration is incorrect: the polling loop in the for construct (for ($attempt = 1; $attempt -le 24 -and -not $ResolvedCid; $attempt++)) sleeps 2 seconds each iteration (Start-Sleep -Seconds 2) so total = 48s; update the Test-Fail string "IPNS resolve did not return expected CID after 24s polling" to reflect "48s" (and likewise adjust any related Test-Pass/diagnostic messages if present) so the message matches the actual polling duration used by the loop and variables $attempt, $ResolvedCid, $ApiUrl remain unchanged.tests/desktop-e2e/scripts/test-round-trip.sh (2)
95-111:⚠️ Potential issue | 🟡 MinorFailure message still references old 60s timeout.
Line 111's failure message says "after 60s polling" but the loop now runs 24 iterations × 5s = 120s.
Proposed fix
- fail "Vault has no rootIpnsName after 60s polling (error: $VAULT_ERROR)" + fail "Vault has no rootIpnsName after 120s polling (error: $VAULT_ERROR)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/desktop-e2e/scripts/test-round-trip.sh` around lines 95 - 111, The failure message still mentions "after 60s polling" but the loop now polls for 24 attempts × 5s = 120s; update the fail message to reflect the current timeout. Locate the polling loop and the post-check that references ROOT_IPNS and VAULT_RESPONSE (variables ROOT_IPNS, VAULT_RESPONSE and the for attempt in $(seq 1 24) loop) and change the static text "after 60s polling" to "after 120s polling" (or compute/derive the timeout from attempt/count/delay if preferred) so the error accurately reports the real timeout.
148-164:⚠️ Potential issue | 🟡 MinorFailure message says "24s" but actual polling duration is 48s.
The loop runs 24 attempts × 2s sleep = 48s total wait time.
Proposed fix
- fail "IPNS resolve did not return expected CID after 24s polling (error: $IPNS_ERROR)" + fail "IPNS resolve did not return expected CID after 48s polling (error: $IPNS_ERROR)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/desktop-e2e/scripts/test-round-trip.sh` around lines 148 - 164, The failure message incorrectly states "24s" while the polling actually sleeps 2s for 24 attempts (48s); update the fail message after the loop to reflect the real timeout (48s) or compute it dynamically (e.g., attempts*sleep) so the message matches the behavior—change the string in the fail call that currently reads "IPNS resolve did not return expected CID after 24s polling (error: $IPNS_ERROR)" to the correct duration and ensure this message is emitted in the same block that references RESOLVED_CID and IPNS_RESPONSE.
🤖 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 `@tests/desktop-e2e/scripts/test-round-trip.ps1`:
- Around line 197-209: The failure message's duration is incorrect: the polling
loop in the for construct (for ($attempt = 1; $attempt -le 24 -and -not
$ResolvedCid; $attempt++)) sleeps 2 seconds each iteration (Start-Sleep -Seconds
2) so total = 48s; update the Test-Fail string "IPNS resolve did not return
expected CID after 24s polling" to reflect "48s" (and likewise adjust any
related Test-Pass/diagnostic messages if present) so the message matches the
actual polling duration used by the loop and variables $attempt, $ResolvedCid,
$ApiUrl remain unchanged.
In `@tests/desktop-e2e/scripts/test-round-trip.sh`:
- Around line 95-111: The failure message still mentions "after 60s polling" but
the loop now polls for 24 attempts × 5s = 120s; update the fail message to
reflect the current timeout. Locate the polling loop and the post-check that
references ROOT_IPNS and VAULT_RESPONSE (variables ROOT_IPNS, VAULT_RESPONSE and
the for attempt in $(seq 1 24) loop) and change the static text "after 60s
polling" to "after 120s polling" (or compute/derive the timeout from
attempt/count/delay if preferred) so the error accurately reports the real
timeout.
- Around line 148-164: The failure message incorrectly states "24s" while the
polling actually sleeps 2s for 24 attempts (48s); update the fail message after
the loop to reflect the real timeout (48s) or compute it dynamically (e.g.,
attempts*sleep) so the message matches the behavior—change the string in the
fail call that currently reads "IPNS resolve did not return expected CID after
24s polling (error: $IPNS_ERROR)" to the correct duration and ensure this
message is emitted in the same block that references RESOLVED_CID and
IPNS_RESPONSE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7be43c5-09a2-4f50-9bf2-a996c0d5dd8d
📒 Files selected for processing (2)
tests/desktop-e2e/scripts/test-round-trip.ps1tests/desktop-e2e/scripts/test-round-trip.sh
…dir in E2E tests The FUSE publish pipeline never fired in CI because after release() spawns the background upload, no further FUSE callbacks triggered drain_upload_completions(). The upload would complete and send its result to the channel, but nothing drained it. Two fixes: 1. Add drain_upload_completions() to handle_readdir() on macOS/Linux (Windows dir_ops already had it) 2. Add explicit `ls` / `Get-ChildItem` to E2E test scripts after file write to force a FUSE readdir callback, ensuring the upload drain fires even when no other filesystem activity occurs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 2710ad6dff0d
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)
tests/desktop-e2e/scripts/test-round-trip.ps1 (1)
202-214:⚠️ Potential issue | 🟡 MinorMinor: Error message arithmetic is off.
The loop runs 24 attempts with 2-second sleeps, totaling ~48 seconds of polling, but line 214 reports "24s polling".
Proposed fix
if ($ResolvedCid) { Test-Pass "IPNS resolve returned CID ($ResolvedCid)" } else { - Test-Fail "IPNS resolve did not return expected CID after 24s polling" + Test-Fail "IPNS resolve did not return expected CID after 48s polling" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/desktop-e2e/scripts/test-round-trip.ps1` around lines 202 - 214, The failure message incorrectly states "24s polling" even though the for loop does 24 attempts with a 2-second Start-Sleep (≈48 seconds); update the Test-Fail call so it reports the correct total polling duration (e.g., "48s polling") or compute it dynamically from the loop variables ($attempt count * 2) before calling Test-Fail; locate the loop using $attempt and $ResolvedCid and change the string passed to Test-Fail (or replace it with a computed value) so the reported duration matches the actual polling time.
🤖 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 `@tests/desktop-e2e/scripts/test-round-trip.ps1`:
- Around line 202-214: The failure message incorrectly states "24s polling" even
though the for loop does 24 attempts with a 2-second Start-Sleep (≈48 seconds);
update the Test-Fail call so it reports the correct total polling duration
(e.g., "48s polling") or compute it dynamically from the loop variables
($attempt count * 2) before calling Test-Fail; locate the loop using $attempt
and $ResolvedCid and change the string passed to Test-Fail (or replace it with a
computed value) so the reported duration matches the actual polling time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba1aad48-5b3e-468b-a7bd-0a33e1a30b0f
📒 Files selected for processing (3)
crates/fuse/src/dir_ops.rstests/desktop-e2e/scripts/test-round-trip.ps1tests/desktop-e2e/scripts/test-round-trip.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/desktop-e2e/scripts/test-round-trip.sh
The single ls after file write wasn't sufficient on macOS — FUSE-T's SMB backend caches directory listings. Add stat + ls on each polling iteration to repeatedly trigger lookup/getattr/readdir callbacks, ensuring drain_upload_completions() fires even with SMB caching. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 79529e555970
Windows E2E was passing before the polling changes. Revert to the original test-round-trip.ps1 from main — the FUSE drain fixes are only needed for macOS FUSE-T's SMB backend. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8450bb21af72
FSM1
force-pushed
the
fix/desktop-e2e-sequence-mismatch
branch
from
8ea1325 to
71515f2
Compare
- Log warnings on IPNS sequence parse failures instead of silent unwrap_or(0) - Add CID mismatch warning and use DB columns as authoritative in parseCachedRecord - Add bounds checks and fail-closed behavior in protobuf group wire type scanning - Redirect API stdout/stderr to log file in desktop E2E workflow for artifact collection - Add test for sequence/CID mismatch between signed record bytes and DB columns Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: b7341cfae98b
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 0d2b6601683b
Merged
This was referenced Jun 14, 2026
This was referenced Jun 26, 2026
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
parseCachedRecordreturning stale sequence number from signed record bytes instead of the DB columnPublishCoordinatorcache with sequence numbers from pre-populate resolvesRoot cause
The desktop E2E "Persistent conflict" failure was caused by
parseCachedRecordparsing thesignedRecordbytes and returning the embedded sequence (0, the client's pre-increment value), while the DB column had the correct value (1). This caused the FUSE client'sresolve_sequenceto return 0, leading to a 409 conflict on the next publish.Changes
apps/api/src/ipns/ipns.service.ts—parseCachedRecordnow usescached.sequenceNumber(DB column) as authoritative, overriding the value parsed from signed record bytes.apps/desktop/src-tauri/src/fuse/mod.rs— Capture sequence numbers from IPNS resolves during FUSE mount pre-population and seed thePublishCoordinatorcache, preventing redundant resolves on first write.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Behavior Changes
Tests
Chores