From 5239ae0c1978525ff8d7e330fb8cd57f128d7762 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Mon, 21 Sep 2026 10:50:28 -0400 Subject: [PATCH] [MG_Remote] (P6/cp): EGL control frames cross to the other process, and answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An eglInitialize issued in one process is dispatched in another and its answer comes back. That is the last thing standing between the session and a native context, because the server creates its context on the apply thread when the client's first eglMakeCurrent crosses - and until now nothing crossed. ONE SEAM, NOT TWELVE. a6 row A4-13 counted twelve Server* forwarders, each ending in ServerLoopInstance().RunSurfaceControlFrame(frame) with no role branch, and concluded twelve edits. They all funnel through that one function, so the branch goes there and the forwarders are untouched. A HOOK, NOT A CALL. ServerLoop::SetRemoteControlSink takes a function pointer the client installs at handshake. The obvious alternative - the server half asking ClientSession::Active() - would have added a fifth symbol to the six MG_Remote already owes itself (a6-link-experiment §5), in the one file with a clean reason not to. The branch is BEFORE the local seq mint, deliberately. RunSurfaceControlFrame numbers unnumbered local posts; a wire request must keep the CLIENT's number all the way through dispatch and reply or its answer cannot be correlated (CONTRACT-P6 §6.3, which says the client mints, always). The client sink encodes with EncodeSurfaceOpFrame, sends, and waits for the SurfaceReply carrying its own seq - under one mutex, because the twelve forwarders are already caller-serialised on the server's m_callerMutex under inproc and two replies racing for one correlation is the same bug on either side. A CapsSnapshot arriving while it waits is ADOPTED rather than dropped: the server republishes caps from inside its MakeCurrent dispatch (a6 row A4-30) so a snapshot legitimately precedes the reply to the op that produced it. The wait is bounded and its expiry is NOT fatal - a peer that is gone is a different fact from a peer that is slow, and only the doorbell's death latch may say which. The server's pump decodes and applies through ServerApplyWireSurfaceOp, which a6 found had no production caller anywhere. It runs on ServerMain's own thread - NOT the apply thread - which is what §6.2 asks for: a control arrival must not wake the applier, and frame decode must not happen on the thread holding the native context. A frame that is not a verifiable SurfaceOp ends the session rather than being ignored, because a silent drop is the failure mode this project refuses everywhere else. THE TEST'S ASSERTION IS THE OUT-PARAMETERS, not the EGL result. A headless machine has no display to initialise, so the op is expected to be refused - and ServerInitializeEGLDisplay writes major/minor "whenever the dispatch ran, success or not" (the forwarder's own comment). They move off -1 if and only if a SurfaceReply came back from the other process; a transport failure returns before the write-back. I got this backwards first and the test said so. Gates: ServerSpawnTest 5/5; unit suite 2345 with the same two pre-existing environmental failures; G1 pull 0 added / 0 removed, .text 10822147 unchanged. Not TCP. It was considered and dropped for now: TCP has no SCM_RIGHTS, so the four segments cannot cross it, and a TCP control plane would have to refuse a data plane by name until StreamLink exists (P6.5). Recorded here so the reason is on file rather than re-derived. --- MobileGL/MG_Remote/Client/ClientSession.cpp | 95 +++++++++++++++++++++ MobileGL/MG_Remote/Client/ClientSession.h | 11 +++ MobileGL/MG_Remote/Server/ServerLoop.cpp | 14 +++ MobileGL/MG_Remote/Server/ServerLoop.h | 18 ++++ MobileGL/MG_Remote/Server/ServerMain.cpp | 39 +++++++-- MobileGL/MG_Test/Wire/ServerSpawnTest.cpp | 40 +++++++++ 6 files changed, 210 insertions(+), 7 deletions(-) diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index d54b8bd6..0b8d952d 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -15,6 +15,7 @@ #include "../Protocol/generated/protocol_generated.h" #include "../Server/ServerLoop.h" #include "../Server/ServerSession.h" +#include "../Protocol/SurfaceOpCodec.h" #include "../Transport/FdPassing.h" #include "../Transport/InProcessTransport.h" #include "WireTables.h" @@ -52,6 +53,10 @@ namespace MobileGL::MG_Remote::Client { // The same bounded handshake deadline the server uses. Bounded, not kWaitForever: a // bring-up that never answers has to be a red lane rather than a wedged CI job. constexpr Uint32 kHandshakeTimeoutMs = 5000; + // cp (CONTRACT-P6 D5b): bounded, and its expiry is NOT fatal. A peer that + // is gone is a different fact from a peer that is slow, and only the + // doorbell death latch may say which. + constexpr Uint32 kRemoteControlTimeoutMs = 5000; // Teardown's drain. Also bounded, and for the same reason - table 3's order is // "publish and wait for the server to drain and acknowledge", and a wait with no // deadline there turns a lost record into a hung process exit. @@ -769,12 +774,102 @@ namespace MobileGL::MG_Remote::Client { m_socketSelfBell = MakeUnique(fds[5], 1, true); } + // `cp`: from here the twelve EGL forwarders cross the socket instead of + // being posted into a mailbox this process does not own. Installed BEFORE + // FinishStartup, because FinishStartup's caps pump can already trigger + // one. + Server::ServerLoopInstance().SetRemoteControlSink(&RemoteControlSinkThunk, this); + // The apply thread IS ALREADY RUNNING, in the other process: ServerMain // started it before it sent the first descriptor. Starting one here would // put a second applier on a ring that already has one. return FinishStartup(m_socketPeerBell.get(), m_socketSelfBell.get(), false); } + MobileGLResult ClientSession::RemoteControlSinkThunk(void* user, + Server::SurfaceControlFrame& frame) { + return static_cast(user)->RunRemoteSurfaceControlFrame(frame); + } + + MobileGLResult ClientSession::RunRemoteSurfaceControlFrame(Server::SurfaceControlFrame& frame) { + if (m_transport == nullptr) { + return MOBILEGL_ERR_NOT_INITIALIZED; + } + // THE CLIENT MINTS, always, dense from 1 (CONTRACT-P6 §6.3). The server's + // local mint exists for unnumbered LOCAL posts; a wire request that let + // the server renumber it would get back a reply it cannot correlate. + if (frame.seq == 0) { + frame.seq = ++m_remoteControlSeq; + } + + ::flatbuffers::FlatBufferBuilder builder(512); + const MG_Remote::SurfaceWireError encoded = MG_Remote::EncodeSurfaceOpFrame(frame, &builder); + if (encoded != MG_Remote::SurfaceWireError::None) { + // A NAMED refusal, not a dropped op. InprocOnlyOpOnTheWire is the + // live one: three of the twelve forwarders ride the frame channel + // without a wire kind (CONTRACT-P6 §2), and a spawn client that + // reaches one has found a gap rather than a transport error. + MGLOG_E("MG_Remote client: surface op kind %d cannot cross the wire (%s)", + static_cast(frame.kind), MG_Remote::SurfaceWireErrorName(encoded)); + return MOBILEGL_ERR_UNSUPPORTED; + } + + // ONE CONTROL OP AT A TIME. The twelve forwarders are already + // caller-serialised on the server's m_callerMutex under inproc; the same + // discipline has to hold here or two replies race for one correlation. + const std::lock_guard lock(m_remoteControlMutex); + const MobileGLResult sent = m_transport->SendFrame( + MobileGLByteSpan{builder.GetBufferPointer(), builder.GetSize()}); + if (sent != MOBILEGL_OK) { + return sent; + } + + // The reply. A BOUNDED wait whose expiry is NOT fatal (CONTRACT-P6 §5.4 + // D5b): a peer that is gone is a different fact from a peer that is slow, + // and only the doorbell's death latch may say which. + for (;;) { + std::vector reply; + const MobileGLResult received = + ReceiveEnvelope(*m_transport, reply, kRemoteControlTimeoutMs); + if (received != MOBILEGL_OK) { + MGLOG_E("MG_Remote client: no SurfaceReply for seq %llu within %u ms (rc=%d)", + static_cast(frame.seq), kRemoteControlTimeoutMs, + static_cast(received)); + return received; + } + const ::MobileGL::Wire::CtrlEnvelope* envelope = ParseEnvelope(reply); + if (envelope == nullptr) { + return MOBILEGL_ERR_PROTOCOL_MISMATCH; + } + if (envelope->msg_type() == ::MobileGL::Wire::CtrlMsg::CapsSnapshot) { + // The server republishes caps from inside its MakeCurrent + // dispatch (a6 row A4-30), so a snapshot can arrive BEFORE the + // reply to the very op that produced it. Adopting it here rather + // than dropping it is what keeps R-12's "a second arrival IS the + // invalidation" true on this path too. + if (const auto* snapshot = envelope->msg_as_CapsSnapshot()) { + AdoptCapsSnapshot(snapshot); + } + continue; + } + if (envelope->msg_type() != ::MobileGL::Wire::CtrlMsg::SurfaceReply || + envelope->msg_as_SurfaceReply() == nullptr) { + MGLOG_E("MG_Remote client: expected a SurfaceReply, got message type %d", + static_cast(envelope->msg_type())); + return MOBILEGL_ERR_PROTOCOL_MISMATCH; + } + const ::MobileGL::Wire::SurfaceReply* wire = envelope->msg_as_SurfaceReply(); + if (wire->seq() != frame.seq) { + MGLOG_E("MG_Remote client: SurfaceReply names seq %llu, we asked for %llu", + static_cast(wire->seq()), + static_cast(frame.seq)); + return MOBILEGL_ERR_PROTOCOL_MISMATCH; + } + MG_Remote::DecodeWireSurfaceReply(*wire, &frame); + return MOBILEGL_OK; + } + } + // The half of startup that is IDENTICAL for inproc and spawn: the ring // producer, the reply pool, the event ring, the segment table, the encoder, // the first CapsSnapshot and the emitter tables. Extracted by `sm` so the diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index a3ad6e4b..f823fb96 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -38,6 +38,7 @@ #include #include +#include "../Server/SurfaceControlFrame.h" #include "../Transport/Doorbell.h" #include "../Transport/EventRing.h" #include "../Transport/ITransport.h" @@ -93,6 +94,12 @@ namespace MobileGL::MG_Remote::Client { // encoder - is byte for byte the inproc path, which is the property // that makes "P6 is a transport swap" true of THIS function even though // it is not true of the phase. + // `cp`: the remote control sink ServerLoop calls when the server is + // another process. Encode, send, wait for the SurfaceReply that carries + // OUR seq back. + static MobileGLResult RemoteControlSinkThunk(void* user, Server::SurfaceControlFrame& frame); + MobileGLResult RunRemoteSurfaceControlFrame(Server::SurfaceControlFrame& frame); + MobileGLResult FinishStartup(Transport::Doorbell* peerBell, Transport::Doorbell* selfBell, bool startApplyThreadHere); @@ -380,6 +387,10 @@ namespace MobileGL::MG_Remote::Client { // than in the transport because which bell is "mine" is the session's // knowledge, not the transport's - the same ruling as inproc's // PeerDoorbell/SelfDoorbell split. + // cp: one control op at a time, and the client's own seq space. + std::mutex m_remoteControlMutex; + Uint64 m_remoteControlSeq = 0; + std::unique_ptr m_socketTransport; std::unique_ptr m_socketSelfBell; std::unique_ptr m_socketPeerBell; diff --git a/MobileGL/MG_Remote/Server/ServerLoop.cpp b/MobileGL/MG_Remote/Server/ServerLoop.cpp index c318b504..615e788b 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.cpp +++ b/MobileGL/MG_Remote/Server/ServerLoop.cpp @@ -655,8 +655,22 @@ namespace MobileGL::MG_Remote::Server { return applied; } + void ServerLoop::SetRemoteControlSink(RemoteControlSink sink, void* user) { + m_remoteSink = sink; + m_remoteSinkUser = user; + } + MobileGLResult ServerLoop::RunSurfaceControlFrame(SurfaceControlFrame& frame) { if (frame.kind == SurfaceControlOp::None) return MOBILEGL_ERR_INVALID_ARGUMENT; + + // `cp`: the server is another process, so the frame CROSSES instead of + // being posted. Checked before the seq mint below, because a wire request + // must carry the CLIENT's number all the way through dispatch and the + // reply - the local mint at :662 is for unnumbered LOCAL requests and + // renumbering a wire one would make its reply uncorrelatable. + if (m_remoteSink != nullptr) { + return m_remoteSink(m_remoteSinkUser, frame); + } // Zero marks an unnumbered local request. A wire request is already numbered by its // client and MUST retain that number all the way through dispatch and the reply. if (frame.seq == 0) frame.seq = m_controlSeq.fetch_add(1, std::memory_order_acq_rel) + 1; diff --git a/MobileGL/MG_Remote/Server/ServerLoop.h b/MobileGL/MG_Remote/Server/ServerLoop.h index edfa097b..c53399b7 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.h +++ b/MobileGL/MG_Remote/Server/ServerLoop.h @@ -223,6 +223,21 @@ namespace MobileGL::MG_Remote::Server { // What did NOT move: the blocking handshake, the one slot, and the apply-thread // ownership. Under spawn the same frame is what SurfaceOpCodec encodes into a // Wire::SurfaceOp - this signature is the seam both transports share. + // P6 `cp`: where a control frame goes when the server is ANOTHER PROCESS. + // + // All twelve Server* forwarders funnel through RunSurfaceControlFrame, so + // this is ONE seam rather than twelve (a6 row A4-13 counted them). The + // client installs a sink at handshake; with none installed the behaviour + // is exactly what it was - post into the one-slot mailbox, or run inline + // when we are already on the apply thread. + // + // A HOOK RATHER THAN A CALL INTO ClientSession, deliberately: the server + // half calling the client half is four of the six symbols a6 found + // MG_Remote owes itself (a6-link-experiment §5), and this is the one + // place that would have added a fifth. + using RemoteControlSink = MobileGLResult (*)(void* user, SurfaceControlFrame& frame); + void SetRemoteControlSink(RemoteControlSink sink, void* user); + MobileGLResult RunSurfaceControlFrame(SurfaceControlFrame& frame); // The TEST seam through the same channel: posts a ProbeForTesting frame whose dispatch @@ -364,6 +379,9 @@ namespace MobileGL::MG_Remote::Server { Bool ControlIsPending() const; private: + RemoteControlSink m_remoteSink = nullptr; + void* m_remoteSinkUser = nullptr; + void ApplyThreadMain(); // Runs a posted control frame, if there is one. Returns true if it ran one. Bool PumpControlRequest(); diff --git a/MobileGL/MG_Remote/Server/ServerMain.cpp b/MobileGL/MG_Remote/Server/ServerMain.cpp index 9300e90e..c79350f5 100644 --- a/MobileGL/MG_Remote/Server/ServerMain.cpp +++ b/MobileGL/MG_Remote/Server/ServerMain.cpp @@ -38,8 +38,10 @@ #include "../Transport/FdPassing.h" #include "../Transport/SocketTransport.h" #include "../Transport/WireLog.h" +#include "../Protocol/SurfaceOpCodec.h" #include "ServerLoop.h" #include "ServerSession.h" +#include "SurfaceControlFrame.h" #include @@ -287,13 +289,36 @@ extern "C" __attribute__((visibility("default"))) int mobilegl_server_main(int a static_cast(result)); break; } - // `cp` owns what a control frame MEANS. Until then the pump exists so the - // stream is drained and EOF is noticed; the echo carries this process's - // own pid so a test that talked to itself cannot pass. - std::string reply(reinterpret_cast(buffer.data()), - static_cast(size)); - reply += "|server-pid=" + std::to_string(selfPid); - if (control->SendFrame(MobileGLByteSpan{reply.data(), reply.size()}) != MOBILEGL_OK) { + // `cp`: decode, apply, reply. THIS THREAD IS NOT THE APPLY THREAD, which + // is what CONTRACT-P6 §6.2 requires - a control arrival must not wake the + // applier, and frame decode must not happen on the thread holding the + // native context. ServerApplyWireSurfaceOp posts through ServerLoop's + // existing one-slot mailbox, unchanged. + const ::MobileGL::Wire::CtrlEnvelope* envelope = nullptr; + { + ::flatbuffers::Verifier verifier(buffer.data(), static_cast(size)); + if (::MobileGL::Wire::VerifyCtrlEnvelopeBuffer(verifier)) { + envelope = ::MobileGL::Wire::GetCtrlEnvelope(buffer.data()); + } + } + if (envelope == nullptr || envelope->msg_type() != ::MobileGL::Wire::CtrlMsg::SurfaceOp || + envelope->msg_as_SurfaceOp() == nullptr) { + WireLogError("MG_Remote server: pid=%d a control frame that is not a verifiable " + "SurfaceOp arrived; ignoring it would be the silent drop this project " + "refuses, so the session ends here", + selfPid); + break; + } + + Server::SurfaceControlFrame reply{}; + const MobileGLResult applied = + ServerApplyWireSurfaceOp(*envelope->msg_as_SurfaceOp(), &reply); + (void)applied; // carried in the reply's ok/result, not thrown away here + + ::flatbuffers::FlatBufferBuilder builder(256); + EncodeSurfaceReplyFrame(reply, &builder); + if (control->SendFrame( + MobileGLByteSpan{builder.GetBufferPointer(), builder.GetSize()}) != MOBILEGL_OK) { break; } } diff --git a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp index 6575b362..625dca6f 100644 --- a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp @@ -34,6 +34,7 @@ // the boundary before the data plane needs it. #include +#include #include #include #include @@ -151,6 +152,45 @@ TEST(ServerSpawnTest, StartsAServerProcessAndHandshakesAcrossIt) { EXPECT_EQ(Server::CountOwnChildren(), before) << "a zombie would still be counted here"; } +TEST(ServerSpawnTest, AnEglControlOpCrossesToTheOtherProcessAndAnswers) { + // `cp`. All twelve Server* EGL forwarders funnel through + // ServerLoop::RunSurfaceControlFrame, so this exercises the ONE seam that + // sends them: encode -> control socket -> the server's pump -> + // ServerApplyWireSurfaceOp -> its one-slot mailbox -> the apply thread -> + // SurfaceReply -> back here. + // + // Under inproc this same call posts into a mailbox in THIS process. The + // assertion that it is not doing that here is the server's pid in the + // handshake plus the fact that ServerLoopInstance() in this process was + // never started - a local post would find no apply thread and time out. + Session session; + ASSERT_TRUE(Bring("eglop", &session)); + ASSERT_EQ(Handshake(session), MOBILEGL_OK); + + // eglInitialize's forwarder: the simplest op that has a real answer. + EGLint major = -1; + EGLint minor = -1; + const bool ok = Server::ServerInitializeEGLDisplay(EGL_NO_DISPLAY, &major, &minor); + + // WHAT IS ASSERTED IS THE ROUND TRIP, not the EGL result. A headless CI + // machine has no display to initialise, so `ok` is expected to be false - + // and that is fine, because `cp` is done when the op reaches the other + // process and an answer comes back. + // + // THE OUT-PARAMETERS ARE THE PROOF. ServerInitializeEGLDisplay writes them + // "whenever the dispatch ran, success or not" (ServerLoop.cpp's own comment + // at the forwarder), so they move off -1 if and only if a SurfaceReply came + // back from the other process. A transport failure returns before the + // write-back and leaves both at -1; that is the difference this asserts. + EXPECT_NE(major, -1) << "no SurfaceReply came back: the op never reached the other process"; + EXPECT_NE(minor, -1) << "no SurfaceReply came back: the op never reached the other process"; + (void)ok; + + Client::ClientSessionInstance().Stop(); + int exitCode = -1; + ASSERT_EQ(Server::ReapServer(session.server, 5000, &exitCode), MOBILEGL_OK); +} + TEST(ServerSpawnTest, AnUnresolvableImageIsANamedRefusalAndStartsNothing) { const int before = Server::CountOwnChildren();