diff --git a/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp b/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp index 0b13148c..00b7264f 100644 --- a/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp +++ b/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp @@ -717,6 +717,12 @@ namespace MobileGL::MG_Remote::Wire { } for (int attempt = 0; attempt < 2; ++attempt) { + // THE WRAP SKIP MAY ONLY BE CHARGED AGAINST BYTES THAT ARE STILL IN FLIGHT. When + // there are none the allocator starts over at offset zero, so a blob the segment + // can hold whole is never refused (see RebaseEmptyStage). On attempt 1 this runs + // AFTER ReclaimStagedBytes, which is the case the finding describes: 8 MiB + // allocated, then retired, then a 28 MiB request that used to abort. + RebaseEmptyStage(); const Uint64 offset = m_stageHead % m_stageCapacity; // A run is always contiguous: one that would straddle the end skips the remainder, // exactly as the ring's wrap pad does, and the skipped bytes are reclaimed with @@ -936,6 +942,32 @@ namespace MobileGL::MG_Remote::Wire { return m_emitSeq; } + // THE EMPTY-STAGE REBASE. Head and tail are monotonic byte counts, so "every staged byte + // has retired" reads head == tail, NOT head == tail == 0, and `head % capacity` is left + // wherever the last run ended. Charging a wrap skip against that offset then costs the + // unused suffix a second time: with head == tail == 64 in a 256 KiB stage, the allocator's + // test became `2*capacity - 64 <= capacity`, which is false at EVERY occupancy, so a blob + // that fits the segment whole was refused with `Fatal{RingOverrun, "SEG_STAGE"}` - whose + // own message then reported `0 bytes still in flight`. ReclaimStagedBytes cannot help, + // because an already-empty tail has nothing left to move. + // + // THE MARK QUEUE COMES WITH IT. A mark holds the ABSOLUTE head cursor it was pushed at and + // ReclaimStagedBytes assigns that value straight to m_stageTail. Every mark not yet + // consumed has StageCursor <= head == tail - the head is monotonic and marks are pushed in + // order - so each of them names a region that is already reclaimed and zero is the + // truthful rebasing of it. Without that, one reclaim after a rebase would put the tail + // AHEAD of the head and StagedBytesInFlight() would underflow to about 2^64. + void PipeWireEncoder::RebaseEmptyStage() { + if (m_stageHead != m_stageTail || m_stageHead == 0) { + return; + } + m_stageHead = 0; + m_stageTail = 0; + for (SizeT i = 0; i < m_stageMarks.size(); ++i) { + m_stageMarks[i].StageCursor = 0; + } + } + void PipeWireEncoder::ReclaimStagedBytes() { if (m_control == nullptr) { return; diff --git a/MobileGL/MG_Remote/Wire/PipeWireCodec.h b/MobileGL/MG_Remote/Wire/PipeWireCodec.h index 1f87f2df..ea2360bb 100644 --- a/MobileGL/MG_Remote/Wire/PipeWireCodec.h +++ b/MobileGL/MG_Remote/Wire/PipeWireCodec.h @@ -290,6 +290,17 @@ namespace MobileGL::MG_Remote::Wire { // RingControl - see ReclaimStagedBytes above. Uint8* StageAllocate(Uint64 size); + // AN EMPTY STAGE STARTS OVER AT ZERO, so that the wrap skip is only ever charged + // against bytes that are really still in flight. Head and tail are monotonic, so once + // everything has retired they are EQUAL BUT NOT ZERO, and `head % capacity` is + // wherever the last run happened to end - a wrap skip charged against that offset + // costs the suffix a second time and refused a blob the whole segment could hold, with + // a message that reported zero bytes in flight while it did so. Rebasing also rewrites + // the marks still held: a mark stores an ABSOLUTE head cursor and a later reclaim + // assigns it to m_stageTail, so leaving a stale one behind would drive the tail past + // the head and underflow StagedBytesInFlight(). + void RebaseEmptyStage(); + Transport::RingControl* m_control = nullptr; Transport::RingProducer* m_cmd = nullptr; Transport::RingProducer* m_stage = nullptr; diff --git a/MobileGL/MG_Test/Wire/PipeWireCodecTest.cpp b/MobileGL/MG_Test/Wire/PipeWireCodecTest.cpp index e29613c1..200bfdbd 100644 --- a/MobileGL/MG_Test/Wire/PipeWireCodecTest.cpp +++ b/MobileGL/MG_Test/Wire/PipeWireCodecTest.cpp @@ -1135,6 +1135,43 @@ TEST_F(PipeWireCodecTest, StagedBytesAreReclaimedOnlyBehindRetiredSeq) { ASSERT_TRUE(wire.PumpOne(&applied)); wire.Encoder().ReclaimStagedBytes(); EXPECT_EQ(wire.Encoder().StagedBytesInFlight(), 0u); + + // ---- AND AN EMPTY STAGE TAKES THE WHOLE SEGMENT --------------------------------- + // The verifier's extension of this case, kept (wave1-codex-verify.md ยง1). The 64-byte + // run has retired and in-flight bytes are ZERO, so every byte of SEG_STAGE is free - + // but head and tail are monotonic and both sit at 64, so `head % capacity` is 64 and the + // allocator used to charge a `capacity - 64` wrap skip against a capacity that had + // nothing in it. The test then read `2*capacity - 64 <= capacity`, false at every + // occupancy, and a blob the segment holds WHOLE aborted with + // `Fatal{RingOverrun, "SEG_STAGE"} ... with 0 bytes still in flight`. + // + // I made it red once, by doing X: X = deleting the `RebaseEmptyStage()` call at the top + // of StageAllocate's attempt loop (PipeWireCodec.cpp). The case then dies with SIGABRT + // inside PipeWireEncoder::StageAllocate on that message, exactly as the verifier + // recorded it. + // + // THE EXACT MAXIMUM. `need = Align8(size)` and the first bound is `need > capacity`, so + // an empty stage takes a blob of exactly the capacity the encoder adopted - here + // Wire2::kStageBytes, and in a real session the whole SEG_STAGE view, i.e. + // MOBILEGL_IPC_STAGE_MB (32 MiB by default; SessionRings.h keeps SEG_STAGE un-ringed and + // un-rounded, so there is no control page to subtract). + const Uint64 maxRecordBefore = wire.Encoder().MaxRecordBytesSeen(); + std::vector whole(Wire2::kStageBytes, 0x5A); + const MGPBlobRef full = wire.Encoder().StageBytes(whole.data(), whole.size()); + EXPECT_EQ(full.Offset, 0u) << "an empty stage must hand a whole-capacity blob offset zero"; + EXPECT_EQ(full.Size, Wire2::kStageBytes); + EXPECT_EQ(full.Seg, static_cast(kSegStage)); + EXPECT_EQ(wire.Encoder().StagedBytesInFlight(), Wire2::kStageBytes); + const void* back = wire.Segments().Resolve(full.Seg, full.Offset, full.Size); + ASSERT_NE(back, nullptr); + EXPECT_EQ(back, wire.StageBase()); + + // R-10's max-record counter DOES NOT SEE IT, and that is the point of R-10's carrier + // rule: EncodeRecord feeds m_maxRecordBytes from `layout.TotalBytes` - header + payload + + // tails, all of it SEG_CMD - while the blob leaves only {Seg, Offset, Size} in the + // record. A quarter-megabyte of staging moved the counter by zero bytes. SEG_STAGE has + // its own bound and its own named Fatal, and MaxRecordBytesSeen() is not it. + EXPECT_EQ(wire.Encoder().MaxRecordBytesSeen(), maxRecordBefore); } // ---- M2 / M3: SEG_STAGE's cursors and the mark queue --------------------------------------