Repository navigation
[AI-78] Refuse foreground kapacitor agent start when another daemon is alive - #51
Conversation
… is alive The detached path (`-d`/`--detach`) has guarded against duplicate launches via the PID file since day one. The foreground path didn't, so a second `kapacitor agent start` (run by mistake, by an automation, by a parallel terminal) would happily spawn a fresh kapacitor-daemon process. That fresh process calls DaemonConnect with empty live_agents (orchestrator not yet wired → GetLiveAgentIds returns []), the server's Register silently replaces the active daemon's slot, ReconcileDaemon([]) flips every running hosted agent to Failed, and the displaced real daemon keeps its WebSocket open without ever noticing. Mirror the detached path's PID-file check into StartForegroundAsync, write the PID file when the foreground daemon launches (so subsequent starts in any mode see it), and clean it up on exit. IsOurDaemon's StartTime check already handles the recycled-PID case if the parent dies hard and leaves a stale file. Server-side belt: kurrent-io/Kurrent.Capacitor PR 590 also tightens the ReconcileDaemon guard to ignore empty live_agents, so the cascade becomes impossible regardless of what produced the second DaemonConnect. Either fix alone closes the bug; both together make it double-safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review Summary by QodoPrevent foreground daemon start when another daemon is alive
WalkthroughsDescription• Add PID file guard to foreground daemon start mode • Prevent duplicate daemon launches that silently displace active daemons • Mirror detached mode's existing PID check into foreground path • Update help text and README to document new behavior Diagramflowchart LR
A["kapacitor agent start"] --> B{"Check PID file<br/>in foreground mode"}
B -->|Daemon alive| C["Exit with error<br/>PID already running"]
B -->|No daemon| D["Start new daemon<br/>Write PID file"]
D --> E["Wait for exit"]
E --> F["Clean up PID file<br/>in finally block"]
C --> G["Return exit code 1"]
F --> H["Return exit code"]
File Changes1. src/kapacitor/Commands/AgentCommands.cs
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
#1: StartForegroundAsync's finally block deleted agent.pid unconditionally, which orphaned a concurrent legitimate daemon's PID file in this race: 1. Process A's daemon-A exits cleanly. Process A enters finally. 2. Process B's `kapacitor agent start` reads the PID file, sees PID-A; IsOurDaemon returns false (PID-A's process is gone), guard passes. 3. Process B spawns daemon-B, writes PID-B. 4. Process A's finally deletes the PID file — orphaning daemon-B. Now we re-read agent.pid in finally and only delete if it still matches the process we spawned. Belt-and-braces against PID race losses. #2: StatusCommand had its own PID-file parser that did ReadAllText().Trim() + int.TryParse(...) — which fails on the multi-line PID|StartTicks format AgentCommands.WritePidFile produces. Without the foreground guard this only triggered for `-d` daemons; this PR makes foreground daemons hit it too. Parse the first non-empty line instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks Qodo. Both fixed in 532000d: #1 PID file deleted incorrectly — Real race. Fixed by re-reading I'm leaving the more atomic acquisition (lock file / create-new) as a follow-up; it's a bigger refactor and the conditional-delete closes the window without changing the file-format contract. The remaining TOCTOU between guard-read and #2 Status parses PID file wrong — Fixed. I considered consolidating into a single PID-file reader (Qodo's "shared helper" suggestion) and decided against it for this PR — |
There was a problem hiding this comment.
Pull request overview
This PR closes the foreground-mode gap in the CLI that allowed launching a second kapacitor-daemon while another daemon was already running, which could disrupt hosted agents on the server. It also updates help/README documentation to reflect the new behavior and fixes PID-file parsing in kapacitor status.
Changes:
- Add a foreground
agent startguard against an already-running daemon and write/cleanup the PID file for foreground runs. - Fix
StatusCommandPID parsing to handle the newer multi-line PID file format. - Update CLI help text and README to document the refusal behavior and that
agent stopapplies to both modes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/kapacitor/Commands/StatusCommand.cs | Parse only the first PID-file line (supports PID + optional StartTicks format). |
| src/kapacitor/Commands/AgentCommands.cs | Refuse duplicate foreground starts; write PID file for foreground; best-effort cleanup on exit. |
| src/Kapacitor.Core/Resources/help-usage.txt | Update top-level usage text for new start/stop behavior. |
| src/Kapacitor.Core/Resources/help-agent.txt | Update agent subcommand help and add “Notes” about refusal behavior. |
| README.md | Document that agent stop applies to both modes and that agent start refuses duplicates. |
Comments suppressed due to low confidence (1)
src/kapacitor/Commands/StatusCommand.cs:64
- StatusCommand now supports the two-line PID file format, but it still only checks whether any process exists with that PID. Since the PID file optionally contains StartTicks to protect against PID reuse, this can incorrectly report "running" when the PID has been recycled to an unrelated process. Consider parsing the optional second line and validating Process.StartTime (or reusing the same IsOurDaemon-style logic as AgentCommands) so status output stays correct under PID recycling.
// The PID file is one or two lines: PID, optionally followed by
// process StartTicks (UTC ticks of Process.StartTime). Parse only
// the first non-empty line as the PID — naively passing the whole
// contents to int.TryParse fails on the two-line format and
// mis-reports a running daemon as "invalid PID file".
var firstLine = (await File.ReadAllTextAsync(pidPath))
.Split('\n', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)
.FirstOrDefault();
if (int.TryParse(firstLine, out var pid)) {
try {
System.Diagnostics.Process.GetProcessById(pid);
await Console.Out.WriteLineAsync($"running (PID {pid})");
} catch (ArgumentException) {
await Console.Out.WriteLineAsync("not running (stale PID file)");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Refuse to start when an existing kapacitor-daemon is alive — even in | ||
| // foreground mode. Without this guard, a second `kapacitor agent start` | ||
| // (run by mistake, by an automation, by a parallel terminal) would | ||
| // happily connect to the server and call DaemonConnect with an empty | ||
| // live_agents list, which used to mass-fail every hosted agent owned | ||
| // by the original daemon (AI-78). The detached path has had this | ||
| // guard since day one; foreground was the only hole. | ||
| if (ReadPidFile() is { } existing && IsOurDaemon(existing.Pid, existing.StartTicks)) { | ||
| await Console.Error.WriteLineAsync($"Agent daemon already running (PID {existing.Pid}). Use `kapacitor agent stop` first."); | ||
|
|
||
| return 1; | ||
| } |
Previous PID-file guard had a TOCTOU window: two concurrent `kapacitor agent start` invocations could both observe no live daemon, both spawn daemons, and only one's PID would end up in the file. The loser's daemon would then race-write DaemonConnect with empty live_agents and trip the cascade we're meant to prevent. Add agent.start.lock opened with FileShare.None (POSIX flock(LOCK_EX), Windows native sharing constraint). Foreground holds the lock for the daemon's entire lifetime; detached holds it just for the check + spawn + WritePidFile window before the parent exits. Two concurrent starts now serialize at the OS level: the second's TryAcquireStartLock returns null (IOException from FileShare.None) and it refuses with "Another `kapacitor agent start` is already in progress …" The lock is per-open-handle so even SIGKILL on the parent releases it cleanly. Refactored StartForegroundAsync into two parts: the lock-acquiring guard wrapper and the original spawn body (now SpawnForegroundAsync) to keep the lock-acquire/release scope easy to read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
You're right — TOCTOU was real, not benign. Pushed 23a9871 with an actual exclusive lock.
Two concurrent Refactored Re: CI — the test failure ( |
Summary
-d/--detach) has guarded against duplicate launches via the PID file since day one (StartDetachedline 67 —Agent daemon already running (PID …). Use \kapacitor agent stop` first.). Foreground was the only hole. A secondkapacitor agent start— run by mistake, by an automation, or by a parallel terminal — happily spawns a freshkapacitor-daemon. Cold-start fresh daemons callDaemonConnectwith emptylive_agents(the orchestrator hasn't been constructed yet,GetLiveAgentIdsis null, the?? []fallback fires), the server silently replaces the active daemon's(owner, name)slot viaDaemonRegistry.Register, thenReconcileDaemon([])` flips every running hosted agent to Failed. The displaced real daemon keeps its WebSocket open and never notices.StartForegroundAsync, write the PID file when the foreground daemon launches so subsequent starts in any mode see it, and clean it up on exit.IsOurDaemon's StartTime check already handles the recycled-PID case if the parent dies hard.help-usage.txt/help-<cmd>.txt/ README in sync.Tested locally:
Independent of the server-side belt (kurrent-io/Kurrent.Capacitor PR 590) which tightens
ReconcileDaemonto ignore emptylive_agents. Either alone closes the cascade; both together make it double-safe.See AI-78 for the full investigation, including macOS process-log evidence of 5 distinct
kapacitor-daemonPIDs today and confirmation the real daemon's HubConnection never reconnected.Test plan
kapacitor agent startwith the existing daemon alive exits 1 with the expected message.try/finallydeletes it; verified manually but worth a second look).🤖 Generated with Claude Code