Skip to content

fix(mcp): recover unsent requests after daemon loss - #1784

Open
Pxx500 wants to merge 2 commits into
DeusData:mainfrom
Pxx500:fix/mcp-frontend-recovery
Open

fix(mcp): recover unsent requests after daemon loss#1784
Pxx500 wants to merge 2 commits into
DeusData:mainfrom
Pxx500:fix/mcp-frontend-recovery

Conversation

@Pxx500

@Pxx500 Pxx500 commented Aug 21, 2026

Copy link
Copy Markdown

this pull request was generated entirely by AI
i can't guarantee its quality, but i've tried to make it as good as possible
i'm happy to revise it if it doesn't meet the repository's standards

What does this PR do?

recovers the stdio MCP frontend when its shared daemon disappears before an application frame is sent
the runtime now reports whether the frame crossed the local transport, which lets the frontend restore its session and retry exactly once only when replay is safe
regression coverage includes daemon replacement, cancellation during reconnect, and shutdown ownership during reconnect

related to #1182

Checklist

  • Every commit is signed off (git commit -s) (required, CI rejects unsigned commits; DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

@Pxx500
Pxx500 requested a review from DeusData as a code owner August 21, 2026 14:12
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@Pxx500

Pxx500 commented Aug 22, 2026

Copy link
Copy Markdown
Author

it looks like the three failed checks came from a race condition, could you please rerun the failed jobs?

@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Aug 24, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thank you for the detailed investigation and for being transparent about the implementation. This changes request sending, reconnect, replay, cancellation, and shutdown ownership, so we are reviewing the correctness and safety boundaries carefully. We will come back with a maintainer decision, including how to handle the failed jobs, once that review is complete. Our review queue is currently full, so this may take a little time. Thank you for your patience and for offering to revise the change.

@Pxx500

Pxx500 commented Aug 26, 2026

Copy link
Copy Markdown
Author

as far as the usability is concerned I have not experienced any problems/crashes since implementing this locally over the last 5 days

Signed-off-by: Pxx500 <pbartulik@gmail.com>
Signed-off-by: Pxx500 <pbartulik@gmail.com>
@Pxx500
Pxx500 force-pushed the fix/mcp-frontend-recovery branch from 4cf2acf to b9da6dc Compare September 5, 2026 14:22
@DeusData

DeusData commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Thanks for the rebase yesterday — the substance holds up under a hostile read, and that is the part that matters most for a recovery path. Replay safety is sound: request_sent_out is captured at runtime.c:3088 right after the frame send and before the cancel-frame reassignment, send_frame reports true only once header and payload are fully written, the Windows wait_pending treats a write completed before CancelIoEx as sent and a cancelled partial write as poisoned, the retry happens exactly once (frontend.c:596 is not re-entered), and a cancel during recovery is read under the mutex before the token reserve. So a request is replayed only when no complete frame can have reached the daemon, and index_repository / manage_adr / delete_project cannot be duplicated. With a local test adaptation (below), daemon_frontend passes 16/16 four times over, the eight daemon/runtime/IPC/bootstrap suites 230/230, mcp + cli 639/0, and reverting the runtime change makes exactly your three new tests fail — they bind.

Four things before it can merge, the first two blocking:

  1. The head does not compile the test-runner (same on CI run 33971609329: every test leg red, lint/smoke green). tests/test_daemon_frontend.c:214-217frontend_idle_run is main's seams-only idle-observer test from Every idle MCP client burns ~0.7 core since 0.9.1-rc.1 (0.9.0: 0%) — N concurrent sessions cost N cores, Windows #1764 and still calls the four-argument cbm_daemon_frontend_mcp_run; your signature has five. The adaptation is mechanical — add a const cbm_daemon_frontend_session_config_t *session to frontend_idle_run_t, keep the build fingerprint and identity in frontend_idle_fixture_t, and build the session in daemon_frontend_idle_uses_one_maintenance_observer:
cbm_daemon_frontend_session_config_t session = {
    .bootstrap = { .role = CBM_DAEMON_PROCESS_MCP_CLIENT,
                   .endpoint = fixture.maintenance.endpoint,
                   .identity = &fixture.identity,
                   .executable_path = "unused",
                   .connect_timeout_ms = 30000, .startup_timeout_ms = 30000 },
    .session_root = fixture.maintenance.parent,
    .tool_profile = CBM_MCP_TOOL_PROFILE_ALL,
};

(with fixture.identity / fixture.build_fingerprint stored on the fixture instead of stack locals, and .session = &session on the run struct). That is the whole delta; I verified it builds and the suite is green.

  1. FRONTEND_RECOVERY_CANCEL orders "cancel routed before release" with cbm_usleep(100000) (~test_daemon_frontend.c:507-511). Under CI load the routing can take longer than 100 ms, the retry path runs, and response_exact fails — a test whose verdict depends on timing is a lottery for us, not a test. Please make the handoff deterministic the way frontend_test_worker_idle_cycles does: a test-seam counter of routed cancellations the test waits on, never a sleep.

  2. Minor: recovery calls cbm_daemon_bootstrap_execute from the worker, and on failure bootstrap_production_diagnostic writes to stderr (bootstrap.c:1023-1040), which contradicts frontend.c's own "never log from the worker" rule. Failure path only, right before _Exit, so a note is enough — either route it through the frontend's reporting or say why the exception is acceptable.

  3. Scope questions, your call as long as they are answered in the PR text: recovery replays SET_CONTEXT and the UI configuration but not the initialize-driven session state (detect_session / maybe_auto_index, pending_background_initialize), so after a daemon replacement the new session gets no auto-index or background activation — in scope here, or a follow-up you'd file? And Windows/Codex: MCP tool calls close transport while CLI/stdio init work; update -y still prompts for binary variant #1182 is a Windows report while the three new tests are #ifndef _WIN32 (fork-based); a Windows-exercisable variant, or the reason the POSIX tests cover the shared path, would close that gap.

Keep the sign-offs on the rebased commits and this merges on green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants