-
Notifications
You must be signed in to change notification settings - Fork 6
Implement function and (re)implement SIGHUP reload with full draining. #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there's a race condition. I didn't see the SIGHUP handler is protected to avoid several threads to run it.
|
||
try: | ||
# Acquire execution lock for the actual execution | ||
with self.execution_lock: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lock around execution. Does that mean a single tool can execute in a given moment? (I'm asking because I know there was some work in using a duckdb connection pool and we usually craft python tools to deal with concurrency.)
timeout: Maximum time to wait in seconds | ||
""" | ||
logger.info("Starting request draining...") | ||
self.server.draining = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That flag is also read in _execute_with_draining
.
Thinking of a scenario where:
- thread 1
_execute_with_draining
sees the flag is currentlyFalse
and moves to the next line, - then thread 2
_drain_requests
sets it to True, calls_get_active_requests
, finds zero, breaks and return, - then thread 1 registers as an active request, setting it the number of active requests to 1,
- then thread 2 executes the DB reload in a context where active request is 1.
At the same time, MCP tools execution seems to be protected by a lock, and reloading the DB is using it too. So maybe that works.
Now if anyway only one execution (or reload) can be performed in a given time, wouldn't it be enough to rely on the execution lock?
Raises: | ||
RuntimeError: If runtime is shutting down | ||
""" | ||
if self._shutdown: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a possibility that self._shutdown
is False
during a call to get_connection
, then method shutdown
sets it to True
and closes the connection pool. Then get_connection
continues and tries to acquire a connection in a moment it shouldn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or:
- thread calling
get_connection
gets a connection (session = self._pool.get()
), hasn't yet added itself to the_active_sessions
- thread calling
shutdown
sets the flag toTrue
, doesn't find active sessions, switches off plugins - the other thread registers itself to
_active_sessions
and executes something that uses a plugin.
Description
Implement function and (re)implement SIGHUP reload with full draining.
In the past, SIHGUP refreshed the DuckDB connection and reloaded Python hooks.
However, it did not drain existing requests, so running Python code could be affected.
This provides both a fix to SIGHUP, by properly Dra
Type of Change
Testing
uv run pytest
uv run ruff check .
uv run black --check .
uv run mypy .
Security Considerations
Additional Notes
Also fixed Discord invite.
Also fixed mxcp bootstrap to generate all folders.