cre-5476: extra recovery (v2)#23132
Conversation
|
👋 mchain0, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Is it important to have a separate table for panics? Could they be recorded as a variant of the existing results?
There was a problem hiding this comment.
I guess you mean workflow_specs ? it's an independent lifecycle and semantic, separation of concerns and avoiding future db issues. imho simpler and more durable as separate
There was a problem hiding this comment.
I agree that workflow_specs is not fitting. Where do returned errors normally go?
There was a problem hiding this comment.
to logs and event. panic is not an erro. to avoid boot loop we need this persistence
There was a problem hiding this comment.
Do you mean that we don't persist the result of each execution already? I think that will be required for guaranteed execution, and it may be more natural to catalog these panics as a variant of execution failure. How about adding some columns for metadata that would allow us to bridge things later as the schema evolves? Maybe just execution ID? 🤔
|
| "github.com/smartcontractkit/chainlink-common/pkg/logger" | ||
| ) | ||
|
|
||
| // fakeModulePanicStore mimics job.KVStore: Get returns a wrapped sql.ErrNoRows |
There was a problem hiding this comment.
it mimics ModulePanicStore right? (and not job.KVStore)
|
@mchain0 IMO this feels like this is at the wrong level -- I agree we want some isolation against a similar class of bugs in the future, but it feels like this should be bound at the WASM module level, not at the engine level? I'm also not a fan of the extra state this introduces. What do you think of adding a recover around the wasmtime module's execute and just erroring the execution if it happens? That should put the recover at a very granular and targeted spot enforcing the invariant that a wasm execution should never panic. |
| } | ||
| count++ | ||
|
|
||
| var buf [8]byte |
There was a problem hiding this comment.
if this is a counter, why do you store it in a byte array and not into int directly? (this would avoid such conversions to byte array and back)
I agree, we should never panic. This is already a second iteration of this, here is the previous attempt: #23130 (imho much better) But @jmank88 disagree and express concern over the state. Please conclude on these concerns here. |
|
@jmank88 What concerns do you have? If we scope this narrowly to the wasm module execution, I don't think there should be any issue around the state, we would abort + throwaway any related state, but maybe I'm missing something? |
But it is not possible to know in the general case if a program is still in a healthy state after recovering from an arbitrary panic. |
4 participants
cre-5476
ref. it's an alternative implementation to #23130