Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

PR Review — #5175 HTTP transaction session lifecycle

Solid, well-motivated fix. The three defects are real, the concurrency reasoning is carefully documented, and the sweep-vs-request race fix (re-validating isSessionRegistered under the per-session lock, combined with checkSessionsValidity doing both cancelIfIdle() and it.remove() inside the manager write lock) is correct: once execute() holds the session lock and passes the registration check, the sweep can no longer roll it back (its cancelIfIdle() tryLock() fails → BUSY). Nice work, and the tests map cleanly onto the acceptance criteria.

A few points, one of which I think is a genuine regression worth addressing before merge.

1. (High) removeSession(exchange) on /commit /rollback bypasses ownership → cross-user transaction leak

DatabaseAbstractHandler.removeSession(exchange) removes the session by the raw header id with no ownership check, and PostCommitHandler/PostRollbackHandler call it unconditionally, even on the session-less degrade path. Trace an authenticated user B who knows user A's session id and posts POST /commit (or /rollback) with arcadedb-session-id: <A's id>:

• setTransactionInThreadLocal → getSessionById(B, A_id) returns null (ownership rejects it), and since requiresTransaction() is false for commit/rollback it degrades session-less (no throw).
• PostCommitHandler.execute: database.isTransactionActive() is false (session-less) → no commit.
• removeSession(exchange) still runs → sessions.remove(A_id) removes A's session from the map but never calls cancel().

A's TransactionContext is now untracked but still open — the idle sweep can no longer reclaim it (it is gone from the map), so it leaks until GC, holding whatever page/lock resources it had. Pre-PR these handlers never called removeSession, so this is newly introduced. The session id is an unguessable UUID so exploitability is limited, but it defeats the ownership model this PR is otherwise establishing, and it is a plain correctness bug for any shared-database scenario.

Suggested fix: only remove a session the caller actually owns, e.g. gate on the resolved session (removeSession only when activeSession != null), or have removeSession(exchange, user) re-check getSessionById(user, id) != null before removing. That also keeps the normal owned path (tx already committed/rolled back before removal) intact.

2. (Minor) POST /query does not degrade session-less like GET /query

The comment in setTransactionInThreadLocal states read-only handlers "degrade to a session-less request, keeping read-after-commit working", but only GetQueryHandler overrides requiresTransaction()→false. PostQueryHandler inherits the default true, so a stale/unresolved session on POST /query throws 404 rather than degrading. If POST /query is intended to be read-only, consider overriding requiresTransaction()→false for consistency with the stated design (not a regression — prior behavior was a hard 401 — but the read-after-commit goal is not met for POST /query).

3. (Minor) Status code change 401 → 404 for a stale session

setTransactionInThreadLocal previously set StatusCodes.UNAUTHORIZED (401); the write-capable path now surfaces HttpSessionException → 404. 404 is arguably more correct, but this is a client-visible behavior change worth a changelog/doc note for API consumers that branch on 401.

4. (Minor) Test assertion is too broad

Issue5026HttpSessionLifecycleTest.executeRefusesToRunOnAnUnregisteredSession asserts .isInstanceOf(Exception.class). Tightening to HttpSessionException would pin the contract and catch a regression where some other exception is thrown first (e.g. an NPE) while still passing.

5. (Nit) Defensive: getSessionById ownership check

if (user != null && !session.user.equals(user)) NPEs if user is non-null while session.user is null. In practice security is all-on or all-off so this cannot occur today, but a null-guard on session.user (or documenting the invariant) would be cheap insurance.

Things I checked and liked

• Catch ordering in AbstractServerHttpHandler correctly places HttpSessionException before TransactionException (it is a subclass) — 404 is reached, not the 500 fallthrough.
• removeSessionsForUser unregisters under the write lock then cancel()s outside it (avoiding holding the manager lock across the unbounded per-session wait) — the same discipline used by close().
• Idempotent-retry guard (isTransactionActive() before commit/rollback) and the invalidateHttpSessions hook on dropUser/setUserPassword/updateUser (all explicit admin actions via PutUserHandler) are appropriately scoped.

Overall: address #1 (ownership on removeSession) and I would be comfortable with this. The rest are minor/optional.

Reviewed with Claude Code.

Thanks for the thorough review. Addressed in 6913686 and 7cf87f9:

1. (High) removeSession bypassed ownership → cross-user orphan. Real bug, fixed. removeSession(exchange, user) now resolves via getSessionById(user, id) and only removes a session the caller actually owns; a foreign or already-gone id is a benign no-op, so another principal's /commit//rollback can no longer evict/orphan a session. New regression test commitWithAnotherUsersSessionDoesNotOrphanIt asserts root's session survives (and stays usable) after another user posts /commit with root's id.

4. (Minor) Test assertion too broad. Tightened executeRefusesToRunOnAnUnregisteredSession to isInstanceOf(HttpSessionException.class).

5. (Nit) getSessionById null-guard. Already added (7cf87f9, also flagged by Gemini): user != null && (session.user == null || !session.user.equals(user)).

2. (Minor) POST /query does not degrade session-less. Left as-is intentionally: PostQueryHandler extends PostCommandHandler, and flipping requiresTransaction()→false there also changes its auto-transaction wrapping semantics, which is out of scope for this fix. As you note it is not a regression (prior behavior was a hard 401/500). Can be a follow-up if we want POST /query read-after-commit parity with GET /query.

3. (Minor) 401 → 404 for a stale session. Intentional and, as you say, more correct (the id is absent, not an auth failure). Noted in the tracking doc; worth a changelog line at release.

Code Review: PR #5175 - HTTP transaction session lifecycle (#5026)

Thorough, well-documented fix. The three defects are real, the root-cause analysis is accurate, and the locking discipline is careful. Below are strengths, then discussion points and a few gaps.

What's done well

• Correct locking discipline. removeSessionsForUser unregisters under the manager write lock, then calls cancel() (which waits on the per-session lock) outside it - matching the pattern already used by close(), and avoiding holding the manager lock while blocking on an in-flight command.
• Deadlock-free sweep race fix. The execute() re-validation works precisely because checkSessionsValidity performs cancelIfIdle() + it.remove() atomically under the manager write lock, and cancelIfIdle uses a non-blocking tryLock() on the session. So a request either sees the session still registered (sweep skipped it as BUSY) or sees it gone.
• Catch-ordering is correct. HttpSessionException extends TransactionException, and AbstractServerHttpHandler catches the subclass first - the comment even calls this out. 404 vs a silent implicit-commit is the right semantic.
• Idempotency guard is right. isTransactionActive() before commit/rollback, combined with requiresTransaction()==false on those endpoints (so a removed session degrades session-less rather than 404), gives the retried-204 behavior, and the ITs verify it end to end.
• Good null-safety for embedded use (invalidateHttpSessions no-ops when the HTTP server is absent), and session.user/id are public final so the lock-free reads in the manager are safe.

Discussion points

1. Behavior change for reads (worth confirming intent). Previously every handler with an unresolvable session id returned 401. Now read handlers (GET /query, requiresTransaction()==false) silently degrade to a session-less request and return 200. A client that passed a session id specifically to read-its-own-writes within an open transaction will no longer be told the session expired - the read just runs outside the session. That is defensible (and is what "read-after-commit preserved" refers to), but it is a semantic change from a hard 401 to silent degradation. Consider documenting it, and note there is no HTTP-level test asserting the read-degradation path (the ITs only exercise writes) - the "read-after-commit preserved" claim is currently untested at the HTTP layer.

2. updateUser invalidation may be too aggressive (low). dropUser and setUserPassword invalidating sessions is clearly correct. updateUser unconditionally rolling back all of a user's live transactions is heavier: a benign metadata update via PUT /user will force-rollback unrelated in-flight transactions for that principal. Consider scoping the invalidation to changes that actually affect authz/credentials, or at least documenting that PUT /user terminates the user's open transactions.

3. Ownership still ultimately rides on equals-by-name + the invalidation hooks (defense-in-depth, low). The recreated-same-name protection depends entirely on every user-removal/mutation path routing through dropUser/updateUser/setUserPassword. Any other path that replaces the users map (e.g. a reload of users from disk, or a future admin flow) would leave stale sessions adoptable by a same-name principal, since getSessionById itself compares by name. A short comment pointing at the single invalidation entry point (or centralizing it) would make that invariant harder to break later.

Minor / nits

4. Per-command overhead (perf mantra). execute() now acquires the manager read lock (isSessionRegistered, a map lookup) on every session command, inside the session lock. Negligible in practice, but it could be avoided entirely by having removal flip a volatile boolean on the HttpSession itself (set in removeSession/cancelIfIdle), which execute() already reads under the session lock - no manager lock, no map lookup on the hot path.

5. Import ordering in ServerSecurity.java: import com.arcadedb.server.http.HttpServer; is inserted between DefaultConsoleReader and ServerException, breaking the existing alphabetical grouping.

Test coverage

Good overall: unit tests exercise the manager/session directly and the IT covers commit/rollback removal, stale-write 404, idempotent retries, and drop-user invalidation across every server. Gaps: (a) no HTTP test for read (GET /query) degradation with a stale session id; (b) no direct test of the sweep-vs-request race under real concurrency (the unit test simulates the removal ordering rather than racing threads, which is a reasonable simplification but leaves the actual interleaving uncovered).

Overall this is a solid, carefully-reasoned fix. The main thing I'd want an explicit decision on is point #1 (silent read degradation vs. an explicit signal), plus a test for it.

Code Review - PR #5175 (fix #5026: HTTP transaction session lifecycle)

Reviewed against CLAUDE.md conventions. Overall this is a strong, well-scoped fix: the three defects are real, the reasoning in the Javadoc/comments is excellent, lock ordering is consistent (session lock then manager lock everywhere; the sweep uses non-blocking tryLock, so no deadlock), and the test coverage (unit + HTTP IT) is genuinely good. A few things worth considering before merge.

Correctness / concurrency

1. (Medium - liveness) Unbounded cancel() wait held under the ServerSecurity monitor. dropUser/updateUser/setUserPassword are synchronized, and they now call invalidateHttpSessions -> removeSessionsForUser -> session.cancel(). cancel() waits unbounded on the per-session lock for any in-flight command (by design, per its Javadoc). removeSessionsForUser correctly removes from the map outside the manager lock, but the cancel() still runs while the calling admin method holds the ServerSecurity intrinsic monitor. If an invalidated session has a long-running in-flight command, that admin call blocks for its full duration, and any caller that serializes on the security monitor (there is an explicit synchronized(server.getSecurity()) pattern noted at ServerSecurity.java:532) stalls with it. Admin ops are infrequent and commands are usually short, so this is low-probability, but consider snapshotting the owned sessions and running the cancel() loop outside the synchronized block, or documenting the trade-off.

2. (Low - behavioral) updateUser invalidates sessions on any update, including database-grant-only edits. PutUserHandler also routes databases (grant) changes through updateUser, so editing a user grants now rolls back that user open HTTP transactions with a 404 on their next call. That is defensible for authorization correctness, but it means uncommitted work is silently discarded on a grant edit, not just a password change. Worth confirming this is intended (and calling it out in the docs file, which currently frames it around drop/recreate and password).

Behavioral note

3. Read handlers now silently degrade a stale session to a session-less request (previously 401). For GET /query this is the intended read-after-commit behavior, but a client passing an expired session id to a read no longer learns the session expired, it just gets a fresh implicit read. Fine, but worth a line in the docs so it is a documented contract, not a surprise. (No existing tests asserted the old 401, so nothing breaks.)

Tests

4. (Minor) recreatedSameNameUserCannotAdoptPriorSession does not test what its name implies. Ownership is by name (ServerSecurityUser.equals compares name), so recreatedAlice equals the original alice. The test passes only because removeSessionsForUser already removed the session from the map, the ownership check plays no role here. The real defense against same-name adoption is invalidation-on-drop, which is correct, but the assertion would pass even if the ownership gate were removed. Consider adding a companion assertion (e.g. that a still-registered session would resolve for a same-name principal) to document that removal, not identity, is the guard.

Otherwise the IT coverage (commit/rollback remove session, stale write to 404, idempotent retry to 204, foreign-session commit is a no-op, drop-user invalidation) is exactly right, and the execute() re-validation happy-path/negative-path pair is a nice touch.

Nits

• ServerSecurity.java import: com.arcadedb.server.http.HttpServer is placed before com.arcadedb.server.ServerException, which is out of alphabetical order.
• Nice catch putting the HttpSessionException catch arm before TransactionException in AbstractServerHttpHandler (since it extends it), the comment makes the ordering intent clear.

Nothing here is a blocker; items 1 and 2 are the ones I would want a second look at. Good work.

Automated review by Claude.

Thanks - second round addressed in 1d799aa:

1. Silent read degradation + missing HTTP test. Added staleSessionOnReadDegradesToSessionlessRead: after /commit removes the session, a GET /query carrying the stale id returns 200 with the committed row (read-after-commit preserved at the HTTP layer). The 401→200-degradation for reads is intentional; noted in the tracking doc for the changelog.

2. updateUser invalidation too aggressive. Agreed - removed. A metadata update no longer force-rolls-back the principal's in-flight transactions; per-request authorization is still re-checked on every command, so a narrowed grant is enforced without tearing down unrelated work. Only dropUser and setUserPassword invalidate live sessions, matching the issue's Defect 3 scope.

3. Ownership invariant / centralization. Added a comment on getSessionById documenting that recreated-same-name protection depends on removeSessionsForUser being invoked from every user removal/mutation path (dropUser/setUserPassword), so any future users-map-replacing path must also invalidate here.

4. Per-command isSessionRegistered overhead. Kept the manager read-lock lookup as the single source of truth for "is this session still live". A volatile boolean on HttpSession would need to be flipped consistently at every removal site (removeSession, removeSessionsForUser, the sweep's it.remove()), and a missed site would leave a stale flag - a correctness risk that outweighs saving an uncontended read-lock map lookup on a path already inside the per-session lock. Happy to revisit if it ever shows up in a profile.

5. Import ordering in ServerSecurity. Fixed - http.HttpServer now sorts after the server.* classes.

3 (behavior)/concurrency test gap: the sweep-vs-request race is covered deterministically (removal ordering) rather than by racing real threads; the existing HttpSessionTimeoutRaceTest covers the thread-racing BUSY path. Left as-is.

Thanks - addressed in daeda6b. (Note: this round reviewed 6913686, before 1d799aa, so items 2, 3-docs and the import nit were already fixed there.)

1. (Medium, liveness) Unbounded cancel() under the security monitor. Fixed. dropUser now mutates the users map under the monitor but calls invalidateHttpSessions (which can block on an in-flight command via cancel()) OUTSIDE the synchronized scope, so a long-running victim command no longer stalls admin ops serialized on the security monitor. setUserPassword is not synchronized, so its invalidation was already outside any monitor. Documented the constraint on invalidateHttpSessions.

2. updateUser invalidation. Already removed in 1d799aa - updateUser no longer touches live sessions; only dropUser/setUserPassword do.

3. Read degradation contract. Documented in the tracking doc (behavior-changes section) and covered by the new staleSessionOnReadDegradesToSessionlessRead HTTP test (also added in 1d799aa).

4. (Minor) recreatedSameNameUserCannotAdoptPriorSession didn't exercise the real guard. Good point - added a companion assertion that a same-name principal WOULD resolve the session before invalidation, pinning that removal-on-drop (not the identity check) is the guard.

Nit (import order). Already fixed in 1d799aa - http.HttpServer now sorts after the server.* classes.

All new + existing session/security tests remain green.

Review: PR #5175 - HTTP transaction session lifecycle (#5026)

Thorough, well-documented fix. The three defects are clearly separated, each with a targeted change and regression coverage, and the concurrency reasoning in the comments is genuinely careful. I checked the lock-ordering and the degradation paths and they hold up. A few notes below, one of which I think is worth addressing before merge.

What's solid

• Lock ordering is correct. execute() acquires the session lock, then briefly takes the manager read lock (isSessionRegistered). The reclaim paths (checkSessionsValidity uses tryLock() and returns BUSY; removeSessionsForUser/close() call cancel() outside the manager write lock) never block-acquire a session lock while holding the manager write lock, so there is no deadlock. The re-validation + tryLock-based sweep together genuinely close the Defect 2 race.
• Idempotency of retried /commit and /rollback (guarded by isTransactionActive(), degrading to session-less via requiresTransaction()==false) is correct and covered by tests.
• Read-after-commit degradation is preserved and pinned by staleSessionOnReadDegradesToSessionlessRead.
• Good defensive touches: null-safe ownership check, ownership-gated removeSession so a foreign id cannot evict/orphan a session, invalidateHttpSessions no-op when the HTTP server is not running (embedded).

Main concern (medium) - Defect 3 protection is leader-only in HA

The recreated-same-name defense relies on removeSessionsForUser being called from every path that removes/replaces a user. It is wired into the direct dropUser/setUserPassword API calls, but not into the two paths that replace the whole users map:

• applyReplicatedUsers(...) (ServerSecurity.java:564, the Raft apply path invoked on every peer for a SECURITY_USERS_ENTRY) swaps this.users without invalidating any sessions.
• loadUsers() / reload (ServerSecurity.java:154) does the same.

Consequence: when a user is dropped on the leader, followers apply the change via applyReplicatedUsers and their follower-local HTTP sessions for that user are not invalidated. Because ownership compares by name (ServerSecurityUser.equals), a request hitting that follower with the old session id and recreated same-name credentials will still resolve the prior session - exactly the adoption the PR set out to prevent. Sessions are node-local and /begin can run on a follower (reads), so this is reachable, not theoretical.

The invariant comment you added in getSessionById ("Any future path that replaces the users map ... must also invalidate the affected sessions") is already violated by applyReplicatedUsers, which is an existing path, not a future one. Suggest diffing the previous vs new user set inside applyReplicatedUsers (and reload) and calling removeSessionsForUser(name) for every user that was dropped or whose password/config changed. The IT passes today only because it drives dropUser directly on the same node that holds the session.

Minor

• Redundant lookup (DatabaseAbstractHandler.removeSession, line 300): inside /commit//rollback the session was already resolved into activeSession by setTransactionInThreadLocal; re-resolving by header here costs an extra read+write lock round-trip. Minor, but you could thread the resolved session through instead.
• Hot-path cost: isSessionRegistered() adds a manager read-lock acquisition to every in-session command. It is a shared lock so contention is low, but worth being aware of on the write hot path.
• Compat note: a stale-session write now returns 404 where the previous code set 401 (StatusCodes.UNAUTHORIZED). This is arguably more correct, but it is a client-visible status change - worth a line in the changelog/release notes. (The docs/5026-*.md note is a nice touch, though it lives in docs/ rather than the usual changelog.)
• Test note: Defect 2 is exercised by simulating the sweep (manual removeSession) rather than a real concurrent sweep. Fine as a regression guard given how hard the true race is to reproduce deterministically - just flagging that it does not prove the timing window itself.

Style/convention

Consistent with the codebase - final on params/locals, no fully-qualified names, AssertJ assertThat(...), IT suffix on the HTTP test, Apache headers present. No new dependencies.

Nice work overall - the core fix is correct; the HA replication path is the one gap I would close before merge.

Automated review; please verify the HA point against your understanding of session/replication topology.

Review: PR #5175 - HTTP transaction session lifecycle (#5026)

Overall this is a strong, well-scoped fix. The three defects are real, the reasoning is documented clearly in-code, the lock ordering is sound (invalidation releases the manager lock before the blocking cancel()), and the iterative review-response commits already tightened the sharp edges (ownership-gated removal, null-safe ownership check, moving invalidateHttpSessions outside the ServerSecurity monitor). Test coverage is good at both the unit and HTTP-e2e level. A few points below, mostly minor.

Correctness / behavior

1. POST /query does not degrade like GET /query (inconsistency). The read-vs-write discriminator is requiresTransaction(). PostQueryHandler extends PostCommandHandler, which leaves requiresTransaction() at the default true, so a stale/expired session id on POST /query (a read endpoint) returns 404, while the identical logical read via GET /query degrades to a session-less 200. The doc and comments state "Read handlers (GET /query) degrade," but POST /query is also a read and won't. Consider keying the degradation on an explicit read/write flag rather than requiresTransaction(), or at least documenting that POST /query with a stale session returns 404. Not a regression (it previously 401'd), but the read-after-commit contract is only half-delivered.

2. dropUser recreate-race can kill a fresh same-name session. dropUser mutates the users map under synchronized(this) and then calls invalidateHttpSessions(userName) outside the monitor (correct, to avoid blocking on cancel()). But in that window another thread could createUser the same name and begin a new session; removeSessionsForUser matches purely by name, so the just-created principal's brand-new session would then be rolled back too. Very narrow, and arguably acceptable given name-based identity, but worth a comment acknowledging it.

Tests

3. executeRefusesToRunOnAnUnregisteredSession simulates the sweep-vs-request race deterministically (manual rollback() + removeSession() before execute()) rather than exercising real concurrency. That is a reasonable and stable choice, but it does not prove the actual tryLock/gap ordering that Defect 2 is about. A comment noting it's a state-based simulation (not a true concurrent race) would set expectations.

4. Nice touch: recreatedSameNameUserCannotAdoptPriorSession now asserts the pre-invalidation resolve, pinning that removal (not the identity check) is the guard. Good regression intent.

Minor / nits

5. 401 -> 404 for a stale session on a write handler is a client-visible status change; it's documented in the changelog section, and 404 is defensible (also avoids leaking session existence to a non-owner), so just flagging for release-notes visibility.

6. removeSession(exchange, user) does two manager lookups (getSessionById then removeSession(id)), each taking the RW lock. Negligible, but a single ownership-checked remove method on the manager would be marginally cleaner and race-free.

Verified as fine

• Catch ordering in AbstractServerHttpHandler is correct (HttpSessionException before its TransactionException supertype).
• import io.undertow.util.StatusCodes correctly removed (no remaining use).
• Lock ordering: execute() takes session-lock then manager read-lock (isSessionRegistered); removeSessionsForUser/dropUser take manager write-lock, release it, then take the session lock via cancel() - no lock-order inversion, no deadlock.
• Idempotent retry path (requiresTransaction()==false -> null session -> isTransactionActive() guard -> 204) is correct for both /commit and /rollback.

Nice work - the concerns above are refinements, not blockers.

Reviewed by Claude

Codecov Report

❌ Patch coverage is 90.90909% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.73%. Comparing base (1b2a2d3) to head (247a9fb).
⚠️ Report is 2 commits behind head on main.

@@             Coverage Diff              @@
##               main    #5175      +/-   ##
============================================
+ Coverage     66.00%   66.73%   +0.73%     
============================================
  Files          1693     1694       +1     
  Lines        136917   137115     +198     
  Branches      29270    29317      +47     
============================================
+ Hits          90368    91502    +1134     
+ Misses        34306    33201    -1105     
- Partials      12243    12412     +169     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Review: HTTP transaction session lifecycle (#5026)

Reviewed the full diff plus the surrounding code (DatabaseAbstractHandler, HttpSession, HttpSessionManager, ServerSecurity, and the commit/rollback/begin/query handlers). This is high-quality work: the three defects are real, the fixes are correct, the lock-ordering reasoning is sound, and the inline documentation is excellent. A few things worth addressing.

Strengths

• Lock ordering is careful and correct. removeSessionsForUser and close() both unregister under the manager write lock and then cancel() outside it, avoiding holding the manager lock across the unbounded per-session wait. The new execute() re-validation (session.lock -> manager read lock) doesn't invert against the sweep, which only ever takes session.lock via non-blocking tryLock() while holding the manager lock. No new deadlock.
• Exception mapping is correct. HttpSessionException extends TransactionException and the new catch arm is correctly placed before the TransactionException arm in AbstractServerHttpHandler.
• The read/write handler split is right. requiresTransaction() is true only for the default write handler and false for /query, /begin, /commit, /rollback, so writes reject a stale session (404) while reads/tx-endpoints degrade session-less. The isTransactionActive() guards make retried commit/rollback genuinely idempotent (204).
• Strong test coverage across both the unit level (ownership, execute re-validation, removeSessionsForUser) and the HTTP/IT level (stale-write 404, idempotent retries, read-degrades-to-200, non-owner commit doesn't orphan, drop-user invalidation).

Findings

1. (Medium - security consistency) Password change via PUT /user does not invalidate live sessions.
setUserPassword() now calls invalidateHttpSessions() with the rationale "a password change re-authenticates the principal: drop its live HTTP transaction sessions." But PutUserHandler changes the password through updateUser() (updatedConfig.put("password", ...) at PutUserHandler.java:63), and updateUser() deliberately does not invalidate sessions. So the same logical operation (rotating a user's password) tears down live sessions via one endpoint (setUserPassword) but not the other (PUT /user). If an admin rotates a password to lock out a compromised principal via the REST endpoint, that principal's open transaction session survives. Consider invalidating in updateUser() when the config actually changes the password (compare old vs new encoded password), while still preserving the "pure metadata/grant update does not tear down in-flight work" behavior you documented.

2. (Low) Narrow race in dropUser between monitor release and invalidation.
dropUser now releases the ServerSecurity monitor before calling invalidateHttpSessions(userName) (good, for the reason documented). But in that window another thread could createUser the same name and /begin a session; the subsequent removeSessionsForUser(name) would then kill the freshly-created same-name user's session. Very unlikely and arguably acceptable, but worth a comment acknowledging it, since same-name recreation is precisely the scenario this fix targets.

3. (Low - test) Defect 2 (sweep-vs-request race) is only simulated, not exercised concurrently.
executeRefusesToRunOnAnUnregisteredSession reproduces the condition by manually removeSession()-ing before execute(), which validates the re-check but not the actual timing race between the sweep's tryLock/it.remove() and execute() acquiring the lock. Since HttpSessionTimeoutRaceTest already exists for the adjacent race, a short concurrent stress variant here would harden confidence that the re-validation closes the real window.

Minor / nits

• In the owner commit/rollback path the session is already resolved in setTransactionInThreadLocal, yet removeSession(exchange, user) re-resolves it with a second getSessionById (extra read-lock round trip). Combined with the isSessionRegistered check in execute(), a single session command now takes 3 manager read/write-lock round trips. All are cheap (RWLockContext), so this is negligible - noting only for completeness.
• HttpSessionException's message includes the raw session id; that's a server-generated AS-<uuid>, so no injection concern, just flagging that it's echoed in the 404 body.

Nice work overall - the fixes are correct and the reasoning in the comments will age well. Finding #1 is the one I'd resolve before merge.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}