fix(runtime): flush recorder queue when the process exits - #304
Open
huynna12 wants to merge 1 commit into
Open
Conversation
lifespan shutdown only fires on a clean exit. If serve crashes on startup the worker thread is a daemon, so it gets killed with records still queued and we lose the lot. Track live writers in a weakset and drain them from atexit, which runs before daemons are killed. close() takes an optional timeout so a wedged disk can't hang exit; defaults to None so existing callers don't change. Closes FastCrest#100
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
tether serve --recordloses queued records when the server doesn't shut down cleanly.#218 moved record writing onto a background daemon thread so it wouldn't block the
/actevent loop;write_request()now returns as soon as the record is queued, andthe worker writes it later. Daemon threads get killed outright at interpreter exit,
mid-write, no cleanup.
close()already drained the queue, and lifespan shutdownalready called it, so Ctrl-C/SIGTERM was fine. The gap: lifespan only fires on a clean
ASGI shutdown, not a startup crash, a bare
sys.exit(), or non-uvicorn embedding. Inthose cases nothing calls
close(), and the whole queue is lost, header included.Fix:
RecordWriterregisters itself in a module-levelweakref.WeakSetonce itsworker starts.
atexit.register(_close_all_writers)drains every writer still in it.atexithandlers run before daemon threads are killed, and that ordering is the fix.WeakSetso writers aren't pinned in memory forever. Safe because a running threadholds a strong ref to its target, so an entry can't vanish while there's still
unwritten work.
close()takes an optionaltimeout, defaultNone(existing callers unaffected).Only the
atexitpath bounds it (5s), so a stalled disk can't hang process exit;on timeout the tail is abandoned and logged instead.
close()waited on the same event three times(
flush_sync(),queue.join(),worker.join()). The queue is FIFO and the workeronly returns after the stop sentinel, so
worker.join()alone already proveseverything ahead of it was written.
No footer is written on the
atexitpath. Footers are optional and imply a cleanend to the session, which a crash exit isn't.
server.pyis otherwise untouched; thelifespan hook was already correct.
Closes #100
Type of Change
How Has This Been Tested?
6 new tests in
tests/test_record.py::TestGracefulShutdown. The two that matter spawna real child interpreter, write 25 records, and exit with no
close()call, soatexitis the only thing that can save them. The gzip variant is the strict check: astream killed mid-write has no trailer and fails to decompress, so reading it back
proves
close()actually ran.Verified the tests can fail: commented out
atexit.register(...)and reran:assert len(requests) == 25
E assert 0 == 25
0 of 25 survived, including the header. Restored the line, both pass.
Remaining 4 tests cover
WeakSetderegistration on close,close()being idempotent(lifespan and
atexitcan both call it), and the timeout path not hanging on a stuckworker.
pytest tests/passes locally, 152 tests, all green, run module-by-module.tether doctorsanity checkpytest tests/test_record.py::TestGracefulShutdown -vto see all 6 pass.
Checklist