diff --git a/MobileGL/MG_Remote/FatalFunnel.h b/MobileGL/MG_Remote/FatalFunnel.h index cda1b541..41b709a9 100644 --- a/MobileGL/MG_Remote/FatalFunnel.h +++ b/MobileGL/MG_Remote/FatalFunnel.h @@ -77,7 +77,7 @@ namespace MobileGL::MG_Remote { // SessionFail calls: the string must carry its `Fatal{Word` and the word must have a .def row. void ArmSessionLatch(); bool SessionLatchArmed(); - // True once the first fault has latched. One acquire load: DrainRing asks it per record. + // True once the first fault has latched. One acquire load: DrainRing asks it before every pop. bool SessionLatched(); // The first latched fault - its family and its line, verbatim. Meaningful only once // SessionLatched() is true; the line's storage lives for the process. diff --git a/MobileGL/MG_Remote/Server/ServerLoop.cpp b/MobileGL/MG_Remote/Server/ServerLoop.cpp index 2302d09e..10a9128a 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.cpp +++ b/MobileGL/MG_Remote/Server/ServerLoop.cpp @@ -657,25 +657,37 @@ namespace MobileGL::MG_Remote::Server { // allocation; `applied == 0` then skips the retire block below entirely. Keep it that // way: anything added here runs ~900 times a frame doing nothing. // - // PH-1 (3), ID-P7-1: THE LATCH CHECK, TWICE, AND EACH ONE HAS A JOB THE OTHER CANNOT DO. - // Once a named fault has latched (an armed spawn / TCP session child only - unarmed this is - // one acquire load of a flag nobody sets), no further record is applied: the rest of the - // ring is declined unread, the apply thread leaves its loop, and RunSession closes the - // session by name. - // * HERE, at the top: a drain ENTERED latched pops nothing. That is the apply thread's - // exit-path drain after it left on the latch, and any drain after RunSession's thread - // latched a control op. ServerLoopTest's latched-batch case goes red without it (the - // exit-path drain applies the record behind the latched one). - // * AFTER EACH RECORD, at the bottom of the loop: a record that latched is the last one - // this drain pops, even when the peer published more behind it in the same batch - // (PeerLatchTest's two-record case and ServerLoopTest's latched-batch case). - // A check at the loop's head instead would only repeat this one on the first iteration. - if (SessionLatched()) return 0; + // PH-1 (3), ID-P7-1: THE LATCH CHECK, ONCE, IMMEDIATELY BEFORE EVERY POP. Once a named + // fault has latched (an armed spawn / TCP session child only - unarmed this is one acquire + // load of a flag nobody sets), no further record is applied: the rest of the ring is + // declined unread, the apply thread leaves its loop, and RunSession closes the session by + // name. The one check does three jobs, and each has a case that goes red without it: + // * a drain ENTERED latched pops nothing - the apply thread's exit-path drain after it + // left on the latch, and any drain after RunSession's thread latched a control op + // (ServerLoopTest's latched-batch case: the exit-path drain applies the record behind + // the latched one); + // * a record that latched is the last one this drain pops, even when the peer published + // more behind it in the same batch (PeerLatchTest's two-record case, ServerLoopTest's + // latched-batch case); + // * a latch ANOTHER thread stores between two records - RunSession's control thread + // latching a malformed SurfaceOp while this drain is mid-batch - stops the next pop + // (codex closeout finding 6; ServerLoopLatchTest's between-records case). The first + // version checked at the top of the function and after each record instead, and a + // latch that landed after the per-record check was still followed by one more pop and + // apply before anything looked again. + // What remains is the window between this load and the pop itself, and it is not a gap + // in the guarantee but its linearization point: a latch stored after this load is ordered + // after the record the load admitted, exactly as a latch stored while that record runs is. + // Closing it would take a lock the control thread shares with every pop. + // + // The idle poll's cost is unchanged: this load replaces the function-top check the first + // version made, and the empty-ring answer is still it plus Pop's cmdHead load. ServerSession& session = *m_session; Transport::SessionConsumer& consumer = session.Consumer(); PipeApplier& applier = session.Applier(); Uint64 applied = 0; for (;;) { + if (SessionLatched()) break; bool corrupt = false; const bool popped = consumer.ApplyOne( [this, &applier](const Transport::RingRecordView& record) { @@ -708,12 +720,17 @@ namespace MobileGL::MG_Remote::Server { } if (!popped) break; ++applied; - if (SessionLatched()) break; // PH-1 (3): the per-record check the top of this function describes // PH-6: the record whose event forfeited the reverse channel is the last one this // session applies. Checked only AFTER a popped record, so the empty-ring answer above - // still costs its two loads and nothing else. (The latch check sits directly under - // `++applied` because ph_latch_sites.py's MECHANICS row pins exactly that shape.) + // still costs its two loads and nothing else. (A record that LATCHED is stopped by the + // loop-head check on the next iteration - ph_latch_sites.py's MECHANICS row pins that + // check as the first statement of this loop.) if (session.ReverseChannelForfeited()) break; + // TEST-ONLY scheduling point BETWEEN two records: after this record's own checks and + // before the next pop's latch check, which is where a latch from RunSession's control + // thread used to slip one more record through. Only after a popped record, so the idle + // poll never pays for it; null outside ServerLoopLatchTest. + if (const auto hook = m_betweenRecordsHook.load(std::memory_order_acquire)) hook(); } if (applied != 0) { // THE TWO TALLIES MUST AGREE, AND THAT IS WHAT MAKES R-9's BATCHING BAN CHECKABLE. diff --git a/MobileGL/MG_Remote/Server/ServerLoop.h b/MobileGL/MG_Remote/Server/ServerLoop.h index c53399b7..6b1455ca 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.h +++ b/MobileGL/MG_Remote/Server/ServerLoop.h @@ -310,6 +310,13 @@ namespace MobileGL::MG_Remote::Server { void SetBeforeRetireHookForTesting(void (*hook)()) { m_beforeRetireHook.store(hook, std::memory_order_release); } + // Scheduling point only, same discipline: DrainRing runs the hook on the apply thread + // BETWEEN two records - after a popped record's own checks, before the next pop's latch + // check - and never on an empty-ring poll. ServerLoopLatchTest latches from a second + // thread there, which is the interleaving codex closeout finding 6 named. + void SetBetweenRecordsHookForTesting(void (*hook)()) { + m_betweenRecordsHook.store(hook, std::memory_order_release); + } // C7 / ID-54 diagnostics, read by ServerLoopTest's C7 and N-3 controls. NativeBindCount is // how many times ApplyMakeCurrent FORWARDED a bind to the backend (a tuple it did not @@ -459,6 +466,7 @@ namespace MobileGL::MG_Remote::Server { // that reached the blocking Park. Relaxed everywhere: it is a gauge. std::atomic m_parkBlocks{0}; std::atomic m_beforeRetireHook{nullptr}; + std::atomic m_betweenRecordsHook{nullptr}; // 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 diff --git a/MobileGL/MG_Test/Wire/PeerLatchTest.cpp b/MobileGL/MG_Test/Wire/PeerLatchTest.cpp index 706fe0eb..f6b6539d 100644 --- a/MobileGL/MG_Test/Wire/PeerLatchTest.cpp +++ b/MobileGL/MG_Test/Wire/PeerLatchTest.cpp @@ -33,7 +33,7 @@ // // THE DECLINE-AND-CLOSE HALF has its own cases below the table (they are not sites, so they are // not rows; scripts/ci/ph_latch_sites.py's MECHANICS list names them): a latched record is the -// last one its batch applies (DrainRing's two latch checks), and a latch raised on the apply +// last one its batch applies (DrainRing's pre-pop latch check), and a latch raised on the apply // thread closes the session while the peer is still connected and silent (RunSession's sliced // control wait). ServerLoopTest holds the in-process half of the same checks. // @@ -394,7 +394,7 @@ namespace { // SEVERAL IMAGES, ONE PUBLISH: each gets its own filler (its ring position is taken right // after that filler is encoded, so a wrap pad between two of them cannot shift it), all are // overwritten, and one PublishAndNotify makes them visible to the server together - the shape - // of a peer that batches, which is what DrainRing's per-record latch check is for. + // of a peer that batches, which is what DrainRing's pre-pop latch check stops mid-batch. // `mutateHeader` applies to the FIRST image's header; `outSeq` is the LAST record's seq. bool PublishImages(Client::ClientSession& client, const std::vector>& images, Uint64* outSeq, @@ -1681,10 +1681,9 @@ namespace { } } // namespace -// DrainRing's two latch checks, over the wire: the record behind the latched one in the same batch -// is never applied - its own named fault never reaches the log - and the session closes having -// latched exactly one. Red with either check deleted: the top one (the apply thread's exit-path -// drain then applies the second record) or the per-record one (the same drain does). +// DrainRing's pre-pop latch check, over the wire: the record behind the latched one in the same +// batch is never applied - its own named fault never reaches the log - and the session closes +// having latched exactly one. Red with the check deleted (the same drain applies the second record). TEST(PeerLatchTest, ALatchedRecordIsTheLastRecordItsBatchApplies) { Supervisor sup; ASSERT_TRUE(Launch("batch", /*tcp=*/false, &sup)); diff --git a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp index 3e458a5f..18d1a887 100644 --- a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp @@ -2912,12 +2912,79 @@ namespace { fixture.Stop(); ::_exit(failed); } + + // A LATCH FROM ANOTHER THREAD, BETWEEN TWO RECORDS OF ONE BATCH (codex closeout finding 6). + // RunSession's control thread latches a malformed SurfaceOp while the apply thread is mid-batch; + // the interleaving the finding names is "just after the apply thread's per-record check". The + // loop's between-records hook is that point, made deterministic: on its FIRST call - after the + // first record's own checks, before the second pop - it starts a thread that latches, and joins + // it, so the latch is stored by another thread and complete before the apply thread moves on. + enum : int { + kBetweenThreadStayed = 1, // the apply thread was still running 3 s after the latch, no Stop() + kBetweenDrainedMore = 2, // a record behind the latch point reached the applier + kBetweenWrongFault = 4, // not exactly one fault latched, or it is not the control thread's + kBetweenWatermark = 8, // appliedSeq moved past the first record + kBetweenHookCalls = 16, // the hook did not run exactly once (the case did not interleave) + }; + + std::atomic g_betweenRecordsCalls{0}; + + void LatchFromAControlThreadOnTheFirstCall() { + if (g_betweenRecordsCalls.fetch_add(1, std::memory_order_acq_rel) != 0) return; + std::thread control([] { + (void)MobileGL::MG_Remote::SessionLatch( + MobileGL::MG_Remote::MGFatalFamily::ProtocolCorruption, + "MGPipe: Fatal{ProtocolCorruption, \"unit.between-records\"} - latched by a control " + "thread after the apply thread's checks on the first record"); + }); + control.join(); + } + + [[noreturn]] void LatchFromAnotherThreadBetweenTwoRecordsAndExit() { + Remote::ArmSessionLatch(); + ServerFixture fixture; + if (!fixture.Handshake() || !fixture.StartLoop()) ::_exit(64); + if (!fixture.WaitUntilTrulyParked()) ::_exit(65); + Server::ServerLoop& loop = Server::ServerLoopInstance(); + loop.SetBetweenRecordsHookForTesting(&LatchFromAControlThreadOnTheFirstCall); + // Three LEGAL draws under ONE publish (the draw ARecordShorterThanItsType... proves applies + // in an armed child without latching), so nothing but the control thread's latch can stop + // the batch. + MG_Pipe::MGPDrawInfo info{}; + info.Mode = 0x0004; // GL_TRIANGLES + info.InstanceCount = 1; + info.MinIndex = ~0u; + info.MaxIndex = ~0u; + info.NumDraws = 1; + const MG_Pipe::MGPDrawRange range{0, 3, 0}; + Uint64 last = Codec::kInvalidSeq; + for (int i = 0; i < 3; ++i) { + last = fixture.encoder.EncodeRecord(MG_Pipe::MGPWireOp::DrawVbo, &info, sizeof(info), &range, + sizeof(range)); + if (last == Codec::kInvalidSeq) ::_exit(66); + } + fixture.encoder.Publish(); + fixture.producer.PublishAndNotify(last); + + int failed = 0; + if (!PollUntil([&] { return !loop.Running(); }, 3000)) failed |= kBetweenThreadStayed; + if (loop.DrainedRecords() != 1) failed |= kBetweenDrainedMore; + if (Remote::SessionLatchCount() != 1 || + std::strstr(Remote::SessionLatchedLine(), "\"unit.between-records\"") == nullptr) { + failed |= kBetweenWrongFault; + } + if (fixture.session->Consumer().AppliedSeq() != 1) failed |= kBetweenWatermark; + if (g_betweenRecordsCalls.load(std::memory_order_acquire) != 1) failed |= kBetweenHookCalls; + loop.SetBetweenRecordsHookForTesting(nullptr); + fixture.Stop(); + ::_exit(failed); + } } // namespace -// DrainRing's two latch checks and the apply thread's own exit, in process. Red with the top -// check deleted (the exit-path drain applies the null-handle record: bits 2|4|16), with the -// per-record check deleted (the same drain applies both records behind it: bits 2|4|16), and with -// ApplyThreadMain's latched break deleted (bit 1: the thread spins on a ring it will not drain). +// DrainRing's pre-pop latch check and the apply thread's own exit, in process. Red with the check +// deleted (the drain applies both records behind the latched one: bits 2|4|16 - the exit-path drain +// would too), and with ApplyThreadMain's latched break deleted (bit 1: the thread spins on a ring it +// will not drain). TEST(ServerLoopLatchTest, ALatchedRecordEndsItsBatchAndTheApplyThreadLeavesWithoutAStop) { EXPECT_EXIT(RunALatchedBatchAndExit(), ::testing::ExitedWithCode(0), ".*") << "bits: 1 = the apply thread was still running 3 s after the latch; 2 = a record behind the " @@ -2959,6 +3026,18 @@ TEST(ServerLoopLatchTest, ARecordShorterThanItsTypeIsLatchedBeforeTheApplierStam << "bits: 1 = the Clear verb was stamped from a record shorter than MGPClear; 2 = the latched " "line is not record.Minimum; 4 = not exactly two records reached the applier (64+ = setup)"; } + +// Codex closeout finding 6: a latch ANOTHER thread stores between two records of one batch stops the +// next pop. Red with DrainRing's pre-pop check put back to the F2 shape (a check at the function's +// top and one under `++applied;`, the hook where it is now): the latch lands after the first +// record's checks, the drain pops and applies the second draw, and only then looks again (bits 2|8). +TEST(ServerLoopLatchTest, ALatchFromAnotherThreadBetweenTwoRecordsStopsTheNextPop) { + EXPECT_EXIT(LatchFromAnotherThreadBetweenTwoRecordsAndExit(), ::testing::ExitedWithCode(0), ".*") + << "bits: 1 = the apply thread was still running 3 s after the latch; 2 = a record behind the " + "latch point reached the applier; 4 = not exactly the control thread's fault latched; 8 = " + "appliedSeq moved past the first record; 16 = the between-records hook did not run exactly " + "once (64+ = setup)"; +} #endif int main(int argc, char** argv) { diff --git a/scripts/ci/ph_latch_sites.py b/scripts/ci/ph_latch_sites.py index a5aac0ef..596ff582 100644 --- a/scripts/ci/ph_latch_sites.py +++ b/scripts/ci/ph_latch_sites.py @@ -23,7 +23,7 @@ behind popcount(Mask) == Count, so only the mismatch is reachable). covering row in turn and requires its group to turn up unmapped. THE DECLINE-AND-CLOSE HALF IS NOT A SITE, so it is not a row: MECHANICS below names each check -that makes a latched session decline the rest of its work and close (DrainRing's two latch checks, +that makes a latched session decline the rest of its work and close (DrainRing's pre-pop latch check, the apply thread's own exit, the control pump's and SurfaceOpCodec's latched answers, RunSession's sliced wait, the pre-gate ahead of the applier's stamp, SessionLatch's own bookkeeping), the code that IS the check, and the cases that go red without it. The script requires both to exist. @@ -75,14 +75,20 @@ WIRE_TESTS = ROOT / "MobileGL" / "MG_Test" / "Wire" # (source, what the check does, a regex that IS the check, [(test file, case name)]) - each case goes # red with the check deleted (the red-once is recorded in the package's NOTE). MECHANICS = [ - (REMOTE / "Server" / "ServerLoop.cpp", "DrainRing: a drain ENTERED latched pops nothing (the exit-path drain)", - r"Uint64 ServerLoop::DrainRing\(\) \{.*?if \(SessionLatched\(\)\) return 0;", + # ONE check, the first statement of DrainRing's loop, immediately before every pop. It replaced + # the two the F2 latch package shipped (one at the function's top, one under `++applied;`): + # codex closeout finding 6 showed a latch that another thread stores after the per-record check + # still let one more record be popped and applied. The one check does all three jobs - a drain + # ENTERED latched pops nothing (the exit-path drain), a record that latched is the last one its + # batch applies, and a latch from RunSession's control thread between two records stops the + # next pop - and each case below goes red with it deleted. The pattern may not leave DrainRing's + # body (`\n }\n` closes a member function in ServerLoop.cpp). + (REMOTE / "Server" / "ServerLoop.cpp", + "DrainRing: no record is popped once the session latched (checked immediately before every pop)", + r"Uint64 ServerLoop::DrainRing\(\) \{(?:(?!\n \}\n).)*?for \(;;\) \{\s*if \(SessionLatched\(\)\) break;", [("ServerLoopTest.cpp", "ALatchedRecordEndsItsBatchAndTheApplyThreadLeavesWithoutAStop"), - ("PeerLatchTest.cpp", "ALatchedRecordIsTheLastRecordItsBatchApplies")]), - (REMOTE / "Server" / "ServerLoop.cpp", "DrainRing: a record that latched is the last one its batch applies", - r"\+\+applied;\s*if \(SessionLatched\(\)\) break;", - [("ServerLoopTest.cpp", "ALatchedRecordEndsItsBatchAndTheApplyThreadLeavesWithoutAStop"), - ("PeerLatchTest.cpp", "ALatchedRecordIsTheLastRecordItsBatchApplies")]), + ("PeerLatchTest.cpp", "ALatchedRecordIsTheLastRecordItsBatchApplies"), + ("ServerLoopTest.cpp", "ALatchFromAnotherThreadBetweenTwoRecordsStopsTheNextPop")]), (REMOTE / "Server" / "ServerLoop.cpp", "ApplyThreadMain: the apply thread leaves its loop on the latch, no Stop()", r"if \(SessionLatched\(\)\) \{\s*MGLOG_E\(\"MG_Remote server: mgl-srv-apply leaves its loop", [("ServerLoopTest.cpp", "ALatchedRecordEndsItsBatchAndTheApplyThreadLeavesWithoutAStop")]),