[Feature] Improved Panic Handling

[kaimast]

This PR is a follow-up on #2813.

Context: Handling Fatal Errors in snarkVM

Generally, snarkVM and snarkOS aim to never panic if it is possible to avoid. However, there is always the possibility of bugs that were overlooked or ending up in a state where the code cannot recover safely.

If the code panics, there are two different scenarios:

• If the panic happens inside a program execution (i.e., inside try_vm_runtime) we can safely log it and continue operating normally. snarkVM will simply treat this as a failed execution.
• If the panic happens outside a program execution (i.e., anywhere else in snarkVM or in snarkOS), we most likely do not want to simply continue execution, and we definitely want to notify the user and developers about the error.
  We had several instances where a thread or tokio task panicked, and, while information about the panic was printed to stdout, there was no mention of it in the logs.

While the former is already implemented. The latter was partially introduced by #2813, but this PR adds some important improvement and fixes. The behavior of try_vm_runtime remains unchanged by this, albeit its implementation changes slightly.

Improved Panic Handling Mechanisms

Panic handling as implemented in #2813 (and also before but to a limited extent) has a major shortcoming: it did not work well with multi-threading. That is because the panic handler is set per-process but, while executing a snarkVM program another thread, unrelated to execution, might panic, and snarkVM may falsely log it as a VM error.

The proposed mechanism in this PR is to store backtraces and error messages in a thread-local variable. try_vm_runtime is changed to retrieve the error message from this thread-local variable.
Additionally, there is now a catch_unwind function that wraps around the function of the same name from the standard library and returns the error message and backtrace on error. The latter is useful when printing error messages about panics, for example, in snarkOS.

Utilities for Task Management

tokio allows propagating panics through JoinHandles. This PR also provides wrappers around tokio's spawn and spawn_blocking functions that contain the boilerplate for propagating such panics.
This replaces logic that before resided in snarkos-node-bft but will be useful to other crates.

Usage

In snarkVM

snarkVM has one notable instance of spawning a background task, the sequential ops thread (see the changes in #2975). 80c81d8 adds catch_unwind to that thread to ensure future panics in the storage thread are logged properly.

In snarkOS

For snarkOS, the plan is to use catch_unwind for important background tasks such as block synchronization. Again, this is to ensure that a panic in block synchronization does not happen silently and block synchronization stops for no apparent reason.

ProvableHQ/snarkOS#3874 contains the full set of proposed changes for snarkOS.

Notes

• For panic handling, there is a potential overhead of storing backtraces that might not always be needed. It is my understanding that panics do not happen very frequently in production, but if this understanding is wrong, please let me know.
• Panic handling for "detached" tokio task (those without JoinHandles) does not work well (yet). Future improvements to tokio will change this, but we also do not use detached tasks much.

[vicsn]

and snarkVM may falsely log it as VM error.

Is this the main and only usecase for the thread local panic logic of this PR? I'm trying to better understand whether the refactor is worth it at the moment.

I thought we printed the error in the VM panic handler. Therefore:

1. if it were to catch a non-VM panic, I'd assume we would clearly observe this in the logs. Clumsy, but no harm done.
2. if it were to catch a VM panic of the wróng thread, would there be a risk of any of the following:
   a. verification of the wrong transaction being logged as wrong?
   b. verification of the wrong transaction being returned as Err?
   c. the above being non-deterministic, and therefore creating a risk of a fork on the network?

[kaimast]

@vicsn I extended the PR description

[raychu86]

Don't all the finalize instances still call the macro try_vm_runtime! instead of errors::try_vm_runtime()?

This would call set_panic_hook multiple times rather than the one time you claim "Should be called exactly once."

[kaimast]

I clarified the documentation that it is safe to call set_panic_hook multiple times. It will just not have any effect after the first invocation. The most recent commit also adds a boolean flag that is set once the handler is installed, to make sure there is no performance impact of calling set_panic_hook multiple times.

It would be great if we could replace that macro eventually, but that would require users of snarkVM to call set_panic_hook themselves.

[raychu86]

Logic looks reasonable, would be nice to see more unit tests to ensure the top level (non-try_vm_runtime panics) are handled correctly.

[eranrund]

I've spent some time looking at this PR and some of the issues and PRs referenced by it.

I think there are a few issues being discussed/addressed here:

---

Capturing VM panics gracefully

The default panic handler is called when a panic happens and it prints the error (+ stacktrace). When running VM code we want to capture that error and alter how it gets printed. Right now this is accomplished by temporarily setting a panic hook, but since panic hooks are global across all threads, this can lead to unrelated panics being reported by the temporarily-installed hook (or a VM halt being reported by the wrong hook). This is explicitly described in a dedicated issue - #3327

To address this, the current PR switches from the existing set-and-restore to a panic hook that is installed once and never removed. The hook itself no longer prints anything; instead it stores the panic message and backtrace in a thread-local, and the code that caught the panic (try_vm_runtime, catch_unwind) retrieves it on a per-thread basis and decides how to report it.

Not swallowing panics

When a Tokio task panics, the panic is only observed if someone awaits the task's JoinHandle, which we don't do for most tasks. That results in panics going unnoticed, leaving the process in a degraded state. We want to systematically address all these places, so that panics propagate to the parent task, and either gracefully shut down the node or immediately exit it. My intuition is that we should crash when a panic occurs, since otherwise the node is left in an unknown state, where some tasks are still running while others have failed.

The work to address this needs to happen mostly in snarkOS. We basically can't just tokio::spawn without either awaiting the returned JoinHandle (somewhere), or wrapping the task in a catch_unwind so we can tell a panic happened and act accordingly (e.g. log and terminate).

Propagating panics

The PR provides some building blocks for propagating panics such that if one task awaits a child task and the child task panics, the parent task also panics (via resume_unwind). This is the stuff in utilities/src/task.rs - wrappers around tokio::spawn/spawn_blocking whose JoinHandle re-raises the child's panic when awaited. It could simplify some code once we started addressing the un-awaited spawned tasks's JoinHandles.

Capturing a stacktrace when a panic happens

catch_unwind does not let us obtain a stacktrace of where the panic happened; we can only get whatever was passed to panic!(). This PR uses a thread-local to capture this information in the panic hook, bundling it together with the panic message. This helps get more useful information when a propagated panic takes place - otherwise, once we propagate the panic (resume_unwind), the stacktrace is lost. This is because the stacktrace is really only available during panic hook execution, which is where it gets logged when the default panic hook is in place.

---

Some thoughts

One consequence of the approach in this PR worth calling out: since the installed hook never prints and never delegates to the previously-installed hook, the panic message/backtrace only surfaces when the panicking code is wrapped in try_vm_runtime/catch_unwind. Any unwrapped panic - e.g. a thread or task we haven't gotten to dealing with yet - becomes completely silent, which is arguably a regression on the "panics going unnoticed" problem.

I understand the motivation for capturing the message/backtrace as data: it lets the catcher log the panic through the proper channels (e.g. https://github.com/ProvableHQ/snarkOS/blob/d8125c9cb2e3ce22cb1a3b19c4385415d5c02e04/snarkos/main.rs#L114) rather than the hook printing raw to stderr. But I think we can have both: install the hook once, gate the "VM safely halted" handling on a thread-local "in VM runtime" flag, and delegate to the previous hook for all other panics. That way unwrapped panics keep the default print/backtrace behavior, and any host-installed hook keeps working. I have a prototype of this in #3333.

The two approaches could compose - the chained hook could also store the (message, backtrace) thread-local so catch_unwind propagation keeps working as in this PR - but I don't think we need to capture the backtrace at all if the hook is responsible for logging it. Since resume_unwind doesn't re-invoke the hook, a panic gets reported exactly once at the panic site, and propagation stays purely a control-flow concern. As it stands, this PR mixes panic reporting into the propagation path.

Where to go from here?

How we proceed depends on whether the approach in #3333 is accepted. If it is, what I think remains, is deciding how to handle unhandled panics in Tokio tasks. That breaks down into two decisions:

1. How to catch them - e.g. a central supervisor that is handed the JoinHandles of one-off tasks and awaits them, or a spawn wrapper with built-in catch_unwind (closer to what this PR proposes), or something along those lines.
2. What to do when one happens - my vote is log and abort: as mentioned in one of the comment threads above, restarting a node is easy, while diagnosing a node that is only partially running is challenging.