Skip to content

fix(cdn): bound goroutine fan-out and add per-request timeout - #37

Merged
bennypowers merged 1 commit into
mainfrom
fix/cdn-concurrency-timeout
Apr 29, 2026
Merged

fix(cdn): bound goroutine fan-out and add per-request timeout#37
bennypowers merged 1 commit into
mainfrom
fix/cdn-concurrency-timeout

Conversation

@bennypowers

Copy link
Copy Markdown
Owner

Summary

  • Share a single semaphore across all recursion depths in CDN resolver, preventing 10^depth goroutine explosion with deep npm dependency trees
  • Release semaphore after HTTP work but before spawning children, preventing deadlock when all slots are held by parents waiting on children
  • Add WithRequestTimeout for per-request context deadlines on registry and CDN fetch calls
  • Add WithConcurrency to configure the shared goroutine limit (default 10)
  • Use select on semaphore acquisition to respect context cancellation
  • Extract clone() method to eliminate field-list duplication across 8 With* builders, with slices.Clone for slice fields

Closes #34, closes #35

Test plan

  • TestSharedSemaphoreBoundsConcurrency -- verifies max concurrent goroutines stay within configured limit
  • TestSharedSemaphoreNoDeadlockMultiDepth -- regression test with concurrency=2 and multi-level transitive deps (would deadlock with old per-level semaphore)
  • TestRequestTimeoutCancelsSlowFetch -- verifies slow CDN fetch is cancelled by timeout
  • TestRequestTimeoutDefaultIsNoTimeout -- verifies default (no timeout) works correctly
  • TestContextCancellationStopsSemAcquisition -- verifies goroutines respect context cancellation while waiting for semaphore slots
  • TestWithConcurrencyDefault and TestWithConcurrencyIgnoresZero -- builder defaults
  • All 338 tests pass, zero lint issues (make lint && make test)

The CDN resolver created a fresh semaphore per recursion level, allowing
10^depth concurrent goroutines for deep dependency trees. Share a single
semaphore across all depths, acquiring it only for HTTP work and releasing
before spawning children to prevent deadlock.

Add WithRequestTimeout for per-request context deadlines on registry and
CDN fetch calls, and WithConcurrency to configure the shared limit.

Extract clone() to eliminate field-list duplication across With* builders,
using slices.Clone for slice fields to prevent aliasing.

Closes #34, closes #35

Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Apr 29, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@bennypowers has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 29 minutes and 34 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a76b20fc-9b29-4daf-b6c4-38179e38c0bd

📥 Commits

Reviewing files that changed from the base of the PR and between a79a8a2 and 415033f.

📒 Files selected for processing (20)
  • resolve/cdn/cdn.go
  • resolve/cdn/cdn_test.go
  • resolve/cdn/testdata/deep-a-package/package.json
  • resolve/cdn/testdata/deep-a-registry/response.json
  • resolve/cdn/testdata/deep-b-package/package.json
  • resolve/cdn/testdata/deep-b-registry/response.json
  • resolve/cdn/testdata/dep-a-package/package.json
  • resolve/cdn/testdata/dep-a-registry/response.json
  • resolve/cdn/testdata/dep-b-package/package.json
  • resolve/cdn/testdata/dep-b-registry/response.json
  • resolve/cdn/testdata/dep-c-package/package.json
  • resolve/cdn/testdata/dep-c-registry/response.json
  • resolve/cdn/testdata/root-package/package.json
  • resolve/cdn/testdata/root-registry/response.json
  • resolve/cdn/testdata/slow-pkg-package/package.json
  • resolve/cdn/testdata/slow-pkg-registry/response.json
  • resolve/cdn/testdata/subdep-a1-package/package.json
  • resolve/cdn/testdata/subdep-a1-registry/response.json
  • resolve/cdn/testdata/subdep-b1-package/package.json
  • resolve/cdn/testdata/subdep-b1-registry/response.json
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/cdn-concurrency-timeout

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 29 minutes and 34 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown

Build Artifacts

OS x64 arm64
Linux mappa-linux-x64 mappa-linux-arm64
macOS mappa-darwin-x64 mappa-darwin-arm64
Windows mappa-win32-x64 mappa-win32-arm64

Built from d67590a @ fix/cdn-concurrency-timeout

@bennypowers
bennypowers merged commit 188ed9c into main Apr 29, 2026
11 checks passed
@bennypowers
bennypowers deleted the fix/cdn-concurrency-timeout branch April 29, 2026 15:47
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.

CDN resolver: unbounded goroutine fan-out per recursion depth CDN resolver: no timeout on HTTP fetch calls

1 participant