Run Litestream commands without a shell and surface failures - #76
Open
cole-robertson wants to merge 2 commits into
Open
cole-robertson wants to merge 2 commits into
cole-robertson wants to merge 2 commits into
Conversation
Commands.run joined argv into one shell string, ignored the exit status
and discarded stderr, so a failing command returned "" (or [] after
table parsing) and looked like success. Arguments with spaces broke, and
argv reached the shell unescaped.
The runner now uses Open3.popen3 with an argv array, raises
CommandFailedException with the exit status and stderr on failure, and
takes an explicit output mode: :table (the existing header/rows
parsing), :raw, or :json when a caller passes json: true (Litestream
>= 0.5). In JSON mode the two opt-in restore skips, which print one
logfmt line on stdout with exit 0, come back as {"skipped" => true,
"message" => ...} so callers can tell "did nothing" from data.
timeout: runs the command in its own process group and TERMs then KILLs
it on expiry, raising CommandTimeoutException with the child reaped.
The LITESTREAM_INSTALL_DIR note prints once per process.
This was referenced Sep 10, 2026
The async path gets its failures from the caller, but the foreground path ran litestream through IO.popen and never looked at $?. `rails litestream:replicate` therefore exited 0 when litestream failed to start, so a supervisor saw a clean stop rather than a crash to restart, and nothing on the way out said why. A signal is still a normal stop: replicate runs until something signals it, so only a non-zero exit raises. replicate's bare rescue re-wrapped every StandardError into a message built from the whole command line, which swallowed the exit status this adds, so CommandFailedException now passes through it untouched.
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.
First of two to bring the gem to Litestream 0.5. This one stands alone and fixes a bug that exists on 0.3.13 today.
Problem
Commands.runjoined argv into one shell string, ignored the exit status, and discarded stderr. A failing command returned""(or[]after table parsing) and looked like success; arguments with spaces broke; argv reached the shell unescaped.Reproduced against 0.5.17:
ltxon a database that is not in the config exits 1 withError: database not found in configon stderr and nothing on stdout. The old wrapper returned[], indistinguishable from "nothing replicated yet". The 0.3.13 binary uses the same exit and stderr conventions.Change
One process path:
Open3.popen3(*cmd, pgroup: true), argv array, no shell.CommandFailedExceptionwith the command, exit status, and stderr.:table(existing parsing),:raw, or:jsonwhen a caller passesjson: true(Litestream ≥ 0.5). Restore's two opt-in skips (-if-db-not-existson an existing output,-if-replica-existswith no backups) exit 0 and print one logfmt line on stdout even with-json; those return{"skipped" => true, "message" => ...}.timeout:kills the process group (TERM, then KILL after a one-second grace, unconditionally) on expiry and raisesCommandTimeoutExceptionwith the child reaped. The deadline also covers the pipe readers, so a descendant that outlives the direct child is caught.Process.spawnrejects Integers, which the old shell-string runner accepted and the README documents (--parallelism 10).LITESTREAM_INSTALL_DIRnote prints once per process.executeis gone; it only existed because failures were swallowed.replicatechecks its exit status too (second commit).rails litestream:replicateexits 0 today when litestream fails to start, so a supervisor sees a clean stop rather than a crash to restart. A signal is still a normal stop, sincereplicateruns until something signals it.Public method signatures and default return shapes are unchanged.
preparestill returns argv.Verified
TestRunnerclass drives a fake executable: table, JSON object/array, empty list, both skip lines, non-zero exit with stderr in the message, an argument containing a space, timeout with reap, timeout with a descendant that ignores TERM, an Integer option value, note printed once.databasesin table and JSON modes,restore -jsonreturning txid, the skip case, a failing restore raising with the stderr message, a 0.2 s timeout killing a sleeping fake with no orphan left.