Die of the signal instead of reporting an interrupt as a failure - #29
Merged
Conversation
A run a signal ends did not fail, so nothing about it is reported now: Execute checks whether a signal arrived before it looks at the error at all, discards the error unread, and re-raises the signal so a parent sees WIFSIGNALED rather than an exit status that only spells one. The watcher resets every registered signal at receipt, which is what lets a second signal end the process at once rather than waiting out the job cancellation. Resetting before recording is load-bearing: the other order leaves a window in which the re-raise lands in a channel nobody reads any more and the process hangs. Not reading the error is also what keeps an interrupted run from being called a timeout, so the exit code 124 it used to produce is unreachable without touching query.go, auth.go or internal/redash.
Its wording no longer has to read as an interruption on its own, now that the user never sees it, and the newline a cancelled prompt writes is still wanted for a different reason: nothing else ends that line. assertInterrupted follows, pinning the sentinel itself rather than text that has stopped being user-facing.
"Any other failure exits 1" stopped covering interrupts once they stopped being reported at all, and an agent reading only that would treat 130 as a failure worth retrying. The README gains the interrupt paragraph as a section of its own, since it is no longer only about timeouts.
A shell hands a background job SIGINT as SIG_IGN, signal.Reset puts that back, and Kill then reports success for a signal that is never delivered. Waiting for it indefinitely wedged the process for good — measured with a `for` loop over rdsh started with `&`, where the run outlived the interrupt and only a SIGTERM ended it. Waiting a bounded 500ms instead costs nothing when the signal does arrive, since the process dies inside the sleep, and leaves the undeliverable case exiting with the status a shell would have reported.
A buffered channel gives the ordering the reset relies on without any of the reasoning about atomics that the comment had to carry, and drops the pointer-to-local the store needed. Stopping the notification on the way out keeps the registration from outliving the run it was made for.
Cancelling first would let the command unwind and the receive run while the send was still pending, putting back the very report this removes. The ordinary-failure test now checks the message it claims to, naming the profile rather than only looking for the Error: prefix.
16 tasks
Merged
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
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.
Why
rdsh caught SIGINT and SIGTERM and then reported the interruption as a failure — differently on every path, and worst of all as a timeout:
query, polling a jobError: context canceled, exit1query, with the job cancellation hangingError: query timed out after 1m30s (use --timeout to allow more time): …, exit124, ten seconds laterdata-source listError: Get "http://…": interrupt signal received, exit1auth loginError: verification failed, nothing saved: …, exit1Ctrl-C is not a failure — someone asked the run to stop — and the
124row actively misled: a run interrupted a second and a half in claimed the 90 s deadline had expired and advised a longer--timeoutthat would have changed nothing. Two related problems went with it: a second Ctrl-C was swallowed, leaving the user stuck for up to the ten-second job-cancellation timeout with no way out, and rdsh always exited normally, so a parent sawWIFEXITEDand a shell loop over rdsh invocations kept going.curl,git,gh,python3andnodeall die of the signal and print nothing. rdsh cannot simply stop catching — it has a server-side job to cancel and a terminal to restore — so it catches, finishes that work, and re-raises.What
Executereplacessignal.NotifyContextwith an explicitsignal.Notifychannel.NotifyContextcannot support either half of this: its cancellation cause is an unexported type that is not anos.Signal, so the signal cannot be recovered, and it keeps the handler installed for the whole run.Executechecks for a signal before it looks at the error at all, and discards the error unread. That is what makes the124path unreachable on an interrupted run, with no change inquery.go,auth.goorinternal/redash.internal/cmd/signal_unix.gore-raises withsyscall.Kill;signal_other.go(Windows, wasm) returns128 + signum, andsignal_plan9.goexists only so the tree keeps building wheresyscall.Signaldoes not exist.go installis rdsh's only distribution channel, so all of these have to compile.Exit codes are otherwise unchanged:
0on success,124on a--timeoutexpiry nobody interrupted,1for every other failure.Two measured details worth calling out
The wait after
syscall.Killis bounded, not indefinite. The issue comment prescribesselect {}after theKill, sinceKillis notraise(3)and execution can continue past it. Blocking forever turns out to have its own failure: a shell hands a background job SIGINT asSIG_IGN,signal.Resetrestores that, andKillthen reports success for a signal that never arrives. Measured with aforloop started with&— rdsh outlived the interrupt entirely and only a SIGTERM ended it, which is worse than the bug being fixed. A/tmp/siginfoprobe confirmed the cause: a bash background job reportsinherited SIGINT ignored: true, a foreground onefalse. So the wait is 500 ms and then falls back to128 + signum; when the signal can arrive, the process dies inside the sleep.TestExecuteUndeliverableSignalStillExitspins it (verified red againstselect {}: the process never exited). The same applies to cflio/slio if they landselect {}.plan9 keeps compiling.
syscall.SIGTERMexists there but thesyscall.Signaltype does not, so typing the recorded signal assyscall.Signalinroot.gowould have broken a target that builds onmaintoday. Keeping the value as anos.Signaluntil the platform file converts it costs one small file and no distortion of the shared path.GOOS=windows,GOOS=plan9andGOOS=jsall build.Verified by hand
Re-raising kills the test process, so the signal-termination half is not observable from
go test. Against a stub Redash whose job never finishes (and, where noted, whose cancel endpoint hangs), driving the built binary:What was checked and what it showed
queryinterrupted while polling —WIFSIGNALED=True WTERMSIG=SIGINT, stdout and stderr both empty. Same with SIGTERM, re-raised as SIGTERM.124and no timeout message.Error: context canceled, exit1, and the loop ran all three iterations. After: the loop stops at the first iteration and the loop shell itself ends signalled (-2).echo $?cannot tell these apart — both spell130— which is why the check readsWIFSIGNALED.auth loginunder a pty, interrupted at the masked prompt: echo is off at the prompt and back on afterwards, the process dies of SIGINT, no profile is written, and nothing is reported. The pre-fix binary restores echo the same way but exits1.GOOS=windows GOARCH=amd64 go build ./..., plusplan9andjs/wasm.Tests
Execute-level tests re-execute the test binary as rdsh through aTestMainhelper branch, which is the only way to observe process termination: interruptedquery(SIGINT and SIGTERM) anddata-source list, a second signal during a hanging job cancellation after--timeouthas already expired (the124path, both signal orders), an undeliverable signal, an uninterrupted--timeoutexpiry (still124, still its message), and an ordinary failure (still1, still reported). Every one of them was confirmed red against the oldExecutefirst.assertInterruptedstill checks exit1, which remains true belowExecute, but now pins the sentinel itself rather than wording no user sees.Closes #26