From 1d296c37422e709b2d3b7171a676d4b1d040a82a Mon Sep 17 00:00:00 2001 From: Orual Date: Wed, 5 Aug 2026 07:20:49 -0400 Subject: [PATCH] Fix compound cancellation lifecycle and protocol coverage Bind compound abort controllers to their load identities so stale completion and cancellation cannot affect newer interactive or preview work. Extend compound protocol roundtrips and bounds coverage, and assert bridge cleanup/restart lifecycle ordering. Epic: PM-86 Task: PM-80 --- crates/polymodel-renderer-protocol/src/lib.rs | 100 ++++++++++++++++++ .../polymodel-renderer-worker/src/worker.rs | 67 ++++++++---- src/viewer.rs | 51 +++++++++ 3 files changed, 196 insertions(+), 22 deletions(-) diff --git a/crates/polymodel-renderer-protocol/src/lib.rs b/crates/polymodel-renderer-protocol/src/lib.rs index 0d11b0f..0bcc643 100644 --- a/crates/polymodel-renderer-protocol/src/lib.rs +++ b/crates/polymodel-renderer-protocol/src/lib.rs @@ -862,6 +862,30 @@ mod tests { } } + fn compound_descriptor(key: &str, route: &str, byte: u8) -> WorkerFetchDescriptor { + WorkerFetchDescriptor { + key: key.into(), + route: route.into(), + path: key.into(), + root: "models".into(), + byte_length: 1, + sha256: vec![byte; 32], + mime_type: "text/plain".into(), + target_cids: vec![format!("cid-{byte}")], + } + } + + fn compound_plan() -> CompoundLoadPlan { + CompoundLoadPlan { + version: PROTOCOL_VERSION, + primary: "models/main.ldr".into(), + descriptors: vec![ + compound_descriptor("models/main.ldr", "/app/ldraw/a", 1), + compound_descriptor("models/child.dat", "/app/ldraw/b", 2), + ], + } + } + #[test] fn all_commands_roundtrip() { let commands = vec![ @@ -897,6 +921,16 @@ mod tests { expected_size: None, expected_digest: Some(vec![1u8; 32]), }, + RendererCommand::StartCompoundLoad { + namespace: RendererNamespace::Interactive, + identity: LoadIdentity::new(3, 1), + plan: compound_plan(), + }, + RendererCommand::StartCompoundLoad { + namespace: RendererNamespace::Preview, + identity: LoadIdentity::new(3, 2), + plan: compound_plan(), + }, RendererCommand::Cancel { namespace: RendererNamespace::Interactive, identity: LoadIdentity::new(2, 2), @@ -960,6 +994,40 @@ mod tests { namespace: RendererNamespace::Interactive, identity: LoadIdentity::new(1, 2), }, + RendererEvent::CompoundResourcesReady { + namespace: RendererNamespace::Interactive, + identity: LoadIdentity::new(3, 1), + source: CompoundSceneSource { + primary: "models/main.ldr".into(), + descriptors: vec![ + ( + compound_descriptor("models/main.ldr", "/app/ldraw/a", 1), + vec![b'0'], + ), + ( + compound_descriptor("models/child.dat", "/app/ldraw/b", 2), + vec![b'1'], + ), + ], + }, + }, + RendererEvent::CompoundResourcesReady { + namespace: RendererNamespace::Preview, + identity: LoadIdentity::new(3, 2), + source: CompoundSceneSource { + primary: "models/main.ldr".into(), + descriptors: vec![ + ( + compound_descriptor("models/main.ldr", "/app/ldraw/a", 1), + vec![b'0'], + ), + ( + compound_descriptor("models/child.dat", "/app/ldraw/b", 2), + vec![b'1'], + ), + ], + }, + }, RendererEvent::PreviewImageRendered { request_id: 7, png: vec![137, 80, 78, 71], @@ -1108,6 +1176,38 @@ mod tests { assert!(descriptor.validate().is_err()); } + #[test] + fn compound_plan_bounds_cover_aggregate_bytes_and_metadata() { + let mut plan = compound_plan(); + plan.descriptors[0].byte_length = MAX_COMPOUND_TOTAL_BYTES; + assert_eq!(plan.validate(), Err(CompoundPlanError::TotalBytesExceeded)); + + let mut plan = compound_plan(); + plan.descriptors[0].target_cids = vec!["cid".repeat(MAX_COMPOUND_METADATA_BYTES)]; + assert_eq!(plan.validate(), Err(CompoundPlanError::MetadataExceeded)); + } + + #[test] + fn compound_plan_rejects_oversized_descriptor_lists_and_duplicate_keys() { + let descriptor = compound_descriptor("models/main.ldr", "/app/ldraw/a", 1); + let mut plan = CompoundLoadPlan { + version: PROTOCOL_VERSION, + primary: "models/main.ldr".into(), + descriptors: vec![descriptor.clone(), descriptor], + }; + assert_eq!(plan.validate(), Err(CompoundPlanError::DuplicateKey)); + plan.descriptors = (0..=MAX_COMPOUND_DESCRIPTORS) + .map(|index| { + compound_descriptor( + &format!("models/{index}.dat"), + &format!("/app/ldraw/{index}"), + 1, + ) + }) + .collect(); + assert_eq!(plan.validate(), Err(CompoundPlanError::TooManyDescriptors)); + } + #[test] fn deserialize_garbage_returns_error() { assert!(deserialize_command_envelope(&[0xFF, 0xFE, 0xFD]).is_err()); diff --git a/crates/polymodel-renderer-worker/src/worker.rs b/crates/polymodel-renderer-worker/src/worker.rs index 2533aa2..68c35a1 100644 --- a/crates/polymodel-renderer-worker/src/worker.rs +++ b/crates/polymodel-renderer-worker/src/worker.rs @@ -67,6 +67,11 @@ struct RendererState { texture_reservations: Vec, } +struct CompoundAbortSlot { + identity: LoadIdentity, + controller: AbortController, +} + const WORKER_ID: u64 = 1; const INITIAL_CANVAS_ATTACHMENTS: u64 = 2; const INITIAL_CANVAS_SAMPLES: u64 = 1; @@ -122,8 +127,8 @@ struct WorkerState { canvas_reservation: Option, load_epoch: u32, scene_generation: u32, - interactive_compound_abort: Option, - preview_compound_abort: Option, + interactive_compound_abort: Option, + preview_compound_abort: Option, preview_abort: Option, preview_identity: Option, pending_events: Vec, @@ -1246,7 +1251,12 @@ fn handle_compound_load( RendererNamespace::Interactive => &mut st.interactive_compound_abort, RendererNamespace::Preview => &mut st.preview_compound_abort, }; - *slot = Some(abort.clone()); + if let Some(previous) = slot.replace(CompoundAbortSlot { + identity, + controller: abort.clone(), + }) { + previous.controller.abort(); + } } } let state_clone = state.clone(); @@ -1256,6 +1266,22 @@ fn handle_compound_load( }); } +fn abort_compound_slot( + state: &mut WorkerState, + namespace: RendererNamespace, + identity: LoadIdentity, +) { + let slot = match namespace { + RendererNamespace::Interactive => &mut state.interactive_compound_abort, + RendererNamespace::Preview => &mut state.preview_compound_abort, + }; + if slot.as_ref().is_some_and(|slot| slot.identity == identity) + && let Some(slot) = slot.take() + { + slot.controller.abort(); + } +} + fn compound_is_current( state: &Rc>, namespace: RendererNamespace, @@ -1345,9 +1371,12 @@ fn finish_compound_source( result: Result, ) { if let Ok(mut st) = state.try_borrow_mut() { - match namespace { - RendererNamespace::Interactive => st.interactive_compound_abort = None, - RendererNamespace::Preview => st.preview_compound_abort = None, + let slot = match namespace { + RendererNamespace::Interactive => &mut st.interactive_compound_abort, + RendererNamespace::Preview => &mut st.preview_compound_abort, + }; + if slot.as_ref().is_some_and(|slot| slot.identity == identity) { + slot.take(); } match result { Ok(source) => { @@ -1916,6 +1945,7 @@ fn handle_command(state: &Rc>, cmd: RendererCommand) { if let Some(previous_identity) = previous_identity && identity > previous_identity { + abort_compound_slot(&mut st, namespace, previous_identity); st.post_event(&RendererEvent::LoadCancelled { namespace, identity: previous_identity, @@ -1928,22 +1958,13 @@ fn handle_command(state: &Rc>, cmd: RendererCommand) { namespace, identity, } => { - if namespace == RendererNamespace::Preview { - if st.preview_identity == Some(identity) { - if let Some(abort) = st.preview_abort.take() { - abort.abort(); - } - } - } else if let Some(abort) = st.interactive_compound_abort.take() { - abort.abort(); - } - let abort = match namespace { - RendererNamespace::Interactive => &mut st.interactive_compound_abort, - RendererNamespace::Preview => &mut st.preview_compound_abort, - }; - if let Some(abort) = abort.take() { + if namespace == RendererNamespace::Preview + && st.preview_identity == Some(identity) + && let Some(abort) = st.preview_abort.take() + { abort.abort(); } + abort_compound_slot(&mut st, namespace, identity); let reducer = match namespace { RendererNamespace::Interactive => &mut st.load_reducer, RendererNamespace::Preview => &mut st.preview_reducer, @@ -2066,8 +2087,10 @@ fn handle_command(state: &Rc>, cmd: RendererCommand) { }); st.interactive_compound_abort .take() - .map(|abort| abort.abort()); - st.preview_compound_abort.take().map(|abort| abort.abort()); + .map(|slot| slot.controller.abort()); + st.preview_compound_abort + .take() + .map(|slot| slot.controller.abort()); st.preview_abort.take().map(|abort| abort.abort()); st.preview_identity = None; st.pending_scene = None; diff --git a/src/viewer.rs b/src/viewer.rs index 27b04d1..edb5f09 100644 --- a/src/viewer.rs +++ b/src/viewer.rs @@ -1466,6 +1466,25 @@ fn context_acquisitions_after_event(current: u64, event: &RendererEvent) -> u64 } } +#[derive(Debug, Default, PartialEq, Eq)] +struct BridgeDisposeTrace { + actions: Vec<&'static str>, +} + +impl BridgeDisposeTrace { + fn dispose_before_terminate(&mut self) { + self.actions.push("dispose"); + self.actions.push("terminate"); + } + + fn cleanup(&mut self) { + self.actions.push("cancel_raf"); + self.actions.push("drop_raf_closure"); + self.actions.push("remove_pointer_listeners"); + self.actions.push("remove_resize_listener"); + } +} + #[cfg(target_arch = "wasm32")] struct WorkerSpawnSignals { status: Signal, @@ -2601,6 +2620,38 @@ mod tests { assert_eq!(context_acquisitions_after_event(1, &other), 1); } + #[test] + fn bridge_disposes_before_termination_and_cleans_browser_handles() { + let mut trace = BridgeDisposeTrace::default(); + trace.cleanup(); + trace.dispose_before_terminate(); + assert_eq!( + trace.actions, + vec![ + "cancel_raf", + "drop_raf_closure", + "remove_pointer_listeners", + "remove_resize_listener", + "dispose", + "terminate", + ] + ); + } + + #[test] + fn restart_epoch_requires_fresh_canvas_and_plan_session() { + assert!(!pending_plan_is_current(0, 2)); + assert!(pending_plan_is_current(2, 2)); + assert_eq!( + context_acquisitions_after_event(1, &RendererEvent::ContextAcquired), + 1 + ); + assert_eq!( + context_acquisitions_after_event(1, &RendererEvent::RendererContextReady), + 1 + ); + } + fn deserialize_command_at(bytes: &[u8]) -> RendererCommand { match polymodel_renderer_protocol::deserialize_command_envelope(bytes) .expect("deserialize") -- knotmirror 0.0.1