Repository navigation
Conversation
LocalSessionTurnGate kept one Semaphore per key for the lifetime of the process: acquire() inserted the entry with computeIfAbsent and the lease returned by it only called release(), so every key ever seen stayed in the map and its Semaphore could never be collected. TurnLease also documents that close() must be idempotent, but the lease was a bare semaphore::release. Each repeated close left one more permit behind, so the next two acquirers for that key could hold it at the same time -- and isRunning() then reported false while a turn was in flight. Count the leases (holders plus waiters) per key and drop the entry once that count reaches zero. The decrement and the removal run inside ConcurrentHashMap.compute, so they are atomic against a concurrent acquire() on the same key: an acquirer either joins the existing gate or, once it is gone, builds a fresh one. Eviction is driven by that count rather than by Semaphore.getQueueLength(), which cannot see a thread that took a just-released permit without ever entering the queue.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
CryoThrust
left a comment
CryoThrust
left a comment
There was a problem hiding this comment.
Checked 09582f5e37b643149fb1f29e579e3ec0a7ba1013 locally. Added two interruption controls: a pre-interrupted acquisition leaves no idle entry, and interrupting one of two queued waiters preserves the active holder and surviving waiter, then retires the entry when everyone finishes. The latter waits for both threads to enter the semaphore queue before interrupting one.
All eight LocalSessionTurnGateTest cases pass. The selected Harness/Control Plane dependency reactor passes clean verify, including formatting: 4,396 tests, zero failures/errors, 27 skips across seven modules. This is not the full repository or a live control-plane deployment. Removing only retire(key, gate) from the interruption catch makes both new controls fail; production is restored.
Test-only commit for adoption: CryoThrust@6224c21 . No production changes or competing fix PR.
The current Linux job's reported error is AgentProtocolTaskStoreHitlTest.submit_pausesOnPermissionAsking_andResumeDenyCompletes (task is already running: hitl-1); the six original gate cases pass in that same job. Windows passes. That locates the failure but does not establish its cause or make the full CI green.
|
Thanks for the independent verification — the eight gate cases and the two interruption controls line up with what I see locally. On the Linux failure you located ( Nothing in |
This branch has not been deployed
4 participants
AgentScope-Java Version
2.1.0-SNAPSHOT
Description
Fixes #3532
Two defects live in the lease that
LocalSessionTurnGate.acquire(String)returns. The issue reports the first; the second is in the same three lines and is arguably worse, because it breaks the gate's own contract.1. Idle keys are never evicted.
acquireinserted aSemaphorewithcomputeIfAbsentand the lease only calledrelease(), so every key ever seen stayed in the map and its semaphore could never be collected.2.
close()is not idempotent, andTurnLeasesays it must be.TurnLeasecarries "Implementations must be idempotent: callingclose()more than once must be safe", but the lease was a baresemaphore::release. Every repeated close leaves one more permit behind, so the next two acquirers for that key can hold it at the same time, andisRunning(key)then reportsfalsewhile a turn is in flight.That is not only a theoretical contract.
HarnessGateway.withGatedTurnbuilds itsAtomicReference<TurnLease>outside theMono.defer(...), so a second subscription of the returnedMonorunsdoFinallytwice against the same lease object.Why this does not use the eviction the issue proposes
The issue suggests releasing, then removing when
semaphore.getQueueLength() == 0. That check cannot see a thread that took a just-released permit without ever queueing —Semaphore.acquire()takes an available permit without entering the queue. So:release(), queue is empty;getQueueLength() == 0and removes the entry while C is inside;acquire, gets a fresh semaphore, and holds the key together with C.This change instead counts the leases (holders plus waiters) per key and drops the entry when the count reaches zero. The decrement and the removal run inside
ConcurrentHashMap.compute, so they are atomic with respect to a concurrentacquireon the same key: an acquirer either joins the existing gate or, once it is gone, builds a fresh one. It can never obtain a second semaphore for a key that still has a holder.Red / green
Red test on
ec9446023(currentmain):With the change:
Validation on commit 725461e:
LocalSessionTurnGateTest: 6 passed (the 3 existing tests plus 3 new ones: the map is empty after 50 acquire/close cycles; a repeatedclose()cannot open the gate twice; and 8 threads × 200 rounds on one key never overlap two holders — that last one passes before and after, and is there to catch an eviction that races a live holder).WakeupDispatcherTest: 5 passed, andControlPlaneSessionTurnGateTest: 3 passed — the control-plane gate falls back toLocalSessionTurnGate.mvn spotless:applyclean; JDK 17.Checklist
mvn spotless:applymvn test) -- the gate, wakeup and control-plane gate tests passed; the full reactor does not finish in this sandbox