diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 6685d53f..c4b60536 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -1504,6 +1504,10 @@ namespace MobileGL::MG_Remote::Client { // One op is outstanding at a time (m_remoteControlMutex), so a report naming any // other seq is a server that has lost track of which op it is running. const auto* progress = envelope->msg_as_SurfaceProgress(); + // An OLDER seq is an op this client already gave up on (its reply budget expired + // while the server's apply thread was blocked - a long GPU stall does it): the + // server is reporting, late, on work it did take. Dropped, like its late reply. + if (progress != nullptr && progress->seq() < frame.seq) continue; if (progress == nullptr || progress->seq() != frame.seq) { MGLOG_E("MG_Remote client: a SurfaceProgress names seq %llu while seq %llu is " "the op outstanding", @@ -1545,6 +1549,19 @@ namespace MobileGL::MG_Remote::Client { return MOBILEGL_ERR_PROTOCOL_MISMATCH; } const ::MobileGL::Wire::SurfaceReply* wire = envelope->msg_as_SurfaceReply(); + if (wire->seq() < frame.seq) { + // THE LATE ANSWER TO AN OP THIS CLIENT TIMED OUT ON. One op is outstanding at a + // time, so every reply older than the one owed is exactly that: the server + // answered after the budget expired. Treating it as a protocol mismatch left the + // session one reply behind for good - every later op read its predecessor's + // answer and failed, which wedged a client for the rest of its life after one + // multi-second GPU stall. Drop it and keep waiting for ours. + MGLOG_W("MG_Remote client: dropping the late SurfaceReply for seq %llu (an op that " + "timed out); still waiting for seq %llu", + static_cast(wire->seq()), + static_cast(frame.seq)); + continue; + } if (wire->seq() != frame.seq) { MGLOG_E("MG_Remote client: SurfaceReply names seq %llu, we asked for %llu", static_cast(wire->seq()), diff --git a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp index f2df6ffe..9a0ce13d 100644 --- a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp @@ -1122,6 +1122,35 @@ TEST(ServerSpawnTest, AFrozenServerReportsNoProgressAndTheOpStillTimesOutAtTheBu EXPECT_FALSE(Client::ClientSession::DeviceLost()) << "a frozen server is not a lost device (5.4)"; } +// THE LATE ANSWER TO AN OP THE CLIENT GAVE UP ON. A server whose apply thread is blocked for +// longer than the reply budget - a GPU hang the driver takes ~10 s to reset does it - answers the +// timed-out op when it comes back. The client used to read that answer as the NEXT op's and call +// it a protocol mismatch, and from then on every op read its predecessor's reply: one long GPU +// stall left the client unable to make a context current for the rest of its life. The late reply +// is now dropped and the next op reads its own. Red once by restoring the mismatch arm: the second +// op comes back MOBILEGL_ERR_PROTOCOL_MISMATCH. +TEST(ServerSpawnTest, AnOpAfterOneThatTimedOutReadsItsOwnReplyAndNotThatLateOne) { + const ScopedEnvironment headless("EGL_PLATFORM", HeadlessEglPlatform()); + const ShortReplyBudgets budgets(1000); + + Session session; + ASSERT_TRUE(Bring("latereply", &session)); + ScopedSessionCleanup cleanup{session}; + ASSERT_EQ(Handshake(session), MOBILEGL_OK); + ASSERT_EQ(::kill(session.server.pid, SIGSTOP), 0); + Server::SurfaceControlFrame first = PbufferCreation(); + const MobileGLResult timedOut = Client::ClientSessionInstance().RunRemoteSurfaceControlFrame(first); + (void)::kill(session.server.pid, SIGCONT); + ASSERT_EQ(timedOut, MOBILEGL_ERR_TIMEOUT) << "the frozen server's op was not timed out"; + + Server::SurfaceControlFrame second = PbufferCreation(); + second.surface = 2; + const MobileGLResult rc = Client::ClientSessionInstance().RunRemoteSurfaceControlFrame(second); + EXPECT_EQ(rc, MOBILEGL_OK) << "the op after a timed-out one came back rc=" << static_cast(rc) + << ": it read the late reply of the op before it"; + EXPECT_FALSE(Client::ClientSession::DeviceLost()) << "a late reply is not a lost device"; +} + TEST(ServerSpawnTest, TheHandshakeCarriesTwoDifferentProcessIdsAndNotTwoZeroes) { // CONTRACT-P6 4.3. Hello::pid was hard-coded 0 and Welcome::serverPid ECHOED it back, so the // handshake carried 0 and 0 and named nothing at all. Both now carry real ids, which makes