fix(sdk): bound get_aggregated_resources wait to per-detector timeout - #5545
Open
sidsri14 wants to merge 1 commit into
Open
fix(sdk): bound get_aggregated_resources wait to per-detector timeout#5545sidsri14 wants to merge 1 commit into
sidsri14 wants to merge 1 commit into
Conversation
|
|
Pull request dashboard statusWaiting on reviewers · refreshed 2026-08-14 17:10 UTC Review the latest changes. Status above doesn't look right?
|
Avoid ThreadPoolExecutor context-manager shutdown(wait=True), which defeats future.result(timeout=...) by joining detectors that outlive the timeout. Shut down without waiting so the caller returns after the configured per-detector bound; a timed-out detector may continue until detect() returns. This also prevents a forked child from wedging in the resource refresh at-fork handler when it inherits a locked detector dependency. Assisted-by: Claude Sonnet 4.6
sidsri14
force-pushed
the
fix/get-aggregated-resources-timeout
branch
from
August 14, 2026 17:09
049a746 to
7ae6a59
Compare
sidsri14
marked this pull request as ready for review
August 14, 2026 17:09
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
1 participant
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.
Summary
Fixes #5533 (Part 1).
get_aggregated_resources()passes atimeouttofuture.result(), but the wait is not actually bounded: exiting thewith concurrent.futures.ThreadPoolExecutor(...)block callsshutdown(wait=True), which joins still-running detector workers. A detector that blocks longer thantimeouttherefore holds up the whole call regardless of thefuture.result(timeout=...)bound.The fix stops using the executor as a context manager and shuts it down explicitly with
shutdown(wait=False, cancel_futures=True)in afinallyblock. A detector that outlives the timeout can continue in its worker thread untildetect()returns, but the caller no longer waits for that worker during executor shutdown.As noted by the reporter, this also fixes Part 2 (the forked-child wedge) as a side effect: the child's
_get_process_dependent_resource()can return with the skip warning instead of hanging forever inside theat_forkhandler.Existing behavior preserved
timeout, ordering, and merge semantics are unchanged.raise_on_errorre-raises theTimeoutErroror detector exception; otherwise a warning is logged and the detector is skipped.Test plan
test_aggregated_resources_timeout_bounds_wait: a detector sleeping for 2 seconds withtimeout=0.1returns within 1 second and logs the skip warning.Changelog
Added
.changelog/5545.fixed.