test: fix two racy tests - #151
Merged
Merged
Conversation
`record_nodrop` and `recorder_drop_staged` deadlock permanently in CI. When it happens the test binary never exits, so the job runs until GitHub's timeout with no failure output. It shows up most often under `cargo llvm-cov`, but nothing about coverage instrumentation is special here; it just makes the whole suite slower, and the suite also runs `mt_record_static`/`mt_record_dynamic`, which keep 32 CPU-bound threads on a 2-4 vCPU runner. The reader thread is then far more likely to be preempted in the one window where this race is lost. `refresh()` reads `recorders` under `truth`, bumps `phase`, and then blocks in `recv()` until it has received that many histograms. That `recv()` cannot fail, since the `Sender` lives in the `Arc<Shared>` the `SyncHistogram` itself holds, so the wait is unbounded. A live `Recorder` only sends when a *subsequent* write observes the new phase, or when it is dropped. Both tests had the writer perform a fixed number of writes and then park on a `Barrier` that the reader only reaches after `refresh()` returns. If the writer got through all of its writes before the reader's `phase.fetch_add`, it never observed the phase shift, parked on the barrier, and could not write again -- while the reader blocked forever on a histogram nobody would send, and so never reached its own `barrier.wait()`. Neither side can make progress. Confirmed by forcing exactly that interleaving under gdb: breaking just before the `recorders` read is enough to hang both tests. So the writers now record until the reader tells them its phase shift is through, which is the pattern `clone_idle_recorder` already uses. The handoff is explicit instead of depending on who gets scheduled first. The cost is that `record_nodrop` loses its exact-count assertions: the number of writes is no longer fixed, so it can only check that the phase shift carried at least one sample across and that everything it carried was `TEST_VALUE_LEVEL`. What the test actually guards -- that `refresh()` is released by a write while the recorder is still alive -- is unchanged, and is now guaranteed rather than hoped for. `recorder_drop_staged` keeps its exact assertions, since the writer returns its own count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
record_nodropandrecorder_drop_stageddeadlock permanently in CI. When it happens the test binary never exits, so the job runs until GitHub's timeout with no failure output. It shows up most often undercargo llvm-cov, but nothing about coverage instrumentation is special here; it just makes the whole suite slower, and the suite also runsmt_record_static/mt_record_dynamic, which keep 32 CPU-bound threads on a 2-4 vCPU runner. The reader thread is then far more likely to be preempted in the one window where this race is lost.refresh()readsrecordersundertruth, bumpsphase, and then blocks inrecv()until it has received that many histograms. Thatrecv()cannot fail, since theSenderlives in theArc<Shared>theSyncHistogramitself holds, so the wait is unbounded. A liveRecorderonly sends when a subsequent write observes the new phase, or when it is dropped.Both tests had the writer perform a fixed number of writes and then park on a
Barrierthat the reader only reaches afterrefresh()returns. If the writer got through all of its writes before the reader'sphase.fetch_add, it never observed the phase shift, parked on the barrier, and could not write again -- while the reader blocked forever on a histogram nobody would send, and so never reached its ownbarrier.wait(). Neither side can make progress. Confirmed by forcing exactly that interleaving under gdb: breaking just before therecordersread is enough to hang both tests.So the writers now record until the reader tells them its phase shift is through, which is the pattern
clone_idle_recorderalready uses. The handoff is explicit instead of depending on who gets scheduled first.The cost is that
record_nodroploses its exact-count assertions: the number of writes is no longer fixed, so it can only check that the phase shift carried at least one sample across and that everything it carried wasTEST_VALUE_LEVEL. What the test actually guards -- thatrefresh()is released by a write while the recorder is still alive -- is unchanged, and is now guaranteed rather than hoped for.recorder_drop_stagedkeeps its exact assertions, since the writer returns its own count.