feat(upload): implement graceful shutdown handling for script and server #1190
Conversation
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
📝 WalkthroughWalkthroughAdds global shutdown state, signal handlers, a shutdown event, and web UI server lifecycle management; runs Waitress in a background thread for Changes
Sequence DiagramsequenceDiagram
participant Main as Main Process
participant Sig as Signal Handler
participant Web as Web UI Server (thread)
participant Event as Shutdown Event
Main->>Main: _reset_shutdown_state() & register signal handlers
Main->>Web: start Waitress server thread (if -webui)
Web->>Main: display access URL
Note over Main,Web: serving requests
Sig->>Event: receive SIGINT/SIGTERM -> set shutdown event
Event->>Web: notify to stop
Web->>Web: graceful shutdown, stop serving
Event->>Main: allow cleanup to proceed
Main->>Main: perform conditional async cleanup / timeout
Main->>Main: print "Shutdown complete" and exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Code Analysis Results🔍 Pyright Type Checking🧹 Ruff Lint🔒 Bandit Security AnalysisThis comment was automatically generated by the CI pipeline. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@upload.py`:
- Around line 75-97: The custom signal handler _handle_shutdown_signal currently
swallows the first SIGINT which prevents asyncio.run() from receiving
KeyboardInterrupt; update _handle_shutdown_signal so that for SIGINT when not
running webui (i.e., _webui_server is None) you delegate to the default INT
handler or re-raise KeyboardInterrupt instead of just setting _shutdown_event,
while preserving the graceful shutdown path for webui runs (keep setting
_shutdown_requested, _shutdown_event and closing _webui_server when present);
ensure SIGTERM behavior remains the graceful shutdown path and only call
signal.default_int_handler(signum, frame) or raise SystemExit/KeyboardInterrupt
for non-webui SIGINT to allow asyncio task cancellation.
🔍 Code Analysis Results🔍 Pyright Type Checking🧹 Ruff Lint🔒 Bandit Security AnalysisThis comment was automatically generated by the CI pipeline. |
…waitress import. fix for related Code Analysis Results
🔍 Code Analysis Results🧹 Ruff LintThis comment was automatically generated by the CI pipeline. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@upload.py`:
- Around line 1336-1347: The current shutdown loop only checks _shutdown_event
and can wait forever if server_thread (running _webui_server.run) dies silently;
modify the loop to also check server_thread.is_alive() each iteration and if the
thread is no longer alive, break the wait and surface the error (e.g., log/raise
an exception or set _shutdown_event) so shutdown proceeds; after detecting the
thread death, ensure _webui_server.close() and server_thread.join(timeout=5.0)
are still called to perform graceful cleanup.
… UI server thread exits unexpectedly
🔍 Code Analysis Results🧹 Ruff LintThis comment was automatically generated by the CI pipeline. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Add proper signal handling (SIGINT/SIGTERM) for a clean shutdown
Fix the WebUI server to respond to a single Ctrl+C
Should support Docker docker stop graceful shutdown
Improve startup/shutdown messages with a clickable URL
before
After
webui "Kill and Clear" requires some rework on the UI side or server side to properly handle the kill event during execution, as it currently struggles to do so(when run in same proces without the use of UA_WEBUI_USE_SUBPROCESS).
As that's corner cases, even not sure if we should redo it or just adjust UI to block kill button, while process running(eg, wait till we get to some point, like confirmation ask)
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.