Skip to content

fix: make backpressure waits interruptible so Ctrl+C can cut a drain short - #318

Closed
drake-nominal wants to merge 1 commit into
perf/direct-enqueuefrom
fix/interruptible-cancel
Closed

fix: make backpressure waits interruptible so Ctrl+C can cut a drain short#318
drake-nominal wants to merge 1 commit into
perf/direct-enqueuefrom
fix/interruptible-cancel

Conversation

@drake-nominal

Copy link
Copy Markdown
Contributor

Stacked on #317 — review that one first.

The problem

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.

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::cancel sets a flag, wakes the parked processor threads, and notifies both
buffers. Then:

  • backpressure waits poll that flag on a bounded interval (CANCEL_POLL_INTERVAL, 50ms) instead of
    waiting indefinitely — a flush still notifies directly, so the timeout only bounds how long
    cancellation takes to be noticed when no flush is coming
  • the batch processors exit rather than uploading their backlog
  • Drop skips the drain when cancelled
  • reservations 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 rather than silently
discard.

Verification

  • 21 cargo tests pass.
  • Avro round-trip output identical to main across every write shape.
  • ruff, mypy, clippy clean.

Note that this does not change how long a graceful close() takes — that is still bounded by
max_request_delay, because the batch processor parks for the full delay between flushes. That is
the subject of a separate change.

🤖 Generated with Claude Code

…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>
@drake-nominal

Copy link
Copy Markdown
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.

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.

1 participant