fix: make backpressure waits interruptible so Ctrl+C can cut a drain short - #318
Closed
drake-nominal wants to merge 1 commit into
Closed
fix: make backpressure waits interruptible so Ctrl+C can cut a drain short#318drake-nominal wants to merge 1 commit into
drake-nominal wants to merge 1 commit into
Conversation
…short A writer blocked in `enqueue` waited on a condvar that only a flush could signal, so a SIGINT arriving while the uploader was backed up could not release it. The existing cancel token only broke the python bridge's channel recv, one layer above; the blocking call underneath never observed it, and `Drop` then spun until every buffered point had uploaded regardless. Adds `NominalDatasetStream::cancel`, which sets a flag, wakes the parked processor threads, and notifies both buffers. Backpressure waits now poll that flag on a bounded interval, the batch processors exit rather than uploading their backlog, and `Drop` skips the drain when cancelled. Reservations made against `unflushed_points` are given back on the cancelled path so `Drop` cannot wait on points that will never flush. `enqueue`/`enqueue_many` keep their signatures and drop points when cancelled; `try_enqueue`/`try_enqueue_many` report it, which is what the python bindings use to raise instead of silently discarding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closing: this implements the wrong semantics. It made Ctrl+C abandon buffered points for a fast exit. The desired behaviour is the opposite — refuse further writes, flush everything already enqueued, then exit — which is implemented py-only in #320 and verified against a live process with a real SIGINT (10,442,916 points enqueued, 10,442,916 stored). If a deliberately fast-and-lossy teardown is wanted later it should be an explicit call, not the Ctrl+C path. |
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.
Stacked on #317 — review that one first.
The problem
A writer blocked in
enqueuewaited on a condvar that only a flush could signal, so a SIGINTarriving while the uploader was backed up could not release it. The existing cancel token only
broke the python bridge's channel
recv, one layer above; the blocking call underneath neverobserved it, and
Dropthen spun until every buffered point had uploaded regardless.So Ctrl+C during sustained backpressure already could not interrupt a drain in progress. #317
removes that channel entirely, which leaves the token with nothing to do on the ingest side — this
PR replaces it with cancellation the blocking call actually observes.
What changes
NominalDatasetStream::cancelsets a flag, wakes the parked processor threads, and notifies bothbuffers. Then:
CANCEL_POLL_INTERVAL, 50ms) instead ofwaiting indefinitely — a flush still notifies directly, so the timeout only bounds how long
cancellation takes to be noticed when no flush is coming
Dropskips the drain when cancelledunflushed_pointsare given back on the cancelled path, soDropcannotwait on points that will never flush
enqueue/enqueue_manykeep their signatures and drop points when cancelled.try_enqueue/try_enqueue_manyreport it, which is what the python bindings use to raise rather than silentlydiscard.
Verification
mainacross every write shape.Note that this does not change how long a graceful
close()takes — that is still bounded bymax_request_delay, because the batch processor parks for the full delay between flushes. That isthe subject of a separate change.
🤖 Generated with Claude Code