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 2 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: needs maintainer review before merge. Reviewed July 27, 2026, 11:21 AM ET / 15:21 UTC. ClawSweeper reviewWhat this changesThe PR forwards the OAuth timeout through Merge readinessKeep this PR open for maintainer merge review. The prior review findings are addressed: OAuth authorization timeouts now reach the Priority: P1 Review scores
Verification
How this fits together
flowchart LR
A[mcporter auth command] --> B[Runtime tool discovery]
B --> C[Keep-alive daemon client]
C --> D[Versioned local socket protocol]
D --> E[Daemon host and OAuth flow]
E --> F[Paginated MCP tools listing]
E --> G[Progress frames]
G --> C
F --> H[Tool list returned to CLI]
Decision needed
Why: The implementation and proof support the fix, but accepting the one-time keep-alive daemon replacement and the new liveness contract is an operational compatibility choice for maintainers. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the focused timeout and liveness design if maintainers accept the v2 local-daemon upgrade behavior, keeping operation deadlines enforced by the daemon and idle recovery enforced by the client. Do we have a high-confidence way to reproduce the issue? Yes, with high confidence from the supplied real local OAuth fixture and the source path: current main's fixed socket budget can expire across an OAuth wait plus multiple Is this the best way to solve the issue? Yes. Refreshing an idle socket deadline from daemon progress while retaining daemon-enforced operation timeouts is a narrower solution than guessing a larger phase-count budget; versioning the local protocol avoids mixed framing during upgrade. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4062298d66ef. 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 (3 earlier review cycles)
|
`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
1 participant
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.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.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.Housekeeping
main.pnpm checkandpnpm testgreen locally (851 passed, 3 skipped); CI green on Ubuntu, macOS and Windows.