fix(#5485): move RepoPool.Release channel send inside mutex#5486
Conversation
|
🤖 Finished Review · ✅ Success · Started 4:20 AM UTC · Completed 4:30 AM UTC |
Site previewPreview: https://e2842be5-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
Labels: PR modifies e2e behaviour test infrastructure (pkg/behaviourtest/world/) |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
ready-for-merge
ifireball
added
ok-to-test
|
/fs-fix rebase the commit on an up-to-date main please |
|
🤖 Finished Fix · ✅ Success · Started 1:38 PM UTC · Completed 1:44 PM UTC |
Release previously unlocked the mutex before sending the name back on the buffered channel. This left a window where the name was in neither the outstanding map nor the names channel. Under concurrent acquire/release at high concurrency (e.g. GODOG_CONCURRENCY=12), this gap could cause lost names or pool corruption. Move the channel send inside the mutex-held region and use defer p.mu.Unlock() for clarity. The send is guaranteed non-blocking because the channel buffer equals pool size and the name was removed from the channel during Acquire. Add TestRepoPool_ConcurrentAcquireRelease_PoolIntegrity which verifies that all names survive concurrent churn: after 20 workers cycle through a pool of 4, every name is back in the channel, no duplicates exist, and the outstanding map is empty. Closes #5485
🔧 Fix agent — iteration 1 (human-triggered)Rebased PR branch onto latest origin/main. The single commit was cleanly replayed on top of 42 new commits from main. All tests pass with -race, go vet clean, secret scan clean. Fixed (1):
Tests: passed Updated by fullsend fix agent |
922ef90 to
ba2eca8
Compare
|
🤖 Finished Retro · ✅ Success · Started 7:05 PM UTC · Completed 7:14 PM UTC |
Retro: PR #5486 — move RepoPool.Release channel send inside mutexVerdict: Clean workflow, no new proposals. All improvement opportunities are already tracked in existing issues. Timeline
Total agent cost: ~$2.10 | Issue-to-merge: ~15h (mostly human review wait) | Issue-to-PR: ~34 min | Rework iterations: 0 (rebase only) What went well
Evidence for existing issues (no new proposals needed)
Autonomy assessmentThis PR is a good example of high agent autonomy readiness: the code agent's fix was correct on the first attempt, the review agent approved correctly, and the human's only intervention was an operational rebase request (not a code quality concern). The human approval added no code-quality findings beyond what the agent already covered. For prescriptive concurrency bug fixes in test infrastructure with comprehensive test coverage, reduced human review wait time could meaningfully improve time-to-merge (14h of the 15h total was human review latency). This pattern is tracked in agents#104. |
1 participant
Summary
Move the
p.names <- namechannel send inside the mutex-held region inRepoPool.Release, closing the inconsistency window where a released name was in neither theoutstandingmap nor thenameschannel. This ensures atomic transition from outstanding to available under concurrent acquire/release at the concurrency levels #5441 will enable.Related Issue
Closes #5485
Changes
pkg/behaviourtest/world/pool.go: Usedefer p.mu.Unlock()and move the channel send before the deferred unlock, eliminating the gap betweendelete(p.outstanding, name)andp.names <- name. The send is non-blocking by invariant (buffer equals pool size, name was removed during Acquire).pkg/behaviourtest/world/pool_test.go: AddTestRepoPool_ConcurrentAcquireRelease_PoolIntegrity— 20 workers cycle through a pool of 4, then the test drains the channel to verify all names survived, no duplicates exist, and the outstanding map is empty.Testing
-race-racego vetcleanChecklist
!for breaking changes)Closes #5485
Post-script verification
agent/5485-fix-repopool-release-window)c9c7540c2995d9a294ede00c2444d2eb600e4852..HEAD)