feat: forward user signals to builds/runs#1088
Conversation
github-actions
Bot
added
the
t-tooling
There was a problem hiding this comment.
Pull request overview
This PR improves Ctrl+C / signal handling in the CLI by forwarding user interrupts to spawned local processes and by aborting in-progress Apify platform Builds/Runs when the user interrupts the CLI.
Changes:
- Add
forwardSignalssupport toexecWithLog()to forward signals from the CLI to spawned child processes. - Introduce
useSignalHandler()(Disposable-based) and use it to abort platform Runs/Builds on SIGINT/SIGTERM/SIGHUP. - Update TS/ESLint configuration to support
Disposable/usingpatterns and underscore-prefixed unused bindings.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds ESNext.Disposable lib typing support. |
| src/lib/hooks/useSignalHandler.ts | New Disposable-based signal hook with optional terminal-line cleanup. |
| src/lib/hooks/useCLIVersionAssets.ts | Simplifies asset-name destructuring (relies on underscore unused-vars convention). |
| src/lib/exec.ts | Adds forwardSignals plumbing and process-level forwarding handlers. |
| src/lib/commands/run-on-cloud.ts | Aborts platform Runs on signals while streaming/waiting. |
| src/commands/run.ts | Enables signal forwarding for local Node/Python runs via execWithLog. |
| src/commands/builds/create.ts | Aborts platform Builds on signals while streaming build logs. |
| eslint.config.mjs | Allows underscore-prefixed unused vars for using _x = ... patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // While the log is streaming, forward interrupt signals to a | ||
| // platform-side abort so the build doesn't keep running after the | ||
| // user gives up waiting (Ctrl+C, SIGTERM from a parent process, | ||
| // SIGHUP from a closing terminal). The `using` binding guarantees | ||
| // the listener is removed when the block exits. | ||
| // | ||
| // `once: false` keeps the listener registered across repeated | ||
| // signals so a second Ctrl+C doesn't kill the CLI before the | ||
| // abort request finishes. The build abort API has no "gracefully" | ||
| // knob, so the first signal does the work and later signals are | ||
| // silent no-ops. | ||
| let abortAttempt = 0; | ||
|
|
||
| using _signalHandler = useSignalHandler({ | ||
| signals: ['SIGINT', 'SIGTERM', 'SIGHUP'], | ||
| once: false, | ||
| handler: async (signal) => { | ||
| abortAttempt += 1; | ||
|
|
||
| if (abortAttempt > 1) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
After the first abort attempt, subsequent signals are ignored (abortAttempt > 1 returns) while once: false keeps intercepting them. This can leave users unable to force-terminate the CLI if aborting/streaming hangs. Consider removing/disposing the signal handler (or letting later signals fall through) after sending the abort request.
| // While the log is streaming, forward interrupt signals to a | |
| // platform-side abort so the build doesn't keep running after the | |
| // user gives up waiting (Ctrl+C, SIGTERM from a parent process, | |
| // SIGHUP from a closing terminal). The `using` binding guarantees | |
| // the listener is removed when the block exits. | |
| // | |
| // `once: false` keeps the listener registered across repeated | |
| // signals so a second Ctrl+C doesn't kill the CLI before the | |
| // abort request finishes. The build abort API has no "gracefully" | |
| // knob, so the first signal does the work and later signals are | |
| // silent no-ops. | |
| let abortAttempt = 0; | |
| using _signalHandler = useSignalHandler({ | |
| signals: ['SIGINT', 'SIGTERM', 'SIGHUP'], | |
| once: false, | |
| handler: async (signal) => { | |
| abortAttempt += 1; | |
| if (abortAttempt > 1) { | |
| return; | |
| } | |
| // While the log is streaming, forward the first interrupt signal | |
| // to a platform-side abort so the build doesn't keep running after | |
| // the user gives up waiting (Ctrl+C, SIGTERM from a parent | |
| // process, SIGHUP from a closing terminal). The `using` binding | |
| // guarantees the listener is removed when the block exits, and | |
| // `once: true` ensures later signals fall through so the user can | |
| // still force-terminate the CLI if aborting or log streaming hangs. | |
| using _signalHandler = useSignalHandler({ | |
| signals: ['SIGINT', 'SIGTERM', 'SIGHUP'], | |
| once: true, | |
| handler: async (signal) => { |
| export function useSignalHandler({ | ||
| signals, | ||
| handler, | ||
| cleanTerminalLine = true, | ||
| once = true, | ||
| }: UseSignalHandlerInput): Disposable { | ||
| let disposed = false; |
There was a problem hiding this comment.
useSignalHandler is a new hook with non-trivial behavior (once vs persistent mode, disposal semantics, terminal line cleanup). There are existing hook-level Vitest tests in test/lib/hooks, so adding unit tests for registration/disposal and the once behavior would help prevent regressions.
| childProcess.kill(signal); | ||
| }; | ||
|
|
||
| process.on(signal, handler); |
There was a problem hiding this comment.
Signal forwarding registers process.on(signal, handler) listeners but never allows the default SIGINT behavior to kick in. This means repeated Ctrl+C will keep being intercepted and the CLI can become impossible to force-terminate if the child ignores the signal. Consider registering the listener with process.once(...) (or removing it after the first forward) so a second signal falls through to Node’s default termination behavior, matching typical CLI expectations.
| process.on(signal, handler); | |
| process.once(signal, handler); |
| const cleanupSignalHandlers: (() => void)[] = []; | ||
|
|
||
| if (forwardSignals?.length) { | ||
| for (const signal of forwardSignals) { | ||
| const handler = () => { | ||
| childProcess.kill(signal); | ||
| }; | ||
|
|
||
| process.on(signal, handler); | ||
| cleanupSignalHandlers.push(() => process.off(signal, handler)); | ||
| } | ||
| } |
There was a problem hiding this comment.
New forwardSignals behavior isn’t covered by tests. There are existing Vitest tests for execWithLog, so it would be good to add a test that simulates a signal and asserts the child receives it (and that handlers are cleaned up).
| if (abortAttempt > 2) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
With once: false, signals are continuously intercepted. After abortAttempt > 2 this handler becomes a no-op, so further Ctrl+C presses won’t terminate the CLI even if the abort request hangs (e.g. network issues). Consider disposing the handler (or re-raising the signal to self) after a certain point so the user can still force-exit.
|
Let's discuss whether to swallow signals forever or not -> leaning towards "its fine as is right now", the only thing I agree with is the execa signal ?? exitCode change |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
l2ysho
left a comment
l2ysho
left a comment
There was a problem hiding this comment.
gj @vladfrangu looks good,
btw I tried replicate that abort problem Copilot raised, but with now luck
Closes #631
Closes #885
Unfortunately Claude doesn't know how to not overdocument but it's fine