Repository navigation
Conversation
Both carried a near-copy of process.NotifyContext: cancel on the first signal, then restore the previous disposition so a second one gets its usual handling. Replace them with the helper and drop their second-signal regression tests, which duplicate the one in libs/process. User-visible signal handling is unchanged. The returned stop now always uninstalls the handler and waits for its goroutine to exit, which air's copy did not. Co-authored-by: Isaac <no-reply@databricks.com>
ssh connect's signal handler never stopped relaying after the first signal, so every later Ctrl-C, SIGHUP, or SIGTERM was dropped and a hung teardown could only be ended with SIGKILL. Nothing after the first signal needs protecting: the ssh child is killed and the tunnel event is logged and uploaded. Use process.NotifyContext so a second signal gets its usual handling, and keep the "cleaning up" message for signals only by checking the cancellation cause. Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: 09154a1
Top 9 slowest tests (at least 2 minutes):
|
This branch has not been deployed
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
2 participants
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.
Changes
Stacked on #7050, which added
process.NotifyContextinlibs/processand used it inssh agent-shim.Moves the remaining hand-rolled first-signal handlers over to the shared helper:
cmd/air(run,logs) andcmd/environments(setup-local) each carried a near-copy ofprocess.NotifyContext: cancel on the first signal, then restore the previous disposition so a second one still gets its usual handling. Both now use the helper, and their second-signal regression tests are dropped as duplicates of the one inlibs/process. User-visible behaviour is unchanged;stopnow always uninstalls the handler and waits for its goroutine to exit, which air's copy did not.ssh connect(experimental/ssh/internal/client/client.goRun) never stopped relaying after the first signal, so every later Ctrl-C, SIGHUP, or SIGTERM was silently dropped and a hung teardown could only be ended withkill -9. Nothing after the first signal needs protecting (thesshchild is killed and the tunnel event is logged and uploaded), so a second signal now gets its usual handling. The "Received termination signal, cleaning up..." message is kept, and is printed only for a signal (checked via the*process.SignalErrorcancellation cause), not when the caller cancels.Why
One implementation of the first-signal cancellation pattern instead of four, and a working second-signal escape hatch for
ssh connect.Tests
Existing tests; the second-signal behaviour is covered by
libs/process/signal_unix_test.go.