Skip to content

fix(runtime): flush recorder queue when the process exits - #304

Open
huynna12 wants to merge 1 commit into
FastCrest:mainfrom
huynna12:feat/record-graceful-shutdown
Open

fix(runtime): flush recorder queue when the process exits#304
huynna12 wants to merge 1 commit into
FastCrest:mainfrom
huynna12:feat/record-graceful-shutdown

Conversation

@huynna12

Copy link
Copy Markdown
Contributor

Description

tether serve --record loses 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
/act event loop; write_request() now returns as soon as the record is queued, and
the worker writes it later. Daemon threads get killed outright at interpreter exit,
mid-write, no cleanup. close() already drained the queue, and lifespan shutdown
already 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. In
those cases nothing calls close(), and the whole queue is lost, header included.

Fix:

  • Each RecordWriter registers itself in a module-level weakref.WeakSet once its
    worker starts. atexit.register(_close_all_writers) drains every writer still in it.
    atexit handlers run before daemon threads are killed, and that ordering is the fix.
  • WeakSet so writers aren't pinned in memory forever. Safe because a running thread
    holds a strong ref to its target, so an entry can't vanish while there's still
    unwritten work.
  • close() takes an optional timeout, default None (existing callers unaffected).
    Only the atexit path bounds it (5s), so a stalled disk can't hang process exit;
    on timeout the tail is abandoned and logged instead.
  • Simplified the drain itself: the old close() waited on the same event three times
    (flush_sync(), queue.join(), worker.join()). The queue is FIFO and the worker
    only returns after the stop sentinel, so worker.join() alone already proves
    everything ahead of it was written.

No footer is written on the atexit path. Footers are optional and imply a clean
end to the session, which a crash exit isn't. server.py is otherwise untouched; the
lifespan hook was already correct.

Closes #100

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Infrastructure / CI update

How Has This Been Tested?

6 new tests in tests/test_record.py::TestGracefulShutdown. The two that matter spawn
a real child interpreter, write 25 records, and exit with no close() call, so
atexit is the only thing that can save them. The gzip variant is the strict check: a
stream 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 WeakSet deregistration on close, close() being idempotent
(lifespan and atexit can both call it), and the timeout path not hanging on a stuck
worker.

  • pytest tests/ passes locally, 152 tests, all green, run module-by-module.
  • tether doctor sanity check
  • Other (please specify): pytest tests/test_record.py::TestGracefulShutdown -v
    to see all 6 pass.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have kept the PR scoped to a single concern (one concern per PR)

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
@huynna12
huynna12 requested a review from rylinjames as a code owner August 18, 2026 04:24
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.

[Reliability] Implement Graceful Shutdown for RecordWriter

1 participant