Bug 5360: FwdState::noteDestinationsEnd() assertion "err" (#1767)

    FATAL: assertion failed: FwdState.cc:660: "err"

When FwdState decides to re-forward a request, it forgets the original
response[^1] but does not create an error object. Since commit e2bbd3b,
FwdState::noteDestinationsEnd() correctly assumed that we only idly wait
for additional destinations after encountering a problem, but
incorrectly asserted that past problems imply error object existence.

Now Squid generates an HTTP 502 (Bad Gateway) response while setting
%err_code/%err_detail to ERR_CANNOT_FORWARD/REFORWARD_TO_NONE.

TunnelStateData::noteDestinationsEnd() code is similar, but it probably
does not suffer from the same bug because an error object is created
before every retryOrBail() call, and there is no re-forwarding code that
forgets an HTTP error response without creating an error. Those
invariants are not well tracked, and this change mimics FwdState changes
in TunnelStateData just in case and to keep the two methods in sync. In
those hypothetical cases, ERR_CANNOT_FORWARD/RETRY_TO_NONE is logged.

[^1]: Long-term we probably want to preserve that original response so
that we do not have to replace it with a generated error, but doing so
requires significant refactoring and is outside this minimal fix scope.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index d92cacaff83dd40ae37579b7108719087fb93cc1..1e1e37cf2917264ce47b331c3ceffdfc7add1a62 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -657,7 +657,13 @@ FwdState::noteDestinationsEnd(ErrorState *selectionError)
     }
 
     // destinationsFound, but none of them worked, and we were waiting for more
-    assert(err);
+    debugs(17, 7, "no more destinations to try after " << n_tries << " failed attempts");
+    if (!err) {
+        const auto finalError = new ErrorState(ERR_CANNOT_FORWARD, Http::scBadGateway, request, al);
+        static const auto d = MakeNamedErrorDetail("REFORWARD_TO_NONE");
+        finalError->detailError(d);
+        fail(finalError);
+    } // else use actual error from last forwarding attempt
     stopAndDestroy("all found paths have failed");
 }
 

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 72487ba55eb183bf17bf9941e5e6b1c9ed567fa6..2b4555282609cb0a8fe881b41229cc08aa7c7b4a 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -1364,7 +1364,15 @@ TunnelStateData::noteDestinationsEnd(ErrorState *selectionError)
     }
 
     // destinationsFound, but none of them worked, and we were waiting for more
-    assert(savedError);
+    debugs(17, 7, "no more destinations to try after " << n_tries << " failed attempts");
+    if (!savedError) {
+        // retryOrBail() must be preceded by saveError(), but in case we forgot:
+        const auto finalError = new ErrorState(ERR_CANNOT_FORWARD, Http::scBadGateway, request.getRaw(), al);
+        static const auto d = MakeNamedErrorDetail("RETRY_TO_NONE");
+        finalError->detailError(d);
+        saveError(finalError);
+    } // else use actual error from last forwarding attempt
+
     // XXX: Honor clientExpectsConnectResponse() before replying.
     sendError(savedError, "all found paths have failed");
 }