--- description: "Audit Vulkan queue/AS sync, ECS lock ordering, scheduler access declarations, RwLock patterns, deadlock potential" argument-hint: "--focus --depth shallow|deep" --- # Concurrency and Synchronization Audit Audit ByroRedux for data races, deadlocks, incorrect lock ordering, missing Vulkan synchronization, and thread-safety violations. **Architecture**: Orchestrator. Each dimension runs as a Task agent (max 3 concurrent). See `.claude/commands/_audit-common.md` for project layout, methodology, deduplication, context rules, finding format, and the path-reference convention. See `.claude/commands/_audit-severity.md` for the severity scale — the rows that matter here: | Condition | Minimum Severity | |-----------|-----------------| | Data race on Vulkan queue / use-after-free / AS built at wrong address | CRITICAL | | Vulkan spec violation (missing barrier, fence misuse) | HIGH | | Missing AS barrier (build → shader read) | HIGH | | Resource / descriptor / command-buffer leak per frame | HIGH | | Missing cleanup on swapchain recreate | HIGH | | ECS deadlock potential (RwLock ordering violation) | HIGH | | FFI lifetime violation across the cxx bridge | CRITICAL | Dimensions below are ordered by **concurrency blast radius**: GPU queue / AS data races (CRITICAL) first, then ECS deadlock surfaces, then the already-closed scheduler/access machinery (now regression guards), then the slower-moving lifecycle / worker / chain dimensions. ## Parameters (from $ARGUMENTS) - `--focus `: Comma-separated dimension numbers (e.g., `1,3`). Default: all 7. - `--depth shallow|deep`: `shallow` = check barrier/lock presence; `deep` = trace concurrent paths and timing windows. Default: `deep`. ## Extra Per-Finding Fields - **Dimension**: Vulkan Queue & AS Sync | Compute → AS → Fragment Chains | ECS Lock Ordering | Scheduler Access Declarations | RwLock Patterns (Resource↔Storage, Physics) | Resource Lifecycle | Worker Threads (Streaming, Debug) - **Trigger Conditions**: Exact timing/concurrency window needed to reproduce - **Verification Path**: For Vulkan-sync findings — whether the failure is observable in `cargo test`, the validation layer, or only RenderDoc (see "Speculative-fix guardrail" below) ## Speculative-fix guardrail (read before reporting any Vulkan-sync finding) Vulkan render-pass / pipeline-barrier / semaphore / fence bugs are largely **invisible to `cargo test`** — there is no headless device assertion that catches a missing image barrier or a wrong stage mask. Per the project's standing rule, do **not** propose shipping a barrier/stage/layout change on reasoning alone. Frame each such finding as **"needs validation-layer or RenderDoc confirmation"** and state the concrete signal that would confirm it (a specific `VUID-*` validation message, a RenderDoc resource-state mismatch, or a visible artifact class). A finding whose only evidence is "this barrier looks wrong" is a HYPOTHESIS row, not a fix. **The cheapest evidence channel is now sync-validation in release (#ec81f233).** `BYRO_VALIDATION=` (`instance.rs::validation_enabled`) turns on the Khronos validation layer + Synchronization Validation in a **release** build — debug builds are too slow to stream into the dense cells that fault. `BYRO_VALIDATION=gpuav` additionally enables GPU-Assisted Validation (shader OOB / descriptor checks). The debug messenger routes layer messages into the Rust log whenever validation is enabled. A sync-validation RAW/WAR hazard count (e.g. the ~40/frame the `--cornell` harness reported pre-#507945d8) is the confirmed-bug signal — prefer it over "looks wrong." Confirm any AS-build-input / barrier finding against a captured validation run before escalating past HYPOTHESIS. ## Phase 1: Setup 1. Parse `$ARGUMENTS` 2. `mkdir -p /tmp/audit/concurrency` 3. Fetch dedup baseline: `gh issue list --repo matiaszanolli/ByroRedux --limit 200 --json number,title,state,labels > /tmp/audit/concurrency/issues.json` ## Phase 2: Launch Dimension Agents ### Dimension 1: Vulkan Queue & Acceleration-Structure Sync (CRITICAL surface) **Entry points**: `crates/renderer/src/vulkan/context/draw.rs` (`draw_frame`), `crates/renderer/src/vulkan/sync.rs`, `crates/renderer/src/vulkan/acceleration/` (`blas_static.rs`, `blas_skinned.rs`, `tlas.rs`), `crates/renderer/src/vulkan/context/resize.rs` **Checklist**: - **Queue submission is single-Mutex.** `graphics_queue` and `present_queue` are both `Arc>` (`context/mod.rs`); `present_queue` is an `Arc::clone` of `graphics_queue`, so one Mutex serialises all submits + presents. `vk::Queue` is `Copy`, so the canonical pattern is "lock → copy the handle out → drop the guard → `queue_submit`": confirm the guard is **not** held across `queue_submit`/`queue_present` (a held guard during a blocking submit would serialise every other queue user; a copied-out handle used after a *second* thread re-submits is a data race). Cross-check the `.lock()` sites in `draw.rs` (main submit ~`queue_submit`, and the secondary one-time-command submit) against this rule. - **Frame-in-flight discipline.** The `in_flight[frame]` fence must be waited on before its command buffer / per-frame resources are reused; `image_available[frame]` semaphore must not be reused while a prior acquire is still pending (the draw.rs comment block around the acquire→submit window documents the ordering — verify it still holds). - **Acquire → render → present semaphore chain** is correct and uses per-image (not per-frame) signal semaphores where required by the swapchain image index. - **AS build → read barrier.** Every BLAS/TLAS build or refit that a later ray query reads must be followed by an `ACCELERATION_STRUCTURE_WRITE_KHR → ACCELERATION_STRUCTURE_READ_KHR` barrier before the fragment-stage ray-query consumer. Static BLAS: `blas_static.rs` (`memory_barrier`, WRITE→READ). Skinned BLAS refit: `blas_skinned.rs` (regression guard, #1790: `record_scratch_serialize_barrier`'s dst mask is WRITE|READ, not WRITE-only — it must cover a first-sight frame's same-command-buffer BUILD-then-UPDATE-refit adjacency, where the UPDATE reads `srcAccelerationStructure`). TLAS: `tlas.rs` (`cmd_pipeline_barrier`). A missing or wrong-stage barrier here is HIGH (CRITICAL if the AS is built at a wrong/stale device address — wrong geometry in shadows/reflections/GI). - **Deferred BLAS-scratch destruction (regression guard, #1782).** `blas_scratch_buffer` retirement on grow/shrink routes through `pending_destroy_scratch` (deferred) instead of an immediate free — same use-after-free class as #a476b256 below, but for the scratch allocation rather than a `BlasEntry`. `build_skinned_blas_batched_on_cmd`'s own grow-destroy is deliberately immediate (safe: runs after that frame's own fence wait) — don't flag it as a missed instance. - **AS build INPUT barrier access flag (regression guard, #507945d8).** Distinct from the build→read barrier above: the *inputs* to a build (instance-buffer copy → TLAS build in `tlas.rs`; skinned-vertex compute write → BLAS build in `draw.rs`) must be made visible with `SHADER_READ` at the `ACCELERATION_STRUCTURE_BUILD` stage, NOT `ACCELERATION_STRUCTURE_READ_KHR` (that flag reads an AS structure, not build inputs). The wrong flag is a copy/compute → build RAW hazard sync-validation catches — confirm via a `BYRO_VALIDATION` run, don't escalate on reasoning alone. - **Deferred AS destruction vs in-flight reads (#a476b256).** BLAS eviction/drop routes the AS handle + buffers through `pending_destroy_blas` (deferred countdown), so an unload/eviction can't free an AS the in-flight frame's ray queries still read. Verify no path re-introduces an immediate `destroy_acceleration_structure` at the eviction site (use-after-free = CRITICAL) and that shutdown drains the queue. - **Swapchain recreate sync.** `recreate_swapchain` (`context/resize.rs`) must cover all in-flight work with `device_wait_idle` (or equivalent fence drain) before destroying/rebuilding swapchain-dependent resources — no use-after-destroy across the recreate. - **One-time command buffers** (BLAS initial build, staging copies) block the main thread on a fence — flag if any such blocking submit runs inside the per-frame hot path rather than at load time. **Output**: `/tmp/audit/concurrency/dim_1.md` ### Dimension 2: Compute → AS → Fragment Chains **Entry points**: `crates/renderer/src/vulkan/skin_compute.rs`, `crates/renderer/src/vulkan/acceleration/blas_skinned.rs` (refit path), `crates/renderer/src/vulkan/svgf.rs`, `crates/renderer/src/vulkan/taa.rs`, `crates/renderer/src/vulkan/caustic.rs`, `crates/renderer/src/vulkan/water_caustic.rs`, `crates/renderer/src/vulkan/volumetrics.rs`, `crates/renderer/src/vulkan/bloom.rs`, `crates/renderer/src/vulkan/material.rs`, `crates/renderer/src/vulkan/context/draw.rs` (master ordering) **Checklist**: - **Skin chain (M29).** Palette build (`skin_compute.rs`) → `COMPUTE_WRITE→SHADER_READ` → per-mesh skin output → BLAS refit (`blas_skinned.rs`) reads it → fragment ray query hits the refit BLAS. The full palette→skin→refit→ray-query chain must be intact; a drift = stale geometry in shadows/reflections/GI. The live raster path uses inline skinning in `triangle.vert` (not the SSBO output), so a `VERTEX_INPUT` barrier is **not** currently required — flag only if a raster-from-skinned-SSBO path is added without one. - **Cross-frame ping-pong (no slot N reads slot N's in-flight write).** SVGF history, TAA history, caustic accumulator, water-caustic per-FIF `R32_UINT` accumulator, and volumetrics (`lighting_volumes` → `integrated_volumes`) all read the *previous* frame's slot. Verify the per-frame-in-flight indexing. - **Volumetrics gate (#1105).** Injection writes `lighting_volumes` (COMPUTE) → integration reads it, writes `integrated_volumes` (COMPUTE→COMPUTE) → `composite.frag` samples it (COMPUTE_WRITE→FRAGMENT_READ). `write_tlas` must run before `dispatch` each gated frame — `volumetrics.rs` keeps a `tlas_written: [bool; MAX_FRAMES_IN_FLIGHT]` latch that `dispatch` `debug_assert!`s and then resets. Verify the latch set/reset symmetry. - **Bloom within-frame RAW chain (#931).** Down-pyramid and up-pyramid each need a per-mip `COMPUTE_WRITE(SHADER_WRITE)→COMPUTE_READ(SHADER_READ)` image barrier so mip[i+1] sees mip[i]'s write; `up_mips[0]` must complete before composite samples it. Confirm the "post-barrier on the just-written mip only" accounting leaves no missing publish on the final up-mip. - **Caustic CLEAR → COMPUTE → FRAGMENT.** Accumulator cleared before compute writes; compute completes before composite reads. - **MaterialBuffer SSBO (R1).** `material.rs` upload is `HOST_WRITE → VERTEX/FRAGMENT_READ`; today it lands before draw recording so the frame fence already covers it — flag only if the upload moves into a compute path mid-frame. **Output**: `/tmp/audit/concurrency/dim_2.md` ### Dimension 3: ECS Lock Ordering & Deadlock **Entry points**: `crates/core/src/ecs/world.rs` (`query_2_mut`, `query_2_mut_mut`, the resource-pair queries), `crates/core/src/ecs/query.rs`, `crates/core/src/ecs/lock_tracker.rs`, all system functions under `byroredux/src/systems/` (`animation.rs`, `audio.rs`, `billboard.rs`, `bounds.rs`, `camera.rs`, `character.rs`, `debug.rs`, `light_anim.rs`, `metrics.rs`, `particle.rs`, `water.rs`, `weather.rs`) **Checklist**: - **TypeId-sorted acquisition is the deadlock-prevention invariant.** `world.rs` multi-component queries acquire storage locks in `TypeId`-ascending order regardless of the generic-parameter order the caller spells (the `if id_a < id_b { … } else { … }` branches in `query_2_mut`). The `lock_tracker` scope guards are set up in the *same* order so the lock-order graph never sees a spurious ABBA edge when the caller writes `` with `TypeId(A) < TypeId(B)` (#313). Verify any new multi-lock accessor follows this and that same-type access still hits the `assert_ne!` panic. - **`lock_tracker` coverage.** Same-thread re-entrant conflict detection is **always-on** in debug builds (`track_read`/`track_write` panic on a conflicting held lock). The cross-thread global lock-order graph is **opt-in via `BYRO_LOCK_ORDER_CHECK=1`** (`global_order` module) — so an ABBA risk between two *parallel* systems is only caught when that env var is set. Flag any CI/test path that should run with it but doesn't (see `docs/contributing.md` `lock-order-check` job). - **Guard lifetime in system bodies.** No `query_mut`/`resource_mut` guard held across a call that re-enters the same storage/resource; nested query patterns (e.g. animation querying Player then Transform) must drop the first guard or use the paired `query_N_mut` accessor; no `World::insert` (structural mutation, `&mut self`) during system execution (systems hold `&World`). - **Poisoning.** Storage `RwLock`s poison on panic; every acquisition resolves `PoisonError` through `storage_lock_poisoned::()` (re-panics with a diagnostic). Confirm no acquisition path silently `unwrap()`s a poisoned guard into torn state. **Output**: `/tmp/audit/concurrency/dim_3.md` ### Dimension 4: Scheduler Access Declarations (regression guard — M27 closed) **Entry points**: `crates/core/src/ecs/scheduler.rs`, `crates/core/src/ecs/access.rs`, `byroredux/src/commands/world_info.rs` (`sys.accesses`) **Status**: M27 (parallel dispatch) and R7 (access declarations) are **closed** (ROADMAP.md). The `parallel-scheduler` feature is **on by default**; the post-migration `sys.accesses` report is **0 unknown / 0 conflicts**. This dimension is therefore a **regression guard**, not migration tracking. **Checklist**: - **The conflict model is sound and matches the enum.** `AccessConflict` has exactly three variants — `None`, `Unknown { left_undeclared, right_undeclared }`, `Conflict { pairs }` (`access.rs`); there is **no `Parallel` variant**. `analyze_pair` returns `Unknown` whenever either side is undeclared (the *pessimistic* fallback, not "no conflict"), `Conflict` on a write/read or write/write overlap on the same component or resource, `None` otherwise. Verify any new variant or analyzer change preserves the "undeclared ⇒ Unknown ⇒ assume serialise" semantics. - **Migration KPIs.** `AccessReport::undeclared_parallel_count()` is the migration KPI (the population the analyzer can reason about); `undeclared_count()` = parallel + exclusive split via `undeclared_parallel_count()` + `undeclared_exclusive_count()` (#1237). `known_conflict_count()` and `unknown_pair_count()` must stay **0** on the engine binary — a regression here means a parallel-stage system was added without an `add_to_with_access` declaration (closures/bare-fns can't override `System::access`, so they need the registration-site channel; #1236). - **Exclusive phase.** Exclusive systems run serially after the parallel batch (`StageData.exclusive`) — they're listed in the report but never paired; the four runtime-mutually-exclusive systems re-staged in M27 Phase 3 (audio, spin, character-mode dispatcher, `player_controller_system`) must stay exclusive. Flag any move back to parallel. - **Re-entry & panic policy.** `Scheduler` is owned by `App`, never a `Resource` — re-entry from a system body is structurally impossible (#868). Panic-in-system is fail-fast by design (#1412); do not report "missing catch_unwind" as a bug. **Output**: `/tmp/audit/concurrency/dim_4.md` ### Dimension 5: RwLock Patterns — Resource↔Storage & Physics Step **Entry points**: `crates/physics/src/sync.rs` (`physics_sync_system`, `set_linear_velocity`, `set_kinematic_translation`), `crates/physics/src/world.rs` (`PhysicsWorld`), `crates/physics/src/components.rs` (`RapierHandles`), `crates/physics/src/config.rs` (`ContactConfig`), `byroredux/src/cell_loader/unload.rs` (`release_victim_rapier_bodies`), `byroredux/src/systems/character.rs` **Checklist**: - **TypeId-sorting does NOT cover the Resource↔Storage pair.** The deadlock-prevention sort in `query_N_mut` orders *storage* locks; a Resource lock (`resource_mut`) and a storage lock (`query`/`query_mut`) are an unordered pair. So no `resource_mut::()` guard may be held across a `query`/`query_mut` iteration and vice-versa. `physics_sync_system` is a 4-phase system (collect newcomers → register → step → pull dynamic). Verify Phase 1 `collect_newcomers` collects to a `Vec` under read guards and **drops them** before `register_newcomers` takes the `PhysicsWorld` + `RapierHandles` write guards. - **Helper lock order.** `set_linear_velocity` / `set_kinematic_translation` read `RapierHandles` via `world.query::()...copied()` (the read guard drops at the end of the `match`/`let` expression because the handle is `Copy`), *then* take `resource_mut::()`. Confirm the read guard is genuinely dropped before the write guard, and that callers (e.g. `character_controller_system`) don't already hold a `PhysicsWorld` guard when they call these. - **`ContactConfig`** is read via `try_resource` (optional) and snapshotted once per batch in `register_newcomers` — confirm it is not re-locked inside the per-newcomer loop. - **Cell-unload teardown (#1520).** `release_victim_rapier_bodies` (`unload.rs`) collects each victim's `RapierHandles` into a scratch `Vec` under the storage read guard, drops it, then removes bodies/colliders from `PhysicsWorld` — same release-reads-before-write discipline; verify it runs before the despawn loop drops the handles. - **Single-threaded placement.** `physics_sync_system` runs in `Stage::Physics` after transform propagation and must not be co-scheduled (parallel) with any other system that touches `PhysicsWorld` / `RapierHandles` / `Transform`. **Output**: `/tmp/audit/concurrency/dim_5.md` ### Dimension 6: Resource Lifecycle (GPU teardown ordering) **Entry points**: `crates/renderer/src/vulkan/context/mod.rs` (Drop impl), all `destroy()` methods, `crates/renderer/src/vulkan/buffer.rs`, `crates/renderer/src/vulkan/acceleration/`, `crates/renderer/src/vulkan/context/resize.rs`, `crates/renderer/src/vulkan/egui_pass.rs`, `crates/renderer/src/vulkan/scene_buffer/`, `crates/renderer/src/vulkan/material.rs` **Checklist**: - **Reverse-order destruction** (Vulkan requirement); Drop reached for all GPU resources; allocator freed last. Note #1483 hoisted allocator-independent destroys out of the allocator-guard scope in Drop — verify no resource that *needs* the allocator is destroyed after the guard is dropped. - **No use-after-destroy across swapchain recreate.** G-buffer / SVGF / TAA / caustic / water-caustic / volumetrics (`lighting_volumes` + `integrated_volumes`) / bloom (`down_mips` + `up_mips`) / composite resources rebuilt on recreate; per-FIF history/accumulator images freed for every in-flight slot; egui framebuffers rebuilt (`resize.rs`). - **AS cleanup on shutdown.** All `BlasEntry` buffers + `TlasState` buffers + scratch released; per-skinned-entity skin output buffers retained until the owning entity is destroyed. - **Other GPU SSBO/descriptor cleanup.** `scene_buffer` cleanup, `MaterialBuffer` SSBO (R1), texture registry, `EguiPass::destroy()` (releases egui-ash-renderer resources + framebuffers; `egui_pass: Option` taken/dropped in reverse order in `context/mod.rs` Drop). - **Per-frame leaks.** Any descriptor / command-buffer / staging allocation created per frame but not freed/reset is HIGH (compounds). **Output**: `/tmp/audit/concurrency/dim_6.md` ### Dimension 7: Worker Threads (Streaming, Debug Server) & Thread-Safety Bounds **Entry points**: `byroredux/src/streaming.rs` (M40 async pre-parse worker), `crates/debug-server/src/listener.rs` (per-client TCP threads), `crates/debug-server/src/system.rs` (`DebugDrainSystem`), `crates/debug-ui/src/lib.rs`, all types with `Send + Sync` bounds (`Component`, `Resource`), `crates/renderer/src/vulkan/allocator.rs` (`SharedAllocator`) **Checklist**: - **Streaming Drop ordering (#1167).** `WorldStreamingState::request_tx` is `Option>` and `worker` is `Option>`. The `Drop` impl must `take()` + drop the Sender BEFORE the worker handle is dropped (declaration-order field drop would otherwise detach the worker before the channel closes, defeating the join). `shutdown(&mut self, timeout)` takes the worker handle so the later `Drop` safety-net observes `worker: None` and short-circuits. Verify the field-drop / `take()` ordering and the join-with-timeout path. - **Worker ↔ main data flow.** Parsed cell payload moves to the main thread via channel — no shared `&mut World` from the worker. Off-thread NIF/texture extract goes through `Arc` whose inner `BsaArchive`/`Ba2Archive` serialise `File` access via Mutex (concurrent extracts safe). BGSM resolution (`&mut`, `MaterialProvider::merge_bgsm_into_mesh`) stays main-thread-only — confirm the worker doesn't touch it. NIF import cache (`Resource`) accessed from the worker must use a read-only fast path with write-back deferred to main. - **Debug server.** Per-client TCP threads do **not** touch the World directly — all mutations route through `DebugDrainSystem` on the main thread (Late-stage exclusive). The command queue between listener and main thread must be bounded (no unbounded buffering on a slow main loop). Screenshot readback completes on a fence wait — verify no race between the drain system and present. - **Allocator sharing.** `SharedAllocator = Arc>` (`allocator.rs`) is held by `VulkanContext` and cloned into `EguiPass` (`egui_pass.rs`), volumetrics, ssao, scene_buffer, etc. All dispatch runs single-threaded inside `draw_frame`; the only correctness concern is that no holder keeps the Mutex locked across a queue submit. The egui overlay dispatch runs after composite on the main loop. - **`Send + Sync` bounds.** Component/Resource storage reached only through World query/resource guards; no raw pointer shared across threads; cxx-bridge pointer lifetimes bounded; Ruffle/wgpu (UI) device is `Send` but not `Sync` — confirm it stays on one thread. - **Out of scope:** parse-time `Material` translation (`byroredux/src/material_translate.rs::translate_material`, `Material::resolve_pbr`) is single-threaded with no Mutex/RwLock/resource_mut — see `/audit-nifal` for that boundary. **Output**: `/tmp/audit/concurrency/dim_7.md` ## Phase 3: Merge 1. Read all `/tmp/audit/concurrency/dim_*.md` files 2. Combine into `docs/audits/AUDIT_CONCURRENCY_.md` 3. Remove cross-dimension duplicates Suggest: `/audit-publish docs/audits/AUDIT_CONCURRENCY_.md`