Don't report the RPC shutdown force-kill as a crash - #8537
Draft
jkschneider wants to merge 1 commit into
Draft
Conversation
`shutdown()` SIGTERMs the subprocess, waits 5s, and escalates to
`destroyForcibly()` if it hasn't exited. It then checked the exit code
against an allowlist of {0, 1, 143} — which covers the graceful SIGTERM
path but not 137, the exit code its own SIGKILL had just produced. A
command whose work had fully completed failed with "Rewrite RPC process
crashed with exit code: 137".
It fired only when the subprocess needed more than 5s to handle SIGTERM,
so it tracked machine load rather than anything about the command, and
presented as a random crash in a healthy RPC process.
Move the exit-code check inside the graceful branch instead of running it
after both. The force-kill branch no longer inspects an exit code it
caused, so a genuine external SIGKILL — an OOM killer kill arriving within
the grace period — is still surfaced, which allowlisting 137 would have
swallowed.
Also:
- The grace period is now settable via `setShutdownGracePeriod(Duration)`,
defaulting to the existing 5s.
- The message names what happened ("exited with code N in response to
shutdown") and points at the stderr log when one is configured, rather
than asserting a crash the caller cannot distinguish from a timeout.
- The exception is held in a local and thrown after the stderr drain join
rather than through it. That join exists to release the parent-side log
handle before `shutdown()` returns; skipping it leaked the handle on
Windows, which mattered little while the throw was spurious and matters
now that it is rare and real.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RewriteRpcProcess.shutdown()force-kills the RPC subprocess, then reports its own kill as a crash — failing commands that had already fully succeeded:Cause
shutdown()sends SIGTERM, waits 5 seconds, and escalates todestroyForcibly()if the subprocess hasn't exited. It then checked the exit code against an allowlist of{0, 1, 143}— which covers the graceful SIGTERM path but not 137 (128 + 9), the exit code its own SIGKILL had just produced two lines earlier.It only fires when the subprocess needs more than 5 seconds to handle SIGTERM, so it tracks machine load rather than anything about the command. That is what makes it expensive: it presents as a random crash in an RPC process that is in fact healthy, so the natural next step is to investigate the subprocess.
Fix
Move the exit-code check inside the graceful branch instead of running it after both branches. The force-kill branch no longer inspects an exit code it caused.
This is deliberately structural rather than adding 137 to the allowlist: a genuine external SIGKILL — an OOM-killer kill arriving within the grace period — is a real failure, and allowlisting 137 would have swallowed it. It still throws.
Three smaller things in the same method:
setShutdownGracePeriod(Duration), defaulting to the existing 5s.Rewrite RPC process exited with code N in response to shutdown, plus a stderr-log pointer when one is configured, instead of asserting a "crash" the caller cannot distinguish from a timeout. For 137 specifically it notes the SIGKILL came from outside Rewrite — which is now the only way to reach that message.shutdown()returns; throwing past it leaked the handle on Windows. That mattered little while the throw was spurious and matters now that it is rare and real.Tests
Two tests in
RewriteRpcProcessTest, both forking a JVM and synchronising on a readiness marker file so they cannot silently degrade into asserting about an ordinary JVM exit:shutdownDoesNotReportItsOwnForceKillAsFailure— the child blocks in a shutdown hook so it cannot exit on SIGTERM. Assertsshutdown()doesn't throw and that the child's exit code was actually 137, so the force-kill path is provably the one exercised.shutdownStillReportsAnExitCodeTheSubprocessChose— the childhalt(3)s from its shutdown hook. Asserts the throw still happens, carries the stderr log path, and that the drain thread was joined before it escaped.Both are
@DisabledOnOs(WINDOWS): thereProcess.destroy()maps toTerminateProcess, which a child can neither delay nor intercept, so neither scenario is constructible.Reintroducing the bug against these tests reproduces the report verbatim (
RuntimeException: Rewrite RPC process crashed with exit code: 137); reverting it turns them green.:rewrite-core:checkpasses.