[Fix] (MG_Remote/Server): ServerLoop - clear m_running under m_controlMutex and re-check before publishing (C2); a backend with no thread is Fatal{ApplyThreadNotRunning} not an inline app-thread fallback (M-7); ApplyAffinity logs the effective sched_getaffinity mask (M-4/codex-11); ServerMakeEGLCurrent binds once per tuple and records a client release without unbinding (C7/ID-54); Start resets its own diagnostics (m-3); doorbell Fatal text corrected (m-4)

MobileGL/MG_Remote/Server/ServerLoop.cpp

@@ -101,7 +101,12 @@ namespace MobileGL::MG_Remote::Server {
 #endif
         }
 
-        // Returns the mask that was actually APPLIED, which is 0 when nothing was.
+        // Returns the mask that the kernel ACTUALLY applied - the EFFECTIVE mask read back with
+        // sched_getaffinity, not the requested one (codex 11). A cpuset that permits only a subset
+        // of the requested cpus is accepted by sched_setaffinity with the intersection, and
+        // logging the request would then claim cpus the thread never ran on - which is precisely
+        // the "an affinity that silently did nothing looks like one that worked" failure the
+        // resolved mask exists to make visible. 0 when nothing was applied.
         Uint64 ApplyAffinity(Uint64 requested) {
             if (requested == 0) return 0;
 #if defined(__linux__) || defined(__ANDROID__)
@@ -116,7 +121,19 @@ namespace MobileGL::MG_Remote::Server {
             }
             if (applied == 0) return 0;
             if (sched_setaffinity(0, sizeof(set), &set) != 0) return 0;
-            return requested;
+            // Read back the mask the kernel really honoured. This is the codex-11 fix: the log
+            // and ResolvedAffinityMask() report the EFFECTIVE set, so a request the cpuset trimmed
+            // is visible as a smaller resolved mask rather than as a lie that matched the request.
+            cpu_set_t effective;
+            CPU_ZERO(&effective);
+            if (sched_getaffinity(0, sizeof(effective), &effective) != 0) {
+                return requested; // best effort: the set succeeded, the read did not
+            }
+            Uint64 mask = 0;
+            for (Uint cpu = 0; cpu < 64; ++cpu) {
+                if (CPU_ISSET(cpu, &effective)) mask |= (1ull << cpu);
+            }
+            return mask;
 #else
             return 0;
 #endif
@@ -188,6 +205,15 @@ namespace MobileGL::MG_Remote::Server {
         }
         m_session = &session;
         m_stopRequested.store(false, std::memory_order_release);
+        // m-3: reset THIS session's own diagnostics. DrainedRecords()/ParkCount() are absolute and
+        // a case asserts == N, so a process that opens a SECOND session (context loss, or
+        // Initialize after Destroy) must start them at zero rather than carry the first session's
+        // tally - and a developer running the binary directly gets the count CI sees.
+        m_drained.store(0, std::memory_order_release);
+        m_parks.store(0, std::memory_order_release);
+        m_nativeBinds.store(0, std::memory_order_release);
+        m_clientReleases.store(0, std::memory_order_release);
+        m_haveCurrentTuple = false;
         {
             const std::lock_guard<std::mutex> lock(m_exitMutex);
             m_exited = false;
@@ -208,6 +234,65 @@ namespace MobileGL::MG_Remote::Server {
     Uint64 ServerLoop::ResolvedAffinityMask() const { return m_affinityMask; }
     Uint64 ServerLoop::DrainedRecords() const { return m_drained.load(std::memory_order_acquire); }
     Uint64 ServerLoop::ParkCount() const { return m_parks.load(std::memory_order_acquire); }
+    Uint64 ServerLoop::NativeBindCount() const { return m_nativeBinds.load(std::memory_order_acquire); }
+    Uint64 ServerLoop::ClientReleaseCount() const {
+        return m_clientReleases.load(std::memory_order_acquire);
+    }
+
+    // C7 / ID-54. A release-current request (the three NO_* markers, exactly IsReleaseCurrentRequest's
+    // test in BackendObject_DirectGLES.cpp) is a ClientRelease the server records but does not
+    // forward. Otherwise an identical (dpy, draw, read, ctx) already held is a RepeatNoOp, and
+    // anything else is a real NativeBind. Pure: a unit case drives it with no EGL context.
+    EglBindAction ClassifyEglMakeCurrent(Bool haveCurrent, EGLDisplay curDpy, EGLSurface curDraw,
+                                         EGLSurface curRead, EGLContext curCtx, EGLDisplay dpy,
+                                         EGLSurface draw, EGLSurface read, EGLContext ctx) {
+        if (draw == EGL_NO_SURFACE && read == EGL_NO_SURFACE && ctx == EGL_NO_CONTEXT) {
+            return EglBindAction::ClientRelease;
+        }
+        if (haveCurrent && curDpy == dpy && curDraw == draw && curRead == read && curCtx == ctx) {
+            return EglBindAction::RepeatNoOp;
+        }
+        return EglBindAction::NativeBind;
+    }
+
+    ServerLoop::MakeCurrentOutcome ServerLoop::ApplyMakeCurrent(MG_Backend::BackendObject* backend,
+                                                               EGLDisplay dpy, EGLSurface draw,
+                                                               EGLSurface read, EGLContext ctx) {
+        // Apply thread only (RunOnApplyThread put us here), so m_haveCurrentTuple/m_cur* need no
+        // lock. The counters are atomic for the reader on the test thread.
+        MakeCurrentOutcome outcome;
+        const EglBindAction action = ClassifyEglMakeCurrent(m_haveCurrentTuple, m_curDpy, m_curDraw,
+                                                            m_curRead, m_curCtx, dpy, draw, read, ctx);
+        switch (action) {
+        case EglBindAction::ClientRelease:
+            // Recorded, NOT forwarded (ID-54): the context stays current on this thread until
+            // ~BackendObject_DirectGLES or context loss. Forwarding backend->MakeEGLCurrent here
+            // would route to DirectGLES::ReleaseCurrent, unbind, and clear
+            // g_backendContextOwnerThread - reinstating the off-thread degradation the phase
+            // removes. m_haveCurrentTuple is left intact so a later identical bind is still a
+            // RepeatNoOp (the driver never lost the context).
+            m_clientReleases.fetch_add(1, std::memory_order_acq_rel);
+            outcome.ok = true;
+            outcome.boundNatively = false;
+            return outcome;
+        case EglBindAction::RepeatNoOp:
+            outcome.ok = true;
+            outcome.boundNatively = false;
+            return outcome;
+        case EglBindAction::NativeBind:
+            break;
+        }
+        outcome.ok = backend->MakeEGLCurrent(dpy, draw, read, ctx);
+        if (!outcome.ok) return outcome;
+        m_haveCurrentTuple = true;
+        m_curDpy = dpy;
+        m_curDraw = draw;
+        m_curRead = read;
+        m_curCtx = ctx;
+        m_nativeBinds.fetch_add(1, std::memory_order_acq_rel);
+        outcome.boundNatively = true;
+        return outcome;
+    }
 
     void ServerLoop::ApplyThreadMain() {
         m_applyThreadId.store(std::this_thread::get_id(), std::memory_order_release);
@@ -305,8 +390,16 @@ namespace MobileGL::MG_Remote::Server {
             m_backend.reset();
         }
 
-        // Anything still parked in RunOnApplyThread has to be answered rather than left
+        // C2: THE m_running CLEAR IS INSIDE THIS SAME CRITICAL SECTION AS THE FINAL DRAIN OF THE
-        // waiting: this thread is the only thing that could have run it.
+        // MAILBOX. It used to be a separate store after the block, and that gap was a lost-forever
+        // hang: a caller that read m_running == true just before this thread exited, then had this
+        // thread run the block (finding nothing pending) and clear m_running OUTSIDE the lock,
+        // would go on to publish a request into a mailbox no thread will ever pump and block on
+        // m_controlDone with no answer possible (the bounded join has already succeeded, so there
+        // is not even a Fatal{ApplyThreadJoinTimeout}). With the clear under the lock, a caller
+        // either takes the lock FIRST (its request is here and answered NOT_INITIALIZED) or takes
+        // it AFTER (it sees !m_running under the lock in RunOnApplyThread and returns
+        // NOT_INITIALIZED without publishing). There is no third order.
         {
             const std::lock_guard<std::mutex> lock(m_controlMutex);
             if (m_controlPending) {
@@ -316,10 +409,10 @@ namespace MobileGL::MG_Remote::Server {
                 MGLOG_E("MG_Remote server: a control request was still posted when mgl-srv-apply "
                         "exited; it is answered NOT_INITIALIZED rather than left blocking");
             }
+            m_running.store(false, std::memory_order_release);
         }
         m_controlDone.notify_all();
 
-        m_running.store(false, std::memory_order_release);
         SignalExited();
     }
 
@@ -420,29 +513,52 @@ namespace MobileGL::MG_Remote::Server {
         // applier calls that wants "run this where the context is". Running inline is the
         // correct answer there and the only non-hanging one.
         if (OnApplyThread()) return work(user);
-        // NO THREAD YET, OR ALREADY GONE: run inline on the caller. That is right for the
-        // window between MG_Backend::Init()'s hook and ClientSession::Start (the backend object
-        // exists, the thread does not, and nothing has made a context current), and for the
-        // window after Stop(). It is NOT a silent fallback to monolith: the thread's absence in
-        // those two windows is a fact about the lifecycle, not about the transport.
-        if (!m_running.load(std::memory_order_acquire)) return work(user);
 
         const std::lock_guard<std::mutex> callerLock(m_callerMutex);
-        {
+        std::unique_lock<std::mutex> lock(m_controlMutex);
-            std::unique_lock<std::mutex> lock(m_controlMutex);
+        // C2 + M-7: the m_running check and the publish are ONE critical section, and m_running is
-            m_controlWork = work;
+        // cleared under this same lock on the way out (ApplyThreadMain's exit block), so this read
-            m_controlUser = user;
+        // cannot see `true` for a thread that then vanishes before the publish. When there is no
-            m_controlPending = true;
+        // thread there are two sub-cases and neither is a silent EGL-on-the-app-thread fallback:
-            m_controlFinished = false;
+        if (!m_running.load(std::memory_order_acquire)) {
+            const Bool backendAlive = m_backend != nullptr;
+            lock.unlock();
+            if (backendAlive) {
+                // M-7: the backend exists but no thread owns it - ClientSession::Start refused
+                // (ServerLoop.cpp Start's !Accepted arm) or std::thread's constructor threw.
+                // Running `work` inline here would issue eglMakeCurrent on the APP thread and
+                // stamp g_backendContextOwnerThread with it, so a lane called split would render
+                // correctly with the context on the wrong thread - the one outcome R-1 exists to
+                // make impossible. Fatal by name, never a fallback; compare CreateBackend's
+                // default: arm, which also refuses rather than substitutes.
+                MGLOG_F("MGPipe: Fatal{ApplyThreadNotRunning, \"EGL on the app thread\"} - a "
+                        "control request reached RunOnApplyThread with the server's backend built "
+                        "but no mgl-srv-apply thread running (ClientSession::Start failed after "
+                        "ServerLoop::CreateBackend). Running it inline would make the context "
+                        "current on the APP thread - the split lane's whole premise. Refusing by "
+                        "name rather than falling back to monolith");
+                std::abort();
+            }
+            // Backend already gone (post-Stop teardown, or the pre-Init window): the forwarders'
+            // ServerBackendOrNull() would answer null anyway, so NOT_INITIALIZED is the honest
+            // result and running inline is pointless. This is the deterministic half of C2 - a
+            // forwarder call after the loop stopped returns NOT_INITIALIZED, it does not hang and
+            // it does not run on the caller.
+            return MOBILEGL_ERR_NOT_INITIALIZED;
         }
+        m_controlWork = work;
+        m_controlUser = user;
+        m_controlPending = true;
+        m_controlFinished = false;
         // Publish THEN ring, in that order and never the other (Doorbell.h:186-193). The bell
         // is rung unconditionally rather than through NotifyIfParked because this side does not
         // know whether the apply thread is parked or spinning, and CondVarDoorbell remembers a
-        // wakeup that arrives while nobody is waiting.
+        // wakeup that arrives while nobody is waiting. Held under m_controlMutex: wait() releases
+        // it atomically, so the apply thread cannot observe the request until this side is
+        // waiting, and the doorbell remembers the notify regardless.
         if (m_session != nullptr) {
             m_session->ConsumerDoorbell().Notify();
         }
-        std::unique_lock<std::mutex> lock(m_controlMutex);
         m_controlDone.wait(lock, [this] { return m_controlFinished; });
         return m_controlResult;
     }
@@ -488,11 +604,12 @@ namespace MobileGL::MG_Remote::Server {
             // a use-after-free whose only symptom is an intermittent crash somewhere else.
             // Aborting here is red, immediate, and names the cause.
             MGLOG_F("MGPipe: Fatal{ApplyThreadJoinTimeout} - mgl-srv-apply did not exit within "
-                    "%u ms of Stop(). It parks on Doorbell::Wait(kWaitForever), which only "
+                    "%u ms of Stop(). Stop() published m_stopRequested (in the park predicate) "
-                    "CondVarDoorbell::Kill() can break (Doorbell.h:211-221, table 3 step 2), so "
+                    "BEFORE it rang, so a plain Notify should already have un-parked the thread; "
-                    "this is a lost wakeup and not a slow thread. Aborting rather than "
+                    "Doorbell::Kill() (Doorbell.h:211-221, table 3 step 2) is the belt to that "
-                    "detaching: a detached apply thread still owns the context and would read "
+                    "Notify's braces. If the thread is still parked after both, the wakeup was "
-                    "rings the client is about to unmap",
+                    "lost, not slow. Aborting rather than detaching: a detached apply thread still "
+                    "owns the context and would read rings the client is about to unmap",
                     kJoinTimeoutMs);
             std::abort();
         }
@@ -605,17 +722,22 @@ namespace MobileGL::MG_Remote::Server {
             MobileGLResult Run() {
                 MG_Backend::BackendObject* backend = ServerBackendOrNull();
                 if (backend == nullptr) return MOBILEGL_ERR_NOT_INITIALIZED;
-                // THE ONE eglMakeCurrent OF THE PROCESS'S LIFE, ON THIS THREAD. Every later
+                // C7 / ID-54: the apply thread binds the native context ONCE per tuple and holds
-                // client-side eglMakeCurrent onto the same surface finds the context already
+                // it for life. ApplyMakeCurrent forwards a real bind only for a new tuple, treats
-                // current here and costs nothing; the owner slot is written once.
+                // an identical repeat as a no-op, and records a client release-current WITHOUT
-                ok = backend->MakeEGLCurrent(dpy, draw, read, ctx);
+                // unbinding - so "the owner slot is written once" is true here even though
-                if (!ok) return MOBILEGL_OK;
+                // DirectGLES::MakeCurrent itself has no shortcut.
+                const ServerLoop::MakeCurrentOutcome outcome =
+                    ServerLoopInstance().ApplyMakeCurrent(backend, dpy, draw, read, ctx);
+                ok = outcome.ok;
+                if (!ok || !outcome.boundNatively) return MOBILEGL_OK;
                 // R-12, arm (a): the caps snapshot is REPUBLISHED because InitCapabilities has
-                // now run for real. DirectGLES has no OnCapsInvalidated producer at all, and
+                // now run for real - and ONLY on a real native bind, not on an identical repeat
-                // c0's answer is that a SECOND arrival IS the invalidation - so the client's
+                // (a repeat re-published nothing). DirectGLES has no OnCapsInvalidated producer at
-                // mirror is refreshed with no dev-shaped backend edit and with no eleventh
+                // all, and c0's answer is that a SECOND arrival IS the invalidation - so the
-                // MGPipeCallbacks slot (MGPipeCallbacks.h:56-58's static_assert exists to make
+                // client's mirror is refreshed with no dev-shaped backend edit and with no
-                // that cost visible).
+                // eleventh MGPipeCallbacks slot (MGPipeCallbacks.h:56-58's static_assert exists to
+                // make that cost visible).
                 ServerSession* session = ServerSession::Active();
                 if (session != nullptr && session->Accepted()) {
                     const MobileGLResult published = session->PublishCapsSnapshot();

MobileGL/MG_Remote/Server/ServerLoop.h

@@ -22,17 +22,27 @@
 // P6 splits it.
 //
 // PARKING AND SHUTDOWN. The thread parks on Doorbell::Wait(consumerParked, ready, spinUs,
-// kWaitForever) and shuts down when Wait returns false with Dead() set. Doorbell::Kill()
+// kWaitForever) and shuts down when Wait returns false with Dead() set. TWO things can un-park a
-// (Doorbell.h:211-221) IS THE ONLY THING that wakes a thread parked on kWaitForever - a fact
+// kWaitForever waiter, not one, and the difference is the whole of review m-4: for the STOP case a
-// ARCHITECTURE.md's teardown order (:537) omits and InProcessTransportTest.cpp:344 already
+// plain Doorbell::Notify() is sufficient, because m_stopRequested is in the park predicate and
-// pins. Kill BEFORE join; join before the client frees any emitter-owned Vector; and the join
+// Stop() publishes it BEFORE it rings (Doorbell.h re-tests ready() after every Park return);
-// must be bounded (that test uses 5 s) so a regression is a red test and not a hung CI job.
+// Doorbell::Kill() (Doorbell.h:211-221) is load-bearing only for a predicate that has NOTHING to
+// see, which is why the redcheck's park-predicate-loses-control entry - the one that drops the
+// control flag FROM the predicate - is the case that actually times out. Kill BEFORE join; join
+// before the client frees any emitter-owned Vector; and the join must be bounded (that test uses
+// 5 s) so a regression is a red test and not a hung CI job.
 //
-// THE EGL OWNERSHIP MOVE. eglMakeCurrent runs ONCE on this thread and is never released
+// THE EGL OWNERSHIP MOVE. eglMakeCurrent runs ONCE per (dpy, draw, read, ctx) tuple on this
-// (DirectGLES.cpp:11925 plus the six cache invalidations at :11933-11953, which become a
+// thread and the context is then held for life. The "once" is NOT free at the DirectGLES layer -
-// one-time startup cost instead of a per-make-current storm). The client's nine EGL virtuals
+// DirectGLES::MakeCurrent always calls native eglMakeCurrent and rewrites the owner (codex C7) -
-// become BLOCKING control requests executed here. ReleaseEGLResources and
+// so ServerMakeEGLCurrent is where the dedup lives (ID-54): an identical repeat is a no-op apart
-// ~BackendObject_DirectGLES MUST be blocking: MobileGL::Destroy() (MobileGL/Init.cpp:68)
+// from the R-12 republish decision, a different tuple is a real rebind, and a client
+// release-current is RECORDED (NativeBindCount / ClientReleaseCount) but NOT forwarded - the apply
+// thread keeps the context current until ~BackendObject_DirectGLES or context loss, which is what
+// makes DirectGLES.cpp's six cache invalidations a one-off startup cost and the 16
+// IsBackendContextCurrentOnThisThread() / 16 CanTouchGLNow() sites answer TRUE on the server. The
+// client's nine EGL virtuals become BLOCKING control requests executed here. ReleaseEGLResources
+// and ~BackendObject_DirectGLES MUST be blocking: MobileGL::Destroy() (MobileGL/Init.cpp:68)
 // otherwise walks on while the server still holds the context.
 //
 // THE FALLBACK IS PRE-DECLARED, NOT INVENTED UNDER PRESSURE (R-1). If the context migration is
@@ -120,6 +130,24 @@ namespace MobileGL::MG_Remote::Server {
         Uint64 DrainedRecords() const;
         Uint64 ParkCount() const;
 
+        // C7 / ID-54 diagnostics, read by the C7 control. NativeBindCount is how many times
+        // ServerMakeEGLCurrent forwarded a REAL native bind (a new tuple); ClientReleaseCount how
+        // many client release-current requests were recorded-and-not-forwarded. Two identical
+        // binds must move the first by one and the second not at all.
+        Uint64 NativeBindCount() const;
+        Uint64 ClientReleaseCount() const;
+
+        // The deduped make-current, on the apply thread. Classifies the request (see
+        // ClassifyEglMakeCurrent), forwards a native bind only for a genuinely new tuple, records
+        // a release without forwarding it, and returns whether a native bind happened so the
+        // caller (ServerMakeEGLCurrent) knows whether to re-publish the caps snapshot (R-12).
+        struct MakeCurrentOutcome {
+            Bool ok = false;            // the request was honoured
+            Bool boundNatively = false; // a native eglMakeCurrent ran (=> republish caps)
+        };
+        MakeCurrentOutcome ApplyMakeCurrent(MG_Backend::BackendObject* backend, EGLDisplay dpy,
+                                            EGLSurface draw, EGLSurface read, EGLContext ctx);
+
     private:
         void ApplyThreadMain();
         // Part of the apply thread's park predicate: a posted control request must be able to
@@ -163,10 +191,30 @@ namespace MobileGL::MG_Remote::Server {
         Uint64 m_affinityMask = 0;
         std::atomic<Uint64> m_drained{0};
         std::atomic<Uint64> m_parks{0};
+
+        // C7 / ID-54: the (dpy, draw, read, ctx) currently bound on the apply thread. Written and
+        // read ONLY on the apply thread inside ApplyMakeCurrent, so it needs no lock; the two
+        // counters beside it are atomic because a test reads them from another thread.
+        Bool m_haveCurrentTuple = false;
+        EGLDisplay m_curDpy = EGL_NO_DISPLAY;
+        EGLSurface m_curDraw = EGL_NO_SURFACE;
+        EGLSurface m_curRead = EGL_NO_SURFACE;
+        EGLContext m_curCtx = EGL_NO_CONTEXT;
+        std::atomic<Uint64> m_nativeBinds{0};
+        std::atomic<Uint64> m_clientReleases{0};
     };
 
     ServerLoop& ServerLoopInstance();
 
+    // C7 / ID-54, factored out so a unit case can drive the DECISION without a live EGL context
+    // (the native bind itself needs the joint lane). Given the tuple currently held on the apply
+    // thread and the request, is this a native bind, an identical no-op repeat, or a client
+    // release-current the server must record without forwarding?
+    enum class EglBindAction { NativeBind, RepeatNoOp, ClientRelease };
+    EglBindAction ClassifyEglMakeCurrent(Bool haveCurrent, EGLDisplay curDpy, EGLSurface curDraw,
+                                         EGLSurface curRead, EGLContext curCtx, EGLDisplay dpy,
+                                         EGLSurface draw, EGLSurface read, EGLContext ctx);
+
     // ---------------------------------------------------------------------------------
     // THE EGL OWNERSHIP MOVE - the part that can sink the phase, expressed as twelve calls
     // ---------------------------------------------------------------------------------