From eb5c6b53336ace3cae0d97f35ea1ee8f6c898baa Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 16 Jun 2026 17:03:25 -0400 Subject: [PATCH 1/2] Do not attempt to reforward pinned requests ... to better tolerate Ftp::Relay::sendCommand() and similar exceptions. ERROR: Squid BUG: assurance failed: ... exception location: FtpClient.cc(835) writeCommand AsyncJob.cc(145) callException: assurance failed ... AsyncJob.cc(100) mustStop: Ftp::Relay will stop, reason: exception ... FATAL: check failed: !request->pinnedConnection() exception location: FwdState.cc(1119) connectStart When branch-added Ftp::Client::writeCommand() assertions throw, the worker dies instead of just killing the affected ftp_port transaction. `JobDialer::dial()` catches the BUG exception, as expected, but the worker still dies because `Client::swanSong()` cleanup code calls `FwdState::handleUnregisteredServerEnd()` which attempts to retry the failed FTP transaction despite true `request->pinnedConnection()`, which triggers another, this time uncaught, exception in `connectStart()`. Also documented suspicions that `FwdState::pinnedCanRetry()` should _always_ return false and, hence, should be removed (instead of adding more and more code/conditions when that method returns false). ---- Cherry-picked osd-7-sanitize-ftp-command commit becd5b4a that was reverted on that branch, so that we can use a dedicated branch to finish this work. --- src/FwdState.cc | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/FwdState.cc b/src/FwdState.cc index 62c33a67f84..5227f8e7e4a 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -1435,6 +1435,31 @@ FwdState::pinnedCanRetry() const if (request->flags.sslBumped) return false; + // Currently, all retries call useDestinations() which never calls + // usePinned() and usually calls connectStart(). The latter asserts that + // request->pinnedConnection() is nil. + if (request->pinnedConnection()) + return false; + + // XXX: The only case where the condition above is false despite true + // `request->flags.pinned` is when HttpRequest::clientConnectionManager job + // disappears. However, in that case, we should probably return `false` + // anyway to avoid retries with a gone client! + + // XXX: FwdState never re-pins a failed connection. In other words, if + // FwdState fails while using a pinned connection, it never pins a new + // connection to the same HttpRequest::clientConnectionManager job. 2013 + // commit 693cb033 removed most re-pinning code. 2019 commit 3dde9e52 + // probably assumed that "FTP proxying and connection-based HTTP + // authentication" cases mentioned below might re-pin, so it added this + // method to avoid breaking those alleged cases, but that assumption was + // probably wrong then and/or is wrong now. + + // TODO: Confirm that "connection-based HTTP authentication" does not need + // some other fix and remove this method. Also adjust peer selection code to + // avoid computing/returning regular destinations (that cannot be used) + // after selecting a PINNED connection. + // The other pinned cases are FTP proxying and connection-based HTTP // authentication. TODO: Do these cases have restrictions? return true; From a3eac80e38aad9f7a1098afba72b80da3808385f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 19 Jun 2026 12:20:39 -0400 Subject: [PATCH 2/2] Do not pretend that we retry pinned non-CONNECT requests Since February 2019 commit 3dde9e52, retries of "pinned" requests[^1] using a new Squid-to-server connections are meant for FTP and connection-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 serious need to retry FTP or connection-authenticated requests via a new connection, we will add proper 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 --- src/FwdState.cc | 51 ++----------------------------------------------- src/FwdState.h | 4 ---- src/tunnel.cc | 2 +- 3 files changed, 3 insertions(+), 54 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 5227f8e7e4a..479fd1082a5 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,53 +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; - - // Currently, all retries call useDestinations() which never calls - // usePinned() and usually calls connectStart(). The latter asserts that - // request->pinnedConnection() is nil. - if (request->pinnedConnection()) - return false; - - // XXX: The only case where the condition above is false despite true - // `request->flags.pinned` is when HttpRequest::clientConnectionManager job - // disappears. However, in that case, we should probably return `false` - // anyway to avoid retries with a gone client! - - // XXX: FwdState never re-pins a failed connection. In other words, if - // FwdState fails while using a pinned connection, it never pins a new - // connection to the same HttpRequest::clientConnectionManager job. 2013 - // commit 693cb033 removed most re-pinning code. 2019 commit 3dde9e52 - // probably assumed that "FTP proxying and connection-based HTTP - // authentication" cases mentioned below might re-pin, so it added this - // method to avoid breaking those alleged cases, but that assumption was - // probably wrong then and/or is wrong now. - - // TODO: Confirm that "connection-based HTTP authentication" does not need - // some other fix and remove this method. Also adjust peer selection code to - // avoid computing/returning regular destinations (that cannot be used) - // after selecting a PINNED connection. - - // 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 1427b928ebb..1dc2d4e3b70 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -161,10 +161,6 @@ class FwdState: public RefCountable, public PeerSelectionInitiator 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 void advanceDestination(const char *stepDescription, const Comm::ConnectionPointer &conn, const StepStart &startStep); diff --git a/src/tunnel.cc b/src/tunnel.cc index 5da201b6bd6..041ae0be595 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; }