feat(utils/throttler): throttler revamped

[rodrodros]

• Throttler now has it's own sub package inside utils
• Swap setter for option pattern, forcing optional settings to throttler startup
• Max queue length flag type changed to uint64 (from int) to avoid negative values
• Add context object so call to resources and actions waiting on the queue could both be interrupted via cancel()
• Fix an issue where a queue with size 0 would cause the resource to not execute anything

[github-actions]

Claude finished @rodrodros's task in 2m 23s —— View job

---

Review: feat(utils/throttler): throttler revamped

I reviewed the throttler rewrite and all the call-site migrations against origin/main. This is a clean, well-tested PR — the concurrency logic is correct, and the new synctest-based tests are deterministic and assert meaningful behavior (queueing, rejection, context cancellation, zero-queue, and overflow guard all covered).

A few things I verified hold:

• No counter leak / no negative QueueLen: defer currentRequests.Add(-1) runs even on the ErrResourceBusy and cancelled-context paths. Defers run LIFO, so the semaphore is released before the counter is decremented, guaranteeing len(sem) ≤ currentRequests and QueueLen() ≥ 0.
• No semaphore leak on cancellation: the select returns ctx.Err() before any sem send succeeds, so a cancelled queued call never holds a slot.
• Overflow guard is correct: maxQueueLen + budget overflow is detected and clamped to MaxUint64 (tested in TestThrottlerMaxQueueLenOverflow).
• Zero-queue fix is correct: maxRequests = budget + 0 lets the full concurrency budget run while rejecting anything beyond it (tested in TestThrottlerZeroQueue).
• The utils → utils/throttler import + ErrResourceBusy migration is consistent across all v8/v9/v10 RPC handlers and node.

Findings

Blocker

• None.

Important

• node/throttled_vm.go: VM calls always pass context.Background(), so the PR's headline cancellation feature isn't realized for the VM path — the heaviest, longest-running workloads. It's blocked on the vm.VM interface not carrying a ctx, so deferring is reasonable, but worth a tracked follow-up. (inline note posted)

Nit

• utils/throttler/throttler.go: currentRequests is atomic.Int32 while maxRequests is uint64 — consider atomic.Uint64 for consistency and to remove a theoretical overflow foot-gun. (inline note posted)
• The default maxQueueLen changed from effectively unbounded (MaxInt32) to 1024. Harmless since every production caller sets WithMaxQueueLen explicitly, and a bounded default is arguably safer — just noting the behavior change for anyone using the bare constructor.

Nice work — the option pattern and the dedicated sub-package read much better than the old setter chain.

[codecov]

Codecov Report

❌ Patch coverage is 83.60656% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.14%. Comparing base (1216dc1) to head (e8b0f7b).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
node/throttled_vm.go	50.00%	0 Missing and 2 partials ⚠️
node/node.go	66.66%	1 Missing ⚠️
rpc/v10/simulation.go	50.00%	1 Missing ⚠️
rpc/v10/trace.go	50.00%	1 Missing ⚠️
rpc/v10/transaction.go	0.00%	1 Missing ⚠️
rpc/v8/simulation.go	50.00%	1 Missing ⚠️
rpc/v8/transaction.go	0.00%	1 Missing ⚠️
rpc/v9/simulation.go	50.00%	1 Missing ⚠️
rpc/v9/transaction.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3750      +/-   ##
==========================================
+ Coverage   74.33%   75.14%   +0.81%     
==========================================
  Files         427      427              
  Lines       38501    38470      -31     
==========================================
+ Hits        28620    28910     +290     
+ Misses       7595     7554      -41     
+ Partials     2286     2006     -280     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Claude finished @rodrodros's task in 3m 0s —— View job

---

Re-review after synchronize — feat(utils/throttler): throttler revamped

• Read the new commits since the last review
• Re-checked concurrency logic in utils/throttler/throttler.go
• Verified call-site migrations and go build ./utils/throttler/... ./node/... (passes)

This push addressed the open feedback. Quick rundown of what changed and what I re-verified:

Prior nit resolved — chore(throttler): make everything a uint64 changed currentRequests from atomic.Int32 to atomic.Uint64, matching maxRequests. The type story is now consistent and the theoretical int32 overflow foot-gun from the last review is gone. The decrement uses Add(^uint64(0)) (two's-complement -1), which is correct.

Re-verified, still holds:

• No counter leak / QueueLen() ≥ 0: defer t.currentRequests.Add(^uint64(0)) runs on every exit path including ErrResourceBusy and cancelled-context. Defers run LIFO so the semaphore (<-t.sem) is released before the counter is decremented, keeping len(sem) ≤ currentRequests.
• No semaphore leak on cancellation: the select returns ctx.Err() before any sem send succeeds.
• Overflow guard correct: maxQueueLen + budget wrap is detected (maxRequests < o.maxQueueLen) and clamped to MaxUint64 (covered by TestThrottlerMaxQueueLenOverflow).
• Zero-queue fix correct: maxRequests = budget + 0 lets the full budget run while rejecting overflow (covered by TestThrottlerZeroQueue).
• Migration consistent: no remaining utils.ErrResourceBusy / utils.NewThrottler references anywhere; all v8/v9/v10 handlers + node import utils/throttler. node.go now passes uint64(cfg.MaxVMQueue) / uint64(cfg.MaxCompilationQueue).

Remaining (already acknowledged, no action needed):

• node/throttled_vm.go still passes context.Background() — cancellation isn't realized for the VM path. As you noted, VM execution can't be interrupted unless VMs run as a separate process (like the compiler), so this is correctly deferred. The inline // vm.VM carries no ctx comments document it well.

Verdict

Blocker: none · Important: none · Nit: none outstanding.

LGTM — the rewrite is correct, well-tested (deterministic synctest), and the migration is clean. No new inline comments since the code is clean and prior feedback is addressed.
· branch rdr/throttler-improvements

[brbrr]

LGTM. left a typo correction, and a note about unbounded concurrency case

[github-actions]

Claude finished @rodrodros's task in 50s —— View job

---

Re-review after synchronize — feat(utils/throttler): throttler revamped

• Read new commits since the last review
• Re-checked concurrency logic in utils/throttler/throttler.go
• Reviewed the open discussion threads (maxConcurrentReqs == 0, decrement idiom)

Only one new commit since my last review: e8b0f7bf fix(throttler): typo — applies brbrr's suggestion, fixing the // guard against overlfow → // guard against overflow comment. No logic change. The rest of the diff is unchanged from the last (clean) review, and the PR now carries brbrr's APPROVE.

On the open discussion: maxConcurrentReqs == 0

I agree with @rodrodros — brbrr's "unbounded" reading isn't what happens. make(chan struct{}, 0) is an unbuffered channel, not an unbounded one. The send t.sem <- struct{}{} only completes when a receiver is ready, and the only receiver (<-t.sem) runs after doer returns — so with a 0 budget the send can never succeed and every call blocks until ctx is cancelled (or is rejected first by the queue guard). Net effect: maxConcurrentReqs == 0 ⇒ nothing executes, which matches the documented intent. So no semaphore-capacity guard is needed here.

The only thing arguably worth a one-line doc note is that a 0 budget is a "park everything" configuration, but that's a nit and not blocking.

Verdict

Blocker: none · Important: none · Nit: none outstanding.

Still LGTM. The rewrite is correct, the synctest-based tests are deterministic, the migration is consistent, and all prior feedback (uint64 consistency, typo, zero-queue, overflow guard, VM-path cancellation deferral) is resolved or appropriately deferred. No new inline comments — the code is clean.
· branch rdr/throttler-improvements