From aa5fcef66ede63adafe42cf2691a1b889d5846e8 Mon Sep 17 00:00:00 2001 From: Mike Solar Date: Tue, 11 Aug 2026 01:30:18 +0800 Subject: [PATCH] fix(oakengine): facade bugs found by integration tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit undo: - NULL/empty label no longer crosses to oakundo as a dangling 0x1 pointer (push, group_begin/end) — fixed SIGSEGV - group_abort now undoes each executed child in reverse order task: - create_project_import addrefs the borrowed project handle instead of freeing it under the async task — fixed UAF/SIGSEGV timeline: - toggle_enabled/delete_clips guard NULL+0 slices — fixed SIGABRT - BlockSplitCommand halves placed correctly (oaktimeline undosplit) - PreservingLinks / ripple remove / ripple delete-gaps commands self-prepare on first redo — fixes silent no-op split/ripple - trim_clips_to targets the block containing the point, not the track - delete_empty_tracks applies the live track removal - ripple facades no longer free borrowed track handles still referenced by commands — fixed UAF node: - project_add_node releases the factory handle — fixes per-call leak - inputs_from(recursive=0) matches direct feeders (BFS off-by-one) - group passthrough id/resolve treat two-stage string length as success - node_connect(_command) reject duplicate connects with E_STATE - folder_add_child enforces one-folder-per-node - value_split_to_tracks splits vector/color per component - set_context_position/expanded establish the first entry - node_get_flags on an empty box returns 0, not u64::MAX - footage_borrow addrefs its wrapper — fixes double-free render: - renderer_create rejects invalid pixel formats (real range check) - render_frame forwards renderer width/height to the ticket tests: repro #[ignore]s removed, bug-behavior assertions corrected, it_undo global-stack tests serialized with a shared lock --- crates/oakengine/src/node.rs | 109 ++++++++++- crates/oakengine/src/render.rs | 17 +- crates/oakengine/src/task.rs | 36 +++- crates/oakengine/src/timeline.rs | 95 +++++++--- crates/oakengine/src/undo.rs | 51 ++++- crates/oakengine/tests/it_node.rs | 261 ++++++++++++-------------- crates/oakengine/tests/it_render.rs | 40 ++-- crates/oakengine/tests/it_task.rs | 44 ++--- crates/oakengine/tests/it_timeline.rs | 230 +++++++++++++---------- crates/oakengine/tests/it_undo.rs | 116 +++++++----- crates/oakengine/tests/undo.rs | 1 + crates/oaktimeline/src/undoripple.rs | 40 +++- crates/oaktimeline/src/undosplit.rs | 29 ++- 13 files changed, 664 insertions(+), 405 deletions(-) diff --git a/crates/oakengine/src/node.rs b/crates/oakengine/src/node.rs index 512b84941..9c5fa9895 100644 --- a/crates/oakengine/src/node.rs +++ b/crates/oakengine/src/node.rs @@ -911,6 +911,22 @@ pub unsafe extern "C" fn oakengine_folder_add_child( return Err(Error::Invalid); } let c = unbox(child)?; + // Mirror the module's live one-folder-per-node check (its UNDOABLE + // FolderAddChild command creator skips it): a node already in + // another folder is rejected with the module STATE error. + let parent = n::oaknode_folder_parent_of(c); + if !parent.ctx.is_null() { + let already_here = + n::oaknode_node_identity(parent) != 0 + && n::oaknode_node_identity(parent) == n::oaknode_node_identity(f); + // The borrowed parent handle's shell is released here. + if let Some(release) = parent.release { + unsafe { release(parent.ctx) }; + } + if !already_here { + return Err(Error::Module(oaknode::error::OAKNODE_E_STATE)); + } + } let cmd = n::oaknode_command_create_folder_add_child(f, c); if cmd.ctx.is_null() { return Err(Error::Failed("folder add child command failed".into())); @@ -1340,12 +1356,16 @@ pub unsafe extern "C" fn oakengine_node_category_at( /// `oakengine_node_get_flags`. #[no_mangle] pub unsafe extern "C" fn oakengine_node_get_flags(self_: *const OakEngineNode) -> u64 { - // Stub: the oaknode module has no per-node flags export. + // Stub: the oaknode module has no per-node flags export. NULL and + // empty (null-ctx) handle boxes both report 0 — the `guard_i64` + // error sentinel would otherwise surface as u64::MAX to C callers. crate::handle::guard_i64(|| unsafe { if self_.is_null() { return Ok(0); } - let _ = unbox(self_)?; + if (*self_).handle.is_null() { + return Ok(0); + } Ok(0) }) as u64 } @@ -2188,6 +2208,12 @@ pub unsafe extern "C" fn oakengine_project_add_node( continue; } if is_node_type(other, &type_id_str) { + // The AddNode command MOVED the node into the project graph, + // so the factory's owned handle is now just a stale view: + // release it (the node itself stays graph-owned) so the + // debug alive counter returns to baseline. + let mut owned = node; + n::oaknode_node_free(&mut owned); return Ok(box_handle::(other)); } } @@ -2262,6 +2288,20 @@ pub unsafe extern "C" fn oakengine_node_connect( } let out_h = unbox(output_node)?; let in_h = unbox(input_node)?; + // Mirror the module's live connect rejection: an already-connected + // input is a STATE error. The UNDOABLE creator validates existence + // and connectability but not "already connected" (its redo swallows + // the state error), so the facade pre-checks like the live variant. + let mut connected: c_int = 0; + Error::from_module(n::oaknode_node_input_is_connected( + in_h, + input_id, + &mut connected, + ))?; + if connected != 0 { + set_node_error("input is already connected"); + return Err(Error::Module(oaknode::error::OAKNODE_E_STATE)); + } let mut cmd: CHandle = CHandle::null(); let rc = n::oaknode_node_connect_undoable(out_h, in_h, input_id, &mut cmd); if rc != 0 { @@ -2325,6 +2365,16 @@ pub unsafe extern "C" fn oakengine_node_connect_command( let out_h = unbox(output_node)?; let in_h = unbox(input_node)?; let _ = element; + // Same duplicate-connect rejection as `oakengine_node_connect`. + let mut connected: c_int = 0; + Error::from_module(n::oaknode_node_input_is_connected( + in_h, + input_id, + &mut connected, + ))?; + if connected != 0 { + return Ok(std::ptr::null_mut()); + } let mut cmd: CHandle = CHandle::null(); let rc = n::oaknode_node_connect_undoable(out_h, in_h, input_id, &mut cmd); if rc != 0 { @@ -3588,6 +3638,16 @@ pub unsafe extern "C" fn oakengine_node_set_context_position( } let ch = unbox(context)?; let nh = unbox(node)?; + // The module's undoable setter requires a pre-existing + // context_positions entry (else NOT_FOUND); create the first entry + // with the live setter so positions can be ESTABLISHED through the + // facade (bug fix: they were previously impossible to create). + let mut x0: f64 = 0.0; + let mut y0: f64 = 0.0; + let mut e0: c_int = 0; + if n::oaknode_node_get_context_position(nh, ch, &mut x0, &mut y0, &mut e0) != 0 { + Error::from_module(n::oaknode_node_set_context_position(nh, ch, 0.0, 0.0, 0))?; + } let mut cmd: CHandle = CHandle::null(); let rc = n::oaknode_node_set_context_position_undoable(nh, ch, x, y, 0, &mut cmd); if rc != 0 { @@ -3642,7 +3702,9 @@ pub unsafe extern "C" fn oakengine_node_set_context_expanded( let mut was: c_int = 0; let rc = n::oaknode_node_get_context_position(nh, ch, &mut x, &mut y, &mut was); if rc != 0 { - return Err(Error::Module(rc)); + // No entry yet: establish one (the module's undoable setter + // below demands a pre-existing context_positions entry). + Error::from_module(n::oaknode_node_set_context_position(nh, ch, 0.0, 0.0, 0))?; } let mut cmd: CHandle = CHandle::null(); let rc = n::oaknode_node_set_context_position_undoable( @@ -3730,7 +3792,9 @@ pub unsafe extern "C" fn oakengine_node_group_get_inner( out_input.len() as c_int, &mut out_element, ); - if rc != 0 || out_node.is_null() { + // The module getter returns the copied string length (>= 0) on + // success; only negative codes are failures. + if rc < 0 || out_node.is_null() { return Ok(0); } // One level only: if the resolved node is the same, nothing moved. @@ -3882,7 +3946,9 @@ pub unsafe extern "C" fn oakengine_group_get_id_of_passthrough( out_input.len() as c_int, &mut out_element, ); - if rc != 0 || out_node.is_null() { + // The module getter returns the copied string length (>= 0) on + // success; only negative codes are failures. + if rc < 0 || out_node.is_null() { continue; } if n::oaknode_node_identity(out_node) == n::oaknode_node_identity(ih) @@ -3936,9 +4002,14 @@ pub unsafe extern "C" fn oakengine_group_get_passthrough_from_id( out_input_size, out_element, ); - if rc != 0 || node.is_null() { + // The module getter returns the copied string length (>= 0) on + // success; only negative codes are failures. + if rc < 0 { return Err(Error::Module(rc)); } + if node.is_null() { + continue; + } if !out_node.is_null() { *out_node = box_handle::(node); } @@ -4016,7 +4087,9 @@ pub unsafe extern "C" fn oakengine_group_resolve_input( out_input_size, out_element, ); - if rc != 0 { + // The module getter returns the copied string length (>= 0) on + // success; only negative codes are failures. + if rc < 0 { return Err(Error::Module(rc)); } if !out_node.is_null() { @@ -5589,7 +5662,9 @@ pub unsafe extern "C" fn oakengine_node_value_split_to_tracks( t.den = n.den; } value_type::COLOR | value_type::VEC2 | value_type::VEC3 | value_type::VEC4 => { - t.f = n.f; + // Per-component split: track `i` carries component `i` + // (combine_tracks reassembles from each track's f[0]). + t.f = [n.f[i], 0.0, 0.0, 0.0]; } value_type::FLOAT | value_type::BEZIER => { t.f[0] = n.f[0]; @@ -6013,7 +6088,10 @@ pub unsafe extern "C" fn oakengine_node_inputs_from( let sh = unbox(self_)?; let oh = unbox(other)?; // BFS over the module's output connections starting at `other` - // (inputs_from: is `other` reachable feeding into `self`?). + // (inputs_from: is `other` reachable feeding into `self`?). Every + // discovered neighbor is checked against the target, so a DIRECT + // feeder is found while expanding the depth-0 frontier; recursion + // merely widens the search beyond it. let target = n::oaknode_node_identity(sh); let mut frontier = vec![oh]; let mut visited: Vec = Vec::new(); @@ -6038,6 +6116,12 @@ pub unsafe extern "C" fn oakengine_node_inputs_from( if n::oaknode_node_output_connection_node_at(cur, i, &mut out) == 0 && !out.is_null() { + // Direct feeders are identified at discovery, before + // the depth counter advances (recursive == 0 must + // still inspect `other`'s own outputs). + if n::oaknode_node_identity(out) == target { + return Ok(1); + } next.push(out); } } @@ -6731,11 +6815,16 @@ pub unsafe extern "C" fn oakengine_footage_borrow( if node.is_null() { return Ok(std::ptr::null_mut()); } - let h = unbox(node)?; + let mut h = unbox(node)?; if !is_node_type(h, TYPE_ID_FOOTAGE) { set_footage_error("node is not a footage node"); return Ok(std::ptr::null_mut()); } + // The borrow takes its OWN reference (addref) so freeing both the + // borrow and the source node shell later is double-free-safe. + if let Some(addref) = h.addref { + unsafe { addref(h.ctx) }; + } Ok(box_handle::(h)) }) } diff --git a/crates/oakengine/src/render.rs b/crates/oakengine/src/render.rs index b02c9e163..7d079696c 100644 --- a/crates/oakengine/src/render.rs +++ b/crates/oakengine/src/render.rs @@ -193,12 +193,13 @@ pub unsafe extern "C" fn oakengine_renderer_create( if seq.is_null() || width <= 0 || height <= 0 || frame_rate_num <= 0 || frame_rate_den <= 0 { return Ok(std::ptr::null_mut()); } - // Validate the pixel format against the oakcommon format enum. - if crate::bridge::common::oakcommon_videoparams_get_format_name( - pixel_format, - std::ptr::null_mut(), - 0, - ) < 0 + // Validate the pixel format against the oakcore enum. The + // oakcommon format_name lookup succeeds for ANY code (unknowns + // format as "Unknown (0x…)"), so only the real formats (U8..F32) + // are accepted; Invalid (-1), the Count sentinel (5) and garbage + // codes are rejected. + if pixel_format < oakcore_rs::PixelFormat::U8 as c_int + || pixel_format > oakcore_rs::PixelFormat::F32 as c_int { return Ok(std::ptr::null_mut()); } @@ -275,8 +276,8 @@ pub unsafe extern "C" fn oakengine_renderer_render_frame( time_den: i64::from(b.frame_rate_num), color_manager: CHandle::null(), mode: b.mode, - force_width: 0, - force_height: 0, + force_width: b.width, + force_height: b.height, force_matrix: [0.0; 16], has_force_matrix: 0, force_format: -1, diff --git a/crates/oakengine/src/task.rs b/crates/oakengine/src/task.rs index 15108e8ec..0e140fc56 100644 --- a/crates/oakengine/src/task.rs +++ b/crates/oakengine/src/task.rs @@ -82,6 +82,11 @@ struct TaskMeta { /// The project a save task writes (addref'd at creation, released at /// free) — the module has no save-project getter. save_project: Option, + /// The project an import task borrows (addref'd at creation, released + /// at free): the module's import task stores its project handle WITHOUT + /// addref, so this facade-side ref keeps the shared box alive while the + /// task runs (see `oakengine_task_create_project_import`). + import_project: Option, /// The encoding-params box an export task owns, dropped at free /// (mirrors the C++ `FacadeExportTask` destructor; stored as `usize` so /// the map stays `Send`). @@ -97,6 +102,7 @@ impl TaskMeta { started: false, cancelled: false, save_project: None, + import_project: None, export_params: None, export_color_manager: None, } @@ -141,13 +147,16 @@ fn meta_set_cancelled(key: usize) { } /// Release every facade-side sidecar of a task (called by -/// [`oakengine_task_free`]): the addref'd save project, the owned +/// [`oakengine_task_free`]): the addref'd save/import projects, the owned /// encoding-params box and the derived color manager of an export task. fn drop_task_meta(key: usize) { if let Some(meta) = meta_lock().remove(&key) { if let Some(mut project) = meta.save_project { unsafe { n::oaknode_project_free(&mut project) }; } + if let Some(mut project) = meta.import_project { + unsafe { n::oaknode_project_free(&mut project) }; + } if let Some(ptr) = meta.export_params { unsafe { crate::codec::oakengine_encoding_params_destroy(ptr as *mut OakEngineEncodingParams) @@ -534,6 +543,13 @@ fn project_filename_of(project: CHandle) -> Result { /// `oakengine_task_import_file_count` documents 0 as "nothing to import, /// free instead of run". `url_count < 0`, a NULL URL inside the array, or a /// folder with no project yield NULL. +/// +/// The module's import task stores the project handle WITHOUT addref, so +/// the facade keeps an addref'd copy in [`TaskMeta::import_project`] +/// (released at free) — without it, releasing the transient borrowed +/// handle here would drop the shared box while the task still references +/// it, and the run would read freed memory (the former SIGSEGV reproduced +/// by `it_task::import_run_single_file`). #[no_mangle] pub unsafe extern "C" fn oakengine_task_create_project_import( folder: *mut OakEngineNode, @@ -551,10 +567,22 @@ pub unsafe extern "C" fn oakengine_task_create_project_import( return Ok(std::ptr::null_mut()); } let h = t::oaktask_create_project_import(fh, project, urls, url_count); - // Release the transient borrowed project handle (the import task - // keeps its own copy). + if h.is_null() { + // Creation failed: release the transient borrowed handle. + n::oaknode_project_free(&mut project); + return Ok(std::ptr::null_mut()); + } + // Keep the project borrowed for the task's lifetime (the module's + // import task stores the handle without addref): addref before the + // transient handle below is released, so the shared box stays alive + // until `oakengine_task_free` drops the meta-side copy. + let mut meta = TaskMeta::new(); + meta.import_project = Some(project.addref()); + // Release the transient borrowed project handle (the task's copy and + // the facade-side addref above keep the box alive). n::oaknode_project_free(&mut project); - Ok(box_task(h)) + meta_insert(h.ctx as usize, meta); + Ok(box_handle::(h)) }) } diff --git a/crates/oakengine/src/timeline.rs b/crates/oakengine/src/timeline.rs index 3f44438c0..4649e63b4 100644 --- a/crates/oakengine/src/timeline.rs +++ b/crates/oakengine/src/timeline.rs @@ -2158,7 +2158,11 @@ pub unsafe extern "C" fn oakengine_sequence_ripple_delete_clip( out_num as i64, out_den as i64, ); - release_handle(track); + // NOTE: `track` is intentionally NOT released — the module command + // stores the borrowed handle for its whole lifetime (its `redo`/ + // `undo` re-resolve the track), and the module model keeps such + // handles alive for the command's lifetime (same as + // `oakengine_sequence_delete_clips`). if cmd.is_null() { set_seq_error("ripple delete command failed"); return Err(Error::Failed("ripple delete command failed".into())); @@ -2450,7 +2454,14 @@ pub unsafe extern "C" fn oakengine_sequence_delete_clips( // (track, in, out) rationals of the deleted clips, for the default // ripple regions. let mut clip_ranges: Vec<(CHandle, i64, i64, i64, i64)> = Vec::new(); - let slice = std::slice::from_raw_parts(clips, clip_count.max(0) as usize); + // NULL with a zero count is a legal empty set; the slice must not be + // constructed from the NULL pointer (`slice::from_raw_parts(NULL, 0)` + // is UB), so it is only built for a positive count. + let slice: &[*mut OakEngineClip] = if clip_count > 0 { + std::slice::from_raw_parts(clips, clip_count as usize) + } else { + &[] + }; for (i, clip) in slice.iter().enumerate() { let c = match unbox(*clip) { Ok(h) => h, @@ -2610,7 +2621,9 @@ pub unsafe extern "C" fn oakengine_sequence_ripple_delete_range( out_num, out_den, ); - release_handle(track); + // NOTE: `track` is intentionally NOT released — the module + // command stores the borrowed handle for its whole lifetime (see + // `oakengine_sequence_ripple_delete_clip`). if cmd.is_null() { return Err(Error::Failed("ripple delete command failed".into())); } @@ -2634,7 +2647,14 @@ pub unsafe extern "C" fn oakengine_clip_toggle_enabled( return Err(Error::Invalid); } let mut children: Vec = Vec::new(); - let slice = std::slice::from_raw_parts(clips, count as usize); + // NULL with a zero count is a legal empty set; the slice must not be + // constructed from the NULL pointer (`slice::from_raw_parts(NULL, 0)` + // is UB), so it is only built for a positive count. + let slice: &[*mut OakEngineClip] = if count > 0 { + std::slice::from_raw_parts(clips, count as usize) + } else { + &[] + }; for (i, clip) in slice.iter().enumerate() { let c = match unbox(*clip) { Ok(h) => h, @@ -2843,7 +2863,9 @@ pub unsafe extern "C" fn oakengine_sequence_ripple_delete_in_to_out( out_num, out_den, ); - release_handle(track); + // NOTE: `track` is intentionally NOT released — the module + // command stores the borrowed handle for its whole lifetime + // (see `oakengine_sequence_ripple_delete_clip`). if cmd.is_null() { release_handle(wa); return Err(Error::Failed("ripple remove command failed".into())); @@ -2977,19 +2999,18 @@ pub unsafe extern "C" fn oakengine_sequence_trim_clips_to( release_handle(track); continue; } + // A trim (in or out) is only meaningful for the block that + // CONTAINS the point (in < point < out); the nearest-before + // queries can pick an insertion-order neighbor that ends before + // the point (which would trim to a negative length), so the + // strictly-containing lookup is used for both modes. let mut block = CHandle::null(); - let rc = if mode == MOVEMENT_MODE_TRIM_IN { - n::oaknode_track_get_nearest_block_before_or_at( - track, - point_num as c_int, - point_den as c_int, - &mut block, - ) - } else { - // The module exports no plain before query; iterate. - block = nearest_block_before(track, point_num, point_den); - if block.is_null() { -1 } else { 0 } - }; + let rc = n::oaknode_track_get_block_containing_time( + track, + point_num as c_int, + point_den as c_int, + &mut block, + ); if rc != 0 || block.is_null() { release_handle(track); continue; @@ -3007,18 +3028,6 @@ pub unsafe extern "C" fn oakengine_sequence_trim_clips_to( let mut out_den: c_int = 0; n::oaknode_block_get_in(block, &mut in_num, &mut in_den); n::oaknode_block_get_out(block, &mut out_num, &mut out_den); - let nearest_time = if mode == MOVEMENT_MODE_TRIM_IN { - (in_num as i64, in_den as i64) - } else { - (out_num as i64, out_den as i64) - }; - if rat_cmp(nearest_time.0, nearest_time.1, point_num, point_den) - == std::cmp::Ordering::Equal - { - release_handle(block); - release_handle(track); - continue; - } // new_length = length - |nearest_time - point|; the in-trim // anchors the out, the out-trim anchors the in (see // `oakengine_clip_trim`). @@ -3030,8 +3039,11 @@ pub unsafe extern "C" fn oakengine_sequence_trim_clips_to( let mut old_len_num: c_int = 0; let mut old_len_den: c_int = 0; Error::from_module(n::oaknode_block_get_length(block, &mut old_len_num, &mut old_len_den))?; + // Trim the addressed block itself (`trim_cmd` anchors on the + // block handle; passing the track used to silently reject the + // trim in the module). let cmd = trim_cmd( - track, + block, mode, old_len_num, old_len_den, @@ -3071,6 +3083,10 @@ pub unsafe extern "C" fn oakengine_sequence_delete_empty_tracks( return Err(Error::Invalid); } let mut children: Vec = Vec::new(); + // (track, owning list) pairs for the live removal compensation + // (the module's `TimelineRemoveTrackCommand` redo is a no-op for the + // list structure; see below). + let mut to_remove: Vec<(CHandle, CHandle)> = Vec::new(); let mut removed: c_int = 0; let mut all_count: c_int = 0; Error::from_module(n::oaknode_sequence_get_all_track_count(sequence, &mut all_count))?; @@ -3095,6 +3111,17 @@ pub unsafe extern "C" fn oakengine_sequence_delete_empty_tracks( continue; } let cmd = tl::oaktimeline_remove_track_command(track); + // Locate the owning list for the live removal (addref the track + // first so it survives the release below). + let mut ttype: c_int = 0; + if n::oaknode_track_get_type(track, &mut ttype) == 0 { + let mut list = CHandle::null(); + if n::oaknode_sequence_get_track_list(sequence, ttype, &mut list) == 0 + && !list.is_null() + { + to_remove.push((track.addref(), list)); + } + } release_handle(track); if cmd.is_null() { return Err(Error::Failed("remove track command failed".into())); @@ -3106,6 +3133,14 @@ pub unsafe extern "C" fn oakengine_sequence_delete_empty_tracks( return Ok(0); } push_multi_commands(&children, "Delete Empty Tracks")?; + // The module's TimelineRemoveTrackCommand redo is a no-op for the + // list structure (undogeneral.rs NOTE), so the removal is applied + // live as compensation (the same documented deviation as + // `oakengine_sequence_remove_track`). + for (track, list) in &to_remove { + n::oaknode_tracklist_remove_track(*list, *track); + release_handle(*list); + } Ok(removed) }) } diff --git a/crates/oakengine/src/undo.rs b/crates/oakengine/src/undo.rs index 6bd100472..4dce556ad 100644 --- a/crates/oakengine/src/undo.rs +++ b/crates/oakengine/src/undo.rs @@ -93,7 +93,15 @@ pub(crate) unsafe fn push_or_run(command_box: *mut OakEngineClipboard, name: *co return if rc == 0 { Ok(()) } else { Err(Error::Module(rc)) }; } let stack = *global_stack(); - let rc = unsafe { u::oakundo_undostack_push(stack, cmd, label.as_ptr() as *const c_char) }; + // The module treats a NULL name like an empty label, but an empty Rust + // String's `as_ptr()` is a DANGLING non-NULL pointer (0x1): the module's + // `read_name` would strlen it and SIGSEGV. Pass a real NULL instead. + let label_ptr = if label.is_empty() { + std::ptr::null() + } else { + label.as_ptr() as *const c_char + }; + let rc = unsafe { u::oakundo_undostack_push(stack, cmd, label_ptr) }; if rc == 0 { // Stack took a reference; release ours by freeing the box. unsafe { free_box(command_box) }; @@ -156,13 +164,19 @@ pub extern "C" fn oakengine_undo_group_end() -> c_int { let multi = open.multi; let name = open.name; drop(g); + // Same NULL-for-empty convention as `push_or_run`: the module's + // `read_name` treats NULL like an empty label, while an empty String's + // dangling `as_ptr()` (0x1) would be strlen'd -> SIGSEGV. + let name_ptr = if name.is_empty() { + std::ptr::null() + } else { + name.as_ptr() as *const c_char + }; // push_pre_executed discards an empty multi command. Either way // the stack took (or destroyed) the command; release our own // reference to the multi handle. let stack = *global_stack(); - let rc = unsafe { - u::oakundo_undostack_push_pre_executed(stack, multi, name.as_ptr() as *const c_char) - }; + let rc = unsafe { u::oakundo_undostack_push_pre_executed(stack, multi, name_ptr) }; let mut multi_handle = multi; unsafe { u::oakundo_command_free(&mut multi_handle) }; if rc == 0 { @@ -181,10 +195,37 @@ pub extern "C" fn oakengine_undo_group_abort() -> c_int { let mut g = group_lock(); let open = g.take().ok_or(Error::State)?; drop(g); - let rc = unsafe { u::oakundo_command_undo_now(open.multi) }; + // The multi command itself is never marked done (each child was + // redo'd eagerly at push time), so `undo_now` on it is a no-op. + // Undo the executed children individually instead, in reverse + // insertion order (mirroring the multi's reverse-order undo), each + // through its own borrowed handle. + let mut count: c_int = 0; + let rc = unsafe { u::oakundo_command_multi_child_count(open.multi, &mut count) }; if rc != 0 { + let mut multi = open.multi; + unsafe { u::oakundo_command_free(&mut multi) }; return Err(Error::Module(rc)); } + for i in (0..count).rev() { + let mut child = CHandle::null(); + let rc = unsafe { u::oakundo_command_multi_child(open.multi, i, &mut child) }; + if rc != 0 { + let mut multi = open.multi; + unsafe { u::oakundo_command_free(&mut multi) }; + return Err(Error::Module(rc)); + } + let rc = unsafe { u::oakundo_command_undo_now(child) }; + // The child handle is borrowed (owns:false): release only its + // shell — the child value lives on in the multi until the multi + // itself is freed below. + unsafe { u::oakundo_command_free(&mut child) }; + if rc != 0 { + let mut multi = open.multi; + unsafe { u::oakundo_command_free(&mut multi) }; + return Err(Error::Module(rc)); + } + } let mut multi = open.multi; unsafe { u::oakundo_command_free(&mut multi) }; Ok(()) diff --git a/crates/oakengine/tests/it_node.rs b/crates/oakengine/tests/it_node.rs index 15abcd84a..48669dc9b 100644 --- a/crates/oakengine/tests/it_node.rs +++ b/crates/oakengine/tests/it_node.rs @@ -37,9 +37,9 @@ //! objects are the only ALIVE sources). Tests that only touch borrowed //! handles or static helpers run in parallel. //! -//! The node family is 327 exports. `oakengine_node_inputs_from` and the -//! context-position setters have module-behavior divergences that are -//! documented inline and repeated in the module docs below the tests. +//! The node family is 327 exports. The facade divergences found while +//! exercising the family (documented in the block below the tests) are all +//! fixed in src/node.rs and asserted as correct behavior here. #[path = "common/mod.rs"] mod common; @@ -274,15 +274,13 @@ fn static_ids_and_pure_helpers() { let mut tracks = [unsafe { std::mem::zeroed::() }; 2]; assert_eq!(unsafe { oakengine_node_value_split_to_tracks(vt::VEC2, &vec2_value(1.0, 2.0), tracks.as_mut_ptr(), 2) }, 0); assert_eq!(tracks[0].kind, vt::VEC2); - // The facade's split copies the WHOLE value into every track for - // vector types (no per-component split); combine then picks each - // track's f[0]. Documented divergence — asserted as actual behavior. + // Track `i` carries component `i`; combine reassembles them. assert!((tracks[0].f[0] - 1.0).abs() < 1e-9); - assert!((tracks[1].f[0] - 1.0).abs() < 1e-9); + assert!((tracks[1].f[0] - 2.0).abs() < 1e-9); let mut out = unsafe { std::mem::zeroed::() }; assert_eq!(unsafe { oakengine_node_value_combine_tracks(vt::VEC2, tracks.as_ptr(), 2, &mut out) }, 0); assert!((out.f[0] - 1.0).abs() < 1e-9); - assert!((out.f[1] - 1.0).abs() < 1e-9); + assert!((out.f[1] - 2.0).abs() < 1e-9); // split/combine of a float keeps a single track; track_count mismatch is // clamped by split and illegal (<= 0) for both. @@ -646,10 +644,10 @@ fn node_family_legal_paths() { // ---- string parameter access (text generator) ------------------- let textgen = unsafe { oakengine_node_factory_create_from_id(TYPE_TEXT.as_ptr()) }; assert!(!textgen.is_null()); - // project + orphan + textgen owned, plus one leaked owned handle per - // project_add_node above (see `project_add_node_owned_handle_leak`). + // project + orphan + textgen owned (the added-node views are + // borrowed; the facade releases the factory's owned handle). let alive_now = alive(); - assert_eq!(alive_now, base + 9, "textgen: alive_now={alive_now} base={base}"); + assert_eq!(alive_now, base + 3, "textgen: alive_now={alive_now} base={base}"); // String-carried types report as STRING in the POD enum (Text has // no dedicated code; the module maps Text/StrCombo to STRING). assert_eq!(unsafe { oakengine_node_input_get_type(textgen, c"text_in".as_ptr()) }, vt::STRING); @@ -668,8 +666,8 @@ fn node_family_legal_paths() { // Unknown input → NOT_FOUND for the string getter. assert_eq!(unsafe { oakengine_node_get_input_string(value, c"nope_in".as_ptr(), buf.as_mut_ptr(), 512) }, NODE_E_NOT_FOUND); unsafe { oakengine_node_free(textgen) }; - // project + orphan owned, plus the 6 leaked add_node handles. - assert_eq!(alive(), base + 8); + // project + orphan owned. + assert_eq!(alive(), base + 2); // ---- at-time values --------------------------------------------- assert_eq!(unsafe { oakengine_node_frame_time_base(value, std::ptr::null_mut(), std::ptr::null_mut()) }, 0); @@ -732,11 +730,10 @@ fn node_family_legal_paths() { assert_eq!(unsafe { oakengine_node_set_value_hint(value, c"value_in".as_ptr(), 0, 0, 0, c"".as_ptr()) }, 0); assert_eq!(unsafe { oakengine_node_set_value_hint(value, c"nope_in".as_ptr(), 0, 0, 0, c"".as_ptr()) }, NODE_E_NOT_FOUND); - // ---- context positions (module requires a pre-existing entry) --- - // The facade's only setter is the undoable variant, and the module's - // undoable setter demands an existing context_positions entry, so a - // first position can never be established through the C ABI. See the - // module docs below; asserted as documented behavior. + // ---- context positions ------------------------------------------- + // The facade establishes the first context_positions entry with the + // module's live setter before pushing the undoable command, so + // positions work on fresh nodes. let root = unsafe { oakengine_project_root(project) }; let mut x: f64 = 0.0; let mut y: f64 = 0.0; @@ -744,9 +741,16 @@ fn node_family_legal_paths() { assert_eq!(unsafe { oakengine_node_context_node_count(root) }, 0); assert_eq!(unsafe { oakengine_node_context_contains_node(root, value) }, 0); assert!(unsafe { oakengine_node_context_node_at(root, 0, &mut x, &mut y, &mut expanded) }.is_null()); - assert_eq!(unsafe { oakengine_node_set_context_position(root, value, 10.0, 20.0) }, NODE_E_NOT_FOUND); - assert_eq!(unsafe { oakengine_node_get_context_position(root, value, &mut x, &mut y, &mut expanded) }, NODE_E_NOT_FOUND); - assert_eq!(unsafe { oakengine_node_set_context_expanded(root, value, 1) }, NODE_E_NOT_FOUND); + // A fresh node gains its first context entry through the facade. + assert_eq!(unsafe { oakengine_node_set_context_position(root, value, 10.0, 20.0) }, 0); + assert_eq!(unsafe { oakengine_node_get_context_position(root, value, &mut x, &mut y, &mut expanded) }, 0); + assert!((x - 10.0).abs() < 1e-9, "x={x}"); + assert!((y - 20.0).abs() < 1e-9, "y={y}"); + assert_eq!(expanded, 0); + // Expanded flag flips through the same path. + assert_eq!(unsafe { oakengine_node_set_context_expanded(root, value, 1) }, 0); + assert_eq!(unsafe { oakengine_node_get_context_position(root, value, &mut x, &mut y, &mut expanded) }, 0); + assert_eq!(expanded, 1); unsafe { oakengine_node_free(root) }; // ---- array inputs (multicam sources_in is an array) -------------- @@ -766,13 +770,9 @@ fn node_family_legal_paths() { // ---- graph editing: connect / disconnect ------------------------- assert_eq!(unsafe { oakengine_node_connect(solid, transform, c"tex_in".as_ptr()) }, 0); assert_eq!(unsafe { oakengine_node_input_is_connected(transform, c"tex_in".as_ptr()) }, 1); - // A second connect on the same input is NOT rejected: the facade - // delegates to the module's UNDOABLE connect creator, which skips - // the live "already connected" check (its redo swallows the state - // error). The call returns 0 and the edge is unchanged - documented - // divergence (module docs; the live `oaknode_node_connect` would - // return NODE_E_STATE). - assert_eq!(unsafe { oakengine_node_connect(solid, transform, c"tex_in".as_ptr()) }, 0); + // A second connect on an already-connected input is rejected with the + // module STATE error, mirroring the live `oaknode_node_connect`. + assert_eq!(unsafe { oakengine_node_connect(solid, transform, c"tex_in".as_ptr()) }, NODE_E_STATE); // Connecting to a non-connectable input → module INVALID. assert_eq!(unsafe { oakengine_node_connect(solid, value, c"value_in".as_ptr()) }, NODE_E_INVALID); // Connecting to an unknown input → module NOT_FOUND. @@ -801,11 +801,10 @@ fn node_family_legal_paths() { // Out-of-range output index. assert_eq!(unsafe { oakengine_node_output_connection_at(solid, 1, &mut conn_node, buf.as_mut_ptr(), 512, &mut elem) }, E_NOT_FOUND); - // inputs_from: recursive reaches a direct feeder... + // inputs_from: recursive and non-recursive both reach a direct + // feeder (the BFS checks neighbors on the depth-0 expansion). assert_eq!(unsafe { oakengine_node_inputs_from(transform, solid, 1) }, 1); - // ...but the non-recursive variant has an off-by-one BFS and never - // checks the direct feeders — documented divergence (module docs). - assert_eq!(unsafe { oakengine_node_inputs_from(transform, solid, 0) }, 0); + assert_eq!(unsafe { oakengine_node_inputs_from(transform, solid, 0) }, 1, "non-recursive still finds a direct feeder"); assert_eq!(unsafe { oakengine_node_inputs_from(value, solid, 1) }, 0); assert_eq!(unsafe { oakengine_node_inputs_from(std::ptr::null(), solid, 1) }, 0); assert_eq!(unsafe { oakengine_node_inputs_from(transform, std::ptr::null(), 1) }, 0); @@ -848,10 +847,10 @@ fn node_family_legal_paths() { let count_before = unsafe { oakengine_project_node_count(project) }; let copied = unsafe { oakengine_node_copy_in_graph(value, std::ptr::null_mut()) }; assert!(!copied.is_null()); - assert_eq!(alive(), base + 9, "copy-in-graph: owned copy + project + orphan + 6 leaked add_node handles"); + assert_eq!(alive(), base + 3, "copy-in-graph: owned copy + project + orphan"); assert_eq!(unsafe { oakengine_project_node_count(project) }, count_before + 1, "the redo inserts a copy into the graph"); unsafe { oakengine_node_free(copied) }; - assert_eq!(alive(), base + 8); + assert_eq!(alive(), base + 2); // add_to_project_command: opaque AddNode command for an orphan. let orphan2 = unsafe { oakengine_node_factory_create_from_id(TYPE_VALUE.as_ptr()) }; @@ -1039,22 +1038,22 @@ fn node_family_legal_paths() { unsafe { oakengine_node_free(pt_node) }; // Out-of-range passthrough index → module NOT_FOUND. assert_eq!(unsafe { oakengine_group_input_passthrough_at(group, 5, buf.as_mut_ptr(), 512, &mut pt_node, buf.as_mut_ptr(), 512, &mut pt_elem) }, NODE_E_NOT_FOUND); - // id_of_passthrough round trip. BUG (reported): the facade treats - // the module's two-stage string length (9 for "value_in") as an - // error code, so the search skips every non-empty-input - // passthrough and reports NOT_FOUND even when the passthrough is - // present. Asserted as actual behavior. + // id_of_passthrough round trip: the module getters return the copied + // string length (>= 0) on success, and the facade treats only + // negative codes as failures. let len = unsafe { oakengine_group_get_id_of_passthrough(group, value, c"value_in".as_ptr(), -1, buf.as_mut_ptr(), 512) }; - assert_eq!(len, E_NOT_FOUND, "facade bug: two-stage length misread as error"); + assert!(len > 0, "the passthrough id must be returned"); + assert_eq!(unsafe { read_buf(&mut buf) }, passthrough_id, "the id matches the generated one"); assert_eq!(unsafe { oakengine_group_get_id_of_passthrough(group, value, c"nope_in".as_ptr(), -1, buf.as_mut_ptr(), 512) }, E_NOT_FOUND); - // get_passthrough_from_id: same facade bug — the module length (9) - // leaks through as the return code and the output node is never - // written. Asserted as actual behavior. + // get_passthrough_from_id writes the inner node, input and element. let passthrough_id_c = CString::new(passthrough_id.as_str()).unwrap(); let mut back_node: *mut OakEngineNode = std::ptr::null_mut(); let rc = unsafe { oakengine_group_get_passthrough_from_id(group, passthrough_id_c.as_ptr(), &mut back_node, buf.as_mut_ptr(), 512, &mut pt_elem) }; - assert_eq!(rc, 9, "facade bug: passthrough_input_at length leaks through as a module code"); - assert!(back_node.is_null(), "facade bug: out_node is never written"); + assert_eq!(rc, 0); + assert!(!back_node.is_null(), "out_node must be written"); + assert_eq!(unsafe { read_buf(&mut buf) }, "value_in"); + assert_eq!(pt_elem, -1); + unsafe { oakengine_node_free(back_node) }; assert_eq!(unsafe { oakengine_group_get_passthrough_from_id(group, c"no-such-id".as_ptr(), &mut back_node, buf.as_mut_ptr(), 512, &mut pt_elem) }, E_NOT_FOUND); // Output passthrough set/get round trip. assert!(unsafe { oakengine_group_get_output_passthrough(group) }.is_null()); @@ -1062,14 +1061,14 @@ fn node_family_legal_paths() { let op = unsafe { oakengine_group_get_output_passthrough(group) }; assert!(!op.is_null()); unsafe { oakengine_node_free(op) }; - // resolve_input: same facade bug as get_id_of_passthrough — the - // module's two-stage length (9) is misread as an error, so the call - // returns 9 and the resolved node is never written. Asserted as - // actual behavior. + // resolve_input resolves the passthrough to its inner node/input. let mut rn: *mut OakEngineNode = std::ptr::null_mut(); let rc = unsafe { oakengine_group_resolve_input(group, c"value_in".as_ptr(), -1, &mut rn, buf.as_mut_ptr(), 512, &mut pt_elem) }; - assert_eq!(rc, 9, "facade bug: resolve_input length leaks through as a module code"); - assert!(rn.is_null(), "facade bug: resolved node is never written"); + assert_eq!(rc, 0); + assert!(!rn.is_null(), "the resolved node must be written"); + assert_eq!(unsafe { read_buf(&mut buf) }, "value_in"); + assert_eq!(pt_elem, -1); + unsafe { oakengine_node_free(rn) }; // After removal the input no longer resolves. assert_eq!(unsafe { oakengine_group_remove_input_passthrough(group, value, c"value_in".as_ptr(), -1) }, 0); assert_eq!(unsafe { oakengine_group_input_passthrough_count(group) }, 0); @@ -1085,12 +1084,17 @@ fn node_family_legal_paths() { let cmd = unsafe { oakengine_group_set_output_passthrough_command(group, value) }; assert!(!cmd.is_null()); unsafe { oakengine_undo_command_free(cmd) }; - // group_get_inner walks one passthrough level. + // group_get_inner walks one passthrough level (the input id is + // pre-filled in the inout buffer). let mut inner_node: *mut OakEngineNode = unsafe { group_inner_slot(group) }; let mut in_buf = [0 as c_char; 256]; + let v_in = b"value_in"; + unsafe { std::ptr::copy_nonoverlapping(v_in.as_ptr() as *const c_char, in_buf.as_mut_ptr(), v_in.len()) }; let mut in_elem: c_int = -1; let moved = unsafe { oakengine_node_group_get_inner(&mut inner_node, in_buf.as_mut_ptr(), 256, &mut in_elem) }; - assert_eq!(moved, 0, "facade bug: the resolve_input length check blocks the passthrough walk"); + assert_eq!(moved, 1, "one passthrough level is walked"); + assert_eq!(unsafe { read_buf(&mut in_buf) }, "value_in"); + assert_eq!(in_elem, -1); unsafe { oakengine_node_free(inner_node) }; // A bare group without passthroughs resolves to itself → 0. let bare = unsafe { oakengine_project_add_node(project, TYPE_GROUP.as_ptr()) }; @@ -1153,9 +1157,11 @@ fn node_family_legal_paths() { assert_eq!(unsafe { oakengine_node_get_effect_input(value, buf.as_mut_ptr(), 512, &mut oty) }, E_NOT_FOUND); // ---- bulk delete ------------------------------------------------- - // Re-connect solid → transform, then delete the edge and a node in - // one multi command. - assert_eq!(unsafe { oakengine_node_connect(solid, transform, c"tex_in".as_ptr()) }, 0); + // The solid → transform edge from the connect section is still live, + // so a redundant reconnect is rejected with the module STATE error; + // the edge is then deleted together with the node in one multi + // command. + assert_eq!(unsafe { oakengine_node_connect(solid, transform, c"tex_in".as_ptr()) }, NODE_E_STATE, "the edge from the connect section is still live"); let del_nodes = [transform]; let edge_outputs = [solid]; let edge_inputs = [transform]; @@ -1221,11 +1227,10 @@ fn node_family_legal_paths() { assert_eq!(unsafe { oakengine_folder_add_child(folder, value) }, 0); assert_eq!(unsafe { oakengine_folder_item_child_count(folder) }, 1); assert_eq!(unsafe { oakengine_folder_item_child_count(root2) }, 1, "moved out of the root"); - // Adding to a second folder is NOT rejected through the facade: it - // delegates to the module's UNDOABLE FolderAddChild command, which - // skips the live one-folder-per-node check. Returns 0 and the node - // ends up in both folders - documented divergence. - assert_eq!(unsafe { oakengine_folder_add_child(root2, value) }, 0); + // A node already in one folder cannot be added to a second one: the + // facade mirrors the module's live one-folder-per-node check (its + // UNDOABLE command creator skips it) and rejects with STATE. + assert_eq!(unsafe { oakengine_folder_add_child(root2, value) }, NODE_E_STATE); // remove_element_command is a documented stub → NULL. assert!(unsafe { oakengine_folder_remove_element_command(root2, value) }.is_null()); // move_children: move the value node back into the root. @@ -1409,16 +1414,14 @@ fn node_family_legal_paths() { let footage_node = unsafe { oakengine_project_node_at(project, footage_idx) }; assert_eq!(unsafe { oakengine_node_is_footage(footage_node) }, 1); assert_eq!(unsafe { oakengine_footage_is_valid(footage_node) }, 0, "module footage is never probed"); - // `oakengine_footage_borrow` wraps the node's own handle WITHOUT an - // addref, so the borrow and the node share one reference: freeing - // both would double-free. The borrow is released (it owns the - // shared reference); the node shell is intentionally leaked. + // `oakengine_footage_borrow` takes its OWN addref'd reference, so the + // borrow and the source node shell can BOTH be freed (no double-free). let borrowed = unsafe { oakengine_footage_borrow(footage_node) }; assert!(!borrowed.is_null()); let len = unsafe { oakengine_footage_get_filename(borrowed, buf.as_mut_ptr(), 512) }; assert!(len > 0); unsafe { oakengine_footage_free(borrowed) }; - // NB: `footage_node` shell not freed (shares the borrow's reference). + unsafe { oakengine_node_free(footage_node) }; // Borrow of a non-footage node → NULL. assert!(unsafe { oakengine_footage_borrow(value) }.is_null()); assert_eq!(unsafe { oakengine_footage_is_valid(value) }, 0); @@ -1458,12 +1461,11 @@ fn node_family_legal_paths() { unsafe { oakengine_node_free(orphan) }; unsafe { oakengine_footage_free(imported) }; unsafe { oakengine_project_free(project) }; - // Two intentional process-lifetime leaks remain: the 7 - // project_add_node owned handles (see - // `project_add_node_owned_handle_leak`) and the hidden probe + // One intentional process-lifetime leak remains: the hidden probe // project created by the first `oakengine_footage_probe` (leaked - // like the C++ EngineCore shell). - assert_eq!(alive(), base + 8, "7 add_node handles + the probe project (both reported leaks)"); + // like the C++ EngineCore shell). The add_node factory handles are + // released by the facade, so they no longer leak. + assert_eq!(alive(), base + 1, "only the probe project leak remains"); }); } @@ -1544,9 +1546,9 @@ fn null_and_empty_handle_failure_paths() { assert!(unsafe { oakengine_node_factory_create_from_id(c"x".as_ptr()) }.is_null()); assert_eq!(unsafe { oakengine_node_category_count(node) }, E_INVALID, "empty handle, not a NULL pointer"); assert_eq!(unsafe { oakengine_node_category_at(node, 0) }, -1); - // Empty handle → the guard_i64 sentinel (-1 as u64); a NULL - // pointer would return 0. - assert_eq!(unsafe { oakengine_node_get_flags(node) }, (-1i64) as u64); + // Empty handle and NULL both report 0 flags (no error sentinel leaks + // through as u64::MAX). + assert_eq!(unsafe { oakengine_node_get_flags(node) }, 0); assert_eq!(unsafe { oakengine_node_get_sub_category(node, buf.as_mut_ptr(), 512) }, E_INVALID); assert_eq!(unsafe { oakengine_node_get_description(node, buf.as_mut_ptr(), 512) }, E_INVALID); assert!(unsafe { oakengine_node_create_copy(node) }.is_null()); @@ -1885,12 +1887,10 @@ fn destroy_contracts_and_alive_count() { }); } -/// Minimal repro of the `oakengine_project_add_node` owned-handle leak: the -/// facade creates the node with `oaknode_factory_create_from_id` (owned, -/// alive-counted) and pushes the AddNode command that MOVES the node into -/// the project graph, but never releases the factory handle. The debug -/// alive counter therefore grows by one per `project_add_node` call and -/// never returns to baseline — even after the project is freed. +/// `oakengine_project_add_node` releases the factory's owned handle after +/// the AddNode command moves the node into the project graph: the debug +/// alive counter returns to baseline once the project (and the borrowed +/// node view) is freed. #[test] fn project_add_node_owned_handle_leak() { with_owned(|| { @@ -1903,85 +1903,68 @@ fn project_add_node_owned_handle_leak() { let node = unsafe { oakengine_project_add_node(project, TYPE_VALUE.as_ptr()) }; assert!(!node.is_null()); - assert_eq!(alive(), base + 2, "project + one owned factory handle"); + // The added-node view is borrowed; the factory's owned handle was + // released by the facade, so only the project is alive-counted. + assert_eq!(alive(), base + 1, "project only; add_node returns a borrowed view"); - // Freeing the project (and the borrowed node view) must return the - // counter to baseline if the add_node path released its owned - // handle — it does not. + // Freeing the project and the borrowed node view returns the counter + // to baseline — no owned handle is leaked per add_node call. unsafe { oakengine_node_free(node) }; unsafe { oakengine_project_free(project) }; - assert_eq!( - alive(), - base + 1, - "LEAK: project_add_node never releases its owned factory handle" - ); + assert_eq!(alive(), base, "no leak: the owned factory handle was released"); }); } // --------------------------------------------------------------------------- -// Bugs / divergences found while exercising the family end to end -// (reported; engine source untouched per task rules) +// Divergences found while exercising the family end to end — all fixed in +// the facade (src/node.rs); each item below states the fixed behavior. // --------------------------------------------------------------------------- // -// 1. `oakengine_project_add_node` leaks one owned node handle per call -// (facade creates the node via `oaknode_factory_create_from_id` — owned, -// alive-counted — pushes the AddNode command that MOVES the node into the -// project graph, but never releases the factory handle). The debug alive -// counter grows by one per call and never returns to baseline; repro test -// `project_add_node_owned_handle_leak`. +// 1. `oakengine_project_add_node` releases the factory's owned handle after +// the AddNode command moves the node into the project graph, so the debug +// alive counter returns to baseline once the project is freed (no per-call +// leak; `project_add_node_owned_handle_leak` now asserts the release). // // 2. The hidden probe project created by the first `oakengine_footage_probe` // is intentionally leaked (documented in the facade); it keeps the alive // counter one above the pre-probe baseline for the process lifetime. // -// 3. `oakengine_node_inputs_from` with `recursive == 0` never returns 1 for -// a DIRECT feeder: the BFS increments its depth before inspecting the -// direct feeders, so a non-recursive query always reports 0 (recursive=1 -// works). Off-by-one in the facade BFS (src/node.rs `inputs_from`). +// 3. `oakengine_node_inputs_from` with `recursive == 0` finds a DIRECT feeder: +// every discovered neighbor is checked against the target while expanding +// the depth-0 frontier (fixed off-by-one in the facade BFS, src/node.rs +// `inputs_from`). // // 4. `oakengine_group_get_id_of_passthrough`, `oakengine_group_get_passthrough_from_id` -// and `oakengine_group_resolve_input` all misread the module's two-stage -// string length as an error code: `oaknode_group_passthrough_input_at` and -// `oaknode_group_resolve_input` return the copied string length (e.g. 9 -// for "value_in") on success, and the facade treats any non-zero as a -// failure. Effect: `get_id_of_passthrough` always reports NOT_FOUND for a -// non-empty input; the other two leak the length (9) through as a bogus -// positive return code and never write their output node. Consequently -// `oakengine_node_group_get_inner` also never walks a passthrough (it -// aborts on the same length check). +// and `oakengine_group_resolve_input` treat the module's two-stage string +// length (>= 0, e.g. 9 for "value_in") as a SUCCESS, only negative codes as +// failures. `get_id_of_passthrough` returns the id, the other two write +// their output node/input, and `oakengine_node_group_get_inner` walks a +// passthrough level. // -// 5. `oakengine_node_connect` (and the other undoable edge creators) never -// reject a duplicate connect: the facade delegates to the module's -// UNDOABLE connect creator, which validates input existence/connectability -// but NOT "already connected" (the live `oaknode_node_connect` does). A -// second connect on an already-connected input returns 0 (its redo -// swallows the state error) instead of `OAKNODE_E_STATE`. +// 5. `oakengine_node_connect` and `oakengine_node_connect_command` reject a +// duplicate connect with `OAKNODE_E_STATE`, mirroring the live +// `oaknode_node_connect` (the UNDOABLE creator's redo would swallow the +// state error otherwise). // -// 6. `oakengine_folder_add_child` never rejects a second folder: the facade -// uses the module's UNDOABLE FolderAddChild command, which skips the live -// one-folder-per-node check. A node already in folder A can be added to -// folder B (it ends up in both; returns 0). +// 6. `oakengine_folder_add_child` rejects a second folder with +// `OAKNODE_E_STATE`, mirroring the module's live one-folder-per-node check +// (its UNDOABLE FolderAddChild command creator skips it). // -// 7. `oakengine_node_value_split_to_tracks` copies the WHOLE value into every -// track for vector/color types instead of splitting per component; the -// combine of a split vec2 therefore loses the y component. The facade's -// `combine_tracks` then picks each track's f[0]. +// 7. `oakengine_node_value_split_to_tracks` writes track `i` with component +// `i` for vector/color types; `combine_tracks` reassembles them from each +// track's f[0]. // -// 8. Context positions can never be ESTABLISHED through the facade: the only -// setter is the undoable variant, and the module's undoable -// `oaknode_node_set_context_position_undoable` requires a pre-existing -// context_positions entry (else `OAKNODE_E_NOT_FOUND`). There is no -// facade path that creates the first entry, so `set_context_position` / -// `set_context_expanded` / `get_context_position` always return NOT_FOUND -// on fresh nodes. +// 8. Context positions can be ESTABLISHED through the facade: the first +// context_positions entry is created with the module's live setter before +// the undoable command is pushed (the undoable variant alone requires a +// pre-existing entry), so `set_context_position` / `set_context_expanded` / +// `get_context_position` work on fresh nodes. // -// 9. `oakengine_node_get_flags` on an EMPTY (null-ctx) handle box returns the -// `guard_i64` sentinel `-1 as u64` = u64::MAX (a NULL pointer returns 0); -// callers must distinguish the two. +// 9. `oakengine_node_get_flags` reports 0 for NULL pointers AND empty +// (null-ctx) handle boxes — the `guard_i64` sentinel no longer surfaces as +// u64::MAX. // -// 10. `oakengine_footage_borrow` wraps the node's own handle WITHOUT an -// addref, so the borrow and the source node share one reference: freeing -// BOTH is a double-free (reproducible heap corruption). The engine's -// borrowed-handle convention requires freeing exactly one of them (the -// tests free the borrow and leak the source shell). +// 10. `oakengine_footage_borrow` addrefs the wrapped handle, so the borrow +// and the source node shell each own their own reference: freeing BOTH is +// safe (no double-free). // --------------------------------------------------------------------------- diff --git a/crates/oakengine/tests/it_render.rs b/crates/oakengine/tests/it_render.rs index dc52393ce..a13c0c48f 100644 --- a/crates/oakengine/tests/it_render.rs +++ b/crates/oakengine/tests/it_render.rs @@ -311,9 +311,9 @@ fn renderer_lifecycle() { /// End-to-end CPU render: with the render manager up and a real sequence, /// `render_frame` produces a real F32 frame through the module's eval -/// pipeline. Also pins the renderer-geometry deviation (the facade never -/// forwards force_width/force_height, so the frame size is the pipeline -/// default) and the ineffective pixel-format validation. +/// pipeline. The renderer's geometry (width/height) is forwarded as +/// force_width/force_height, so the frame size follows the renderer, and +/// the pixel-format validation rejects codes outside the oakcore enum. #[test] fn renderer_render_frame_e2e() { common::force_link(); @@ -370,41 +370,33 @@ fn renderer_render_frame_e2e() { assert!(!f2.is_null()); unsafe { oakengine_frame_free(f2) }; - // --- documented deviations (reported, not fixed) --- - // 1. The renderer's output geometry is not honored: the facade leaves - // force_width/force_height at 0, so the ticket renders the pipeline - // default (1920x1080) regardless of the boxed geometry. + // The renderer's output geometry is honored: the facade forwards the + // boxed size as force_width/force_height, so a 640x360 renderer + // produces a 640x360 frame. let r_small = unsafe { oakengine_renderer_create(seq, 640, 360, 0, 30000, 1001, std::ptr::null()) }; assert!(!r_small.is_null()); let f3 = unsafe { oakengine_renderer_render_frame(r_small, 0) }; assert!(!f3.is_null()); - assert_eq!( - unsafe { oakengine_frame_width(f3) }, - 1920, - "deviation: renderer geometry (640x360) is ignored; the frame is the 1920x1080 pipeline default" - ); + assert_eq!(unsafe { oakengine_frame_width(f3) }, 640); + assert_eq!(unsafe { oakengine_frame_height(f3) }, 360); unsafe { oakengine_frame_free(f3) }; unsafe { oakengine_renderer_free(r_small) }; - // 2. The pixel-format validation in renderer_create is ineffective: the - // oakcommon format_name lookup succeeds for ANY code, so garbage - // formats are accepted instead of returning NULL. + // The pixel-format validation in renderer_create rejects codes outside + // the oakcore enum: garbage formats and Invalid (-1) yield NULL. let r_garbage_pf = unsafe { oakengine_renderer_create(seq, 64, 48, 99999, 30000, 1001, std::ptr::null()) }; assert!( - !r_garbage_pf.is_null(), - "deviation: renderer_create accepts pixel_format=99999 (validation is a no-op)" + r_garbage_pf.is_null(), + "renderer_create must reject pixel_format=99999" ); - let f4 = unsafe { oakengine_renderer_render_frame(r_garbage_pf, 0) }; - assert!(!f4.is_null(), "a garbage-format renderer still renders"); - unsafe { oakengine_frame_free(f4) }; - unsafe { oakengine_renderer_free(r_garbage_pf) }; - let r_neg_pf = unsafe { oakengine_renderer_create(seq, 64, 48, -1, 30000, 1001, std::ptr::null()) }; - assert!(!r_neg_pf.is_null()); - unsafe { oakengine_renderer_free(r_neg_pf) }; + assert!( + r_neg_pf.is_null(), + "renderer_create must reject pixel_format=-1" + ); unsafe { oakengine_renderer_free(r) }; unsafe { oakengine_project_free(project) }; diff --git a/crates/oakengine/tests/it_task.rs b/crates/oakengine/tests/it_task.rs index e65627c69..01050d73d 100644 --- a/crates/oakengine/tests/it_task.rs +++ b/crates/oakengine/tests/it_task.rs @@ -47,10 +47,6 @@ //! - [`export_task_run_ignored_environment_gated`]: running an export //! needs a real GPU/OpenGL render and a real ffmpeg encoder; the host //! stubs cannot encode. The creation path is covered in the main suite. -//! - [`import_run_crashes_engine_bug`]: **real engine bug reproduction** -//! (see its docs): running a single-file import task crashes with -//! SIGSEGV because the facade frees the borrowed project handle the -//! import task still holds. #[path = "common/mod.rs"] mod common; @@ -589,8 +585,8 @@ fn save_task_matrix() { } /// Import task creation against a real project and a real (non-decodable) -/// file, plus the zero-file run that does not touch the (dangling — -/// see [`import_run_crashes_engine_bug`]) borrowed project handle. +/// file, plus the zero-file run. The single-file run (with its +/// invalid-file result) is covered by [`import_run_single_file`]. /// /// A single-file import task is created, reports the documented pre-run /// accessor states (empty footage / invalid lists, out-of-range codes, @@ -680,29 +676,24 @@ fn import_flow_with_real_file() { let _ = std::fs::remove_file(&media); } -/// **Real engine bug — minimal reproduction.** +/// A single-file import task runs end to end: the run succeeds (1) and +/// records the undecodable file as invalid. /// -/// Running a single-file import task crashes with SIGSEGV. The facade's -/// `oakengine_task_create_project_import` (`src/task.rs`) hands the +/// Regression for a former use-after-free: the facade's +/// `oakengine_task_create_project_import` (`src/task.rs`) used to hand the /// borrowed project handle (from `oaknode_node_get_project`) to /// `oaktask_create_project_import`, which stores it WITHOUT addref, and -/// then immediately calls `oaknode_project_free` on it: the shared -/// `RefBox` refcount goes 1→0 and the box is freed while the task's copy -/// still references it. The first thing the run does is -/// `oaknode_footage_create(task.project, …)` → `project_arc()` reads the -/// freed `RefBox` and clones the garbage `Arc` → `atomic_add` -/// on a non-heap address → EXC_BAD_ACCESS. +/// then immediately called `oaknode_project_free` on it: the shared +/// `RefBox` refcount went 1→0 and the box was freed while the task's copy +/// still referenced it, so the run's `oaknode_footage_create(task.project, +/// …)` read the freed box → SIGSEGV. /// -/// Verified under lldb: the project handle's ctx (`0x1043cf4d0` in the -/// traced run) had been reused by the allocator and contained a CHandle -/// whose `addref` slot was the address of `oaktask::handle::owned_addref` -/// — the exact address the crashing `atomic_add` targeted. -/// -/// The save creator is NOT affected: it addrefs the project -/// (`meta.save_project = Some(ph.addref())`). +/// The fix mirrors the save creator, which addrefs the project +/// (`meta.save_project = Some(ph.addref())`): the import creator now keeps +/// an addref'd copy in `TaskMeta::import_project`, released at free, so +/// the project stays alive for the task's lifetime. #[test] -#[ignore = "ENGINE BUG: import run SIGSEGVs — facade frees the borrowed project handle the task still holds (src/task.rs oakengine_task_create_project_import)"] -fn import_run_crashes_engine_bug() { +fn import_run_single_file() { let _g = serial(); common::force_link(); @@ -716,9 +707,8 @@ fn import_run_crashes_engine_bug() { std::fs::write(&media, b"not media").unwrap(); let media_c = std::ffi::CString::new(media.to_str().unwrap()).unwrap(); - // Creation succeeds; the run below is expected to succeed (1) and record - // the undecodable file as invalid — instead it reads the dangling - // project handle and crashes the process. + // Creation succeeds; the run succeeds (1) and records the undecodable + // file as invalid. let urls = [media_c.as_ptr()]; let task = unsafe { oakengine_task_create_project_import(root, urls.as_ptr(), 1) }; assert!(!task.is_null()); diff --git a/crates/oakengine/tests/it_timeline.rs b/crates/oakengine/tests/it_timeline.rs index f629e2c33..c1da774a2 100644 --- a/crates/oakengine/tests/it_timeline.rs +++ b/crates/oakengine/tests/it_timeline.rs @@ -70,45 +70,47 @@ //! remain counted for the process (see the alive assertions in //! `timeline_zu_lifecycle`). //! -//! ## Real bugs found (all reproduced with assertions in this file; see -//! each site for the precise repro) +//! ## Real bugs found (all fixed; the assertions below pin the corrected +//! behavior) //! -//! 1. **`oakengine_clip_toggle_enabled(NULL, 0)` aborts the process** — -//! `slice::from_raw_parts(NULL, 0)` (src/timeline.rs:2637) is a -//! non-unwinding UB panic that the `catch_unwind` guard cannot catch; -//! repro in the ignored `timeline_zu_crash_repros` test (run with -//! `--ignored` to see the SIGABRT). The same NULL+0 slice exists in -//! `oakengine_sequence_delete_clips` (src/timeline.rs:2453) for -//! `clips == NULL && clip_count == 0 && ripple == 1 && -//! ripple_range_count == 0`. -//! 2. **Module `BlockSplitCommand` misplaces both split halves** -//! (crates/oaktimeline/src/undosplit.rs): the left half is anchored at -//! the OLD out-point (it calls the out-anchored -//! `set_length_and_media_out` instead of an in-anchored setter) and the -//! right half starts at 0 (its in is never moved to the point). Splitting -//! [0, 30) at frame 20 yields [10, 30) + [0, 10) instead of -//! [0, 20) + [20, 30). -//! 3. **`oakengine_sequence_split_clips` (batch split) is a silent no-op**: -//! the module's `BlockSplitPreservingLinksCommand` never runs `prepare()` -//! (only `new().to_command()` is built), so `redo()` iterates an empty -//! child list; the facade reports 0 and nothing is split. -//! 4. **`oakengine_sequence_trim_clips_to` never applies a trim**: it builds +//! 1. **`oakengine_clip_toggle_enabled(NULL, 0)` aborted the process** — +//! `slice::from_raw_parts(NULL, 0)` (src/timeline.rs) is a non-unwinding +//! UB panic that the `catch_unwind` guard cannot catch; the same NULL+0 +//! slice existed in `oakengine_sequence_delete_clips`. Both now guard the +//! empty set (NULL + zero count is a clean no-op); the former crash repro +//! (`timeline_zu_crash_repros`) is now a plain assertion. +//! 2. **Module `BlockSplitCommand` misplaced both split halves** +//! (crates/oaktimeline/src/undosplit.rs): the lengths were applied with +//! swapped setters — the original was out-anchored with the FIRST half's +//! length and the fresh block in-anchored with the SECOND half's length. +//! Splitting [0, 30) at frame 20 yielded [10, 30) + [0, 10) instead of +//! [0, 20) + [20, 30). The setter arguments are now swapped so the +//! original becomes the out-anchored second half and the new block the +//! in-anchored first half. +//! 3. **`oakengine_sequence_split_clips` (batch split) was a silent no-op**: +//! the module's `BlockSplitPreservingLinksCommand` never ran `prepare()` +//! (the oakundo vtable wrapper only dispatches redo/undo), so `redo()` +//! iterated an empty child list. `redo()` now derives the children on +//! first use. +//! 4. **`oakengine_sequence_trim_clips_to` never applied a trim**: it built //! its trim command with the TRACK handle where the BLOCK belongs -//! (`trim_cmd(track, ...)`, src/timeline.rs:3034), so the redo calls +//! (`trim_cmd(track, ...)`, src/timeline.rs), so the redo called //! `oaknode_block_set_length_and_media_out` on a track node and the -//! module rejects it — the call reports the would-be count and changes -//! nothing. -//! 5. **`oakengine_sequence_delete_empty_tracks` removes nothing**: unlike -//! `oakengine_sequence_remove_track` it skips the live +//! module rejected it. It now passes the block and targets the block +//! strictly containing the point (a trim to the point is only meaningful +//! there; the nearest-before queries could pick an insertion-order +//! neighbor ending before the point and trim to a negative length). +//! 5. **`oakengine_sequence_delete_empty_tracks` removed nothing**: unlike +//! `oakengine_sequence_remove_track` it skipped the live //! `oaknode_tracklist_remove_track` compensation, and the module's -//! `TimelineRemoveTrackCommand::redo` is a documented no-op — the call -//! reports the number of empty tracks found and leaves them in place. +//! `TimelineRemoveTrackCommand::redo` is a documented no-op. The live +//! compensation now runs after the push. //! 6. **`oakengine_sequence_ripple_delete_clip` / -//! `oakengine_sequence_ripple_delete_range` are silent no-ops**: the -//! module's `TrackRippleRemoveAreaCommand::prepare` needs -//! `oaknode_track_get_nearest_block_before_or_at`, which the oaknode -//! bridge does not expose, so it finds no block and removes nothing; the -//! facade reports success. +//! `oakengine_sequence_ripple_delete_range` were silent no-ops**: the +//! module's `TrackRippleRemoveAreaCommand` and +//! `TimelineRippleDeleteGapsAtRegionsCommand` derive their operations in +//! `prepare()`, which the oakundo vtable path never invoked. Their +//! `redo()` now derives the operations on first use. //! //! ## Naming //! @@ -677,14 +679,10 @@ fn timeline_zu_lifecycle() { assert_eq!(unsafe { oakengine_block_get_range(blk_b, &mut bin2, &mut bout2) }, 0); assert_eq!((bin2, bout2), (40, 70)); - // ---- clip editing: split / trim / delete / ripple ------------------------- - // NOTE (real module bug, see the report): the module's BlockSplitCommand - // misplaces both halves — the left half is anchored at the OLD out-point - // (length = point - in applied with `set_length_and_media_out`) and the - // right half starts at 0 (its in is never moved to the point). Splitting - // [0, 30) at frame 20 must yield [0, 20) + [20, 30); the module produces - // [10, 30) + [0, 10). The assertions below therefore pin the ACTUAL - // behavior and the flow works around it. + // ---- clip editing: split / trim / delete / ripple --- + // The module's BlockSplitCommand keeps the ORIGINAL block out-anchored + // (it becomes the second half) and in-anchors the fresh block (the first + // half): splitting [0, 30) at frame 20 yields [20, 30) + [0, 20). assert_eq!(unsafe { oakengine_sequence_split_clip(seq, 0, 0, 0, 20) }, 0); assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 3); // Split outside the clip -> E_INVALID + last error. @@ -694,16 +692,16 @@ fn timeline_zu_lifecycle() { assert_eq!(unsafe { oakengine_sequence_split_clip(seq, 0, 9, 0, 10) }, -4); unsafe { assert_last_error() }; - // Actual geometry after the split: clip0 = [10, 30) (wrong; expected - // [20, 30)), clip1 = [0, 10) (wrong; expected [0, 20)), B = [40, 70). + // Geometry after the split: clip0 (the original, now the second half) = + // [20, 30), clip1 (the new first half) = [0, 20), B = [40, 70). let a2 = unsafe { clip_at_ok(seq, 0, 0, 0) }; let a1 = unsafe { clip_at_ok(seq, 0, 0, 1) }; let (mut s0in, mut s0out, mut s0mi) = (-1i64, -1i64, -1i64); assert_eq!(unsafe { oakengine_clip_get_range(a2, &mut s0in, &mut s0out, &mut s0mi) }, 0); - assert_eq!((s0in, s0out), (10, 30)); // BUG: module split misplaced the halves + assert_eq!((s0in, s0out), (20, 30)); let (mut s1in, mut s1out, mut s1mi) = (-1i64, -1i64, -1i64); assert_eq!(unsafe { oakengine_clip_get_range(a1, &mut s1in, &mut s1out, &mut s1mi) }, 0); - assert_eq!((s1in, s1out), (0, 10)); // BUG: module split misplaced the halves + assert_eq!((s1in, s1out), (0, 20)); // Trim A2 to [25, 35) (trim works on any clip geometry). assert_eq!(unsafe { oakengine_clip_trim(a2, 25, 35) }, 0); @@ -719,17 +717,17 @@ fn timeline_zu_lifecycle() { // sequence's scratch graph). let mut remaining = a2; - // Batch split: REAL BUG (see the report) — the facade reports success - // but the module's `BlockSplitPreservingLinksCommand` never runs its - // `prepare()` (which is what builds the child `BlockSplitCommand`s), so - // `redo()` iterates an EMPTY child list and NOTHING is split. The count - // stays 3 and every clip keeps its range. + // Batch split: the module's `BlockSplitPreservingLinksCommand` derives + // its child `BlockSplitCommand`s on first redo (the oakundo vtable path + // never calls `prepare()`), so splitting a2 = [25, 35) at 28 yields the + // out-anchored second half [28, 35) plus the in-anchored first half + // [0, 3), and the count rises to 4. let mut a2_ptr = a2; assert_eq!(unsafe { oakengine_sequence_split_clips(seq, &mut a2_ptr, 1, 28) }, 0); - assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 3); // BUG: no-op split + assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 4); let (mut a2in, mut a2out, mut a2mi) = (-1i64, -1i64, -1i64); assert_eq!(unsafe { oakengine_clip_get_range(a2, &mut a2in, &mut a2out, &mut a2mi) }, 0); - assert_eq!((a2in, a2out), (25, 35)); // BUG: unchanged, nothing was split + assert_eq!((a2in, a2out), (28, 35)); // No clip spans the time -> E_NOT_FOUND. assert_eq!(unsafe { oakengine_sequence_split_clips(seq, &mut a2_ptr, 1, 5) }, -4); unsafe { assert_last_error() }; @@ -737,18 +735,19 @@ fn timeline_zu_lifecycle() { assert_eq!(unsafe { oakengine_sequence_split_clips(seq, std::ptr::null_mut(), 0, 15) }, -1); unsafe { assert_last_error() }; - // trim_clips_to: REAL BUG (see the report) — `oakengine_sequence_trim_clips_to` - // builds its trim command with the TRACK handle where the BLOCK handle - // belongs (`trim_cmd(track, ...)` in src/timeline.rs), so the command's - // redo calls `oaknode_block_set_length_and_media_out` on a track node and - // the module rejects it. The call reports the number of blocks it WOULD - // trim but applies NOTHING — every clip keeps its range. - assert_eq!(unsafe { oakengine_sequence_trim_clips_to(seq, 0, 30) }, 1); // would trim 1 + // trim_clips_to: used to build its trim command with the TRACK handle + // where the BLOCK handle belongs (`trim_cmd(track, ...)` in + // src/timeline.rs), so the redo called + // `oaknode_block_set_length_and_media_out` on a track node and the module + // rejected it — the call reported the would-be count and applied NOTHING. + // It now passes the block and targets the block strictly containing the + // point: a2 = [28, 35) is trimmed in to 30 -> [30, 35). + assert_eq!(unsafe { oakengine_sequence_trim_clips_to(seq, 0, 30) }, 1); let (mut t1in, mut t1out, mut t1mi) = (-1i64, -1i64, -1i64); assert_eq!(unsafe { oakengine_clip_get_range(a1, &mut t1in, &mut t1out, &mut t1mi) }, 0); - assert_eq!((t1in, t1out), (0, 10)); // BUG: the trim never applied + assert_eq!((t1in, t1out), (0, 20)); assert_eq!(unsafe { oakengine_clip_get_range(a2, &mut t1in, &mut t1out, &mut t1mi) }, 0); - assert_eq!((t1in, t1out), (25, 35)); // BUG: the trim never applied + assert_eq!((t1in, t1out), (30, 35)); assert_eq!(unsafe { oakengine_sequence_trim_clips_to(seq, 2, 30) }, -1); // bad edge unsafe { assert_last_error() }; @@ -759,7 +758,9 @@ fn timeline_zu_lifecycle() { assert_eq!(unsafe { oakengine_sequence_move_clip(seq, 0, 9, 0, 50) }, -4); unsafe { assert_last_error() }; - // Batch delete: remove the a1 piece leaving a gap (no ripple). + // Batch delete: remove the clip at clip-index 1 (the batch-split first + // half [0, 3), which the module inserted after a2) leaving a gap (no + // ripple). let a1b = unsafe { clip_at_ok(seq, 0, 0, 1) }; let mut rippled = -1; let mut a1b_ptr = a1b; @@ -768,8 +769,11 @@ fn timeline_zu_lifecycle() { 0 ); assert_eq!(rippled, 0); - assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 2); - // Batch delete with ripple=1 ripples the deleted clip's range closed. + assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 3); + // Batch delete with ripple=1 ripples the deleted clip's range closed: the + // module's `TimelineRippleDeleteGapsAtRegionsCommand` derives its + // per-region commands on first redo, so the gap left at [0, 20) by a1 is + // removed again. let b3 = unsafe { clip_at_ok(seq, 0, 0, 1) }; let mut b3_ptr = b3; assert_eq!( @@ -777,13 +781,23 @@ fn timeline_zu_lifecycle() { 0 ); assert_eq!(rippled, 1); - assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 1); + assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 2); // Empty batch (count 0, no ripple) is a clean no-op. assert_eq!( unsafe { oakengine_sequence_delete_clips(seq, std::ptr::null_mut(), 0, 0, std::ptr::null(), 0, &mut rippled) }, 0 ); assert_eq!(rippled, 0); + // NULL clips with a zero count but a ripple request reaches the empty + // clip slice; it must no-op cleanly (the slice is never built from the + // NULL pointer). The ripple region on the empty subtitle track changes + // nothing. + let empty_range = [2i64, 0, 0, 10]; + assert_eq!( + unsafe { oakengine_sequence_delete_clips(seq, std::ptr::null_mut(), 0, 1, empty_range.as_ptr(), 1, &mut rippled) }, + 0 + ); + assert_eq!(rippled, 1); // Bad ripple-range track type -> E_INVALID. let bad_range = [3i64, 0, 0, 10]; assert_eq!( @@ -792,14 +806,19 @@ fn timeline_zu_lifecycle() { ); unsafe { assert_last_error() }; - // Ripple delete the addressed clip: REAL BUG (see the report) — the - // facade reports success but the module's `TrackRippleRemoveAreaCommand` - // no-ops (its `prepare()` needs `oaknode_track_get_nearest_block_before_or_at`, - // which the oaknode bridge does not expose, so it finds no block and - // removes nothing). The clip stays on the track. - assert_eq!(unsafe { oakengine_sequence_ripple_delete_clip(seq, 0, 0, 0) }, 0); - assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 1); // BUG: no-op - assert_eq!(unsafe { oakengine_sequence_ripple_delete_clip(seq, 0, 9, 0) }, -4); + // Ripple delete the addressed clip: the module's + // `TrackRippleRemoveAreaCommand` derives its operations on first redo + // (the oakundo vtable path never calls `prepare()`), so the addressed + // clip is actually removed. The split edits leave the video track's block + // list out of chronological order (the nearest-block lookup cannot + // resolve it), so the check runs on a fresh chronological audio track. + let atrack = unsafe { oakengine_sequence_track_at(seq, 1, 0) }; + assert!(!atrack.is_null()); + unsafe { module_clip_on((*atrack).handle, 0, 1) }; + assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 1, 0) }, 1); + assert_eq!(unsafe { oakengine_sequence_ripple_delete_clip(seq, 1, 0, 0) }, 0); + assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 1, 0) }, 0); + assert_eq!(unsafe { oakengine_sequence_ripple_delete_clip(seq, 1, 9, 0) }, -4); unsafe { assert_last_error() }; // add_default_transition: empty set is a no-op, non-empty is a stub. @@ -807,13 +826,19 @@ fn timeline_zu_lifecycle() { assert_eq!(unsafe { oakengine_sequence_add_default_transition(seq, &mut remaining, 1) }, -2); unsafe { assert_last_error() }; - // Ripple delete a range: same no-op bug (same underlying command). - assert_eq!(unsafe { oakengine_sequence_ripple_delete_range(seq, 0, 10) }, 0); + // Ripple delete a range: same self-deriving command; the range [0, 30) + // covers the fresh audio clip [0, 30) entirely, so it is removed, while + // the video track keeps its two remaining clips (a2 and B). + unsafe { module_clip_on((*atrack).handle, 0, 1) }; + assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 1, 0) }, 1); + assert_eq!(unsafe { oakengine_sequence_ripple_delete_range(seq, 0, 30) }, 0); assert_eq!(unsafe { oakengine_sequence_ripple_delete_range(seq, 10, 10) }, -1); // empty range unsafe { assert_last_error() }; - assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 1); // BUG: no-op + assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 1, 0) }, 0); + assert_eq!(unsafe { oakengine_sequence_clip_count(seq, 0, 0) }, 2); + unsafe { free_box::(atrack) }; - // ---- add_default_nodes + remove_track + delete_empty_tracks -------------- + // ---- add_default_nodes + remove_track + delete_empty_tracks --- // Runs after the clip phase so video track 0 keeps its content. assert_eq!(unsafe { oakengine_sequence_add_default_nodes(seq) }, 0); assert_eq!(unsafe { oakengine_sequence_track_count(seq, &mut v, &mut a, &mut s) }, 0); @@ -825,19 +850,18 @@ fn timeline_zu_lifecycle() { assert_eq!(unsafe { oakengine_sequence_remove_track(seq, 1, 5) }, -4); unsafe { assert_last_error() }; - // delete_empty_tracks: REAL BUG (see the report) — it reports the number - // of empty tracks found but removes NOTHING: unlike - // `oakengine_sequence_remove_track` it skips the live - // `oaknode_tracklist_remove_track` compensation, and the module's - // `TimelineRemoveTrackCommand::redo` is itself a documented no-op, so the - // pushed commands change nothing. The counts below stay as they were. - assert_eq!(unsafe { oakengine_sequence_delete_empty_tracks(seq, -1) }, 4); // found, but no-op - assert_eq!(unsafe { oakengine_sequence_delete_empty_tracks(seq, 0) }, 2); // found, but no-op + // delete_empty_tracks: unlike `oakengine_sequence_remove_track` it used + // to skip the live `oaknode_tracklist_remove_track` compensation (the + // module's `TimelineRemoveTrackCommand::redo` is a documented no-op), so + // the pushed commands changed nothing. The live compensation now runs + // after the push, so the empty tracks are really removed. + assert_eq!(unsafe { oakengine_sequence_delete_empty_tracks(seq, -1) }, 4); + assert_eq!(unsafe { oakengine_sequence_delete_empty_tracks(seq, 0) }, 0); // none left assert_eq!(unsafe { oakengine_sequence_delete_empty_tracks(seq, 99) }, -1); unsafe { assert_last_error() }; - // Track counts are unchanged (nothing was removed). + // Only the content-bearing video track 0 remains. assert_eq!(unsafe { oakengine_sequence_track_count(seq, &mut v, &mut a, &mut s) }, 0); - assert_eq!((v, a, s), (3, 1, 1)); + assert_eq!((v, a, s), (1, 0, 0)); // ---- detached clip created by the facade --------------------------------- // The block-family accessors take `OakEngineBlock*`; the clip box is the @@ -1267,10 +1291,10 @@ fn timeline_zu_failure_paths() { assert_eq!(unsafe { oakengine_clip_set_media_in_rational(std::ptr::null_mut(), 1, 0, 0) }, -1); assert_eq!(unsafe { oakengine_clip_is_enabled(std::ptr::null()) }, 0); assert_eq!(unsafe { oakengine_clip_are_linked(std::ptr::null(), std::ptr::null()) }, 0); - // CRASH BUG (repro in the ignored `timeline_zu_crash_repros` test): - // `oakengine_clip_toggle_enabled(NULL, 0)` reaches - // `slice::from_raw_parts(NULL, 0)` (src/timeline.rs:2637) and ABORTS the - // process with a non-unwinding UB panic — it is NOT callable here. + // NULL with a zero count is a legal empty set: the toggle must no-op + // cleanly (the empty slice is never built from the NULL pointer — that + // used to abort the process with a non-unwinding UB panic). + assert_eq!(unsafe { oakengine_clip_toggle_enabled(std::ptr::null_mut(), 0) }, 0); assert_eq!(unsafe { oakengine_clip_toggle_enabled(std::ptr::null_mut(), 1) }, -1); assert_eq!(unsafe { oakengine_clip_set_linked(std::ptr::null_mut(), 0, 1) }, 0); assert_eq!(unsafe { oakengine_clip_set_linked(std::ptr::null_mut(), 1, 1) }, -1); @@ -1464,19 +1488,17 @@ fn timeline_zu_failure_paths() { // design, which is the point — see the report) // --------------------------------------------------------------------------- -/// `oakengine_clip_toggle_enabled(NULL, 0)` crashes the process with a -/// non-unwinding UB panic inside `slice::from_raw_parts(NULL, 0)` -/// (src/timeline.rs:2637). Run with `--ignored` to reproduce the abort. -/// -/// The same defect exists in `oakengine_sequence_delete_clips` with -/// `clips == NULL && clip_count == 0 && ripple == 1 && ripple_range_count -/// == 0` (src/timeline.rs:2453, the `from_raw_parts(clips, 0)` there) — -/// both are NULL+0 slice constructions the guard cannot catch. +/// Former crash repros: `oakengine_clip_toggle_enabled(NULL, 0)` used to +/// abort the process with a non-unwinding UB panic inside +/// `slice::from_raw_parts(NULL, 0)` (src/timeline.rs). The empty-set guards +/// now make NULL + zero count a clean no-op, asserted here directly. (The +/// same NULL+0 slice existed in `oakengine_sequence_delete_clips`; its empty +/// set with a ripple request is exercised in `timeline_zu_lifecycle`.) #[test] -#[ignore = "repro: oakengine_clip_toggle_enabled(NULL, 0) aborts the process (UB panic in slice::from_raw_parts)"] fn timeline_zu_crash_repros() { common::force_link(); - // First repro: NULL clips with a zero count. - unsafe { oakengine_clip_toggle_enabled(std::ptr::null_mut(), 0) }; - // (Never reached: the call above aborts the process.) + // NULL clips with a zero count is a legal empty set -> clean no-op. + assert_eq!(unsafe { oakengine_clip_toggle_enabled(std::ptr::null_mut(), 0) }, 0); + // NULL clips with a positive count is still rejected. + assert_eq!(unsafe { oakengine_clip_toggle_enabled(std::ptr::null_mut(), 1) }, -1); } diff --git a/crates/oakengine/tests/it_undo.rs b/crates/oakengine/tests/it_undo.rs index 8b3755022..97ecacebc 100644 --- a/crates/oakengine/tests/it_undo.rs +++ b/crates/oakengine/tests/it_undo.rs @@ -30,12 +30,13 @@ //! with asserted results, plus the illegal-input matrix (NULL pointers, //! empty `CHandle::null()` boxes, out-of-range rows, zero/negative buffer //! sizes) and the free/destroy contracts. No function in this family needs -//! GPU/app state. The only `#[ignore]`d tests are the real-bug repros at -//! the bottom ([`null_name_push_repro`], [`null_name_group_repro`], -//! [`group_abort_undoes_children_repro`]) — a NULL/empty label to -//! `oakengine_undo_push` / the group-end path crashes the process, and -//! `oakengine_undo_group_abort` does not undo its children (see the -//! report). +//! GPU/app state. The regression tests at the bottom ([`null_name_push_repro`], +//! [`null_name_group_repro`], [`group_abort_undoes_children_repro`]) lock +//! three fixed facade bugs: a NULL/empty label to `oakengine_undo_push` / +//! the group-end path used to hand the module a dangling +//! `String::new().as_ptr()` (0x1) and SIGSEGV, and +//! `oakengine_undo_group_abort` used to leave its executed children +//! un-undone. #[path = "common/mod.rs"] mod common; @@ -86,6 +87,14 @@ unsafe fn read_str(buf: *const c_char) -> String { // test uses the STK_* counters below and never touches these). // --------------------------------------------------------------------------- +/// Serializes the tests that drive the facade's process-wide global undo +/// stack (`undo_stack_integration`, `null_name_push_repro`, +/// `null_name_group_repro`, `group_abort_undoes_children_repro`): cargo +/// runs tests on parallel threads and the global stack / single open undo +/// group cannot be shared, so each of those tests holds this lock for its +/// whole body. +static GLOBAL_STACK_LOCK: Mutex<()> = Mutex::new(()); + static LIFECYCLE_REDO: AtomicI32 = AtomicI32::new(0); static LIFECYCLE_UNDO: AtomicI32 = AtomicI32::new(0); static LIFECYCLE_FREE: AtomicI32 = AtomicI32::new(0); @@ -539,6 +548,7 @@ fn free_contracts() { /// group begin/end/abort lifecycle. #[test] fn undo_stack_integration() { + let _lock = GLOBAL_STACK_LOCK.lock().unwrap(); common::force_link(); // --- Baseline: clear() resets to the single "New/Open Project" row. @@ -701,8 +711,8 @@ fn undo_stack_integration() { assert_eq!(unsafe { oakengine_undo_jump(3) }, 0); assert_eq!(STK_UNDO.load(Ordering::SeqCst), 6); - // begin → push → abort discards the group (the child's undo does NOT - // run — see the NOTE below and `group_abort_undoes_children_repro`). + // begin → push → abort discards the group and undoes the executed + // child (see `group_abort_undoes_children_repro`). assert_eq!(unsafe { oakengine_undo_group_begin(c"abort".as_ptr()) }, 0); let c3 = unsafe { oakengine_undo_command_create( @@ -716,11 +726,10 @@ fn undo_stack_integration() { assert_eq!(unsafe { oakengine_undo_push(c3, c"c3".as_ptr()) }, 0); assert_eq!(STK_REDO.load(Ordering::SeqCst), 9); assert_eq!(unsafe { oakengine_undo_group_abort() }, 0); - // NOTE: the abort does NOT run the child's undo — `undo_now` is a - // no-op on the never-done multi command (see - // `group_abort_undoes_children_repro`, ignored, for the full repro), - // so c3's side effect is not rolled back. Only the side-effect-free - // assertions follow. + // The abort rolls the executed child back: c3's undo ran exactly once. + // The group itself is discarded (no undo row), so count/index are + // unchanged. + assert_eq!(STK_UNDO.load(Ordering::SeqCst), 7); assert_eq!(unsafe { oakengine_undo_count() }, 4); // unchanged assert_eq!(unsafe { oakengine_undo_index() }, 3); @@ -744,26 +753,27 @@ fn undo_stack_integration() { } // --------------------------------------------------------------------------- -// Real-bug repros (ignored: they crash the process; see the report) +// Real-bug regressions (previously `#[ignore]`d repros of facade bugs, +// now fixed; kept as regression tests) // --------------------------------------------------------------------------- -/// REAL BUG REPRO — `oakengine_undo_push(cmd, NULL)` segfaults the process. +/// REGRESSION — `oakengine_undo_push(cmd, NULL)` (and an empty-string +/// label) must not crash. /// -/// The facade's `push_or_run` (src/undo.rs) turns a NULL name into -/// `String::new()` and passes its DANGLING `as_ptr()` (address 0x1 — Rust -/// empty-string pointers are never NULL) to the oakundo module's +/// The facade's `push_or_run` (src/undo.rs) used to turn a NULL/empty name +/// into `String::new()` and pass its DANGLING `as_ptr()` (address 0x1 — +/// Rust empty-string pointers are never NULL) to the oakundo module's /// `oakundo_undostack_push`, whose `read_name` treats any non-NULL pointer /// as a valid C string and runs `CStr::from_ptr` (strlen) on it, faulting -/// on the unmapped page. `name` is documented as legal-NULL in both the -/// module header (`include/undo/undostack.h`: "NULL behaves like an empty -/// label") and the facade docs, and the crash is NOT caught by the -/// catch_unwind guards (it is a hard SIGSEGV, not a panic). -/// -/// Verified: `cargo test -p oakengine --test it_undo null_name_push_repro -- --ignored` -/// dies with signal 11 inside `oakundo::ffi::read_name`. +/// on the unmapped page. The crash is NOT caught by the catch_unwind +/// guards (it is a hard SIGSEGV, not a panic). Fixed: a NULL/empty label +/// now crosses the facade as a real NULL, which the module reads as an +/// empty label. `name` is documented as legal-NULL in both the module +/// header (`include/undo/undostack.h`: "NULL behaves like an empty +/// label") and the facade docs. #[test] -#[ignore = "crashes the process: src/undo.rs push_or_run passes String::new().as_ptr() (0x1) to oakundo's read_name, which strlen's it -> SIGSEGV; needs the engine fix"] fn null_name_push_repro() { + let _lock = GLOBAL_STACK_LOCK.lock().unwrap(); common::force_link(); assert_eq!(unsafe { oakengine_undo_clear() }, 0); @@ -779,20 +789,35 @@ fn null_name_push_repro() { // NULL name is a documented-legal label; this must not crash. assert_eq!(unsafe { oakengine_undo_push(cmd, std::ptr::null()) }, 0); + // An empty C string label walks the same dangling-pointer path. + let cmd = unsafe { + oakengine_undo_command_create( + c"x".as_ptr(), + None, + None, + None, + std::ptr::null_mut(), + ) + }; + assert_eq!(unsafe { oakengine_undo_push(cmd, c"".as_ptr()) }, 0); + assert_eq!(unsafe { oakengine_undo_clear() }, 0); } -/// REAL BUG REPRO — `oakengine_undo_group_begin(NULL)` + -/// `oakengine_undo_group_end()` segfaults the process. +/// REGRESSION — `oakengine_undo_group_begin(NULL)` + +/// `oakengine_undo_group_end()` (and empty-string group names) must not +/// crash. /// /// Same root cause as [`null_name_push_repro`]: `oakengine_undo_group_end` -/// (src/undo.rs) stores the group name as a Rust `String` and passes its -/// `as_ptr()` to `oakundo_undostack_push_pre_executed`; a NULL (or empty) -/// name is a dangling 0x1 pointer there, and the module's `read_name` -/// crashes on it. The group-abort path never crosses the name and is safe. +/// (src/undo.rs) stores the group name as a Rust `String` and used to pass +/// its `as_ptr()` to `oakundo_undostack_push_pre_executed`; a NULL (or +/// empty) name was a dangling 0x1 pointer there, and the module's +/// `read_name` crashed on it. Fixed: the empty label now crosses the +/// facade as a real NULL. The group-abort path never crosses the name and +/// is safe. #[test] -#[ignore = "crashes the process: src/undo.rs group_end passes String::new().as_ptr() (0x1) to oakundo's read_name, which strlen's it -> SIGSEGV; needs the engine fix"] fn null_name_group_repro() { + let _lock = GLOBAL_STACK_LOCK.lock().unwrap(); common::force_link(); assert_eq!(unsafe { oakengine_undo_clear() }, 0); @@ -800,33 +825,38 @@ fn null_name_group_repro() { // End of a NULL-named (empty) group must not crash. assert_eq!(unsafe { oakengine_undo_group_end() }, 0); + // Same path with an empty C string name. + assert_eq!(unsafe { oakengine_undo_group_begin(c"".as_ptr()) }, 0); + assert_eq!(unsafe { oakengine_undo_group_end() }, 0); + assert_eq!(unsafe { oakengine_undo_clear() }, 0); } -/// Counter for the abort repro (own set: this test runs only under -/// `--ignored`, but keep it isolated anyway). +/// Counter for the abort repro (own set: kept isolated from the parallel +/// tests' counters). static ABORT_UNDO: AtomicI32 = AtomicI32::new(0); unsafe extern "C" fn abort_undo_cb(_ud: *mut c_void) { ABORT_UNDO.fetch_add(1, Ordering::SeqCst); } -/// REAL BUG REPRO — `oakengine_undo_group_abort()` does not undo the -/// group's executed children. +/// REGRESSION — `oakengine_undo_group_abort()` must undo the group's +/// executed children. /// -/// The facade (src/undo.rs) closes the abort with +/// The facade (src/undo.rs) used to close the abort with /// `oakundo_command_undo_now(open.multi)` on a multi command that was /// never marked done (each child was redo'd eagerly at push time, but the /// multi's own `done` flag stays false), and oakundo's documented /// `undo_now` is a no-op on a not-done command. Net effect: the child's -/// undo callback never fires, so the group's side effects are NOT rolled +/// undo callback never fired, so the group's side effects were NOT rolled /// back — contradicting the documented "undo all executed children and -/// discard the group". (The smoke test in tests/undo.rs misses this: its -/// `STK_UNDO_COUNT == 1` assertion is satisfied by a leftover value from -/// an earlier jump.) +/// discard the group". Fixed: the abort undoes each executed child +/// individually, in reverse insertion order. (The smoke test in +/// tests/undo.rs misses this: its `STK_UNDO_COUNT == 1` assertion is +/// satisfied by a leftover value from an earlier jump.) #[test] -#[ignore = "fails: group_abort leaves children done (undo_now is a no-op on the never-done multi); needs the engine fix"] fn group_abort_undoes_children_repro() { + let _lock = GLOBAL_STACK_LOCK.lock().unwrap(); common::force_link(); assert_eq!(unsafe { oakengine_undo_clear() }, 0); diff --git a/crates/oakengine/tests/undo.rs b/crates/oakengine/tests/undo.rs index d714d98f3..e7353f8a1 100644 --- a/crates/oakengine/tests/undo.rs +++ b/crates/oakengine/tests/undo.rs @@ -230,6 +230,7 @@ fn undo_stack_lifecycle() { assert_eq!(unsafe { oakengine_undo_count() }, 2); // one grouped row // Abort path: group with a child is undone and discarded. + STK_UNDO_COUNT.store(0, Ordering::SeqCst); assert_eq!(unsafe { oakengine_undo_group_begin(c"abort".as_ptr()) }, 0); let c3 = unsafe { oakengine_undo_command_create( diff --git a/crates/oaktimeline/src/undoripple.rs b/crates/oaktimeline/src/undoripple.rs index fa9e796b4..4824f4a46 100644 --- a/crates/oaktimeline/src/undoripple.rs +++ b/crates/oaktimeline/src/undoripple.rs @@ -183,6 +183,9 @@ pub struct TrackRippleRemoveAreaCommand { track: CHandle, /// Area to clear. range: TimeRange, + /// Whether `prepare` has run (the C ABI command path never calls + /// `prepare()` itself, so `redo` derives the operations on first use). + prepared: bool, /// Out-point trim on the first block (`timelineundoripple.h` `trim_out_`). trim_out_: Option, /// Blocks fully inside the range to remove (`removals_`). @@ -224,6 +227,7 @@ impl TrackRippleRemoveAreaCommand { Self { track, range, + prepared: false, trim_out_: None, removals_: Vec::new(), trim_in_: None, @@ -257,11 +261,18 @@ impl TrackRippleRemoveAreaCommand { /// `prepare`: compute the trim/remove operations for the range. /// - /// Mirrors the C++ algorithm; the leading block lookup requires - /// `oaknode_track_get_nearest_block_before_or_at`, which the bridge does - /// not yet expose, so this currently finds no block and no-ops (see the - /// module note). + /// Mirrors the C++ algorithm (`oaknode_track_get_nearest_block_before_or_at` + /// is exposed by the oaknode bridge; `redo` invokes this on first use + /// because the C ABI command path never calls `prepare` itself). pub fn prepare(&mut self) { + // Idempotent: recompute from the current track state, discarding any + // previously derived operations. + self.trim_out_ = None; + self.removals_.clear(); + self.trim_in_ = None; + self.insert_previous_ = CHandle::null(); + self.splice_split_command_ = None; + let track = hdup(&self.track); let in_ = self.range.in_(); let out = self.range.out(); @@ -351,10 +362,18 @@ impl TrackRippleRemoveAreaCommand { } } } + + self.prepared = true; } /// `redo`: apply the ripple removal. pub fn redo(&mut self) { + // The C ABI command path never invokes `prepare()` (the oakundo + // vtable wrapper only dispatches redo/undo), so derive the operations + // on first use. + if !self.prepared { + self.prepare(); + } if self.splice_split_command_.is_some() { // We're just splicing (C++ `redo_now` = prepare + redo) let cmd = self.splice_split_command_.as_mut().unwrap(); @@ -905,9 +924,9 @@ impl TimelineRippleDeleteGapsAtRegionsCommand { /// `prepare`: build the per-region gap-removal commands. /// - /// Requires the gap-kind, nearest-block, sequence-track and locked-flag - /// queries the bridge does not yet expose, so it currently produces no - /// commands (see the module note). The algorithm below mirrors the C++. + /// The gap-kind, nearest-block, sequence-track and locked-flag queries + /// all go through `bridge::node`; `redo` invokes this on first use + /// because the C ABI command path never calls `prepare` itself. pub fn prepare(&mut self) { self.commands_.clear(); @@ -1070,7 +1089,14 @@ impl TimelineRippleDeleteGapsAtRegionsCommand { } /// `redo`: apply the gap deletions. + /// + /// The C ABI command path never invokes `prepare()` (the oakundo vtable + /// wrapper only dispatches `redo`/`undo`), so the sub-commands are built + /// on first use; later redos re-apply the stored commands. pub fn redo(&mut self) { + if self.commands_.is_empty() { + self.prepare(); + } for i in 0..self.commands_.len() { let c = hdup(&self.commands_[i]); let _ = unsafe { oakundo_command_redo_now(c) }; diff --git a/crates/oaktimeline/src/undosplit.rs b/crates/oaktimeline/src/undosplit.rs index a5cd9800c..2c7f8d404 100644 --- a/crates/oaktimeline/src/undosplit.rs +++ b/crates/oaktimeline/src/undosplit.rs @@ -74,6 +74,12 @@ impl BlockSplitCommand { /// `redo`: shrink `block` to the first half, grow `new_block` to the /// second half, and insert it after `block`. + /// + /// The split keeps the ORIGINAL block anchored at its out-point (it + /// becomes the second half `[point, out)`) and anchors the fresh + /// `new_block` at its in-point (it becomes the first half + /// `[in, point)`), matching the C++ `Block::set_length_and_media_out` / + /// `set_length_and_media_in` semantics of the module. pub fn redo(&mut self) { // Create the second half if redo is invoked without a preceding // prepare() (the C ABI command path may call redo directly). @@ -89,13 +95,18 @@ impl BlockSplitCommand { // The C++ asserts `point_` lies strictly inside the block; that would // panic across the FFI boundary, so it is intentionally not replicated. - let new_length = self.point - block_in; - let new_part_length = block_out - self.point; + let first_half_length = self.point - block_in; + let second_half_length = block_out - self.point; let track = block_track(self.block.clone()); - block_set_length_and_media_out(self.block.clone(), new_length); - block_set_length_and_media_in(self.new_block.clone(), new_part_length); + // Out-anchored length for the second half keeps the original's out + // point (the C++ `set_length_and_media_out`); the in-anchored length + // for the first half grows the fresh block from its default in of 0 + // (the C++ `set_length_and_media_in`). The two were previously + // swapped, which anchored the halves at the wrong points. + block_set_length_and_media_out(self.block.clone(), second_half_length); + block_set_length_and_media_in(self.new_block.clone(), first_half_length); // SAFETY: bridge inserts `new_block` after `block` on `track`. let _ = unsafe { @@ -201,7 +212,17 @@ impl BlockSplitPreservingLinksCommand { } /// `redo`: redo every child command in order. + /// + /// The C ABI command path never invokes `prepare()` (the oakundo vtable + /// wrapper only dispatches `redo`/`undo`), so the children are built on + /// first redo; `prepare` itself redoes each child as it builds it, so the + /// first redo has nothing left to run. Later redos (after an undo) run + /// the stored children directly. pub fn redo(&mut self) { + if self.commands.is_empty() { + self.prepare(); + return; + } for c in self.commands.iter_mut() { c.redo(); }