Implement workaround for PanicException serialization - #3142
Conversation
Added a workaround for PanicException serialization failures by dynamically registering a module. This allows for successful pickle/unpickle operations.
|
sure I'll have a look |
|
Hey @doomedraven — thanks for chasing this down. I tried to reproduce the failure and ended up somewhere different: I couldn't get the stub approach to work, but I did find something in The wedge: a worker that dies before pickup orphans its task foreverpebble's per-task timeout only starts once a task is running. If the worker dies before picking the task up, pebble can't associate the dead worker with any task ( It isn't panic-specific, and that's what makes it worth fixing on its own. Any initializer failure does it (real pebble 5.1.0, one worker, one task): Note the asymmetry: a worker dying during a task is handled correctly — pebble reports The fix: reap tasks that were scheduled but never ran--- a/lib/cuckoo/core/processing_engine/pebble.py
+++ b/lib/cuckoo/core/processing_engine/pebble.py
@@
def __init__(self, task_fn, worker_init, source, parallel, timeout,
- max_tasks=0, max_count=0):
+ max_tasks=0, max_count=0, stall_grace=300):
super().__init__(task_fn, worker_init, source, parallel, timeout)
self.max_tasks = max_tasks
self.max_count = max_count
+ self.stall_grace = stall_grace
self._pending = {} # future -> task_id
+ self._scheduled_at = {} # future -> time.monotonic() when scheduled
def _done(self, future):
"""Pebble done-callback: fires in the pool's internal thread."""
task_id = self._pending.pop(future, None)
+ self._scheduled_at.pop(future, None)
+ def _reap_stalled(self):
+ """Fail tasks that were scheduled but never ran.
+
+ pebble only applies a task's timeout once that task is RUNNING. If the
+ worker dies before picking it up, pebble cannot associate the dead
+ worker with any task, so the future never resolves: the task never
+ runs, never times out, is never marked failed, and stays in
+ ``_pending`` forever. Both the scheduling loop (via the saturation
+ check) and the drain loop then spin indefinitely.
+
+ Anything still pending past ``timeout + stall_grace`` is therefore
+ presumed dead and failed explicitly."""
+ if not self.stall_grace or not self._pending:
+ return
+
+ deadline = self.timeout + self.stall_grace
+ now = time.monotonic()
+ for future in list(self._pending):
+ if now - self._scheduled_at.get(future, now) < deadline:
+ continue
+ task_id = self._pending.pop(future, None)
+ self._scheduled_at.pop(future, None)
+ log.error(
+ "[%s] Task never ran (worker died before pickup?); marking it failed after %ss",
+ task_id, deadline,
+ )
+ with suppress(Exception):
+ future.cancel()
+ if task_id is not None:
+ self.source.mark_failed(task_id)
+
def run(self):
@@
while not self.max_count or count < self.max_count:
+ # Fail anything that was scheduled but never picked up, so a
+ # dead worker can't wedge the saturation check below forever.
+ self._reap_stalled()
+
@@
self._pending[future] = task.id
+ self._scheduled_at[future] = time.monotonic()
future.add_done_callback(self._done)
@@
# Drain: wait for all in-flight tasks to finish before returning.
+ # Reap here too: a task orphaned by a dead worker would otherwise
+ # keep this loop spinning forever.
while self._pending:
+ self._reap_stalled()
time.sleep(0.2)
This needs one companion change, because @@ def _done(self, future):
- except (pebble.ProcessExpired, Exception) as error:
+ except BaseException as error:
+ # BaseException, not Exception: anything escaping this callback
+ # propagates into pebble's message-manager thread and kills it.
log.exception("[%s] Exception when processing task: %s", task_id, error)That one's worth having regardless. Tests —
|
|
well that fix worked for me, so at least for now i have my servers working, i will review on monday your suggestion, thank you for checking. the traceback just in case: |
|
yes your changes seems to work, i have pushed those + few other fixes to another things, thanks for review @wmetcalf |
|
now error with current code, so as is yara_x issue will see with Victor as im on latest version of rust/yara-x/python-yara-x |
|
Your Three follow-ups came out of chasing this, so I've put them against this branch rather than a cleanup PR that never happens — #3152. Three separate commits, drop any of them freely:
One thing worth knowing if you ever want to kill the per-scan straight to stderr from PyO3. Not worth trading a panic for that to save ~0.19ms/file. There's a comment at the cache site saying so. And on the yara-x side generally: I installed 1.19.0 and spent a while trying to make it misbehave — syntax errors, unknown modules, duplicate rules, malformed PE/ELF/Mach-O/dotnet input. It degrades cleanly into |
Added a workaround for PanicException serialization failures by dynamically registering a module. This allows for successful pickle/unpickle operations.
@wmetcalf can you review it? without this it just breaks process.py for me