indigocode/igasset-viewer-react

Fork 0

[H2] Data race on the loaded/error maps once exec runs off-thread #20

New issue

Closed

opened 2026-06-27 03:54:47 +00:00 by sessamekesh · 3 comments

sessamekesh commented

2026-06-27 03:54:47 +00:00

Owner

File: wasm/src/observable_filestore.cc

loading_state_lock_ only guards loading_igassets_ / loading_igpacks_. The worker-thread lambda passed to exec_task_list_->run(...) writes the following with no lock held:

loaded_igassets_ (line 86)
igasset_load_errors_ (line 74)
loaded_igpacks_ (line 133)
igpack_load_errors_ (line 129)

Meanwhile the main thread reads those same maps in generate_event() (lines 194-219) and in the load_* entry guards (lines 38, 96). Concurrent read/write on a std::map is undefined behavior.

Impact: real UB the moment exec_task_list_ actually runs on a worker thread. Currently masked only because the thread pool is never wired (see H3), so exec tasks happen to run on the main thread. The existing generate_event "concurrency is hard" comment acknowledges a visibility race but not this data race.

Fix: protect all shared maps under the mutex, or marshal the parsed result back to the main thread before storing (mirroring how the loading-state clear is already posted to main_thread_task_list_).

Filed by an automated AI code review (manual proof-of-concept run). Code locations reference the various-cleanup branch.

**File:** `wasm/src/observable_filestore.cc` `loading_state_lock_` only guards `loading_igassets_` / `loading_igpacks_`. The worker-thread lambda passed to `exec_task_list_->run(...)` writes the following with **no lock held**: - `loaded_igassets_` (line 86) - `igasset_load_errors_` (line 74) - `loaded_igpacks_` (line 133) - `igpack_load_errors_` (line 129) Meanwhile the main thread reads those same maps in `generate_event()` (lines 194-219) and in the `load_*` entry guards (lines 38, 96). Concurrent read/write on a `std::map` is undefined behavior. **Impact:** real UB the moment `exec_task_list_` actually runs on a worker thread. Currently masked only because the thread pool is never wired (see H3), so exec tasks happen to run on the main thread. The existing `generate_event` "concurrency is hard" comment acknowledges a *visibility* race but not this *data* race. **Fix:** protect all shared maps under the mutex, or marshal the parsed result back to the main thread before storing (mirroring how the loading-state clear is already posted to `main_thread_task_list_`). --- _Filed by an automated AI code review (manual proof-of-concept run). Code locations reference the `various-cleanup` branch._

sessamekesh added the

bug:critical

filed-by-ai

needs-review

labels

2026-06-27 03:54:47 +00:00

sessamekesh commented

2026-06-27 19:03:14 +00:00

Author

Owner

Maps should only be copied in critical sections to avoid paying the cost of iterating through them. But yes, this should be acknowledged...

sessamekesh added

ai-eligible

and removed

needs-review

labels

2026-06-27 19:03:23 +00:00

Ghost added the

ai-failed

label

2026-06-28 01:10:50 +00:00

sessamekesh removed the

ai-failed

label

2026-06-28 01:27:01 +00:00

Ghost added

ai-failed

and removed

ai-eligible

labels

2026-06-28 01:57:48 +00:00

sessamekesh commented

2026-06-28 03:35:39 +00:00

Author

Owner

I think a better approach for this would just be to make sure that the following two actions only ever run on the main thread:

Notify listeners (this should already be the case! I'm not actually sure what happens if a listener is invoked from the wrong thread)
Make changes to internal state.

Using the exec_task_list_ to perform the actual de-seraizliation + metadata listing is fine - but a .then_consuming(...) is what should perform the actual mutations. This should include mutating the "is_loading" state - that should be set immediately, then an exec_task_list_ should serialize the data and return it, so a main_thread_task_list_ then_consuming call can take the serialized data and set it on the main object.

I think a better approach for this would just be to make sure that the following two actions only ever run on the main thread: 1. Notify listeners (this should already be the case! I'm not actually sure what happens if a listener is invoked from the wrong thread) 2. Make changes to internal state. Using the `exec_task_list_` to perform the actual de-seraizliation + metadata listing is fine - but a `.then_consuming(...)` is what should perform the actual mutations. This should include mutating the "is_loading" state - that should be set immediately, then an `exec_task_list_` should serialize the data and return it, so a `main_thread_task_list_` `then_consuming` call can take the serialized data and set it on the main object.

sessamekesh added

ai-eligible

and removed

ai-failed

labels

2026-06-28 03:35:55 +00:00

Ghost added

ai-failed

and removed

ai-eligible

labels

2026-06-28 03:41:48 +00:00