fix: add combined size pool#1848
Closed
wayfarer3130 wants to merge 3 commits into
Closed
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jbocce
reviewed
May 8, 2026
jbocce
reviewed
May 8, 2026
jbocce
reviewed
May 8, 2026
jbocce
previously requested changes
May 8, 2026
jbocce
left a comment
jbocce
left a comment
Collaborator
There was a problem hiding this comment.
Couple of things to look at. Thanks.
Collaborator
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
Collaborator
|
FYI... the OHIF downstream validation is failing. |
feat: add combined size pool to overall request pool Re-applies the combined request-pool limit onto main. Adds a Metadata request type and an overall maxConcurrentRequests cap shared across the metadata/interaction/thumbnail/prefetch queues, so lower-priority fetches cannot starve interaction fetches (interaction is always allowed at least one request). Adds jest unit tests covering the combined cap, interaction guarantee, outstandingRequests accounting, and the separate compute queue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> @
wayfarer3130
force-pushed
the
feat/add-combined-size-pool
branch
from
July 11, 2026 21:33
6f854ed to
cb6d526
Compare
📝 WalkthroughWalkthrough
ChangesRequest scheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RequestPoolManager
participant MetadataQueue
participant InteractionQueue
participant ComputeQueue
RequestPoolManager->>MetadataQueue: dispatch metadata within remaining capacity
RequestPoolManager->>InteractionQueue: dispatch interaction with priority allowance
RequestPoolManager->>ComputeQueue: dispatch compute independently
RequestPoolManager-->>RequestPoolManager: update outstanding request count
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/test/requestPoolManager.jest.js (1)
56-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test mixing Metadata with another type under the combined cap.
Existing "combined pool limit" tests only use a single type (Prefetch), so they don't catch the Metadata-exclusion issue flagged in
requestPoolManager.ts(Lines 313-356) where Metadata's in-flight count isn't deducted from the budget seen by other types (and vice versa), letting the total exceedmaxConcurrentRequests.it('deducts in-flight Metadata requests from the combined pool available to other types', () => { const manager = new RequestPoolManager(); manager.setMaxConcurrentRequests(5); const metadataFns = addPending(manager, RequestType.Metadata, 5); expect(dispatchedCount(metadataFns)).toBe(5); // Combined pool of 5 is already fully consumed by Metadata. const prefetchFns = addPending(manager, RequestType.Prefetch, 3); expect(dispatchedCount(prefetchFns)).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/requestPoolManager.jest.js` around lines 56 - 79, Extend the “combined pool limit” tests with a Metadata-plus-Prefetch regression case: configure maxConcurrentRequests to 5, dispatch five Metadata requests, then add three Prefetch requests and assert none dispatch. Use the existing RequestPoolManager, addPending, RequestType, and dispatchedCount helpers without changing production code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/requestPool/requestPoolManager.ts`:
- Around line 313-356: Update the outstandingRequests getter to include
this.numRequests[RequestType.Metadata] in the combined in-flight request count.
Preserve the existing startGrabbing() cap calculations so metadata reduces the
available budget for interaction, thumbnail, and prefetch requests.
---
Nitpick comments:
In `@packages/core/test/requestPoolManager.jest.js`:
- Around line 56-79: Extend the “combined pool limit” tests with a
Metadata-plus-Prefetch regression case: configure maxConcurrentRequests to 5,
dispatch five Metadata requests, then add three Prefetch requests and assert
none dispatch. Use the existing RequestPoolManager, addPending, RequestType, and
dispatchedCount helpers without changing production code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d55ca82a-8e96-4bca-8164-2fc05324a0e3
📒 Files selected for processing (3)
packages/core/src/enums/RequestType.tspackages/core/src/requestPool/requestPoolManager.tspackages/core/test/requestPoolManager.jest.js
Comment on lines
+313
to
+356
| public get outstandingRequests() { | ||
| return ( | ||
| this.numRequests[RequestType.Interaction] + | ||
| this.numRequests[RequestType.Thumbnail] + | ||
| this.numRequests[RequestType.Prefetch] | ||
| ); | ||
| } | ||
|
|
||
| protected startGrabbing(): void { | ||
| const hasRemainingMetadataRequests = this.sendRequests( | ||
| RequestType.Metadata, | ||
| this.maxConcurrentRequests - this.outstandingRequests | ||
| ); | ||
| let available = this.maxConcurrentRequests - this.outstandingRequests; | ||
| // The && available after checking for > 0 means it uses the available requests as a positive value | ||
| // Then, allow 1 interaction request always | ||
| // Only allow 0 requests for other types - this is the || 0 part to get a 0 result rather than a false one | ||
| const hasRemainingInteractionRequests = this.sendRequests( | ||
| RequestType.Interaction | ||
| RequestType.Interaction, | ||
| (available > 0 && available) || | ||
| (this.numRequests[RequestType.Interaction] === 0 && 1) || | ||
| 0 | ||
| ); | ||
| available = this.maxConcurrentRequests - this.outstandingRequests; | ||
| // Allow an extra request for thumbnail if there are no outstanding | ||
| // interaction or thumbnail requests. | ||
| const hasRemainingThumbnailRequests = this.sendRequests( | ||
| RequestType.Thumbnail | ||
| RequestType.Thumbnail, | ||
| (available > 0 && available) || | ||
| (this.numRequests[RequestType.Interaction] === 0 && | ||
| this.numRequests[RequestType.Thumbnail] === 0 && | ||
| 1) || | ||
| 0 | ||
| ); | ||
| const hasRemainingPrefetchRequests = this.sendRequests( | ||
| RequestType.Prefetch | ||
| RequestType.Prefetch, | ||
| this.maxConcurrentRequests - this.outstandingRequests | ||
| ); | ||
|
|
||
| // Compute requests are on a different queue as they don't use http requests | ||
| const hasRemainingComputeRequests = this.sendRequests(RequestType.Compute); | ||
|
|
||
| if ( | ||
| !hasRemainingMetadataRequests && |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant region.
ast-grep outline packages/core/src/requestPool/requestPoolManager.ts --view expanded || true
wc -l packages/core/src/requestPool/requestPoolManager.ts
sed -n '1,220p' packages/core/src/requestPool/requestPoolManager.ts
sed -n '220,420p' packages/core/src/requestPool/requestPoolManager.tsRepository: cornerstonejs/cornerstone3D
Length of output: 15232
Count Metadata in the combined request cap. outstandingRequests excludes RequestType.Metadata, but startGrabbing() uses it for every cap calculation. That lets metadata run without reducing the budget for interaction/thumbnail/prefetch, so total in-flight requests can exceed maxConcurrentRequests (for example, 6 metadata + 6 interaction + 4 thumbnail with defaults). Include metadata in the combined gating.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/requestPool/requestPoolManager.ts` around lines 313 - 356,
Update the outstandingRequests getter to include
this.numRequests[RequestType.Metadata] in the combined in-flight request count.
Preserve the existing startGrabbing() cap calculations so metadata reduces the
available budget for interaction, thumbnail, and prefetch requests.
wayfarer3130
dismissed
jbocce’s stale review
July 15, 2026 15:01
Applied a comment to explain - can't change the underlying code in this particular case.
docs: document combined request pool and raise default cap to 50 Raise the default combined `maxConcurrentRequests` cap from 10 to 50, a more reasonable initial default for the shared image-load pool. Add a 5.x migration guide for the combined request pool limit and document the combined cap, `setMaxConcurrentRequests`, and the `Metadata` request type in the request pool manager concept docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> @
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/docs/docs/concepts/cornerstone-core/requestPoolManager.md`:
- Around line 87-107: The combined-concurrency contract is inconsistent with the
scheduler because metadata requests are omitted from the shared count and
Interaction requests may bypass a full cap. Update the scheduler’s
combined-budget logic to enforce one clearly defined policy across metadata and
image requests, including whether the Interaction exception is retained; then
update packages/docs/docs/concepts/cornerstone-core/requestPoolManager.md lines
87-107 and packages/docs/docs/migration-guides/5x/3-request-pool.md lines 18-31
to describe that exact policy and effective-budget behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 244131ed-baad-4e35-91af-e0bffd12544a
📒 Files selected for processing (3)
packages/core/src/requestPool/requestPoolManager.tspackages/docs/docs/concepts/cornerstone-core/requestPoolManager.mdpackages/docs/docs/migration-guides/5x/3-request-pool.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/requestPool/requestPoolManager.ts
Comment on lines
+87
to
+107
| In addition to the per-type maximums above, each pool enforces a **combined | ||
| cap** on the total number of concurrent HTTP requests across all types | ||
| (`metadata`, `interaction`, `thumbnail`, and `prefetch`). This prevents | ||
| lower-priority work (such as a large prefetch backlog) from saturating the | ||
| browser's connection pool and starving interaction fetches. | ||
|
|
||
| The combined cap defaults to `50` and can be changed with | ||
| `setMaxConcurrentRequests`: | ||
|
|
||
| ```js | ||
| import { imageLoadPoolManager } from '@cornerstonejs/core'; | ||
|
|
||
| // Total concurrent HTTP requests across all types for this pool. | ||
| imageLoadPoolManager.setMaxConcurrentRequests(100); | ||
| ``` | ||
|
|
||
| The number of requests actually dispatched for a given type is the smaller of | ||
| its per-type maximum (`setMaxSimultaneousRequests`) and the remaining combined | ||
| budget. Even when the combined pool is full, at least one `Interaction` request | ||
| is always allowed through so background fetches can never fully block | ||
| interaction. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve the combined-concurrency contract mismatch.
The documentation promises a strict cap across metadata and image requests, but the supplied scheduler excludes in-flight metadata from outstandingRequests and can dispatch one interaction request when no combined capacity remains. Align the implementation and both documents before merge.
packages/docs/docs/concepts/cornerstone-core/requestPoolManager.md#L87-L107: define whether the cap is strict or includes documented exceptions, then match the scheduler.packages/docs/docs/migration-guides/5x/3-request-pool.md#L18-L31: update the migration contract and effective-budget description consistently.
📍 Affects 2 files
packages/docs/docs/concepts/cornerstone-core/requestPoolManager.md#L87-L107(this comment)packages/docs/docs/migration-guides/5x/3-request-pool.md#L18-L31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/docs/docs/concepts/cornerstone-core/requestPoolManager.md` around
lines 87 - 107, The combined-concurrency contract is inconsistent with the
scheduler because metadata requests are omitted from the shared count and
Interaction requests may bypass a full cap. Update the scheduler’s
combined-budget logic to enforce one clearly defined policy across metadata and
image requests, including whether the Interaction exception is retained; then
update packages/docs/docs/concepts/cornerstone-core/requestPoolManager.md lines
87-107 and packages/docs/docs/migration-guides/5x/3-request-pool.md lines 18-31
to describe that exact policy and effective-budget behavior.
Collaborator
Author
|
Based on feedback offline, it seems like having different queues for different things that all are basically http sounds like a bad idea. Going to redo this in a way that combines the queues |
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
2 participants
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.
Context
This PR lays the groundwork for adding display fetch controls that delay initial fetching of thumbnails and prefetches until the primary viewports are completed.
Default combined cap raised 10 → 50. The initial maxConcurrentRequests default of 10 was too low for a general default — it would globally throttle the shared imageLoadPoolManager (which currently sets Interaction/Thumbnail/Prefetch to 1000 each) down to 10 concurrent HTTP requests. Bumped the default to 50 as a more reasonable starting point.
RequestType.Metadata producer is intentionally not wired here. The new metadata queue is added in this PR but nothing enqueues RequestType.Metadata yet. Tagging metadata fetches with this type will be wired up later on the OHIF side.
setMaxConcurrentRequests caller is intentionally not wired here. No consumer calls it in this PR, so the 50 default governs. OHIF will call setMaxConcurrentRequests to tune the cap for its own use case in a follow-up.
Changes & Results
Adds a prefetch queue for metadata
Adds an overall maximum size prefetch queue that applies to the metadata through prefetch queues, with the higher priority queues being done first. The intent of this is in an application such as OHIF to allow all the metadata queries to be sent off initially, and then let those continue to come in, while the initial images are not fetched initially. Then, the viewport specific images would be fetched and then thumbnails and finally prefetch images.
Testing
Link into OHIF, and display a study with thumbnails and prefetch. Add a log message on prefetch queue.
Reduce the delay in study browser to 125 ms or something like that, and all the thumbnails should be before the prefetch.
Should also add the metadata prefetch to the queue at level metadata, and add a viewport fetch for first image at low priority as soon as the display sets are created. That will demonstrate slowly fetching viewport/thumbnail images mixed in with metadata requests.
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-apito update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Summary
Metadataas a highest-priority request type.