fix(coprocessor): scheduler, catch panic#2170
Conversation
|
@claude /pr-review |
There was a problem hiding this comment.
[AUTOMATED]
Code Review
PR: fix(coprocessor): scheduler, catch panic
Summary
This PR adds resilience to the FHE scheduler by wrapping run_computation() in std::panic::catch_unwind(), converting panics (e.g., from division/remainder overflow in TFHE operations) into structured SchedulerError::ExecutionPanic(String) errors. The changes are well-structured:
- scheduler.rs: Wraps the computation call with panic catching, extracts the panic message (handling
&str,String, and unknown payloads), and properly logs + reports via telemetry. - types.rs: Adds
ExecutionPanic(String)variant, correctly removesCopy(sinceStringisn'tCopy), and addsDisplayformatting. - tfhe_worker.rs: Updates from copy-dereference (
*err.downcast_ref()) to.cloned()— necessary and correct change sinceSchedulerErroris no longerCopy. - Tests: Comprehensive test that triggers panics via div/rem overflow, verifies errors are properly caught and recorded in the DB with
ExecutionPanicmessages.
Review Results
Checked for bugs, security issues, guideline compliance, and error handling quality. No issues found.
Key observations:
- The
catch_unwindclosure correctly satisfiesUnwindSafewithout needingAssertUnwindSafe— all captured values (i32,Vec,usize,&Vec<u8>) are inherently unwind-safe. - Panic payload extraction covers the two standard panic types (
&strfrom literal panics,Stringfrom formatted panics) with a sensible fallback. - The
ExecutionPanicerror correctly falls through toset_computation_error()in the downstream worker — this is the desired behavior since panics during execution represent actual operation failures. - No
panic = "abort"profile is configured, socatch_unwindwill work as expected with the default unwind strategy. - The test infrastructure addition (
errors_on_allowed_handles) is clean and reusable.
|
The review has been posted. Here's a summary of what was done: PR Review Complete: #2170 — fix(coprocessor): scheduler, catch panicReview Process
Verdict: APPROVED ✅The PR correctly wraps FHE computation in |
Changed Lines CoverageCoverage of added/modified lines: N/A |
|
clippy fails but otherwise, nice! lgtm |
b1716a2 to
7afe56e
Compare
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 1 hour 40 minutes 15 seconds in the queue, including 1 hour 39 minutes 43 seconds running CI. Required conditions to merge
|
No description provided.