From 2a8758b65e91462d0946e2edae27e92c39c1fef7 Mon Sep 17 00:00:00 2001 From: Patrick O'Reilly Date: Thu, 1 Oct 2026 20:25:41 -0700 Subject: [PATCH] durable_manager.test: assert sem-implies-on-disk, not a timing race (#551) SemFiresOnlyAfterFlush asserted that the durability sem had not fired right after Save(), on the premise that the writer waits out its write delay before flushing. It never has. Util::SleepUntil's comparison is inverted, so it never sleeps (#576), and the writer flushes the moment a save arrives. The negative assertion was a race between the writer and the test thread's next instruction, which aarch64 lost three times after #551's fix. Widening the delay to 2s could not help, because the delay is never applied. Check the #277 property in the direction that cannot race instead: after sem.Pop(), the durable file must already be in the engine's file set. The writer inserts it (InsertFile adds it to the map synchronously) and waits for that op before ReleaseSavers pushes the sem, so this holds on every interleaving. A pre-#277 synchronous push still fails it, because Pop() returns while the file is still being written. --- changelog.d/551-sem-implies-on-disk.md | 1 + orly/indy/disk/durable_manager.test.cc | 46 +++++++++++--------------- 2 files changed, 21 insertions(+), 26 deletions(-) create mode 100644 changelog.d/551-sem-implies-on-disk.md diff --git a/changelog.d/551-sem-implies-on-disk.md b/changelog.d/551-sem-implies-on-disk.md new file mode 100644 index 00000000..3a396708 --- /dev/null +++ b/changelog.d/551-sem-implies-on-disk.md @@ -0,0 +1 @@ +- **Fixed**: `durable_manager.test` `SemFiresOnlyAfterFlush` kept flaking on aarch64 after its #551 fix (09-15, 09-18, 09-19). The fixture asserted the durability sem had not fired right after `Save()`, on the premise that the writer waits out its write delay first. It does not: `Util::SleepUntil` never sleeps (#576), so the writer flushes the moment a save arrives, and the assertion was a race against it, which is also why widening the delay to 2s changed nothing. The fixture now checks the #277 property in the direction that cannot race: once the sem fires, the durable file is already registered with the engine (#551). diff --git a/orly/indy/disk/durable_manager.test.cc b/orly/indy/disk/durable_manager.test.cc index d517938a..671b67b9 100644 --- a/orly/indy/disk/durable_manager.test.cc +++ b/orly/indy/disk/durable_manager.test.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -156,8 +157,8 @@ static void RunOnFiber(const std::function *frame_pool_manager) { TScheduler scheduler(TScheduler::TPolicy(4, 8, milliseconds(30000))); @@ -167,40 +168,33 @@ FIXTURE(SemFiresOnlyAfterFlush) { const Durable::TTtl ttl(600); const Durable::TDeadline deadline = Durable::TDeadline::clock::now() + ttl; const std::string blob = "some serialized durable"; - /* The write delay this fixture runs the manager at. Deliberately longer than the 300ms - the others use: the negative assertion below is only meaningful while the flush has not - yet had a chance to run, so the window has to be wide relative to scheduler jitter on a - loaded CI runner. The cost is that sem.Pop() then waits this long -- ~2s on one fixture - against a 203-binary suite (#551). */ - const auto write_delay = milliseconds(2000); /* manager scope */ { TDurableManager durable_manager(&scheduler, runner_cons, frame_pool_manager, &rep_stub, mem_engine.GetEngine(), 100UL /* max cache size */, - write_delay, + milliseconds(300) /* write delay */, milliseconds(300) /* merge delay */, milliseconds(10000) /* layer cleaning interval */, 20UL /* temp file consol thresh */, true /* create */); Durable::TSem sem; - const auto saved_at = steady_clock::now(); durable_manager.Save(id, deadline, ttl, blob, &sem); - const bool fired_immediately = sem.GetFd().IsReadable(0); - const auto elapsed = steady_clock::now() - saved_at; - /* Not yet: durability must not be signalled before the data is written (#277). - (Pre-#277, Save() pushed the sem synchronously, which makes this assertion fail.) - - Guarded on measured elapsed time rather than asserted outright. The check is only - SOUND while less than the write delay has actually passed -- if this thread was - descheduled past it, the flush has legitimately run and a fired sem proves nothing - about #277. Asserting unconditionally is what made this fixture fail intermittently - on loaded runners and, worse, point the blame at whatever change was under test - (#551). With a 2s window the skip should be vanishingly rare; it exists so that when - it does happen the answer is "unprovable here", not "regression". */ - if (elapsed < write_delay) { - EXPECT_FALSE(fired_immediately); - } - /* Now block for it: the flush makes it fire. */ sem.Pop(); + /* #277: by the time the sem fires, the save must already be on disk. The writer registers + the durable file with the engine, and waits for that to land, before it releases the + savers (TSortedByIdFile's constructor, then ReleaseSavers), so this holds on every + interleaving and cannot fail on correct code. Pre-#277, Save() pushed the sem + synchronously: Pop() then returns at once, while the writer has barely started writing + the file, so the set is all but certainly still empty -- a regression is caught on + essentially every run, and correct code is never failed. + + This used to be asserted the other way round: "the sem has not fired just after Save()", + on the premise that the writer waits out its write delay first. It does not -- + Util::SleepUntil never sleeps (#576), so the writer flushes the moment the save arrives + and that assertion was a race against it. That race is the #551 flake, and why widening + the delay to 2s did not stop it. */ + std::vector durable_files; + mem_engine.GetEngine()->AppendFileGenSet(TDurableManager::DurableByIdFileId, durable_files); + EXPECT_FALSE(durable_files.empty()); /* Visible through this manager, too. */ std::string loaded; EXPECT_TRUE(durable_manager.TryLoad(id, loaded));