Repository navigation
Conversation
NewPubSubControllerWithConfig never initialized the semaphore field, so the `if c.semaphore != nil` guard in PubSubTriggerHandler never fired and TriggerConfig.MaxConcurrentRuns was silently ignored — PubSub triggers ran agents with unbounded concurrency. The sibling NewEventarcControllerWithConfig already initializes it correctly (eventarc.go); this mirrors that one line. Add a white-box regression test asserting both controllers size their concurrency semaphore from MaxConcurrentRuns.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Thanks for the PR, @PratikDhanave. One thing before review. Our contribution guide asks for an issue behind each change, and I could not find one linked here.
If this is a small doc or typo fix, ignore me. A maintainer can open the issue for you if you would rather. |
wolo-lab
left a comment
wolo-lab
left a comment
There was a problem hiding this comment.
This fixes the bug for the adkgo launcher, and the new test fails on main as it should. There is one case to handle before merge: a MaxConcurrentRuns of zero now makes every PubSub request hang.
make(chan struct{}, 0) returns an unbuffered channel, not nil, so the if c.semaphore != nil guard in PubSubTriggerHandler passes and the send blocks forever. The send doesn't watch r.Context(), so a client timeout or disconnect doesn't free the handler either. On main the same config returns 200 straight away, because a nil semaphore meant no limit.
The launcher isn't affected, since it rejects values ≤ 0 (launcher check). It does affect anyone calling NewPubSubControllerWithConfig or the deprecated NewPubSubController directly with a TriggerConfig that leaves the field unset. Both are released API and neither validates the value. A negative value makes the constructor panic.
NewEventarcControllerWithConfig already has the same behavior (eventarc.go), so copying it moved the problem over to PubSub. Whatever you pick (create the semaphore only when the value is positive, or return an error from the WithConfig constructor), please apply it to both controllers and add a test for the zero case.
…pacity
- pubsub.go: MaxConcurrentRuns<=0 now leaves semaphore nil ("no limit"),
matching prior behavior. make(chan, 0) is an unbuffered channel (not nil),
so the guard would pass and every request would block forever.
- Replace the capacity-only test with a behavioral one: fire MaxConcurrentRuns+1
concurrent requests at a blocking agent and assert only MaxConcurrentRuns are
in flight; add a regression test that MaxConcurrentRuns=0 does not hang. Moved
into pubsub_test.go per review.
- fakes: make FakeSessionService concurrency-safe (mutex) so the new concurrency
test is race-clean; no behavior change for existing sequential tests.
65501c6 to
564ef1b
Compare
|
Thanks @wolo-lab — both addressed in the latest push. The var semaphore chan struct{}
if cfg.TriggerConfig.MaxConcurrentRuns > 0 {
semaphore = make(chan struct{}, cfg.TriggerConfig.MaxConcurrentRuns)
}The capacity-only test. Replaced it with a behavioral test in Driving the handler concurrently surfaced that (I scoped this to PubSub as requested; note |
|
Please adjust the commit msg: "Co-Authored-By: Claude Opus 4.8 (1M context) [email protected]" It blocks a CLA check. |
564ef1b to
e696245
Compare
Summary
NewPubSubControllerWithConfignever initialized thesemaphorefield, so the concurrency guard inPubSubTriggerHandleris dead code andTriggerConfig.MaxConcurrentRunsis silently ignored — PubSub triggers run agents with unbounded concurrency.The handler gates on the semaphore (
server/adkrest/controllers/triggers/pubsub.go):but the constructor returned
&PubSubController{runner: retriable}withsemaphoreleftnil. The structurally-identical siblingNewEventarcControllerWithConfiginitializes it correctly:Change
PubSubController.semaphorefromcfg.TriggerConfig.MaxConcurrentRuns, mirroring the Eventarc controller (one line).MaxConcurrentRuns.Verification
pubsub semaphore cap = 0, want 4) and passes after.go build ./...,go vet ./server/adkrest/controllers/triggers/, andgo test ./server/adkrest/controllers/triggers/all pass.🤖 Generated with Claude Code