Skip to content

Minor improvements - #1

Merged
akx merged 11 commits into
masterfrom
todo-fixes
Jan 19, 2026
Merged

Minor improvements#1
akx merged 11 commits into
masterfrom
todo-fixes

Conversation

@akx

@akx akx commented Jan 16, 2026

Copy link
Copy Markdown
Member

Asked a robot to review the code for possible improvements.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request introduces improvements to process management and signal handling in Carnival, focusing on proper process group termination and enhanced shutdown behavior.

Changes:

  • Improved process termination using process groups with os.killpg to properly terminate child processes
  • Added configurable graceful shutdown timeout via stop_timeout_ms parameter
  • Enhanced signal handling to support SIGHUP and track which signal triggered shutdown
  • Made init commands interruptible and async with shutdown support
  • Added utility function for formatting process exit statuses in logs

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/carnival/process.py Added process group termination using os.killpg, configurable stop timeout, better exit status logging
src/carnival/manager.py Enhanced signal handling to track signal source, made init commands async and interruptible
src/carnival/config.py Added stop_timeout_ms configuration field and as_command_line() helper method
src/carnival/async_utils.py New utility module for waiting on process or event completion
tests/test_process.py Updated tests to verify process group termination with os.killpg
tests/test_manager.py Updated test assertion for new signal logging message
tests/snapshots/test_config.ambr Updated snapshot to include new stop_timeout_ms field

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_process.py Outdated
Comment thread src/carnival/async_utils.py
Comment thread src/carnival/manager.py
Comment thread src/carnival/manager.py
Comment thread src/carnival/config.py
Comment thread src/carnival/async_utils.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/carnival/async_utils.py Outdated
Comment thread src/carnival/process.py
Comment thread src/carnival/config.py
Comment thread src/carnival/manager.py
Comment thread src/carnival/manager.py
Comment thread src/carnival/process.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/carnival/async_utils.py
@akx
akx merged commit 77c9008 into master Jan 19, 2026
10 checks passed
@akx
akx deleted the todo-fixes branch January 19, 2026 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants