Skip to content

agent: kernel-locked manifest writes off the runtime, LISTEN_PID required, one frame deadline - #139

Merged
jaredLunde merged 1 commit into
mainfrom
jared/mcp-skills-hardening
Oct 7, 2026
Merged

agent: kernel-locked manifest writes off the runtime, LISTEN_PID required, one frame deadline#139
jaredLunde merged 1 commit into
mainfrom
jared/mcp-skills-hardening

Conversation

@jaredLunde

@jaredLunde jaredLunde commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What

This PR fixes the three low-severity findings from the re-audit of #135. Each fix has a test that fails without it. I reverted each fix in a scripted mutation run, and all 5 reverts were caught.

# Finding Fix Proving test (fails without the fix)
1a The manifest cache's write lock was a create-new lockfile, broken once it was older than 10s. Two waiters could both judge it stale, both remove and recreate it, and both hold it. The lock is now the repo's sturdier lock, session_store::acquire_session_lock, on <manifest>.lock. It is a kernel file lock that dies with its holder, so a crashed writer's leftover file is simply unlocked and there is no staleness to judge. mcp_manifest::tests::a_dead_writers_leftover_lock_admits_exactly_one_holder_at_a_time: 16 waiters race for a lock file with an old timestamp, 20 rounds, and at most one may hold it at a time. Restoring the old age-based break fails it.
1b The lock wait slept on the calling thread for up to 5s, and update_store was called from connection tasks on tokio workers. store, forget and store_skills are now async and run the locked read-modify-write with spawn_blocking. Callers in mcp.rs and mcp_skills.rs await them. mcp_manifest::tests::a_write_waiting_for_the_lock_does_not_block_the_runtime: on a single-threaded runtime, a write that waits 400ms for the lock still lets a 10ms ticker run. Calling the write directly on the runtime fails it.
2 A comment said the LISTEN_PID == getpid() guard was enforced, but listenfd skips the check when LISTEN_PID is unset. Production now requires it. serve_ws::check_listen_pid refuses at startup, with a clear error, when LISTEN_PID is unset or names another process. Every real activator (systemd, systemd-socket-activate) sets it, so an unset one means the variables did not come from activation. The docs now match. The test harness's HeldPort keeps working through a test-only mechanism outside the binary: it starts serve the way an activator does, through sh -c 'LISTEN_PID=$$; export LISTEN_PID; exec "$0" "$@"' (exec keeps the pid), so the binary has no test-only path. serve_systemd::serve_refuses_passed_sockets_without_its_own_listen_pid (unset, and a foreign pid: serve exits naming LISTEN_PID; removing the check makes it adopt and serve). serve_systemd::serve_adopts_a_passed_socket_whose_listen_pid_is_its_own and the unit test passed_sockets_are_taken_only_when_listen_pid_names_this_process. The existing serve_adopts_a_systemd_activated_socket (real systemd-socket-activate, present on this host) still passes, as do all the MCP Events suites that use HeldPort.
3 The frame-read deadline existed only in serve_state_reporting. One shared helper. common::FRAME_DEADLINE (60s) is used by every serve reader: common::serve_frames for stdout (240 readers across the 25 serve_* suites that read stdout), common::ws_next_frame for WebSockets (so every WS suite is covered), and skills_env::Serve, which is now built on the same Frames reader instead of its own copy of the logic. A stalled run fails with a message saying why, instead of waiting for nextest's kill. serve_harness_deadlines: a silent stdout and a silent WebSocket each fail at their deadline (removing the WS timeout makes the read wait 30s), and every_serve_suite_reads_frames_with_the_shared_deadline fails if any serve_* suite or skills_env reads stdout through a bare BufReader.

Proof (local, 6a9898f)

  • Every beyond-ai-agent test except exec_endpoint_live (lib, all mcp_*, all serve_*): 2512/2512 passed.
  • Lib suite: 1537/1537, with TMPDIR inside the worktree and with the system TMPDIR.
  • clippy (-D warnings, agent + test-support + fleet-sim, all targets, code-mode), cargo fmt --check and dprint check: clean.

CI: green on the first attempt: every job passed (run 37574497085), including agent-serve, agent-rest, agent-lib, agent-code-mode and the changed-lines mutants.

Notes

  • <manifest>.lock now stays on disk between writes (a kernel lock needs a file to lock; removing it would reopen the race this fixes). It is empty.
  • serve launched with LISTEN_FDS but without its own LISTEN_PID used to adopt whatever sat at fd 3. It now refuses to start. Only an environment that was never real socket activation is affected.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk

@jaredLunde @claude
agent: kernel-locked manifest writes off the runtime, LISTEN_PID requ…
6a9898f 
…ired, one frame deadline

Re-audit of #135:
- The manifest cache's write lock was a create-new lockfile broken once
  older than 10 s: two waiters could both judge it stale, remove and
  recreate it, and both hold it. It is now the repo's session lock
  (`session_store::acquire_session_lock`) on `<manifest>.lock`, a kernel
  file lock that dies with its holder, so there is nothing to judge
  stale. The lock wait blocks, so `store`/`forget`/`store_skills` are
  async and run it with `spawn_blocking`; they were called from
  connection tasks on tokio workers.
- `serve` adopted socket-activated descriptors with `LISTEN_PID` unset
  (`listenfd` only checks it when present), and the comment claimed the
  guard was enforced. Production now requires `LISTEN_PID` to name this
  process and refuses otherwise at startup. The test harness's
  `HeldPort` starts `serve` the way an activator does (a shell exports
  its own pid and execs in place), so the binary has no test-only path.
- One frame deadline (`FRAME_DEADLINE`, 60 s) for every `serve` reader:
  `serve_frames` for stdout in all serve_* suites, `ws_next_frame` for
  WebSockets, and `skills_env::Serve`, which is now built on it. A
  stalled run fails promptly instead of at the runner's kill;
  `serve_harness_deadlines` holds every serve_* suite to it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk
@jaredLunde
jaredLunde merged commit 73d5e2e into main Oct 7, 2026
21 checks passed
@jaredLunde
jaredLunde deleted the jared/mcp-skills-hardening branch October 7, 2026 05:20
@jaredLunde jaredLunde mentioned this pull request Oct 7, 2026
Merged
jaredLunde added a commit that referenced this pull request Oct 7, 2026
@jaredLunde @claude
agent: file_lock module, unreachable deadline-free readers, LISTEN_PI…
502bce7 
…D operator note

Audit of #139:
- Operators: ARCHITECTURE.md's transport section says what changed for
  launchers that pass sockets without LISTEN_PID (`systemfd --no-pid`,
  forking wrappers), the error they now see, and the fix (`exec` the
  daemon, or set LISTEN_PID in the child just before it runs).
- The frame-deadline lint matched `BufReader::new(` and `stdout` on one
  line. Now every child-stdout read in the serve_* and mcp_* suites and
  tests/common goes through `common::child_frames`, the events fixture
  included, and the lint fails on any other `ChildStdout` or
  `.stdout.take()`/`as_mut`/`as_ref` (comments dropped, whitespace
  ignored, so formatting cannot hide one). It found one more raw read.
- `update_store_off_runtime` logs a JoinError (a panicking manifest
  write) at warn instead of discarding it.
- The kernel lock and same-process path registry are their own module,
  `file_lock`; the session lock and the manifest lock both use it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk
@jaredLunde jaredLunde mentioned this pull request Oct 7, 2026
Merged
jaredLunde added a commit that referenced this pull request Oct 7, 2026
@jaredLunde @claude
agent: file_lock module, unreachable deadline-free readers, LISTEN_PI…
72213ed 
…D operator note

Audit of #139:
- Operators: ARCHITECTURE.md's transport section says what changed for
  launchers that pass sockets without LISTEN_PID (`systemfd --no-pid`,
  forking wrappers), the error they now see, and the fix (`exec` the
  daemon, or set LISTEN_PID in the child just before it runs).
- The frame-deadline lint matched `BufReader::new(` and `stdout` on one
  line. Now every child-stdout read in the serve_* and mcp_* suites and
  tests/common goes through `common::child_frames`, the events fixture
  included, and the lint fails on any other `ChildStdout` or
  `.stdout.take()`/`as_mut`/`as_ref` (comments dropped, whitespace
  ignored, so formatting cannot hide one). It found one more raw read.
- `update_store_off_runtime` logs a JoinError (a panicking manifest
  write) at warn instead of discarding it.
- The kernel lock and same-process path registry are their own module,
  `file_lock`; the session lock and the manifest lock both use it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk
jaredLunde added a commit that referenced this pull request Oct 7, 2026
@jaredLunde @claude
agent: file_lock module, unreachable deadline-free readers, LISTEN_PI…
57c8eda 
…D operator note (#141)

* agent: file_lock module, unreachable deadline-free readers, LISTEN_PID operator note

Audit of #139:
- Operators: ARCHITECTURE.md's transport section says what changed for
  launchers that pass sockets without LISTEN_PID (`systemfd --no-pid`,
  forking wrappers), the error they now see, and the fix (`exec` the
  daemon, or set LISTEN_PID in the child just before it runs).
- The frame-deadline lint matched `BufReader::new(` and `stdout` on one
  line. Now every child-stdout read in the serve_* and mcp_* suites and
  tests/common goes through `common::child_frames`, the events fixture
  included, and the lint fails on any other `ChildStdout` or
  `.stdout.take()`/`as_mut`/`as_ref` (comments dropped, whitespace
  ignored, so formatting cannot hide one). It found one more raw read.
- `update_store_off_runtime` logs a JoinError (a panicking manifest
  write) at warn instead of discarding it.
- The kernel lock and same-process path registry are their own module,
  `file_lock`; the session lock and the manifest lock both use it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk

* agent tests: the guard holds the stdout pipe; lint every route to it; test the inode recheck

Audit of 502bce7:
- The lint was bypassable (`std::mem::take(&mut child.stdout)` survived,
  as would `Option::take`, or destructuring a raw `Child`). `ChildGuard`
  now takes the pipe off the `Child` at spawn, so `child.stdout` is
  `None` however it is reached; `child_frames` takes it from the guard,
  and `raw_stdout` exists only for tests about the pipe itself. The lint
  is a function over source text with tests per bypass form: the
  `ChildStdout` type, `Option` methods on the field, assignment,
  `take`/`replace`/`swap(&mut ….stdout)`, `Child { .., stdout, .. }`
  patterns, unguarded `.spawn()`, and `raw_stdout`; it leaves an
  `Output`'s bytes and harness fields named `stdout` alone. The suites
  outside its scope (smoke, settings_store, run_cli_flags) read through
  `child_frames` too; run_stdout_robustness uses `raw_stdout` for its
  EPIPE tests.
- `file_lock`'s inode recheck is tested through `try_lock`: a test seam
  replaces the file between open and lock, and `try_lock` must go round
  again and return the lock on the file now at the path.
- The lint scans directory modules (`tests/mcp_tasks_env/mod.rs`).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jaredLunde