Skip to content

Commit 48e5bfd

Browse files
ebmifaclaude
andauthored
Fix arm-after-restart race in test_auto_fork_recovery_transaction_fork (#27322)
## Description `test_auto_fork_recovery_transaction_fork` (added in #27062) fails when the restarted target validator completes catch-up re-execution before the fork-injection failpoint is armed — the injection only fires on the checkpoint-executor path, so once catch-up finishes the fork window is closed forever and `assert!(forked, ...)` times out after 60s. The test's own comment documents the bet ("start() returns before the executor catches up, so the injection lands first"), but the ordering is not guaranteed: catch-up can run inside `start()`'s internal awaits, and how much real-thread progress happens there is environment-dependent. There is also a latent hazard: a fork tripping before `register_fork_kill_failpoints` is armed would `fatal!`-abort the whole sim. Observed on the v1.77.0 version-bump PR (#27321): the `simtest` job failed twice with the identical signature ([run](https://github.com/MystenLabs/sui/actions/runs/29551659120)) — same merge commit, same sha-derived `MSIM_TEST_SEED` (`11397606143398739615`). Full analysis on [#27062](#27062 (comment)). **The fix**: arm the fork injection and the kill hooks **before** restarting the target. The target's new sim node id is unknowable until `start()` returns, so the closures match by exclusion instead: while the target is down, snapshot the sim ids of every running node (remaining validators + fullnodes) — any id outside that set executing transactions can only be the restarted target (nothing else starts during the test). `register_fork_kill_failpoints` now takes a resolver closure instead of a fixed map; the two sibling call sites pass a closure over their existing maps, behavior unchanged. ## Test plan All local, on this branch (`cargo simtest -p sui-benchmark`): - `test_auto_fork_recovery_transaction_fork`: PASS on seeds 1, 2, 3 and on CI's failing seed `11397606143398739615` - Siblings using the refactored helper: `test_auto_fork_recovery_checkpoint_fork` PASS, `test_split_brain_recovery_via_checkpoint_overrides` PASS Honest caveat: I could not reproduce the CI failure locally — the unfixed test passed 10/10 on this 16-core box across seeds (including CI's exact merge tree + seed) and under CPU saturation. The failure needs whatever real-thread scheduling conditions CI runners produce; the fix removes the ordering dependence entirely rather than tuning timings, so it holds regardless of which environments can currently lose the race. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 2684bb6 commit 48e5bfd

1 file changed

Lines changed: 57 additions & 32 deletions

File tree

crates/sui-benchmark/tests/simtest.rs

Lines changed: 57 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,18 +1044,19 @@ mod test {
10441044
fn register_fork_kill_failpoints(
10451045
forked_validators: Arc<Mutex<HashSet<AuthorityName>>>,
10461046
checkpoint_overrides: Arc<Mutex<BTreeMap<u64, String>>>,
1047-
node_to_authority_map: std::collections::HashMap<
1048-
sui_simulator::task::NodeId,
1049-
AuthorityName,
1050-
>,
1047+
resolve_authority: impl Fn(sui_simulator::task::NodeId) -> Option<AuthorityName>
1048+
+ Clone
1049+
+ Send
1050+
+ Sync
1051+
+ 'static,
10511052
) {
10521053
register_fail_point_arg("kill_checkpoint_fork_node", {
10531054
let forked_validators = forked_validators.clone();
10541055
let checkpoint_overrides = checkpoint_overrides.clone();
1055-
let node_to_authority_map = node_to_authority_map.clone();
1056+
let resolve_authority = resolve_authority.clone();
10561057
move || {
1057-
let current = node_to_authority_map.get(&sui_simulator::current_simnode_id())?;
1058-
if forked_validators.lock().unwrap().contains(current) {
1058+
let current = resolve_authority(sui_simulator::current_simnode_id())?;
1059+
if forked_validators.lock().unwrap().contains(&current) {
10591060
Some(checkpoint_overrides.clone())
10601061
} else {
10611062
None
@@ -1064,11 +1065,10 @@ mod test {
10641065
});
10651066
register_fail_point_if("kill_transaction_fork_node", {
10661067
let forked_validators = forked_validators.clone();
1067-
let node_to_authority_map = node_to_authority_map.clone();
1068+
let resolve_authority = resolve_authority.clone();
10681069
move || {
1069-
node_to_authority_map
1070-
.get(&sui_simulator::current_simnode_id())
1071-
.is_some_and(|current| forked_validators.lock().unwrap().contains(current))
1070+
resolve_authority(sui_simulator::current_simnode_id())
1071+
.is_some_and(|current| forked_validators.lock().unwrap().contains(&current))
10721072
}
10731073
});
10741074
// Split-brain bookkeeping: record the canonical (non-forked) digest into checkpoint_overrides.
@@ -1515,7 +1515,10 @@ mod test {
15151515
register_fork_kill_failpoints(
15161516
forked_validators.clone(),
15171517
checkpoint_overrides.clone(),
1518-
node_to_authority_map.clone(),
1518+
{
1519+
let node_to_authority_map = node_to_authority_map.clone();
1520+
move |id| node_to_authority_map.get(&id).copied()
1521+
},
15191522
);
15201523
}
15211524
}),
@@ -1659,7 +1662,10 @@ mod test {
16591662
register_fork_kill_failpoints(
16601663
forked_validators.clone(),
16611664
checkpoint_overrides.clone(),
1662-
node_to_authority_map.clone(),
1665+
{
1666+
let node_to_authority_map = node_to_authority_map.clone();
1667+
move |id| node_to_authority_map.get(&id).copied()
1668+
},
16631669
);
16641670
}
16651671
}),
@@ -1770,43 +1776,62 @@ mod test {
17701776
.stop();
17711777
tokio::time::sleep(std::time::Duration::from_secs(8)).await;
17721778

1773-
// Restart the target (it gets a new sim node id) and rebuild the map before arming the
1774-
// injection. start() returns before the executor catches up, so the injection lands first.
1775-
test_cluster
1779+
// Arm the injection and kill hooks BEFORE restarting the target: its catch-up
1780+
// re-execution can complete inside start()'s internal awaits, so arming afterwards
1781+
// races the executor and loses on some schedules (the fork window closes forever once
1782+
// catch-up finishes — and a fork tripped before the kill hooks are armed would
1783+
// fatal!-abort the sim). The target's new sim node id is unknowable until start()
1784+
// returns, so match by exclusion instead: while the target is down, snapshot the sim
1785+
// ids of every running node — any id outside that set executing transactions must be
1786+
// the restarted target (nothing else starts during this test).
1787+
let known_other_ids: std::collections::HashSet<sui_simulator::task::NodeId> = test_cluster
17761788
.swarm
1777-
.validator_nodes()
1778-
.find(|validator| validator.name() == target)
1779-
.unwrap()
1780-
.start()
1781-
.await
1782-
.unwrap();
1789+
.all_nodes()
1790+
.filter_map(|node| {
1791+
node.get_node_handle()
1792+
.map(|handle| handle.with(|n| n.get_sim_node_id()))
1793+
})
1794+
.collect();
17831795
let node_to_authority_map = fork_test_node_to_authority_map(&test_cluster);
17841796

17851797
// Fork the target only on the executor path, so its catch-up re-execution trips the tx check.
17861798
register_fail_point_arg("simulate_fork_during_execution", {
17871799
let forked_validators = forked_validators.clone();
17881800
let effects_overrides = effects_overrides.clone();
1789-
let node_to_authority_map = node_to_authority_map.clone();
1801+
let known_other_ids = known_other_ids.clone();
17901802
move || {
1791-
let current = node_to_authority_map.get(&sui_simulator::current_simnode_id())?;
1792-
if *current == target {
1803+
if known_other_ids.contains(&sui_simulator::current_simnode_id()) {
1804+
None
1805+
} else {
17931806
Some((
17941807
forked_validators.clone(),
17951808
/* full_halt: */ false,
17961809
effects_overrides.clone(),
17971810
1.0f32,
17981811
/* executor_path_only: */ true,
17991812
))
1800-
} else {
1801-
None
18021813
}
18031814
}
18041815
});
1805-
register_fork_kill_failpoints(
1806-
forked_validators.clone(),
1807-
checkpoint_overrides.clone(),
1808-
node_to_authority_map.clone(),
1809-
);
1816+
register_fork_kill_failpoints(forked_validators.clone(), checkpoint_overrides.clone(), {
1817+
let node_to_authority_map = node_to_authority_map.clone();
1818+
let known_other_ids = known_other_ids.clone();
1819+
move |id| {
1820+
node_to_authority_map
1821+
.get(&id)
1822+
.copied()
1823+
.or_else(|| (!known_other_ids.contains(&id)).then_some(target))
1824+
}
1825+
});
1826+
1827+
test_cluster
1828+
.swarm
1829+
.validator_nodes()
1830+
.find(|validator| validator.name() == target)
1831+
.unwrap()
1832+
.start()
1833+
.await
1834+
.unwrap();
18101835

18111836
// Wait until the target forks (after which kill_transaction_fork_node shuts it down).
18121837
let mut forked = false;

0 commit comments

Comments
 (0)