diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 3fb7278c..1817ce21 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -1112,7 +1112,19 @@ namespace MobileGL::MG_Pipe { // below and this aborts by name on the first glBufferData that carries data. const Uint64 before = MG_Remote::Client::ClientWireRecordsEmitted(); MGPipeEmitResourceSubData(buffer, 0, static_cast(buffer.GetSize())); - if (MG_Remote::Client::ClientWireRecordsEmitted() == before) { + // EXCEPT ON A SESSION THAT HAS STOPPED CARRYING RECORDS (P12). Once the device-lost + // latch is set every verb is DECLINED by design (CONTRACT-P6 5.3, ClientSession.cpp + // :1837) - the resource_subdata above included - so "it emitted nothing" is the latch + // working, not bytes this client forgot to carry: there is no server left for them to + // exist on, and P12's own exit gate (a) is that killing the server gives a CLEAN + // device-lost latch rather than a corpse. Aborting here turned every clean loss into a + // client SIGABRT at its next glBufferData that carries data. + // + // THE SELF-CHECK KEEPS ITS FULL FORCE ON A LIVE SESSION, which is the case it was + // written for: there, a follow-up that emits nothing is still the R-13.3 gap it names + // - and DeviceLost() is false. + if (MG_Remote::Client::ClientWireRecordsEmitted() == before && + !MG_Remote::Client::ClientSession::DeviceLost()) { // UncarriedInitialBytes, not a second word for the same family: WireTables.cpp:418 // already dies of exactly this - a respecify that crossed with no initial bytes - // under that name, and two words for one family is the vocabulary drift a6 diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index fc14e81a..75b796f0 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -1577,6 +1577,43 @@ namespace MobileGL::MG_Remote::Client { Uint64 replyBytes = 0; session.EmitAndWait(op, payload, bytes, nullptr, 0, &result, sizeof(result), &status, &replyBytes); + // A DECLINED SYNC REPLY IS AN ANSWER, NOT A CORRUPTION (P12). ReplySink opens the + // three-value space with "DECLINED IS A REAL ANSWER, not a failure" + // (PipeWireCodec.h:457-460) and the escape rows are held to it the other way round - + // "ERROR IS NOT A DECLINE", an escape may not fold 2 into false + // (WireTables.cpp:446-450). This site folded 1 AND 2 into one abort. + // + // BOTH VALUES OF 1 ARE REACHABLE WITHOUT A BYTE OF CORRUPTION: + // - the SERVER declines these two rows by design whenever it cannot answer them - + // PostReply(op, seq, ok ? kStatusOk : kStatusDeclined, ok ? &result : nullptr, + // ok ? sizeof(result) : 0) at PipeWireCodec.cpp:1820-1830, which is a legitimate + // zero-length answer to a kWaitReply row (R-5); + // - the CLIENT produces the same status itself once the session stops carrying + // records: the device-lost latch (ClientSession.cpp:1837) and a doorbell that died + // during teardown (ClientSession.cpp:2025, where the comment says outright that + // "the verb did not happen" is reported as DECLINED and not as ERROR). + // + // AND THE NO-OP IS THE ONE THIS FRONTEND ALREADY GIVES for a fence it cannot wait on: + // GL_Sync.cpp:103-106 answers GL_ALREADY_SIGNALED for a sync with no backend handle, + // PipeApplier.cpp:175-177 answers GL_ALREADY_SIGNALED for a fence with no native + // object (a fence that provably did no work), and DirectGLES.cpp:16489 does the same + // for an absent sync. A lost device is that same class of fact. + // + // WHAT ABORTING HERE COSTS, MEASURED: after a clean device loss Minecraft 26.2's next + // glClientWaitSync - the chunk-upload fences - reached this line with DECLINED and died + // of Fatal{ProtocolCorruption, "Fence.reply"}, rc=134, instead of the no-op the design + // promises ("GL calls become no-ops", design/08-runtime-and-platform.md:33). + if (status == Wire::ReplySink::kStatusDeclined) { + MGLOG_E_ONCE("MG_Remote client: the peer DECLINED a %s reply - the verb did not " + "happen (a lost device, or a teardown); answering the no-op the " + "frontend uses for a fence it cannot wait on", + Wire::WireOpName(op)); + return op == MG_Pipe::MGPWireOp::FenceWait ? static_cast(GL_ALREADY_SIGNALED) + : 1u; + } + // ERROR AND A SHORT OK ARE STILL THE PROTOCOL CORRUPTIONS THEY ALWAYS WERE: a + // transport fault is not the server saying no, and an OK answer that arrived empty is + // a reply nobody can read. if (status != Wire::ReplySink::kStatusOk || replyBytes != sizeof(result)) Wire::WireProtocolFatal("Fence.reply", "missing or malformed sync result"); return result; diff --git a/MobileGL/MG_Remote/Client/QueryEmit.inc b/MobileGL/MG_Remote/Client/QueryEmit.inc index 2e236fd5..a506e0b1 100644 --- a/MobileGL/MG_Remote/Client/QueryEmit.inc +++ b/MobileGL/MG_Remote/Client/QueryEmit.inc @@ -16,6 +16,30 @@ Int32 status = Wire::ReplySink::kStatusError; Uint64 returned = 0; session.EmitAndWait(op, request, size, nullptr, 0, &result, sizeof(result), &status, &returned); + // A DECLINED QUERY IS AN ANSWER TOO (P12), for ReadFenceReply's reason and on the + // same three-value space (PipeWireCodec.h:457-460). The server declines these three + // rows by design whenever it cannot answer them - QueryAvailable :1841-1847, + // QueryResult :1848-1854 and QueryTimestamp :1855-1859 all post kStatusDeclined with + // a zero-length payload - and the client produces the same status once the session + // stops carrying records (ClientSession.cpp:1837 is the device-lost latch, :2025 a + // doorbell that died during teardown). `returned != sizeof(result)` is guaranteed for + // such an answer, so the guard below used to abort on every one of them. + // + // THE ZEROED RESULT IS THE NO-OP EACH ROW ALREADY HAS, and it is what a query has + // when there is no answer YET rather than no answer ever: + // - QueryAvailable -> 0 -> false, "the result is not available" (the same answer an + // unfinished query gives - GL_QUERY_RESULT_AVAILABLE exists to be told that); + // - QueryResult -> Produced = 0 -> the caller returns false with *value untouched + // (:70 spells the healthy analogue of the same answer); + // - QueryTimestamp -> 0, no timestamp to report. + // ERROR stays fatal below, and so does an OK reply that arrived short. + if (status == Wire::ReplySink::kStatusDeclined) { + MGLOG_E_ONCE("MG_Remote client: the peer DECLINED a %s reply - the verb did not " + "happen (a lost device, or a teardown); answering the row's " + "'no answer yet' value", + Wire::WireOpName(op)); + return result; + } if (status != Wire::ReplySink::kStatusOk || returned != sizeof(result)) Wire::WireProtocolFatal("Query.reply", "missing or malformed backend query reply"); return result; diff --git a/MobileGL/MG_Test/Wire/RemoteClientControls.inc b/MobileGL/MG_Test/Wire/RemoteClientControls.inc index d2118fb2..f21066a7 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientControls.inc +++ b/MobileGL/MG_Test/Wire/RemoteClientControls.inc @@ -736,4 +736,130 @@ TEST(RemoteClientControls, ADrawOnASessionWithoutTheRunAheadBitStaysStampedBarri }); ExpectChildSuccess(child); } + +// ===================================================================================== +// P12: a DECLINED reply is an answer, not a corruption +// ===================================================================================== +// +// ReplySink's status space is three-valued and its own header says what the values mean: +// 0 OK, 1 DECLINED IS A REAL ANSWER, 2 ERROR (PipeWireCodec.h:457-460). The escape rows are +// held to the other half of the same rule - "ERROR IS NOT A DECLINE", an escape may not fold +// 2 into `false` (WireTables.cpp:446-450). The fence and query consumers folded BOTH 1 and 2 +// into a single abort (EmitTables.cpp ReadFenceReply, QueryEmit.inc ReadQueryReply), so a +// client that had latched device-lost - where every verb is DECLINED by design, +// ClientSession.cpp:1837 - died of Fatal{ProtocolCorruption, "Fence.reply"} on its next +// glClientWaitSync instead of going quiet the way the design promises ("GL calls become +// no-ops", design/08-runtime-and-platform.md:33). +// +// THE PEER BELOW IS THE SERVER'S OWN DECLINE, not a substitution of the status byte: these +// verbs answer false when the server cannot answer them, and the codec posts exactly that as +// DECLINED with a zero-length payload (PipeWireCodec.cpp:1820-1830 for the two fence rows, +// :1841-1861 for the three query rows). The CLIENT's own decline is pinned separately, by +// LongFenceWaitBudgetStillWakesOnShutdown above. +// +// THE RED, RUN ONCE: take the `if (status == Wire::ReplySink::kStatusDeclined)` arm out of +// ReadFenceReply and the first case below dies of Fatal{ProtocolCorruption, "Fence.reply"} +// instead of asserting the no-op - which is the split client's Minecraft 26.2 crash. +struct DecliningSyncPeer : Codec::WireVerbSink { + Bool OnFenceCreate(const MG_Pipe::MGPHandleOnly&) override { return true; } + Bool OnFenceStatus(const MG_Pipe::MGPHandleOnly&, Uint32&) override { return false; } + Bool OnFenceWait(const MGPFenceWait&, Uint32&) override { return false; } + Bool OnQueryCreate(const MGPQueryDesc&) override { return true; } + Bool OnQueryBegin(const MGPQueryDesc&) override { return true; } + Bool OnQueryCounter(const MGPQueryDesc&) override { return true; } + Bool OnQueryAvailable(const MG_Pipe::MGPHandleOnly&, Uint32&) override { return false; } + Bool OnQueryResult(const MGPQueryResultRequest&, Codec::QueryResultReply&) override { return false; } + Bool OnQueryTimestamp(const MGPTimestampRequest&, Int64&) override { return false; } + void Install() { + const auto rc = Srv::ServerLoopInstance().RunProbeOnApplyThreadForTesting([](void* self) { + auto& decoder = Srv::ServerSessionInstance().Applier().*PeerMember(DecoderTag{}); + decoder.SetVerbSink(static_cast(self)); + return MOBILEGL_OK; + }, this); + if (rc != MOBILEGL_OK) ::_exit(130); + } +}; + +TEST(RemoteClientControls, DeclinedFenceReplyAnswersSignaledInsteadOfAborting) { + const auto child = RunInChild([] { + StartControlSession(); + DecliningSyncPeer peer; + peer.Install(); + auto* sync = RemoteEmitTable().GL.FenceSync(); + if (sync == nullptr) ::_exit(131); + // glClientWaitSync on a fence nobody can wait on: signaled now, as every fallback in + // this tree already answers (GL_Sync.cpp:103-106, PipeApplier.cpp:175-177). + if (RemoteEmitTable().GL.ClientWaitSync(sync, 0, 0) != GL_ALREADY_SIGNALED) ::_exit(132); + // GL_SYNC_STATUS: the same answer, in the boolean the row carries. + if (!RemoteEmitTable().GL.GetSyncStatus(sync)) ::_exit(133); + RemoteEmitTable().GL.DeleteSync(sync); + ClientSessionInstance().Stop(); + }); + ExpectChildSuccess(child); +} + +TEST(RemoteClientControls, DeclinedQueryRepliesAnswerNoAnswerYetInsteadOfAborting) { + const auto child = RunInChild([] { + StartControlSession(); + DecliningSyncPeer peer; + peer.Install(); + auto* query = RemoteEmitTable().GL.BeginTimeElapsedQuery(); + if (query == nullptr) ::_exit(134); + // GL_QUERY_RESULT_AVAILABLE: "not available" - the answer an unfinished query gives, + // and the one that makes a waiting application yield rather than spin. + if (RemoteEmitTable().GL.IsQueryResultAvailable(query)) ::_exit(135); + // GL_QUERY_RESULT: no value produced, and the caller's out-parameter untouched. + Uint64 nanoseconds = 7; + if (RemoteEmitTable().GL.GetQueryResult64(query, false, &nanoseconds) || nanoseconds != 7) + ::_exit(136); + // glGetInteger64v(GL_TIMESTAMP): there is no timestamp to report. + if (RemoteEmitTable().GL.GetGpuTimestampNs() != 0) ::_exit(137); + RemoteEmitTable().GL.DeleteBackendQuery(query); + ClientSessionInstance().Stop(); + }); + ExpectChildSuccess(child); +} + +TEST(RemoteClientControls, ErrorFenceReplyStillRefusesByName) { + // THE TOLERANCE IS NOT BLANKET. An ERROR is a transport fault or a server-side refusal, + // never "the server said no" (R-5/M4), and it keeps the named Fatal it always had. + const auto child = RunInChild([] { + StartControlSession(); + // The rows still decline (so the fence is never handed to a backend that cannot + // create one) and EVERY answer that is posted says ERROR instead. + DecliningSyncPeer rows; + rows.Install(); + ErrorPeer peer; + peer.Install(); + auto* sync = RemoteEmitTable().GL.FenceSync(); + if (sync == nullptr) ::_exit(138); + (void)RemoteEmitTable().GL.ClientWaitSync(sync, 0, 0); + }); + ExpectNamedAbort(child, "Fatal{ProtocolCorruption, \"Fence.reply\"}"); +} + +TEST(RemoteClientControls, DeviceLostRespecifyDoesNotDieOfTheUncarriedBytesCheck) { + // P12 exit gate (a): killing the server must give a CLEAN device-lost latch. With the + // latch set every verb is DECLINED, so the resource_subdata that R-13.3 owes emits + // nothing - which is the latch working, not bytes the client forgot to carry. The check + // at PipeFill.cpp:1115 compared client record ordinals only, so the next glBufferData + // with data aborted the client by name instead. + const auto child = RunInChild([] { + StartControlSession(); + // Taken through the production entry point, the way the barrier and the encoder take + // it when a descriptor whose far end only the peer held reports hangup. + ClientSessionInstance().LatchDeviceLost("RemoteClientControls: the self-check case"); + MG_Config::Features.PipePush |= kMGPipeSubsystemResources; + MG_State::pGLContext = MakeUnique(); + GLuint buffer = 0; + MG_Impl::GLImpl::GenBuffers(1, &buffer); + MG_Impl::GLImpl::BindBuffer(0x8892, buffer); + Uint8 seed[64]; + std::memset(seed, 0x5a, sizeof(seed)); + MG_Impl::GLImpl::BufferData(0x8892, sizeof(seed), seed, 0x88E4); + ClientSessionInstance().Stop(); + }); + ExpectChildSuccess(child); +} + #endif