fix(kubo): remove the address rewriter proxy that now degrades provider records - #263
Merged
Merged
Conversation
Kubo 0.43.0 fixed the bug the proxy worked around: provider records PUT to a delegated HTTP router no longer drop the browser-dialable transports (AutoTLS /tls/ws, webrtc-direct). See ipfs/kubo#11394, closing the ipfs/kubo#11369 we filed. Kubo's Routing.Routers now hold the router URLs verbatim instead of loopback proxy URLs. Keeping the proxy is worse than removing it. It rebuilds the addr set from `kubo id` + `kubo swarm addrs`, and self swarm addrs carry no certhash, so the webrtc-direct and webtransport addrs it publishes are ones no browser can dial. A survey of the six production routers shows exactly that: nodes still behind the proxy have certhash-less records while natively announcing nodes have complete ones. Removed: - src/runtime/node/addresses-rewriter-proxy-server.ts (incl. the 2-minute failed-key retry loop; Kubo owns provide/reprovide now, and the browser-facing case is already covered by reprovide-on-address-change.ts) - src/runtime/node/address-rewriter-db.ts and its SQLite request logging - the proxy port machinery: PKC_ADDRESSES_REWRITER_START_PORT, the free-port walk, the persisted httprouter_proxy_<url> storage mapping - tcp-port-used from dependencies (still a devDependency, the test server and tests use it) setup-kubo-address-rewriter-and-http-router.ts is renamed to setup-kubo-http-routers.ts and now returns void; pkc.ts keeps a single _httpRouterSetupPromise in place of the setup/destroy pair. normalizeSelfAddrsForProvider moves to reprovide-on-address-change.ts, its only remaining caller, along with its unit tests. Tests: httprouter.test.ts asserts the routers are configured directly and that the records reaching them are usable. httprouter-direct-kubo.test.ts gains a kubo#11369 regression test: the throwaway daemon now listens on webrtc-direct, and the addr must survive into the record the router stored.
📝 WalkthroughWalkthroughPKC now configures Kubo with HTTP routers directly. The address-rewriter proxy, database, persistence, and cleanup paths were removed. Provider address normalization moved to local-community reprovisioning, and tests now cover direct routing and reconciliation. ChangesDirect HTTP-router migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PKC
participant HTTPRouterSetup
participant Kubo
participant HTTPRouters
PKC->>HTTPRouterSetup: start reconciliation
HTTPRouterSetup->>Kubo: set Routing.Routers
Kubo->>HTTPRouters: submit provider records
PKC->>HTTPRouterSetup: await setup during destroy
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkc/pkc.ts`:
- Around line 408-429: Update _waitForHttpRoutersSetupToSettle and the
setupKuboHttpRouters completion flow so the promise remains pending after POST
/shutdown until the embedded Kubo accepts a lightweight RPC request again; poll
the appropriate readiness endpoint with the existing Kubo client utilities, then
resolve only once readiness is confirmed while preserving destroyed/error
handling. Adjust the regression test to simulate delayed Kubo readiness rather
than merely stubbing _httpRouterSetupPromise.
In `@src/runtime/node/community/local-community/reprovide-on-address-change.ts`:
- Around line 41-45: Update isBrowserDialableAddr() to return true for addresses
containing /webtransport in addition to the existing /webrtc and /ws checks, and
add a regression test covering a WebTransport address.
In `@src/runtime/node/setup-kubo-http-routers.ts`:
- Around line 194-200: Update the HTTP-router setup flow around
settingOptionRetryOption, setHttpRouterOnAllNodes, and PKC.destroy() so
destruction cancels the retry operation immediately and settles
_httpRouterSetupPromise without waiting for backoff retries. Pass the destroy
abort signal to every router-setup fetch, and ensure pending or scheduled
attempts observe cancellation rather than retrying against the destroyed PKC.
In `@test/node/httprouter.test.ts`:
- Around line 9-14: Add a concise comment immediately above the
describeSkipIfRpc call explaining that RPC clients cannot read or modify the
local Kubo Routing configuration, so this suite cannot run under RPC.
🪄 Autofix
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 Plus
Run ID: 877054cc-3fa0-4bb9-a1f9-dd2adfe4634c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (16)
docs/protocol/data-path-migration.mdpackage.jsonsrc/pkc/pkc.tssrc/rpc/src/index.tssrc/rpc/test/node/rpc.auto-start-kubo-restart.test.tssrc/runtime/browser/setup-kubo-address-rewriter-and-http-router.tssrc/runtime/browser/setup-kubo-http-routers.tssrc/runtime/node/address-rewriter-db.tssrc/runtime/node/addresses-rewriter-proxy-server.tssrc/runtime/node/community/local-community/reprovide-on-address-change.tssrc/runtime/node/setup-kubo-http-routers.tstest/node/address-rewriter-logging.unit.test.tstest/node/community/reprovide-on-address-change.unit.test.tstest/node/httprouter-direct-kubo.test.tstest/node/httprouter.test.tstest/node/kubo-address-rewriter.unit.test.ts
💤 Files with no reviewable changes (5)
- src/runtime/browser/setup-kubo-address-rewriter-and-http-router.ts
- test/node/address-rewriter-logging.unit.test.ts
- test/node/kubo-address-rewriter.unit.test.ts
- src/runtime/node/addresses-rewriter-proxy-server.ts
- src/runtime/node/address-rewriter-db.ts
The `pkc.destroyed` guard only runs at the top of a retry attempt, so a destroy() landing inside a backoff window still had to wait out the remaining delay (up to the 60s maxTimeout) on the setup promise it awaits. Wire the destroy abort signal into the retry operation: stop() drops the scheduled attempt and the promise resolves immediately. The same signal now also aborts the in-flight config GET/POST and the /shutdown POST. Regression test asserts destroy() returns in under 3s while the setup loop sits in a 8s backoff against an unreachable kubo node (7s without the fix). Also states why the http router suite is skipped under RPC, per AGENTS.md.
Rinse12
changed the title
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
1 participant
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.
Closes #262.
Why now
AddressesRewriterProxyServerexisted to work around a Kubo bug: provider records PUT to a delegated HTTP router were missing the browser-dialable transports (AutoTLS/tls/ws,webrtc-direct), so a browser that found a node through a router had no address it could dial. We proxied every provider PUT and rewroteAddrs.Kubo 0.43.0 fixed it (
fix(http-routing): keep browser transports in provider records, ipfs/kubo#11394, closing the ipfs/kubo#11369 we filed). This repo already pins 0.43.0 and all production nodes run it.Verified against production before writing any code. Querying
/routing/v1/providers/<cid>for a live board on all six default routers fromsrc/schema.ts:/tls/wsTwo things that survey showed:
webrtc-directandwebtransportwith no certhash, which no browser can dial. That is the proxy's signature: it rebuilds the addr set fromkubo id+kubo swarm addrs, and self swarm addrs carry no certhash. Nodes announcing natively have complete records.peers.plebpubsub.xyzstores zero/dns*addrs while the same peer IDs have/dns4/.../tls/wseverywhere else. Stale router deploy, out of scope here, tracked in Remove the address rewriter proxy now that kubo 0.43.0 publishes browser transports to HTTP routers #262.What changed
setup-kubo-address-rewriter-and-http-router.ts→setup-kubo-http-routers.ts, returningvoid. Kubo'sRouting.Routersnow hold the router URLs verbatim. The typed config helpers, the merge logic, theProvide.DHT.SweepEnabledwrite and the capped retry are unchanged.addresses-rewriter-proxy-server.ts,address-rewriter-db.tsand its SQLite request/reprovide logging, plus the browser stub.PKC_ADDRESSES_REWRITER_START_PORT, the free-port walk, the persistedhttprouter_proxy_<url>storage mapping.tcp-port-usedmoves to devDependencies (the test server and tests still use it).pkc.tskeeps one_httpRouterSetupPromiseinstead of the setup-promise/destroy-callback pair.normalizeSelfAddrsForProvidermoves toreprovide-on-address-change.ts, its only remaining caller, with its unit tests..address-rewriter/is described as a legacy directory that can be deleted.What replaces the 2-minute
_retryFailedKeysloopNothing custom. It retried provider keys whose PUT had failed, it was not a general reprovide loop. Kubo owns provide and reprovide once it talks to the routers directly, and its reprovide cycle stays inside the routers' 24h record expiry. The browser-facing case that actually mattered is already independent of the proxy:
reprovide-on-address-change.tsre-provides the connection-critical CIDs whenever the node's browser-dialable self-addresses change.Tests
test/node/httprouter.test.tsnow asserts Kubo is configured directly against the router (no proxy endpoint), that provider records reaching the router are usable, and that the config survivespkc.destroy().test/node/httprouter-direct-kubo.test.tsgains the kubo#11369 regression test: the throwaway daemon now also listens onwebrtc-direct, and the test asserts that addr survives into the record the router actually stored, not just intoipfs id. The two disagreeing is exactly what the bug looked like. Still runs withProvide.DHT.SweepEnabledboth off and on.address-rewriter-logging.unit.test.ts(tested the proxy's SQLite logging) andkubo-address-rewriter.unit.test.ts(its cases moved with the helper).All green locally against the local test server:
httprouter,httprouter-direct-kubo,reprovide-on-address-change.unit,rpc.auto-start-kubo-restart,pkc,datapath-retention.pkc.npm run build,npx tsc --project test/tsconfig.json --noEmitandknipare clean.Out of scope
peers.plebpubsub.xyz.Provide.DHT.SweepEnabled=falseand the legacy serial provider's throughput, unchanged here.Summary by CodeRabbit
Improvements
Documentation
.address-rewriter/directories are obsolete and may be safely deleted.Bug Fixes