feat(logs): capture daemon output in a sibling process

[jdx]

The problem

The supervisor held the read end of every daemon's output pipe, which put it in the data path. Kill it and the pipe has no reader, so the daemon's next write takes SIGPIPE and the daemon usually dies with it. I confirmed this directly before writing any code — a plain echo loop was gone within seconds of its supervisor being killed:

>>> SIGKILL the supervisor
  -> daemon DIED (SIGPIPE on stdout write)

That undermines the re-adoption added in #633: a daemon that logs to stdout rarely survives long enough to be adopted. Anything it had written but the supervisor had not yet read was lost too.

The change

Capture moves to a sibling process — pitchfork log-sink, this binary re-executed — which holds the read end and writes to the log store. A supervisor crash becomes invisible to logging: the daemon keeps writing, the sink keeps recording, nothing is dropped. This is the arrangement runit uses, where runsv "creates a pipe, redirects service/run's standard output to the pipe" and starts a separate log service on the other end, staying out of the stream itself.

The supervisor retains a spare read end. That does two things: a sink that dies cannot leave the pipe readerless and kill the daemon, and its replacement can be handed the same pipe. It deliberately keeps no write end, which would stop the sink from ever seeing end of file. A sink that exits while its daemon is still monitored is replaced (mirroring runsv restarting a log service); one that exits cleanly has reached EOF and is not.

A regression this introduced, and the fix

Failed starts report what the daemon printed by querying the log store, and the in-process path guaranteed the ordering by flushing synchronously before signalling — lifecycle.rs even comments that this is "so collect_startup_logs sees the line". The sink broke that: output reached the store but not the user, losing exactly the diagnostics you need when a daemon won't start.

Restored by having the failure path wait for the sink's final write, bounded at 2s so a daemon whose descendants hold the pipe open cannot stall the caller. This was only caught by chasing the question directly — the existing suite passed 295/295 without noticing, and there's now a test for it.

Scope

Daemons using ready_output, an on_output hook, or pty = true keep the in-process path, because the supervisor itself has to read the stream to serve them, and so remain crash-vulnerable. Teaching the sink to evaluate those and report matches back over IPC is a follow-up — it is a larger change and better reviewed on its own.

Tests

Three new tests in logs.bats, each verified to fail without the corresponding behavior:

• log capture survives a supervisor crash — the daemon is a plain echo loop with no trap '' PIPE. My pre-change reproduction needed that guard to survive at all; not needing it is the clearest demonstration of the fix.
• a log sink that dies is replaced — SIGKILL the sink; a different pid takes over, the daemon is unaffected, capture resumes.
• start failure diagnostics include the daemon's output — guards the ordering above.

Also verified by hand: capture continued across a supervisor SIGKILL (+14 lines written while no supervisor existed), and a killed sink was replaced with capture resuming.

523/523 unit tests, clippy clean under -D warnings, 55/55 log suites, 56/56 basic + hooks. Full suite still running locally as I open this; CI covers it including Windows.

Builds on #660, which fixed the concurrent-open failure this design would otherwise hit with several writers on one store.

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches core supervisor lifecycle, process/pipe wiring, and shared SQLite log writes; failures could affect daemon startup, log durability, or backpressure behavior, though fallbacks and bounded waits limit blast radius.

Overview
Moves daemon stdout/stderr capture out of the supervisor into a re-execed sibling pitchfork log-sink process, so a supervisor crash no longer drops the pipe reader (avoiding SIGPIPE and lost logs). The supervisor keeps a spare read end, wires eligible daemons to the pipe before spawn, and restarts sinks that exit unexpectedly while the daemon is still monitored; ready_output, on_output, and PTY daemons still use in-process capture.

The hidden log-sink CLI reads stdin asynchronously, batches parsed lines into SQLite, and documents --log-format. SQLite open now sets an explicit busy_timeout, enables WAL with a tolerant race handler, and adds contention tests. Failed starts wait briefly for sink output to land in the store so startup diagnostics stay visible; orphan-adoption docs note logging continues across supervisor restarts.

New logs.bats coverage exercises capture across supervisor SIGKILL, sink replacement, and failure diagnostics.

^{Reviewed by Cursor Bugbot for commit dd96486. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• New Features
  • Added out-of-process log capture via a hidden log-sink command to keep daemon stdout/stderr logs recording across supervisor crashes and restarts.
  • Log capture continues automatically if the sink process restarts.
  • Added --log-format selection for captured logs (text, json, logfmt, auto).
• Bug Fixes
  • Improved SQLite concurrency handling to prevent lost log entries under contention.
  • Ensures queued logs are flushed when streams close and preserves daemon start diagnostics on failure.
• Documentation
  • Updated orphan adoption documentation to clarify stdout/stderr capture behavior after supervisor crashes.
• Tests
  • Added end-to-end log-sink coverage and additional SQLite contention tests.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a hidden log-sink CLI command that batches stdin logs into SQLite, supervisor-managed sink processes for daemon output capture, SQLite contention handling, lifecycle integration, and end-to-end crash and replacement tests.

Changes

Out-of-process log capture

Layer / File(s)	Summary
Log-sink CLI ingestion docs/cli/commands.json, pitchfork.usage.kdl, src/cli/*	Defines and routes the hidden log-sink command, which parses newline-delimited stdin, batches entries, and appends them to the log store.
Concurrent log-store writes src/log_store/sqlite.rs	Adds busy-timeout and WAL initialization handling, plus tests for contended and concurrent writers.
Sink process lifecycle src/supervisor/log_sink.rs, src/supervisor/adopt.rs, src/supervisor/mod.rs	Creates retained pipes, launches sibling sink processes, polls for output, and respawns sinks while monitoring remains valid.
Daemon wiring and validation src/supervisor/lifecycle.rs, settings.toml, test/logs.bats	Routes supported daemon output through sinks, waits for final failure output, updates adoption documentation, and tests supervisor crashes, sink replacement, and startup diagnostics.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Daemon
  participant Supervisor
  participant LogSink as pitchfork log-sink
  participant LogStore
  Daemon->>Supervisor: start with stdout/stderr pipes
  Supervisor->>LogSink: launch sibling sink process
  Daemon->>LogSink: write output through retained pipe
  LogSink->>LogStore: append parsed log batches
  Supervisor->>LogSink: respawn after sink termination

Loading

Possibly related PRs

• jdx/pitchfork#620: Changes supervisor output draining and persistence behavior on monitor exit paths.
• jdx/pitchfork#633: Updates orphan-daemon adoption and monitoring behavior used by sink capture.

Poem

I’m a rabbit with logs in a pipe,
Hopping through crashes both day and night.
Sink siblings nibble each line into store,
And spring back to life when one hops no more.
SQLite waits, then writes with delight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: moving daemon output capture to a sibling process.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

Adds out-of-process daemon log capture and supporting lifecycle behavior.

• Introduces a hidden log-sink command with bounded line processing and batched SQLite writes.
• Supervises and replaces sink processes while retaining a spare pipe reader.
• Waits briefly for startup-failure output to reach the log store.
• Improves SQLite contention handling and adds end-to-end log capture tests.

Confidence Score: 5/5

The PR appears safe to merge, with no blocking failures remaining in the follow-up scope.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/cli/log_sink.rs	Implements bounded byte-stream parsing and asynchronous batched persistence for the new log-sink process.
src/supervisor/log_sink.rs	Implements pipe ownership, sink spawning, replacement supervision, and failed-start output settling.
src/supervisor/lifecycle.rs	Wires eligible daemons to sibling log sinks and preserves startup-failure diagnostics.
src/log_store/sqlite.rs	Adds explicit lock waiting and retry-tolerant WAL initialization for concurrent log writers.
test/logs.bats	Adds coverage for supervisor-crash survival, sink replacement, and startup diagnostics.

_{Reviews (11): Last reviewed commit: "fix(logs): signal a pending sink synchro..." | Re-trigger Greptile}

[coderabbitai]

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/cli/log_sink.rs`:
- Around line 21-31: Update the LogSink log_format field’s documentation to
remove “auto” from the advertised formats, leaving only json, logfmt, and text.
Do not change parsing behavior in parse(); ensure the clap help text
consistently reflects the supported values.

In `@src/supervisor/lifecycle.rs`:
- Around line 444-472: Update the early-return handling around cmd.spawn() and
the child.id() None branch in run_once to explicitly reap any already-started
sink_child before returning. Take the sink child and asynchronously await
sink.wait(), preserving the existing error and DaemonFailed return behavior
while preventing dropped sink processes from relying on best-effort cleanup.

🪄 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: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 542fb376-9a2c-4b4e-924f-9c2e22739b03

📥 Commits

Reviewing files that changed from the base of the PR and between febcdc4 and 9dcf54e.

📒 Files selected for processing (11)

• docs/cli/commands.json
• pitchfork.usage.kdl
• settings.toml
• src/cli/log_sink.rs
• src/cli/mod.rs
• src/log_store/sqlite.rs
• src/supervisor/adopt.rs
• src/supervisor/lifecycle.rs
• src/supervisor/log_sink.rs
• src/supervisor/mod.rs
• test/logs.bats

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n '"auto"' src/log_parse.rs
ast-grep run --pattern 'fn parse($$$) { $$$ }' --lang rust src/log_parse.rs

Repository: jdx/pitchfork

Length of output: 151

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== candidate files =="
fd -a 'log_sink\.rs|log_parse\.rs|commands\.json|usage\.kdl|cli' . | sed 's#^\./##' | head -200

echo
echo "== log_sink occurrences =="
rg -n "LogSink|log_format|auto|json|logfmt|text" -S src log docs 2>/dev/null | head -300

echo
echo "== relevant files sizes =="
for f in $(fd 'log_sink\.rs|log_parse\.rs|commands\.json|usage\.kdl' .); do
  echo -n "$f: "; wc -l "$f"
done

Repository: jdx/pitchfork

Length of output: 28648

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== src/log_sink.rs =="
cat -n src/cli/log_sink.rs

echo
echo "== src/log_parse.rs outline/contents =="
wc -l src/log_parse.rs
cat -n src/log_parse.rs

echo
echo "== commands.json relevant entries =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path('docs/cli/commands.json')
data=json.loads(p.read_text())
# Locate log subcommands containing log_format/help around auto/logfmt
for item in data.get('commands', []):
    if any('log' in str(s).lower() or 'sink' in str(s).lower() for s in [item.get('name',''), item.get('path',''), item.get('id','')]):
        for sec in ['help_text','long_help','arguments','subcommands']:
            if sec in item:
                print(f'--- {sec} ---')
                print(json.dumps(item.get(sec), indent=2)[:2500])
PY

echo
echo "== usage.kdl relevant log sections =="
rg -n -C 5 "log|log_format|auto" pitchfork.usage.kdl docs/cli 2>/dev/null | head -300

echo
echo "== behavior probe of parsed branches in current code by parsing source text =="
python3 - <<'PY'
from pathlib import Path
s=Path('src/log_parse.rs').read_text()
print("contains '\"auto\"' in log_parse.rs:", '"auto"' in s)
print("contains case-insensitive match auto:", 'match' in s and 'auto' in s)
print("'json' branch present:", '\\\"json\\\"' in s or '"json"' in s)
print("'logfmt' branch present:", '\\\"logfmt\\\"' in s or '"logfmt"' in s)
print("plain fallback occurrences:")
for i,line in enumerate(s.splitlines(),1):
    if 'ParsedLog::plain' in line:
        print(f"{i}: {line.strip()}")
PY

Repository: jdx/pitchfork

Length of output: 50371

---

Remove auto from the documented log formats.

LogSink advertises auto as a supported --log-format, but src/log_parse.rs::parse() only handles "json", "logfmt", and falls every other value to plain-text. --log-format auto therefore acts like --log-format text, so either implement auto-detection or remove it from the argument/help text consistently.

🤖 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/cli/log_sink.rs` around lines 21 - 31, Update the LogSink log_format
field’s documentation to remove “auto” from the advertised formats, leaving only
json, logfmt, and text. Do not change parsing behavior in parse(); ensure the
clap help text consistently reflects the supported values.

[jdx]

I could not retrieve the body of this comment — the API returns only the analysis chain, with no claim attached, so I may be answering the wrong thing.

Taking it at the line it points to, the --log-format default of text: the supervisor always passes --log-format explicitly, resolved from the daemon's log_format or logs.log_format, so the default only applies if someone runs the hidden subcommand by hand. If the concern was something else, please restate it and I will address it.

This comment was generated by Claude Code.

[coderabbitai]

@jdx, the concern is unrelated to the text default.

src/cli/log_sink.rs advertises auto as a supported --log-format value:

json, logfmt, auto, or text

However, src/log_parse.rs::parse() has explicit parsing only for json and logfmt; all other values—including auto and text—fall back to plain-text parsing. So auto is currently not an auto-detect mode.

Please either implement auto-detection or, for the minimal fix, remove auto from the LogSink argument help and generated CLI documentation.

[cursor]

Startup wait shorter than store lock

Medium Severity

Failed starts call wait_for_output with a 400ms cap while log batches can sit behind SQLite writes that wait up to five seconds on busy_timeout. The CLI may query the store before the sink persists the daemon’s last lines, so startup failure output can be missing under writer contention.

Additional Locations (1)

• src/supervisor/log_sink.rs#L75-L116

 

^{Reviewed by Cursor Bugbot for commit 621137f. Configure here.}

[jdx]

True, and I am keeping the 400ms cap deliberately.

Matching the store's five-second busy timeout would mean a failed pitchfork start could pause five seconds before reporting — and the retry zero fails immediately with one attempt test caps a failed start at three seconds, so it is not even available. Under that much write contention no cap I can pick is reliable anyway.

What the cap protects is the common case, which it does: measured ~120ms, and a daemon printing eight lines before failing reports all eight. Under heavy contention the diagnostics degrade to partial or empty, but nothing is lost — the lines are still written and pitchfork logs <daemon> shows them. I would rather have a fast failure with occasionally incomplete inline diagnostics than a start command that can hang for five seconds.

This comment was generated by Claude Code.

[coderabbitai]

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/supervisor/log_sink.rs`:
- Line 61: Update the retry loop in the supervisor log-sink spawning flow around
SPAWN_RETRY_DELAY to retain the initial 200ms retry, then apply capped
exponential backoff after consecutive spawn failures. Reset the backoff to the
initial delay whenever a replacement sink starts successfully, and continue
honoring the existing monitor-token validity behavior.
- Around line 92-101: Update the polling logic around LogQuery and
LOG_STORE.query so it observes log growth without materializing every matching
LogEntry on each poll. Add or reuse a count/latest-row query, or track an
incremental identifier, while preserving the existing daemon and since filters
and returning the equivalent growth signal used by the poller.
- Around line 97-104: Update the log-count polling around the
tokio::task::spawn_blocking call so SQLite/query errors are propagated rather
than converted to found = 0. On read failure, reset or extend the settling timer
and continue polling until a successful stable read or the existing deadline;
retain the no-new-output behavior only for successful queries.
- Around line 87-104: Update wait_for_output’s polling loop around
LOG_STORE.query and the deadline to enforce the timeout before and during each
SQLite poll, including returning immediately when timeout is zero. Use a
deadline-aware query path or wrap the blocking operation so a slow query cannot
keep the caller waiting beyond the requested deadline, while preserving the
existing polling behavior for queries that complete in time.

🪄 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: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: d7bca03e-a2e2-489a-9e9a-a394c0292434

📥 Commits

Reviewing files that changed from the base of the PR and between 9dcf54e and 621137f.

📒 Files selected for processing (3)

• src/cli/log_sink.rs
• src/supervisor/lifecycle.rs
• src/supervisor/log_sink.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/supervisor/lifecycle.rs
• src/cli/log_sink.rs

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit a097289. Configure here.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/supervisor/log_sink.rs`:
- Around line 176-182: Replace the unconditional tokio::spawn in
PendingSink::drop with cleanup that does not depend on an active runtime: return
or retain the kill JoinHandle and reap it synchronously where safe, while
scheduling the async kill from the supervisor task and providing a synchronous
fallback when no supervisor is available. Ensure child termination is not
abandoned or panics after runtime shutdown.

🪄 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: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: b37f625b-998c-4543-974a-937e44d8a7f4

📥 Commits

Reviewing files that changed from the base of the PR and between a097289 and a55839f.

📒 Files selected for processing (3)

• src/cli/log_sink.rs
• src/supervisor/lifecycle.rs
• src/supervisor/log_sink.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/cli/log_sink.rs
• src/supervisor/lifecycle.rs