Repository navigation
Observed tests: wait for the restarted collector's init before reading it - #80
Conversation
…g it (G-090)
The floor leg failed once in "a call during the restart gap is answered...": snapshot/1
answered {:error, :not_started} after the loop had found the restarted pid. GenServer
registers the name before init/1 runs, and init/1 creates the table snapshot/1 reads, so a
poll on the name can land between the two; the floor runner is slow enough to. Both restart
tests now make a call to the new process (:sys.get_state/1, answered only after init returns)
before reading it. Measured: with a 200 ms sleep put into init/1 before the table, the old
tests fail (both of them: the kill test had the same window, latent) and the new ones pass;
unmodified, 30 seeds, 0 failures. No package change: the gap is the collector's documented
restart gap, and it ends when init returns.
Signed-off-by: Ayla Croft <aylacroft@proton.me>
b3d8436 to
47dd2e1
Compare
minitru
left a comment
minitru
left a comment
There was a problem hiding this comment.
Post-merge review (+8 lines, one test file). The fix is correct for the flake it targets, but the second restart test still has the same race in the opposite direction. I read the diff, the merged test file and observed.ex; I did not run the suite.
What it fixes, and why it's right
GenServerregisters the name beforeinit/1runs, andinit/1creates the ETS tablesnapshot/1reads. The tests polledProcess.whereisand could read in between.:sys.get_state(new)is a sound barrier: a system message is only answered afterinitreturns. It's better than a sleep or a retry loop onsnapshot.- It's test-only;
snapshot/1returning{:error, :not_started}in that window matches its documented contract, so no production change is needed.
Main finding: the "gap" assertions can lose the race the other way
In the second test, right after assert_receive {:DOWN, ...}, the test asserts:
Observed.snapshot(name) == {:error, :not_started}{Observed, name}is not in the telemetry handler list
Both assume the supervisor has not restarted the child yet. Nothing holds it back: the supervisor gets its exit signal at about the same time the test gets :DOWN, and the new init is only a table create plus a detach and attach. If the restart lands first, snapshot returns {:ok, ...} and the handler is attached again, so both assertions fail.
The PR's evidence doesn't cover this direction. The forced 200 ms sleep in init widens the gap, which makes this failure less likely, and 30 clean seeds on an idle machine say little about a loaded CI runner, which is where the first flake showed up.
A deterministic fix is to hold the supervisor still for the gap: :sys.suspend(sup) before the kill, run the gap assertions, then :sys.resume(sup) and wait for the restart as now.
Minor
- The same four-line comment and call are pasted in both tests. A small helper (for example
await_restart(name, old_pid)that does the polling and theget_state) would give the explanation one home. - If the restarted process dies between
whereisandget_state, the test exits rather than fails an assertion. That's acceptable, just noisier to read in CI.
2 participants
The floor leg failed once on #78's PR run ("a call during the restart gap is answered..."):
snapshot/1answered{:error, :not_started}right after the test found the restarted pid. GenServer registers the name beforeinit/1runs, andinit/1creates the tablesnapshot/1reads, so the test could read in between.Both restart tests now call the new process (
:sys.get_state/1, answered only afterinitreturns) before reading it. Evidence: with a 200 ms sleep forced intoinit/1, the old tests fail (both; the kill test had the same latent window) and the new ones pass; unmodified, 30 seeds, 0 failures. Test-only; local gate passes.