Repository navigation
Conversation
loadKuboConfigFile looked in .ipfs-bitsocial-cli while kubo runs in .bitsocial-cli.ipfs or $IPFS_PATH, so the read always came back empty and the daemon fell back to env vars and defaults. Both now use resolveKuboRepoPath. That makes the multiaddr parsers reachable for the first time, so they now handle IPv6 hosts, list-valued addresses and unix sockets.
computeKuboConfig(kuboConfig, reachabilityConfig, enabled) is the one place that decides the kubo edits. planKuboConfig also returns a record of what changed, which tells our entries from identical operator ones and lets disable restore the touched fields byte for byte.
An explicit KUBO_RPC_URL or IPFS_GATEWAY_URL can name a kubo other than the one in this repo, so it still wins over the kubo config. Connectivity checks now strip the brackets a URL keeps around an IPv6 host.
reachability/config.json holds the operator's choices and refuses to load a newer version rather than drop its fields. The daemon's check results go in a separate file so they cannot overwrite a setup in progress.
…e status Caddy keeps certificates for old addresses and other CAs until they expire, so status now filters to the configured CA and current names. A damaged runtime status file reads as no checks yet, and the text output prints domain and Caddy checks.
…le tests Caddy is downloaded on first use and checked against a sha256 pinned in source. The generated Caddyfile keeps Caddy off UDP 443 and port 80. The integration test issues a certificate for 127.0.0.1 from Pebble and completes a js-libp2p handshake with kubo through Caddy.
Discovers public addresses from interfaces, kubo and the router, obtains an IP certificate, applies the kubo config and restarts kubo. It checks what kubo really listens on, because kubo stays silent when it cannot bind UDP 443.
--domain gets a normal certificate when 443 is free, or prints a proxy snippet for the detected server when it is busy. --announce is added only after a libp2p handshake through it succeeds. A name that does not resolve here is refused unless --force is passed.
The daemon spawns Caddy and keeps it up, re-derives public IPs and verifies announced names every 5 minutes, and moves the certificate and announcement on an IP change. A stale name warns hourly and is dropped after 24 hours. Without a configuration it prints one line naming the setup command.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 23 minutes. View limit details
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. |
|
Consolidated into #208. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 140081fbc0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Only addresses that hold a certificate move. A proxy-only node has nothing here to change. | ||
| if (config.publicIps.length > 0) { | ||
| const nextIps = planIpChange(config.publicIps, derived); |
There was a problem hiding this comment.
When an IP change is detected but certificate issuance temporarily fails for every new address, handleIpChange persists publicIps: []. Every subsequent check then skips planIpChange because of this guard, so the daemon never retries issuance even after the CA or network recovers, and WSS remains disabled until the operator reruns setup. The same guard also prevents a configuration with no initially issued IP from gaining one later.
Useful? React with 👍 / 👎.
| if (!fromInterfaces.some((ip) => net.isIPv4(ip))) { | ||
| this.router ??= await discoverNatRouter(); | ||
| const external = await this.router?.externalIp().catch(() => undefined); | ||
| if (external) for (const ip of publicIpsFromMultiaddrs([`/ip4/${external}`])) ips.add(ip); |
There was a problem hiding this comment.
After a host/router reboot, the saved reachability configuration can exist while its UPnP mappings no longer do. The first check discovers the router and reads its external IP but does not call mapPorts; that only happens on the independent 20-minute lease-refresh timer, leaving TCP/UDP 443 and TCP 80 unreachable for up to 20 minutes after daemon startup. Create the mappings when the router is first discovered rather than waiting for the refresh interval.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const issued = this.deps.waitForIssuance | ||
| ? await this.deps.waitForIssuance(config, nextIps) | ||
| : (await waitForCertificates(this.deps.dataPath, config.acmeDirectory, nextIps)).issued; | ||
| if (issued.length !== nextIps.length) { | ||
| this.deps.warn(`No certificate could be issued for ${nextIps.filter((ip) => !issued.includes(ip)).join(", ")} after the IP change.`); | ||
| await fs.writeFile(caddyfilePath(this.deps.dataPath), generateCaddyfile({ ...spec, ips: issued })); | ||
| await this.watchCaddy(); | ||
| } | ||
| const port = config.httpsPort ?? 443; | ||
| const ipAddress = (ip: string) => `/${net.isIPv6(ip) ? "ip6" : "ip4"}/${ip}/tcp/${port}/tls/ws`; | ||
| const oldIpAddresses = config.publicIps.map(ipAddress); | ||
| const announce = [...issued.map(ipAddress), ...config.announce.filter((address) => !oldIpAddresses.includes(address))]; | ||
| this.deps.log(`Public address changed from ${config.publicIps.join(", ") || "none"} to ${issued.join(", ") || "none"}. Announcing: ${announce.join(", ") || "nothing"}`); | ||
| return this.saveAndApply({ ...config, publicIps: issued, announce }); |
There was a problem hiding this comment.
Bug: one failed certificate wait after an IP change disables IP reachability permanently.
If waitForCertificates times out (ACME rate limit/outage, router not yet forwarding 80/443), issued is [], so the config is saved with publicIps: [], the IP announce addresses are removed, and watchCaddy stops Caddy. runChecksOnce only plans IP changes when config.publicIps.length > 0 (line 373), so it never retries, even after the problem clears. An IP-only node stays unreachable over WSS until the operator manually reruns bitsocial reachability setup.
Suggested fix: on total failure keep the old publicIps/announce (or record the target IPs) so the next check retries.
bitsocial-cli/src/reachability/runtime.ts
Lines 288 to 303 in 140081f
| if (await this.caddy.isUp()) { | ||
| this.caddyRunning = true; | ||
| this.loadedCaddyfile = undefined; |
There was a problem hiding this comment.
Bug: an adopted Caddy is never watched, so it is never restarted if it dies.
When isUp() is true the code sets caddyRunning = true but keeps no child and registers no exit hook; only the daemon-started path (line ~229) resets the flag. Scenario: the operator reruns bitsocial reachability setup while the daemon's Caddy is failing; the daemon adopts setup's temporary Caddy within 5 s, setup then calls stopCaddy, and the daemon still believes Caddy is running. WSS stays down until the daemon restarts (same for a leftover Caddy from a killed daemon that later dies).
Suggested fix: while Caddy is adopted (no managed child), re-check isUp() each tick and clear caddyRunning when it stops answering.
bitsocial-cli/src/reachability/runtime.ts
Lines 223 to 227 in 140081f
| if (verdict.restore) { | ||
| const stillDropped = (next.droppedAnnounce ?? []).filter((address) => address !== entry.address); | ||
| next = { ...next, announce: [...next.announce, entry.address], ...(stillDropped.length > 0 ? { droppedAnnounce: stillDropped } : {}) }; | ||
| if (stillDropped.length === 0) delete next.droppedAnnounce; |
There was a problem hiding this comment.
Bug: restoring a name can append a duplicate announce address.
judgeName decides restore from status.domains[name].droppedFromAnnounce (runtime-status.json), not from config.droppedAnnounce. After a name is dropped, the warning tells the operator to fix DNS and rerun bitsocial reachability setup; setup rebuilds announce including the domain and neither clears droppedAnnounce nor the status flag. On the next check the name is healthy, restore is true, and announce: [...next.announce, entry.address] adds the address a second time (persisted, shown in status, and a misleading "announced again" log).
Suggested fix: dedupe on restore (e.g. [...new Set([...next.announce, entry.address])]) or decide restore from config.droppedAnnounce.includes(address).
bitsocial-cli/src/reachability/runtime.ts
Lines 327 to 332 in 140081f
| private async handleIpChange(config: ReachabilityConfig, nextIps: string[]): Promise<ReachabilityConfig> { | ||
| // Moves only IP-certificate addresses. A domain's or proxy's address does not contain our IP. | ||
| const spec = { | ||
| domains: config.domain && config.announce.some((address) => address.includes(`/${config.domain}/`)) ? [config.domain] : [], |
There was a problem hiding this comment.
Bug: a dropped domain loses its Caddy site on IP change and is not re-added when restored.
spec.domains only includes the domain if it is currently in config.announce. If the domain was dropped (it's in droppedAnnounce) and the IP then changes, the regenerated Caddyfile has no site for it. When DNS recovers, checkNames puts the address back into announce (line ~330) but nothing regenerates the Caddyfile, so the node announces /dns4/<domain>/tcp/443/tls/ws while Caddy has no site/certificate for it and browser TLS handshakes fail (and renewals stop).
Suggested fix: also count config.droppedAnnounce when building domains, or regenerate the Caddyfile on restore.
bitsocial-cli/src/reachability/runtime.ts
Lines 275 to 277 in 140081f
| await this.watchCaddy(); | ||
| this.timers.push(setInterval(() => void this.watchCaddy().catch((error) => this.deps.warn(String(error))), CADDY_WATCH_INTERVAL_MS)); | ||
| this.timers.push(setInterval(() => void this.runChecks().catch((error) => this.deps.warn(String(error))), CHECK_INTERVAL_MS)); | ||
| this.timers.push(setInterval(() => void this.refreshLeases().catch((error) => this.deps.warn(String(error))), LEASE_REFRESH_INTERVAL_MS)); |
There was a problem hiding this comment.
Bug: a single failure of the first watchCaddy() leaves the runtime dead for the daemon's lifetime.
start() awaits watchCaddy() before creating any timer. watchCaddy can throw (ensureCaddyBinary download fails when offline at boot, or reloadCaddy fails on an adopted Caddy). Then start() rejects, daemon.ts only logs "Reachability failed to start", and the Caddy watch, IP/name checks and lease refresh never run until the daemon is restarted.
Suggested fix: wrap the first call like the interval calls (await this.watchCaddy().catch(warn)) or create the timers first.
bitsocial-cli/src/reachability/runtime.ts
Lines 138 to 143 in 140081f
Part of #198 (step 7). Based on #206, so earlier commits show here until those merge.
Summary
flowchart TD start[daemon start] --> cfg{reachability/config.json} cfg -- missing --> line[one line naming the setup command] cfg -- disabled --> quiet[nothing] cfg -- enabled --> run[log addresses and expiry, keep Caddy up] run -- every 5 s --> watch[start, adopt, reload or stop Caddy to match the stored state] run -- every 5 min --> checks[re-derive IPs, verify names, check expiry] checks -- IP changed on 2 readings --> move[Caddyfile, reload, wait for certificate, AppendAnnounce, restart kubo] checks -- name failing --> warn[warn hourly; drop at 24 h; restore on recovery] run -- every 20 min --> lease[refresh router mappings]Choices the issue left open, each with a test:
--forced and--announced names only have to resolve. A proxy answers for them by design, so they would otherwise be wrongly dropped after 24 hours. This addsdomainForcedto the config.droppedAnnounceto the config.runtime-status.json, so a daemon restart doesn't reset the 24 hours.A failure anywhere in this runtime logs a warning and never stops the daemon.
Found by the end-to-end test: the daemon sends stderr only to its log file, so
console.warnwould never have reached an operator's terminal. These warnings now go to stdout.Evidence
setupleft a config that nothing served.After:
npm run test:cli: 62 files, 715 passed, 1 skipped.The IP-change path is tested with fakes for the router and Caddy. No real router or address change has exercised it, and the UPnP refresh is untested against real hardware.
Merge Danger
Door: two-way
Blast Radius: daemon-startup
Every daemon now loads this module at start and may print one extra line. It stays off for a daemon using an external kubo through
--pkcOptions.kuboRpcClientsOptions. An enabled node gains a Caddy child process, and on an IP change the daemon restarts kubo.