Skip to content

Commit 56353d5

Browse files
author
Mark Pollack
committed
Model Reactor's one-shot cancel in the PendingResponses Lincheck sink
The lincheck job (scale 10) failed with "request 2 caused 0 $/cancel_request, cancelled before any signal: true". The interleaving: two threads cancel request 2. The first runs the cancel callback, remembers the id and is preempted before its pending.remove. The second finds the cancel callback already taken, runs the dispose callback, and that removes the entry; the first's remove then fails, so no cancel is reported. Reactor cannot produce this. MonoCreate's sink swaps its state once in cancel() and runs onCancel then onDispose on the winning thread; a second cancel is a no-op. The model's RecordingSink let a second cancel run the dispose callback. It now acts on the first cancel only, and a plain test pins the Reactor contract it relies on. Green at scale 10 and 20. The model still fails each planted bug: a cancel also sent from the dispose path, a cancel sent without the removal check, and an entry left behind.
1 parent e80dea5 commit 56353d5

1 file changed

Lines changed: 84 additions & 1 deletion

File tree

‎acp-core/src/test/java/com/agentclientprotocol/sdk/spec/PendingResponsesLincheckTest.java‎

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,14 @@
55
package com.agentclientprotocol.sdk.spec;
66

77
import java.util.HashSet;
8+
import java.util.Objects;
89
import java.util.Set;
10+
import java.util.concurrent.CountDownLatch;
11+
import java.util.concurrent.ExecutorService;
12+
import java.util.concurrent.Executors;
13+
import java.util.concurrent.Future;
14+
import java.util.concurrent.TimeUnit;
15+
import java.util.concurrent.atomic.AtomicBoolean;
916
import java.util.concurrent.atomic.AtomicInteger;
1017
import java.util.concurrent.atomic.AtomicIntegerArray;
1118
import java.util.concurrent.atomic.AtomicReference;
@@ -23,10 +30,15 @@
2330
import org.junit.jupiter.api.AfterAll;
2431
import org.junit.jupiter.api.BeforeAll;
2532
import org.junit.jupiter.api.Test;
33+
import org.reactivestreams.Subscription;
2634
import reactor.core.Disposable;
35+
import reactor.core.publisher.BaseSubscriber;
36+
import reactor.core.publisher.Mono;
2737
import reactor.core.publisher.MonoSink;
2838
import reactor.util.context.Context;
2939

40+
import static org.assertj.core.api.Assertions.assertThat;
41+
3042
/**
3143
* Model checks {@link PendingResponses}, the table of requests waiting for a response, with
3244
* Lincheck. Requests are registered, answered, cancelled (their sink is disposed, as a
@@ -84,6 +96,62 @@ void noRequestIsLostOrCompletedTwice() {
8496
.check(Requests.class);
8597
}
8698

99+
/**
100+
* The Reactor contract the model's sink relies on: a cancel that arrives while another is
101+
* still in the cancel callback does nothing, so the dispose callback (which removes the
102+
* entry) cannot overtake the cancel callback (which reports the cancel only if it removes
103+
* the entry itself).
104+
*/
105+
@Test
106+
void reactorRunsTheCallbacksOfOnlyTheFirstCancel() throws Exception {
107+
CountDownLatch inCancelCallback = new CountDownLatch(1);
108+
CountDownLatch release = new CountDownLatch(1);
109+
AtomicInteger cancelCallbacks = new AtomicInteger();
110+
AtomicBoolean disposed = new AtomicBoolean();
111+
AtomicReference<@Nullable Subscription> subscription = new AtomicReference<>();
112+
Mono.create(sink -> {
113+
sink.onCancel(() -> {
114+
cancelCallbacks.incrementAndGet();
115+
inCancelCallback.countDown();
116+
awaitQuietly(release);
117+
});
118+
sink.onDispose(() -> disposed.set(true));
119+
}).subscribe(new BaseSubscriber<Object>() {
120+
@Override
121+
protected void hookOnSubscribe(Subscription s) {
122+
subscription.set(s);
123+
}
124+
});
125+
Subscription sink = Objects.requireNonNull(subscription.get());
126+
ExecutorService first = Executors.newSingleThreadExecutor();
127+
try {
128+
Future<?> firstCancel = first.submit(sink::cancel);
129+
assertThat(inCancelCallback.await(5, TimeUnit.SECONDS)).isTrue();
130+
131+
sink.cancel();
132+
133+
assertThat(disposed).as("the second cancel ran the dispose callback").isFalse();
134+
release.countDown();
135+
firstCancel.get(5, TimeUnit.SECONDS);
136+
assertThat(cancelCallbacks).hasValue(1);
137+
assertThat(disposed).isTrue();
138+
}
139+
finally {
140+
release.countDown();
141+
first.shutdownNow();
142+
}
143+
}
144+
145+
private static void awaitQuietly(CountDownLatch latch) {
146+
try {
147+
if (!latch.await(5, TimeUnit.SECONDS)) {
148+
throw new IllegalStateException("not released");
149+
}
150+
}
151+
catch (InterruptedException e) {
152+
Thread.currentThread().interrupt();
153+
}
154+
}
87155

88156
@Param(name = "id", gen = IntGen.class, conf = "1:2")
89157
public static class Requests {
@@ -218,13 +286,28 @@ static final class RecordingSink implements MonoSink<AcpSchema.JSONRPCResponse>
218286

219287
volatile boolean cancelled;
220288

289+
/** Whether a cancel already ran: Reactor's sink acts on the first cancel only. */
290+
private final AtomicBoolean cancelRan = new AtomicBoolean();
291+
221292
private final AtomicReference<@Nullable Disposable> onDispose = new AtomicReference<>();
222293

223294
private final AtomicReference<@Nullable Disposable> onCancel = new AtomicReference<>();
224295

225-
/** Like Reactor's: the cancel callback, then the dispose callback, unless already signalled. */
296+
/**
297+
* Like Reactor's: the cancel callback, then the dispose callback, unless already
298+
* signalled. Only the first cancel does anything, as in Reactor's sink, whose
299+
* {@code cancel()} swaps its state once and runs both callbacks on the thread that
300+
* won: a second cancel (the caller disposes as its timeout fires, say) cannot run the
301+
* dispose callback while the first is still in the cancel callback. A model that let
302+
* it would report a lost {@code $/cancel_request} that Reactor never produces
303+
* ({@link PendingResponsesLincheckTest#reactorRunsTheCallbacksOfOnlyTheFirstCancel()} pins
304+
* that contract).
305+
*/
226306
void cancel() {
227307
cancelled = true;
308+
if (!cancelRan.compareAndSet(false, true)) {
309+
return;
310+
}
228311
Disposable callback = onCancel.getAndSet(null);
229312
if (callback != null && signals.get() == 0) {
230313
callback.dispose();

0 commit comments

Comments
 (0)