[Fix] (Remote): order buffer writebacks by producing seq so a stale capture writeback cannot retire a newer GPU-write mark

MobileGL/MG_Impl/Pipe/ResourceTracker.h

@@ -579,7 +579,11 @@ namespace MobileGL::MG_Pipe {
     //                                transport this is rule B in the reverse direction and is
     //                                Fatal{ProtocolCorruption, "OnBufferWriteback.Seg"};
     //   anything else             -> the same Fatal.
-    inline void MGPipeClientOnBufferWriteback(MGPipeHandle res, Uint64 offset, MGPBlobRef bytes) {
+    //
+    // `producedAtSeq` is the event's ProducedAtSeq (the record the server read the bytes back
+    // at); 0 for the monolith callback, whose answer is synchronous with its request.
+    inline void MGPipeClientOnBufferWritebackAt(MGPipeHandle res, Uint64 offset, MGPBlobRef bytes,
+                                                Uint64 producedAtSeq) {
         auto* buffer = MGPipeResourceTrackerInstance().Resolve(res);
         if (buffer == nullptr) {
             MGLOG_E_ONCE("MGPipe: OnBufferWriteback for a handle {%u,%u} that resolves to no buffer",
@@ -641,7 +645,12 @@ namespace MobileGL::MG_Pipe {
         }
 #endif
         buffer->WritebackFromBackend(DataPtr{bytePtr, static_cast<SizeT>(bytes.Size)},
-                                     static_cast<SizeT>(offset));
+                                     static_cast<SizeT>(offset), producedAtSeq);
+    }
+
+    // The monolith callback's shape (MGPipeCallbacks::OnBufferWriteback).
+    inline void MGPipeClientOnBufferWriteback(MGPipeHandle res, Uint64 offset, MGPBlobRef bytes) {
+        MGPipeClientOnBufferWritebackAt(res, offset, bytes, 0);
     }
 
     // A draw or dispatch wrote these ranges. ARCHITECTURE.md 7.1 calls this a NARROWING
@@ -670,7 +679,7 @@ namespace MobileGL::MG_Pipe {
                          res.Gen);
             return;
         }
-        buffer->MarkGpuWritten();
+        buffer->MarkGpuWrittenInChannelOrder();
     }
 
     // MONOLITH ONLY (CONTRACT-P5C §4.1): with an active transport the three reverse entries

MobileGL/MG_Remote/Client/ClientSession.cpp

@@ -508,9 +508,9 @@ namespace MobileGL::MG_Remote::Client {
                     blob.Seg = Wire::kSegEvent;
                     blob.Offset = events.OffsetInSegment(bytes);
                     blob.Size = head->Size;
-                    MG_Pipe::MGPipeClientOnBufferWriteback(
+                    MG_Pipe::MGPipeClientOnBufferWritebackAt(
                         MG_Pipe::MGPipeHandle{head->Resource.Slot, head->Resource.Gen},
-                        head->Offset, blob);
+                        head->Offset, blob, head->ProducedAtSeq);
                     ++delivered;
                     break;
                 }

MobileGL/MG_Remote/Client/GpuWritePending.cpp

@@ -31,7 +31,7 @@ namespace MobileGL::MG_Remote::Client {
         ClientSession* session = ClientSession::Active();
         if (session == nullptr) return 0;
         // A quarter of the ring, not the MaxRecordBytes half: the record carries its own
-        // header and the 24-byte EventBufferWritebackHead beside the payload, and the ring
+        // header and the 32-byte EventBufferWritebackHead beside the payload, and the ring
         // may still hold a few small events (gpu-written, gl-error) posted earlier in the
         // same verb's apply. Each slice round-trips with its own barrier + drain, so the
         // ring never holds more than one slice's bytes.
@@ -72,6 +72,11 @@ namespace MobileGL::MG_Remote::Client {
         return MGPipeStageChunkBytesFor(session->StageCapacityBytes());
     }
 
+    Uint64 GpuWriteOrderSeq() {
+        ClientSession* session = ClientSession::Active();
+        return session == nullptr ? 0 : session->LastPublishedSeq();
+    }
+
     void AwaitBufferWriteback(BufferObject& buffer) {
         // THE WAIT IS THE BARRIER'S WAIT (R-3). The reply-slot id IS the record's seq, so
         // "appliedSeq reached my readback" and "my answer is back" are one condition, and

MobileGL/MG_Remote/Client/GpuWritePending.h

@@ -33,6 +33,12 @@ namespace MobileGL::MG_Remote::Client {
     // caller writes, because the loop would be a hang in exactly that configuration.
     void AwaitBufferWriteback(MG_State::GLState::BufferObject& buffer);
 
+    // The order a GPU-write mark is stamped with: the last command seq this session
+    // published, so the work the mark stands for is in a LATER record and only a writeback
+    // the server produced after that record can retire it (BufferObject::WritebackFromBackend).
+    // 0 with no session, where every writeback is synchronous and needs no ordering.
+    Uint64 GpuWriteOrderSeq();
+
 
     // The slice a whole-buffer readback is cut into so one writeback event always fits
     // SEG_EVENT. The writeback's bytes travel INLINE in the event record (P5c ev, CONTRACT-P5C

MobileGL/MG_Remote/Server/ServerSession.cpp

@@ -457,6 +457,7 @@ namespace MobileGL::MG_Remote::Server {
             head.Resource = Transport::EventHandle{res.Slot, res.Gen};
             head.Offset = offset;
             head.Size = bytes.Size;
+            head.ProducedAtSeq = session.Consumer().ProducingSeq();
             std::memcpy(slot, &head, sizeof(head));
             if (bytes.Size != 0) {
                 std::memcpy(static_cast<Uint8*>(slot) + sizeof(head),

MobileGL/MG_Remote/Transport/EventRing.h

@@ -73,12 +73,20 @@ namespace MobileGL::MG_Remote::Transport {
     // makes "the destination is the client's shadow" (contract table 1 row 22)
     // reachable without a second segment. `Size` is therefore the record's own
     // tail length and is cross-checked against it.
+    //
+    // `ProducedAtSeq` is the SEG_CMD seq of the record whose apply posted the
+    // writeback (the record count the server had applied, when it was posted
+    // between records). Writebacks a client did not ask for - a transform
+    // feedback capture closing - drain whenever the client next waits, which
+    // can be after it has marked the same buffer written by a later record; the
+    // seq is what lets it tell an old writeback from the answer to its own.
     struct EventBufferWritebackHead {
         EventHandle Resource;
-        std::uint64_t Offset; // destination offset inside the resource
-        std::uint64_t Size;   // inline byte count that follows
+        std::uint64_t Offset;        // destination offset inside the resource
+        std::uint64_t Size;          // inline byte count that follows
+        std::uint64_t ProducedAtSeq; // SEG_CMD seq the bytes are current as of
     };
-    static_assert(sizeof(EventBufferWritebackHead) == 24, "wire shape");
+    static_assert(sizeof(EventBufferWritebackHead) == 32, "wire shape");
 
     // OnGpuWritten(res, rangeCount, ranges). EventRange[RangeCount] follows.
     struct EventGpuWrittenHead {

MobileGL/MG_Remote/Transport/SessionRings.h

@@ -466,10 +466,20 @@ namespace MobileGL::MG_Remote::Transport {
             // A terminal rejection after Pop must not acknowledge an unapplied record.
             // A bool callback returning false ends this consumer's session; it cannot retry
             // the popped record. Existing void callbacks retain their original semantics.
-            if constexpr (std::is_same_v<std::invoke_result_t<Apply, const RingRecordView&>, bool>) {
-                if (!apply(view)) return false;
-            } else {
-                apply(view);
+            // ProducingSeq() names THIS record while its apply runs, so a reverse
+            // event the apply posts carries the seq of the record that produced it.
+            struct ApplyingScope {
+                bool& flag;
+                explicit ApplyingScope(bool& f) : flag(f) { flag = true; }
+                ~ApplyingScope() { flag = false; }
+            };
+            {
+                const ApplyingScope applying(m_applying);
+                if constexpr (std::is_same_v<std::invoke_result_t<Apply, const RingRecordView&>, bool>) {
+                    if (!apply(view)) return false;
+                } else {
+                    apply(view);
+                }
             }
             ++m_appliedSeq;
             Watermark::AdvanceApplied(*m_control, m_appliedSeq);
@@ -535,6 +545,11 @@ namespace MobileGL::MG_Remote::Transport {
         void NotifyClient();
 
         std::uint64_t AppliedSeq() const { return m_appliedSeq; }
+        // The seq of the record whose apply is running, or - between records -
+        // the last one applied. Every byte the server reads back at this point is
+        // current as of exactly this record, which is what a writeback event
+        // stamps so the client can order it against its own later records.
+        std::uint64_t ProducingSeq() const { return m_applying ? m_appliedSeq + 1 : m_appliedSeq; }
         // How many kRecBorrowSlot records this consumer has seen. Non-zero in P5
         // is a finding, not a statistic.
         std::uint64_t BorrowedRecordsSeen() const { return m_borrowedSeen; }
@@ -556,6 +571,7 @@ namespace MobileGL::MG_Remote::Transport {
         std::uint64_t m_retirableCursor = 0;
         std::uint64_t m_borrowedSeen = 0;
         bool m_borrowHeld = false;
+        bool m_applying = false;
 
         // Out of line so ApplyOne, which is a template in a header this layer
         // keeps free of MobileGL/Includes.h, can still log.

MobileGL/MG_State/GLState/BufferState/BufferObject.cpp

@@ -480,7 +480,7 @@ namespace MobileGL::MG_State::GLState {
         return m_publishedLiveHostWrites;
     }
 
-    void BufferObject::WritebackFromBackend(DataPtr data, SizeT atOffset) {
+    void BufferObject::WritebackFromBackend(DataPtr data, SizeT atOffset, Uint64 producedAtSeq) {
         MOBILEGL_ASSERT(atOffset + data.size <= m_size,
                         "WritebackFromBackend out of bounds: atOffset (%zu) + data.size (%zu) > m_size (%zu)", atOffset,
                         data.size, m_size);
@@ -498,16 +498,42 @@ namespace MobileGL::MG_State::GLState {
         // P13 W4a: monolith's record arm refreshes an aliased store through this function too
         // (Managers.cpp RefreshAliasedStoreFromGpu), and a whole-store landing there is exactly as
         // current as one under split.
-        if (atOffset == 0 && data.size >= m_size) {
+        //
+        // ORDERED, NOT MERELY WHOLE. A writeback the client never asked for - the end of a
+        // transform feedback capture - is drained whenever the client next waits, which under
+        // run-ahead can be after a later dispatch has marked this same buffer written again.
+        // Those bytes are the capture's, not the dispatch's, so they may not retire the newer
+        // mark: only a writeback produced by a record after the one the mark was taken for can
+        // (the readback SyncGpuWrites emits always is). 0 is the synchronous answer - monolith
+        // and every no-session case - which has nothing to be ordered against.
+        if (atOffset == 0 && data.size >= m_size &&
+            (producedAtSeq == 0 || producedAtSeq > m_gpuWriteMarkSeq)) {
             m_gpuWritePending = false;
         }
     }
 
     void BufferObject::MarkGpuWritten() {
+#if MOBILEGL_BUILD_DISAGGREGATED
+        MarkGpuWrittenAfter(MG_Remote::Client::GpuWriteOrderSeq());
+#else
+        MarkGpuWrittenAfter(0);
+#endif
+    }
+
+    void BufferObject::MarkGpuWrittenInChannelOrder() {
         m_hasDefinedContent = true;
         m_gpuWritePending = true;
     }
 
+    void BufferObject::MarkGpuWrittenAfter(Uint64 orderSeq) {
+        m_hasDefinedContent = true;
+        m_gpuWritePending = true;
+        // Assigned, not maxed: within a session the seq only grows, and a new session (or
+        // none, 0) restarts the numbering - a stamp kept from the old one would outrank every
+        // answer the new one can give.
+        m_gpuWriteMarkSeq = orderSeq;
+    }
+
     void BufferObject::SyncGpuWrites() {
         if (!m_gpuWritePending) return;
 #if MOBILEGL_BUILD_DISAGGREGATED
@@ -642,10 +668,9 @@ namespace MobileGL::MG_State::GLState {
         if (bytes.size > 0 && MG_Pipe::MGPipeResourceSubsystemEnabled() &&
             MG_Pipe::MGPipeResourceOpsHaveSubDataResident()) {
             MG_Pipe::MGPipeEmitBufferSubDataResident(*this, offset, bytes.data, bytes.size);
-            m_hasDefinedContent = true;
             ++m_changeSerial;
             MGP_NOTE_AGGREGATE(BufferChange);
-            m_gpuWritePending = true;
+            MarkGpuWritten();
             return;
         }
         // No resident op: the write lands in place below, after retiring the GPU writes this

MobileGL/MG_State/GLState/BufferState/BufferObject.h

@@ -113,7 +113,14 @@ namespace MobileGL {
             // Shadow-only write used when the backend copies GPU results (e.g. ReadPixels
             // into a pixel-pack buffer) back into the frontend mirror. Does not issue a
             // backend op: the backend storage already holds these bytes.
-            void WritebackFromBackend(DataPtr data, SizeT atOffset);
+            //
+            // `producedAtSeq` is the command seq the bytes are current as of (the event's
+            // ProducedAtSeq); 0 means the writeback is synchronous with its request and needs
+            // no ordering. A whole-buffer writeback clears the outstanding GPU write only when
+            // it is newer than the record the latest mark was taken for - an older one (a
+            // capture's writeback drained after a later dispatch re-marked the buffer) still
+            // lands its bytes but cannot vouch for the write it predates.
+            void WritebackFromBackend(DataPtr data, SizeT atOffset, Uint64 producedAtSeq = 0);
 
             // A draw or dispatch just ran with this buffer bound where a shader can write
             // it (shader storage / atomic counter). The next read has to reconcile with
@@ -121,6 +128,16 @@ namespace MobileGL {
             // memory - wait for the work that wrote them to retire. Which of the two is
             // the backend's business; the flag only says a GPU write is outstanding.
             void MarkGpuWritten();
+            // The same mark, stamped with an explicit command seq: the mark stands for GPU
+            // work in records AFTER `orderSeq`. MarkGpuWritten stamps the session's last
+            // published seq (0 without a session).
+            void MarkGpuWrittenAfter(Uint64 orderSeq);
+            // The mark a server announces on the reverse channel (OnGpuWritten). That channel
+            // is in order with the writebacks, so every writeback drained after this mark was
+            // produced after the write it announces: the standing stamp is kept, not raised to
+            // the drain point - raising it would outrank the very readback the next read is
+            // already waiting on.
+            void MarkGpuWrittenInChannelOrder();
             // Refreshes the shadow from the backend when a GPU write is outstanding. Called
             // from every path that reads the shadow on the app's behalf.
             void SyncGpuWrites();
@@ -258,6 +275,10 @@ namespace MobileGL {
             // and, in a split build, cleared by WritebackFromBackend instead, because there
             // the answer arrives later than the request. See SyncGpuWrites' definition.
             Bool m_gpuWritePending = false;
+            // The command seq the latest GPU-write mark was taken after. A monotonic record
+            // seq, never a count of writebacks: only a writeback produced by a LATER record
+            // can retire the mark (WritebackFromBackend).
+            Uint64 m_gpuWriteMarkSeq = 0;
             // The last value of MGPResourceDesc::HasLiveHostWrites this object published.
             // Behind the option so the pull build's layout - and therefore every inlined
             // constructor and accessor in it - does not move (G1).

MobileGL/MG_Test/Buffer/SplitBufferTest.cpp

@@ -540,6 +540,39 @@ TEST_F(SplitBufferSet, UnderSplitTheWritebackClearsThePendingFlagAndNotTheReques
     EXPECT_FALSE(buffer->HasOutstandingGpuWrite());
 }
 
+// A writeback the client never asked for (a capture's end) drains whenever the client next
+// waits, which can be after a later record marked the same buffer written again. Only a
+// writeback produced by a record AFTER the mark's may retire it; an older one lands its bytes
+// and leaves the mark standing, so the next read still reconciles with the newer write.
+TEST_F(SplitBufferSet, AnOlderWritebackCannotRetireANewerGpuWriteMark) {
+    auto buffer = MakeBuffer(27u, 96);
+    Vector<Uint8> capture(96, static_cast<Uint8>(0x11));
+
+    // The capture closed at record 4; a dispatch emitted after record 6 re-marked the buffer.
+    buffer->MarkGpuWrittenAfter(6);
+    buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0, /*producedAtSeq=*/4);
+    EXPECT_TRUE(buffer->HasOutstandingGpuWrite())
+        << "the capture's writeback predates the dispatch and cannot vouch for its write";
+    buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0, /*producedAtSeq=*/6);
+    EXPECT_TRUE(buffer->HasOutstandingGpuWrite()) << "record 6 is the mark's own seq, not after it";
+
+    // The readback the next read emits is a later record, and it does retire the mark.
+    buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0, /*producedAtSeq=*/7);
+    EXPECT_FALSE(buffer->HasOutstandingGpuWrite());
+
+    // An unordered (synchronous) writeback keeps the old rule.
+    buffer->MarkGpuWrittenAfter(9);
+    buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0);
+    EXPECT_FALSE(buffer->HasOutstandingGpuWrite());
+
+    // A server-announced mark is drained in channel order, so it keeps the standing stamp: the
+    // readback the client emitted after its own mark at 10 still answers it.
+    buffer->MarkGpuWrittenAfter(10);
+    buffer->MarkGpuWrittenInChannelOrder();
+    buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0, /*producedAtSeq=*/11);
+    EXPECT_FALSE(buffer->HasOutstandingGpuWrite());
+}
+
 // The tier gate in P11 B2's shape (CONTRACT-P11 §1 and B2). 0 / 1 are SETTLED AT THE HANDSHAKE by
 // the data plane rather than discovered at the first map_persistent: a stream refuses them by name
 // and runs T2 on either side; over shared segments a client with 0 ASKS T0 (0), with 1 is refused
@@ -826,6 +859,7 @@ TEST(SplitBufferSet, BothEdgesOfAWriteMapPublishOneStateRecord) { MGL_SPLIT_ONLY
 TEST(SplitBufferSet, AZeroBlockSizeTurnsThePushOffRatherThanMakingItUnlimited) { MGL_SPLIT_ONLY_OR_SKIP(); }
 TEST(SplitBufferSet, ADestroyedBufferLeavesTheSet) { MGL_SPLIT_ONLY_OR_SKIP(); }
 TEST(SplitBufferSet, UnderSplitTheWritebackClearsThePendingFlagAndNotTheRequest) { MGL_SPLIT_ONLY_OR_SKIP(); }
+TEST(SplitBufferSet, AnOlderWritebackCannotRetireANewerGpuWriteMark) { MGL_SPLIT_ONLY_OR_SKIP(); }
 TEST(SplitBufferSet, TheAdoptTierIsSettledAtTheHandshakeAndNeverFatal) { MGL_SPLIT_ONLY_OR_SKIP(); }
 TEST(SplitBufferSet, ASubPageMapIsOneProtectedPageAndAWriteThroughItFaults) { MGL_SPLIT_ONLY_OR_SKIP(); }
 TEST(SplitBufferSet, AShadowThatFailsThePageGranularTestKeepsTheInwardAlignmentAndItsEdges) {

MobileGL/MG_Test/Wire/SessionTest.cpp

@@ -461,11 +461,17 @@ TEST(SessionTest, AppliedSeqAdvancesExactlyOncePerRecordAndIsNeverBatched) {
     session.producer.PublishAndNotify(kRecords);
 
     std::uint64_t applied = 0;
-    while (session.consumer.ApplyOne([&](const RingRecordView&) { ++applied; })) {
+    EXPECT_EQ(session.consumer.ProducingSeq(), 0u);
+    while (session.consumer.ApplyOne([&](const RingRecordView&) {
+        // A reverse event posted by this apply is stamped with THIS record's seq.
+        EXPECT_EQ(session.consumer.ProducingSeq(), applied + 1);
+        ++applied;
+    })) {
         EXPECT_EQ(session.Control().Progress.appliedSeq.load(), applied)
             << "appliedSeq did not move with the record; a barrier waiter would be blocked on "
                "work that already ran";
         EXPECT_EQ(session.consumer.AppliedSeq(), applied);
+        EXPECT_EQ(session.consumer.ProducingSeq(), applied) << "between records: the last one applied";
     }
     EXPECT_EQ(applied, kRecords);
 }