Skip to content

Conversation

@jssmith
Copy link
Contributor

@jssmith jssmith commented Jan 20, 2026

Summary

Simplify the shutdown handling by replacing complex signal-based shutdown with a cleaner try/finally approach.

Problem

The previous implementation used custom signal handlers that called sys.exit() from async code, which could cause:

  • "asyncio.run() cannot be called from a running event loop" errors
  • Messy tracebacks on Ctrl+C
  • Complex shutdown logic that was hard to reason about

Solution

  • Remove custom signal handlers that called sys.exit() from async code
  • Use try/finally in main() to ensure cleanup() is always called
  • Handle KeyboardInterrupt in __init__.py to suppress traceback on Ctrl+C
  • Remove unused imports (asyncio, signal, sys) from server.py
  • Rename shutdown() to cleanup() to reflect its simpler purpose

This approach:

  • Lets Python's default signal handling work naturally
  • Ensures database connections are closed on any exit path
  • Is simpler and more maintainable

Attribution

Based on work by @ahmedmustahid in #78. This PR extracts and simplifies just the shutdown handling portion.

Test plan

  • All existing tests pass (189 passed)
  • New tests verify cleanup is called on normal exit and on exception
  • Tests verify cleanup handles database close errors gracefully

🤖 Generated with Claude Code

Replace complex signal-based shutdown with simpler try/finally cleanup:

- Remove custom signal handlers that called sys.exit() from async code
- Use try/finally in main() to ensure cleanup() is always called
- Handle KeyboardInterrupt in __init__.py to suppress traceback on Ctrl+C
- Remove unused imports (asyncio, signal, sys) from server.py
- Rename shutdown() to cleanup() to reflect its simpler purpose

This approach:
- Lets Python's default signal handling work naturally
- Avoids "asyncio.run() cannot be called from a running event loop" errors
- Ensures database connections are closed on any exit path
- Is simpler and more maintainable

Based on work by @ahmedmustahid in #78.

Co-Authored-By: Ahmed Mustahid <ahmedmustahid@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d00d517313

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +640 to +642
finally:
# Clean up database connections on exit
await cleanup()

Choose a reason for hiding this comment

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

P2 Badge Ensure cleanup runs under cancellation

When the server is interrupted (SIGINT/SIGTERM), asyncio.run() cancels the main() task; that cancellation propagates into the finally block and will cancel the awaited cleanup() call unless it is shielded or the CancelledError is handled. In that case, db_connection.close() may never run, so database connections can remain open on Ctrl+C despite the new shutdown simplification. Consider shielding cleanup() or catching CancelledError so cleanup completes even under cancellation.

Useful? React with 👍 / 👎.

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