plugin: fix 3 critical bugs from R1 audit (FFI panic, dialog deadlock, init hang) - #35
Merged
Merged
Conversation
From the plugin subsystem audit:
(1) Janet C functions now wrap their bodies in std::panic::catch_unwind.
They were declared `unsafe extern "C-unwind"` which would technically
allow Rust panics to propagate into Janet's C runtime, but Janet
isn't built to clean up after foreign unwinds — heap corruption and
segfaults follow. catch_unwind converts any panic to a safe default
(Janet false for confirm, nil for select).
(2) send_dialog no longer blocks indefinitely on `reply_rx.recv()`.
It now polls every DIALOG_POLL (50 ms) and checks a shared
`SHUTDOWN: Arc<AtomicBool>` thread-local. Worker::Drop flips that
flag before sending Cmd::Shutdown, so an in-flight harness/confirm
or harness/select wakes up and returns None within ~one poll
instead of pinning the worker forever when the UI receiver is
dropped. Before this, a UI exit during a plugin dialog would
cascade into hangs on shutdown.
(3) The init handshake now uses recv_timeout(INIT_TIMEOUT = 10s)
instead of recv(). A worker panic before init_tx.send() would
previously hang main forever; the watchdog bounds that worst
case.
(4) wrap_string asserts via i32::try_from instead of a silent `as i32`
truncation. >2 GB strings now return Janet nil instead of letting
Janet read past the allocation. Real-world dialogs never produce
such strings — this is just defense in depth.
(5) The UI dialog arm no longer eats unrelated user events. Paste,
mouse, scroll, resize, and unrecognized keys during a confirm/
select dialog are now stashed in a `deferred: Vec<UserEvent>`
and re-queued via user_tx.send after the dialog ends. Ctrl+C
inside a dialog is treated as cancel (same as Esc) so the user
can always escape a stuck dialog without dropping queued work.
Tests:
* shutdown_flag_aborts_in_flight_dialog — exercises the cancellation
path: spawn worker, kick off confirm, flip the shutdown flag,
verify the eval returns within 2 s (vs. forever before R1).
* wrap_string_handles_empty — round-trips an empty string through
select to catch wrap_string size-handling regressions.
Total: 504 pass with plugin feature (was 502).
Refs dirge-woq.
3 tasks
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
1 participant
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.
Summary
Plugin subsystem audit found three critical issues that could hang or
crash the agent. This PR fixes all three, plus two related smaller
issues.
std::panic::catch_unwind—a Rust panic inside
harness/confirm/harness/selectwas free tounwind into Janet's C runtime, which isn't built for it. Panics now
return a safe default (Janet false / nil).
send_dialogpolls a shared shutdown flag every 50 ms insteadof blocking on
reply_rx.recv().Worker::Dropflips the flagfirst so an in-flight dialog cleanly aborts when the UI exits or
the worker is being torn down — previously the worker would hang
on
recv()forever andjoin()would never return.recv_timeout(10s)instead ofrecv(),so a worker panic before
init_tx.send()no longer hangs main.Plus:
wrap_stringswitchesbytes.len() as i32→i32::try_fromsokey events while a
confirm/selectdialog is open — they'restashed in a
deferred: Vec<UserEvent>and re-queued viauser_tx.sendafter the dialog ends. Ctrl+C inside a dialognow acts as cancel.
Test plan
cargo test --features plugin— 504 pass, 0 fail (was 502).shutdown_flag_aborts_in_flight_dialogexercises theWorker→cfn cancellation path end-to-end.
wrap_string_handles_emptyround-trips an empty stringthrough select to catch wrap_string regressions.
cargo buildandcargo testbaseline (withoutplugin):452 pass + 12 pre-existing plugin-test fails (unchanged).
plugins/confirm_destructive.janet,trigger a confirm, kill the terminal mid-dialog — agent
shuts down promptly instead of hanging.
Refs dirge-woq.