fix(cli): apply the documented 600s read timeout, and report a timeout as one (#42)

docs/cli-usage.md

@@ -15,7 +15,7 @@
 | `--https` | Use HTTPS |
 | `--insecure` | Skip TLS certificate verification |
 | `--json` | Output in machine-readable JSON |
-| `--timeout <sec>` | Request timeout in seconds |
+| `--timeout <sec>` | Per-request read timeout in seconds (default: 600). Raise it for long classruns and ATC runs. |
 | `--session-file <path>` | Persist session for lock/write/unlock workflows |
 | `--color` / `--no-color` | Force or disable ANSI color output |
 | `-v` / `-vv` | INFO / DEBUG logging to stderr |

docs/spec2.md

@@ -297,7 +297,7 @@ erpl-adt [global-flags] <command-group> <action> [flags] [args]
 | `-v, --verbose` | Verbose output |
 | `-q, --quiet` | Suppress non-essential output |
 | `--version` | Print version |
-| `--timeout <seconds>` | Request timeout (default: 120) |
+| `--timeout <seconds>` | Per-request read timeout (default: 600) |
 | `--insecure` | Skip TLS certificate verification |
 
 ### 5.2 Command Groups and Actions
@@ -482,7 +482,7 @@ connection:
 defaults:
   package: ZTEST
   transport_prefix: "AI:"
-  timeout: 120
+  timeout: 600
   activate_after_write: true
 
 # For erpl-adt compatibility: multi-repo deployment

include/erpl_adt/adt/adt_session.hpp

@@ -10,12 +10,21 @@
 
 namespace erpl_adt {
 
+// The per-request read timeout, in seconds, when --timeout says nothing.
+// One constant, because two of them drifted: the session 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. ABAP that runs longer keeps running server-side
+// and completes, so giving up early reports a failure for work that
+// succeeded (issue #42).
+inline constexpr int kDefaultReadTimeoutSeconds = 600;
+
 // ---------------------------------------------------------------------------
 // AdtSessionOptions — configuration for the ADT HTTP session.
 // ---------------------------------------------------------------------------
 struct AdtSessionOptions {
     std::chrono::seconds connect_timeout{30};
-    std::chrono::seconds read_timeout{120};
+    std::chrono::seconds read_timeout{kDefaultReadTimeoutSeconds};
     bool disable_tls_verify = false;
     std::chrono::seconds poll_interval{2};
     // SAP logon language for the connection. Sent as the Accept-Language

include/erpl_adt/config/app_config.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <erpl_adt/adt/adt_session.hpp>
 #include <erpl_adt/core/types.hpp>
 
 #include <cstdint>
@@ -36,7 +37,7 @@ struct AppConfig {
     bool json_output = false;
     bool verbose = false;
     bool quiet = false;
-    int timeout_seconds = 600;
+    int timeout_seconds = kDefaultReadTimeoutSeconds;
 };
 
 } // namespace erpl_adt

src/adt/adt_session.cpp

@@ -203,6 +203,60 @@ struct AdtSession::Impl {
         }
     }
 
+    // Shape a transport failure (no HTTP response at all) into an Error.
+    //
+    // A read timeout used to arrive as "HTTP request failed: Failed to read
+    // connection", which named neither the timeout nor the flag that raises
+    // it — and the CLI then invited the user to file a bug for a limit they
+    // had simply hit (issue #42). Worse, the ABAP behind a timed-out
+    // classrun keeps running and usually completes, so the failure is about
+    // our patience, not their work.
+    Error MakeTransportError(const std::string& operation,
+                             const std::string& path,
+                             httplib::Error http_error) const {
+        const auto category = CategoryFromHttpTransportError(http_error);
+
+        // Never reached the server at all: the limit that expired is the
+        // connection timeout, and --timeout would not have helped — saying
+        // otherwise sends people to raise the wrong number.
+        if (http_error == httplib::Error::ConnectionTimeout) {
+            auto error = MakeSessionError(
+                operation, path, std::nullopt,
+                "connection timed out after " +
+                    std::to_string(options.connect_timeout.count()) +
+                    "s (connecting to " + base_url_ + ")",
+                category);
+            if (!error.hint.has_value()) {
+                error.hint = "The server did not answer. Check the host, port "
+                             "and that it is reachable from here.";
+            }
+            return error;
+        }
+
+        // The request was sent and the answer did not arrive in time.
+        if (http_error == httplib::Error::Read ||
+            http_error == httplib::Error::Timeout) {
+            auto error = MakeSessionError(
+                operation, path, std::nullopt,
+                "request timed out after " +
+                    std::to_string(options.read_timeout.count()) +
+                    "s (connecting to " + base_url_ + ")",
+                category);
+            if (!error.hint.has_value()) {
+                error.hint = "Raise it with --timeout <seconds>. Work already "
+                             "started on the server may still be running and "
+                             "may complete — check before repeating it.";
+            }
+            return error;
+        }
+
+        return MakeSessionError(operation, path, std::nullopt,
+                                "HTTP request failed: " +
+                                    httplib::to_string(http_error) +
+                                    " (connecting to " + base_url_ + ")",
+                                category);
+    }
+
     // Check if a request path targets the BW Modeling API.
     static bool IsBwPath(std::string_view path) {
         // "/sap/bw/modeling/" = 17 chars
@@ -318,11 +372,7 @@ struct AdtSession::Impl {
         if (!res) {
             const auto http_error = res.error();
             return Result<HttpResponse, Error>::Err(
-                MakeSessionError("Get", std::string(path), std::nullopt,
-                                 "HTTP request failed: " +
-                                     httplib::to_string(http_error) +
-                                     " (connecting to " + base_url_ + ")",
-                                 CategoryFromHttpTransportError(http_error)));
+                MakeTransportError("Get", std::string(path), http_error));
         }
         LogResponse(res->status, res->headers, res->body);
         CaptureContextId(res->headers);
@@ -348,11 +398,7 @@ struct AdtSession::Impl {
         if (!res) {
             const auto http_error = res.error();
             return Result<HttpResponse, Error>::Err(
-                MakeSessionError("Post", std::string(path), std::nullopt,
-                                 "HTTP request failed: " +
-                                     httplib::to_string(http_error) +
-                                     " (connecting to " + base_url_ + ")",
-                                 CategoryFromHttpTransportError(http_error)));
+                MakeTransportError("Post", std::string(path), http_error));
         }
         LogResponse(res->status, res->headers, res->body);
         CaptureContextId(res->headers);
@@ -378,11 +424,7 @@ struct AdtSession::Impl {
         if (!res) {
             const auto http_error = res.error();
             return Result<HttpResponse, Error>::Err(
-                MakeSessionError("Put", std::string(path), std::nullopt,
-                                 "HTTP request failed: " +
-                                     httplib::to_string(http_error) +
-                                     " (connecting to " + base_url_ + ")",
-                                 CategoryFromHttpTransportError(http_error)));
+                MakeTransportError("Put", std::string(path), http_error));
         }
         LogResponse(res->status, res->headers, res->body);
         CaptureContextId(res->headers);
@@ -405,11 +447,7 @@ struct AdtSession::Impl {
         if (!res) {
             const auto http_error = res.error();
             return Result<HttpResponse, Error>::Err(
-                MakeSessionError("Delete", std::string(path), std::nullopt,
-                                 "HTTP request failed: " +
-                                     httplib::to_string(http_error) +
-                                     " (connecting to " + base_url_ + ")",
-                                 CategoryFromHttpTransportError(http_error)));
+                MakeTransportError("Delete", std::string(path), http_error));
         }
         LogResponse(res->status, res->headers, res->body);
         CaptureContextId(res->headers);
@@ -439,12 +477,7 @@ struct AdtSession::Impl {
             if (!res) {
                 const auto http_error = res.error();
                 return Result<std::string, Error>::Err(
-                    MakeSessionError("FetchCsrfToken", fetch_path,
-                                     std::nullopt,
-                                     "HTTP request failed: " +
-                                         httplib::to_string(http_error) +
-                                         " (connecting to " + base_url_ + ")",
-                                     CategoryFromHttpTransportError(http_error)));
+                    MakeTransportError("FetchCsrfToken", fetch_path, http_error));
             }
             LogResponse(res->status, res->headers, res->body);
             if (res->status != 200) {

src/cli/command_executor.cpp

@@ -6992,7 +6992,7 @@ void PrintTopLevelHelp(const CommandRouter& router, std::ostream& out, bool colo
         {"--https",                 "Use HTTPS"},
         {"--insecure",              "Skip TLS verification (with --https)"},
         {"--json",                  "JSON output"},
-        {"--timeout <sec>",         "Request timeout in seconds"},
+        {"--timeout <sec>",         "Per-request read timeout in seconds (default: 600)"},
         {"--session-file <path>",   "Persist session for lock/write/unlock workflows"},
         {"--color",                 "Force colored output"},
         {"--no-color",              "Disable colored output"},

src/cli/output_formatter.cpp

@@ -202,6 +202,13 @@ void OutputFormatter::PrintJson(const std::string& json) const {
 static constexpr const char* kIssueHint =
     "  Unexpected? Please report it: https://github.com/DataZooDE/erpl-adt/issues";
 
+// ...except where nothing unexpected happened. A read timeout is a limit the
+// caller configured and can raise, and the error already says how; inviting a
+// bug report for it sends people to the tracker for working software (#42).
+static bool WorthReporting(const Error& error) {
+    return error.category != ErrorCategory::Timeout;
+}
+
 void OutputFormatter::PrintError(const Error& error) const {
     if (json_mode_) {
         err_ << error.ToJson() << "\n";
@@ -224,7 +231,9 @@ void OutputFormatter::PrintError(const Error& error) const {
             err_ << "  " << kYellow << "Hint: " << kReset
                  << error.hint.value() << "\n";
         }
-        err_ << kDim << kIssueHint << kReset << "\n";
+        if (WorthReporting(error)) {
+            err_ << kDim << kIssueHint << kReset << "\n";
+        }
         return;
     }
 
@@ -241,7 +250,9 @@ void OutputFormatter::PrintError(const Error& error) const {
     if (error.hint.has_value() && !error.hint->empty()) {
         err_ << "  Hint: " << error.hint.value() << "\n";
     }
-    err_ << kIssueHint << "\n";
+    if (WorthReporting(error)) {
+        err_ << kIssueHint << "\n";
+    }
 }
 
 void OutputFormatter::PrintSuccess(const std::string& message) const {

src/config/config_loader.cpp

@@ -214,7 +214,7 @@ Result<AppConfig, Error> LoadFromCli(int argc, const char* const* argv) {
         .default_value(false)
         .implicit_value(true);
     program.add_argument("--timeout")
-        .help("Timeout in seconds")
+        .help("Per-request read timeout in seconds (default: 600)")
         .scan<'i', int>();
     program.add_argument("--json")
         .help("JSON output")
@@ -380,7 +380,7 @@ AppConfig MergeConfigs(const AppConfig& yaml_base, const AppConfig& cli_override
     if (cli_overrides.quiet) {
         merged.quiet = true;
     }
-    if (cli_overrides.timeout_seconds != 600) {
+    if (cli_overrides.timeout_seconds != kDefaultReadTimeoutSeconds) {
         merged.timeout_seconds = cli_overrides.timeout_seconds;
     }
     if (cli_overrides.log_file.has_value()) {

src/main.cpp

@@ -108,7 +108,7 @@ void PrintMcpHelp(std::ostream& out) {
     out << "  --language <iso>     SAP logon language (2-letter ISO, e.g. EN, DE; default: EN)\n";
     out << "  --https              Use HTTPS\n";
     out << "  --insecure           Skip TLS verification (with --https)\n";
-    out << "  --timeout <sec>      Request timeout in seconds\n";
+    out << "  --timeout <sec>      Per-request read timeout in seconds (default: 600)\n";
     out << "  -v                   Verbose logging (INFO level)\n";
     out << "  -vv                  Debug logging (DEBUG level)\n\n";
     out << "TRANSPORT\n";

test/adt/test_adt_session.cpp

@@ -1,6 +1,7 @@
 #include <catch2/catch_test_macros.hpp>
 
 #include <erpl_adt/adt/adt_session.hpp>
+#include <erpl_adt/config/app_config.hpp>
 #include "../../test/mocks/mock_adt_session.hpp"
 
 #include <httplib.h>
@@ -1011,3 +1012,73 @@ TEST_CASE("AdtSession: ResetStatefulSession clears cookies and CSRF token", "[ad
     CHECK(j.contains("cookies"));
     CHECK(j["cookies"].empty());
 }
+
+// ===========================================================================
+// Read timeout: the default, and how exceeding it is reported (#42)
+// ===========================================================================
+
+TEST_CASE("AdtSessionOptions: the default read timeout is the documented one",
+          "[adt][session]") {
+    // These two drifted apart once already: the session defaulted to 120s
+    // while AppConfig documented 600, and because both CLI entry points only
+    // assign read_timeout when --timeout is passed, 120s was what every call
+    // actually got. A classrun longer than that failed while the ABAP ran on
+    // to completion server-side.
+    CHECK(AdtSessionOptions{}.read_timeout ==
+          std::chrono::seconds(kDefaultReadTimeoutSeconds));
+    CHECK(AppConfig{}.timeout_seconds == kDefaultReadTimeoutSeconds);
+}
+
+TEST_CASE("AdtSession: a read timeout is reported as a timeout, naming --timeout",
+          "[adt][session][live]") {
+    // "HTTP request failed: Failed to read connection" said nothing about a
+    // timeout, named no flag, and was followed by an invitation to file a bug
+    // — for a limit the caller can simply raise.
+    httplib::Server svr;
+    svr.Get("/sap/bc/adt/slow", [&](const httplib::Request&,
+                                    httplib::Response& res) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(1500));
+        res.set_content("<ok/>", "text/xml");
+    });
+
+    LocalServer server(svr);
+    auto client = SapClient::Create("001");
+    REQUIRE(client.IsOk());
+    AdtSessionOptions opts;
+    opts.connect_timeout = std::chrono::seconds{5};
+    opts.read_timeout = std::chrono::seconds{1};
+    AdtSession session("127.0.0.1", static_cast<uint16_t>(server.Port()), false,
+                       "testuser", "testpass", client.Value(), opts);
+
+    auto result = session.Get("/sap/bc/adt/slow");
+    REQUIRE(result.IsErr());
+    const auto& error = result.Error();
+    CHECK(error.category == ErrorCategory::Timeout);
+    CHECK(error.message.find("timed out after 1s") != std::string::npos);
+    REQUIRE(error.hint.has_value());
+    CHECK(error.hint->find("--timeout") != std::string::npos);
+}
+
+TEST_CASE("AdtSession: an unreachable server is not blamed on the read timeout",
+          "[adt][session][live]") {
+    // Whether a closed port is refused (Linux) or quietly dropped until the
+    // connect timeout expires (Windows), the request never reached the
+    // server — so the read timeout is not the number that ran out, and
+    // --timeout is not the flag that would have helped.
+    auto client = SapClient::Create("001");
+    REQUIRE(client.IsOk());
+    AdtSessionOptions opts;
+    opts.connect_timeout = std::chrono::seconds{2};
+    opts.read_timeout = std::chrono::seconds{2};
+    // Port 1 on loopback: nothing listens, so this is refused, not slow.
+    AdtSession session("127.0.0.1", 1, false, "testuser", "testpass",
+                       client.Value(), opts);
+
+    auto result = session.Get("/sap/bc/adt/test");
+    REQUIRE(result.IsErr());
+    const auto& error = result.Error();
+    CHECK(error.message.find("request timed out after") == std::string::npos);
+    if (error.hint.has_value()) {
+        CHECK(error.hint->find("--timeout") == std::string::npos);
+    }
+}