Conversation
… lock in Drop - Introduce InferenceGuard: the model's raw FFI handle is now only accessible through the guard returned by lock_inference(), making it a compile error to touch the handle without holding the lock. stop() remains the sole documented exception (atomic-only). - Rewrite token_trampoline to use &CallbackState (shared ref) with Cell/UnsafeCell interior mutability and an in_callback re-entrancy guard, eliminating the previous &mut aliasing risk. - Acquire inference_lock in Model::drop so cactus_destroy waits for any in-flight FFI operation to complete. - Add SAFETY comment for the stack-pinned CallbackState pointer passed to cactus_complete. Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
✅ Deploy Preview for hyprnote-storybook canceled.
|
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote canceled.
|
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: cactus soundness — enforce Sync invariant, guard callback aliasing, lock in Drop
Summary
Addresses three soundness issues identified in
crates/cactus:Compile-time Sync invariant enforcement (
model.rs): IntroducedInferenceGuard— the model's raw FFI handle is now only accessible through the guard returned bylock_inference(). Previously,raw_handle()was a standalonepub(crate)method callable without holding the lock, relying purely on convention. Now it's a compile error to touch the handle without locking.stop()remains the sole exception (atomic-only, documented).Token callback aliasing (
llm/complete.rs):token_trampolinepreviously castuser_datato&mut CallbackState, which would be instant UB if C++ ever re-entered the callback. Replaced with a shared&CallbackStatereference usingCell/UnsafeCellfor interior mutability, plus anin_callbackre-entrancy guard that bails out before creating a second&mutto the closure.Lock in
Model::drop(model.rs):cactus_destroynow waits for any in-flight FFI operation to complete by acquiringinference_lockbefore destroying the handle.std::env::set_varunsoundness inCloudConfig::prepare_envis not addressed here (out of scope per discussion).Review & Testing Checklist for Human
UnsafeCell+in_callbackpattern intoken_trampoline: Confirm the C++ side calls the token callback strictly sequentially from a single thread (never re-entrantly).Cellis!Sync, so multi-threaded callback invocation would be unsound — thein_callbackguard only protects against single-threaded re-entrancy.Model::dropdeadlock risk: The new lock acquisition is safe under normal Rust ownership (exclusive&mut selfat drop), but verify no code path could holdinference_lockand then trigger a drop on the same thread (e.g. viaArccycle or panic unwinding).Transcriber::process/call_stop/dropremain correct — they lock inference for serialization but access the Transcriber's own stream handle, not the model handle. No functional change intended.Recommended test: run existing integration/unit tests for LLM completion (streaming + non-streaming), STT (batch + streaming), and VAD to confirm no regressions.
Notes
cactus,ci,fmt,desktop_ci(linux-x86_64, linux-aarch64) all green. macOS desktop CI was queued at time of submission.cactus-sysdue to architecture mismatch). CI was the first real compilation check.