Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure runner id is safe for filenames & implement wait_for_connectivity #19

Merged
merged 4 commits into from
Dec 6, 2023
Merged

Ensure runner id is safe for filenames & implement wait_for_connectivity #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7371371
Ensure runner id is safe for filenames & implement wait_for_connectivity
benjaminwood Dec 3, 2023
71005fe
Do not use return in thread (fix local jump error)
benjaminwood Dec 4, 2023
83e8bde
Exit with error status when transport process fails to start
benjaminwood Dec 5, 2023
7d30468
Also handle IOError when terminating the transport process
benjaminwood Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions lib/selective/ruby/core/controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def initialize(runner, debug: false, log: false)
@debug = debug
@runner = runner
@retries = 0
@runner_id = ENV.fetch("SELECTIVE_RUNNER_ID", generate_runner_id)
@runner_id = safe_filename(ENV.fetch("SELECTIVE_RUNNER_ID", generate_runner_id))
@logger = init_logger(log)
end

Expand All @@ -23,6 +23,7 @@ def start(reconnect: false)
@transport_pid = spawn_transport_process(reconnect ? transport_url + "&reconnect=true" : transport_url)

handle_termination_signals(transport_pid)
wait_for_connectivity
run_main_loop
rescue NamedPipe::PipeClosedError
retry!
Expand Down Expand Up @@ -66,8 +67,6 @@ def init_logger(enabled)
def run_main_loop
loop do
message = pipe.read
next sleep(0.1) if message.nil? || message.empty?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm removing this from run_main_loop because so far I've only been able to observe the nil message when we're starting up. I believe this happens when we're waiting for the transport to open the pipe for writing.

At one point in the past I observed receiving both nil and "" (empty string) multiple times, not just at initialization. But, I wasn't able to reproduce that here, so I think we need to rediscover what scenario causes that (if any). We should certainly do some more manual testing with this before release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, note that the handling of the initial nil has been moved to wait_for connectivity.


response = JSON.parse(message, symbolize_names: true)

@logger.info("Received Command: #{response}")
Expand Down Expand Up @@ -153,6 +152,31 @@ def spawn_transport_process(url)
end
end

def wait_for_connectivity
@connectivity = false

Thread.new do
sleep(5)
unless @connectivity
puts "Transport process failed to start. Exiting..."
kill_transport
exit(1)
end
end

loop do
message = pipe.read

# The message is nil until the transport opens the pipe
# for writing. So, we must handle that here.
next sleep(0.1) if message.nil?

response = JSON.parse(message, symbolize_names: true)
@connectivity = true if response[:command] == "connected"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to be paired with a change in the backend that sends the "connected" command as soon as the socket connects.

break
end
end

def handle_termination_signals(pid)
["INT", "TERM"].each do |signal|
Signal.trap(signal) do
Expand All @@ -175,7 +199,7 @@ def kill_transport(signal: "TERM")

sleep(1)
end
rescue NamedPipe::PipeClosedError
rescue NamedPipe::PipeClosedError, IOError
# If the pipe is close, move straight to killing
# it forcefully.
end
Expand Down Expand Up @@ -272,6 +296,13 @@ def debug?
@debug
end

def safe_filename(filename)
filename
.gsub(/[\/\\:*?"<>|\n\r]+/, '_')
.gsub(/^\.+|\.+$/, '')
.strip[0, 255]
end
Comment on lines +299 to +304
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handles runner_ids like foo/bar that wouldn't otherwise be suitable for the pipe filename.


def with_error_handling(include_header: true)
yield
rescue => e
Expand Down
1 change: 1 addition & 0 deletions spec/selective/ruby/core/controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
before do
allow(Process).to receive(:spawn).and_return(123)
allow(controller).to receive(:handle_termination_signals)
allow(controller).to receive(:wait_for_connectivity)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tricky method to test because it contains a blocking read that'll screw up the test run if we let it. Open to ideas about how we can test it. It is tested implicitly by running the tests with selective so 🤷🏻

allow(controller).to receive(:exit)
end

Expand Down
Loading