fix(#10790, #10752, #10707, #10567): a Redis key is the client's key, a stopped server opens no database, a database on disk is a database prefix - #10812

Merge Risk: Moderate · previous review 2528d

A transient directory-listing failure can make the server fail to recognize an unopened database. Redis writes with that database prefix are then stored as ordinary keys instead of in the database, and this persists until the directory changes. Fix this before merging by not publishing a failed listing and retrying on the next lookup.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Out of Scope Changes check		The PR includes production behavior and tests for objectives outside the two active directly linked issues. The #10707 changes add a stopped-server database-open barrier. The #10567 changes add on-dis…	Link the active issues #10707, #10567, and #10752 to this pull request, or move their implementation and tests to separate pull requests. Keep this pull request limited to the #10790 and #10694 key-identity changes and supporting tests.
Docstring Coverage		Docstring coverage is 34.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title names four issues and four changes, which is a long multi-part title. It is partially related to the changeset: the Redis key identity change (#10790/#10694), binary key preservation (#10752…
Description check		The description follows the template structure. It has 'What changes' (equivalent to 'What does this PR do?'), a motivation embedded in the per-issue sections, 'Behaviour changes (release notes)', and…
Linked Issues check		The PR satisfies #10790 and #10694. DatabaseInternal.getGlobalVariableStore() and its LocalDatabase, ServerDatabase, and RaftReplicatedDatabase implementations provide exact-key storage. The R…

Full details: Title check

Explanation

The title names four issues and four changes, which is a long multi-part title. It is partially related to the changeset: the Redis key identity change (#10790/#10694), binary key preservation (#10752), stopped-server database gating (#10707), and on-disk database prefix recognition (#10567) all appear in the diff. The title is overly broad and reads as a list of issue summaries rather than one primary change. It also runs to 145 characters, above the concise single-sentence guideline, and it uses a 'fix(...)' prefix with issue numbers in the scope. The changeset does cover all four topics, so the title is not off-topic.

Full details: Description check

Explanation

The description follows the template structure. It has 'What changes' (equivalent to 'What does this PR do?'), a motivation embedded in the per-issue sections, 'Behaviour changes (release notes)', and 'Tests'. The 'Related issues' content is given inline as 'Fixes #10790, #10752, #10707, #10567' and the 'Additional Notes' content is partially covered by the release notes and the superseded #10791 discussion. The Checklist section is missing: it does not state whether 'mvn clean package' was run or confirm that unit tests cover failure and success scenarios. The description also details tests and documentation in an external repository, and it explains the reasoning for each change clearly. The missing checklist and the lack of a separate Motivation heading are minor gaps, so the description is mostly complete.

Full details: Linked Issues check

Explanation

The PR satisfies #10790 and #10694. DatabaseInternal.getGlobalVariableStore() and its LocalDatabase, ServerDatabase, and RaftReplicatedDatabase implementations provide exact-key storage. The Redis wire and redis query paths use this store for database-backed keys. This preserves $k and k as separate keys and permits parent, current, depth, path, stack, and history. Issue10790RedisReservedKeyNamesTest covers reserved names, dollar-prefixed names, SQL visibility, query-language commands, and transactions. Issue10790GlobalVariableStoreTest verifies exact store keys and the separate SQL rules.

Full details: Out of Scope Changes check

Explanation

The PR includes production behavior and tests for objectives outside the two active directly linked issues. The #10707 changes add a stopped-server database-open barrier. The #10567 changes add on-disk database discovery and Redis on-demand opening. The #10752 changes add opaque binary-key argument handling and a binary-key census. These changes are not needed to implement #10790 or #10694.

Resolution

Link the active issues #10707, #10567, and #10752 to this pull request, or move their implementation and tests to separate pull requests. Keep this pull request limited to the #10790 and #10694 key-identity changes and supporting tests.

Full details: Docstring Coverage

Explanation

Docstring coverage is 34.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 95 functions across 19 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

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.}

Review

Reviewed the Redis, engine and HA changes. I did not read the server-side diffs for #10707 and #10567 (ArcadeDBServer, ServerDatabase, DatabaseDirectoryNames) in full, and I did not run the tests.

Overall: the approach is sound. Reading and writing the global-variable map directly fixes the $-stripping and reserved-name problems at the source. It also removes six duplicated helpers. There are no security hazards in what I read. Each issue has a dedicated regression test, and the census test for #10752 is a good guard against future key commands being added without a probe.

Points to consider

1. Exposing the raw map widens the contract. getGlobalVariableStore() returns the mutable ConcurrentMap, and callers now skip validateVariableName (null and reserved names). The DatabaseInternal Javadoc should say that this is a raw store, that callers own key validation, and that the SQL reserved names must not be relied on. It would also help to say that null values throw an NPE and that removal must use remove. In the Redis code a put of null would NPE where setGlobalVariable(..., null) used to remove. The hDel change correctly switched to remove. Please confirm that no other path, such as setVariable callers or INCR with a null value, can reach put(null).
2. getGlobalVariable now hides reserved names. This is a behavior change for every SQL caller, not only Redis. If anything legitimately stores parent or depth through another path (for example the redis query language), SQL can no longer read it. Is that intended? It matches the PR text, but a note in the changelog would help.
3. Raft/HA. RaftReplicatedDatabase.getGlobalVariableStore() returns the local node's map, which is consistent with the existing non-replicated caveat. Please extend that comment block to mention the new accessor (it only lists the older ones).
4. isDatabaseOfServer runs on every prefixed key command. The Javadoc says the directory part is an in-memory listing, so there is no stat per command. Please confirm this stays true when a database is created or dropped outside the server, and that the cache is invalidated correctly. A stale listing would turn a foo.key write into a connection-local write again, which is the A database that is not available (restore in progress, closed by command, being dropped) reaches gRPC, Redis and MongoDB clients as a generic server error #10553 failure mode.
5. Minor style. The field alignment in RedisNetworkExecutor (ConcurrentMap<String, Object> defaultBucket) breaks the column alignment of its neighbours. RedisArguments could be built with a small helper instead of the static-init merge loop, but that is optional.

Tests

Coverage looks good. As required, please confirm that the two reworked Issue8254* tests and SQLMethodTransformTest still pass. These are the existing tests the change touches, so they are the likeliest to be affected by the semantic changes.

Nothing blocking from my side.

Codecov Report

❌ Patch coverage is 84.81013% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.02%. Comparing base (2a205e8) to head (d40d11c).
⚠️ Report is 5 commits behind head on main.

@@            Coverage Diff             @@
##               main   #10812    +/-   ##
==========================================
  Coverage     79.01%   79.02%            
+ Complexity     4768     4767     -1     
==========================================
  Files          2302     2303     +1     
  Lines        251584   251716   +132     
  Branches      54903    54927    +24     
==========================================
+ Hits         198785   198910   +125     
+ Misses        32544    32543     -1     
- Partials      20255    20263     +8     

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

Addressed in 62eeca5:

1. getGlobalVariableStore() Javadoc now says the store is raw: no validation, null key/value refused, removal is remove. No Redis path can reach put(null): a RESP $-1 argument is refused before any handler runs, the query language's SET value is a parsed token, compute returning null removes (as ConcurrentHashMap defines), and the overlay's buffered null (GETDEL) is published with remove.
2. Intended: a reserved name is the SQL context's own, and SQL cannot SET GLOBAL one, so any entry under such a name came from another key space. Documented in the Redis docs ("Keys" section).
3. The HA comment block now lists computeGlobalVariable and getGlobalVariableStore (seven accessors).
4. The listing is invalidated by every change the server makes itself (deregister/close, drop, restore release, new lifecycle), so those are seen at once. A change made outside the server (a directory copied in or deleted) is picked up within one second: one stat of the directory's mtime, plus one per sub-directory that held no database yet (a copy in progress), with a guard for coarse mtime granularity. Issue10567RedisDatabaseOnDiskTest.aDroppedDatabaseIsNoDatabaseAnyMore covers the drop.
5. Kept: ConcurrentMap<String, Object> is wider than the declaration column, like selectedDatabaseName next to it.

Also in this push: the first CI run exhausted the HTTP port range in redisw. Five Redis test classes (three of mine copied the pattern from Issue9057BinarySafeAndHmgetMissingRidTest) overrode endTest() without @AfterEach, so JUnit never stopped their servers. The overrides were redundant and are removed, and BaseGraphServerTest now refuses such an override at the first test. Main's redisw run already peaked at port 2488 for the same reason. The other unit-lane failures were Issue10153AlreadyExistsTest (red on main too) and four timing tests (Issue10747, QueryHeapBudgetIssue8591, Issue10160, gremlin QueryAdmissionGate…Issue9518), which pass locally on this branch.

Review of #10812 (Redis key space, binary keys, stopped server, databases on disk)

I read the diff only. I did not build it or run the tests. I found nothing that needs a security label. Findings are below, with the most important first.

Design

The getGlobalVariableStore() approach is clean. It removes the six duplicated helper paths in RedisNetworkExecutor (database-backed and connection-local), and ResolvedKey(key, space) now covers both key spaces. RedisArguments also gives one auditable table for binary-safe positions, and the census test enforces it.

Points to consider

1. Behaviour change for existing data (Redis wire: keys named parent, current, depth, path, stack or history are refused as reserved SQL variables on the database-backed key space #10790). Before this PR, Redis SET $k v stored the key k. Now it stores $k, and SQL can no longer read it as $k. Redis SET k v is still visible to SQL as $k. Values written under the old rule stay under the stripped name, so a client that reads $k after the upgrade gets nothing. I'd call this out in the release notes or the PR description as an intentional break.
2. Reserved-name check on every SQL read. LocalDatabase.getGlobalVariable now does a RESERVED_VARIABLE_NAMES.contains(name) lookup on every call. That is cheap if the collection is a Set. Please confirm it is one and not a list. Also check that getGlobalVariables() (which still returns the full map) and computeGlobalVariable are consistent with the "reserved names are absent" rule, since the new tests only cover getGlobalVariable.
3. Access to a database that is on disk but not open (Redis: a <database>.<key> whose database is on disk but not open yet is kept as a plain key of the connection and acknowledged with OK #10567). Any foo.bar key where foo is a directory on disk now goes through getAuthorizedDatabase("foo"). Before, a plain dotted key such as foo.bar worked as a connection-local key. Now a user with no access to foo may get a security error instead. This already happened for open databases, so it is consistent. Still, consider whether a "not authorized" result should fall back to treating the key as plain, or whether failing is the intended choice. It is a visible change, so a test pinning the intended behaviour would help.
4. DatabaseDirectoryNames races and cost.
   • Several threads can pass the nextCheckNanos test at once and each run the lastModified and candidate stats. That is harmless, but you could use a compareAndSet on the next check time to let one thread do it.
   • The candidate list is re-stat'd every second. With thousands of non-database sub-directories this becomes noticeable.
   • list() uses File.listFiles, which returns null on I/O error. That is handled, but it yields an empty listing for a second. A transient error therefore makes databases look absent, and prefixed keys silently go to the local bucket. A log line at FINE or WARNING level would help here.
5. databasesClosedByStop (A stopped server reopens its databases on demand, so a security-reload sweep still running at stop can lock 'graph' and fail the next test's startServers with OverlappingFileLockException #10707). The flag is cleared at the start of startInternal. A concurrent late getDatabase that runs between the clear and the real start would load databases early. This is probably fine, since it only happens once the server is starting anyway. Confirm that HA restarts (stop followed by start on the same instance) do not depend on a load happening between those two points.
6. BaseGraphServerTest.checkEndTestIsALifecycleMethod. This is a good guard. It does apply to every subclass in every module, so one existing offender will now fail its whole class. Please check that the whole test tree was scanned, not just the modules this PR touches, for example ha-raft and e2e.
7. Style and CLAUDE.md.
   • RedisNetworkExecutor field alignment was broken by the ConcurrentMap change (defaultBucket). It's cosmetic.
   • The new Javadoc comments are very long. That is fine, but a few can be shortened.
   • I saw no System.out and no new dependencies.

Tests

Coverage is good: engine store test, Redis reserved-name test, binary-key census, on-disk test, and stopped-server test. Two suggestions:

• Add a test for a key that starts with $ ($k) read back through SQL $$k and $k. This pins point 1.
• Add a concurrency test for DatabaseDirectoryNames invalidation during list(), or at least one that checks an invalidation that lands mid-listing is not overwritten by a stale publish.

Overall this looks solid. Points 1 and 3 are the ones I'd want a maintainer decision on before merging.

Second review, addressed in 855f8c9:

1. $k written before the upgrade: global variables are in memory only (they are not persisted and not replicated), so an upgrade, which is a restart, starts with an empty map. No stored key changes meaning. $k and k being two keys is the fix of On the Redis wire a key starting with $ is the same key as the one without it: GET, SET, GETDEL, EXISTS and INCR alias <db>.$k onto <db>.k, so one write overwrites another key #10694, and the docs' "Keys" section says so.
2. RESERVED_VARIABLE_NAMES is a Set.of (O(1)). The SQL writers (setGlobalVariable*, computeGlobalVariable) still refuse a reserved name, so SQL can neither write nor read one. getGlobalVariables() is the raw listing and shows what the store holds, Redis keys included.
3. No access to a database on disk: intended, and now pinned by Issue10567RedisDatabaseOnDiskTest.aUserWithoutAccessIsRefusedAndTheDatabaseIsNotOpened: NOPERM for both <db>.<key> and SELECT, and the database is not opened for that user (getAuthorizedDatabase checks the grant before it opens). Falling back to a plain key would make what <name>.<key> names depend on who asks, and it is what open databases already do.
4. Several threads may each pass the 1-second check, at the cost of one stat each; that is harmless and keeps the hot path lock-free. Non-database sub-directories in the database directory are a handful at most (system/storage directories). A directory that cannot be listed is now logged at WARNING. DatabaseDirectoryNamesTest.anInvalidationDuringAListingIsNotLostToIt pins the case where an invalidation lands while a listing that began before it is running.
5. Clearing the stop flag in startInternal is the intent: a starting server loads its databases. HA restarts go through start() on a new lifecycle like any other.
6. The scan covered every module's test sources (server, ha-raft, e2e, protocol modules). The only endTest() overrides without @AfterEach were the five redisw ones, now removed; the rest are TestHelper hooks, which the guard does not see.

Review

Overall this is a clean, well-scoped change, and the tests are thorough: a census test for the binary-key table, a regression test per issue, and stop/restart coverage for #10707. I found no security hazard, so I'm not adding the do-not-merge label. I didn't build or run the tests here, so this is a read of the diff only.

What looks good

• getGlobalVariableStore() collapses six near-identical helpers in RedisNetworkExecutor into a single ConcurrentMap path. The atomic putIfAbsent, computeIfPresent and compute guarantees are kept for both key spaces.
• Authorization is unchanged. A database prefix still goes through getAuthorizedDatabase before the store is touched, so the wider isDatabaseOfServer check can't bypass permissions.
• databasesClosedByStop is set and read under databasesLock, and cleared in startInternal. That ordering closes the window described in A stopped server reopens its databases on demand, so a security-reload sweep still running at stop can lock 'graph' and fail the next test's startServers with OverlappingFileLockException #10707.
• RedisArguments gives one table of opaque positions. The lookup is done once per command rather than once per argument, and keyPositions returns a clone.

Points to consider

1. Behaviour change for dotted keys. A plain key such as user.session.1 now goes to the database store whenever a directory named user that holds a database exists on disk, even if it isn't open. Before, it went to the connection-local bucket. This is the intent of Redis: a <database>.<key> whose database is on disk but not open yet is kept as a plain key of the connection and acknowledged with OK #10567. But a client without access to that database now gets an authorization error, or a LOADING reply for a pending snapshot, on a key it used to be able to write. I'd mention it in the release notes.
2. Compatibility of $k vs k. Redis $k and k are now two keys in the database store, and SQL $k only reads k. This is documented in the code, but a Redis key that starts with $ is no longer reachable from SQL. It's worth a line in the docs.
3. Raw store exposure. getGlobalVariableStore() hands out the live mutable map, with no validation and no null handling (a ConcurrentHashMap throws NPE). The Javadoc says so. If the Redis wrapper stays the only caller, I'd consider narrowing the visibility or adding a @apiNote that the method is internal. SQLMethodTransformTest returning null from its stub is fine.
4. DatabaseDirectoryNames.contains. When a re-check falls due, every concurrent caller runs check() or list() independently. That means several threads can stat the directory and its candidates at the same moment, once per second at most. This is minor, but a single-flight guard such as a CAS on nextCheckNanos would remove the duplicate work on a busy Redis port. With many non-database sub-directories, the candidates loop is one stat per directory on every re-check.
5. stopInternal ordering. databasesClosedByStop is set only after the services have stopped, so a request that races the earlier stop steps can still open a database. This looks intended, since the close loop follows right after. A one-line comment saying so would help the next reader.
6. Style (CLAUDE.md). I only noticed one small point. The field alignment in RedisNetworkExecutor was broken by the ConcurrentMap<String, Object> defaultBucket change, so the column alignment of the neighbouring fields now differs. Otherwise the new code matches the surrounding style (final usage, braces-free single statements, AssertJ).

Tests: coverage is good. One suggestion: a test where a Redis client hits a dotted key with a prefix naming a database it lacks permission for, to pin down point 1.

🤖 Generated with Claude Code

Third review, addressed in 2528daa:

• 4: a due re-check of the directory listing is now single-flight (one caller runs it, the others answer from the listing in force).
• 1, 2: listed under "Behaviour changes (release notes)" in the PR description. The $ behaviour is also in the docs' Keys section. The permission case is pinned by Issue10567RedisDatabaseOnDiskTest.aUserWithoutAccessIsRefusedAndTheDatabaseIsNotOpened (855f8c9).
• 3: getGlobalVariableStore() is on DatabaseInternal, which is already the internal interface, and its Javadoc says the store is raw. An interface method cannot be narrowed.
• 5: the barrier comment in stopInternal already says why it sits right before the close loop.
• 6: kept. The type is wider than the declaration column, like PreAuthConnectionGate.Ticket preAuthTicket in the same block.

Review

Overall this is a clean, well-reasoned change. I found no security hazards, so I'm not adding the "do not merge" label.

What I checked

• getGlobalVariableStore() plus the single ConcurrentMap path in RedisNetworkExecutor removes six near-duplicate helpers and fixes the $-stripping and reserved-name problems at the source. The reasoning for not taking the $-escape approach of fix(#10694): a Redis key starting with $ is no longer the same key as the one without it #10791 holds up.
• RedisArguments gives one table of opaque positions. The lookup is done once per command and not per argument, which is good for the hot path. The census test that fails when a key command has no probe is a nice guard.
• DatabaseDirectoryNames is lock-free on reads. The generation counter stops an old listing from hiding an invalidation, a single checking flag keeps concurrent commands from all statting the directory, and the mtime-granularity guard is a thoughtful touch.
• databasesClosedByStop is set and read under databasesLock, so a load in flight either registers before the close or is refused. It is cleared at the start of startInternal.
• I checked the setVariable / computeVariable call sites. None can pass a null value, so ConcurrentHashMap's refusal of nulls is safe. The old setGlobalVariable(x, null) removal is replaced by an explicit remove in hDel.

Points to consider (non-blocking)

1. Existence oracle and on-demand open. Any authenticated Redis user can now cause a closed database to be opened by sending <name>.<key>. They get NOPERM for names they can't access, versus an ordinary key for names that don't exist. This lets them learn which database directories exist. Open databases already had this behaviour, so the extra exposure is small. It is still worth a line in the docs, or consider answering NOPERM only when the user has some access to the server.
2. Behaviour change for dotted keys. A client that today uses a key like sales.2024 while a sales directory exists on disk will have its key redirected into that database, or get NOPERM. You list this in the release notes. Because it can break existing deployments, a stronger upgrade note may be warranted.
3. Listing failure. In DatabaseDirectoryNames.list(), children == null logs a WARNING on every re-listing, which is at most once per second under steady failure. That could flood the logs. Consider rate-limiting the warning.
4. Race at the edge of the recheck. In contains(), the generation comparison and the nextCheckNanos comparison are read separately. A listing published between those reads is harmless, but a short comment saying so would help the next reader.
5. Style. In RedisNetworkExecutor, the field alignment on defaultBucket no longer matches its neighbours. The ConcurrentMap<String, Object> defaultBucket = ... line widens the column, so the other fields look off.

Tests
Coverage is good: each of the four issues has a regression test, and the two tests that pinned the old behaviour were updated. I did not run the suite myself. Your PR description reports the redisw, server, ha-raft and engine runs green, so please confirm CI agrees.

Fourth review: 3 addressed in 37920a1 (the listing failure is logged once per failure, not at every re-listing). Not changed:

• 1: a name's existence was already observable this way for open databases, and over HTTP. The on-demand open happens only for a user granted the database: getAuthorizedDatabase checks the grant before it opens.
• 2: listed under "Behaviour changes (release notes)" in the PR description.
• 4: a listing published between the two reads is answered from either listing, both current; no change in behaviour.
• 5: kept, see the previous reply.

Review of #10812 (Redis global-variable store, binary keys, on-disk default DB)

Overall: The change is well structured. It exposes one raw ConcurrentMap store and collapses the six key helpers in RedisNetworkExecutor into a single code path. RedisArguments is a good single table for opaque argument positions. Each issue has a regression test, including a census test for the key positions. I found no security hazards: no new dependencies, no new network surface, and no path from user input to anything beyond the per-database map. I did not build the PR or run its tests, so none of this is verified by execution.

Points to consider

1. Null values in the store. ConcurrentHashMap.put throws an NPE on a null value. RedisQueryEngine handles this by branching to remove. The RedisNetworkExecutor MSET-style loop does store.put(keys[i], values[i]) directly, so please confirm values[i] can never be null there. The Javadoc on getGlobalVariableStore mentions the restriction, but the previous setGlobalVariable(k, null) call tolerated null (it removed the key). This is a silent semantic change for any other caller.
2. Raw store bypasses SQL-side invariants. Any caller of getGlobalVariableStore() can now write reserved names such as parent or depth. LocalDatabase.getGlobalVariable hides them from SQL, but getGlobalVariables() (the immutable copy) still returns them. Check that no consumer, for example a Studio or HTTP variables listing or SET echo, treats that map as SQL-visible only.
3. Key-space mixing. Redis keys and SQL LET/SET GLOBAL variables share one map, as before. A Redis client with write access can now write any key SQL could name, as $key. That is not new, but the raw access makes it easier, so a short note in the docs would help. Also confirm the Redis path still enforces the database authorization checks (getAuthorizedDatabase) before the store is returned. From the diff it does.
4. Behaviour change in LocalDatabase.getGlobalVariable. It now returns null for reserved names. This is reasonable. Please add a CHANGELOG or upgrade note, because anything that stored a reserved name through another path will see it vanish from SQL.
5. HA caveat. The Raft comment is updated correctly, and the store is not replicated. That is consistent with the previous behaviour.
6. Style. In RedisNetworkExecutor the field alignment of the neighbouring declarations is now inconsistent after the ConcurrentMap retype. In SQLMethodTransformTest the anonymous DatabaseInternal stub returns null from getGlobalVariableStore. That is fine for a stub. A default method on the interface would avoid touching every implementor, but a forced override is arguably safer.
7. Tests. Issue10790GlobalVariableStoreTest calls store.clear() in finally. That is fine for TestHelper, which creates a fresh database. Consider adding a concurrency test for GETDEL or INCR through the raw store, to guard the atomicity that the old setGlobalVariable(k, null) call was relied on for (The atomic INCR and DECR fix landed on the Redis wire path only, so the redis query engine still does a get-then-set and loses concurrent increments over HTTP #8248). ConcurrentHashMap.remove and compute are atomic, so the code looks right.

Nothing blocking from my side. Please confirm item 1.

🤖 Generated with Claude Code

Fifth review:

• 1 confirmed: in the transient HSET loop, values[i] is json.toString() of a parsed object, never null, and every element is parsed before the first is stored. The only remover that used setGlobalVariable(k, null) (transient HDEL) now calls remove, and the query language's buffered GETDEL null is published with remove.
• 7: the atomicity through the raw store is covered by the existing concurrency tests, which run unchanged and green on this branch: Issue7776IncrAtomicityTest and Issue8248RedisQueryEngineIncrAtomicityTest (concurrent INCR/INCRBY/INCRBYFLOAT/DECR/DECRBY on both surfaces), RedisRespCorrectnessTest.concurrentSetNxOnSharedDatabaseKeyOnlyLetsOneWinnerThrough, and Issue9322 (GETDEL claimed once across retries).
• 2, 3, 4: answered in the earlier replies. The release-notes section of the PR description covers 4, and the docs' Keys section covers 3.

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.}

Review

Author is a maintainer, and I found no security hazard, so I did not add the label. I read the engine, ha-raft and server changes and skimmed the redisw changes. I did not build the project or run any tests.

What looks good

• getGlobalVariableStore() fixes Redis wire: keys named parent, current, depth, path, stack or history are refused as reserved SQL variables on the database-backed key space #10790 at the source. Redis keys no longer go through SQL name rules, and LocalDatabase.getGlobalVariable now returns null for reserved names, so a Redis key named parent or depth can't answer $parent or $depth. The Raft wrapper delegates to the proxied database, and its not-replicated comment is updated to match.
• databasesClosedByStop is set and checked under databasesLock, and start() clears it. That closes the A stopped server reopens its databases on demand, so a security-reload sweep still running at stop can lock 'graph' and fail the next test's startServers with OverlappingFileLockException #10707 race cleanly.
• DatabaseDirectoryNames keeps stat calls off the hot path. It re-checks at most once per second, one caller at a time, and invalidate() bumps a generation counter so a stale listing is never published. If the directory can't be listed, it keeps answering from the last good listing and logs a warning once.
• The tests are regression-style, one per issue, and the binary-key census test is a good guard against a key command being added without a probe.

Suggestions (non-blocking)

1. getGlobalVariableStore() exposes the live mutable map on DatabaseInternal. Its Javadoc already warns about this. Any caller can put a reserved name, or $x, with no validation. I'd consider annotating it as internal and noting that only the wire-protocol wrappers should use it. Also check that nothing under a SQL SELECT $all or getGlobalVariables() surface now shows Redis keys the user didn't expect. That is existing behaviour, but the $-prefixed keys make it slightly more visible.
2. The 2s MODIFIED_TIME_GRANULARITY_MS window means a database copied in by an operator can be missed for up to about 3s. That's acceptable, but the Javadoc could state the worst-case staleness explicitly.
3. In stopInternal, any shutdown-time code that opens a database after the flag is set, such as a final backup, HA snapshot or security sweep, now gets DatabaseNotAvailableException. That is the intent, but please confirm nothing legitimate in that window needs it. I only looked at the code around the flag.
4. The SQLMethodTransformTest stub returns null from the new method. Fine for that test, but other DatabaseInternal implementers in other modules need the new method too. CI will show if any were missed.

No blocking issues from my read. Overall LGTM, subject to green CI.

Merged main after #10791 landed (d40d11c). #10791 fixed #10694 by escaping a leading $ before the SQL accessors. This PR reads and writes the raw store, where the escape would store $$k, so RedisVariableKeys and its unit test are removed and the store-based paths are kept. Issue10694RedisDollarKeyAliasingTest from #10791 is kept and passes, along with the whole redisw suite (233/233).

Review

Author is a maintainer, so I read this for correctness rather than supply-chain risk. I found no security hazard: no new dependencies, no reflection or deserialization, and getGlobalVariableStore() is an internal API reached only from the Redis wrapper. I did not apply the "do not merge" label.

Scope of this review: I read the engine, ha-raft, RedisArguments and the first half of the RedisNetworkExecutor changes in full. I did not read the ArcadeDBServer, ServerDatabase and DatabaseDirectoryNames hunks (#10707 and #10567), or the second half of RedisNetworkExecutor. The #10707 locking and #10567 listing logic need a separate look. I did not build the PR or run any tests.

What looks good

• The raw store accessor fixes Redis wire: keys named parent, current, depth, path, stack or history are refused as reserved SQL variables on the database-backed key space #10790 at the source, and the Javadoc is clear about the ConcurrentHashMap null-key and null-value contract and the no-HA-replication caveat. Merging the six key helpers in RedisNetworkExecutor into one ConcurrentMap path is a good simplification.
• RedisArguments gives one table for opaque positions. opaquePositions does a single lookup per command, and keyPositions returns a clone so callers can't mutate it. The census test that fails when a key command has no probe is a good guard.
• Regression tests exist per issue, and the existing tests were adjusted.

Points to check

1. Reserved-name filter on a hot path. LocalDatabase.getGlobalVariable now does a RESERVED_VARIABLE_NAMES.contains(name) lookup on every read. If that set is a Set.of/HashSet the cost is negligible. Please confirm it is not a List.
2. Behaviour change for existing data. A Redis key stored before this PR as $k was stored in the map as k, and a key k is now distinct from $k. That is fine and intended, but worth a line in the release notes, since a GET $k after upgrade still finds k's value only if the old entry was written as k.
3. Other DatabaseInternal implementers. The new abstract method breaks any out-of-tree implementer. Consider whether a default throwing UnsupportedOperationException is safer. In-tree, the test stub in SQLMethodTransformTest returns null, which is acceptable for a stub.
4. hDel transient mode. The remove(variable) count semantics match GETDEL now. A null variable would NPE on the ConcurrentHashMap. Arguments are guarded earlier, but I couldn't confirm that for HMGET/HDEL with an empty key list.
5. Tests. The tests follow the assertThat(...) style. I did not see any wall-clock assertions in the parts I read. Please check that Issue10707StoppedServerOpensNoDatabaseTest, which I did not read, derives its ports from the fixture and uses no timing bounds, per the CLAUDE.md rules.
6. Formatting nit. In RedisNetworkExecutor, the field alignment of defaultBucket is now off compared to its neighbours.

Overall this is a solid change. I'd like a second look at the server-side hunks before merge.

🤖 Generated with Claude Code

Re-review on the merge commit, all covered:

• 1: RESERVED_VARIABLE_NAMES is a Set.of.
• 2: global variables are in memory only, so nothing stored survives the upgrade. See "Behaviour changes (release notes)" in the description.
• 3: abstract on purpose. A default that throws would compile and fail at runtime; every in-tree implementer is updated.
• 4: no null key can reach the store. A RESP $-1 argument is refused before dispatch, and HDEL/HMGET without keys are refused by the arity table (HDEL <db>.<rid> alone is the record form, which deletes by RID).
• 5: Issue10707StoppedServerOpensNoDatabaseTest runs on BaseGraphServerTest with the fixture's ports. Its only bound is a 30 s get on the late-open future, a hang detector, not a latency assertion.
• The server-side hunks (A stopped server reopens its databases on demand, so a security-reload sweep still running at stop can lock 'graph' and fail the next test's startServers with OverlappingFileLockException #10707, Redis: a <database>.<key> whose database is on disk but not open yet is kept as a plain key of the connection and acknowledged with OK #10567) were covered by the earlier review rounds, and the last one on them ended "Overall LGTM, subject to green CI".