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>
Integration test reportCommit: 09154a1
Top 9 slowest tests (at least 2 minutes):
|
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
Approving. The refactor is clean, and CI passes. I left two non-blocking comments and one nit inline. Two nits about the PR description:
[nit] The description says this PR moves "the remaining hand-rolled first-signal handlers" and leaves "one implementation … instead of four". Other handlers still exist: experimental/postgres/cmd/signals.go, experimental/aitools/cmd/{query,batch,discover_schema}.go, cmd/apps/{dev,run_local}.go, cmd/genie/ask.go, and cmd/pipelines/show.go. The postgres watchInterruptSignals also drops a second Ctrl-C, as ssh connect did before this PR. Change the wording, or name a follow-up PR for these callers.
[nit] The commits have a Co-authored-by: Isaac trailer, but the PR body has no agent disclosure line. .agents/skills/pr-checklist/SKILL.md asks for this line at the end of the body. Add one line, for example _This PR was written with Isaac._.
| watchCtx, stop := notifyInterrupt(ctx) | ||
| // Catch the interrupt so the log stream unwinds and handleWatchResult | ||
| // prints resume guidance; the CLI root installs no signal handler of its own. | ||
| watchCtx, stop := process.NotifyContext(ctx, os.Interrupt) |
There was a problem hiding this comment.
[nice to have] process.NotifyContext cancels with a *process.SignalError cause, and net/http returns context.Cause(ctx) for some cancelled requests. This happens while a request dials or waits for a connection, and on HTTP/1.1. Then the error is Get "...": interrupt signal received, and errors.Is(err, context.Canceled) is false. handleWatchResult returns this raw error and does not print the resume guidance. The old context.WithCancel code always gave context.Canceled, so air run --watch and air logs (logs.go:121) change behavior. Add func (e *SignalError) Unwrap() error { return context.Canceled } to libs/process in #7050. With this change, the guidance comes back, and the libs/process and ssh client tests pass.
|
|
||
| sigCh := make(chan os.Signal, 1) | ||
| signal.Notify(sigCh, os.Interrupt, syscall.SIGHUP, syscall.SIGTERM) | ||
| ctx, stopSignals := process.NotifyContext(ctx, os.Interrupt, syscall.SIGHUP, syscall.SIGTERM) |
There was a problem hiding this comment.
[nice to have] Before this change, Run caught SIGINT, SIGHUP, and SIGTERM until the process exited. Now a second signal gets the default action and stops the CLI at once. When a user closes the terminal, the CLI gets two SIGHUPs. The shell sends one to its jobs, and the kernel sends one when the session leader exits. The second SIGHUP stops the CLI before cmd/root uploads the SshTunnelEvent that the outcome defer logged. Thus the description is not correct when it says the tunnel event is "logged and uploaded". Keep the old handler for ssh connect. Or ignore SIGHUP after the first signal (signal.Ignore(syscall.SIGHUP)), so that only a second Ctrl-C stops the CLI.
| cancel() | ||
| <-ctx.Done() | ||
| if _, ok := errors.AsType[*process.SignalError](context.Cause(ctx)); ok { | ||
| cmdio.LogString(ctx, "Received termination signal, cleaning up...") |
There was a problem hiding this comment.
[nit] The goroutine now prints "Received termination signal, cleaning up..." after cancel, and nothing waits for it. The line can come after the error that Run returns. When stopSignals reports a late signal, the line comes after Run ends. The goroutine also wakes on every return, because stopSignals always cancels ctx. Add a short comment about this order and about why the goroutine checks the cause.
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.