From f8d60c1606d20288c57ba0e34e2b8484fcd73544 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 29 Sep 2026 15:15:59 -0700 Subject: [PATCH 1/6] Stop rbspy and its command when singed is terminated Closes #39. Kernel#system left sudo, rbspy and the profiled command running when singed was killed, for instance by a script stopping a backgrounded `singed -- bin/rails server`. Now singed passes SIGTERM on to rbspy as SIGINT, which sudo relays and is the only signal rbspy stops cleanly on: it kills the command and writes the flamegraph, and singed then opens it as usual. Only the first SIGTERM is passed on, because rbspy exits without writing anything when it's interrupted twice, and scripts can send more than one: the issue's runs its cleanup from both its signal and EXIT traps. For the same reason, singed ignores SIGINT while rbspy runs rather than passing it on, since Ctrl-C at a terminal already reaches rbspy. Ignoring it also means Ctrl-C no longer kills singed before it opens the flamegraph, when sudo runs rbspy without a pty. --- README.md | 2 + lib/singed/cli.rb | 32 ++++++++- spec/singed/cli_spec.rb | 140 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+), 2 deletions(-) create mode 100644 spec/singed/cli_spec.rb diff --git a/README.md b/README.md index 28832c6..ca2a906 100644 --- a/README.md +++ b/README.md @@ -196,6 +196,8 @@ $ bundle exec singed -- bin/rails runner 'Model.all.to_a' The flamegraph is opened afterwards. +To profile a command that runs until it's stopped, like a server, stop it with Ctrl-C. Or, when `singed` runs in the background, such as from a script, stop it with `kill`'s default SIGTERM. Either way, rbspy stops the command and writes the flamegraph, which `singed` then opens. + ## Limitations diff --git a/lib/singed/cli.rb b/lib/singed/cli.rb index e501618..30c70fa 100644 --- a/lib/singed/cli.rb +++ b/lib/singed/cli.rb @@ -163,7 +163,7 @@ def show_help? @show_help end - # Never nil or false: exception: true makes Kernel#system raise instead. + # Never nil or false: a command that fails raises instead. #: (Array[String | Integer | Pathname | nil], reason: String, ?env: Hash[String, String]) -> bool def sudo(system_args, reason:, env: {}) loop do @@ -183,7 +183,35 @@ def sudo(system_args, reason:, env: {}) # Sorbet can't check a splat of an array of unknown length: https://srb.help/7019 #: self as untyped - system(env, *sudo_args, exception: true) + pid = spawn(env, *sudo_args) + status = wait_passing_on_signals(pid) + raise "#{Shellwords.join(sudo_args)} failed (#{status})" unless status.success? + + true + end + + # Kernel#system would leave the command running when singed is killed. Instead, the first SIGTERM is + # passed on as SIGINT, which sudo relays and is the only signal rbspy stops cleanly on, killing the + # command and writing the flamegraph. Nothing more is passed on, because rbspy exits without writing + # anything when interrupted twice, and Ctrl-C at a terminal already reaches it directly. + # https://github.com/rbspy/rbspy/blob/v0.53.0/src/main.rs#L164-L211 + #: (Integer) -> Process::Status + def wait_passing_on_signals(pid) + interrupted = false #: bool + previous_int = trap("INT", "IGNORE") + previous_term = trap("TERM") do + Process.kill("INT", pid) unless interrupted + interrupted = true + rescue Errno::ESRCH + # It has already exited. + end + + # Process.wait2 only returns nil when told not to block. + waited = Process.wait2(pid) #: as !nil + waited.last + ensure + trap("INT", previous_int) + trap("TERM", previous_term) end #: () -> String? diff --git a/spec/singed/cli_spec.rb b/spec/singed/cli_spec.rb new file mode 100644 index 0000000..aab430e --- /dev/null +++ b/spec/singed/cli_spec.rb @@ -0,0 +1,140 @@ +# typed: false +# frozen_string_literal: true + +require "rbconfig" +require "singed/cli" + +# Runs exe/singed with stand-ins for sudo, for rbspy, and for the commands that open flamegraphs. +RSpec.describe Singed::CLI do + let(:dir) { Pathname(Dir.mktmpdir("singed-cli-spec")) } + let(:bin) { dir.join("bin").tap(&:mkpath) } + let(:interrupts) { dir.join("interrupts.log") } # a line for each SIGINT rbspy gets + let(:opened) { dir.join("opened.log") } + let(:output) { dir.join("output.log") } + let(:rbspy_exit_status) { nil } # for rbspy to fail with, straight away + let(:started) { dir.join("started") } # the profiled command's pid, once it runs + + # Writes the stand-ins first, so no path through the hooks runs singed with the real sudo. It runs as + # `bundle exec singed` would, but without this process's Bundler environment, whose BUNDLER_ORIG_PATH + # would have singed's Bundler.with_unbundled_env take the stand-ins back off PATH. And it runs in its + # own process group, so the after hook can clean up whatever singed leaves running. + let!(:singed) do + write_stand_ins + Bundler.with_unbundled_env do + Process.spawn( + { + "PATH" => "#{bin}:#{ENV.fetch('PATH')}", + "BUNDLE_GEMFILE" => File.expand_path("../../Gemfile", __dir__), + "RUBYOPT" => "-rbundler/setup", + }, + RbConfig.ruby, File.expand_path("../../exe/singed", __dir__), "--output-directory", dir.to_s, + "--", RbConfig.ruby, "-e", "File.write(ARGV[0], Process.pid.to_s); sleep", started.to_s, + chdir: dir.to_s, out: output.to_s, err: output.to_s, pgroup: true + ) + end + end + + after do + Process.kill("KILL", -singed) + rescue Errno::ESRCH, Errno::EPERM + # Everything has exited. macOS says EPERM when only an unreaped singed is left. + ensure + FileUtils.rm_rf(dir) + end + + def write_stand_ins + write_executable "sudo", <<~SH + #!/bin/sh + while [ "${1#-}" != "$1" ]; do shift; done + exec "$@" + SH + # Like rbspy, stops at the first SIGINT, then takes a moment to write the flamegraph, and exits + # without writing anything at a second SIGINT. + write_executable "rbspy", <<~RUBY + #!#{RbConfig.ruby} + #{"exit #{rbspy_exit_status}" if rbspy_exit_status} + require "json" + file = ARGV[ARGV.index("--file") + 1] + command = Process.spawn(*ARGV.drop(ARGV.index("--") + 1)) + trap("INT") do + File.write(#{interrupts.to_s.inspect}, "INT\\n", mode: "a") + exit!(1) if $interrupted + $interrupted = true + Process.kill("KILL", command) + end + Process.wait(command) + sleep 0.5 if $interrupted + File.write(file, JSON.generate(shared: { frames: [{ name: "
", file: "script.rb" }] }, profiles: [])) + RUBY + ["npx", "open", "xdg-open"].each do |opener| + write_executable opener, <<~SH + #!/bin/sh + echo "$0 $*" >> #{opened} + SH + end + end + + def write_executable(name, script) + bin.join(name).write(script) + bin.join(name).chmod(0o755) + end + + def eventually(timeout: 15) + deadline = Process.clock_gettime(Process::CLOCK_MONOTONIC) + timeout + until (result = yield) + raise "Timed out. singed's output:\n#{output.read}" if Process.clock_gettime(Process::CLOCK_MONOTONIC) > deadline + + sleep 0.02 + end + result + end + + def exit_status + eventually { Process.wait2(singed, Process::WNOHANG)&.last } + end + + it "stops rbspy and the profiled command when terminated, then opens the flamegraph" do + command = eventually { started.exist? && started.read.to_i.nonzero? } + Process.kill("TERM", singed) + + expect(exit_status).to be_success, output.read + expect(interrupts.read).to eq("INT\n") + expect { Process.kill(0, command) }.to raise_error(Errno::ESRCH) + expect(dir.glob("speedscope-cli-*.json")).not_to be_empty + expect(opened.read).not_to be_empty + end + + it "passes on only the first SIGTERM, so rbspy can finish writing the flamegraph" do + eventually { started.exist? } + Process.kill("TERM", singed) + eventually { interrupts.exist? } + Process.kill("TERM", singed) + + expect(exit_status).to be_success, output.read + expect(interrupts.read).to eq("INT\n") + expect(dir.glob("speedscope-cli-*.json")).not_to be_empty + end + + it "leaves Ctrl-C's SIGINT to reach rbspy from the terminal" do + eventually { started.exist? } + Process.kill("INT", singed) + sleep 0.5 + + expect(Process.wait2(singed, Process::WNOHANG)).to be_nil + expect(interrupts).not_to exist + + Process.kill("TERM", singed) + + expect(exit_status).to be_success, output.read + expect(interrupts.read).to eq("INT\n") + end + + context "when rbspy fails" do + let(:rbspy_exit_status) { 3 } + + it "fails too" do + expect(exit_status).not_to be_success + expect(output.read).to match(/rbspy record .* failed \(pid \d+ exit 3\)/) + end + end +end From 5039f936d45298fec282d0373f6a733f58a558a7 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 29 Sep 2026 15:16:00 -0700 Subject: [PATCH 2/6] Leave --rate off rbspy's command line unless it's given A nil value in the rbspy options makes a flag, so without `--rate` the CLI passed rbspy a bare `--rate`, which rbspy rejects: error: a value is required for '--rate ' but none was supplied Only pass it when it's set, so rbspy falls back to its default rate. --- lib/singed/cli.rb | 3 ++- spec/singed/cli_spec.rb | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/singed/cli.rb b/lib/singed/cli.rb index 30c70fa..ec51d89 100644 --- a/lib/singed/cli.rb +++ b/lib/singed/cli.rb @@ -87,12 +87,13 @@ def run ) @filename = Singed::Flamegraph.generate_filename(label: "cli") + # nil values are for flags. rbspy uses its default rate unless one was given. options = { format: "speedscope", file: filename.to_s, - rate: @rate, silent: nil, } + options[:rate] = @rate if @rate rbspy_args = [ "record", diff --git a/spec/singed/cli_spec.rb b/spec/singed/cli_spec.rb index aab430e..7415ac0 100644 --- a/spec/singed/cli_spec.rb +++ b/spec/singed/cli_spec.rb @@ -11,6 +11,7 @@ let(:interrupts) { dir.join("interrupts.log") } # a line for each SIGINT rbspy gets let(:opened) { dir.join("opened.log") } let(:output) { dir.join("output.log") } + let(:rbspy_args) { dir.join("rbspy_args.json") } let(:rbspy_exit_status) { nil } # for rbspy to fail with, straight away let(:started) { dir.join("started") } # the profiled command's pid, once it runs @@ -54,6 +55,7 @@ def write_stand_ins #!#{RbConfig.ruby} #{"exit #{rbspy_exit_status}" if rbspy_exit_status} require "json" + File.write(#{rbspy_args.to_s.inspect}, JSON.generate(ARGV)) file = ARGV[ARGV.index("--file") + 1] command = Process.spawn(*ARGV.drop(ARGV.index("--") + 1)) trap("INT") do @@ -99,6 +101,7 @@ def exit_status expect(exit_status).to be_success, output.read expect(interrupts.read).to eq("INT\n") + expect(JSON.parse(rbspy_args.read)).to start_with("record", "--format", "speedscope", "--file", a_string_ending_with(".json"), "--silent", "--") expect { Process.kill(0, command) }.to raise_error(Errno::ESRCH) expect(dir.glob("speedscope-cli-*.json")).not_to be_empty expect(opened.read).not_to be_empty From 1764d45b4df8b027cce03a66e090fe6ba3a6e905 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 29 Sep 2026 15:46:39 -0700 Subject: [PATCH 3/6] Release 0.4.0 Push Gem now releases when singed.gemspec's version changes, so bumping it releases this change once it merges. It's a minor bump because main has gained features since 0.3.0: profiling with vernier, the Sidekiq middleware, explicit start and stop methods and the vendored speedscope, and the minimum Ruby is now 3.3. Gemfile.lock records singed's version, so it changes too; otherwise bundle install would rewrite it and rake release would refuse to release from a dirty tree. --- Gemfile.lock | 2 +- singed.gemspec | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index d19a845..1825b8d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - singed (0.3.0) + singed (0.4.0) stackprof (>= 0.2.13) GEM diff --git a/singed.gemspec b/singed.gemspec index 050e805..27f09e5 100644 --- a/singed.gemspec +++ b/singed.gemspec @@ -3,7 +3,7 @@ Gem::Specification.new do |spec| spec.name = "singed" - spec.version = "0.3.0" + spec.version = "0.4.0" spec.license = "MIT" spec.authors = ["Josh Nichols"] spec.email = ["josh.nichols@gusto.com"] From 85b6c6f1c2e58c84d03bae4b561848ce6deece46 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 29 Sep 2026 16:04:35 -0700 Subject: [PATCH 4/6] Name Gusto Engineers as singed's authors --- singed.gemspec | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/singed.gemspec b/singed.gemspec index 27f09e5..35f08e9 100644 --- a/singed.gemspec +++ b/singed.gemspec @@ -5,8 +5,8 @@ Gem::Specification.new do |spec| spec.version = "0.4.0" spec.license = "MIT" - spec.authors = ["Josh Nichols"] - spec.email = ["josh.nichols@gusto.com"] + spec.authors = ['Gusto Engineers'] + spec.email = ['dev@gusto.com'] spec.summary = "Quick and easy way to get flamegraphs from a specific part of your code base" spec.required_ruby_version = ">= 3.3" spec.homepage = "https://github.com/rubyatscale/singed" From e67f9bccadc350289d532cdd1a88bfdb215b89c5 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 29 Sep 2026 17:27:37 -0700 Subject: [PATCH 5/6] Keep sudo type-checked and harden the CLI spec `#: self as untyped` binds self for the rest of the method, so Sorbet had stopped checking everything in `sudo` after the spawn, including the call to wait_passing_on_signals. Only the spawn is untyped now. trap returns nil for a handler installed outside Ruby, and trap(sig, nil) ignores the signal, so the handlers are restored as DEFAULT then. The spec waited 15 s for each step, which a loaded machine overran, so it waits up to 60 s. Every example now waits for the command's pid rather than for its file, which can exist before the pid is written. rbspy's stand-in traps SIGINT before it starts the command, as the examples signal once the command runs. It also checks that -r is passed on as --rate, and that nothing is opened when rbspy fails. TMPDIR keeps the page the bundled speedscope writes out of the system's temporary directory. The README now says that a kill -INT sent to singed alone is ignored. --- README.md | 2 +- lib/singed/cli.rb | 9 +++++---- spec/singed/cli_spec.rb | 38 ++++++++++++++++++++++++++++---------- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index ca2a906..b0446c2 100644 --- a/README.md +++ b/README.md @@ -196,7 +196,7 @@ $ bundle exec singed -- bin/rails runner 'Model.all.to_a' The flamegraph is opened afterwards. -To profile a command that runs until it's stopped, like a server, stop it with Ctrl-C. Or, when `singed` runs in the background, such as from a script, stop it with `kill`'s default SIGTERM. Either way, rbspy stops the command and writes the flamegraph, which `singed` then opens. +To profile a command that runs until it's stopped, like a server, stop it with Ctrl-C. Or, when `singed` runs in the background, such as from a script, stop it with `kill`'s default SIGTERM. Either way, rbspy stops the command and writes the flamegraph, which `singed` then opens. `singed` ignores a SIGINT sent to it alone, such as by `kill -INT`, because Ctrl-C's reaches rbspy directly. ## Limitations diff --git a/lib/singed/cli.rb b/lib/singed/cli.rb index ec51d89..711ed60 100644 --- a/lib/singed/cli.rb +++ b/lib/singed/cli.rb @@ -183,8 +183,8 @@ def sudo(system_args, reason:, env: {}) puts "$ #{Shellwords.join(sudo_args)}" # Sorbet can't check a splat of an array of unknown length: https://srb.help/7019 - #: self as untyped - pid = spawn(env, *sudo_args) + process = Process #: as untyped + pid = process.spawn(env, *sudo_args) #: as Integer status = wait_passing_on_signals(pid) raise "#{Shellwords.join(sudo_args)} failed (#{status})" unless status.success? @@ -211,8 +211,9 @@ def wait_passing_on_signals(pid) waited = Process.wait2(pid) #: as !nil waited.last ensure - trap("INT", previous_int) - trap("TERM", previous_term) + # trap returns nil for a handler installed outside Ruby, and restoring nil would ignore the signal. + trap("INT", previous_int || "DEFAULT") + trap("TERM", previous_term || "DEFAULT") end #: () -> String? diff --git a/spec/singed/cli_spec.rb b/spec/singed/cli_spec.rb index 7415ac0..81a5bbe 100644 --- a/spec/singed/cli_spec.rb +++ b/spec/singed/cli_spec.rb @@ -10,6 +10,7 @@ let(:bin) { dir.join("bin").tap(&:mkpath) } let(:interrupts) { dir.join("interrupts.log") } # a line for each SIGINT rbspy gets let(:opened) { dir.join("opened.log") } + let(:options) { [] } # singed's, before the command let(:output) { dir.join("output.log") } let(:rbspy_args) { dir.join("rbspy_args.json") } let(:rbspy_exit_status) { nil } # for rbspy to fail with, straight away @@ -18,7 +19,8 @@ # Writes the stand-ins first, so no path through the hooks runs singed with the real sudo. It runs as # `bundle exec singed` would, but without this process's Bundler environment, whose BUNDLER_ORIG_PATH # would have singed's Bundler.with_unbundled_env take the stand-ins back off PATH. And it runs in its - # own process group, so the after hook can clean up whatever singed leaves running. + # own process group, so the after hook can clean up whatever singed leaves running. TMPDIR keeps the + # page that the bundled speedscope writes to show the flamegraph in dir too. let!(:singed) do write_stand_ins Bundler.with_unbundled_env do @@ -27,8 +29,9 @@ "PATH" => "#{bin}:#{ENV.fetch('PATH')}", "BUNDLE_GEMFILE" => File.expand_path("../../Gemfile", __dir__), "RUBYOPT" => "-rbundler/setup", + "TMPDIR" => dir.to_s, }, - RbConfig.ruby, File.expand_path("../../exe/singed", __dir__), "--output-directory", dir.to_s, + RbConfig.ruby, File.expand_path("../../exe/singed", __dir__), "--output-directory", dir.to_s, *options, "--", RbConfig.ruby, "-e", "File.write(ARGV[0], Process.pid.to_s); sleep", started.to_s, chdir: dir.to_s, out: output.to_s, err: output.to_s, pgroup: true ) @@ -57,14 +60,14 @@ def write_stand_ins require "json" File.write(#{rbspy_args.to_s.inspect}, JSON.generate(ARGV)) file = ARGV[ARGV.index("--file") + 1] - command = Process.spawn(*ARGV.drop(ARGV.index("--") + 1)) trap("INT") do File.write(#{interrupts.to_s.inspect}, "INT\\n", mode: "a") exit!(1) if $interrupted $interrupted = true - Process.kill("KILL", command) + Process.kill("KILL", $command) end - Process.wait(command) + $command = Process.spawn(*ARGV.drop(ARGV.index("--") + 1)) + Process.wait($command) sleep 0.5 if $interrupted File.write(file, JSON.generate(shared: { frames: [{ name: "
", file: "script.rb" }] }, profiles: [])) RUBY @@ -81,7 +84,7 @@ def write_executable(name, script) bin.join(name).chmod(0o755) end - def eventually(timeout: 15) + def eventually(timeout: 60) deadline = Process.clock_gettime(Process::CLOCK_MONOTONIC) + timeout until (result = yield) raise "Timed out. singed's output:\n#{output.read}" if Process.clock_gettime(Process::CLOCK_MONOTONIC) > deadline @@ -91,12 +94,16 @@ def eventually(timeout: 15) result end + def command_pid + eventually { started.exist? && started.read.to_i.nonzero? } + end + def exit_status eventually { Process.wait2(singed, Process::WNOHANG)&.last } end it "stops rbspy and the profiled command when terminated, then opens the flamegraph" do - command = eventually { started.exist? && started.read.to_i.nonzero? } + command = command_pid Process.kill("TERM", singed) expect(exit_status).to be_success, output.read @@ -108,7 +115,7 @@ def exit_status end it "passes on only the first SIGTERM, so rbspy can finish writing the flamegraph" do - eventually { started.exist? } + command_pid Process.kill("TERM", singed) eventually { interrupts.exist? } Process.kill("TERM", singed) @@ -119,7 +126,7 @@ def exit_status end it "leaves Ctrl-C's SIGINT to reach rbspy from the terminal" do - eventually { started.exist? } + command_pid Process.kill("INT", singed) sleep 0.5 @@ -132,12 +139,23 @@ def exit_status expect(interrupts.read).to eq("INT\n") end + context "with a rate" do + let(:options) { ["--rate", "50"] } + + it "passes it on to rbspy" do + command_pid + + expect(JSON.parse(rbspy_args.read)).to start_with("record", "--format", "speedscope", "--file", a_string_ending_with(".json"), "--silent", "--rate", "50", "--") + end + end + context "when rbspy fails" do let(:rbspy_exit_status) { 3 } - it "fails too" do + it "fails too, without opening anything" do expect(exit_status).not_to be_success expect(output.read).to match(/rbspy record .* failed \(pid \d+ exit 3\)/) + expect(opened).not_to exist end end end From 6c9e309c58b906365477aa442ebabfe65d442856 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 29 Sep 2026 17:28:11 -0700 Subject: [PATCH 6/6] Interrupt rbspy through sudo before 1.9.13 too sudo before 1.9.13 doesn't relay a signal sent from its own process group, and singed is in that group, so that sudo stays in the terminal's foreground. With Ubuntu 22.04's sudo 1.9.9, singed's SIGINT was dropped, and singed then waited forever, ignoring SIGINT and SIGTERM. Now a kill in a process group of its own sends it. That kill fails while sudo briefly runs entirely as root, as it does starting up, and a later SIGTERM then tries again. https://github.com/sudo-project/sudo/commit/36742deec3041413af9b293706a64530321b96b5 rbspy can also exit within milliseconds of the SIGINT, after which singed restored its SIGTERM handler, so a second SIGTERM, like the one the issue's script sends a few milliseconds after the first, killed singed before it opened the flamegraph. Once rbspy has been interrupted, singed now ignores SIGTERM until it exits. The spec's sudo stand-in now drops signals from its own process group, as sudo 1.9.12 did, and an example sends a SIGTERM while the flamegraph opens. The README says to signal singed alone rather than its process group, since sudo relays a SIGTERM straight to rbspy, which then exits without writing anything, and that rbspy kills only the command itself. --- README.md | 2 ++ lib/singed/cli.rb | 23 ++++++++++++++++------- spec/singed/cli_spec.rb | 41 ++++++++++++++++++++++++++++++++++++----- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index b0446c2..926a8c0 100644 --- a/README.md +++ b/README.md @@ -198,6 +198,8 @@ The flamegraph is opened afterwards. To profile a command that runs until it's stopped, like a server, stop it with Ctrl-C. Or, when `singed` runs in the background, such as from a script, stop it with `kill`'s default SIGTERM. Either way, rbspy stops the command and writes the flamegraph, which `singed` then opens. `singed` ignores a SIGINT sent to it alone, such as by `kill -INT`, because Ctrl-C's reaches rbspy directly. +Send SIGTERM to `singed` alone, though, not to its whole process group as `kill -- -` and `timeout` without `--foreground` do: sudo passes a SIGTERM straight on to rbspy, which then exits without writing the flamegraph. And rbspy kills only the command itself, so processes the command started may be left running. + ## Limitations diff --git a/lib/singed/cli.rb b/lib/singed/cli.rb index 711ed60..b445a50 100644 --- a/lib/singed/cli.rb +++ b/lib/singed/cli.rb @@ -22,6 +22,7 @@ class CLI def initialize(argv) @argv = argv @opts = OptionParser.new #: OptionParser + @interrupted = false #: bool parse_argv! end @@ -194,17 +195,14 @@ def sudo(system_args, reason:, env: {}) # Kernel#system would leave the command running when singed is killed. Instead, the first SIGTERM is # passed on as SIGINT, which sudo relays and is the only signal rbspy stops cleanly on, killing the # command and writing the flamegraph. Nothing more is passed on, because rbspy exits without writing - # anything when interrupted twice, and Ctrl-C at a terminal already reaches it directly. + # anything when interrupted twice, and Ctrl-C at a terminal already reaches it directly. Later SIGTERMs + # are ignored until singed exits, so they can't stop it opening the flamegraph either. # https://github.com/rbspy/rbspy/blob/v0.53.0/src/main.rs#L164-L211 #: (Integer) -> Process::Status def wait_passing_on_signals(pid) - interrupted = false #: bool previous_int = trap("INT", "IGNORE") previous_term = trap("TERM") do - Process.kill("INT", pid) unless interrupted - interrupted = true - rescue Errno::ESRCH - # It has already exited. + @interrupted ||= interrupt(pid) end # Process.wait2 only returns nil when told not to block. @@ -213,7 +211,18 @@ def wait_passing_on_signals(pid) ensure # trap returns nil for a handler installed outside Ruby, and restoring nil would ignore the signal. trap("INT", previous_int || "DEFAULT") - trap("TERM", previous_term || "DEFAULT") + trap("TERM", previous_term || "DEFAULT") unless @interrupted + end + + # sudo before 1.9.13 doesn't relay a signal sent from its own process group, which singed shares to keep + # sudo in the terminal's foreground, so a kill in a group of its own sends it. kill fails while sudo + # briefly runs entirely as root, as it does starting up, and then a later SIGTERM tries again. + # https://github.com/sudo-project/sudo/commit/36742deec3041413af9b293706a64530321b96b5 + #: (Integer) -> bool + def interrupt(pid) + kill = Process.spawn("kill", "-INT", pid.to_s, pgroup: true, err: File::NULL) + waited = Process.wait2(kill) #: as !nil + !!waited.last.success? end #: () -> String? diff --git a/spec/singed/cli_spec.rb b/spec/singed/cli_spec.rb index 81a5bbe..2b02f3e 100644 --- a/spec/singed/cli_spec.rb +++ b/spec/singed/cli_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Singed::CLI do let(:dir) { Pathname(Dir.mktmpdir("singed-cli-spec")) } let(:bin) { dir.join("bin").tap(&:mkpath) } + let(:hold_open) { dir.join("hold_open") } # while it exists, opening the flamegraph doesn't finish let(:interrupts) { dir.join("interrupts.log") } # a line for each SIGINT rbspy gets let(:opened) { dir.join("opened.log") } let(:options) { [] } # singed's, before the command @@ -47,11 +48,29 @@ end def write_stand_ins - write_executable "sudo", <<~SH - #!/bin/sh - while [ "${1#-}" != "$1" ]; do shift; done - exec "$@" - SH + # Like sudo before 1.9.13, relays SIGINT and SIGTERM that another process sends, but not from a process + # in its own process group that's still running. It's Perl because Ruby can't tell who sent a signal. + # https://github.com/sudo-project/sudo/blob/SUDO_1_9_12p2/src/exec_nopty.c#L150-L170 + write_executable "sudo", <<~'PERL' + #!/usr/bin/env perl + use strict; + use warnings; + use POSIX (); + + shift @ARGV while @ARGV && $ARGV[0] =~ /^-/; + my $command; + for my $signal (POSIX::SIGINT, POSIX::SIGTERM) { + my $relay = sub { + my $sender = $_[1]{pid} or return; + kill $signal, $command if $command && getpgrp($sender) != getpgrp(0); + }; + POSIX::sigaction($signal, POSIX::SigAction->new($relay, POSIX::SigSet->new, POSIX::SA_SIGINFO)); + } + $command = fork // die "fork: $!"; + exec { $ARGV[0] } @ARGV or die "exec: $!" unless $command; + 1 until waitpid($command, 0) == $command; + exit($? & 127 ? 128 + ($? & 127) : $? >> 8); + PERL # Like rbspy, stops at the first SIGINT, then takes a moment to write the flamegraph, and exits # without writing anything at a second SIGINT. write_executable "rbspy", <<~RUBY @@ -75,6 +94,7 @@ def write_stand_ins write_executable opener, <<~SH #!/bin/sh echo "$0 $*" >> #{opened} + while [ -e #{hold_open} ]; do sleep 0.01; done SH end end @@ -125,6 +145,17 @@ def exit_status expect(dir.glob("speedscope-cli-*.json")).not_to be_empty end + it "ignores later SIGTERMs until it has opened the flamegraph" do + command_pid + FileUtils.touch(hold_open) + Process.kill("TERM", singed) + eventually { opened.exist? } + Process.kill("TERM", singed) + hold_open.delete + + expect(exit_status).to be_success, output.read + end + it "leaves Ctrl-C's SIGINT to reach rbspy from the terminal" do command_pid Process.kill("INT", singed)