[adapters] Fix test_clock_advance_survives_checkpoint.#6581
Conversation
This fixes two bugs that caused the test to fail: 1. A bug in the test itself, where the test did not wait for the journal to finish replay before probing pipeline's state 2. A minor bug introduced by the recent fix to exactly-once FT where the last step before the pipeline terminated did not get recorded in the journal. I don't think this violates the exactly-once FT contract, but it does make it harder to write robust tests. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
| let reader = controller.get_input_endpoint("now").unwrap(); | ||
| let clock_reader = reader.as_any().downcast::<super::ClockReader>().unwrap(); | ||
|
|
||
| // Replay runs asynchronously on the circuit thread, feeding the journaled |
There was a problem hiding this comment.
thanks, I guess it would be nice to hide this complexity somehow, I am pretty sure I wont remember this needs to be done next time I have to work with this API.
maybe get_input_endpoint could block until replay is complete
There was a problem hiding this comment.
Hm, I guess this test should have checked for controller status. Can't spend more time on this now.
mythical-fred
left a comment
There was a problem hiding this comment.
Both fixes read cleanly.
The controller.rs change is the interesting one: keeping the journal write on shutdown while skipping only the endpoint-lifecycle diff is exactly right. The old early-return silently dropped the last step's already-computed input, which is precisely the kind of "silent loss of committed work" that FT is supposed to make impossible. The new comment spells out both halves — why we skip the diff, and why we must not skip the journal entry — which is what future-me would want to see when staring at this code again.
The test fix (waiting for is_replaying()) is a textbook async-test hazard and the 30s bounded wait with an explicit assertion is the right shape.
Nit: the regression-coverage doc comment on the test is helpful; consider tying it to a bug id or a short "before this fix, restart observed value X" example so it survives future refactors.
This fixes two bugs that caused the test to fail:
Describe Manual Test Plan
Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes