Make InMemoryReactiveSessionRegistry updates atomic

[junhyeong9812]

Overview

InMemoryReactiveSessionRegistry can lose a session when saveSessionInformation and removeSessionInformation run concurrently for the same principal.

Problem

The per-principal session set was updated non-atomically:

• saveSessionInformation added the session id outside the computeIfAbsent mapping function.
• removeSessionInformation pruned the principal key with a non-atomic get → isEmpty() → remove(principal).

Interleaving (principal P owns one session S1):

1. Thread A removes S1; the set becomes empty and A is about to remove key P.
2. Thread B saves S2: computeIfAbsent(P) still sees key P, returns the same set, and adds S2.
3. Thread A removes key P → the set {S2} is orphaned; getAllSessions(P) is empty and S2 is lost.

The blocking SessionRegistryImpl already avoids this by performing both updates atomically with compute/computeIfPresent (on the same ConcurrentMap field shape and constructor); the reactive registry did not.

Fix

Mirror the blocking SessionRegistryImpl: perform the add inside compute, and the remove-and-prune inside computeIfPresent (returning null to drop the key only while the set is empty). This makes the per-principal updates atomic.

A 1000-iteration, latch-synchronized concurrent save/remove stress test is added; it fails on the previous code and passes with the fix.

Closes gh-19338

[ronodhirSoumik]

private Set<String> addSessionToSet(String principal, String sessionId, Set<String> existing) {
    if (existing == null) {
        existing = new CopyOnWriteArraySet<>();
    }
    existing.add(sessionId);
    return existing;
}

private Set<String> removeSessionFromSet(Set<String> sessions, String sessionId) {
    sessions.remove(sessionId);
    return sessions.isEmpty() ? null : sessions;
}

[junhyeong9812]

Thanks for taking the time to review this, I appreciate it! I'd prefer to keep the add/remove logic inline here, though. This change is specifically about bringing the reactive registry into line with the blocking SessionRegistryImpl, which performs the exact same mutation inline inside compute/computeIfPresent (same shape, same sessionsUsedByPrincipal parameter name). Keeping it inline mirrors that sibling and keeps the diff minimal.

Extracting also has a couple of downsides here: the proposed addSessionToSet(String principal, ...) leaves principal unused, and pulling the body out of the remapping function makes the "this must run inside the atomic compute block" intent a bit less visible (it doesn't break atomicity as long as the helper is used as the remapping function, but it's easier to misuse later). Happy to revisit if a maintainer prefers the extracted form.

[ronodhirSoumik]

sessionsUsedByPrincipal-> sessions

[junhyeong9812]

You're right that sessionsUsedByPrincipal here is the compute/computeIfPresent remapping parameter. I kept that name intentionally to mirror the blocking SessionRegistryImpl, which uses the exact same compute/computeIfPresent shape and the same sessionsUsedByPrincipal parameter name — this PR is largely about aligning the reactive registry with that sibling, so I'd lean towards keeping it for consistency. That said, it's purely stylistic, so I'm happy to shorten it to sessions in both lambdas if maintainers prefer.

[ronodhirSoumik]

this iterative block can be declared as a function - would be modular

[junhyeong9812]

Good point. I left the loop body inline because it's a single save/remove-concurrently/assert race scenario run under load, and only this one test uses it — keeping the setup, the concurrent actions, and the assertion together makes the race being exercised easier to audit, and a failure points straight at the relevant lines. I can extract it into a helper if you feel strongly, but I'm not sure it adds much here.

[ronodhirSoumik]

got it. My suggestion was actually more for better readability than future maintenance. I thought separating the method here would better the understandability of the functionality.

[junhyeong9812]

Thanks for clarifying — I gave the readability angle some more thought and I think you're right. Rather than having the reader work through the entire loop body inline, describing one race in a named method and modularizing it reads better and makes the scenario's intent clearer. I've pushed a follow-up commit (72feea0d) extracting the per-iteration body into assertAddedSessionSurvivesConcurrentSaveAndRemove(...), so the loop now just runs that race N times. Would appreciate it if you could take a look — thanks for the nudge!