Merge TASK-031: Handler error-propagation contract (DR-009)

Author: etr

specs/tasks/M5-routing-lifecycle/TASK-031.md

@@ -8,12 +8,12 @@
 Implement the 6-point error-propagation contract from §5.2 / DR-009 in the dispatch path so any uncaught exception lands at the configured `internal_error_handler` with documented behavior.
 
 **Action Items:**
-- [ ] Wrap handler invocation in dispatch with `try { ... } catch (const std::exception& e) { ... } catch (...) { ... }`.
-- [ ] On `std::exception`: log via `error_logger` (whatever callback the user wired), invoke `internal_error_handler` with `e.what()`, send the resulting response (default 500 if no handler set).
-- [ ] On non-`std::exception`: same path but with message `"unknown exception"`.
-- [ ] If `internal_error_handler` itself throws: log generically, send hardcoded 500 with empty body.
-- [ ] `feature_unavailable` is a `std::runtime_error`; no special status mapping (just lands as a 500 like any other exception).
-- [ ] Document the contract in `webserver.hpp` Doxygen comments (full README pass in M6).
+- [x] Wrap handler invocation in dispatch with `try { ... } catch (const std::exception& e) { ... } catch (...) { ... }`.
+- [x] On `std::exception`: log via `error_logger` (whatever callback the user wired), invoke `internal_error_handler` with `e.what()`, send the resulting response (default 500 if no handler set).
+- [x] On non-`std::exception`: same path but with message `"unknown exception"`.
+- [x] If `internal_error_handler` itself throws: log generically, send hardcoded 500 with empty body.
+- [x] `feature_unavailable` is a `std::runtime_error`; no special status mapping (just lands as a 500 like any other exception).
+- [x] Document the contract in `webserver.hpp` Doxygen comments (full README pass in M6).
 
 **Dependencies:**
 - Blocked by: TASK-027, TASK-030
@@ -29,4 +29,4 @@ Implement the 6-point error-propagation contract from §5.2 / DR-009 in the disp
 **Related Requirements:** PRD-FLG-REQ-002 (sentinel/throw behavior)
 **Related Decisions:** DR-009, §5.2, AR-007
 
-**Status:** Not Started
+**Status:** Done

specs/tasks/_index.md

@@ -113,7 +113,7 @@ Nominally: **13 sequential tasks**, each S–XL. Most other tasks parallelize of
 | TASK-028 | Routing-semantics regression gate | M5 | Done | TASK-027 |
 | TASK-029 | Naming consistency — `stop_and_wait`, `block_ip`/`unblock_ip` | M5 | Done | TASK-014 |
 | TASK-030 | `_handler` suffix renames + `explicit` constructor | M5 | Done | TASK-014 |
-| TASK-031 | Handler error-propagation contract (DR-009) | M5 | Not Started | TASK-027, TASK-030 |
+| TASK-031 | Handler error-propagation contract (DR-009) | M5 | Done | TASK-027, TASK-030 |
 | TASK-032 | Thread-safety contract stress test (DR-008) | M5 | Not Started | TASK-027, TASK-031 |
 | TASK-033 | `create_webserver` builder cleanup | M5 | Not Started | TASK-006, TASK-014 |
 | TASK-034 | Build-flag-independent public API + `webserver::features()` | M5 | Not Started | TASK-003, TASK-019, TASK-033 |

specs/unworked_review_issues/2026-05-11_225150_task-031.md

@@ -30,6 +30,7 @@
 #include <functional>
 #include <limits>
 #include <string>
+#include <string_view>
 #include <utility>
 #include <variant>
 #include <vector>
@@ -43,10 +44,19 @@ namespace httpserver {
 class webserver;
 class http_request;
 
-// TASK-030 / PRD-NAM-REQ-003: the three error-page setters take a function-
-// shaped handler that returns http_response by value, matching the on_*
-// family (detail::lambda_handler) introduced in TASK-025.
+// TASK-030 / PRD-NAM-REQ-003: the not_found_handler and method_not_allowed_handler
+// setters take a function-shaped handler that returns http_response by value,
+// matching the on_* family (detail::lambda_handler) introduced in TASK-025.
 typedef std::function<http_response(const http_request&)> error_handler;
+
+// TASK-031 / DR-009 §5.2: internal_error_handler receives a second argument
+// carrying the originating exception's message — `e.what()` for
+// std::exception, the literal "unknown exception" for non-std throws. The
+// message is a non-owning view; callers MUST NOT capture it past the
+// handler's return (it points into either a caught std::exception's
+// internal storage or a fixed string literal owned by libhttpserver).
+typedef std::function<http_response(const http_request&, std::string_view message)> internal_error_handler_t;
+
 typedef std::function<bool(const std::string&)> validator_ptr;
 typedef std::function<void(const std::string&)> log_access_ptr;
 typedef std::function<void(const std::string&)> log_error_ptr;
@@ -380,7 +390,34 @@ class create_webserver {
          return *this;
      }
 
-     create_webserver& internal_error_handler(error_handler handler) {
+     /**
+      * Set the handler invoked when a registered request handler throws
+      * an uncaught exception.
+      *
+      * Per DR-009 / §5.2 (PRD-FLG-REQ-002), the dispatch path wraps every
+      * handler call in a two-branch try/catch and funnels here:
+      *
+      *  - `std::exception`        -> @p handler is invoked with
+      *                               `e.what()` as the message argument.
+      *  - non-std::exception      -> @p handler is invoked with the
+      *                               literal string `"unknown exception"`.
+      *  - @p handler unset        -> a default 500 with the message in
+      *                               the response body is sent and the
+      *                               event is logged via log_error.
+      *  - @p handler itself throws -> a hardcoded 500 with an EMPTY body
+      *                               is sent and the event is logged via
+      *                               log_error.
+      *
+      * `feature_unavailable` is a `std::runtime_error` subclass: it lands
+      * here with the default 500 status, not a special 503.
+      *
+      * Note: @p handler may be invoked concurrently from multiple MHD
+      * worker threads. It MUST be thread-safe.
+      *
+      * @param handler invoked with (request, message); returns the
+      *                http_response to send (status / headers / body).
+     **/
+     create_webserver& internal_error_handler(internal_error_handler_t handler) {
          _internal_error_handler = std::move(handler);
          return *this;
      }
@@ -528,7 +565,7 @@ class create_webserver {
      bool _tcp_nodelay = false;
      error_handler _not_found_handler = nullptr;
      error_handler _method_not_allowed_handler = nullptr;
-     error_handler _internal_error_handler = nullptr;
+     internal_error_handler_t _internal_error_handler = nullptr;
      file_cleanup_callback_ptr _file_cleanup_callback = nullptr;
      auth_handler_ptr _auth_handler = nullptr;
      std::vector<std::string> _auth_skip_paths;

src/httpserver/create_webserver.hpp

@@ -53,6 +53,7 @@
 #include <set>
 #include <shared_mutex>
 #include <string>
+#include <string_view>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ -305,8 +306,43 @@ class webserver_impl {
     // owning webserver (via `parent`).
     std::shared_ptr<::httpserver::http_response> not_found_page(modded_request* mr) const;
     std::shared_ptr<::httpserver::http_response> method_not_allowed_page(modded_request* mr) const;
-    std::shared_ptr<::httpserver::http_response> internal_error_page(modded_request* mr,
-                                                       bool force_our = false) const;
+    // TASK-031: error-propagation entry point. @p msg carries the
+    // originating exception's text (e.what() for std::exception, the
+    // sentinel "unknown exception" for non-std throws, or a fixed
+    // internal-diagnostic string for the few non-handler-throw call
+    // sites that synthesise a 500 with no exception in flight).
+    //
+    // Behaviour matches DR-009:
+    //   - parent->internal_error_handler set, !force_our:
+    //       invoke it with (*mr->dhr, msg) and return the result.
+    //   - force_our=true: return a hardcoded 500 with an EMPTY body
+    //       (the "double-fault" fallback used when the user handler
+    //       itself threw).
+    //   - otherwise (no handler set, !force_our): return a default
+    //       500 whose body surfaces @p msg, so the unset-handler
+    //       default is informative.
+    //
+    // Throws nothing on its own; if parent->internal_error_handler
+    // throws, that exception propagates to the caller. Callers in the
+    // handler-throw path use run_internal_error_handler_safely() to
+    // contain that double-fault.
+    std::shared_ptr<::httpserver::http_response> internal_error_page(
+        modded_request* mr,
+        std::string_view msg,
+        bool force_our = false) const;
+
+    // TASK-031: log @p msg via parent->log_error if a logger is configured.
+    // Swallows any exception thrown by the logger -- dispatch must never
+    // re-enter the catch from inside its own catch.
+    void log_dispatch_error(std::string_view msg) const;
+
+    // TASK-031: invoke the user-supplied internal_error_handler safely.
+    // On success, returns the response it produced. If the user handler
+    // itself throws, logs generically via log_dispatch_error and returns
+    // a hardcoded empty-body 500.
+    std::shared_ptr<::httpserver::http_response>
+        run_internal_error_handler_safely(modded_request* mr,
+                                          std::string_view msg) const;
     bool should_skip_auth(const std::string& path) const;
     void invalidate_route_cache();
 

src/httpserver/detail/webserver_impl.hpp

@@ -76,6 +76,32 @@ namespace httpserver {
 
 /**
  * Class representing the webserver. Main class of the apis.
+ *
+ * ### Handler error-propagation contract (DR-009 / §5.2 / PRD-FLG-REQ-002)
+ *
+ * Every registered request handler is invoked from the dispatch path under
+ * a two-branch try/catch. The contract:
+ *
+ *   1. The handler call is wrapped in
+ *      `try { ... } catch (const std::exception& e) { ... } catch (...) { ... }`.
+ *   2. On `std::exception`: the message is logged via the configured
+ *      `log_error` callback, then `internal_error_handler` is invoked with
+ *      `e.what()`. The response it returns is sent on the wire (default
+ *      500 with the message in the body when no handler is configured).
+ *   3. On non-`std::exception` (e.g. `throw 42`): same path with the
+ *      message replaced by the literal string `"unknown exception"`.
+ *   4. If `internal_error_handler` itself throws while servicing 2 or 3,
+ *      the failure is logged generically and a hardcoded 500 with an
+ *      EMPTY body is sent. No exception ever escapes into libmicrohttpd.
+ *   5. `feature_unavailable` (a `std::runtime_error` subclass) is NOT
+ *      mapped to a special status: it lands as a generic 500 like any
+ *      other `std::exception`.
+ *   6. The `log_error` callback may be invoked concurrently from multiple
+ *      MHD worker threads; user implementations MUST be thread-safe.
+ *
+ * The contract is the single source of truth for dispatch-time exception
+ * handling; resource implementations are encouraged to throw rather than
+ * synthesise an http_response with a 500 status.
 **/
 class webserver {
  public:
@@ -468,7 +494,7 @@ class webserver {
      const bool tcp_nodelay;
      const error_handler not_found_handler;
      const error_handler method_not_allowed_handler;
-     const error_handler internal_error_handler;
+     const internal_error_handler_t internal_error_handler;
      const file_cleanup_callback_ptr file_cleanup_callback;
      const auth_handler_ptr auth_handler;
      const std::vector<std::string> auth_skip_paths;

src/httpserver/webserver.hpp

@@ -1610,14 +1610,60 @@ std::shared_ptr<http_response> webserver_impl::method_not_allowed_page(detail::m
     }
 }
 
-std::shared_ptr<http_response> webserver_impl::internal_error_page(detail::modded_request* mr, bool force_our) const {
-    if (parent->internal_error_handler != nullptr && !force_our) {
-        return std::make_shared<http_response>(parent->internal_error_handler(*mr->dhr));
-    } else {
+std::shared_ptr<http_response> webserver_impl::internal_error_page(
+    detail::modded_request* mr,
+    std::string_view msg,
+    bool force_our) const {
+    // TASK-031 / DR-009 §5.2 point 4: the double-fault fallback. Used when
+    // the user-supplied internal_error_handler itself threw or when the
+    // belt-and-suspenders site after get_raw_response_with_fallback fires.
+    // The body is intentionally empty and the message is intentionally
+    // ignored.
+    if (force_our) {
         return std::make_shared<http_response>(
-            http_response::string(std::string{constants::GENERIC_ERROR})
+            http_response::empty()
                 .with_status(http_utils::http_internal_server_error));
     }
+    // §5.2 point 2/3: invoke the user handler with the originating message.
+    if (parent->internal_error_handler != nullptr) {
+        return std::make_shared<http_response>(
+            parent->internal_error_handler(*mr->dhr, msg));
+    }
+    // No handler configured: surface the message in the default body so
+    // the unset-handler path is informative for debugging. Operators who
+    // need a fixed body can wire a constant-returning handler.
+    return std::make_shared<http_response>(
+        http_response::string(std::string{msg})
+            .with_status(http_utils::http_internal_server_error));
+}
+
+void webserver_impl::log_dispatch_error(std::string_view msg) const {
+    if (parent->log_error == nullptr) {
+        return;
+    }
+    // A misbehaving user logger must not poison the catch from inside the
+    // catch. Swallow any exception it throws; we have no recovery beyond
+    // dropping the log line.
+    try {
+        parent->log_error(std::string(msg));
+    } catch (...) {
+        // Intentionally suppressed.
+    }
+}
+
+std::shared_ptr<http_response>
+webserver_impl::run_internal_error_handler_safely(
+    detail::modded_request* mr,
+    std::string_view msg) const {
+    try {
+        return internal_error_page(mr, msg, /*force_our=*/false);
+    } catch (...) {
+        // §5.2 point 4: the user handler itself threw. Log generically
+        // and return an empty-body 500. No exception escapes from here.
+        log_dispatch_error("internal_error_handler threw; "
+                           "sending hardcoded empty-body 500");
+        return internal_error_page(mr, "", /*force_our=*/true);
+    }
 }
 
 void webserver_impl::invalidate_route_cache() {
@@ -1871,7 +1917,11 @@ struct MHD_Response* webserver_impl::get_raw_response_with_fallback(detail::modd
     try {
         struct MHD_Response* raw = materialize_response(mr->dhrs.get());
         if (raw == nullptr) {
-            mr->dhrs = internal_error_page(mr);
+            // TASK-031: no exception was thrown, but the body materializer
+            // returned null. Route through the safe internal-error path so
+            // a misbehaving user handler can't escape.
+            mr->dhrs = run_internal_error_handler_safely(
+                mr, "materialize_response returned null");
             raw = materialize_response(mr->dhrs.get());
         }
         return raw;
@@ -1882,9 +1932,19 @@ struct MHD_Response* webserver_impl::get_raw_response_with_fallback(detail::modd
         } catch(...) {
             return nullptr;
         }
+    } catch(const std::exception& e) {
+        log_dispatch_error(std::string("materialize threw: ") + e.what());
+        try {
+            mr->dhrs = run_internal_error_handler_safely(mr, e.what());
+            return materialize_response(mr->dhrs.get());
+        } catch(...) {
+            return nullptr;
+        }
     } catch(...) {
+        log_dispatch_error("materialize threw unknown exception");
         try {
-            mr->dhrs = internal_error_page(mr);
+            mr->dhrs = run_internal_error_handler_safely(mr,
+                                                         "unknown exception");
             return materialize_response(mr->dhrs.get());
         } catch(...) {
             return nullptr;
@@ -2072,7 +2132,12 @@ MHD_Result webserver_impl::finalize_answer(MHD_Connection* connection, struct de
                 // shared_ptr has no operator->*, so call as ((*ptr).*pmf)(...).
                 mr->dhrs = ((*hrm).*(mr->callback))(*mr->dhr);  // copy in memory (move in case)
                 if (mr->dhrs.get() == nullptr || mr->dhrs->get_status() == -1) {
-                    mr->dhrs = internal_error_page(mr);
+                    // TASK-031: no exception was thrown, but the handler
+                    // returned a null/invalid response. Route through the
+                    // safe internal-error path so a misbehaving user
+                    // handler can't escape into libmicrohttpd.
+                    mr->dhrs = run_internal_error_handler_safely(
+                        mr, "handler returned null response");
                 }
             } else {
                 mr->dhrs = method_not_allowed_page(mr);
@@ -2096,23 +2161,31 @@ MHD_Result webserver_impl::finalize_answer(MHD_Connection* connection, struct de
                     mr->dhrs->with_header(http_utils::http_header_allow, header_value);
                 }
             }
-        } catch(...) {
-            // The user-supplied internal_error_handler may itself throw;
-            // fall back to the built-in error page in that case (force_our=true)
-            // so we never let exceptions escape into libmicrohttpd.
-            try {
-                mr->dhrs = internal_error_page(mr);
-            } catch(...) {
-                mr->dhrs = internal_error_page(mr, true);
-            }
+        } catch (const std::exception& e) {
+            // TASK-031 / DR-009 §5.2 point 2: handler threw std::exception.
+            // Log via error_logger, forward e.what() to internal_error_handler.
+            // run_internal_error_handler_safely contains a possible
+            // re-throw from the user handler (point 4).
+            log_dispatch_error(std::string("dispatch: handler threw "
+                                           "std::exception: ") + e.what());
+            mr->dhrs = run_internal_error_handler_safely(mr, e.what());
+        } catch (...) {
+            // §5.2 point 3: handler threw non-std::exception. Same flow
+            // as the std::exception arm but with the sentinel message.
+            log_dispatch_error("dispatch: handler threw unknown exception");
+            mr->dhrs = run_internal_error_handler_safely(mr,
+                                                         "unknown exception");
         }
     } else if (mr->dhrs == nullptr) {
         mr->dhrs = not_found_page(mr);
     }
 
     raw_response = get_raw_response_with_fallback(mr);
     if (raw_response == nullptr) {
-        mr->dhrs = internal_error_page(mr, true);
+        // Belt-and-suspenders: even get_raw_response_with_fallback's
+        // own try/catch couldn't produce a response. Force the empty-body
+        // 500 fallback so MHD always has something to queue.
+        mr->dhrs = internal_error_page(mr, "", /*force_our=*/true);
         raw_response = materialize_response(mr->dhrs.get());
     }
     decorate_mhd_response(raw_response, *mr->dhrs);