Repository navigation
Propagate a caller cancellation while the line editor waits for a key - #146
carldebilly wants to merge 11 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 744f326978
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Changes requested at exact head 744f326978fe5c3c79f774747eb10319cf5f1f0b: one P2 integration regression, detailed inline.
The local caller-cancellation correction itself is verified. Independent PTY tests across mergebase/head/current-base/merged-tree variants cover advanced input enabled/disabled, three cancellation policies and input-gate cleanup. The audit also checked actual StreamedReplHost and loopback WebSocket cancellation/EOF behavior.
However, after merging with current base 8c357903568801594fc64e292a8a7a4e4abf5bb3 (which contains #139), a genuine timed text read cancelled through a linked reader token throws TaskCanceledException instead of selecting its default. The exact prospective merge tree 86bc182bb761821e7ede7d24089a00e6af5309ee matches fetched refs/pull/146/merge commit 597b0ee5db49a5615c39e76d4b0997454a7bcbb7. Head alone and current base alone both return the default. Parent reproduced the merged failure and those controls with unchanged archived Core sources; an independent activated-reader harness confirmed it and the mapped-command outcome changing from Success/0 to HandlerException/1.
Parent Release solution build with -warnaserror: zero warnings/errors; full head suite 2,399 passed, 0 failed, 3 expected skips. All applicable CI checks pass, but neither the suite nor those green checks cover the linked-reader interaction. Independent audit executed 156 differential targeted cases, including 72 PTY cases.
The existing Codex Telnet peer-EOF hang was separately checked: it reproduces on mergebase, head, current base and merge, so it is real but pre-existing, not a newly introduced P1 attributed to this patch. I will clarify that in its existing thread rather than duplicate it. No repository changes or merge performed.
…ts for a key The key-reader line editor, used by hosted sessions and by the local console with advanced input, turned any cancellation of a key read into the end of input. A caller cancelling while it waited therefore ended the interactive session as an empty input, with exit code 0, ignoring both the default propagation and the configured cancellation policies; a prompt waiting there answered its default. Only a reader that stops on its own is now the end of input: a cancelled caller token propagates, so the run reports a cancellation as everywhere else. The cancellation docs, the hosted entry points and a Changed in 0.12 note describe the hosted-session consequence, and the remote-hosting sample endpoints treat a client that went away as the end of the session. Fixes #142
…n as its prompt's IReplKeyReader lets a reader observe the token it is handed through one linked to its own lifetime. Once a cancelled key read propagated instead of ending the input, such a reader's cancellation kept its linked token: a timed text prompt no longer recognised its deadline and failed instead of answering its default, and a caller cancellation surfaced carrying the reader's token rather than the caller's. The line editor's key read now attributes it to the token it was handed through PromptCancellation, as the secret prompt's key read and the echo flush already do. The caller-cancellation, exit-code, resolver and hosted-prompt tests also run through a reader that observes a linked token. The cancellation docs no longer contradict each other: the cycle's mark carries no code while the run ends as a cancellation, and the hosted session's exception doc says terminal detection always propagates it.
…istration A Telnet session never noticed its peer closing the connection: the input bridge stopped at the end of the peer's input without completing the host, so the session kept waiting for input until its token was cancelled. It now completes the host then, and the session ends with exit code 0, as a WebSocket session already did; a cancellation still ends the framing's input too, but stays the session's to report. The session also never disposed its StreamedReplHost, so every connection left its metadata in the session registry. It now disposes it, and a disposed host ignores the terminal metadata a transport may still report, such as a late TERMINAL-TYPE, which would otherwise register the ended session again.
744f326 to
0f7ab54
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… of answering it A text, confirmation or choice prompt whose hosted terminal disconnected settled as unanswered, through PromptFallback: it answered its default, so a dropped connection could confirm a default-yes action for a user who was no longer there, or failed with InvalidOperationException under PromptFallback.Fail. It now cancels the prompt with an OperationCanceledException, as a secret prompt already does since #139. Redirected input that ends still leaves the prompt to its fallback, as scripted input relies on. The prompts document the cancellation, PromptFallback says it does not apply to a disconnect, and the abandonment message is shared.
…he cancellation policy A hosted session's token cancelled while the session detected the terminal (the VT probe, or Telnet's wait for NAWS) propagated an OperationCanceledException even with ExitCodes.Cancelled or a resolver set, unlike a cancellation at any later point. The session now lets the run observe it, which reports it under the configured policy, or propagates it by default, as everywhere else.
After `exit`, a Telnet session kept reading its peer until the peer closed its side, so the host's handler never returned; a peer gone before the opening negotiation could be sent left the session waiting for input forever. Disposing the framing now sends the output already written, then cancels the read RunAsync has pending, only while it runs, so RunAsync returns and the caller's pipe is left open and usable. A failed opening negotiation ends the session, and a disposed framing no longer runs. A WebSocket aborts a receive that is cancelled, so WebSocketStream no longer passes a read's cancellation to it: a cancelled read closes the WebSocket instead, disposing the stream closes it too, and a peer that does not answer the close within a second has its connection aborted. WebSocket failures surface as IOException, as a stream's should.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e58cebbe69
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Revalidated exact head e58cebbe69c352a4ac40b271d155c9f056b7d00d against current base 9a7ab52645931570cd05782f3771cffe11365fae. Head source tree matches the prospective merge tree fbfa25b9c7ef6a9939179405d23742e89983028b.
The previous linked-reader timeout finding is corrected. Parent reran the original 24-case cancellation/echo matrix; independent core review verified caller-token normalization, hosted prompt disconnect versus redirected fallback, terminal-detection exit policies, late metadata cleanup and secret-tail handling. No remaining introduced P0–P2 in that core scope.
Changes remain requested for WebSocketStream:
- Existing P2 atomic close-task publication race confirmed deterministically, documented in its existing thread: #146 (comment)
- New separate P2 inline: sequential read cancellation then disposal disables the one-second unanswered-close timeout and can leave the receive outstanding indefinitely. Confirmed using native WebSocket.CreateFromStream peers, with no race needed and base/responsive-peer controls. Atomic publication alone is insufficient.
The existing Telnet negotiation/disposal comment is also real, but its startup behavior predates this patch. It is recorded as a gap in the newly documented drain contract rather than mislabeled as a newly introduced runtime regression: #146 (comment)
Completed parent validation: full Release solution build with -warnaserror, 0 warnings/errors; 2,478 tests passed, 0 failed, 3 expected skips. All applicable CI checks are green. Three bounded independent audits completed. Parent personally reran the WebSocket race/native-peer and Telnet framing differential probes against head/base. The passing suite does not cover these disposal cases yet.
No product edits or merge performed.
WebSocketStream started its close after marking it started, so a disposal racing a cancelled read could find nothing to wait for and return mid-close. The close is now started under a lock, where a disposal always finds it. Disposing after cancelling a read also discarded the one-second deadline while the receive still waited for the peer's answer, leaving it pending forever: asynchronous disposal now waits for pending receives, which that deadline ends, and synchronous disposal aborts a receive or close still under way. Disposing twice no longer throws. Disposing a TelnetFraming during its opening negotiation returned while the negotiation still wrote to the caller's pipe; the negotiation is now part of the sending disposal waits for, and a run no longer starts once disposal began. That wait could also last forever on a peer that stopped reading: output that makes no progress for two seconds is now given up, a slow but steady peer still receiving all of it.
…ing it A WebSocket aborts a receive that is cancelled. ReplWebSocketSession cancelled its receive as soon as the session ended and only then tried to close the socket, which was already aborted: on `exit` the peer saw the connection dropped without a close handshake, as a WebSocketException. The session now closes the connection, or answers the peer's own close, while its receive is still pending, and lets that receive end with the peer's answer; one one-second deadline covers both, aborting only a peer that misses it. The receive keeps reading after the session's close until the answer comes, without applying what the peer still sends. A session ended by cancelling its token still aborts at once.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Revalidated exact head 77293cea506d8c11fb095990446f928a6df180bb against base 9a7ab52645931570cd05782f3771cffe11365fae. Head and prospective merge source trees match (e730eed621896b14b364012001ea5fa73097affc).
The earlier linked-reader normalization remains fixed; this delta does not change core prompt handling. Atomic WebSocket close publication, the original sequential cancel/dispose timeout case, and Telnet startup-operation tracking are corrected. Confirmation added in their existing threads.
Changes remain requested for three P2 cases:
- WebSocketStream can finish disposal after an ordinary data receive without finishing or bounding the close handshake; evidence consolidated into existing thread 4349218768 rather than opening a duplicate.
- New inline: Telnet negotiation completion does not renew the drain idle allowance and can drop queued output from a steadily progressing peer.
- New inline: ReplWebSocketSession cleanup leaves the socket CloseSent/Open if the receive already faulted or never started due to caller cancellation.
Parent personally reran all load-bearing exact-source probes against previous/base controls, including native WebSocket and Pipe endpoints. Three independent bounded audits completed. Full Release solution build with -warnaserror passed; 2,492 tests succeeded, 0 failed, 3 skipped. Applicable CI was green at validation. These passing tests do not cover the cases above. No product edits or merge performed.
… receive Disposing a WebSocketStream treated the end of its pending receive as the end of the close handshake, so a receive that got data instead of the peer's answer left the socket CloseSent with no deadline. ReplWebSocketSession made the same assumption: a receive that had failed left the close unanswered, and a session cancelled before it started left the socket open. Both now finish the handshake themselves within the close's one-second deadline, reading and discarding what the peer still sends until its answer, and abort a connection still not closed; a cancelled session aborts at once, and a failed receive surfaces only once the connection is settled. A Telnet negotiation that completes during disposal now renews the drain allowance as a data write does, so a slow but steady peer no longer loses the output queued behind it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25725adbd2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Approved at exact head 25725adbd22d2c0580c1e66b2350aedb1e5a2e0b, current base 9a7ab52645931570cd05782f3771cffe11365fae. This approval supersedes my previous requests for changes: all reported blockers are fixed. Confirmations and differential evidence are in the existing inline threads.
Reviewed the delta and relevant full shutdown contracts, with three independent bounded revalidations. Parent personally reran the load-bearing exact-source probes: 22 WebSocketStream assertions, 21 native WebSocket session cases, and Telnet framing head/previous/base differential controls (11 head assertions). Previous-head regressions remain reproducible as controls; latest source handles data-before-close, failed/already-ended receives, pre-cancelled callers, total close deadlines, and negotiation drain-progress renewal correctly. No remaining actionable introduced P0–P2 found in the reviewed scope.
Full Release solution build with -warnaserror passed. Full suite: 2,497 succeeded, 0 failed, 3 expected skips (2,500 total). Applicable live CI checks are green, including MCP HTTP Inspector. Prospective merge and head source trees match 273693a0582bd7cded71f54fe61d645095c18402. Product checkout is clean; no product edits or merge performed.
A peer that sent a command and then closed its side of the connection never got the reply: once its input ended, the framing completed its output before the session had processed the command, and the reply was dropped. TelnetFraming.RunAsync now returns at the end of the peer's input with only the session's input ended; the output stays open until the framing is disposed, which sends what remains, so disposing it is now required. Over a WebSocket, the stream also still writes once the peer has closed its side, so that reply reaches it there too. The drain allowance counted one long write as idle time, cutting off a large output a slow peer was still reading; the send loop now writes in slices of at most 4 KB, each one progress. And a read could slip into a WebSocketStream between a synchronous disposal's decision and its end, starting a receive nothing would end; reads are now refused once disposal started.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5c61c24b1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The one-second deadline of a close a cancelled read began only took effect through a pending receive: a cancelled read that got data instead of the peer's answer left the socket CloseSent indefinitely unless something read again or disposed the stream, and a synchronous disposal then dropped the deadline. The deadline now aborts a connection still not closed by itself, and a synchronous disposal aborts any close it leaves unfinished. A read starting while the stream was being disposed asynchronously was still admitted and could race the receive that finishes the close, turning a normal closure into an abort; reads are now refused as soon as that disposal begins, and the finishing receive is started once however many disposals run.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Request changes at a578253: the existing slow-output P2 remains incompletely fixed. Reproduction and previous/base plus same-rate batching controls are in the existing inline thread: #146 (comment)
Verified improvements in this bounded revalidation: delayed command replies survive peer half-close on native Pipes and real WebSocket.CreateFromStream peers, both sessions return 0, and the WebSocket finishes Closed. The previous approved head drops both half-close replies; open-peer controls pass. EOF returns RunAsync while sending remains pending; subsequent caller cancellation settles the sender, validating CTS ownership beyond RunAsync. Startup/established drain, negotiation/data progress, failed opening write, stalled negotiation, concurrent EOF/disposal and repeated-disposal controls pass.
Validation: 22 focused probe outcomes across exact-source head/previous/base controls, including explicit assertions of the residual failure. Sources were hash-checked and rebuilt in an external diagnostic harness, not the production test assembly. No full suite or repository modification. This verdict supersedes the earlier approval for the previous head within the reviewed framing scope.
Fixes #142.
What changes
ConsoleLineReader.ReadLineRemoteAsync), used by hosted sessions and by the local console with advanced input, turned any cancellation of a key read into a null line. A caller cancelling while it waited therefore ended the interactive session as an empty input, with exit code 0, ignoring the default propagation,ExitCodes.Cancelledand the resolver; a prompt waiting there answered its default. Only a reader that stops on its own is now the end of input (asVtKeyReaderreports the end of its stream); a cancelled caller token propagates, and the run reports a cancellation as everywhere else.IReplKeyReaderlets a reader observe the token it is handed through one linked to its own lifetime. The line editor's key read now attributes such a cancellation to the token it was handed (PromptCancellation, from Mask a secret typed in a hosted terminal #139), so a timed text prompt still recognises its deadline and answers its default, and a caller cancellation carries the caller's token.StreamedReplHost.RunSessionAsync,ReplWebSocketSession.RunAsyncandReplTelnetSession.RunAsyncas a cancellation.docs/execution-pipeline.mdgains a "Changed in 0.12" note; the hosted entry points document the exception (stated once onRunSessionAsync, inherited by the transport overloads, including that terminal detection always propagates it); the remote-hosting sample endpoints treat a client that went away (RequestAborted) as the end of the session.StreamedReplHost, leaking its entry in the session registry per connection; it now disposes it, and a disposed host ignores late terminal metadata (such as a TERMINAL-TYPE sent afterexit) that would otherwise register the ended session again.ExitCodes.Cancelled(XML doc, configuration reference, exit-code table) lists a cancellation while an interactive session waits for input; the cancellation section no longer contradicts itself (the cycle's mark carries no code, the run ends as a cancellation).Verification
Given_InteractiveCallerCancellation, through a key reader that waits on its token directly or through a linked token:OperationCanceledExceptioncarrying the caller's token;ExitCodes.Cancelled = 73returns 73; a resolver alone seesCancelledand returns 130;Given_TelnetSession(integration): a peer that closes its input ends the session with 0; the ended session is no longer registered; cancelling at the prompt propagates, or returnsExitCodes.Cancelled.Given_ReplSessionIO: metadata updates after a host is disposed leave its session unregistered.TaskCanceledException), the Telnet peer-EOF test (timed out), the registry and post-dispose tests (session still registered).Also fixed here (found while reviewing)
InvalidOperationExceptionunderPromptFallback.Fail), so a dropped connection could confirm a default-yes action. It now throws anOperationCanceledException, as a secret prompt does since Mask a secret typed in a hosted terminal #139; redirected input that ends still usesPromptFallback.ExitCodes.Cancelledor a resolver set; it now goes through them like any later cancellation.TelnetFraming.RunAsyncnow ends only the session's input there, and the output stays open until the framing is disposed (disposing it is now required); over a WebSocket,WebSocketStreamalso writes once the peer has closed its side.exitand when the negotiation cannot be sent. Afterexit,ReplTelnetSession.RunAsyncwaited for the peer to close its side; it now sends its remaining output, stops reading (TelnetFraming.DisposeAsync, without completing a pipe the caller owns) and returns. A peer gone before the opening negotiation no longer leaves the session waiting.WebSocketStreamnow closes the WebSocket when a read is cancelled or the stream is disposed (normal closure, abort if the peer does not answer within a second), and reports WebSocket failures asIOException.TelnetFraming.DisposeAsyncalso waits for an opening negotiation still being written, and gives up on output the peer leaves unread for two seconds (renewed by each completed write), so a peer that stopped reading cannot hold the end of a session.WebSocketStreampublishes its close before starting it, keeps the one-second deadline armed until pending receives end, and aborts on synchronous disposal rather than leaving a receive or close without a deadline.ReplWebSocketSessioncloses normally. It used to cancel its receive (aborting the socket) before closing, so onexitthe peer got aWebSocketException. It now closes, or answers the peer's close, while the receive is pending, under one one-second deadline; a cancelled session still aborts at once.Tests:
Given_HostedPromptDisconnect,Given_HostedSessionCancellation,Given_TelnetFraming,Given_WebSocketStream,Given_WebSocketSession, and newGiven_TelnetSessioncases (exit with the peer connected, reset connection, normal WebSocket closure, unanswered close); each fix started from a test that failed with a successful build, except the drain-before-cancel ordering and the unanswered-close timeout, which are guards. Full suite: 2504 total, 0 failed.Not changed
ReplWebSocketSession's receive loop still completes the host's input when it ends on cancellation, but the caller's and the command's linked tokens are cancelled in registration order, so a prompt's token is already cancelled when it does; no failing case could be built.IAC SBthat never ends, and the output queue of a peer that sends commands but never reads).