Repository navigation
fix(rewrite): process wrapper prefixes - #3937
Conversation
Table-driven timeout/time/nice/nohup; unknown options and shell syntax before the command fall through to passthrough. Co-authored-by: YOMXXX <liguanchen@xiaomi.com>
Relay SIGINT/SIGTERM to the child, escalating to SIGKILL then re-raise so a stubborn child cannot hold rtk open.
a8c63c3 to
ea67c7e
Compare
pszymkowiak
left a comment
There was a problem hiding this comment.
Nice work on this one. The wrapper peeling is fail-closed where it counts: an option the table doesn't describe drops the rewrite rather than guessing which token is the command, and the option-form coverage (-k 5s, -k5s, --kill-after=5s, --, nice -10/+5) is the part that usually breaks on the first real user. Leaving stdbuf out is the right call and the README says why.
The signal relay is the half I looked at hardest, and it holds up. The handler only touches kill, signal, raise and atomics, so it stays async-signal-safe, and the drop order means the handlers come off before the child is reaped. signal_flush_test.rs tests the behaviour rather than the wiring, which is what makes it worth having.
Approving. Three things I'd like noted rather than fixed here, unless you disagree:
-
The relay installs on every
run_streaming, not just wrapped invocations, so Ctrl-C on any rtk command now behaves differently (child gets SIGINT twice, rtk survives to print). That's probably what we want, but it deserves a line in the CHANGELOG since the PR title doesn't hint at it. -
CHILD_PID/RELAYED/FINISHEDare process-wide, so this quietly assumes one streaming run at a time. True today, but nothing in the code says it has to stay true. A short comment would save someone a bad afternoon later. -
test_toml_filter_rewrites_bare_and_wrapped_invocationswidens the blast radius to user TOML filters, withis_fully_anchoredas the only thing keeping a filter off the wrapper itself. You documented it in the test comment, which is the right instinct - I'm just flagging it so it's visible in the review history.
Re-raise SIGINT/SIGTERM once output is printed so callers still see signal death, not a plain exit code.
Closes #2375. Supersedes #2568, whose wrapper set and peel-recurse-reprepend shape this builds on (@YOMXXX).
What changes
timeout 300 cargo testnow rewrites totimeout 300 rtk cargo test. Same fortime,nice,nohup, and path-qualified forms like/usr/bin/timeout.PROCESS_WRAPPERSinregistry.rsdescribes each wrapper's own arguments (value options, flag options, attached values like-k5sand-n10, positionals such as timeout's duration). One generic scanner walks them in order. Adding a wrapper is a table entry.Two rules keep it safe:
stdbufis excluded on purpose. It exists to get incremental output, and rtk buffers until the child exits, so rewriting it removes the reason to type it.Signal handling
Wrapping changes who gets signalled:
timeout 300 rtk cargo testsignals rtk, not cargo. Before this PR rtk died on the default disposition and every captured line was lost, which is worse than running unwrapped.core::streamnow relays SIGINT/SIGTERM to the child and lets the normal filter-and-print path finish. If the child does not exit, it escalates to SIGKILL after 1.5s, then restores the default disposition and re-raises after another 1.5s, so rtk can never be held open.How this was tested
Shim-based execution tests, with a fake
cargoon PATH that records its argv:timeout 2 cargo clippytimeout 2 rtk cargo clippybeforetimeout 2 rtk cargo clippyaftertimeout --foregroundno longer orphans the child.Differential fuzz against develop, 6,567 generated command lines through both binaries:
Shape check asserts a rewrite only ever inserts
rtktokens, never reorders or drops shell text. Rewriting is idempotent, so the hook can re-run over its own output.cargo fmt --all,cargo clippy --all-targets,cargo test --all: clean, 3341 unit tests plus all integration targets.tests/signal_flush_test.rsfails without the relay.Needs a decision
test_toml_filter_rewrites_bare_command_but_not_wrapped_invocationsasserted that a wrapped TOML-filtered command is never rewritten. That pinned the absence of wrapper peeling, which is what this PR adds, so it now asserts the wrapper-preserving rewrite and is renamed. What keeps a filter off the wrapper itself is theis_fully_anchoredgate onmatch_command, unchanged and still tested incore::toml_filter.