diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 943a8eea..e69fe173 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -124,6 +124,30 @@ namespace MobileGL::MG_Remote::Client { // second-scale wait, while a lost record never completes at all. 30 s separates the two // without turning a loaded CI machine into a red lane, and the Fatal names the seq. constexpr Uint32 kBarrierTimeoutMs = 30000; + + Uint64 AppliedWaitBudgetMs(MG_Pipe::MGPWireOp op, const void* payload) { + if (op != MG_Pipe::MGPWireOp::FenceWait) return kBarrierTimeoutMs; + const Uint64 timeoutNs = static_cast(payload)->TimeoutNs; + // ClientWaitSync may legitimately block longer than the ordinary verb barrier. + // Round up without overflowing UINT64_MAX, then allow the usual transport grace. + // FenceWaitServer queues a GPU wait; its GL_TIMEOUT_IGNORED is not a CPU budget. + return timeoutNs / 1000000 + (timeoutNs % 1000000 != 0) + kBarrierTimeoutMs; + } + + Transport::SessionWait WaitForAppliedBudget(Transport::SessionProducer& producer, + Uint64 seq, Uint64 remainingMs) { + // The transport takes Uint32 milliseconds with UINT32_MAX meaning forever. + // Keep even the largest GL timeout finite, using bounded chunks and preserving + // the doorbell's immediate shutdown result. Avoid a giant chrono deadline too. + constexpr Uint32 kMaxFiniteWaitMs = Transport::kWaitForever - 1; + for (;;) { + const Uint32 chunkMs = remainingMs > kMaxFiniteWaitMs + ? kMaxFiniteWaitMs : static_cast(remainingMs); + const auto wait = producer.WaitForApplied(seq, chunkMs); + if (wait != Transport::SessionWait::TimedOut || remainingMs <= chunkMs) return wait; + remainingMs -= chunkMs; + } + } // How many queued control frames one pump will drain. A backlog deeper than this is a // finding, not a steady state. constexpr Uint32 kMaxControlFramesPerPump = 16; @@ -776,7 +800,8 @@ namespace MobileGL::MG_Remote::Client { } const BarrierWaitScope waiting; - const Transport::SessionWait wait = m_producer.WaitForApplied(seq, kBarrierTimeoutMs); + const Uint64 waitBudgetMs = AppliedWaitBudgetMs(op, payload); + const Transport::SessionWait wait = WaitForAppliedBudget(m_producer, seq, waitBudgetMs); if (wait == Transport::SessionWait::ShutDown) { // The doorbell died: the server went away. The only thing that returns from a // kWaitForever park, and therefore the only way a client blocked in the barrier @@ -796,9 +821,10 @@ namespace MobileGL::MG_Remote::Client { } if (wait != Transport::SessionWait::Reached) { MGLOG_F("MGPipe: Fatal{BarrierTimeout, \"%s\"} - appliedSeq did not reach %llu within " - "%u ms. A bounded wait is deliberate: a wedged CI job and a lost record look " + "%llu ms. A bounded wait is deliberate: a wedged CI job and a lost record look " "identical from outside, and only one of them is a bug worth finding", - Wire::WireOpName(op), static_cast(seq), kBarrierTimeoutMs); + Wire::WireOpName(op), static_cast(seq), + static_cast(waitBudgetMs)); std::abort(); } diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index 6a50b9cf..216ed756 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -873,6 +873,10 @@ namespace MobileGL::MG_Remote::Client { "Fatal here rather than a buffer of stale bytes"); std::abort(); } + if (status != Wire::ReplySink::kStatusOk) { + MGLOG_F("MGPipe: Fatal{ReplyStatusInvalid, \"ReadPixels\"} - unknown reply status %d", status); + std::abort(); + } if (replySize != expected) { MGLOG_F("MGPipe: Fatal{ReadbackReplyShort, \"ReadPixels %llu < %llu\"} - the OK " "reply carried fewer bytes than the read's own DstSize (CONTRACT-P5 row " diff --git a/MobileGL/MG_Test/Wire/RemoteClientControls.inc b/MobileGL/MG_Test/Wire/RemoteClientControls.inc index 1f6c4d52..e40a97dd 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientControls.inc +++ b/MobileGL/MG_Test/Wire/RemoteClientControls.inc @@ -20,6 +20,54 @@ struct RepliesTag { friend Type PeerMember(RepliesTag); }; template struct PeerAccess; +struct ProducerBellTag { + using Type = Transport::Doorbell* Transport::SessionProducer::*; + friend Type PeerMember(ProducerBellTag); +}; +template struct PeerAccess; + +// Observe the actual transport park selected by EmitAndWait. The peer waits until +// the client parks, so no GPU or wall-clock 60-second sleep is required. +struct BudgetBell : Transport::Doorbell { + Transport::Doorbell* original; + std::atomic firstMs{0}; + std::atomic entered{false}; + Bool shutDown = false; + explicit BudgetBell(Transport::Doorbell* bell) : original(bell) {} + void Notify() override { original->Notify(); } + void Reset() override { original->Reset(); } + bool Park(std::uint32_t timeoutMs) override { + if (!entered.load()) { + firstMs.store(timeoutMs); + entered.store(true); + } + return shutDown ? false : original->Park(timeoutMs); + } + bool Dead() const override { return (shutDown && entered.load()) || original->Dead(); } +}; + +struct FenceBudgetPeer : Codec::WireVerbSink { + BudgetBell& bell; + std::atomic release{false}; + explicit FenceBudgetPeer(BudgetBell& value) : bell(value) {} + void AwaitPark() { + while (!bell.entered.load() || (bell.shutDown && !release.load())) std::this_thread::yield(); + } + Bool OnFenceWait(const MGPFenceWait&, Uint32& result) override { + AwaitPark(); + result = GL_TIMEOUT_EXPIRED; + return true; + } + Bool OnFenceWaitServer(const MGPFenceWait&) override { AwaitPark(); return true; } + void Install() { + if (Srv::ServerLoopInstance().RunOnApplyThread([](void* self) { + auto& decoder = Srv::ServerSessionInstance().Applier().*PeerMember(DecoderTag{}); + decoder.SetVerbSink(static_cast(self)); + return MOBILEGL_OK; + }, this) != MOBILEGL_OK) ::_exit(92); + } +}; + struct BackendTag { using Type = UniquePtr Srv::ServerLoop::*; friend Type PeerMember(BackendTag); @@ -151,6 +199,89 @@ TEST(RemoteClientControls, ErrorReadPixelsReplyRefusesByName) { ExpectNamedAbort(child, "Fatal{ReplyError, \"ReadPixels\"}"); } +TEST(RemoteClientControls, UnknownReadPixelsReplyStatusRefusesTightAndBounce) { + for (const Bool bounce : {false, true}) { + const auto child = RunInChild([bounce] { + StartControlSession(); + if (bounce) { + MG_State::pGLContext = MakeUnique(); + MG_State::pGLContext->SetPixelStoreParam(PixelStoreParam::PackRowLength, 8); + } + ReadPeer peer; + peer.status = 3; + peer.Install(); + Uint8 pixels[128]{}; + RemoteEmitTable().GL.ReadPixels(0, 0, 4, 3, GL_RGBA, GL_UNSIGNED_BYTE, pixels); + }); + ExpectNamedAbort(child, "Fatal{ReplyStatusInvalid, \"ReadPixels\"}"); + } +} + +TEST(RemoteClientControls, FenceWaitBudgetHonorsGlTimeoutAndFiniteTransportChunks) { + struct WaitCase { Uint64 timeoutNs; MGPWireOp op; Uint32 expectedParkMs; }; + const WaitCase cases[] = { + {0, MGPWireOp::FenceWait, 30000}, + {1, MGPWireOp::FenceWait, 30001}, + {60000000000ull, MGPWireOp::FenceWait, 90000}, + {~Uint64(0), MGPWireOp::FenceWait, Transport::kWaitForever - 1}, + {~Uint64(0), MGPWireOp::FenceWaitServer, 30000}, + }; + for (const auto test : cases) { + const auto child = RunInChild([test] { + StartControlSession(); + auto& session = ClientSessionInstance(); + auto& producer = session.Producer(); + BudgetBell bell(producer.SelfDoorbell()); + FenceBudgetPeer peer(bell); + peer.Install(); + producer.*PeerMember(ProducerBellTag{}) = &bell; + MGPFenceWait request{}; + request.TimeoutNs = test.timeoutNs; + Uint32 result = 0; + Int32 status = -1; + Uint64 size = 0; + session.EmitAndWait(test.op, &request, sizeof(request), nullptr, 0, + &result, sizeof(result), &status, &size); + producer.*PeerMember(ProducerBellTag{}) = bell.original; + const auto observed = bell.firstMs.load(); + // Doorbell converts the remaining deadline to whole milliseconds. Allow + // scheduler delay, but neither a 30s clamp nor an unbounded sentinel. + if (!bell.entered.load() || observed > test.expectedParkMs || + observed < test.expectedParkMs - 100 || observed == Transport::kWaitForever) ::_exit(93); + if (test.op == MGPWireOp::FenceWait && + (status != Codec::ReplySink::kStatusOk || size != sizeof(result) || + result != GL_TIMEOUT_EXPIRED)) ::_exit(94); + session.Stop(); + }); + ExpectChildSuccess(child); + } +} + +TEST(RemoteClientControls, LongFenceWaitBudgetStillWakesOnShutdown) { + const auto child = RunInChild([] { + StartControlSession(); + auto& session = ClientSessionInstance(); + auto& producer = session.Producer(); + BudgetBell bell(producer.SelfDoorbell()); + bell.shutDown = true; + FenceBudgetPeer peer(bell); + peer.Install(); + producer.*PeerMember(ProducerBellTag{}) = &bell; + MGPFenceWait request{}; + request.TimeoutNs = ~Uint64(0); + Uint32 result = 0; + Int32 status = -1; + Uint64 size = 7; + session.EmitAndWait(MGPWireOp::FenceWait, &request, sizeof(request), nullptr, 0, + &result, sizeof(result), &status, &size); + producer.*PeerMember(ProducerBellTag{}) = bell.original; + peer.release.store(true); + if (status != Codec::ReplySink::kStatusDeclined || size != 0) ::_exit(95); + session.Stop(); + }); + ExpectChildSuccess(child); +} + TEST(RemoteClientControls, ResourceRespecifyErrorIsNotADecline) { const auto child = RunInChild([] { StartControlSession();