fix(daemon-state): detect PID reuse so stale state files aren't treated as live daemons - #67
Conversation
…ed as live daemons (#66) A bare process.kill(pid, 0) liveness check let a stale state file — written by a daemon inside a Docker container (PID 8 in its namespace) into the bind-mounted data dir — match a kernel thread on the host. `update install` then SIGINT'd the unrelated process and restarted the daemon twice on the same port. State files now record the OS-reported process start time (procStartTime, /proc/<pid>/stat field 22, `ps -o lstart=` fallback); aliveness checks compare it, so a reused PID is detected and pruned. Legacy state files without the field fall back to requiring that the process command line references bitsocial — kernel threads have an empty cmdline and are correctly pruned.
📝 WalkthroughWalkthroughDaemon state files now store and verify OS process identity ( ChangesProcess Identity Verification for Daemon State
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/common-utils/daemon-state.tsOops! Something went wrong! :( ESLint: 8.27.0 Error: ESLint configuration in --config » eslint-config-oclif is invalid:
Referenced from: /.eslintrc test/common-utils/daemon-state.test.tsOops! Something went wrong! :( ESLint: 8.27.0 Error: ESLint configuration in --config » eslint-config-oclif is invalid:
Referenced from: /.eslintrc 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/common-utils/daemon-state.ts`:
- Around line 54-55: The current fallback uses `const { stdout } = await
execFileAsync("ps", ["-p", String(pid), "-o", "args="]); return stdout.trim() ||
undefined;` which turns an empty `ps` args result into `undefined`; change the
return to preserve an empty string (so the caller can distinguish empty args
from missing info) by returning the raw/trimmed-end stdout (e.g. `return
stdout.trimEnd();` or `return stdout.replace(/\r?\n$/, '');`) instead of `||
undefined`. Update the return expression at the `execFileAsync` call in
daemon-state.ts so empty `args=` remains `""`.
In `@test/common-utils/daemon-state.test.ts`:
- Around line 7-8: The tests are touching the real defaults.PKC_DATA_PATH
causing potential data loss; change daemon-state.ts to accept an injected data
directory (e.g., add an optional parameter or constructor arg like dataPath or
allow reading from an overridable env var) and use that path instead of directly
importing defaults.PKC_DATA_PATH inside getAliveDaemonStates() and related
functions; then update daemon-state.test.ts to create a temporary directory
(fs.mkdtemp/tmpdir) and pass that temp path into the daemon-state API (or set
the env var or mock defaults.PKC_DATA_PATH) before writing test files and
calling getAliveDaemonStates() so tests operate only on the isolated temp
directory.
🪄 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: aac5935d-afb2-4729-a4fd-95779d55f9ef
📒 Files selected for processing (2)
src/common-utils/daemon-state.tstest/common-utils/daemon-state.test.ts
| const { stdout } = await execFileAsync("ps", ["-p", String(pid), "-o", "args="]); | ||
| return stdout.trim() || undefined; |
There was a problem hiding this comment.
Preserve empty ps command lines in the fallback path.
This collapses an empty args= result into undefined, so Line 133 falls back to liveness-only instead of treating the PID as a non-daemon match. On systems where /proc is unavailable, that reopens the stale-state false positive this PR is trying to eliminate.
Suggested fix
try {
const { stdout } = await execFileAsync("ps", ["-p", String(pid), "-o", "args="]);
- return stdout.trim() || undefined;
+ return stdout.trimEnd();
} catch {
return undefined;
}🤖 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/common-utils/daemon-state.ts` around lines 54 - 55, The current fallback
uses `const { stdout } = await execFileAsync("ps", ["-p", String(pid), "-o",
"args="]); return stdout.trim() || undefined;` which turns an empty `ps` args
result into `undefined`; change the return to preserve an empty string (so the
caller can distinguish empty args from missing info) by returning the
raw/trimmed-end stdout (e.g. `return stdout.trimEnd();` or `return
stdout.replace(/\r?\n$/, '');`) instead of `|| undefined`. Update the return
expression at the `execFileAsync` call in daemon-state.ts so empty `args=`
remains `""`.
| import defaults from "../../dist/common-utils/defaults.js"; | ||
|
|
There was a problem hiding this comment.
Don't run these regressions against the real daemon-state directory.
These cases write raw files under defaults.PKC_DATA_PATH and then call getAliveDaemonStates(), which deletes stale entries from that same directory. If a developer has real daemon state on disk, this test can prune unrelated files or interfere with a live local daemon. Please redirect daemon-state.ts to an injected temp directory under test instead of touching the default path.
Also applies to: 126-189
🤖 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/common-utils/daemon-state.test.ts` around lines 7 - 8, The tests are
touching the real defaults.PKC_DATA_PATH causing potential data loss; change
daemon-state.ts to accept an injected data directory (e.g., add an optional
parameter or constructor arg like dataPath or allow reading from an overridable
env var) and use that path instead of directly importing defaults.PKC_DATA_PATH
inside getAliveDaemonStates() and related functions; then update
daemon-state.test.ts to create a temporary directory (fs.mkdtemp/tmpdir) and
pass that temp path into the daemon-state API (or set the env var or mock
defaults.PKC_DATA_PATH) before writing test files and calling
getAliveDaemonStates() so tests operate only on the isolated temp directory.
Closes #66
Bug
isPidAlive()insrc/common-utils/daemon-state.tsonly checkedprocess.kill(pid, 0). A PID being alive does not prove the process is the bitsocial daemon that wrote the state file (classic stale-pidfile / PID-reuse hazard).Observed in the wild: a daemon running inside a Docker container (PID 8 in the container's PID namespace) wrote its state file into the bind-mounted data dir. The container died without graceful shutdown; on the host, PID 8 is a kernel thread — alive but unrelated.
bitsocial update installthen:waitUntilUsedsaw the first daemon's portFix
Two layers in
daemon-state.ts:procStartTime:/proc/<pid>/statfield 22 on Linux,ps -o lstart=fallback elsewhere). Aliveness checks compare it — a reused PID has a different start time, so the state is pruned.procStartTime) fall back to requiring the process command line to referencebitsocial. Kernel threads have an empty cmdline, so the original failure case is correctly pruned. If identity can't be determined at all, behavior falls back to the old liveness-only check (fail-safe: never treats a real running daemon as dead).Tests
The bug was reproduced in a regression test before fixing (failed on unfixed code):
sleepstanding in for the kernel thread)procStartTimedoesn't match the process now under that PIDSpawn-based tests
await once(child, "spawn")and use a compound bash command to avoid the fork/exec race and bash's exec-optimization (caught as a flake in a full-suite run).Full suite: 244 passed, 1 skipped.
Summary by CodeRabbit