feat(client): workerchild awaits TaskProducer futures#642
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ```bash | ||
| # Generate 5 tasks | ||
| python src/examples/cli.py generate --count 5 | ||
| python src/examples/cli.py spawn --count 5 |
There was a problem hiding this comment.
Unrelated docs fix, spawn is the correct command
| local_shutdown.set() | ||
|
|
||
| signal.signal(signal.SIGTERM, handle_sigterm) | ||
| signal.signal(signal.SIGINT, signal.SIG_IGN) |
There was a problem hiding this comment.
SIGINT is already handled in the worker parent process, which uses the standard shutdown path for child processes, ensuring they drain pending futures. This means we can ignore SIGINT in the worker child processes.
There was a problem hiding this comment.
Not sure I follow the rationale. If the parent process is already managing the shutdown triggering the shutdown on the children why do we need to intercept sigterm here ?
It should be up to the main process to tell the children when to shut down. That would make the process a lot easier to follow and more deterministic.
There was a problem hiding this comment.
It should be up to the main process to tell the children when to shut down
I agree, my understanding is that when the main process receives SIGINT it propagates the signal to all child processes as well (if I'm understanding this thread correctly). By ignoring the signal in child processes, we allow the main process to shut down all child processes when getting a SIGINT (via the standard shutdown process).
I'm not sure when SIGINT is sent other than when using ctrl + c in a dev environment, so this probably isn't a huge deal and I can remove this if you'd prefer.
There was a problem hiding this comment.
I'm not sure when SIGINT is sent other than when using ctrl + c in a dev environment, so this probably isn't a huge deal and I can remove this if you'd prefer.
I think that is the main scenario it gets used. K8s uses SIGTERM.
There was a problem hiding this comment.
Broken out into _task_execution_complete() below
| app.load_modules() | ||
| metrics = app.metrics | ||
| # Signals when the parent worker pool terminates the child | ||
| local_shutdown = threading.Event() |
There was a problem hiding this comment.
what is the difference to shutdown_event?
There was a problem hiding this comment.
If I'm understanding this right, shutdown_event is shared across all worker child processes in the workerpool: https://github.com/getsentry/taskbroker/blob/main/clients/python/src/taskbroker_client/worker/worker.py#L633
I didn't want to trigger shutdown on all child processes at once by setting shutdown_event in a single child process, so each worker child uses local_shutdown when getting SIGTERM.
There was a problem hiding this comment.
ah shutdown_event is a multiprocessing event, nevermind
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 83e7339. Configure here.
bmckerry
force-pushed
the
ben/workerchild-await-futures
branch
from
6e0120d to
ebbb5e3
Compare
| local_shutdown.set() | ||
|
|
||
| signal.signal(signal.SIGTERM, handle_sigterm) | ||
| signal.signal(signal.SIGINT, signal.SIG_IGN) |
There was a problem hiding this comment.
I'm not sure when SIGINT is sent other than when using ctrl + c in a dev environment, so this probably isn't a huge deal and I can remove this if you'd prefer.
I think that is the main scenario it gets used. K8s uses SIGTERM.
4 participants
This PR updates the worker child process to:
TaskProducerrun_workerloopProcessingResultfor a task that has pending futures once all futures in the task are complete