Skip to content

agent: OAuth fixture coverage (DELETE, streamed in-POST question); session lock is a record lock (serve_ws flake root cause) - #144

Merged
jaredLunde merged 1 commit into
mainfrom
jared/mcp-oauth-fixture-coverage
Oct 7, 2026
Merged

jaredLunde merged 1 commit into
mainfrom
jared/mcp-oauth-fixture-coverage

Conversation

@jaredLunde

@jaredLunde jaredLunde commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

1. DELETE is asserted

The OAuth fixture now records each DELETE's Authorization header and Mcp-Session-Id.

mcp_oauth_routing::closing_an_oauth_servers_connection_ends_its_session_with_an_authenticated_delete: under serve with BEYOND_AI_AGENT_MCP_IDLE_SECS=1, the idle reaper closes the connection after the call. The test checks for exactly one DELETE carrying Bearer access-token-0 and session-0, which also shows the DELETE goes through the OAuth layer.

Found: a process that just exits (agent run) never closes its HTTP connections, so it sends no DELETE. Only a closed connection does (the idle reaper, a registry rebuild). The spec's DELETE is a SHOULD, and stdio servers are already retired on exit. I documented this in ARCHITECTURE.md and didn't change it here.

2. A realistic streamed SSE response with an in-POST server→client request

The fixture's new ask_user tool answers like a real server does. It opens an SSE stream on the call's own POST and flushes it one event at a time: first a notifications/progress (with the request's progress token), then an elicitation/create to the client. It sends the response only once the client's answer has come back on a separate POST. The fixture also now accepts JSON-RPC responses that the client POSTs back.

mcp_oauth_routing::an_oauth_servers_in_post_question_is_routed_to_the_client_and_answered_mid_stream (serve):

  • the progress event arrives as a tool_progress frame before the elicitation_request;
  • the question is routed to the serve client, which answers accept {name: Ferris};
  • the call completes with hello-Ferris, and the fixture received exactly one accept answer.

A client that buffered the stream until it ended would never see the question. The fixture would then answer "no answer before the stream had to end" after 10 s, and the test would fail.

3. The flaky serve_ws test, fixed at the root cause

The test: serve_ws::tests::a_session_directory_with_a_segment_is_left_alone. It failed during the audit with assertion failed: acquire_session_lock(&path).unwrap().is_some(), meaning a session lock was still held right after it had been released.

The cause: the session lock was an flock. An flock belongs to the open file description. When any other thread forks a child (a bash call, an MCP stdio server, another test), the child shares that description until it execs, because close-on-exec only acts at exec. A lock released during that window stays held, and the next owner is refused. Measured: with 8 threads spawning true, 180 of 3000 immediate re-takes found the lock still held.

The fix: the session lock is now a POSIX record lock (fcntl(F_SETLK), via nix's safe wrapper; this crate forbids unsafe). A record lock belongs to the process and a fork never inherits it.

  • It is also exactly how NFS/EFS implements flock, so local and network filesystems now behave the same.
  • Its sharp edge is that closing any descriptor to the file drops the lock. The existing once-per-process registry already guards against that, as it had to for NFS.
  • nix was already in the build graph at 0.31 as a transitive dependency.

Proof of stability:

  • session_store::tests::a_released_session_lock_is_free_at_once_while_other_threads_spawn_processes asserts 0/3000 with the process spawners running. It was 180/3000 before the fix.
  • a_session_lock_still_excludes_another_process checks that another process (python3's fcntl.lockf) is refused while the lock is held and allowed after it is released.
  • Heavy parallel load: 4 concurrent copies of the whole lib suite, 5 rounds (20 suite runs).
    • Old flock: the serve_ws test failed 1/20 and the regression test 20/20.
    • New lock: 0 failures across all 440 serve_ws test runs.

Rollout note: on a local filesystem, an old binary (using flock) and a new one (using record locks) don't see each other's locks. On NFS/EFS they do, because there both are record locks. The lock is liveness-only (the epoch fence provides correctness), so mixed versions on a local disk cost nothing worse than two live owners racing the fence.

Also fixed: the same load run showed tools::output::tests::ensure_temp_file_sweeps_stale_siblings_but_leaves_fresh_ones failing 3/20. It used a fixed file name in the shared system temp dir, so concurrent runs swept each other's files. It now uses a unique prefix.

ARCHITECTURE.md is updated (Locks, the OAuth fixture coverage, the run-exit DELETE note).

Follow-ups (rebased onto #141's file_lock; commits 59f6585, 8015737, d17152f)

Rebase. #141 moved the session lock and the MCP manifest lock onto a shared file_lock::try_lock, which still used flock. The record lock now lives in file_lock, so both locks get it. file_lock::try_lock(Target::Dir | Target::File) names its own lock files, and no flock-only path is left live.

# Audit item Fix Proving test (fails with the fix reverted; mutation-checked)
1 file_lock::try_lock still flock POSIX record lock (fcntl(F_SETLK) via nix) in file_lock session_store::tests::a_released_session_lock_is_free_at_once_while_other_threads_spawn_processes (0/3000 with 8 spawner threads)
2 Registry key not canonical: another spelling acquired a second time and unlocked the first holder on drop The key is the canonical path: the parent canonicalized, then the file name file_lock::tests::another_spelling_of_a_held_path_is_refused_and_never_unlocks_the_holder (symlinked parent and s1/../s1 are refused; another process still sees it held after the refused drop)
3 Close-any-fd trap The record lock file has a distinctive name built only in file_lock.rs. The file tools' read/write/search (tools::fs::local) and the worktree copy refuse or skip it (file_lock::is_record_lock_file). The failed-start directory is taken back through file_lock::dir_holds_only_lock_files + FileLock::remove_files, never by name file_lock::tests::only_this_module_names_the_record_lock_file (lint); session_store::tests::reading_a_locked_session_through_any_public_api_leaves_it_locked (list, open, fork, tool read and search of a locked session: another process still sees it held)
4 Mixed rollout The new binary also takes the legacy flock on <dir>/lock / <file>.lock (its own close-on-exec descriptor), retried up to 250 ms so a sibling's fork→exec window isn't mistaken for a holder. The record lock is authoritative. The flock half is removed in a later release (documented in the module and ARCHITECTURE.md) file_lock::tests::an_old_binary_holding_only_the_legacy_flock_excludes_this_one (a python flock holder excludes this binary until it lets go); the 3000-iteration test still 0. Removing the retry fails the 3000 test

Heavy parallel load (4 concurrent full lib suites × 5 rounds): serve_ws lock test 0 failures, regression test 0, 440/440 serve_ws test runs ok.

HTTP sessions end on graceful exit (8015737). Each connection records the session it opened. OAuthHttp sees each response's Mcp-Session-Id and each DELETE rmcp sends (tools/mcp_http_exit.rs). mcp_stdio::sweep_before_exit covers every graceful exit (exit_process and ExitSweep). It first sends a DELETE for every session still open:

  • in parallel, with the current bearer and the connection's headers;
  • on its own thread and runtime, with a fresh pool-less client (a grant connector's client keeps its SSRF resolver);
  • bounded by 1.5 s, overlapping the stdio grace.

Tests:

  • mcp_oauth_routing::an_agent_run_ends_its_oauth_session_with_one_authenticated_delete_before_it_exits: exactly one DELETE (bearer + session id).
  • …an_exit_whose_delete_is_never_answered_stays_bounded: the server holds the DELETE for 30 s, and the run still exits within baseline + 4 s.

Both fail with the sweep removed. The second fails with the deadline removed. The "no DELETE on exit" note is gone from ARCHITECTURE.md.

mcp_apps_session::view_html_is_cached… flake (d17152f). rmcp handles list_changed on a task of its own, so the response to change_view can arrive first, and the test slept 200 ms and hoped. The handler now logs (debug) once it has run, and the test waits for that line in the daemon's stderr, within a deadline. Under 8 concurrent copies plus a full lib suite × 6 rounds: 48/48.

The OAuth routing tests now read the child's stdout through common::child_frames, as #141's harness rule requires.

Local (final head): all mcp_*, serve_* and run_* suites plus lib units: 2303 passed. The OAuth routing suite stressed as 6 concurrent copies × 3 rounds alongside the lib suite: all passed. Clippy --workspace --all-targets -D warnings, cargo fmt --check and dprint are clean.

Re-audit follow-ups (e5a494f, rebased onto c596e96)

# Finding Fix Proving test (fails with the fix reverted; mutation-checked)
1 (MEDIUM) A symlink or hard link to a held record lock file passed the name-only check. The in-process tools opened and closed it, releasing the lock, and write scribbled into it The registry now keeps held record files' (dev, inode).
file_lock::guard_path refuses a path that resolves to one (symlink or hard link), without opening it.
file_lock::guard_opened checks what an open actually reached. If it's a held lock (path retargeted after the check), the descriptor is never closed, so the lock stays held.
Applied to: the file tools' read/write/edit/search, is_writable (which opened every stat'ed file for write), write_atomic, the worktree copy, and @file/context-file reads (open_guarded, read_to_string_guarded).
What remains is an atomic write's rename in the instant after its check; it never opens the lock file. Child processes such as bash cat are separate processes and can't affect this process's record lock (documented)
session_store::tests::a_link_to_a_held_lock_file_is_refused_by_every_file_tool: symlink and hard link refused by read/write/edit/search, the lock still held for another process, nothing written.
file_lock::tests::a_held_lock_file_reached_after_the_check_is_refused_and_never_closed
2 (LOW, V5) The worktree seeding skip was untested test worktree::tests::seeding_never_copies_a_held_lock_file_and_leaves_it_held: neither the record file nor a hard link to it is copied, and the lock stays held
3 (LOW) OPEN held sessions strongly Sessions are held weakly. A connection that goes away with its session still open sends the DELETE at once on its runtime. It is kept for the exit only until that DELETE is answered, or when the runtime itself is tearing down, so the agent run exit DELETE still works mcp_http_exit::tests::a_connection_gone_with_its_session_open_ends_it_and_is_not_kept; …a_session_rmcp_ended_is_neither_kept_nor_sent_again

Local (final head): all mcp_*, serve_* and run_* suites plus lib units: 2341 passed. Clippy --workspace --all-targets -D warnings, cargo fmt --check, dprint clean.

Lock redesign: open file description locks (d950f1f, rebased onto 771d91b)

Per the decision, the lock is now an OFD lock (fcntl(F_OFD_SETLK) through nix's safe fcntl), and the per-opener guard machinery is removed: guard_path, guard_opened, the dev/ino registry, open_guarded/read_to_string_guarded, the per-tool refusals, and the in-process path registry.

  • Owned by the one open that took it. Any other descriptor of the lock file (a file tool, the memory tool, an @file read, by name, through a symlink or a hard link) can open and close it without releasing or sharing the lock. Two descriptions conflict even in one process, so the kernel refuses a same-process double acquire.
  • Released with an explicit F_UNLCK before the descriptor closes, so a forked child's copy of the description cannot keep it. The fork-window flake stays fixed.
  • Off Linux: flock, which is also per-description, released with an explicit LOCK_UN.
  • NFS/EFS: verified on a loopback NFSv4.2 mount with local_lock=none, so locks go to the server. The file_lock tests, the 3000-iteration fork test and the every-reader probe all pass with their files on the mount.
  • Legacy flock half: kept for mixed rollout, also released with an explicit LOCK_UN.
  • Kept:
    • write_atomic refuses to rename over a lock file. Replacing the file is the one remaining hazard, and it isn't about descriptors.
    • The worktree seed still leaves lock files behind.
  • Dropped: the lock-file-name lint. Other modules naming the file is harmless now.
Proof Test (fails with the fix reverted; mutation-checked)
Fork test stays at 0 session_store::tests::a_released_session_lock_is_free_at_once_while_other_threads_spawn_processes (fails without the explicit unlock)
Read, write, edit, search, memory view and @file, through a symlink and through a hard link, leave the lock held for another process and refused here, with the file untouched session_store::tests::every_in_process_reader_reaching_the_lock_file_leaves_it_held (fails with POSIX F_SETLK, and fails with write_atomic's refusal removed)
Another descriptor neither releases nor shares the lock file_lock::tests::another_descriptor_of_the_lock_file_neither_releases_nor_shares_it
A child holding the description cannot keep a released lock file_lock::tests::a_released_lock_is_free_while_a_child_still_holds_the_description
Same-process double acquire is refused file_lock::tests::one_holder_at_a_time_in_this_process_and_free_again_on_drop
An old flock holder excludes us file_lock::tests::an_old_binary_holding_only_the_legacy_flock_excludes_this_one
ORPHANS is bounded: an unanswered DELETE is dropped after its timeout. Tests use their own registry, so they are hermetic mcp_http_exit::tests::an_unanswered_orphan_delete_is_dropped_after_its_timeout (fails if entries are kept until answered)

Heavy load (4 concurrent full lib suites × 5 rounds): the serve_ws lock test failed 0 times, the regression test 0 times, and all 440 serve_ws test runs passed.

Local (final head): all mcp_*, serve_* and run_* suites plus lib units: 2347 passed. One test, mcp_skills_integrity::a_listing_that_never_ends_is_cut_off_and_said_so, failed once and passed on retry. It is a 45 s stdio timing test and doesn't touch locks. Clippy -D warnings, cargo fmt --check and dprint are clean.

Rebased onto #137 (0602cac), squashed into one commit (14ec00c)

# Item Fix Proving test (fails without the fix)
1 #137 locks its journal key file via file_lock::try_lock(&path) and writes through lock.file() New Target::Itself(path) locks the file itself. There's no lock file beside it, and FileLock::file() returns the locked descriptor, so #137's "lock, re-read, write through the locked fd" is unchanged. There's no legacy flock on it: no released binary took one, and an flock on the same file conflicts with our own OFD lock on NFS, where flock is a process-owned POSIX lock. I found that on the loopback mount: with it, concurrent_first_journal_writers_all_use_one_key failed on NFS; without it, it passes on v4.2, v4.0 and v3 file_lock::tests::a_file_locked_itself_is_written_through_its_lock. #137's concurrent_first_journal_writers_all_use_one_key and all its journal-key tests still pass; all 7 fail if Itself falls back to a sidecar lock file
2 NFS: release_session_lock unlinked the lock files while their fds were open (silly-rename to .nfs*, then remove_dir gives ENOTEMPTY) FileLock::release_and_remove_files(self) unlocks and closes first, then unlinks; remove_dir follows serve_ws::tests::an_empty_session_directory_is_taken_back_with_its_lock on the loopback NFSv4.2 mount: fails with the old unlink→rmdir→close order (the dir stays) and passes with the fix. A direct probe confirmed the mechanism: unlinking an open lock file leaves .nfs000… and rmdir gives ENOTEMPTY until close
3 Stale "never inherited" doc Rewritten: the OFD lock is inherited by a fork and released by the explicit unlock. The acquire_session_lock doc is updated too doc
4 write_atomic refused only .beyond-lock file_lock::is_lock_file also covers a legacy lock / <f>.lock beside a record lock file, so an unrelated Cargo.lock is still written tools::tests::write_atomic_never_replaces_a_lock_file (the legacy file, directly and through a symlink, keeps its inode; Cargo.lock is written); file_lock::tests::a_legacy_lock_file_is_one_only_beside_a_record_lock_file

Local (final head): all mcp_*, serve_* and run_* suites plus lib units: 2376 passed. Two tests from #136 passed only on retry under full load: mcp_events_nested::in_a_daemon_a_nested_elicitation_during_an_events_poll_reaches_the_events_session and mcp_events_routing::a_permanently_refused_configured_subscription_is_reported_and_does_not_keep_the_session. Both are timeouts in the events fixture's wait. Run in isolation they passed 5/5. Clippy -D warnings, cargo fmt --check and dprint are clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk

@jaredLunde
jaredLunde force-pushed the jared/mcp-oauth-fixture-coverage branch 3 times, most recently from e5a494f to d950f1f Compare October 7, 2026 09:48
…ession locks

One commit for #144, rebased onto #137/#148/#150 (0602cac).

- OAuth fixture: full streamable-HTTP server (sessions + 404 expiry, SSE
  streamed event by event with an in-POST elicitation, GET stream, DELETE).
  Routing tests: DELETE with bearer + session on reap and on exit, in-POST
  question routed and answered mid-stream.
- HTTP MCP sessions get a bounded, parallel, authenticated DELETE on every
  graceful exit (mcp_http_exit); sessions held weakly, orphan DELETEs dropped
  once their attempt is over; the registry is a value, tests use their own.
- file_lock: open file description locks (F_OFD_SETLK via nix), released by
  an explicit unlock — no other descriptor releases them, a forked child's
  copy cannot keep them, two descriptions conflict in one process. Off
  Linux, flock with explicit LOCK_UN. Legacy flock half for mixed rollout.
  Target::Itself for #137's journal key: locked itself, written through the
  locked descriptor, no legacy flock (it would self-conflict on NFS).
  Verified on loopback NFS v4.2/v4.0/v3.
- A failed start's session dir: lock released and closed before its files
  are unlinked (NFS silly-renames open files and rmdir then fails).
- write_atomic never renames over a lock file, record or legacy (only a
  legacy one beside a record one, so Cargo.lock is untouched).
- apps cache test waits for list_changed deterministically; output.rs sweep
  test uses a unique prefix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk
@jaredLunde
jaredLunde force-pushed the jared/mcp-oauth-fixture-coverage branch from d950f1f to 14ec00c Compare October 7, 2026 11:09
@jaredLunde
jaredLunde merged commit 86b7b23 into main Oct 7, 2026
21 checks passed
@jaredLunde
jaredLunde deleted the jared/mcp-oauth-fixture-coverage branch October 7, 2026 11:37
jaredLunde added a commit that referenced this pull request Oct 7, 2026
Tests that failed only under heavy parallel load, each made independent of
host timing:

- mcp_events_nested (a real race): serve started the MCP Events hub before
  installing the session's elicitation and sampling gates, so a server
  asking a question during the very first events/poll was declined as "no
  client". The hub now starts after the gates. A debug-only seam
  (BEYOND_AI_AGENT_TEST_SLOW_GATE_INSTALL_MS) holds the window open; the
  new test fails on the old order.
- mcp_events_receiver a_retry_that_beats_the_resubscribe (a real race): a
  restarted daemon answered 410 (stop) to a delivery for a persisted
  callback that arrived before its events session had read its state and
  reserved the token. Unknown tokens are now 503 from daemon start until
  that session has reserved its tokens (bounded). A debug-only seam
  (BEYOND_AI_AGENT_TEST_SLOW_EVENTS_RESTORE_MS) holds the window open, and
  the test no longer sleeps 800 ms.
- serve_uds uds_stale_socket_file_is_reclaimed: the "stale" node was a
  listener bound and dropped in the test process, which a child forked by
  another test in that instant keeps alive until its exec; a connect then
  succeeded against it. The node is now a datagram socket, which no stream
  connect can reach.
- mcp_stdio a_panic_unwinding_past_the_exit_guard_still_sweeps:
  sweep_before_exit's wait is bounded, and a loaded host could stall a
  sweep thread past it. The exiting thread now kills every server still
  running itself after the wait. The test reads "killed" from the kernel
  (gone, a zombie, or SIGKILL pending), not from scheduling.
- mcp_events wire many_events_in_one_chunk_drain_in_linear_time: counts the
  bytes scanned and moved (must stay within 2x the input) instead of
  timing the drain.
- serve_lifecycle a_slow_lifecycle_consumer_does_not_delay_the_prompt_response:
  the collector holds every POST unanswered until released, and the prompt
  response must arrive first: proved by order, not by a 1.5 s bound.
- serve_health: /readyz callers wait out the one transient answer, "shard
  probe timed out" (the probe's real answer lands in the memo); any other
  answer, a 503 for another reason included, is returned at once.
- mcp_skills_integrity a_listing_that_never_ends_is_cut_off_and_said_so: the
  listing stays fresh for the run, so the turn does not page through all 64
  again under the refresh's 30 s bound; asserts exactly 64 skills/list.

- tool_reactor_stall (#148's probe) failed on CI: a 4 MB write took under
  a millisecond on a CI runner, and the tool's fixed per-call overhead
  (~0.26 ms in a debug build) alone was a third of that baseline. The edit
  and write subjects are now ~36 MB, so the baseline is milliseconds
  everywhere and the bar (25%) is unchanged.

#137 follow-ups:
- The journal key's maker re-reads it through the locked descriptor
  (FileLock::file); with #144's OFD locks another descriptor's close no
  longer releases the lock, NFS included.
- An old-name key (not yet carried over) trashes, restores and hard-deletes
  with its session.
- Why a prompt needs no drain of queued journal writes is documented where
  the prompt reads the journal.

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
Tests that failed only under heavy parallel load, each made independent of
host timing:

- mcp_events_nested (a real race): serve started the MCP Events hub before
  installing the session's elicitation and sampling gates, so a server
  asking a question during the very first events/poll was declined as "no
  client". The hub now starts after the gates. A debug-only seam
  (BEYOND_AI_AGENT_TEST_SLOW_GATE_INSTALL_MS) holds the window open; the
  new test fails on the old order.
- mcp_events_receiver a_retry_that_beats_the_resubscribe (a real race): a
  restarted daemon answered 410 (stop) to a delivery for a persisted
  callback that arrived before its events session had read its state and
  reserved the token. Unknown tokens are now 503 from daemon start until
  that session has reserved its tokens (bounded). A debug-only seam
  (BEYOND_AI_AGENT_TEST_SLOW_EVENTS_RESTORE_MS) holds the window open, and
  the test no longer sleeps 800 ms.
- serve_uds uds_stale_socket_file_is_reclaimed: the "stale" node was a
  listener bound and dropped in the test process, which a child forked by
  another test in that instant keeps alive until its exec; a connect then
  succeeded against it. The node is now a datagram socket, which no stream
  connect can reach.
- mcp_stdio a_panic_unwinding_past_the_exit_guard_still_sweeps:
  sweep_before_exit's wait is bounded, and a loaded host could stall a
  sweep thread past it. The exiting thread now kills every server still
  running itself after the wait. The test reads "killed" from the kernel
  (gone, a zombie, or SIGKILL pending), not from scheduling.
- mcp_events wire many_events_in_one_chunk_drain_in_linear_time: counts the
  bytes scanned and moved (must stay within 2x the input) instead of
  timing the drain.
- serve_lifecycle a_slow_lifecycle_consumer_does_not_delay_the_prompt_response:
  the collector holds every POST unanswered until released, and the prompt
  response must arrive first: proved by order, not by a 1.5 s bound.
- serve_health: /readyz callers wait out the one transient answer, "shard
  probe timed out" (the probe's real answer lands in the memo); any other
  answer, a 503 for another reason included, is returned at once.
- mcp_skills_integrity a_listing_that_never_ends_is_cut_off_and_said_so: the
  listing stays fresh for the run, so the turn does not page through all 64
  again under the refresh's 30 s bound; asserts exactly 64 skills/list.

- tool_reactor_stall (#148's probe) failed on CI: a 4 MB write took under
  a millisecond on a CI runner, and the tool's fixed per-call overhead
  (~0.26 ms in a debug build) alone was a third of that baseline. The edit
  and write subjects are now ~36 MB, so the baseline is milliseconds
  everywhere and the bar (25%) is unchanged.

#137 follow-ups:
- The journal key's maker re-reads it through the locked descriptor
  (FileLock::file); with #144's OFD locks another descriptor's close no
  longer releases the lock, NFS included.
- An old-name key (not yet carried over) trashes, restores and hard-deletes
  with its session.
- Why a prompt needs no drain of queued journal writes is documented where
  the prompt reads the journal.


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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant