durable_manager.test: assert sem-implies-on-disk, not a timing race (#551) - #577
Merged
Merged
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the #551 flake for real. Test-only.
Why the first fix didn't hold
SemFiresOnlyAfterFlushasserted that the durability sem had not fired just afterSave(), on the premise that the writer waits out its write delay before flushing. #553 guarded that assertion on elapsed time and widened the delay to 2s. It still failed on aarch64 on 09-15, 09-18 and 09-19, each time with the fix in place.The premise is false.
Util::SleepUntilhas had its comparison inverted since 2014 and never sleeps (#576). SoTFlush::WaitForreturns at once, and the writer flushes the moment a save arrives. The negative assertion was a race between the writer and the test thread's very next instruction. The 2s delay couldn't help, because it is never applied.What changes
The fixture now checks the #277 property in the direction that cannot race. After
sem.Pop(), the durable file must already be in the engine's file set:TFileService::InsertFileadds the file to its map synchronously, before queueing the op.TSortedByIdFile's constructor waits on that op's completion.ReleaseSaverspush the sem.So the check holds on every interleaving and can't fail on correct code. A pre-#277 synchronous push still fails it, because
Pop()returns while the file is still being written. That is proven below.The write delay goes back to the 300ms the other fixtures use. It is a no-op either way until #576 is decided.
Proven to catch the regression
A throwaway branch reintroduced the pre-#277 behavior (
signal_now = trueinSave()), andci.ymlwas dispatched on it. Result:make testfailed on exactly the new assertion (run 36960134990; branch since deleted):