[Bugfix] Respect explicit token acquisition timeout - #248
Conversation
Signed-off-by: luozijian <luozijian0924@gmail.com>
|
A quick CI update: in Buildkite build #791, the P/D Disaggregation Test (4 GPUs) exited during dependency installation. The final attempt failed while installing scikit-learn with: This attempt did not reach the P/D test itself, so I am not claiming GPU validation passed. The build summary shows the other 10 steps passed. When convenient, could someone with CI access please check the worker's available disk space and rerun the failed step once the space issue is resolved? I have left the patch unchanged since this log does not indicate a source-code failure. Thank you for your help! |
There was a problem hiding this comment.
🟡 Changes recommended
The new queue-timeout unit test can panic due to Instant::now() - age under rare clock/uptime conditions, which should be made safe before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes queued token acquisition timing out earlier than the caller-specified budget by ensuring TokenBucket::acquire_timeout waits according to the explicit deadline (rather than inheriting acquire’s initial refill estimate) and by reusing a shared polling helper for both code paths.
Changes:
- Refactor token acquisition waiting into a shared
wait_for_tokenshelper and makeacquire_timeouthonor the caller’s full timeout budget under contention. - Add regression tests covering contention, short deadlines, returned tokens, dropped waiters, and real queue/middleware behavior (including a loopback HTTP test).
- Add crate-local queue-timeout unit tests via a
#[cfg(test)]module include.
File summaries
| File | Description |
|---|---|
src/core/token_bucket.rs |
Makes acquire_timeout wait within the caller’s explicit deadline and shares the polling loop via wait_for_tokens. |
src/middleware.rs |
Wires in queue-timeout unit tests under #[cfg(test)]. |
src/middleware_queue_timeout_tests.rs |
Adds unit tests validating QueueProcessor timeout behavior vs. elapsed queue age and configured budget. |
tests/token_bucket_timeout_probe.rs |
Adds integration-style tests probing explicit token acquisition timeout behavior under contention/cancellation. |
tests/queue_timeout_http_test.rs |
Adds loopback HTTP regression test exercising the production queue + middleware end-to-end. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let (first_tx, first_rx) = oneshot::channel(); | ||
| let (second_tx, second_rx) = oneshot::channel(); | ||
| let queued_at = Instant::now() - age; | ||
| for permit_tx in [first_tx, second_tx] { |
There was a problem hiding this comment.
Addressed in de50d0f: use checked_sub and, if the timestamp cannot be represented, wait for the requested age before creating/exhausting the token bucket. A plain fallback to now would erase the elapsed queue budget; creating the bucket before that wait would refill it and also change the test's meaning.
A new test explicitly exercises the None branch. All five queue tests and all 489 library tests pass on Rust 1.95.0, as do formatting and all-targets/all-features Clippy with warnings denied. This is a test-only change, not a fix for the previously reported Buildkite disk-space failure or a GPU validation claim. AI-assisted implementation and verification; the contributor reviewed and personally signed the published commit.
Signed-off-by: luozijian <luozijian0924@gmail.com>
Purpose
Fixes #247 by honoring the caller's explicit token acquisition deadline under
contention. Previously, acquire_timeout wrapped acquire, whose initial
estimated-refill timeout could expire much sooner than the configured queue
budget. QueueProcessor could consequently send HTTP 408 despite enough time
remaining for token refills.
Share the existing polling helper between the two methods. Preserve legacy
acquire behavior, token refill policy and all public signatures. No new
dependencies, queue-capacity changes, FIFO promises or drain behavior.
Test Plan
cargo +1.95.0 test --locked --lib --test api_endpoints_test --test token_bucket_timeout_probe --test queue_timeout_http_test -- --test-threads=4 cargo +1.95.0 fmt --check cargo +1.95.0 clippy --locked --all-targets --all-features -- -D warningsTest Result
47 passed. The library count includes the focused library cases above.
488 + 47 + 1 HTTP + 4 token tests = 540 passed, no failures/ignored/filtered.
two 408 responses; candidate: two 200 responses and complete expected bodies.
expired queue entries and time already spent queued.
-D warnings pass under the CI-pinned Rust 1.95.0, without lint allowances.
Diff whitespace checks pass. This was local, not an upstream CI run.
reproduced them. No lint-policy changes included. Behavioral tests listed
above have now also passed under 1.95.0. Neither local run is upstream CI.
Tests use the production queue and middleware with a local terminal handler;
no real inference/GPU or full repository/performance validation is claimed.
Implementation and verification are AI-assisted with Codex. The contributor
confirmed human review and personally signed off commit
520303f1052ea01285e1906ea1c68d01b31a902d.Description checklist