[Fix] (MG_Remote, Transport): the knob names the ring and the segment adds a control page, a backwards watermark is Fatal rather than a permanent hang, RetireThrough stops at a borrowed slot, the inproc peer gets a real second mapping through dup and Adopt, and the producer remembers its own published seq

MobileGL/MG_Remote/Transport/ReplySlot.h

@@ -50,6 +50,14 @@
 #include <cstdlib>
 #include <cstring>
 
+// P9's account, named here rather than left to be rediscovered: the seq stamp
+// catches a sequence space drifted by anything that is NOT a multiple of
+// slotCount. A drift of exactly 8, 16, ... lands on the same slot with a matching
+// stamp and reads as this call's answer. Under R-1's verb barrier the in-flight
+// depth is one and a drift cannot open at all; P9 is what removes the barrier,
+// and it is what has to widen the stamp (a generation beside the seq) or bound
+// the drift some other way.
+
 namespace MobileGL::MG_Remote::Transport {
 
     // CONTRACT-P5 table 0, "reply slot header".
@@ -106,8 +114,24 @@ namespace MobileGL::MG_Remote::Transport {
                              static_cast<unsigned long long>(sizeof(ReplySlotHeader)));
                 return;
             }
+            // EVERY SLOT MUST START 8-ALIGNED. The header is written by one thread
+            // and read by another; the fences below order the payload against the
+            // stamp, but the stamp's own 8-byte Seq has to be untorn for the
+            // wrong-slot self-check to mean anything, and that is only true while
+            // it is naturally aligned. A geometry whose slotBytes is not a
+            // multiple of 8 puts later slots on odd boundaries, so it is refused
+            // here rather than left to a future caller to discover.
+            if ((slotBytes % 8) != 0 ||
+                (reinterpret_cast<std::uintptr_t>(base) % alignof(ReplySlotHeader)) != 0) {
+                WireLogError("MG_Remote reply pool: rejected, a %llu byte slot at base alignment "
+                             "%llu would put a slot header on an unaligned address, and the seq "
+                             "stamp the wrong-slot check reads has to be untorn",
+                             static_cast<unsigned long long>(slotBytes),
+                             static_cast<unsigned long long>(
+                                 reinterpret_cast<std::uintptr_t>(base) % alignof(ReplySlotHeader)));
+                return;
+            }
             m_base = static_cast<std::uint8_t*>(base);
-            m_size = sizeBytes;
             m_slots = slotCount;
             m_mask = slotCount - 1;
             // Truncated to 32 bits deliberately: the header's Size field is
@@ -242,7 +266,6 @@ namespace MobileGL::MG_Remote::Transport {
         }
 
         std::uint8_t* m_base = nullptr;
-        std::uint64_t m_size = 0;
         std::uint32_t m_slots = 0;
         std::uint32_t m_mask = 0;
         std::uint32_t m_slotBytes = 0;

MobileGL/MG_Remote/Transport/Ring.cpp

@@ -12,6 +12,7 @@
 
 #include <MG_Util/Debug/Log.h>
 
+#include <cstdlib>
 #include <cstring>
 
 namespace MobileGL::MG_Remote::Transport {
@@ -320,6 +321,14 @@ namespace MobileGL::MG_Remote::Transport {
         return value;
     }
 
+    std::uint64_t SegmentBytesForRing(std::uint64_t ringBytes) {
+        const std::uint64_t ring = LargestPowerOfTwoAtMost(ringBytes);
+        if (ring < kMinRingCapacity || ring > kMaxRingCapacity) {
+            return 0;
+        }
+        return ring + sizeof(RingControl);
+    }
+
     std::uint64_t RingCapacityForSegment(std::uint64_t segmentBytes) {
         if (segmentBytes <= sizeof(RingControl)) {
             return 0;
@@ -333,18 +342,26 @@ namespace MobileGL::MG_Remote::Transport {
         namespace {
             // A watermark may be published LATE but never EARLY, and it may never
             // move BACKWARDS. Backwards is the half that is mechanically
-            // detectable from inside, so it is refused loudly here; "early" can
+            // detectable from inside, and it is FATAL rather than logged: the
-            // only be caught at the call site, which is why every advance below
+            // consumer keeps its own counter, so a shared watermark left behind
-            // has exactly one caller and a named unit case.
+            // makes every later advance a no-op and every WaitForApplied on
+            // kWaitForever - the verb barrier and every reply wait - block for
+            // ever. A hang with one ERROR line in the log is strictly worse than
+            // an abort at the instruction that caused it, and Ring.h:181-185
+            // already rules the same way for the cursor invariants on this page.
+            // "Early" can only be caught at the call site, which is why every
+            // advance below has exactly one caller and a named unit case.
             void AdvanceMonotonic(std::atomic<std::uint64_t>& watermark, std::uint64_t to,
                                   const char* name) {
                 const std::uint64_t current = watermark.load(std::memory_order_relaxed);
                 if (to < current) {
-                    MGLOG_E("MG_Remote watermark: refusing to move %s backwards, %llu -> %llu; a "
+                    MGLOG_F("MGPipe: Fatal{ProtocolCorruption, \"watermark\"} %s moved backwards, "
-                            "waiter that already resumed on the higher value cannot be un-resumed",
+                            "%llu -> %llu. A waiter that already resumed on the higher value cannot "
+                            "be un-resumed, and every later advance of this watermark would be a "
+                            "no-op, so the verb barrier and every reply wait would block for ever",
                             name, static_cast<unsigned long long>(current),
                             static_cast<unsigned long long>(to));
-                    return;
+                    std::abort();
                 }
                 if (to == current) {
                     return;
@@ -404,6 +421,7 @@ namespace MobileGL::MG_Remote::Transport {
         m_stage = nullptr;
         m_peerBell = nullptr;
         m_selfBell = nullptr;
+        m_lastPublishedSeq = 0;
     }
 
     void SessionProducer::PublishAndNotify(std::uint64_t submittedSeq) {
@@ -415,8 +433,20 @@ namespace MobileGL::MG_Remote::Transport {
         if (m_stage != nullptr) {
             m_stage->Publish();
         }
+        // Kept locally as well as on the shared page: the shared watermark is
+        // allowed to lag (Ring.h:72-77), and teardown's drain must not.
+        //
+        // Clamped UP, not passed through. A caller that republishes an older
+        // bound - teardown does exactly that, and so does any batched publisher
+        // that lost track - is publishing LATE, which R-9 permits; it is not the
+        // same thing as a watermark moving backwards, which is Fatal. Doing the
+        // clamp here keeps that distinction at the one boundary where a stale
+        // argument is legitimate.
+        if (submittedSeq > m_lastPublishedSeq) {
+            m_lastPublishedSeq = submittedSeq;
+        }
         // 2. the diagnostic watermark, after the bytes it describes.
-        Watermark::AdvanceSubmitted(*m_control, submittedSeq);
+        Watermark::AdvanceSubmitted(*m_control, m_lastPublishedSeq);
         // 3. and only now the bell. Publish-then-ring, never ring-then-publish.
         if (m_peerBell != nullptr) {
             NotifyIfParked(*m_peerBell, m_control->consumerParked);
@@ -483,6 +513,8 @@ namespace MobileGL::MG_Remote::Transport {
         m_selfBell = selfBell;
         m_spinUs = spinUs;
         m_appliedSeq = control == nullptr ? 0 : control->appliedSeq.load(std::memory_order_acquire);
+        m_retirableCursor = cmd == nullptr ? 0 : cmd->LocalTail();
+        m_borrowHeld = false;
     }
 
     void SessionConsumer::Detach() {
@@ -490,6 +522,8 @@ namespace MobileGL::MG_Remote::Transport {
         m_cmd = nullptr;
         m_peerBell = nullptr;
         m_selfBell = nullptr;
+        m_retirableCursor = 0;
+        m_borrowHeld = false;
     }
 
     SessionWait SessionConsumer::WaitForWork(std::uint32_t timeoutMs) {
@@ -515,7 +549,45 @@ namespace MobileGL::MG_Remote::Transport {
             return;
         }
         Watermark::AdvanceRetired(*m_control, seq);
-        m_cmd->PublishRetired();
+        // PublishRetiredUpTo, NOT PublishRetired: the latter stores m_localTail
+        // into BOTH tails, i.e. it hands back every byte the consumer has popped
+        // whether or not a record among them was borrowed into the GPU timeline.
+        // That would defeat the whole reason the ring carries two tails
+        // (Ring.h:24-27) and would let the producer overwrite a slot the GPU is
+        // still reading. m_retirableCursor stops at the first borrowed record.
+        m_cmd->PublishApplied();
+        m_cmd->PublishRetiredUpTo(m_retirableCursor);
+        NotifyClient();
+    }
+
+    void SessionConsumer::RetireBorrowedUpTo(std::uint64_t cursor) {
+        if (!Valid()) {
+            return;
+        }
+        if (cursor > m_retirableCursor) {
+            m_retirableCursor = cursor;
+            // A release that reaches everything popped so far clears the latch;
+            // anything still unreleased keeps it set, so a second borrow behind
+            // the first is not skipped.
+            m_borrowHeld = cursor < m_cmd->LocalTail();
+        }
+        m_cmd->PublishRetiredUpTo(m_retirableCursor);
+        NotifyClient();
+    }
+
+    void SessionConsumer::CompleteFrame(std::uint64_t serial) {
+        if (!Valid()) {
+            return;
+        }
+        Watermark::AdvanceCompletedFrame(*m_control, serial);
+        NotifyClient();
+    }
+
+    void SessionConsumer::ReturnPresentCredit(std::uint64_t serial) {
+        if (!Valid()) {
+            return;
+        }
+        Watermark::AdvancePresentAck(*m_control, serial);
         NotifyClient();
     }
 

MobileGL/MG_Remote/Transport/RoleMemory.h

@@ -77,8 +77,10 @@ namespace MobileGL::MG_Remote::Transport {
 
     RoleMemorySample SampleRoleMemory(MemoryRole role);
 
-    // Emits one line at ERROR level (the wire layer's only level - WireLog.h) so
+    // Emits one line at INFO level, which is what every P5 lane builds at, so
-    // t1's harness can grep it out of a lane log without a new log sink.
+    // t1's harness can grep it out of a lane log without a new log sink. Not
+    // DEBUG, which the INFO build compiles out; not ERROR, which this is not.
+    // The grep tag is `MG_Remote memory[`.
     // `phase` is a short tag: "handshake", "first-frame", "teardown".
     void LogRoleMemory(const char* phase, const RoleMemorySample& sample);
 

MobileGL/MG_Remote/Transport/SessionRings.h

@@ -28,27 +28,37 @@
 // phase is trying not to repeat.
 //
 // ---------------------------------------------------------------------------
-// THE RING CAPACITY IS HALF THE SEGMENT, AND THAT IS ARITHMETIC, NOT A CHOICE.
+// THE KNOB NAMES THE RING; THE SEGMENT IS THE RING PLUS ONE CONTROL PAGE.
 //
-// Ring.h:11-13 puts RingControl at the HEAD of SEG_CMD, and RingProducer requires
+// Ring.h:11-13 puts RingControl at the HEAD of SEG_CMD and RingProducer requires
-// a POWER-OF-TWO capacity (Ring.cpp:89-103, the mask is the indexing). A segment
+// a POWER-OF-TWO capacity (Ring.cpp:89-103 - the mask IS the indexing). Those two
-// of 8 MiB therefore has 8 MiB - 4096 bytes left for records, and the largest
+// facts together mean a segment and its ring cannot both be 8 MiB, and one of the
-// power of two that fits is 4 MiB. A record may be at most half the ring
+// two numbers has to give.
-// (RingProducer::MaxRecordBytes), so the real cap on one record is 2 MiB.
 //
-// CONTRACT-P5 §5 and Config.h's MOBILEGL_IPC_RING_MB comment both say "8 MiB caps
+// The one that gives is the SEGMENT: SEG_CMD is `MOBILEGL_IPC_RING_MB` MiB PLUS
-// one record at 4 MiB". That arithmetic assumed the whole segment is ring bytes
+// 4096, so the ring inside it is exactly MOBILEGL_IPC_RING_MB MiB and
-// and did not subtract the control page. The number here is HALF of theirs, and
+// RingProducer::MaxRecordBytes() is exactly half of that. CONTRACT-P5 §5 and
-// the deviation is deliberately in the SAFE direction: R-10's obligation is to
+// Config.h's MOBILEGL_IPC_RING_MB comment - "A RECORD MAY BE AT MOST HALF OF
-// PROVE no record ever approaches the cap, and a lower cap makes that proof fire
+// THIS, so 8 MiB caps one record at 4 MiB" - are then TRUE AS WRITTEN, which
-// earlier and louder rather than later and silently. The alternatives were both
+// matters because that sentence is what every other package sizes against.
-// worse - announcing SegmentRef.sizeBytes as 4096 + 8 MiB breaks the four sizes
+//
-// ProtocolSmokeTest.cpp:72 pins, and moving RingControl out of SEG_CMD needs a
+// The first version of this file did the opposite: an 8 MiB segment with a 4 MiB
-// fifth SegmentRef that Welcome does not have.
+// ring and a 2 MiB record cap, on the grounds that ProtocolSmokeTest.cpp:72 pinned
+// the four announced sizes. That was wrong on the facts - that test builds four
+// SegmentRefs from its own literals and round-trips them through the schema; it
+// says nothing about what a session announces, and it never mentions
+// SessionSegments at all. So the alternative was available at no cost, and the
+// version that made two live documents false and left half of SEG_CMD mapped and
+// unreachable was the worse of the two.
+//
+// SegmentRef.sizeBytes therefore announces the MAPPING size (ring + page), which
+// is what a spawn peer must mmap. The four numbers a reader recognises - 8 MiB /
+// 32 MiB / 8 MiB / 256 KiB - are the RING sizes, which is what the knobs name.
 //
 // SEG_STAGE has no control page of its own: RingControl carries TWO cursor
-// triples (Ring.h:101-109) and the stage triple is the second. So SEG_STAGE's
+// triples (Ring.h:101-109) and the stage triple is the second. So SEG_STAGE is
-// capacity is its whole segment, and 32 MiB is already a power of two.
+// exactly its ring, and 32 MiB is already a power of two. SEG_REPLY is not a ring
+// at all.
 // ---------------------------------------------------------------------------
 
 #pragma once
@@ -65,22 +75,29 @@
 
 namespace MobileGL::MG_Remote::Transport {
 
-    // The four sizes are CONTRACT-P5's and are pinned by ProtocolSmokeTest.cpp:72.
+    // RING sizes, not segment sizes - see the header block. The four defaults are
-    // MOBILEGL_IPC_RING_MB / MOBILEGL_IPC_STAGE_MB move the first two.
+    // CONTRACT-P5's; MOBILEGL_IPC_RING_MB / MOBILEGL_IPC_STAGE_MB move the first
+    // two, and ServerSession applies them unless SetSegmentSizes overrode them.
     struct SessionSegmentSizes {
-        std::uint64_t CmdBytes = 8ull * 1024 * 1024;
+        std::uint64_t CmdRingBytes = 8ull * 1024 * 1024;    // + one control page
-        std::uint64_t StageBytes = 32ull * 1024 * 1024;
+        std::uint64_t StageRingBytes = 32ull * 1024 * 1024; // no control page
-        std::uint64_t ReplyBytes = 8ull * 1024 * 1024;
+        std::uint64_t ReplyBytes = 8ull * 1024 * 1024;      // not a ring
-        std::uint64_t EventBytes = 256ull * 1024;
+        std::uint64_t EventRingBytes = 256ull * 1024;       // + one control page
         std::uint32_t ReplySlotCount = kDefaultReplySlotCount;
     };
 
     // Largest power of two <= `bytes`, or 0 when there is none. The ring's
-    // indexing is a mask, so this is what any segment's usable ring area is.
+    // indexing is a mask, so this is what a ring capacity has to be rounded to.
     std::uint64_t LargestPowerOfTwoAtMost(std::uint64_t bytes);
 
-    // Usable ring capacity of a segment that carries a RingControl page at its
+    // How big a segment has to be to hold `ringBytes` of ring behind its control
-    // head. See the header block above for why this is half the segment.
+    // page. `ringBytes` is rounded DOWN to a power of two first, so an operator
+    // who asks for 6 MiB gets a 4 MiB ring in a 4 MiB + 4096 segment rather than
+    // a segment whose tail can never be addressed.
+    std::uint64_t SegmentBytesForRing(std::uint64_t ringBytes);
+
+    // The usable ring inside a segment that carries a RingControl page at its
+    // head. The inverse of SegmentBytesForRing for any size it produced.
     std::uint64_t RingCapacityForSegment(std::uint64_t segmentBytes);
 
     enum class SessionSegmentSlot : std::uint32_t {
@@ -111,9 +128,24 @@ namespace MobileGL::MG_Remote::Transport {
         // and SEG_EVENT's), and books the mapping in `role`'s ledger.
         MobileGLResult Create(const SessionSegmentSizes& sizes, MemoryRole role);
 
-        // The inproc peer's view: the SAME mapping, booked under the OTHER role.
+        // The inproc peer's view of the owner's four segments, booked under the
-        // It does not re-init the control pages - there is one shared page and
+        // OTHER role. It does NOT re-init the control pages - there is one shared
-        // re-initialising it would zero the owner's cursors under it.
+        // page per ring and re-initialising it would zero the owner's cursors out
+        // from under whoever is already using them.
+        //
+        // ON POSIX THIS IS A REAL SECOND MAPPING, NOT AN ALIAS: each descriptor is
+        // dup()ed and adopted through ShmSegment::Adopt + Map, so the peer gets
+        // its own virtual addresses over the same memfd. That is the same reason
+        // inproc uses ShmSegment at all (see the header block): the ATTACH half is
+        // the half P6 replaces with an SCM_RIGHTS Adopt, and aliasing the owner's
+        // ShmSegment objects would leave it first exercised on the day the second
+        // process appears - which is exactly the criticism this file levels at
+        // new[]. It also removes a raw lifetime coupling: an aliased view holds
+        // pointers into the owner's members with no ownership, so the two Closes
+        // have to be ordered by hand.
+        //
+        // Windows has no Adopt (ShmSegment::Adopt is POSIX-only; the section name
+        // travels in SegmentRef instead), so there it still aliases and says so.
         MobileGLResult AttachInProcess(SessionSegments& owner, MemoryRole role);
 
         void Close();
@@ -177,10 +209,19 @@ namespace MobileGL::MG_Remote::Transport {
     // THE ONE RULE THAT MATTERS: a watermark may be published LATE but NEVER
     // EARLY. Late costs a waiter some latency; early makes every waiter a silent
     // use of work that has not happened, and there is no checksum anywhere on
-    // this ring that would catch it. So the advances below REFUSE to move a
+    // this ring that would catch it. The callers are responsible for never
-    // watermark backwards (that is the detectable half) and the callers are
+    // calling an advance before the work is done - that is the half only a
-    // responsible for never calling them before the work is done (that is the
+    // call-site review and R-9's unit cases can enforce.
-    // half only a call-site review and R-9's unit cases can enforce).
+    //
+    // THE HALF THAT IS MECHANICALLY DETECTABLE - a watermark moving BACKWARDS -
+    // IS FATAL, not logged-and-ignored. Logging it and returning was the first
+    // version of this file and it was worse than useless: SessionConsumer keeps
+    // its own counter, so once the shared appliedSeq is behind, every later
+    // advance is a no-op for ever and every WaitForApplied(seq, kWaitForever) -
+    // the verb barrier and every reply wait - blocks permanently. The user sees a
+    // hang and the only evidence is one ERROR line. This is the same class as
+    // RingCursorsValid returning false, and Ring.h:181-185 already calls that "a
+    // Fatal{ProtocolCorruption}, never a retry".
     namespace Watermark {
 
         // Producer, after Publish. Nobody waits on it - it is the answer to "how
@@ -247,6 +288,17 @@ namespace MobileGL::MG_Remote::Transport {
         // call order; this is the one place production code performs it.
         void PublishAndNotify(std::uint64_t submittedSeq);
 
+        // The last seq THIS producer published, kept locally rather than read back
+        // out of RingControl::submittedSeq. Teardown's drain needs it: Ring.h:72-77
+        // explicitly permits submittedSeq to be published LAZILY and Ring.h:243
+        // encourages batching the publish, so the shared watermark may lag the
+        // emitter - and a drain that waits for `appliedSeq >= submittedSeq` would
+        // then under-wait and free an emitter's var-tail while a record still
+        // names it. With the verb barrier armed the two are equal; with
+        // MOBILEGL_IPC_VERB_BARRIER=0, R-1's negative control which the phase has
+        // to run once, they are not.
+        std::uint64_t LastPublishedSeq() const { return m_lastPublishedSeq; }
+
         // The verb barrier's wait, AND the reply's wait: they are the same wait
         // (R-3/R-5), which is why a blocking ReadPixels, MapPersistent's decline
         // and the four Bool acceptances cost ZERO extra round trips.
@@ -277,6 +329,7 @@ namespace MobileGL::MG_Remote::Transport {
         Doorbell* m_peerBell = nullptr;
         Doorbell* m_selfBell = nullptr;
         std::uint32_t m_spinUs = kDefaultSpinUs;
+        std::uint64_t m_lastPublishedSeq = 0;
     };
 
     // -----------------------------------------------------------------------
@@ -322,15 +375,50 @@ namespace MobileGL::MG_Remote::Transport {
             ++m_appliedSeq;
             Watermark::AdvanceApplied(*m_control, m_appliedSeq);
             m_cmd->PublishApplied();
+            // THE BYTE CURSOR RetireThrough MAY RECLAIM TO, which is NOT simply
+            // "everything popped". A record carrying kRecBorrowSlot has been lent
+            // into the GPU timeline and its slot can only be recycled after
+            // completedFrameSerial (Ring.h:24-27), so the reclaimable cursor stops
+            // AT the first borrowed record and does not move again until
+            // RetireBorrowedUpTo releases it. Nothing sets kRecBorrowSlot yet;
+            // this is here so that the day something does, the producer does not
+            // overwrite a slot the GPU is still reading.
+            if ((view.flags & kRecBorrowSlot) == 0 && !m_borrowHeld) {
+                m_retirableCursor = view.cursor + sizeof(RingRecordHeader) + view.payloadSize;
+            } else {
+                m_borrowHeld = true;
+            }
             NotifyClient();
             return true;
         }
 
-        // Records without kRecBorrowSlot retire as soon as they are applied; a
+        // Publish the retire watermark and hand back every byte up to the first
-        // borrowed slot retires on completedFrameSerial, which is why this is a
+        // still-borrowed record.
-        // separate call and not folded into ApplyOne.
+        //
+        // IT IS MANDATORY, NOT OPTIONAL. RingProducer::FreeBytes() reclaims against
+        // retiredTail ONLY (Ring.cpp:110-115) and nothing else in this class
+        // publishes it, so an apply loop that calls ApplyOne and never this wedges
+        // the producer on the first full ring. Call it once per drain batch.
         void RetireThrough(std::uint64_t seq);
 
+        // Release borrowed slots up to `cursor` once completedFrameSerial has
+        // passed them. `cursor` is a RingRecordView::cursor the apply loop kept.
+        // This is the only thing that moves the reclaim point past a borrowed
+        // record - see ApplyOne.
+        void RetireBorrowedUpTo(std::uint64_t cursor);
+
+        // The byte cursor RetireThrough would reclaim to right now. Diagnostic;
+        // a borrow that is never released shows up as this number standing still.
+        std::uint64_t RetirableCursor() const { return m_retirableCursor; }
+
+        // completedFrameSerial and presentAckSerial, advanced AND rung. The free
+        // functions in namespace Watermark advance only: a v1 caller that used one
+        // directly would leave a client parked in WaitForPresentAck(kWaitForever)
+        // with nothing to wake it, because the advance and the doorbell are two
+        // separate stores and only the pair is a wakeup.
+        void CompleteFrame(std::uint64_t serial);
+        void ReturnPresentCredit(std::uint64_t serial);
+
         // Ring the client's bell, but only when it said it is parked: a store to
         // a shared cache line otherwise burns a big core for a whole frame on a
         // phone (Doorbell.h:13-22).
@@ -350,6 +438,8 @@ namespace MobileGL::MG_Remote::Transport {
         Doorbell* m_selfBell = nullptr;
         std::uint32_t m_spinUs = kDefaultSpinUs;
         std::uint64_t m_appliedSeq = 0;
+        std::uint64_t m_retirableCursor = 0;
+        bool m_borrowHeld = false;
     };
 
     // -----------------------------------------------------------------------

MobileGL/MG_Remote/Transport/ShmSegment.cpp

@@ -25,6 +25,10 @@
 #include <cstring>
 #include <utility>
 
+#if !defined(_WIN32)
+#include <unistd.h>
+#endif
+
 namespace MobileGL::MG_Remote::Transport {
 
     ShmSegment::~ShmSegment() { Close(); }
@@ -180,17 +184,37 @@ namespace MobileGL::MG_Remote::Transport {
 
     MobileGLResult SessionSegments::Create(const SessionSegmentSizes& sizes, MemoryRole role) {
         Close();
+        // Set BEFORE the loop, so that Close() on a failure INSIDE it really
+        // closes what has already been created: Close only walks m_owned when
+        // m_owns is true, and setting it afterwards left a failure at segment 3
+        // holding segments 0-2's descriptors and mappings open with Valid()
+        // false, against ShmSegment.h:66's "unmaps and releases the descriptor".
+        m_owns = true;
 
         struct Spec {
             const char* name;
             std::uint64_t bytes;
         };
+        // The sizes are RING sizes; a segment that carries a control page at its
+        // head is that much bigger. SEG_STAGE drives the SECOND cursor triple of
+        // SEG_CMD's page and SEG_REPLY is not a ring at all, so neither of those
+        // two grows.
         const Spec specs[kSlotCount] = {
-            {"mgl-cmd", sizes.CmdBytes},
+            {"mgl-cmd", SegmentBytesForRing(sizes.CmdRingBytes)},
-            {"mgl-stage", sizes.StageBytes},
+            {"mgl-stage", LargestPowerOfTwoAtMost(sizes.StageRingBytes)},
             {"mgl-reply", sizes.ReplyBytes},
-            {"mgl-event", sizes.EventBytes},
+            {"mgl-event", SegmentBytesForRing(sizes.EventRingBytes)},
         };
+        for (std::size_t index = 0; index < kSlotCount; ++index) {
+            if (specs[index].bytes == 0) {
+                MGLOG_E("MG_Remote session: segment %s was asked for a ring size that cannot be "
+                        "made into one (a ring is a power of two between %llu and %llu bytes)",
+                        specs[index].name, static_cast<unsigned long long>(kMinRingCapacity),
+                        static_cast<unsigned long long>(kMaxRingCapacity));
+                Close();
+                return MOBILEGL_ERR_INVALID_ARGUMENT;
+            }
+        }
 
         for (std::size_t index = 0; index < kSlotCount; ++index) {
             const MobileGLResult created =
@@ -215,7 +239,6 @@ namespace MobileGL::MG_Remote::Transport {
             }
         }
 
-        m_owns = true;
         m_replySlotCount = sizes.ReplySlotCount;
         DeriveViews();
         if (!m_valid) {
@@ -240,10 +263,51 @@ namespace MobileGL::MG_Remote::Transport {
         if (!owner.Valid()) {
             return MOBILEGL_ERR_NOT_INITIALIZED;
         }
+#if !defined(_WIN32)
+        // A REAL SECOND MAPPING, not an alias. dup + Adopt + Map is byte for byte
+        // the call sequence P6's SCM_RIGHTS client runs, so mapping, the fstat
+        // size check inside Adopt (ShmSegmentPosix.cpp:119-141), alignment and the
+        // peer's own lifetime are all exercised now rather than on the day the
+        // second process appears. Aliasing the owner's ShmSegment objects would
+        // leave the attach half untested for exactly the reason this file refuses
+        // to allocate the rings with new[].
+        m_owns = true;
+        for (std::size_t index = 0; index < kSlotCount; ++index) {
+            const ShmSegment* theirs = owner.m_segments[index];
+            const int duplicate = theirs == nullptr ? -1 : ::dup(theirs->Fd());
+            if (duplicate < 0) {
+                MGLOG_E("MG_Remote session: could not dup the owner's descriptor for segment %zu",
+                        index);
+                Close();
+                return MOBILEGL_ERR_INVALID_ARGUMENT;
+            }
+            // Adopt takes ownership of `duplicate` on success only.
+            const MobileGLResult adopted =
+                ShmSegment::Adopt(duplicate, theirs->Size(), m_owned[index]);
+            if (adopted != MOBILEGL_OK) {
+                ::close(duplicate);
+                Close();
+                return adopted;
+            }
+            // Read/write: under inproc the client writes SEG_CMD and SEG_STAGE and
+            // reads SEG_REPLY and SEG_EVENT, and one ShmSegment maps the whole
+            // thing one way. The per-segment read-only peer view is P6's, where
+            // the roles are separable.
+            const MobileGLResult mapped = m_owned[index].Map(false);
+            if (mapped != MOBILEGL_OK) {
+                Close();
+                return mapped;
+            }
+        }
+#else
+        // Windows has no Adopt (ShmSegment::Adopt is POSIX-only; a Windows peer
+        // resolves the section by the name carried in SegmentRef). Alias, and say
+        // so: this arm does not exercise the attach path P6 replaces.
         for (std::size_t index = 0; index < kSlotCount; ++index) {
             m_segments[index] = owner.m_segments[index];
         }
         m_owns = false;
+#endif
         m_replySlotCount = owner.m_replySlotCount;
         DeriveViews();
         if (!m_valid) {