fix(cli): apply the documented 600s read timeout, and report a timeout as one (#42) - #56
Merged
Conversation
…t as one Closes #42. AdtSessionOptions defaulted to 120s while AppConfig::timeout_seconds documented 600, and since both CLI entry points assign read_timeout only when --timeout is passed, 120s was what every call actually got — the MCP server included. Anything slower failed at 121s while the ABAP ran on server-side and completed, so the caller saw a failure for work that had in fact succeeded and had to go query the result to find out. There is now one constant, kDefaultReadTimeoutSeconds, behind both. The failure said "HTTP request failed: Failed to read connection", which named neither the timeout nor the flag that raises it, and the CLI then invited a bug report for a limit the caller had simply hit. The five sites that shaped a transport failure by hand now share MakeTransportError, which for the timeout family says how long it waited, hints at --timeout, and warns that the server-side work may still complete. PrintError drops the issue-tracker line for that category — nothing unexpected happened. Verified against a4h with the class from the issue: `object run ZCL_ERPL_WAIT` (WAIT UP TO 170 SECONDS) now prints WAITED 170 and exits 0 after 170s, where it used to fail at 121s. With --timeout 5 it exits 10 with "request timed out after 5s" and the hint.
Windows caught it: connecting to a closed port there is not refused, it is dropped until the connect timeout runs out, and the message then said 'request timed out after 600s' and pointed at --timeout — neither of which is the number that expired nor the flag that would help. A connection timeout now says so and suggests checking reachability.
…tput Left over from the silent-success fix: test_20 still asserted exit 0 with 'does not exist' as console output, which is exactly the reading that fix removed, while test_24 asserts the opposite. The contract stays in test_24; test_20 now pins that the message still names the cause.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #42.
Two defaults, and the lower one won
AdtSessionOptions::read_timeoutdefaulted to 120s whileAppConfig::timeout_secondsdocumented 600. Both CLI entry points assignread_timeoutonly underif (HasFlag(args, "timeout")), so unless--timeoutwas passed the session kept its own 120s — for the CLI, the legacy deploy path and the MCP server alike.Anything slower failed at 121s. The ABAP behind it keeps running server-side and normally completes, so the caller was told the work failed when it had succeeded, and had to go query the result to find out which.
There is now one constant,
kDefaultReadTimeoutSeconds, behind both.A timeout that says so
The five places that shaped a transport failure by hand now share one
MakeTransportError. Only the timeout family gets the new wording; every other transport error is byte-for-byte what it was.PrintErrordrops the issue-tracker line forErrorCategory::Timeout— that invitation is for surprises, not for a limit the caller configured and can raise.--timeout's help now states the default and that it is a per-request read timeout, in all three places that describe it, plusdocs/spec2.md, which still said 120.Verified live
Against a4h, with exactly the class from the issue (
WAIT UP TO 170 SECONDS):1355 unit tests pass, 5 of them new: the two defaults agreeing, a real socket timing out through the
LocalServerharness, a connection refusal not being dressed up as a timeout, and the issue-hint carve-out in both plain and color mode. Integration smoke suite passes.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.