From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed, 24 Jun 2026 07:45:24 +0000 (+0000)
Subject: Remove doomed attempts to reforward pinned requests (#2449)
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3567505fc8c152f63acf4a9588e93312f7f2f584;p=thirdparty%2Fsquid.git

Remove doomed attempts to reforward pinned requests (#2449)

    AsyncJob.cc(100) mustStop: Ftp::Relay will stop, reason: exception
    ...
    FATAL: check failed: !request->pinnedConnection()
        exception location: FwdState.cc(1119) connectStart

Since February 2019 commit 3dde9e52, retries of "pinned" requests[^1]
using a new Squid-to-server connections are meant for FTP and
connection-based authentication traffic only. Back then, we agreed that
such re-pinning is wrong[^2], and different mechanisms should be used to
handle forwarding errors in those cases, but we preserved the
corresponding functionality in fear of breaking "working" cases[^3]. On
that preserved code path, `request->pinnedConnection()` is true at
pinnedCanRetry() check time.

Since September 2019 commit daf80700, connectStart() throws if
`request->pinnedConnection()` is true, effectively breaking retries for
nearly all preserved cases mentioned above! The only pinned retries that
could hypothetically continue working after that commit are those where
ConnStateData job had been destroyed between pinnedCanRetry() and
connectStart(), and those rare cases are not interesting since
ConnStateData destruction ought to abort request forwarding anyway.

Both 2019 commits are present in v5.0.1 and beyond. We are not aware of
any associated problems. Thus, our fears were excessive, and we can
remove code that preserves problematic functionality. If we discover a
need to retry pinned FTP or connection-authenticated requests via a new
connection, we will add mechanisms to support that functionality.

Removing this code also helps avoid FATAL errors observed during recent
FTP work.

CONNECT tunnels
---------------

Existing tunnel.cc code ignores `request->flags.pinned` when deciding
whether to retry a failed tunneling attempt. This change does not affect
the corresponding "check pinned connections" TODO in that tunneling
code. FWIW, that TODO is limited to tunnelStart(); switchToTunnel()
cases already do not retry due to a "committed to server" retry ban on
their code path. It is likely that pinned retries should be disabled in
tunneled cases as well, but that adjustment deserves a dedicated
analysis/change.

[^1]: Here, a "pinned" request is, roughly speaking, a request sent by
Squid over a Squid-to-server connection that was kept/provided by a
client-to-Squid connection manager (ConnStateData). See usePinned().

[^2]: "automatically re-opening a PINNED connection from the middleware
is invalid in todays networks" at
https://github.com/squid-cache/squid/pull/351#discussion_r250015937

[^3]: "keep re-pinning as is" at
https://github.com/squid-cache/squid/pull/351#discussion_r253757986
---

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 62c33a67f8..479fd1082a 100644
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -708,7 +708,7 @@ FwdState::checkRetry()
     if (exhaustedTries())
         return false;
 
-    if (request->flags.pinned && !pinnedCanRetry())
+    if (request->flags.pinned)
         return false;
 
     if (!EnoughTimeToReForward(start_t))
@@ -1323,7 +1323,7 @@ FwdState::reforward()
 
     debugs(17, 3, e->url() << "?" );
 
-    if (request->flags.pinned && !pinnedCanRetry()) {
+    if (request->flags.pinned) {
         debugs(17, 3, "pinned connection; cannot retry");
         return 0;
     }
@@ -1418,28 +1418,6 @@ FwdState::exhaustedTries() const
     return n_tries >= Config.forward_max_tries;
 }
 
-bool
-FwdState::pinnedCanRetry() const
-{
-    assert(request->flags.pinned);
-
-    // pconn race on pinned connection: Currently we do not have any mechanism
-    // to retry current pinned connection path.
-    if (pconnRace == raceHappened)
-        return false;
-
-    // If a bumped connection was pinned, then the TLS client was given our peer
-    // details. Do not retry because we do not ensure that those details stay
-    // constant. Step1-bumped connections do not get our TLS peer details, are
-    // never pinned, and, hence, never reach this method.
-    if (request->flags.sslBumped)
-        return false;
-
-    // The other pinned cases are FTP proxying and connection-based HTTP
-    // authentication. TODO: Do these cases have restrictions?
-    return true;
-}
-
 time_t
 FwdState::connectingTimeout(const Comm::ConnectionPointer &conn) const
 {
diff --git a/src/FwdState.h b/src/FwdState.h
index 1427b928eb..1dc2d4e3b7 100644
--- a/src/FwdState.h
+++ b/src/FwdState.h
@@ -161,10 +161,6 @@ private:
 
     void usePinned();
 
-    /// whether a pinned to-peer connection can be replaced with another one
-    /// (in order to retry or reforward a failed request)
-    bool pinnedCanRetry() const;
-
     template <typename StepStart>
     void advanceDestination(const char *stepDescription, const Comm::ConnectionPointer &conn, const StepStart &startStep);
 
diff --git a/src/tunnel.cc b/src/tunnel.cc
index 5da201b6bd..041ae0be59 100644
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -517,7 +517,7 @@ TunnelStateData::checkRetry()
     if (request->hier.peer_reply_status != Http::scNone && !Http::IsReforwardableStatus(request->hier.peer_reply_status))
         return "received HTTP status code is not reforwardable";
 
-    // TODO: check pinned connections; see FwdState::pinnedCanRetry()
+    // TODO: check pinned connections; see request->flags.pinned in FwdState::checkRetry()
     return nullptr;
 }