Fix stuck-write give-up path spinning forever instead of actually closing

shutdown() only sets shutDown=true and expects the fifo to drain
normally before eventReceived()'s keepAlive check
(!shutDown || (fifo.size() > 0 && !readHangup)) lets the connection
close - correct for a graceful QUIT/LOGOUT reply that just needs a few
more successful writes to flush, but exactly backwards for the give-up
path: the whole reason that path was reached is that the front of the
fifo can never be sent. Leaving it queued meant keepAlive stayed true
forever, resetSocket() kept re-arming EPOLLWRNORM because fifo wasn't
empty, the very next write attempt hit the identical stall
immediately, and shutdown() got called again - forever, at whatever
rate the kernel could fire EPOLLWRNORM.

Confirmed live on a real internet-facing server (Zephora's game
server, an ordinary port-scanner connection was enough to trigger it):
one socket logged "consecutive writes made zero progress - giving up
and shutting down" 2.4 million+ times in under 4 milliseconds,
pegging a worker thread at 100% CPU indefinitely. The give-up path now
clears the undeliverable fifo entry before calling shutdown(), so
keepAlive correctly evaluates to just !shutDown (now false) on the
next cycle and the connection actually tears down.
This commit is contained in:
Brad Arant committed 2026-09-06 19:41:07 +00:00
1 parent bee8b92a6d
commit cfc0b19418
1 file changed
+20
+20
View File
@@ -272,6 +272,26 @@ namespace core
// the confirmed real damage (a 55GB, then 12.9GB debug log) // the confirmed real damage (a 55GB, then 12.9GB debug log)
// this caused before being noticed. // this caused before being noticed.
coreutils::Log(coreutils::LOG_EXCEPT) << "Socket " << getDescriptor() << ": " << consecutiveWriteStalls << " consecutive writes made zero progress - giving up and shutting down."; coreutils::Log(coreutils::LOG_EXCEPT) << "Socket " << getDescriptor() << ": " << consecutiveWriteStalls << " consecutive writes made zero progress - giving up and shutting down.";
// shutdown() alone only sets shutDown=true and expects the
// fifo to drain normally before the connection actually
// closes (see eventReceived()'s keepAlive: "!shutDown ||
// (fifo.size() > 0 && !readHangup)") - exactly backwards
// for THIS path, since the whole reason we're here is that
// the front of the fifo can never be sent. Leaving it
// queued meant keepAlive stayed true forever, resetSocket()
// kept re-arming EPOLLWRNORM because fifo.size() > 0, the
// very next write attempt hit this identical stall
// immediately, and shutdown() got called again - forever,
// at whatever rate the kernel could fire EPOLLWRNORM.
// Confirmed live: 2.4 million+ repeats of this exact log
// line for one socket in under 4 milliseconds, pegging a
// worker thread at 100% CPU indefinitely (a real game
// server, brought down by nothing more than an ordinary
// port-scanner connection). Clearing the undeliverable data
// here lets keepAlive correctly evaluate to just !shutDown
// (now false) on the very next cycle, so the connection
// actually tears down instead of spinning forever.
std::queue<std::string>().swap(fifo);
shutdown("stuck write retry loop - no progress after repeated attempts"); shutdown("stuck write retry loop - no progress after repeated attempts");
} }
} }