fix(auth): keep the OAuth listTools request alive across every phase - #241
fix(auth): keep the OAuth listTools request alive across every phase#241umutkeltek wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab62f071c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| preParsedRequest | ||
| ); | ||
| socket.write(JSON.stringify(response), () => { | ||
| const stopProgress = startProgressFrames(socket, preParsedRequest?.id ?? 'unknown'); |
There was a problem hiding this comment.
When a keep-alive server accepts resources/list or resources/read but never responds, this unconditional heartbeat continues while the daemon event loop remains healthy, and the client resets its 30-second socket deadline on every frame. Those runtime methods have no operation timeout, so the CLI now waits forever instead of timing out, restarting the daemon, and retrying as it did before this commit. Restrict these heartbeats to the bounded listTools path or retain an absolute recovery deadline for the other methods.
Useful? React with 👍 / 👎.
| // deadline, so a request stays alive for as many phases as it needs -- an OAuth | ||
| // code wait plus any number of paginated `tools/list` pages -- without the | ||
| // client having to predict how many phases there will be. | ||
| export const DAEMON_PROGRESS_INTERVAL_MS = 250; |
There was a problem hiding this comment.
When an API caller supplies ListToolsOptions.timeoutMs <= 250 (also accepted by --oauth-timeout), the client arms that same socket timeout immediately, but the host waits 250 ms before its first progress frame. Because the daemon starts the operation timeout only after receiving and dispatching the request, the socket deadline can fire first, causing invoke() to classify this as a transport failure and restart/replay the request instead of returning the non-retryable operation_timeout. Send an initial frame immediately or coordinate the interval with the requested timeout.
Useful? React with 👍 / 👎.
ab62f07 to
9a296ab
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: found issues before merge. Reviewed August 2, 2026, 9:07 PM ET / August 3, 2026, 01:07 UTC. ClawSweeper reviewWhat this changesThis branch forwards OAuth-aware tool-discovery timing through the CLI and local daemon, adds daemon progress frames and operation bounds, guards repeated pagination cursors, and changes daemon replacement to drain in-flight work. Merge readinessKeep this PR open: the previously identified P1 remains at the unchanged head, and the branch is now dirty against a substantially changed current main. The OAuth replay motivation is partly reduced by merged work, but the proposed daemon liveness behavior remains distinct and needs a corrected idle-drain implementation plus a rebase. Priority: P1 Review scores
Verification
How this fits togetherMCPorter can route CLI operations through a persistent local daemon, which invokes the MCP runtime and may wait for OAuth authorization or paginated tool discovery. The changed path determines when the socket sees liveness, when an operation fails, and whether an upgraded client may replace a shared daemon. flowchart LR
CLI[CLI auth or list command] --> Client[Keep-alive daemon client]
Client --> Socket[Local daemon socket]
Socket --> Host[Daemon host]
Host --> Runtime[MCP runtime]
Runtime --> OAuth[OAuth authorization]
Runtime --> Tools[Paginated tool discovery]
Host --> Frames[Progress and result frames]
Frames --> Client
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Rebase the daemon liveness design onto the MCP SDK v2 runtime, exclude control probes from active-operation accounting, and prove both idle replacement and busy-peer preservation through the real host/socket path. Do we have a high-confidence way to reproduce the issue? Yes — source inspection gives a high-confidence path: the host increments the active counter for Is this the best way to solve the issue? No — the proposed liveness approach may still be useful, but its idle-drain implementation is incorrect and the branch must be adapted to current main's MCP SDK v2 runtime before it is a maintainable solution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ed2dee1cf8df. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (29 earlier review cycles; latest 8 shown)
|
`mcporter auth` runs the interactive OAuth browser flow inside the SDK's
`tools/list` request, but `listTools` never forwarded a per-request timeout. The
SDK's 60s DEFAULT_REQUEST_TIMEOUT_MSEC killed the request long before the 300s
OAuth code wait could finish, so slow providers could never be authorized --
every `mcporter auth` died with `MCP error -32001: Request timed out`.
Mirror the existing `callTool` timeout forwarding:
- add `timeoutMs` to `ListToolsOptions` and pass `{ timeout,
resetTimeoutOnProgress, maxTotalTimeout }` to `client.listTools`, with
`raceWithTimeout` as an outer guard;
- have the `auth` path pass `MCPORTER_OAUTH_TIMEOUT_MS` (default 300s) so the
request lives at least as long as the OAuth code wait;
- carry `timeoutMs` across the daemon protocol so keep-alive servers get the
same deadline, and report a phase that blows it as `operation_timeout` so the
keep-alive wrapper stops treating it as a dead server worth restarting;
- version the daemon protocol so a daemon started before this change is
replaced instead of silently dropping the new field.
9a296ab to
6d72274
Compare
|
Thanks — both review passes found real things. All three items are addressed and the PR body now carries an inspectable before/after run. Summary: P1 — liveness frames outliving a wedged request. The stated premise doesn't hold:
P2 — short deadlines expiring before the first frame. Fixed, and a step further than suggested: an immediate first frame alone still loses a sub-250ms deadline on the second gap, so the cadence is now derived from the caller's own deadline (sent on the request envelope) and the first frame is written immediately on dispatch. P3 — release-owned changelog. Correct, and I checked it against this repo's history rather than taking it on trust: my own merged #234 didn't touch Real behavior proof is in the PR body: a local OAuth-protected MCP server (dynamic registration, PKCE Each item has regression coverage; |
clawsweeper
Bot
added
proof: sufficient
6d72274 to
2d6d820
Compare
|
Fixed the remaining P2 — good catch, the finding is exact.
I did not reject short values at the input boundary instead: Regression added below the old floor, asserting the invariant directly rather than racing a timer:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
Sizing the daemon socket deadline for a fixed number of phases cannot work. `McpRuntime.listTools()` issues one `tools/list` request per cursor page and gives each its own `timeoutMs`, so an OAuth wait followed by two or more slow pages outlives any constant multiple of the caller deadline. The socket then expires mid-flight, the client restarts the daemon and resends the listing, and the user is walked through a second OAuth flow. Stop predicting the phase count and observe the daemon instead. While a request is in flight the host emits newline-delimited progress frames, and the client treats each frame as proof of life and restarts its socket deadline. The deadline becomes a liveness budget rather than an operation budget: a request survives however many phases it needs, while a daemon that goes silent is still torn down and restarted exactly as before. Liveness alone would be too weak, so every operation stays provably bounded: - the first frame goes out immediately and the cadence is derived from the caller's own deadline, so a deadline shorter than the default interval cannot expire before the daemon has proved it is alive; - operations with a fixed phase count -- at most one connect plus one MCP request -- carry an absolute ceiling equal to the sum of the deadlines the daemon already applies to those phases, so a server that accepts a request and never answers yields a non-retryable `operation_timeout` rather than an indefinite wait; - `listTools` keeps its per-page deadline and gains a repeated-cursor guard, so the one operation whose phase count is data-dependent cannot page forever. Both peers decode frames incrementally, so the daemon's own status probe and stop handshake stay readable, and a response with no trailing newline still parses. Regression coverage drives the real client transport and the real host framing over a socket: an OAuth wait followed by three delayed `tools/list` pages resolves with one `listTools` request, no replay, and no daemon relaunch. Sibling cases cover a deadline below the default frame interval, a daemon that goes silent, and a server that repeats a pagination cursor.
2d6d820 to
0e37f7e
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
The previous code's protocol-version check classified any pre-v2 daemon as stale and called stop() on it from ensureDaemon(). A second client upgrading mid-request -- OAuth code wait or a delayed cursor page -- would race ahead of the first client's in-flight call, kill the daemon, and force the very replay the liveness mechanism was added to prevent. The fix makes the replacement an idle-drain transition, not a preempt: * host: the status response now reports the live in-flight count, sampled at response time so a long-running request still shows up as in flight. * protocol: StatusResult.activeRequests is optional so a v1 daemon that cannot advertise it does not break old clients. * client: before restartDaemon() issues stop(), it polls the live daemon up to MCPORTER_DAEMON_DRAIN_TIMEOUT_MS (60s default) for activeRequests to read zero. Pre-v2 daemons are treated as permanently busy, so the drain timeout is the only knob for them too. If the daemon is still busy past the timeout, the replacement is abandoned and the caller fails loud rather than killing the peer. Drain timeout is env-var overridable so the test suite can exercise the refusal branch without waiting the full minute.
Regression coverage for the Codex/ClawSweeper review round: * daemon-client-config-stale: two new tests pin the drain behavior. The first asserts a busy shared daemon is left alone until its in-flight count flips to zero, then replaced; the second asserts a daemon that never drains is left running rather than killed mid-request. * daemon-host: three new tests pin the operation ceiling for callTool, listResources, and readResource against a wedged runtime, using MCPORTER_OAUTH_TIMEOUT_MS=50 and fake timers so the OAuth-timeout-plus-60s-DEFAULT_REQUEST_TIMEOUT_MSEC ceiling fires in milliseconds rather than the production minute-plus. * daemon-listtools-progress: one new test exercises a 30ms caller deadline -- well under the 250ms default progress cadence -- to prove the first progress frame beats the deadline. The existing 120ms test already exercised this implicitly; the 30ms case makes the P2 fix visible in the test name.
clawsweeper
Bot
added
status: 👀 ready for maintainer look
The earlier drain landed a contract bug: it only checked whether the original daemon had gone idle, then immediately fell through to stop(). A second client that won the race to replace the busy daemon during the drain would see its fresh daemon killed by the upgrading client the moment the drain returned. Three changes close the gap: * client: add `probeLiveStatus` that sends a raw status probe without the pid-match filter `readVerifiedStatus` applies. `readVerifiedStatus` collapses pid mismatches into null, which would short-circuit the drain check and re-introduce the very regression the drain exists to prevent. * client: `waitForDaemonIdle` now returns both `drained` and `replacedByPeer`. The pid is tracked across polls so a swap mid-wait is visible at the moment the drain resolves. * client: `restartDaemon` uses `probeLiveStatus` and bails out on `replacedByPeer` instead of issuing `stop`. A third regression test pins the new path: a busy daemon that another client replaces mid-drain must not receive stop, and the upgrading client must not relaunch a daemon of its own.
The drain handles the case where a peer swaps in a fresh daemon *while* the upgrading client is waiting. A subtler race happens *before* the drain even starts: a peer has already replaced the busy daemon, but the on-disk metadata still names the old PID, so the previous early-return check (which also required a fresh config) did not fire and the code fell through to stop() against the peer's fresh daemon. Make the pid-mismatch short-circuit in restartDaemon unconditional: if the live daemon's pid does not match the expected one, do not stop it, whether or not the metadata still reads stale. The peer owns the replacement; the upgrading client just uses whatever daemon is live. This subsumes the pid-mismatch + fresh-config fast path, which is removed rather than stacked on. The transport-error path is unaffected because expectedPid is undefined there and the guard is skipped, so the client still self-heals when no daemon is responding. A new regression test pins the scenario: a peer wins the swap before the drain starts, the upgrading client must not call stop, and the peer's daemon must keep serving.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Heads-up for the next rebase: #253 just landed on main and overlaps this PR's territory. That may shrink what this PR needs to solve — the OAuth-replay motivation is partly addressed — but the core liveness-frame design for multi-phase |
|
Triage update, and an apology for the timing. Your analysis holds, and the bug is still real on Your central point survives too: The bad news is that this branch cannot be rebased. Between your submission and now, the repo migrated to MCP SDK v2 for the 2026-07-28 protocol (#255), reworked elicitation and the daemon runtime (#256, #262), and rewrote large parts of Asking you to redo 1,500 lines against a moved target would be a poor trade for your time, so I am reimplementing your design directly, with I will link the replacement PR here and close this one once it lands. If you would rather carry it yourself against 0.13.0, say so and I will stand down — it is your work and I would rather you got the merge. Thank you for the report and for the second iteration on it; the diagnosis was correct and it outlived the code it was written against. |
|
Landed as #270 ( The implementation follows your design: progress frames from the daemon, an idle deadline on the client that they refresh, a daemon-owned operation deadline returned as a non-retryable error so a slow request is never replayed into a second prompt, and incremental frame decoding so split and coalesced writes parse. Version skew is handled in both directions — progress emission requires an explicit v2 opt-in, so an old client never sees a frame it cannot parse, and a new client talking to a daemon that outlived its build simply retains the flat deadline. One detail from verification you may find interesting: Node's Closing this in favour of the replacement, but the diagnosis and the design are yours — thank you for both, and for the second iteration on the original report. Sorry the migration landed on top of it. |
|
Thank you. This is a great outcome. I am happy for #270 to be the canonical implementation, and I really appreciate you preserving the design, carrying the attribution through as co-author and in the changelog, and verifying the original failure mode against current main. The socket.setTimeout inactivity behavior is a useful detail too. It is good to know that the daemon’s progress writes are the actual mechanism and the explicit re-arm is just belt and braces. No apology needed. The migration timing was simply unlucky. I am glad the diagnosis and liveness-budget framing survived the rewrite and landed cleanly. Thanks again for handling this so thoughtfully. |
2 participants
Resubmission of #237 with the correctness gap fixed and a clean history, now updated for the Codex and ClawSweeper review round.
The gap in #237
resolveOperationSocketBudgetsized thelistToolssocket at2 * timeoutMs + 5s: one phase for OAuth, one for a singletools/list. ButMcpRuntime.listTools()walksnextCursorand gives every page its owntimeoutMs, so an OAuth wait plus two or more slow pages outlives the socket. The client sees a transport timeout, restarts the daemon, resends the listing, and the user gets the second OAuth flow this PR exists to prevent.Any constant multiple has the same defect — it encodes a phase count the runtime does not promise.
The fix: observe the daemon instead of predicting it
While a request is in flight the daemon host emits newline-delimited progress frames. The client treats each frame as proof of life and restarts its socket deadline. That turns the socket deadline from an operation budget into a liveness budget:
operation_timeout, which the client does not retry.resolveOperationSocketBudgetand its 5s grace are gone. ThelistToolssocket uses the caller deadline verbatim. Both peers decode frames incrementally (DaemonFrameDecoder), so the daemon's own status probe and stop handshake stay readable and a response with no trailing newline still parses. The daemon protocol is versioned, so a daemon started before this change is replaced rather than left speaking the old wire format.Real behavior proof
A local OAuth-protected MCP server (dynamic client registration, PKCE
S256, authorization code, bearer-gated/mcp) that paginatestools/listacross 4 pages at 4s each.mcporter authruns the real interactive browser flow — the browser launch is shimmed so approval lands 4s after the URL opens — routed through the real keep-alive daemon (lifecycle.mode = keep-alive, confirmed daemon pid).MCPORTER_OAUTH_TIMEOUT_MS=6000, so every individual phase is comfortably inside its deadline and only the total exceeds a fixed budget.Before — 462865b, the head that was closed (
2 * 6000 + 5000= 17s socket budget):After — this branch:
One authorization, one completed listing, no replay, no daemon restart. The replay in the "before" run begins 17.3s after the listing started, which is exactly the
2 * timeoutMs + 5sbudget — the failure mode described in the #237 review, reproduced and then removed.(The provider is a local fixture rather than a third-party service so the run is inspectable and repeatable; it exercises the real SDK OAuth client path — discovery, registration, PKCE, code exchange, bearer-gated MCP — not a stub.)
Review round: what changed
[P1] Liveness frames could outlive a wedged request. The premise as stated — that
listResources/readResourcehave no operation timeout — does not hold: every SDK request carriesDEFAULT_REQUEST_TIMEOUT_MSEC(60s) and every OAuth wait carriesDEFAULT_OAUTH_CODE_TIMEOUT_MS(300s). But the underlying worry was real and I found a stronger case than the one reported:McpRuntime.listTools()walksnextCursorwith no guard, so a server that repeats a cursor pages forever — and heartbeats would then keep the client waiting forever. Both are now closed:operation_timeoutinstead of an indefinite wait.listTools, whose page count is data-dependent, keeps its per-page deadline and gains a repeated-cursor guard in the runtime.[P2] Short deadlines could expire before the first frame. Fixed, and slightly further than suggested: the first frame is written immediately when the request is dispatched, and the cadence is derived from the caller's own deadline (
resolveProgressInterval, sent on the request envelope) rather than fixed at 250ms. An immediate frame alone would still lose a sub-250ms deadline on the second gap.[P3] Release-owned changelog. Correct — checked against this repo's history: my own merged #234 did not touch
CHANGELOG.md; the maintainer added the entry at release with thethanks @…credit line. The entry has been removed from this branch.[P1, ClawSweeper] An upgraded client could stop a live shared daemon mid-request. The protocol-version check classified any pre-v2 daemon as stale and called
stop()on it fromensureDaemon(). A second client upgrading mid-request — OAuth code wait or a delayed cursor page — could race ahead of the first client's in-flight call, kill the daemon, and force the very replay this PR is meant to remove. The replacement is now an idle-drain transition, not a preempt:statusresponse reports the live in-flight count (StatusResult.activeRequests, sampled at response time so a long-running request still shows as in flight). The field is optional, so v1 daemons that cannot advertise it still answer old clients.restartDaemon()polls the live daemon up toMCPORTER_DAEMON_DRAIN_TIMEOUT_MS(60s default, env-var overridable) for the counter to read zero before issuingstop. Pre-v2 daemons are treated as permanently busy, so the drain timeout is also their safety bound.waitForDaemonIdlereturns bothdrainedandreplacedByPeer, and the upgrading client bails out withoutstop.If the daemon is still busy past the timeout the replacement is abandoned and the caller fails loud rather than killing the peer.
Regression coverage
tests/daemon-listtools-progress.test.tsdrives the real client transport against the real host framing over a socket:survives an OAuth wait followed by several paginated tools/list pages— an OAuth wait plus three delayed pages, every phase longer than the socket deadline. Asserts the tools resolve,listToolsran exactly once (no replay), onelistToolsrequest reached the daemon, andlaunchDaemonDetachedwas never called (no restart). Verified it fails without the mechanism: stubbing out the progress emitter makes it die withDaemon did not stop before restart could begin.reaches a deadline shorter than the default progress interval— the P2 case.emits the first progress frame before any cadence interval can elapse— pins the P2 fix at a 30ms deadline, well under the 250ms default cadence.still trips the socket deadline when the daemon stops sending progress frames— silence is still fatal.walks every cursor page but refuses to page forever on a repeated cursor— the P1 case.tests/daemon-client-timeout.test.tspins the budget itself: thelistToolssocket deadline equals the caller deadline, so a phase-count multiplier cannot come back unnoticed.tests/daemon-host.test.tspins the operation ceiling for the fixed-phase methods against a wedged runtime:callTool,listResources, andreadResourceall returnoperation_timeoutafter the sum ofMCPORTER_OAUTH_TIMEOUT_MSandDEFAULT_REQUEST_TIMEOUT_MSEC, using fake timers so the test does not wait the production minute-plus.tests/daemon-client-config-stale.test.tspins the ClawSweeper P1 fix:waits for a busy shared daemon to drain before sending stop— the upgrading client observesactiveRequestsflip to zero before issuingstop, proving no in-flight OAuth or cursor page was killed.does not stop a peer fresh daemon if it replaces the busy one mid-drain— a second client that wins the race to replace the busy daemon during the drain window has its fresh daemon left alone; the upgrading client does not callstopand does not relaunch.refuses to replace a daemon that never drains before the timeout—MCPORTER_DAEMON_DRAIN_TIMEOUT_MS=500exercises the refusal branch without waiting the production minute, asserting the busy peer's daemon is left running.Housekeeping
main.pnpm checkandpnpm testgreen locally (860 passed, 3 skipped); CI green on Ubuntu, macOS and Windows.