Skip to content

Commit 6ced352

Browse files
glasstigerclaude
andcommitted
Fix flaky token-store steal-contention test
testConcurrentStealContentionPreservesMutualExclusion asserted a hard mutual-exclusion invariant (overlaps == 0, maxInside == 1) that the FileTokenStore cross-process lock does not provide. stealIfStale is best-effort by design and documents a three-actor residual - a peer recreating the lock while a second captures that fresh live lock and a third claims the freed path - under which two holders briefly run at once, degrading only to one extra token refresh (Layer-1 atomic rename keeps the credential intact). Under four-way contention that residual is reachable, so the test failed intermittently on CI. A stress harness confirmed the mechanism: 8 contenders overlapped in 12 of 700 iterations (maxInside 2), while 2 contenders never overlapped in 800 iterations. Split the one over-strict test in two: - testConcurrentStealContentionTwoWayPreservesMutualExclusion runs two contenders and keeps the strict overlaps == 0 / maxInside == 1 assertions. The three-actor residual cannot arise with two threads, so this is deterministic and still pins the exclusion the atomic capture guarantees. - testConcurrentStealContentionDegradesCleanly runs four contenders and asserts only the benign invariants - every contender runs its section and no capture temp leaks - tolerating the documented residual. No production change. Full FileTokenStoreTest is 40/40 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 1d67163 commit 6ced352

1 file changed

Lines changed: 58 additions & 8 deletions

File tree

core/src/test/java/io/questdb/client/test/cutlass/auth/FileTokenStoreTest.java

Lines changed: 58 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ public void testClearOnEmptyStoreIsNoOp() throws Exception {
185185
}
186186

187187
@Test
188-
public void testConcurrentStealContentionPreservesMutualExclusion() throws Exception {
188+
public void testConcurrentStealContentionDegradesCleanly() throws Exception {
189189
assertMemoryLeak(() -> {
190190
Path dir = storeDir();
191191
Files.createDirectories(dir);
@@ -195,14 +195,64 @@ public void testConcurrentStealContentionPreservesMutualExclusion() throws Excep
195195
Files.write(lock, "crashed-holder-stamp".getBytes(StandardCharsets.UTF_8));
196196
Files.setLastModifiedTime(lock, FileTime.fromMillis(System.currentTimeMillis() - 600_000));
197197

198-
// several "processes" race to steal the one abandoned lock. The staleness window (60s) far exceeds
199-
// each ~100ms hold, so a live holder's lock is never judged stale: only the abandoned lock is
200-
// stealable, and the atomic, stamp-verified steal must admit exactly one holder at a time. This is a
201-
// mutual-exclusion invariant guard under steal contention: it exercises the steal path concurrently
202-
// and fails on any gross loss of exclusion. It does not deterministically reproduce the narrow
203-
// isStale->delete race the atomic steal closes - that needs a timing seam this final class does not
204-
// expose - so the fix itself rests on the atomic capture + stamp re-check, not on this test alone.
198+
// several "processes" race to steal the one abandoned lock. This exercises the steal path under
199+
// N-way contention and asserts it degrades CLEANLY: every contender eventually runs its critical
200+
// section (none is starved or wedged) and no atomic-capture temp file leaks. It deliberately does
201+
// NOT assert strict mutual exclusion. stealIfStale is best-effort by design and documents a
202+
// three-actor residual - a peer recreating the lock in the isStale->capture gap while a second
203+
// captures that fresh live lock and a third claims the momentarily-free path, all at once - under
204+
// which two holders can briefly run concurrently. That residual needs three or more contenders and
205+
// degrades only to one extra token refresh (a re-prompt on a rotating-refresh-token IdP), never a
206+
// torn or forged credential, since the Layer-1 atomic-rename write is independent of the lock.
207+
// testConcurrentStealContentionTwoWayPreservesMutualExclusion pins the exclusion the atomic capture
208+
// does guarantee, deterministically, with two contenders.
205209
final int threads = 4;
210+
AtomicInteger ran = new AtomicInteger();
211+
TokenStore.CriticalSection section = () -> {
212+
Os.sleep(100);
213+
ran.incrementAndGet();
214+
return true;
215+
};
216+
217+
Thread[] ts = new Thread[threads];
218+
for (int i = 0; i < threads; i++) {
219+
// a generous acquire budget so a contender waits for the lock rather than giving up early
220+
FileTokenStore store = new FileTokenStore(dir, 30_000, 60_000);
221+
ts[i] = new Thread(() -> store.inLock(key, section));
222+
}
223+
for (Thread t : ts) {
224+
t.start();
225+
}
226+
for (Thread t : ts) {
227+
t.join();
228+
}
229+
230+
Assert.assertEquals("every contender must run its critical section", threads, ran.get());
231+
assertNoCaptureTempFiles(dir, key);
232+
});
233+
}
234+
235+
@Test
236+
public void testConcurrentStealContentionTwoWayPreservesMutualExclusion() throws Exception {
237+
assertMemoryLeak(() -> {
238+
Path dir = storeDir();
239+
Files.createDirectories(dir);
240+
TokenStoreKey key = sampleKey();
241+
// a lock abandoned by a crashed holder, backdated well past the staleness window
242+
Path lock = lockFile(dir, key);
243+
Files.write(lock, "crashed-holder-stamp".getBytes(StandardCharsets.UTF_8));
244+
Files.setLastModifiedTime(lock, FileTime.fromMillis(System.currentTimeMillis() - 600_000));
245+
246+
// TWO processes race to steal the one abandoned lock. With exactly two contenders the steal is
247+
// deterministically mutually exclusive: the atomic capture (rename) lets exactly one steal the
248+
// abandoned lock, and once a winner holds a freshly-stamped lock the loser reads that live stamp,
249+
// judges it not stale (the 60s window far exceeds each ~100ms hold) and waits rather than stealing
250+
// it; the empty-lock grace likewise stops the loser stealing the winner's lock in its brief
251+
// create->stamp gap. The three-actor residual stealIfStale documents - a peer recreating the lock
252+
// while a second captures it AND a third claims the freed path - structurally cannot arise with two
253+
// threads, so exclusion holds exactly here (the N-way best-effort path is
254+
// testConcurrentStealContentionDegradesCleanly).
255+
final int threads = 2;
206256
AtomicInteger inside = new AtomicInteger();
207257
AtomicInteger maxInside = new AtomicInteger();
208258
AtomicInteger overlaps = new AtomicInteger();

0 commit comments

Comments
 (0)