Fix spurious resource request failure from reused ResourceGuard::Request#106690
Conversation
…uest`
`ResourceGuard::Request` is reused per thread via the `Request::local()`
thread-local instance. Its `exception` member is set by `failed()` but was
never cleared on reuse, so a thread that had previously run a failed resource
request kept a stale `exception`. A subsequent request scheduled on that thread
could be granted via `execute()` (no exception set), yet `Request::wait()` would
still observe the stale `exception` and throw `RESOURCE_ACCESS_DENIED`
("Resource request failed: ... Scheduler queue with resource request is about to
be destructed"), even though that request had actually been granted.
This can surface to users as a query spuriously failing with
`RESOURCE_ACCESS_DENIED` when a workload's queue is restructured/dropped (e.g.
`CREATE WORKLOAD`/`ALTER`/`DROP`) fails one request and the same worker thread
is then reused for another request that is in fact granted.
Clear `exception` in `Request::enqueue()` (the reuse reset point) so a granted
request can never observe a previous request's failure. Genuinely failed
requests still throw: `failed()` sets `exception` and `wait()` throws as before;
the value is only reset when the request is reused for a new enqueue.
Related: #100177
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…d regression tests The default-lock `ResourceGuard` constructor enqueues the request and then calls `Request::wait()` inside the constructor. If either `enqueue()` or `wait()` throws (the request failed because its queue is being destructed), the constructor propagates the exception and `~ResourceGuard()` never runs, so the request is never finished. The reused thread-local `Request::local()` instance is then left in the `Enqueued` or `Dequeued` state, and the next request enqueued on that thread hits `chassert(state == Finished)` in `Request::enqueue()` (an exception in debug/sanitizer builds), even though this pull request is specifically making that same-thread reuse valid. Wrap the constructor's `enqueue`/`wait` in a `try`/`catch` and restore the reused request to the `Finished` state before rethrowing, via the new `Request::recoverAfterConstructorFailure`. For a request failed in `wait()` (state `Dequeued`) this mirrors what `~ResourceGuard()` does on the `Lock::Defer` path: `finish` is a documented no-op for a failed request. For a request that failed in `enqueue()` (state `Enqueued`) the request never entered the scheduler, so only the state is reset. Add two regression tests in `gtest_workload_resource_manager`: - `ReuseRequestAfterFailedDeferRequestIsGranted` covers the original stale `exception` reset: a `Lock::Defer` request fails (queue destroyed) and a subsequent granted request on the same thread must not observe the previous failure. Without clearing `exception` in `enqueue()`, the granted request spuriously throws `RESOURCE_ACCESS_DENIED`. - `ReuseRequestAfterFailedDefaultRequestIsGranted` covers the constructor failure path above: a `Lock::Default` request fails and a granted request is then issued on the same thread. An uncaught exception in a worker thread is swallowed by the thread pool, so the granted reuse is wrapped in `EXPECT_NO_THROW` to surface failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📊 Cloud Performance Report ✅ AI verdict: This PR only fixes an exception-recovery bug in the workload I/O scheduler's ResourceGuard (clearing a stale error on a reused request) plus adds two regression tests; it does not touch the normal query-execution path. The two flagged improvements on tpch_adapted_1_official (Q7 -32%, Q20 -44%) therefore cannot plausibly be caused by this change, and the source side had few runs with noisy measurements (Q20 especially). Both have been downgraded to not-sure as likely run-to-run variance rather than a real PR effect. clickbench🟢 No significant changes tpch_adapted_1_officialFlagged queries (2 of 22)
q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on. Debug info
|
…sourceTest.h` The style check flags `ErrorCodes::INVALID_SCHEDULER_NODE` as defined but not used in `ResourceTest.h`: the check excludes `tests/*.cpp` files but not headers, so the declaration must live in `gtest_workload_resource_manager.cpp` where the code is actually used. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review: `enqueueRequestUsingBudget` commits `budget.ask` before calling the concrete `enqueueRequest`, which can throw before linking the request into the queue (`INVALID_SCHEDULER_NODE` when the queue is being destructed, or `SERVER_OVERLOADED` when `max_waiting_queries` is reached). The request never enters the scheduler and `finish` never runs for it, but the queue budget had already been changed assuming the request would be granted `request->cost` and consume `estimated_cost`. In particular, when the queue carries a debt from a previous over-consumption, `ask` raises the request's cost to repay it, and a failed enqueue would silently forget the debt, skewing costs of subsequent requests. Wrap `enqueueRequest` in `try`/`catch` and roll the transaction back via `budget.adjust(estimated_cost, request->cost)` before rethrowing. Add a regression test `SchedulerRoot.BudgetAfterFailedEnqueue`: it creates a negative budget, purges the queue so `enqueueRequest` throws, and checks the budget is unchanged after the failed enqueue (without the rollback the budget observably jumps from the debt value back to zero). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed two commits:
|
`ResourceGuardSessionDataHooks` in `HTTPConnectionPool.cpp` is the second owner of a `ResourceGuard::Request` (besides the thread-local instance used by `ResourceGuard`). Its `atStart` does `request.enqueue` followed by `request.wait`, and `HTTPSession::write`/`HTTPSession::receive` catch any exception thrown from `atStart` (note that `DB::Exception` is a `Poco::Exception`) and unconditionally call `atFail`. If `enqueue` throws before the request is linked into the scheduler (for example `INVALID_SCHEDULER_NODE` from a purged queue, or `SERVER_OVERLOADED` from `max_waiting_queries`), the request is left in the `Enqueued` state. The subsequent `atFail` then calls `request.finish(0, link)` on a non-`Dequeued` request, tripping `chassert(state == Dequeued)` in debug/sanitizer builds and adjusting the queue budget with a stale `estimated_cost` in release builds. Wrap `atStart` in a try/catch that calls `recoverAfterConstructorFailure` on failure (mirroring the `ResourceGuard` constructor), and gate `atFinish`/`atFail` behind an `active` flag set only after `wait` succeeds, so the unconditional `atFail` during unwinding does not finish the request twice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The sole CI red on the previous head ( @groeneai, could you take a look at |
| // call `atFail()` while unwinding, so recover the request to the `Finished` state now and keep | ||
| // `active` false. Otherwise `atFail()` would call `request.finish()` on a non-`Dequeued` request, | ||
| // tripping `chassert(state == Dequeued)` in debug builds and corrupting the queue budget in release. | ||
| request.recoverAfterConstructorFailure(link); |
There was a problem hiding this comment.
This recovery path needs a focused regression test for the ResourceGuardSessionDataHooks owner as well. The new tests cover the thread-local ResourceGuard::Request::local() owner and the budget rollback, but they do not exercise the Poco control flow that makes this line necessary: atStart throws, HTTPSession::write/receive catches it, and then unconditionally calls atFail.
Without a test around this hook path, dropping either this recovery call or the active guard below would leave all current regressions green while reintroducing the debug chassert / release budget-drift failure for scheduled HTTP socket I/O. Please add the smallest test that drives an HTTP session with an attached read or write ResourceLink, makes the scheduler request fail during atStart (for example by purging/filling the queue), and verifies the hook unwinds cleanly and the request can be reused/finished correctly.
|
@alexey-milovidov Investigated. The recorded red for Evidence (CIDB + the
The contention self-resolved: shard OOM-kills dropped 304 (06-10) -> 59 (06-11) -> 12 (06-12), and So no per-test fix is warranted. A |
LLVM Coverage ReportChanged lines: Changed C/C++ lines covered by tests: 120/127 (94.49%) | Lost baseline coverage: none · Uncovered code |
robot-ch-test-poll
added
the
pr-synced-to-cloud
4 participants
ResourceGuard::Requestis reused per thread via theRequest::local()thread-local instance. Itsexceptionmember is set byfailed()but was never cleared on reuse, so a thread that had previously run a failed resource request kept a staleexception. A subsequent request scheduled on that thread could be granted viaexecute()(no exception set), yetRequest::wait()would still observe the staleexceptionand throwRESOURCE_ACCESS_DENIED("Resource request failed: ... Scheduler queue with resource request is about to be destructed"), even though that request had actually been granted.This can surface to users as a query spuriously failing with
RESOURCE_ACCESS_DENIEDwhen restructuring/dropping a workload's queue (e.g.CREATE WORKLOAD/ALTER/DROP) fails one request and the same worker thread is then reused for another request that is in fact granted.The fix clears
exceptioninRequest::enqueue()(the reuse reset point) so a granted request can never observe a previous request's failure. Genuinely failed requests still throw:failed()setsexceptionandwait()throws as before; the value is only reset when the request is reused for a new enqueue.This was found while investigating a reliable deadlock in
SchedulerWorkloadResourceManager.DropNotEmptyQueueLongexposed by the LIFO worker-thread wake order; it is a pre-existing latent bug that makes sense to fix on its own. Verified againstSchedulerWorkloadResourceManager.*: passes under ThreadSanitizer and under heavy parallel stress (previously a reliable hang/spurious failure).Related: #100177
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed a rare spurious
RESOURCE_ACCESS_DENIEDerror ("Scheduler queue with resource request is about to be destructed") for a query that was actually granted access to a workload resource, caused by a reused thread-local request object retaining a previous request's failure.Documentation entry for user-facing changes
Version info
26.6.1.760