Ensure runner id is safe for filenames & implement wait_for_connectivity #19
Conversation
We discovered a scenario where things would hang if the transport crashed before it could open the write pipe. This commit adds a wait_for_connectivity method that will wait for the transport to open the pipe for writing. If it doesn't happen within 5 seconds, the controller will exit. We also discovered that the runner id could contain characters that are not allowed in filenames. This commit adds a safe_filename method that will strip out any characters that are not allowed in filenames.
| @@ -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) | |||
There was a problem hiding this comment.
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 🤷🏻
| @@ -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? | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, note that the handling of the initial nil has been moved to wait_for connectivity.
| def safe_filename(filename) | ||
| filename | ||
| .gsub(/[\/\\:*?"<>|\n\r]+/, '_') | ||
| .gsub(/^\.+|\.+$/, '') | ||
| .strip[0, 255] | ||
| end |
There was a problem hiding this comment.
This handles runner_ids like foo/bar that wouldn't otherwise be suitable for the pipe filename.
| next sleep(0.1) if message.nil? | ||
|
|
||
| response = JSON.parse(message, symbolize_names: true) | ||
| @connectivity = true if response[:command] == "connected" |
There was a problem hiding this comment.
This is to be paired with a change in the backend that sends the "connected" command as soon as the socket connects.
Co-authored-by: Nate Vick <nate.vick@hint.io>
Co-authored-by: Nate Vick <nate.vick@hint.io>
benjaminwood
force-pushed
the
handle-early-transport-crashes
branch
2 times, most recently
from
d937ed4
to
bf1fc70
Compare
We havne't been able to reproduce it locally or in test, but it's happened on CI. See: /lib/selective/ruby/core/named_pipe.rb:38:in `write': closed stream (IOError) selective-ruby-core-71005fe4776b/lib/selective/ruby/core/named_pipe.rb:38:in `write' selective-ruby-core-71005fe4776b/lib/selective/ruby/core/controller.rb:193:in `kill_transport' selective-ruby-core-71005fe4776b/lib/selective/ruby/core/controller.rb:162:in `block in wait_for_connectivity' Co-authored-by: Nate Vick <nate.vick@hint.io>
benjaminwood
force-pushed
the
handle-early-transport-crashes
branch
from
bf1fc70
to
7d30468
Compare
🚀
We discovered a scenario where things would hang if the transport crashed before it could open the write pipe. This commit adds a wait_for_connectivity method that will wait for the transport to open the pipe for writing. If it doesn't happen within 5 seconds, the controller will exit.
We also discovered that the runner id could contain characters that are not allowed in filenames. This commit adds a safe_filename method that will strip out any characters that are not allowed in filenames.