Skip to content

fix(runtime): refuse to boot a second khived instance - #2281

Merged
oceanwaves630 merged 3 commits into
mainfrom
fix/duplicate-daemon-refusal
Sep 11, 2026
Merged

fix(runtime): refuse to boot a second khived instance#2281
oceanwaves630 merged 3 commits into
mainfrom
fix/duplicate-daemon-refusal

Conversation

@oceanwaves630

Copy link
Copy Markdown
Collaborator

What

Startup's duplicate-daemon check treated any accepting Unix socket as an incumbent. A live
PID plus an accepting socket is not proof of khived — any unrelated process that has bound
the same path also answers connect(). Treating that as an incumbent refuses a legitimate
boot; treating it as stale removes a socket a real instance is serving.

Startup now probes the listener before deciding: a bounded probe_only frame carrying this
process's own config_id, requiring the exact probe-branch response shape back.

Why the whole shape is checked, not ok=true

The metrics branch is otherwise identical to the probe-ack branch — same ok, same
absent result and error, same protocol version, same served_config_id shape — and is
distinguished only by carrying metrics: Some(...). Accepting a metrics reply as an
identity ack would let any responder echoing those fields impersonate an incumbent. So the
check requires ok=true, result=None, error=None, metrics=None, request_id=None
(this probe frame never sets one), no mismatch flags, matching protocol version, and a
matching served_config_id shape.

A frame that parses but reports version_mismatch or config_mismatch still proves the
peer speaks this protocol, so it counts as an incumbent rather than falling through to
stale-socket recovery. A connect that succeeds but never answers, times out, or replies
with non-protocol bytes is not khived and takes the recovery path as before.

The probe is bounded at 500ms — short enough that a hung or foreign listener cannot stall
startup, long enough for a live instance under normal load to answer.

Relationship to #2230

This is the daemon half of #2230, cut onto a fresh branch from current main. #2230 mixed
this with an unrelated stdio-bridge idle-timeout change across nine commits and went
conflicted; the two halves are independent (khive-runtime sits below khive-mcp in the
dependency order, so nothing here depends on the bridge work). The bridge half is not
included and does not block this.

Worth noting for anyone reconstructing the split: the commit subjects do not partition the
work. fix(mcp): scope the idle timeout to genuinely idle sessions and probe daemon identity is labelled as an mcp-only change but contributes 113 lines to
crates/khive-runtime/src/daemon.rs — in fact the probe function above. Splitting on the
subject lines would have dropped it.

Verification

Run against this branch, from a clean target directory:

  • cargo fmt --all -- --check — clean
  • cargo clippy -p khive-runtime --all-targets -- -D warnings — clean
  • cargo test -p khive-runtime — 1535 passed, 0 failed, 8 ignored across all targets,
    including the new second_daemon_boot_refuses_loudly_while_first_is_live

The new integration test needs the fault-injection feature: tests under tests/ link the
lib as an external crate, so items gated any(test, feature = "fault-injection") are
otherwise unreachable. Hence the dev self-dependency in Cargo.toml.

One doc comment cites crates/khive-mcp/src/daemon.rs::probe_daemon_identity; that function
is pre-existing on main, so the reference resolves without the bridge half.

A live PID plus an accepting Unix socket is not proof of khived: any unrelated
process that happens to have bound the same path also answers `connect()`.
Treating an accepting socket as an incumbent refuses a legitimate boot; treating
it as stale removes a socket a real instance is serving.

Startup now probes the listener before deciding. It sends a bounded `probe_only`
frame carrying this process's own `config_id` and requires the exact probe-branch
response shape back — `ok=true`, `result=None`, `error=None`, `metrics=None`,
`request_id=None`, no mismatch flags, matching protocol version, and a matching
`served_config_id` shape. A peer that answers that way is a real khived instance
and must not be treated as stale; one that connects but never answers, times out,
or replies with non-protocol bytes falls through to the existing stale-socket
recovery path.

The response shape is checked in full rather than by `ok=true` alone. The
metrics branch is otherwise identical to the probe-ack branch — same `ok`, same
absent result and error, same protocol version, same `served_config_id` shape —
and is distinguished only by carrying `metrics: Some(...)`. Accepting a metrics
reply as an identity ack would let any responder that echoes those fields
impersonate an incumbent, so `metrics` and `request_id` must both be absent.

A frame that parses but reports `version_mismatch` or `config_mismatch` still
proves the peer speaks this protocol, so it counts as an incumbent. The probe is
bounded at 500ms: short enough that a hung or foreign listener cannot stall
startup, long enough for a live instance under normal load to answer.

Adds an integration test asserting a second boot refuses loudly while the first
is live. It needs the `fault-injection` feature, which tests under `tests/` link
as an external crate, hence the dev self-dependency.

Co-Authored-By: leo <noreply@khive.ai>
@ohdearquant

Copy link
Copy Markdown
Owner

Moving this back to draft: the incumbent-detection path can delete the
rendezvous of a daemon that is actively serving, which is the opposite of what
this change is for.

cleanup_stale_daemon returns Some(pid) only when the PID parses, the PID is
eligible and running, the socket exists, and
socket_speaks_khived_protocol returns true. Any one of those being false falls
through to an unconditional remove_file of both the socket and the PID file,
after which the caller binds a fresh listener
(crates/khive-runtime/src/daemon.rs:2081-2102).

socket_speaks_khived_protocol returns false for connect, write and read
errors, parse failure, the 500 ms timeout, and each of version_mismatch,
namespace_mismatch and config_mismatch
(crates/khive-runtime/src/daemon.rs:2040-2064). So a live daemon serving a
different config_id answers the probe correctly, is classified false, and has
its socket and PID file removed and taken over. A healthy daemon that simply
answers slower than 500 ms under load gets the same treatment.

ADR-049 Amendment 6 allows cleanup-and-bind only in states 10 and 11; states
2-9 and 13-15 must refuse and must not unlink
(docs/adr/ADR-049-khived-daemon.md:554-574). A boolean cannot carry that
distinction — it collapses "live but not mine", "uncertain", and "genuinely
stale" into one destructive branch. Returning a closed classification instead,
and reaching the unlink path only from an established state 10/11, would keep
the refusal states non-destructive.

Two smaller items in the same area:

  1. The exact-incumbent refusal ends in anyhow::bail!
    (crates/khive-runtime/src/daemon.rs:1639-1660), so startup exits nonzero,
    while Amendment 6 state 1 specifies refusing with exit 0
    (docs/adr/ADR-049-khived-daemon.md:556). This is load-bearing for process
    supervision: a supervisor configured to restart only on unclean exit will
    restart on what the contract defines as a deliberate terminal stop. The new
    integration test currently codifies the nonzero form
    (tests/duplicate_daemon_refusal.rs:74-87).
  2. With a live PID and no socket, the same catch-all removes the PID file and
    binds, which collapses state 12 into state 11 even though production passes
    allow_same_process_incumbent = false.

The test also does not fail if the new predicate alone is reverted: with "any
accepting socket is an incumbent", the real daemon still accepts and the
pre-existing refusal branch still returns Err, so no assertion fires. Coverage
for the mismatch, malformed-reply, silent-listener, slow-reply and
live-PID-no-socket cases would pin the behaviour that actually matters here.

The Cargo.toml dev self-dependency for the fault-injection feature is the
conventional shape for exposing a test-only library view to an integration test
target and has no effect on production builds; no issue there.

@ohdearquant
ohdearquant marked this pull request as draft August 30, 2026 00:46
@ohdearquant
ohdearquant marked this pull request as ready for review September 1, 2026 16:40

@ohdearquant ohdearquant left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head 233b8d5: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

Both sides changed what a live PID file owner means. The branch refuses loudly
with the pid named; main declines to remove an ambiguous live owner because a
draining incumbent closes its listener before it releases writers. Neither alone
is right, so the check now has three outcomes: a live process that answers the
khived protocol, a live process that answers nothing, and stale. The first two
both refuse to start and differ only in the message; only stale proceeds.
@ohdearquant

Copy link
Copy Markdown
Owner

Rebased onto current main by merge. The two sides disagreed about what a live
PID file owner means, so this needed a decision rather than a mechanical resolve.

This branch refuses loudly when another instance owns the store: non-zero exit
with the pid named, because a silent Ok(()) is what let two detached daemons
coexist with neither side noticing. It reached that conclusion by probing the
socket, and treated a live pid whose socket did not answer as stale, removing
the socket and the PID file.

main meanwhile added the opposite guard: a draining incumbent closes its
listener before it releases writers, so an unanswered socket is ambiguous, and
deleting that PID file is precisely how a second daemon gets started on one
store.

Both are right about their own case. The check now has three outcomes:

  • a live process that answered the khived protocol on the socket,
  • a live process that owns the PID file and answered nothing,
  • stale, where nothing live owns the store.

The first two both refuse to start and differ only in the message; the second
names the draining possibility and states that nothing was removed. Only the
third proceeds. The protocol probe is still what distinguishes the two refusals,
so its own tests are unchanged, and main's assertion that a live incumbent
retains ownership holds for both socket-present and socket-absent arms.

Verified on the pinned 1.95.0 toolchain: cargo fmt --all -- --check rc 0,
cargo clippy --workspace --all-targets -- -D warnings rc 0,
cargo test --workspace --no-run rc 0, cargo test -p khive-runtime daemon
75 passed 0 failed, and the duplicate_daemon_refusal integration test passes.

The new enum landed between the cfg attribute and the function, so the
attribute gated the enum and the function lost it, and the Windows compile
reached callees that do not exist there. Also restore the self-dependency
row the lockfile needs under --locked.
@oceanwaves630
oceanwaves630 merged commit b452f1e into main Sep 11, 2026
30 checks passed
@oceanwaves630
oceanwaves630 deleted the fix/duplicate-daemon-refusal branch September 11, 2026 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants