op-batcher: exit process on criticial throttling RPC error #17924
Conversation
and wire up to fatal error condition for throttling loop
![almanax-ai[bot]](https://avatars.githubusercontent.com/in/934833?s=60&v=4){kind=small}
| func (l *BatchSubmitter) shutdownOnCriticalError(err error) { | ||
| l.Log.Error("Shutting down batcher on critical error", "err", err) | ||
| // Call closeApp to trigger process to exit (gracefully) | ||
| l.closeApp(err) | ||
| } |
There was a problem hiding this comment.
Unprotected nil closeApp invocation can cause panic
The new shutdownOnCriticalError method unconditionally calls l.closeApp(err), which is passed as nil in many test and example invocations. This will cause a runtime panic when a critical throttling RPC error occurs, leading to an unintended crash.
Add a nil check before calling l.closeApp(err), or default to a safe no-op function if closeApp is not provided.
Don't like this finding? Reply "dismiss" and it won't appear again in future scans.
| func (l *BatchSubmitter) shutdownOnCriticalError(err error) { | ||
| l.Log.Error("Shutting down batcher on critical error", "err", err) | ||
| // Call closeApp to trigger process to exit (gracefully) | ||
| l.closeApp(err) | ||
| } |
There was a problem hiding this comment.
Tests for this would be good; does it shut down? Is it graceful?
There was a problem hiding this comment.
I tested this manually so far, and yes it does shut down successfully. I'm not totally sure we want to write a full end to end test for this, because we would need to spawn the batcher in a subprocess, attach it to an rpc endpoint returning MethodNotFound, and check on that process terminating. We don't have tests like this for sending an interrupt to the process, which should have the same effect. I'll see if I can add a little bit more unit tests to shore this up.
There was a problem hiding this comment.
I extended an existing test here a57e224. I'm pretty happy with this level of testing, it shows the "closeApp" is called under the right conditions. Having that closeApp actually cause the process to exit is arguably out of scope, since this is generic behavior from op-service that we rely on for multiple services.
| } | ||
| } | ||
|
|
||
| func isCriticalThrottlingRPCError(err error) bool { |
There was a problem hiding this comment.
Tests for this would be good.
2 participants
Confirmed manually that with these changes, the process is exited instead of just stopping the "work" of the batcher only.