paavohtl Claude Opus 4.6 (1M context) commited on
Commit
dae59f3
Β·
1 Parent(s): 4557f7d

fix: eliminate replay test flakiness under high concurrency

Browse files

Three independent concurrency bugs caused 10-15% failure rates when
running replay tests at -j 4+ (and ~13% at -j 16 for the worms2d suite):

1. Exclusive file locking: WA.exe opens data files (.img, .wav, .bmp)
with dwShareMode=0. Force FILE_SHARE_READ on all CreateFileA calls
in headless mode so concurrent instances can read shared game assets.

2. _findfirst directory contention: WA's file-existence check for
steam.dat uses _findfirst64i32 which does FindFirstFileA directory
enumeration. Under high concurrency, NTFS directory contention from
.openwa_tmp operations causes transient failures. Hook the check
function (0x4DFA30) to always return "exists" in headless mode.

3. thm.prv race condition: MAP_VIEW_LOAD creates, reads, and deletes
a temporary thm.prv file during terrain processing. Concurrent
instances fighting over this file caused heap corruption (crash in
ntdll RtlAllocateHeap). Extend file isolation to hook FindFirstFileA
and DeleteFileA in addition to CreateFileA, so all thm.prv operations
are redirected to per-PID temp directories.

Also adds custom.dat to isolated root files, removes the now-unnecessary
50ms inter-test launch stagger, and updates worms2d suite docs to
reflect that -j 16 is now reliable (~40s for 597 tests).

Verified: 0/750 failures at -j 16 (base suite), 0/597 failures at
-j 16 (worms2d suite, 3 consecutive runs).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

crates/openwa-core/src/address.rs CHANGED
@@ -696,6 +696,8 @@ pub mod va {
696
  fn FRONTEND_LOCAL_MP_CTOR = 0x0049_C420;
697
  fn FRONTEND_TRAINING_CTOR = 0x004E_0880;
698
  fn FRONTEND_MISSIONS_CTOR = 0x0049_9190;
 
 
699
  fn FRONTEND_POST_INIT_CTOR = 0x004C_91B0;
700
  fn FRONTEND_MAIN_MENU_CTOR = 0x0048_66C0;
701
  fn FRONTEND_SINGLE_PLAYER_CTOR = 0x004D_69F0;
 
696
  fn FRONTEND_LOCAL_MP_CTOR = 0x0049_C420;
697
  fn FRONTEND_TRAINING_CTOR = 0x004E_0880;
698
  fn FRONTEND_MISSIONS_CTOR = 0x0049_9190;
699
+ /// File-existence check via _findfirst (fastcall, ECX=filename)
700
+ fn/Fastcall FILE_EXISTS_CHECK = 0x004D_FA30;
701
  fn FRONTEND_POST_INIT_CTOR = 0x004C_91B0;
702
  fn FRONTEND_MAIN_MENU_CTOR = 0x0048_66C0;
703
  fn FRONTEND_SINGLE_PLAYER_CTOR = 0x004D_69F0;
crates/openwa-dll/src/replacements/file_isolation.rs CHANGED
@@ -1,11 +1,11 @@
1
  //! Per-instance file path isolation for concurrent test execution.
2
  //!
3
- //! Hooks `kernel32!CreateFileA` to redirect temp/scratch files to a per-PID
4
- //! subdirectory, preventing races when multiple WA.exe instances run
5
- //! simultaneously. Only active when `OPENWA_HEADLESS=1` is set.
6
  //!
7
  //! Redirected files:
8
- //! Game dir: writetest.txt, mono.tmp, custom.dat
9
  //! DATA\: land.dat, landgen.svg, current.thm, playback.thm
10
  //! ERRORLOG: ERRORLOG.TXT β†’ OPENWA_ERRORLOG_PATH (if set)
11
 
@@ -30,7 +30,14 @@ type CreateFileAFn = unsafe extern "system" fn(
30
  hTemplateFile: HANDLE,
31
  ) -> HANDLE;
32
 
 
 
 
 
 
33
  static ORIG_CREATE_FILE_A: AtomicU32 = AtomicU32::new(0);
 
 
34
 
35
  static ERRORLOG_PATH: OnceLock<Option<String>> = OnceLock::new();
36
  static TEMP_DIR: OnceLock<Option<String>> = OnceLock::new();
@@ -58,7 +65,9 @@ fn temp_dir() -> Option<&'static str> {
58
  }
59
 
60
  /// Files in the game root directory that need isolation.
61
- const ROOT_FILES: &[&str] = &["writetest.txt", "mono.tmp"];
 
 
62
 
63
  /// Files in the DATA subdirectory that need isolation.
64
  const DATA_FILES: &[&str] = &["land.dat", "landgen.svg", "current.thm", "playback.thm"];
@@ -80,7 +89,7 @@ fn redirect_path(path: &str) -> Option<String> {
80
 
81
  let tmp = temp_dir()?;
82
 
83
- // Check root-level files (writetest.txt, mono.tmp, custom.dat)
84
  for &name in ROOT_FILES {
85
  if lower.ends_with(&format!("\\{name}")) || lower == name {
86
  return Some(format!("{tmp}\\{name}"));
@@ -99,6 +108,11 @@ fn redirect_path(path: &str) -> Option<String> {
99
  None
100
  }
101
 
 
 
 
 
 
102
  unsafe extern "system" fn hook_create_file_a(
103
  lp_file_name: *const u8,
104
  desired_access: u32,
@@ -110,6 +124,12 @@ unsafe extern "system" fn hook_create_file_a(
110
  ) -> HANDLE {
111
  let orig: CreateFileAFn = core::mem::transmute(ORIG_CREATE_FILE_A.load(Ordering::Relaxed));
112
 
 
 
 
 
 
 
113
  if !lp_file_name.is_null() {
114
  if let Ok(path) = CStr::from_ptr(lp_file_name as *const i8).to_str() {
115
  if let Some(new_path) = redirect_path(path) {
@@ -138,6 +158,63 @@ unsafe extern "system" fn hook_create_file_a(
138
  )
139
  }
140
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
141
  pub fn install() -> Result<(), String> {
142
  let is_test =
143
  std::env::var("OPENWA_HEADLESS").is_ok() || std::env::var("OPENWA_REPLAY_TEST").is_ok();
@@ -147,25 +224,43 @@ pub fn install() -> Result<(), String> {
147
  }
148
 
149
  unsafe {
150
- let module = windows_sys::Win32::System::LibraryLoader::GetModuleHandleA(
151
  c"kernel32.dll".as_ptr().cast(),
152
  );
153
- if module.is_null() {
154
  return Err("kernel32.dll not loaded".to_string());
155
  }
156
 
157
- let proc = windows_sys::Win32::System::LibraryLoader::GetProcAddress(
158
- module,
159
- c"CreateFileA".as_ptr().cast(),
160
- );
161
- let addr = proc.ok_or("CreateFileA not found in kernel32.dll")?;
162
- let target = addr as *mut c_void;
163
- let trampoline = minhook::MinHook::create_hook(target, hook_create_file_a as *mut c_void)
164
- .map_err(|e| format!("MinHook create_hook failed for CreateFileA: {e}"))?;
165
- minhook::MinHook::queue_enable_hook(target)
166
- .map_err(|e| format!("MinHook queue_enable_hook failed for CreateFileA: {e}"))?;
167
 
168
- ORIG_CREATE_FILE_A.store(trampoline as u32, Ordering::Relaxed);
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
169
 
170
  let pid = std::process::id();
171
  let tmp_msg = temp_dir().map(|d| format!(" β†’ {d}")).unwrap_or_default();
@@ -173,13 +268,35 @@ pub fn install() -> Result<(), String> {
173
  .map(|t| format!(", ERRORLOG.TXT β†’ {t}"))
174
  .unwrap_or_default();
175
  let _ = log_line(&format!(
176
- "[FileIsolation] Hooked CreateFileA (pid={pid}): temp files{tmp_msg}{errorlog_msg}"
177
  ));
178
  }
179
 
180
  Ok(())
181
  }
182
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
183
  /// Clean up the per-PID temp directory. Called during DLL detach or test cleanup.
184
  pub fn cleanup() {
185
  if let Some(tmp) = TEMP_DIR.get().and_then(|o| o.as_deref()) {
 
1
  //! Per-instance file path isolation for concurrent test execution.
2
  //!
3
+ //! Hooks `kernel32!CreateFileA`, `FindFirstFileA`, and `DeleteFileA` to redirect
4
+ //! temp/scratch files to a per-PID subdirectory, preventing races when multiple
5
+ //! WA.exe instances run simultaneously. Only active when `OPENWA_HEADLESS=1`.
6
  //!
7
  //! Redirected files:
8
+ //! Game dir: writetest.txt, mono.tmp, custom.dat, thm.prv
9
  //! DATA\: land.dat, landgen.svg, current.thm, playback.thm
10
  //! ERRORLOG: ERRORLOG.TXT β†’ OPENWA_ERRORLOG_PATH (if set)
11
 
 
30
  hTemplateFile: HANDLE,
31
  ) -> HANDLE;
32
 
33
+ type FindFirstFileAFn =
34
+ unsafe extern "system" fn(lpFileName: *const u8, lpFindFileData: *mut u8) -> HANDLE;
35
+
36
+ type DeleteFileAFn = unsafe extern "system" fn(lpFileName: *const u8) -> i32;
37
+
38
  static ORIG_CREATE_FILE_A: AtomicU32 = AtomicU32::new(0);
39
+ static ORIG_FIND_FIRST_FILE_A: AtomicU32 = AtomicU32::new(0);
40
+ static ORIG_DELETE_FILE_A: AtomicU32 = AtomicU32::new(0);
41
 
42
  static ERRORLOG_PATH: OnceLock<Option<String>> = OnceLock::new();
43
  static TEMP_DIR: OnceLock<Option<String>> = OnceLock::new();
 
65
  }
66
 
67
  /// Files in the game root directory that need isolation.
68
+ /// `thm.prv` is a temporary file created/read/deleted by MAP_VIEW_LOAD during
69
+ /// terrain processing β€” concurrent instances fight over it without isolation.
70
+ const ROOT_FILES: &[&str] = &["writetest.txt", "mono.tmp", "custom.dat", "thm.prv"];
71
 
72
  /// Files in the DATA subdirectory that need isolation.
73
  const DATA_FILES: &[&str] = &["land.dat", "landgen.svg", "current.thm", "playback.thm"];
 
89
 
90
  let tmp = temp_dir()?;
91
 
92
+ // Check root-level files
93
  for &name in ROOT_FILES {
94
  if lower.ends_with(&format!("\\{name}")) || lower == name {
95
  return Some(format!("{tmp}\\{name}"));
 
108
  None
109
  }
110
 
111
+ // ─── CreateFileA hook ──────────────────────────────────────────────────────
112
+
113
+ /// WA.exe opens data files with `dwShareMode=0` (exclusive access), which blocks
114
+ /// concurrent instances from reading the same files. In headless mode, force
115
+ /// `FILE_SHARE_READ` on all file opens so multiple test instances can coexist.
116
  unsafe extern "system" fn hook_create_file_a(
117
  lp_file_name: *const u8,
118
  desired_access: u32,
 
124
  ) -> HANDLE {
125
  let orig: CreateFileAFn = core::mem::transmute(ORIG_CREATE_FILE_A.load(Ordering::Relaxed));
126
 
127
+ // Force FILE_SHARE_READ on all opens to prevent exclusive locking between
128
+ // concurrent WA.exe instances. WA opens .img, .wav, .bmp etc. with share=0,
129
+ // which causes "File Error" failures under high concurrency.
130
+ const FILE_SHARE_READ: u32 = 0x00000001;
131
+ let share_mode = share_mode | FILE_SHARE_READ;
132
+
133
  if !lp_file_name.is_null() {
134
  if let Ok(path) = CStr::from_ptr(lp_file_name as *const i8).to_str() {
135
  if let Some(new_path) = redirect_path(path) {
 
158
  )
159
  }
160
 
161
+ // ─── FindFirstFileA hook ───────────────────────────────────────────────────
162
+
163
+ /// Redirect FindFirstFileA for isolated files. MAP_VIEW_LOAD's file reader
164
+ /// (FUN_004dfa70) uses FindFirstFileA to get the file size before _fopen.
165
+ /// Without this hook, it looks in the game directory instead of the per-PID
166
+ /// temp directory where CreateFileA wrote the file.
167
+ unsafe extern "system" fn hook_find_first_file_a(
168
+ lp_file_name: *const u8,
169
+ lp_find_file_data: *mut u8,
170
+ ) -> HANDLE {
171
+ let orig: FindFirstFileAFn =
172
+ core::mem::transmute(ORIG_FIND_FIRST_FILE_A.load(Ordering::Relaxed));
173
+
174
+ if !lp_file_name.is_null() {
175
+ if let Ok(path) = CStr::from_ptr(lp_file_name as *const i8).to_str() {
176
+ if let Some(new_path) = redirect_path(path) {
177
+ let cstr: Vec<u8> = new_path.bytes().chain(std::iter::once(0)).collect();
178
+ return orig(cstr.as_ptr(), lp_find_file_data);
179
+ }
180
+ }
181
+ }
182
+
183
+ orig(lp_file_name, lp_find_file_data)
184
+ }
185
+
186
+ // ─── DeleteFileA hook ──────────────────────────────────────────────────────
187
+
188
+ /// Redirect DeleteFileA for isolated files. MAP_VIEW_LOAD deletes the temporary
189
+ /// `thm.prv` file after reading it. Without this hook, the delete targets the
190
+ /// game directory while the file is in the per-PID temp directory.
191
+ unsafe extern "system" fn hook_delete_file_a(lp_file_name: *const u8) -> i32 {
192
+ let orig: DeleteFileAFn = core::mem::transmute(ORIG_DELETE_FILE_A.load(Ordering::Relaxed));
193
+
194
+ if !lp_file_name.is_null() {
195
+ if let Ok(path) = CStr::from_ptr(lp_file_name as *const i8).to_str() {
196
+ if let Some(new_path) = redirect_path(path) {
197
+ let cstr: Vec<u8> = new_path.bytes().chain(std::iter::once(0)).collect();
198
+ return orig(cstr.as_ptr());
199
+ }
200
+ }
201
+ }
202
+
203
+ orig(lp_file_name)
204
+ }
205
+
206
+ // ─── File-exists check hook ────────────────────────────────────────────────
207
+
208
+ /// WA's file-existence check (0x4DFA30) uses `_findfirst` which does a directory
209
+ /// enumeration. Under high concurrency, NTFS directory contention causes transient
210
+ /// failures. Replace with a simple "always exists" in headless mode β€” the checked
211
+ /// files (steam.dat, graphics\Font.bmp) are guaranteed present in the game dir.
212
+ unsafe extern "fastcall" fn hook_file_exists_check(_filename: *const u8) -> u32 {
213
+ 1 // always report file as existing
214
+ }
215
+
216
+ // ─── Installation ──────────────────────────────────────────────────────────
217
+
218
  pub fn install() -> Result<(), String> {
219
  let is_test =
220
  std::env::var("OPENWA_HEADLESS").is_ok() || std::env::var("OPENWA_REPLAY_TEST").is_ok();
 
224
  }
225
 
226
  unsafe {
227
+ let k32 = windows_sys::Win32::System::LibraryLoader::GetModuleHandleA(
228
  c"kernel32.dll".as_ptr().cast(),
229
  );
230
+ if k32.is_null() {
231
  return Err("kernel32.dll not loaded".to_string());
232
  }
233
 
234
+ // Hook CreateFileA β€” path redirection + FILE_SHARE_READ forcing
235
+ hook_kernel32_fn(
236
+ k32,
237
+ c"CreateFileA",
238
+ hook_create_file_a as *mut c_void,
239
+ &ORIG_CREATE_FILE_A,
240
+ )?;
 
 
 
241
 
242
+ // Hook FindFirstFileA β€” path redirection for file size lookups
243
+ hook_kernel32_fn(
244
+ k32,
245
+ c"FindFirstFileA",
246
+ hook_find_first_file_a as *mut c_void,
247
+ &ORIG_FIND_FIRST_FILE_A,
248
+ )?;
249
+
250
+ // Hook DeleteFileA β€” path redirection for temp file cleanup
251
+ hook_kernel32_fn(
252
+ k32,
253
+ c"DeleteFileA",
254
+ hook_delete_file_a as *mut c_void,
255
+ &ORIG_DELETE_FILE_A,
256
+ )?;
257
+
258
+ // Hook file-exists check to avoid _findfirst contention under concurrency
259
+ crate::hook::install(
260
+ "FileExistsCheck",
261
+ openwa_core::address::va::FILE_EXISTS_CHECK,
262
+ hook_file_exists_check as *const (),
263
+ )?;
264
 
265
  let pid = std::process::id();
266
  let tmp_msg = temp_dir().map(|d| format!(" β†’ {d}")).unwrap_or_default();
 
268
  .map(|t| format!(", ERRORLOG.TXT β†’ {t}"))
269
  .unwrap_or_default();
270
  let _ = log_line(&format!(
271
+ "[FileIsolation] Hooked CreateFileA+FindFirstFileA+DeleteFileA (pid={pid}): temp files{tmp_msg}{errorlog_msg}"
272
  ));
273
  }
274
 
275
  Ok(())
276
  }
277
 
278
+ /// Helper: hook a kernel32 function via MinHook.
279
+ unsafe fn hook_kernel32_fn(
280
+ module: *mut c_void,
281
+ name: &core::ffi::CStr,
282
+ hook_fn: *mut c_void,
283
+ orig_store: &AtomicU32,
284
+ ) -> Result<(), String> {
285
+ let fn_name = name.to_str().unwrap_or("?");
286
+ let proc = windows_sys::Win32::System::LibraryLoader::GetProcAddress(
287
+ module as _,
288
+ name.as_ptr().cast(),
289
+ );
290
+ let addr = proc.ok_or(format!("{fn_name} not found in kernel32.dll"))?;
291
+ let target = addr as *mut c_void;
292
+ let trampoline = minhook::MinHook::create_hook(target, hook_fn)
293
+ .map_err(|e| format!("MinHook create_hook failed for {fn_name}: {e}"))?;
294
+ minhook::MinHook::queue_enable_hook(target)
295
+ .map_err(|e| format!("MinHook queue_enable_hook failed for {fn_name}: {e}"))?;
296
+ orig_store.store(trampoline as u32, Ordering::Relaxed);
297
+ Ok(())
298
+ }
299
+
300
  /// Clean up the per-PID temp directory. Called during DLL detach or test cleanup.
301
  pub fn cleanup() {
302
  if let Some(tmp) = TEMP_DIR.get().and_then(|o| o.as_deref()) {
crates/openwa-test-runner/CLAUDE.md CHANGED
@@ -45,10 +45,10 @@ A headful test passes when: no crash, no `[PANIC]` in OpenWA.log, no `[GAMEPLAY
45
 
46
  ## Worms 2D Speedrun Suite
47
 
48
- ~598 replays in `testdata/replays/worms2d/` (`.wagame` extension). Must run with `-j 1` or `-j 2` β€” higher concurrency causes ~5-7% flakes from shared resource contention. Full suite takes ~3-4 min at `-j 1`. Use for final validation only.
49
 
50
  ```bash
51
- openwa-test -d testdata/replays/worms2d -j 1
52
  ```
53
 
54
  ## Key Paths
 
45
 
46
  ## Worms 2D Speedrun Suite
47
 
48
+ ~598 replays in `testdata/replays/worms2d/` (`.wagame` extension). Supports high concurrency (`-j 16` or higher). Full suite takes ~40s at `-j 16`. Use for final validation after complex changes.
49
 
50
  ```bash
51
+ openwa-test -d testdata/replays/worms2d -j 16
52
  ```
53
 
54
  ## Key Paths
crates/openwa-test-runner/src/main.rs CHANGED
@@ -427,12 +427,7 @@ fn run_tests_parallel(
427
  }
428
  drop(tx); // Drop our sender so rx closes when all workers finish
429
 
430
- // Small stagger between launches to reduce startup races on shared
431
- // resources (thm.prv, custom.dat) that can't be safely redirected.
432
  for (idx, test) in tests.into_iter().enumerate() {
433
- if idx > 0 {
434
- thread::sleep(Duration::from_millis(50));
435
- }
436
  let _ = work_tx.send((idx, test));
437
  }
438
  drop(work_tx); // Signal no more work
@@ -1597,9 +1592,6 @@ fn run_baseline_parallel(
1597
  drop(tx);
1598
 
1599
  for (idx, test) in tests.into_iter().enumerate() {
1600
- if idx > 0 {
1601
- thread::sleep(Duration::from_millis(50));
1602
- }
1603
  let _ = work_tx.send((idx, test));
1604
  }
1605
  drop(work_tx);
 
427
  }
428
  drop(tx); // Drop our sender so rx closes when all workers finish
429
 
 
 
430
  for (idx, test) in tests.into_iter().enumerate() {
 
 
 
431
  let _ = work_tx.send((idx, test));
432
  }
433
  drop(work_tx); // Signal no more work
 
1592
  drop(tx);
1593
 
1594
  for (idx, test) in tests.into_iter().enumerate() {
 
 
 
1595
  let _ = work_tx.send((idx, test));
1596
  }
1597
  drop(work_tx);