Repository navigation
Conversation
03fc97c to
6b44525
Compare
6b44525 to
ccaed67
Compare
ccaed67 to
e251b66
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async fn wait_for_ds_state( | ||
| dsc_client: &Client, | ||
| cid: u32, | ||
| want: DownstairsState, | ||
| ) -> Result<()> { | ||
| loop { | ||
| let state = dsc_client | ||
| .dsc_get_ds_state(cid) | ||
| .await | ||
| .context("failed to get downstairs state")? | ||
| .into_inner(); | ||
| if state == want { | ||
| return Ok(()); | ||
| } | ||
| tokio::time::sleep(tokio::time::Duration::from_secs(4)).await; | ||
| } |
There was a problem hiding this comment.
🟡 (optional) If a downstairs crashes, replace-reconcile and replace-before-active now hang forever instead of failing. wait_for_ds_state keeps polling every 4s when dsc reports Failed or Error. Those states are terminal, so the target Exit or Running state never comes. Fix: return an error from wait_for_ds_state when the state is DownstairsState::Failed or DownstairsState::Error and that is not the wanted state. Adding an overall timeout would also help. This covers both calls in recreate_downstairs: the wait for Exit and the wait for Running.
Why this was flagged
In dsc/src/main.rs:1510-1516, a downstairs that exits with a status code is reported as DownstairsState::Failed. The monitor also clears keep_running and start_once, so it never restarts and the state never changes again. Panics in the monitor are reported as Error (dsc/src/main.rs:1533). recreate_downstairs calls dsc_start and then wait_for_ds_state(.., Running). The first poll can come before the Running update arrives, and then the loop sleeps 4s. If the new downstairs fails during that window, the loop polls forever and there is no timeout. In the same way, if the old downstairs already exited non-zero, the Exit wait never finishes. On the base branch, replace_before_active only logged the dsc_start result, so a broken downstairs showed up as a failed replace in the next loop. After this change, crutest stalls silently and the CI or test run hangs instead of reporting a failure.
Verification: wait_for_ds_state (crutest/src/main.rs:2501-2517) returns only when state == want. Otherwise it sleeps 4s and loops, with no timeout and no check for terminal states. So wait_for_ds_state(.., DownstairsState::Running) at line 2573 polls forever if the new downstairs fails. The same unbounded polling already exists in these tests on the base branch.
replace-reconcile and replace-before-active reuse the downstairs they replace as the replacement in a later loop. After each replacement, recreate the old downstairs' region with the new dsc command, so every replacement these tests use is empty and has never been active.
e251b66 to
d41d7a1
Compare
replace-reconcile and replace-before-active both reuse the
downstairs they replace as the replacement in a later loop.
After each replacement, recreate the old downstairs' region
with the new dsc command, so every replacement these tests
use is empty and has never been active.