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