Add more exit codes to detect interruption reason #764
Conversation
Store interupt reason directly instead of interrupted + browser crashed flags, use it to infer proper exit code, and more exit code, one per interruption reason. Other small changes: - Rename `InterruptedGraceful` exit code into `Cancelled` for clarity / consistency with redis operation - Rename `InterruptedImmediate` exit code to `Interrupted` since there is no more confusion possible with `Cancelled` and other graceful interuptions have their own exit codes - fix handling of SIGINT in serializeAndExit for which there is no interrupted value positioned
benoit74
force-pushed
the
more_exit_codes
branch
from
6d786f0 to
8f326d3
Compare
|
Thanks, overall looks good! I actually have another PR that adds a conflict with this, but will clean it up.
It's possible for interrupted to be true, if an interrupt request was made, but the crawler has not yet exited. |
- only attempt to close browser if not browser crashed - add timeout for browser.close() - ensure browser crash results in healthchecker failure - bump to 1.5.3
…wler into benoit74-more_exit_codes
- keep browser.crashed flag in Browser, skip close if crashed - healthchecker checks browser.crash - keep markBrowserCrashed()
|
Nice to see this improvement! It would be great to document these exit codes in the crawler documentation so that folks don't have to dig into the code to understand what the codes mean. |
Indeed, my bad.
Definitely, will tackle this as well. Probably in a distinct PR to move this code change asap to release, should documentation take more discussions than expected ... Where should this be placed? Should we create a new |
Thank you! Much appreciated :) I think a new Exit Codes section would be the simplest/most easily discovered way to document this. |
|
Yep, looks good! I merged some changes that readded browser.crashed, which is also used by healthchecker. Can add docs in a follow-up, this should be good to go! |
src/main.ts
Outdated
| crawler.gracefulFinishOnInterrupt(); | ||
| if (!crawler.interruptReason) { | ||
| logger.info("SIGNAL: interrupt request received, finishing current pages before exit..."); | ||
| crawler.gracefulFinishOnInterrupt(InterruptReason.Cancelled); |
There was a problem hiding this comment.
Actually, this isn't quite correct - this isn't necessarily cancelled.
When we run in k8s, interrupt does not mean to cancel the crawl, it means to stop and possibly restart the crawler..
The signal could come from user or from k8s system (such as memory pressure, etc...)
|
Ah but there is some confusion but cancellation / interruption. However, this does not mean the crawl is cancelled, esp. in k8s, but that the crawler needs to be restarted (for whatever reason). It could mean that its being moved to a different node, etc.. This is very important that the deletion of pod does not actually cancel the crawl automatically. The 'cancel' state is only entered through a special message sent via redis. In This is slightly more confusing because need to support different environments, both k8s and plain docker/podman Note: It might make sense to revisit cancellation to make it clearer, but than can be done as follow-up. |
…erruptForce for first and subsequent signal interruptions
Fix #584
Following recent changes in handling of exit code, and as suggested, I propose to refine exit codes a bit further, especially to be better informed of situations where we would like to indicate something to the end user in Kiwix usage.
I replaced
interruptedandbrowserCrashedproperties of the crawler with a singleinterrupt_reasonproperty, whose value is an enum indicating (when possible) the interruption reason.I slightly changed the message on cancellation since there was now a repetition of "gracefully finishing current pages".
These changes made me realize that there was maybe a flaw in the
serializeAndExitfunction since according to my code analysis theinterruptedproperty is never set when this function is called, unless I miss something.Tested locally and works as expected, some cases (reasonably easily feasible ones) covered by automated tests.
There is finally a small fix of documentation.