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 abae1d6a5ac471e31abe31018534cdadc7194f3d..34cbd873ca17aa90113875661ccef7c64ec55a8a 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -647,7 +647,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 dca34bdc5d492d4f66c072c3580acf8ef49fe20a..81cd72cbb7688702738884dfd9107c1dc246c006 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");
 }