From 8b13d8a84cf9b7478acc3d7dfd7fbbf92f98ea29 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Mon, 21 Sep 2026 18:49:56 -0400 Subject: [PATCH] [MG_Remote, MG_Impl] (P6/dl): the device-lost latch, taken from a descriptor and never from a deadline Before this, CONTRACT-P6 5.3's own sentence held: "a dead server leaves the client silently DECLINE-ing every verb forever with glGetError clean". FatalCode::DeviceLost had zero producers and zero consumers and glGetGraphicsResetStatus returned GL_NO_ERROR unconditionally, so an application had no way to learn that the process holding its GPU objects was gone. Measured cost of that: a server killed on its first Clear left the client waiting out the full 120 s barrier and reporting Fatal{BarrierTimeout} - the wrong diagnosis, two minutes late. It now notices in 0.01 s. THE MACHINERY WAS ALREADY THERE. Doorbell::Wait breaks out on Dead(), SessionProducer::Park turns that into SessionWait::ShutDown, and ClientSession already had the named DECLINED path. What did not exist was the death FACT: D5c's finding is that the client parks on clientBell[0] and rings ITSELF through clientBell[1], holding both ends, so EOF can never arrive on the one descriptor it watches. A bell that can wake itself cannot hear a death. SetDeathWitness(fd) fixes that with no new descriptor: the control socket's far end is held only by the server. The subtlety is that the bell must watch a socket it must NEVER read - the transport's reassembler owns those bytes and a control reply is not a death. So the witness enters the poll set with `events = POLLRDHUP` and nothing else: POLLHUP/POLLERR/POLLNVAL arrive in revents unrequested, so the hangup is seen while POLLIN never is. Nothing is recv'd from it and its ownership stays with SocketTransport. Dead() AND PeerHungUp() ARE TWO FACTS. Park sets m_dead for POLLERR, POLLNVAL and unrecognised revents too - each a fault in THIS process's descriptor - and CondVarDoorbell::Kill() sets it on every orderly Stop(). A latch taken from Dead() would report a lost GPU for a local fd bug and a context reset for every clean exit, so only PeerHungUp() arms it. GL_UNKNOWN_CONTEXT_RESET, not GUILTY or INNOCENT: those assign blame, and the server died for a reason that never crossed the wire. D5b, in the control-reply wait, which had the right comment and no code: EOF on the control stream is device loss; a TIMEOUT with the socket still open is "ALIVE BUT SILENT" and says so by name. 5.4 forbids calling a frozen or merely slow peer dead, and under P5e run-ahead a server one frame behind is the INTENDED steady state - a latch armed by a deadline would fire on a healthy session under load. MOBILEGL_IPC_RESPAWN is now a NAMED REFUSAL rather than a variable nothing reads. The latch is one-way on purpose: every handle this client minted names an object inside the server process, so recovery means re-pushing the world. THREE TEST NAMES ADDED, none renamed (G14), and R-16 run both ways: * remove the witness -> AKilledServerLatchesDeviceLostFromTheHangupAndNot- ADeadline goes red, takes 2 s instead of 0.01 s, and names D5c. * collapse PeerHungUp() into Dead() -> ADeadBellIsNotAlwaysAHungUpPeer goes red naming the local-fd-bug consequence. * AnOrderlyStopIsNotADeviceLoss stays GREEN under that collapse, and its comment now says so. The first draft claimed it was the control for it; running the falsification proved otherwise - under spawn the self bell is a SocketDoorbell and Stop() never kills one. The comment was corrected rather than the claim quietly kept. G1 RE-MEASURED, NOT ASSUMED: 0 symbol diff against the c6 baseline and .text unchanged at 0xa52203. That measurement earned its keep - it caught a 16-byte drift that had nothing to do with dl: P6's O_APPEND log-sink fix was compiling into the PULL build, where a second process can never exist. Now guarded, and .text is back to the baseline exactly. Full unit suite: 2350 tests, the same two pre-existing environmental failures and no others. All six OpenRA arms (2 backends x monolith/inproc/spawn) green. --- MobileGL/ConfigLoader.cpp | 24 ++++ MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp | 28 +++++ MobileGL/MG_Remote/CONTRACT-P6.md | 31 ++++- MobileGL/MG_Remote/Client/ClientSession.cpp | 77 ++++++++++++- MobileGL/MG_Remote/Client/ClientSession.h | 29 +++++ MobileGL/MG_Remote/Transport/Doorbell.cpp | 68 ++++++++--- MobileGL/MG_Remote/Transport/Doorbell.h | 42 ++++++- MobileGL/MG_Test/Wire/ServerSpawnTest.cpp | 113 +++++++++++++++++++ MobileGL/MG_Util/Debug/Log.cpp | 11 ++ 9 files changed, 405 insertions(+), 18 deletions(-) diff --git a/MobileGL/ConfigLoader.cpp b/MobileGL/ConfigLoader.cpp index 5ea497ba..73710986 100644 --- a/MobileGL/ConfigLoader.cpp +++ b/MobileGL/ConfigLoader.cpp @@ -427,6 +427,30 @@ namespace MobileGL::MG_ConfigLoader { ipc.RoleSplitState = false; } + // P6 `dl` (CONTRACT-P6 5.3): MOBILEGL_IPC_RESPAWN IS A NAMED REFUSAL, NOT A NO-OP. + // + // The device-lost latch is deliberately one-way - a session whose server died has + // nothing to recover into, because every handle the client minted names an object in a + // process that no longer exists. Re-pushing the world onto a fresh server is the work + // this knob would turn on, and no stage has written it. + // + // Refused BY NAME rather than parsed and ignored, for the reason Config.h gives about + // the whole IPC family: an environment variable nothing consumes is indistinguishable + // from one that is consumed and does nothing, and an operator who set this one would + // otherwise conclude that recovery had been tried and had not helped. + { + String respawn; + QueryEnvVariable("MOBILEGL_IPC_RESPAWN", respawn, ""); + if (!respawn.empty() && respawn != "0") { + MGLOG_E("Config: MOBILEGL_IPC_RESPAWN='%s' names a recovery NO STAGE HAS " + "IMPLEMENTED. The device-lost latch is one-way on purpose: every handle " + "this client minted names an object inside the server process, so a new " + "server would have to be re-pushed the entire world before a single verb " + "could land. This run will latch device-lost and stay there.", + respawn.c_str()); + } + } + if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return; // One line, on the arm where these numbers decide behaviour, because every one of // them is a number a bug report has to quote. diff --git a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp index 9112df2c..27876ad7 100644 --- a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp +++ b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp @@ -18,6 +18,13 @@ #include #include #include + +// GUARDED AT THE INCLUDE, not just around the call. This file compiles in the PULL build too, +// and G1 requires that build's symbol set and .text to be byte-identical - a header that drags +// in MG_Remote would change it whether or not any code path reaches the session. +#if MOBILEGL_BUILD_DISAGGREGATED +#include +#endif #include #include #include @@ -2853,6 +2860,27 @@ namespace MobileGL::MG_Impl::GLImpl { } GLenum GetGraphicsResetStatus() { +#if MOBILEGL_BUILD_DISAGGREGATED + // P6 `dl` (CONTRACT-P6 5.3): THE ONE GL-VISIBLE CONSEQUENCE OF A DEAD SERVER. + // + // Before this, "a dead server leaves the client silently DECLINE-ing every verb forever + // with glGetError clean" - the contract's own words, and the reason the latch is a + // package rather than carriage. An application that asks the robustness question has no + // other way to find out, because a DECLINE is indistinguishable from a verb that + // happened. + // + // GL_UNKNOWN_CONTEXT_RESET, not GUILTY or INNOCENT. Those two assign BLAME - guilty + // means this context's commands caused the reset, innocent means another context's did - + // and neither is knowable here: the server died for a reason that never crossed the + // wire, and it could as easily have been a driver fault as this client's draw. UNKNOWN is + // the spec's answer for exactly that, and it is the one that does not lie. + // + // WHOLLY INSIDE THE GUARD, including the include above. In a pull build this function is + // byte-for-byte what it was, which is what G1 measures. + if (MG_Remote::Client::ClientSession::DeviceLost()) { + return GL_UNKNOWN_CONTEXT_RESET; + } +#endif // MobileGL does not implement robustness reset notification, so report GL_NO_ERROR // ("no reset detected"). Returning the generic stub's (GLenum)1 makes dEQP read a lost // device after every case (gl3cTestPackages.cpp:121) and, under the default diff --git a/MobileGL/MG_Remote/CONTRACT-P6.md b/MobileGL/MG_Remote/CONTRACT-P6.md index 3e8ef7fc..f7d635f8 100644 --- a/MobileGL/MG_Remote/CONTRACT-P6.md +++ b/MobileGL/MG_Remote/CONTRACT-P6.md @@ -293,8 +293,17 @@ parsed **nowhere** (only a comment at `Config.h:466`). Today a dead server leave silently `DECLINE`-ing every verb forever with `glGetError` clean. **It is a package (`dl`), not carriage.** The latch is session-scoped and set from -`Doorbell::Dead()`, **never from a timeout**. `MOBILEGL_IPC_RESPAWN=1` is a named refusal until some -stage implements the re-push. +`Doorbell::PeerHungUp()` — D5c amends "`Dead()`" here, and §5.4's block says why — **never from a +timeout**. `MOBILEGL_IPC_RESPAWN=1` is a named refusal until some stage implements the re-push. + +**LANDED.** `ClientSession::DeviceLost()` / `LatchDeviceLost()`, armed at the verb barrier, at the +forced run-ahead wait and at the control-reply wait, and read by `GetGraphicsResetStatus`, which +now answers **`GL_UNKNOWN_CONTEXT_RESET`**. Not `GUILTY` or `INNOCENT`: those assign blame, and the +server died for a reason that never crossed the wire — it could as easily have been a driver fault +as this client's draw. The consult and its include sit wholly inside `MOBILEGL_BUILD_DISAGGREGATED`; +G1 re-measured after it, **0 symbol diff and `.text` unchanged at `0xa52203`**. That measurement +also caught an unrelated 16-byte drift the guards did not: P6's O_APPEND log-sink fix was compiled +into the pull build too, and is now guarded for the same reason. ### 5.4 Slow, dead, and frozen are three states @@ -332,6 +341,24 @@ diagnostic. Packages: `cp` (a), `dl` (b). > already exactly that and needs no new descriptor. The fix does not belong to `sm`: holding both > ends is required for self-notification and is not the defect — assuming one descriptor could > answer both questions is. +> +> **LANDED.** `SocketDoorbell::SetDeathWitness(fd)`; the client hands it +> `SocketTransport::StreamFd()`. The witness enters the poll set with `events = POLLRDHUP` **and +> nothing else**, which is what lets it watch a socket it must never read: `POLLHUP`/`POLLERR`/ +> `POLLNVAL` arrive in `revents` unrequested, so the hangup is seen while `POLLIN` never is — a +> control reply queued on that descriptor can neither wake the bell nor be consumed from under +> `SocketTransport`'s reassembler. Measured: `kill -9` on the server is noticed in **0.01 s** +> against a 120 000 ms barrier, and with the witness removed the same test takes the full 2 s +> retry budget and fails naming this paragraph. +> +> **`Dead()` AND `PeerHungUp()` ARE TWO FACTS, and only the second may arm the latch.** `Park` +> sets `m_dead` for `POLLERR`, `POLLNVAL` and unrecognised `revents` as well — each a fault in +> *this* process's descriptor — and `CondVarDoorbell::Kill()` sets it on every orderly `Stop()`. +> A latch taken from `Dead()` would report a lost GPU for a local fd bug and a context reset for +> every clean exit. `ADeadBellIsNotAlwaysAHungUpPeer` is the red-once for the distinction; +> `AnOrderlyStopIsNotADeviceLoss` is **not** — it stays green under the collapse, because under +> spawn the self bell is a `SocketDoorbell` and `Stop()` never kills one. That was found by +> running the falsification rather than by reading the code. --- diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index d6d981bf..a57561d0 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -466,6 +466,27 @@ namespace MobileGL::MG_Remote::Client { // monolith answer. Every call that PRESUMES a session aborts instead. ClientSession* ClientSession::Active() { return g_active; } + Bool ClientSession::DeviceLost() { + const ClientSession* session = Active(); + return session != nullptr && session->m_deviceLost.load(std::memory_order_acquire); + } + + void ClientSession::LatchDeviceLost(const char* why) { + // exchange, so the log line is written EXACTLY once however many waits notice the same + // hangup at the same moment - and there will be several: the barrier, the control wait + // and the event pump can all be in flight when the server goes. + if (m_deviceLost.exchange(true, std::memory_order_acq_rel)) { + return; + } + MGLOG_E("MGPipe: DEVICE LOST - %s. The server process is gone; every verb from here is " + "DECLINED and glGetGraphicsResetStatus reports GL_UNKNOWN_CONTEXT_RESET. This is " + "latched from the peer's HANGUP and not from any deadline (CONTRACT-P6 5.4): a " + "server one frame behind is P5e's intended steady state and must never read as a " + "dead one. There is no recovery - MOBILEGL_IPC_RESPAWN is the stage that would " + "add one and it is refused by name until then.", + why == nullptr ? "the peer hung up" : why); + } + ClientSession& ClientSessionInstance() { // Leak at exit, deliberately and per ID-8, exactly as ServerSessionInstance does. static ClientSession* instance = new ClientSession{}; @@ -835,7 +856,20 @@ namespace MobileGL::MG_Remote::Client { // fds[5] our park end // fds[6] ring OURSELVES m_socketPeerBell = MakeUnique(-1, fds[4], 1, true); - m_socketSelfBell = MakeUnique(fds[5], fds[6], 1, true); + // Built into a TYPED pointer first: the witness is SocketDoorbell's, and the member + // is a Doorbell so that the inproc path can put a CondVarDoorbell in it. + auto selfBell = MakeUnique(fds[5], fds[6], 1, true); + + // `dl`: THE DEATH WITNESS, and it has to be a descriptor this side does not also + // write to. Our own bell is a socketpair we hold BOTH ends of - slot 5 to park on, + // slot 6 to ring ourselves - so it can never hang up however dead the server is. The + // control socket's far end is held only by the server, so its hangup IS the death. + // + // Passing the fd rather than the transport keeps the ownership honest: the bell polls + // it for hangup alone and never reads it, and SocketTransport goes on owning and + // closing it. CONTRACT-P6 D5c. + selfBell->SetDeathWitness(m_socketTransport->StreamFd()); + m_socketSelfBell = Move(selfBell); } // `cp`: from here the twelve EGL forwarders cross the socket instead of @@ -896,6 +930,32 @@ namespace MobileGL::MG_Remote::Client { const MobileGLResult received = ReceiveEnvelope(*m_transport, reply, kRemoteControlTimeoutMs); if (received != MOBILEGL_OK) { + // `dl` (D5b): THE EXPIRY IS NOT FATAL AND IT IS NOT ONE ANSWER. The transport + // already distinguishes the two states this wait can end in, and the contract + // says they must stay distinguished without a threshold: + // + // TRANSPORT_CLOSED - the control socket returned EOF. Only the server held the + // far end, so it is gone. That is the device-loss fact, taken from a + // descriptor. + // TIMEOUT - nothing arrived in the budget, and the socket is still open. The + // peer is ALIVE AND SILENT: wedged, frozen, SIGSTOP'd, or merely slower than + // the budget. 5.4 forbids calling that a death, so it gets a NAME instead. + // The bell is asked anyway, because a hangup noticed by the ring side counts + // here too. + if (received == MOBILEGL_ERR_TRANSPORT_CLOSED) { + LatchDeviceLost("the control stream reached EOF while a reply was owed"); + } else if (const Transport::Doorbell* bell = m_producer.SelfDoorbell(); + bell != nullptr && bell->PeerHungUp()) { + LatchDeviceLost("a control reply timed out and the peer had hung up"); + } else { + MGLOG_E("MG_Remote client: the server is ALIVE BUT SILENT - no SurfaceReply " + "for %s seq %llu within %u ms and the control stream is still open. " + "This is NOT device loss (CONTRACT-P6 5.4): a frozen or merely slow " + "peer produces no hangup, and only a hangup may arm the latch. If " + "this repeats, the server is wedged rather than gone.", + Server::SurfaceControlOpName(frame.kind), + static_cast(frame.seq), kRemoteControlTimeoutMs); + } MGLOG_E("MG_Remote client: no SurfaceReply for seq %llu within %u ms (rc=%d)", static_cast(frame.seq), kRemoteControlTimeoutMs, static_cast(received)); @@ -1418,6 +1478,15 @@ namespace MobileGL::MG_Remote::Client { const Transport::SessionWait wait = WaitForAppliedBudget(m_producer, m_events, seq, waitBudgetMs); if (wait == Transport::SessionWait::ShutDown) { + // `dl`: SHUTDOWN IS TWO DIFFERENT EVENTS AND ONLY ONE IS A DEVICE LOSS. Stop() kills + // this same bell to wake a parked applier, so an orderly exit arrives here too - and + // latching on Dead() alone would arm the device-lost flag on every clean teardown. + // PeerHungUp() is true only when a descriptor whose far end ONLY THE SERVER held + // reported hangup, which nothing this side can cause. + if (const Transport::Doorbell* bell = m_producer.SelfDoorbell(); + bell != nullptr && bell->PeerHungUp()) { + LatchDeviceLost("the barrier woke on a peer that had hung up"); + } // 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 // survives a server that is gone. It is teardown, not a server fault - so the answer @@ -1564,6 +1633,12 @@ namespace MobileGL::MG_Remote::Client { const Transport::SessionWait wait = WaitForAppliedBudget(m_producer, m_events, target, kBarrierTimeoutMs); if (wait == Transport::SessionWait::ShutDown) { + // `dl`: the same two-events distinction the verb barrier makes. Only a peer that + // HUNG UP is a device loss; Stop() reaches this path too. + if (const Transport::Doorbell* bell = m_producer.SelfDoorbell(); + bell != nullptr && bell->PeerHungUp()) { + LatchDeviceLost("a forced run-ahead wait woke on a peer that had hung up"); + } // The doorbell died: teardown, not a fault - the same answer EmitAndWaitTails // gives, and for the same reason. MGLOG_E("MG_Remote client: the forced wait for %s woke on a dead doorbell; the " diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index 194df5df..a9adea71 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -35,6 +35,8 @@ #pragma once #include +#include // the device-lost latch is read off the GL thread + #include #include @@ -64,6 +66,30 @@ namespace MobileGL::MG_Remote::Client { // Null until Start() succeeds; MG_Backend::Init() is the only caller of Start(). static ClientSession* Active(); + // P6 `dl` (CONTRACT-P6 5.3). THE DEVICE-LOST LATCH, and the two words that matter are + // SESSION-SCOPED and LATCH: it is set at most once per session and never cleared, because + // a session whose server died has nothing left to recover into. MOBILEGL_IPC_RESPAWN is + // where the recovery would go and it is a named refusal until some stage writes it. + // + // SET FROM Doorbell::PeerHungUp(), NEVER FROM A TIMEOUT. 5.4's whole point is that slow + // and dead are different states and must be told apart without a threshold: under P5e + // run-ahead a server one frame behind is the INTENDED steady state, so a latch armed by a + // deadline would fire on a healthy session under load. The descriptor answers instead. + // + // The static form answers false whenever no session is active, so monolith and the pull + // build get the honest answer with no branch of their own. + static Bool DeviceLost(); + + // Arms the latch and says why, once. Safe to call repeatedly and from any thread; only + // the first call logs. + void LatchDeviceLost(const char* why); + + // The bell this session parks on, for the `dl` red-once ONLY. A test cannot otherwise + // ask the question the latch turns on - "did the peer hang up" is a fact about a + // descriptor, and the pair of tests that pin D5c has to be able to kill a server and + // then watch this object, not a side effect three layers up. + Transport::Doorbell* SelfDoorbellForTest() { return m_producer.SelfDoorbell(); } + ~ClientSession(); // Builds the four segments, performs Hello/Welcome, takes the first CapsSnapshot, and @@ -414,6 +440,9 @@ namespace MobileGL::MG_Remote::Client { Transport::ReplySlotPool m_replies; Transport::ITransport* m_transport = nullptr; Bool m_started = false; + // Written by whichever thread first notices the hangup - the GL thread in the barrier, + // or the event pump - and read by glGetGraphicsResetStatus on the GL thread. + std::atomic m_deviceLost{false}; }; // One per process in P5, because P5 serves one context, and LEAKED AT EXIT like every other diff --git a/MobileGL/MG_Remote/Transport/Doorbell.cpp b/MobileGL/MG_Remote/Transport/Doorbell.cpp index 606f7f29..a6926c01 100644 --- a/MobileGL/MG_Remote/Transport/Doorbell.cpp +++ b/MobileGL/MG_Remote/Transport/Doorbell.cpp @@ -22,6 +22,22 @@ // Same fallback as FdPassing.cpp: on macOS / BSD the protection is SO_NOSIGPIPE on the // socket, set in SocketDoorbell's constructor, not a per-send flag. +// THE WITNESS'S `events`, and the whole reason it can watch a socket it must not read. +// +// POLLRDHUP is what Linux and bionic report when the PEER shuts down its writing end - the exact +// fact the death witness wants - and asking for it alone means poll never reports POLLIN for that +// descriptor, so unread control replies queued on it can neither wake this bell nor be consumed +// from under SocketTransport's reassembler. POLLHUP / POLLERR / POLLNVAL arrive in revents whether +// requested or not, so an 0 here would still catch a full close; POLLRDHUP only makes the +// half-close case - the one a dying peer actually produces first - visible too. +#if !defined(_WIN32) +#if defined(POLLRDHUP) +#define MOBILEGL_POLL_PEER_HANGUP POLLRDHUP +#else +#define MOBILEGL_POLL_PEER_HANGUP 0 +#endif +#endif + #if !defined(_WIN32) && !defined(MSG_NOSIGNAL) #define MSG_NOSIGNAL 0 #endif @@ -275,7 +291,7 @@ namespace MobileGL::MG_Remote::Transport { if (written < 0 && (errno == EPIPE || errno == ECONNRESET)) { // The peer is gone: it can never ring back either, so latch it // here too rather than waiting for a Park to discover it. - m_dead = true; + m_dead.store(true, std::memory_order_release); return; } MGLOG_D("MG_Remote doorbell: send failed (errno=%d)", errno); @@ -289,7 +305,7 @@ namespace MobileGL::MG_Remote::Transport { if (m_fd < 0) { return false; } - if (m_fd < 0 || m_dead) { + if (m_fd < 0 || m_dead.load(std::memory_order_acquire)) { return false; } const auto start = std::chrono::steady_clock::now(); @@ -302,10 +318,22 @@ namespace MobileGL::MG_Remote::Transport { const long long remaining = static_cast(timeoutMs) - elapsed; pollTimeout = remaining <= 0 ? 0 : static_cast(remaining); } - struct pollfd pfd{}; - pfd.fd = m_fd; - pfd.events = POLLIN; - const int ready = ::poll(&pfd, 1, pollTimeout); + // THE WITNESS ASKS FOR HANGUP AND NOTHING ELSE. POLLHUP/POLLERR/POLLNVAL are + // reported in revents whether or not they were requested, so an `events` of + // POLLRDHUP alone gets the peer's shutdown and never POLLIN - which is the whole + // point: the control socket legitimately carries unread REPLY bytes, and asking for + // readability there would make every queued reply a wakeup, spin this loop, and race + // SocketTransport's reassembler for the same bytes. Nothing is ever recv'd from it. + struct pollfd pfds[2]{}; + pfds[0].fd = m_fd; + pfds[0].events = POLLIN; + const bool watching = m_witnessFd >= 0; + if (watching) { + pfds[1].fd = m_witnessFd; + pfds[1].events = MOBILEGL_POLL_PEER_HANGUP; + } + const int ready = ::poll(pfds, watching ? 2 : 1, pollTimeout); + struct pollfd& pfd = pfds[0]; if (ready < 0) { if (errno == EINTR) { continue; // a signal is not a wakeup; keep the deadline @@ -316,6 +344,20 @@ namespace MobileGL::MG_Remote::Transport { if (ready == 0) { return false; // timed out } + // THE WITNESS FIRST, because a peer that is gone makes every other answer stale. It + // reports only hangup shapes by construction, so any revent on it IS the death. + if (watching && pfds[1].revents != 0) { + MGLOG_I("MG_Remote doorbell: the peer hung up (witness fd %d, revents=0x%X) - " + "this is the DEATH FACT, taken from a descriptor and not from a " + "deadline: a server one frame behind is the intended steady state and " + "must never be mistaken for one that is gone (CONTRACT-P6 5.4)", + m_witnessFd, static_cast(pfds[1].revents)); + // THE CAUSE BEFORE THE FACT, so a reader that sees Dead() can never find + // PeerHungUp() still false and conclude "orderly teardown". + m_peerHungUp.store(true, std::memory_order_release); + m_dead.store(true, std::memory_order_release); + return false; + } // revents has to be inspected, not just `ready > 0`. Once the peer // closes its end the descriptor is permanently poll-ready with // nothing to read (measured on Linux: revents=POLLIN|POLLHUP, @@ -326,14 +368,14 @@ namespace MobileGL::MG_Remote::Transport { if ((pfd.revents & (POLLERR | POLLNVAL)) != 0) { MGLOG_D("MG_Remote doorbell: fd %d unusable (revents=0x%X)", m_fd, static_cast(pfd.revents)); - m_dead = true; + m_dead.store(true, std::memory_order_release); return false; } if ((pfd.revents & POLLIN) != 0) { if (Drain() != 0) { return true; // a real wakeup byte } - if (m_dead) { + if (m_dead.load(std::memory_order_acquire)) { return false; // EOF, not an event } // Ready but empty and still alive: someone else drained it. @@ -341,7 +383,7 @@ namespace MobileGL::MG_Remote::Transport { return true; } if ((pfd.revents & POLLHUP) != 0) { - m_dead = true; + m_dead.store(true, std::memory_order_release); return false; } // Readiness with no bit we requested or recognise: there is @@ -349,7 +391,7 @@ namespace MobileGL::MG_Remote::Transport { // poll this descriptor again. MGLOG_D("MG_Remote doorbell: fd %d ready with revents=0x%X", m_fd, static_cast(pfd.revents)); - m_dead = true; + m_dead.store(true, std::memory_order_release); return false; } } @@ -368,7 +410,7 @@ namespace MobileGL::MG_Remote::Transport { if (got == 0) { // Orderly shutdown on a stream socket: the peer is gone and // will never ring again. - m_dead = true; + m_dead.store(true, std::memory_order_release); return consumed; } if (errno == EINTR) { @@ -378,13 +420,13 @@ namespace MobileGL::MG_Remote::Transport { return consumed; // drained } MGLOG_D("MG_Remote doorbell: recv failed (errno=%d)", errno); - m_dead = true; + m_dead.store(true, std::memory_order_release); return consumed; } } void SocketDoorbell::Reset() { - if (m_fd < 0 || m_dead) { + if (m_fd < 0 || m_dead.load(std::memory_order_acquire)) { return; } (void)Drain(); diff --git a/MobileGL/MG_Remote/Transport/Doorbell.h b/MobileGL/MG_Remote/Transport/Doorbell.h index 43f1dc1b..afe230d1 100644 --- a/MobileGL/MG_Remote/Transport/Doorbell.h +++ b/MobileGL/MG_Remote/Transport/Doorbell.h @@ -171,6 +171,17 @@ namespace MobileGL::MG_Remote::Transport { // bell that cannot die. virtual bool Dead() const { return false; } + // P6 `dl`: DEAD AND DEAD-BECAUSE-THE-PEER-DIED ARE NOT THE SAME FACT, and only the + // second one is a device loss. Dead() is also true after an ORDERLY teardown - + // CondVarDoorbell::Kill() is how Stop() wakes a parked applier, and the barrier reports + // SessionWait::ShutDown for that too. Latching device-lost from Dead() would therefore + // arm it on every clean exit. + // + // This says the peer HUNG UP: a descriptor whose far end only the peer held reported + // hangup. Nothing this side did can produce it. False for every doorbell that has no + // peer process, which is what makes the whole latch a no-op under inproc and monolith. + virtual bool PeerHungUp() const { return false; } + // How many Wait() calls on this bell exhausted their spin budget and // really blocked. THE WAIT LEDGER'S RAW READING: the pair (waits, parks) // is what says whether the spin budget is sized for the workload, and it @@ -419,10 +430,30 @@ namespace MobileGL::MG_Remote::Transport { void Notify() override; bool Park(std::uint32_t timeoutMs) override; void Reset() override; - bool Dead() const override { return m_dead; } + bool Dead() const override { return m_dead.load(std::memory_order_acquire); } + bool PeerHungUp() const override { return m_peerHungUp.load(std::memory_order_acquire); } int Fd() const { return m_fd; } + // P6 `dl` (CONTRACT-P6 D5c). THE DESCRIPTOR THAT ANSWERS "IS THE PEER STILL THERE", + // which for this object can never be the one it parks on. + // + // The client parks on clientBell[0] and rings ITSELF through clientBell[1], so it holds + // BOTH ends of that socketpair. EOF arrives only when every writer closes and the client + // is one of them, so that descriptor cannot hang up no matter what happens to the server. + // A bell that can wake itself is a bell that cannot hear a death. Measured: a server that + // died on its first Clear left the client waiting out the full 120 s barrier and + // reporting Fatal{BarrierTimeout} - the wrong diagnosis, two minutes late. + // + // The witness is a descriptor whose FAR END ONLY THE PEER HOLDS - in practice the control + // socket. It is NOT OWNED here and is NEVER READ FROM: Park adds it to the poll set with + // `events` asking for hangup ALONE, so a control reply sitting unread in its queue cannot + // wake the bell and cannot be consumed from under SocketTransport's reassembler. What it + // contributes is exactly one fact, and only once: the peer is gone. + // + // -1 disables it, which is what every bell that has no peer socket uses. + void SetDeathWitness(int fd) { m_witnessFd = fd; } + private: // Consumes every queued wakeup byte and returns how many. Latches // m_dead on EOF: recv returning 0 on a stream socket is the peer's @@ -432,9 +463,16 @@ namespace MobileGL::MG_Remote::Transport { int m_fd; // polled int m_notifyFd; // written; equals m_fd in the single-fd (cross-process) form + int m_witnessFd = -1; // polled for hangup only; NOT owned, NEVER read + // Set ONLY by the witness branch of Park, never by Kill or by our own EOF. See + // Doorbell::PeerHungUp for why the distinction is the whole design. + std::atomic m_peerHungUp{false}; std::uint8_t m_code; bool m_ownsFd; - bool m_dead = false; + // ATOMIC BECAUSE THE LATCH READS IT ACROSS THREADS. Park() runs on whichever thread is + // waiting; the device-lost latch is consulted from the GL thread and from + // glGetGraphicsResetStatus. It was a plain bool while Dead() had no cross-thread reader. + std::atomic m_dead{false}; }; #endif diff --git a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp index f65ef91f..03a152f5 100644 --- a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp @@ -43,6 +43,7 @@ #include #include +#include #include #include #include @@ -50,6 +51,7 @@ #include #if !defined(_WIN32) +#include #include #endif @@ -191,6 +193,117 @@ TEST(ServerSpawnTest, AnEglControlOpCrossesToTheOtherProcessAndAnswers) { ASSERT_EQ(Server::ReapServer(session.server, 5000, &exitCode), MOBILEGL_OK); } +TEST(ServerSpawnTest, AKilledServerLatchesDeviceLostFromTheHangupAndNotFromADeadline) { + // `dl`, and exit gate S2: kill -9 the server mid-session and the client must find out FROM A + // DESCRIPTOR. This is the red-once for CONTRACT-P6 D5c, whose whole content is that the + // client's own bell CANNOT witness the death - it holds both ends of that socketpair, so the + // fact has to come from somewhere else. + // + // WHAT MAKES THIS FALSIFIABLE IS THE CLOCK, not the boolean. A timeout would eventually + // report "gone" too, and 5.4 forbids that: under P5e run-ahead a server one frame behind is + // the INTENDED steady state, so anything armed by a deadline fires on a healthy session under + // load. The bound below is therefore deliberately far under every wait in the system - the + // verb barrier is 120000 ms and the control reply 5000 ms - so a pass here cannot be a + // timeout wearing the right answer. + Session session; + ASSERT_TRUE(Bring("devicelost", &session)); + ASSERT_EQ(Handshake(session), MOBILEGL_OK); + EXPECT_FALSE(Client::ClientSession::DeviceLost()) + << "a healthy session must not read as a lost device"; + + Transport::Doorbell* bell = Client::ClientSessionInstance().SelfDoorbellForTest(); + ASSERT_NE(bell, nullptr); + EXPECT_FALSE(bell->PeerHungUp()) << "nothing has hung up yet"; + + ASSERT_EQ(::kill(session.server.pid, SIGKILL), 0); + + // Park with a SHORT budget, repeatedly, until the hangup lands. Each Park is what a real + // waiter does; the loop exists because the kill is asynchronous and the first poll may win + // the race. 200 x 10 ms is two seconds of patience against waits measured in minutes. + const auto start = std::chrono::steady_clock::now(); + for (int attempt = 0; attempt < 200 && !bell->PeerHungUp(); ++attempt) { + (void)bell->Park(10); + } + const auto elapsedMs = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start).count(); + + EXPECT_TRUE(bell->PeerHungUp()) + << "the witness never reported the hangup; the bell is watching a descriptor this side " + "also holds, which is the exact defect D5c names"; + EXPECT_TRUE(bell->Dead()) << "a peer that hung up is a dead bell"; + EXPECT_LT(elapsedMs, 3000) + << "took " << elapsedMs << " ms: too slow to be a hangup and fast enough only for a " + "descriptor, so this assertion is what separates the two mechanisms"; + + // AND THE LATCH ITSELF. PeerHungUp is the cause; DeviceLost is the session-scoped + // consequence, and it is what glGetGraphicsResetStatus reports. + Client::ClientSessionInstance().LatchDeviceLost("the test killed the server"); + EXPECT_TRUE(Client::ClientSession::DeviceLost()); + + Client::ClientSessionInstance().Stop(); + int exitCode = -1; + (void)Server::ReapServer(session.server, 5000, &exitCode); +} + +TEST(ServerSpawnTest, AnOrderlyStopIsNotADeviceLoss) { + // The session-level control: a clean shutdown must leave the latch disarmed, or every exit + // would report a context reset and the status would mean nothing. + // + // NOTE WHAT THIS DOES *NOT* PROVE, because the first draft of this comment claimed it did + // and the R-16 falsification caught the lie. Collapsing PeerHungUp() into Dead() leaves this + // test green: Stop() kills the bell through CondVarDoorbell::Kill(), and under SPAWN the + // self bell is a SocketDoorbell, which has no Kill at all - so Dead() is false here either + // way. ADeadBellIsNotAlwaysAHungUpPeer below is the case that separates them. + Session session; + ASSERT_TRUE(Bring("orderly", &session)); + ASSERT_EQ(Handshake(session), MOBILEGL_OK); + + Transport::Doorbell* bell = Client::ClientSessionInstance().SelfDoorbellForTest(); + ASSERT_NE(bell, nullptr); + + Client::ClientSessionInstance().Stop(); + int exitCode = -1; + ASSERT_EQ(Server::ReapServer(session.server, 5000, &exitCode), MOBILEGL_OK); + EXPECT_EQ(exitCode, 0) << "the server must have exited cleanly, not been killed"; + + EXPECT_FALSE(bell->PeerHungUp()) + << "an orderly Stop() must not look like a peer that died"; + EXPECT_FALSE(Client::ClientSession::DeviceLost()) + << "a clean teardown armed the device-lost latch; every exit would report a reset"; +} + +TEST(ServerSpawnTest, ADeadBellIsNotAlwaysAHungUpPeer) { + // THE ASSERTION THAT MAKES `dl`'s CAUSE/FACT SPLIT LOAD-BEARING, and it exists because the + // session-level control above turned out not to. Park() latches m_dead for several reasons + // that are NOT a peer death - POLLERR, POLLNVAL, an unrecognised revents - and each of them + // is a fault in THIS process's descriptor. A latch armed from Dead() would report a lost + // GPU for a bug in our own fd handling. + // + // Falsified by construction: make PeerHungUp() return Dead() and this goes red, which is + // exactly what the contract's D5c distinction has to be able to do. + int pair[2] = {-1, -1}; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, pair), 0); + + // ownsFds=false: the bell must not close what this test is about to invalidate underneath + // it, or the destructor closes a descriptor number somebody else has since been given. + Transport::SocketDoorbell bell(pair[0], pair[1], /*code=*/1, /*ownsFds=*/false); + bell.SetDeathWitness(-1); // no witness: nothing here can legitimately report a hangup + EXPECT_FALSE(bell.Dead()); + EXPECT_FALSE(bell.PeerHungUp()); + + // Close the park end UNDER the bell. poll() then answers POLLNVAL, which Park treats as a + // descriptor it must never poll again - dead, but not bereaved. + ASSERT_EQ(::close(pair[0]), 0); + (void)bell.Park(50); + + EXPECT_TRUE(bell.Dead()) << "a bell polling a closed fd cannot be anything but dead"; + EXPECT_FALSE(bell.PeerHungUp()) + << "our own descriptor went bad and the bell called it a peer death; a device-lost latch " + "taken from this would report a lost GPU for a local fd bug"; + + ::close(pair[1]); +} + TEST(ServerSpawnTest, AnAbstractEndpointNeedsNoWritableDirectoryAndLeavesNoFile) { // THE ONLY RENDEZVOUS THAT WORKS ON ANDROID, and the reason is not a preference. // An app has no writable /tmp; the first device run of the spawn retrace launched diff --git a/MobileGL/MG_Util/Debug/Log.cpp b/MobileGL/MG_Util/Debug/Log.cpp index 7b90a488..57ecf710 100644 --- a/MobileGL/MG_Util/Debug/Log.cpp +++ b/MobileGL/MG_Util/Debug/Log.cpp @@ -92,12 +92,23 @@ namespace MobileGL { // it is always started by a client that has already opened // this path, and a truncate from the far side would delete // the very startup lines a failed session needs. + // + // GUARDED, BECAUSE G1 MEASURES BYTES AND NOT INTENT. Only a disaggregated + // build can ever have a second process on this path, so a pull build gains + // nothing from the append - and it would pay for it in .text, which G1 + // requires to be unchanged. Measured: unguarded, the pull library's .text + // moved 16 bytes with its symbol set identical, which is exactly the kind of + // drift that is invisible unless somebody looks. +#if MOBILEGL_BUILD_DISAGGREGATED const char* role = std::getenv("MOBILEGL_IPC_ROLE"); const bool isSpawnedServer = role != nullptr && std::strcmp(role, "server") == 0; if (!isSpawnedServer) { if (FILE* truncate = std::fopen(logPath, "w")) std::fclose(truncate); } s_logFile = std::fopen(logPath, "a"); +#else + s_logFile = std::fopen(logPath, "w"); +#endif } } #endif