Skip to content

feat(taskworker): Restore autoreload for the local taskworker - #128673

Merged
alexsohn1126 merged 7 commits into
masterfrom
alexsohn/taskworker-autoreload-reland
Oct 23, 2026
Merged

alexsohn1126 merged 7 commits into
masterfrom
alexsohn/taskworker-autoreload-reland

Conversation

@alexsohn1126

@alexsohn1126 alexsohn1126 commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

This re-lands #128564, which was reverted in 3e7ab78 because it broke a test on master.

That test makes sure starting the Sentry command line doesn't import any package that only dev installs have. The original PR imported the file watching library at the top of the file. That library pulls in another one, which tries to load an optional package that only dev installs have. Production doesn't have that package, and the library handles it being missing, so prod was fine. But the test blocks it with a slightly different error than the library expects, so the test failed. Selective testing didn't pick that test on the original PR, which is why its CI was green.

The fix is to import the file watcher only when the worker is started with autoreload, so normal startup never loads it. While re-testing, saving a file several times a second also turned up a bug in the original. The worker told the parent it was ready to be restarted a bit too early, before it had set up its handler for the stop request. During a burst of saves the stop request landed in that gap, so the worker skipped its normal shutdown, hung, and got force-killed, leaving its child processes running forever. Now it only says it's ready once that handler is in place. Stopping the worker while a new one was still starting also printed tracebacks and exited with an error, so that's quiet now too.

I stress tested it with saves 20 times a second, about 11,000 saves a second for 5 seconds, five files at once, bursts with random pauses, random gaps for 90 seconds, and a module that keeps breaking and getting fixed. I also stopped it mid-burst with Ctrl-C and with the devserver's shutdown signal. There was never more than one worker, nothing got orphaned or force-killed, and it always ended up running the latest code. Rapid saves get batched, so 200 saves turn into about 4 restarts.

Everything else is the same as the original PR, which is below.


When you change backend code while running Sentry locally, the web server picks up the change on its own, but the taskworker doesn't. You have to stop and restart the worker by hand, which is easy to forget, and then tasks quietly run the old code. It's also why people end up running a separate worker with devservices foreground taskworker instead of the one the devserver starts.

The worker used to have an autoreload option, but it was removed in #96486. That option used Django's reloader, which runs your code on a background thread and keeps the main thread for watching files. Once the taskworker started setting up its own handlers for shutdown signals, it broke, because Python only lets the main thread do that. The worker crashed on startup, so the option was taken out.

This brings --autoreload back with a different setup. A small parent process watches the Python files and does nothing else. The worker runs as its own separate process, so it has its main thread again and can set up its signal handlers like normal. When a file changes, the parent asks the worker to shut down the usual way, waits for it to finish, and starts a fresh one. The devserver now passes the flag to its worker whenever reload is on (the default), and the devservices taskworker programs use it too. Production doesn't pass the flag, so nothing changes there.

Saving several times in quick succession needed one more piece. A save could land while the new worker was still starting up, before it had set up its signal handlers, and the stop request then printed a KeyboardInterrupt traceback. Now the parent waits for the new worker to say it's done starting before it restarts it again. Saves made during that wait still cause one more restart afterwards, so nothing is missed. If the worker crashes while starting, say from a typo, the parent notices it's gone and stops waiting, so the next save brings it back.

The file watching comes from watchfiles, which is already installed because our web server's reload uses it.

getsentry needs a matching one-line config change, which should land after this.

ai slop diagrams:

before:
image

after:
image

alexsohn1126 and others added 2 commits October 23, 2026 13:32
When you change backend code while running Sentry locally, the web
server picks up the change on its own, but the taskworker doesn't. You
have to stop and restart the worker by hand, which is easy to forget,
and then tasks quietly run the old code. It's also why people end up
running a separate worker with `devservices foreground taskworker`
instead of the one the devserver starts.

The worker used to have an autoreload option, but it was removed in
#96486. That option used Django's reloader, which runs your code on a
background thread and keeps the main thread for watching files. Once the
taskworker started setting up its own handlers for shutdown signals, it
broke, because Python only lets the main thread do that. The worker
crashed on startup, so the option was taken out.

This brings `--autoreload` back with a different setup. A small parent
process watches the Python files and does nothing else. The worker runs
as its own separate process, so it has its main thread again and can set
up its signal handlers like normal. When a file changes, the parent asks
the worker to shut down the usual way, waits for it to finish, and
starts a fresh one. The devserver now passes the flag to its worker
whenever reload is on (the default), and the devservices taskworker
programs use it too. Production doesn't pass the flag, so nothing
changes there.

Saving several times in quick succession needed one more piece. A save
could land while the new worker was still starting up, before it had set
up its signal handlers, and the stop request then printed a
KeyboardInterrupt traceback. Now the parent waits for the new worker to
say it's done starting before it restarts it again. Saves made during
that wait still cause one more restart afterwards, so nothing is missed.
If the worker crashes while starting, say from a typo, the parent
notices it's gone and stops waiting, so the next save brings it back.

The file watching comes from `watchfiles`, which is already installed
because our web server's reload uses it.

getsentry needs a matching one-line config change, which should land
after this.

ai slop diagrams:

before:
<img width="645" height="273" alt="image"
src="https://github.com/user-attachments/assets/c939f6bc-5ef1-4618-bb8a-4fd5dfd90a9d"
/>

after:
<img width="634" height="304" alt="image"
src="https://github.com/user-attachments/assets/be1c4bf9-24bc-4daf-952b-ba33c5e06340"
/>
watchfiles imports anyio, which tries to import sniffio. test_devimports
blocks sniffio with a plain ImportError, which anyio doesn't catch, so
importing watchfiles at CLI startup broke test_startup_imports[sentry].
Import it inside the autoreload path instead.
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 23, 2026
The reloaded worker reported ready right after configure(), but it only
registers its SIGINT handler later in start(). During a burst of saves
the restart SIGINT landed in that gap, so the worker skipped its
shutdown, hung waiting on its children, and was SIGKILLed by watchfiles,
leaving the children orphaned. Report ready once the handler is in
place instead.
Stopping the autoreloading worker while a new one was still starting
printed KeyboardInterrupt tracebacks and made the parent exit 1. Catch
the interrupt around configure() as well as the worker run, ignore the
second SIGINT watchfiles sends during that exit, and treat an interrupt
while waiting on a restart as a normal stop.
@alexsohn1126

Copy link
Copy Markdown
Member Author

bugbot review

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8c77d79. Configure here.

except KeyboardInterrupt:
# Stopped while waiting on a restart. watchfiles has already stopped
# the worker on its way out.
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SIGTERM orphans the reloaded worker

Medium Severity

The autoreload parent only catches KeyboardInterrupt and never handles SIGTERM. Honcho and other supervisors stop sentry run taskworker by signaling that PID, so the parent exits immediately and the spawned worker plus its children keep running.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8c77d79. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This one is a false positive. watchfiles.run_process calls catch_sigterm() before it starts the worker, which turns SIGTERM into a KeyboardInterrupt. Its finally: process.stop() then stops the worker before our except runs. Honcho also signals the whole process group with os.killpg, not just this PID. I tested SIGTERM to the parent PID alone, including in the middle of a restart, and it exits 0 with nothing left running.

@alexsohn1126 alexsohn1126 added the Trigger: Override Selective Testing Run the full test suite; necessary in cases where selected tests are flaky due to reshuffling label Oct 23, 2026
@alexsohn1126

Copy link
Copy Markdown
Member Author

^ running all tests just to make sure

@alexsohn1126
alexsohn1126 marked this pull request as ready for review October 23, 2026 20:20
@alexsohn1126
alexsohn1126 requested review from a team as code owners October 23, 2026 20:20
@alexsohn1126
alexsohn1126 merged commit 63b6fb3 into master Oct 23, 2026
107 checks passed
@alexsohn1126
alexsohn1126 deleted the alexsohn/taskworker-autoreload-reland branch October 23, 2026 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components Trigger: Override Selective Testing Run the full test suite; necessary in cases where selected tests are flaky due to reshuffling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants