fix(#8855): SQL queries no longer fail while an index is created or dropped on the same type - #8857

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.

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

sequenceDiagram
  participant Query
  participant SelectExecutionPlanner
  participant TypeIndex
  participant IndexMetadata
  Query->>SelectExecutionPlanner: Plan query
  SelectExecutionPlanner->>TypeIndex: Check index readiness
  TypeIndex-->>SelectExecutionPlanner: Readiness state
  SelectExecutionPlanner->>IndexMetadata: Inspect candidate
  IndexMetadata-->>SelectExecutionPlanner: Metadata or IndexException
  SelectExecutionPlanner-->>Query: Plan using available indexes

Merge Risk: 🟡 Moderate · up to 62654

Queries during index creation can omit matching records because an index becomes eligible before population completes. Gate readiness on completed population before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 62654

The change improves handling of concurrent index changes without demonstrating a new access path or weaker permission checks. Complete-result and recovery guarantees remain partly unresolved, so the risk is low rather than minimal.

Retained concerns
No architecture-level concerns identified.

• Fix all pre-merge checks with AI

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

Autopilot is currently an internal CodeRabbit preview.

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

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

The approach is sound: snapshot plus validity filter in the planner, ConcurrentHashMap for indexesByProperties, and a regression test that reproduces the issue. A few points, none blocking except possibly the first.

Issues

1. Javadoc is now attached to the wrong methods (SelectExecutionPlanner.java ~4600-4612 and ~4808-4820). The new plannableIndexes Javadoc was inserted after the existing findBestIndexFor Javadoc (@param context ... @return), so that block is now orphaned above plannableIndexes. Likewise the existing Javadoc of buildIndexSearchDescriptorForFulltext now sits on buildIndexSearchDescriptorForFulltextSafely. Please move the new methods above the old Javadoc blocks, or move the old blocks back onto their methods.

2. TypeIndex.indexesOnBuckets is still a plain ArrayList. Reordering valid = false before clear() and volatile valid help, but a reader can still race with clear():

   • getFirstUnderlyingIndex() does getFirst(), which checks size and then reads elementData[0]. If clear() lands between the two, it returns null instead of throwing NoSuchElementException, so a NPE can still surface in getType()/getNullStrategy().
   • Catching NoSuchElementException instead of calling isEmpty() first does not close that window. The other direct isEmpty() followed by getFirst() pairs (lines 122, 177, 366, 646) have the same shape.
   • The planner-side try/catch IndexException shrinks the window but cannot cover that NPE. A CopyOnWriteArrayList (writes are rare: DDL only) or reading the first element once into a local would remove it. getType() falling back to null when the list is empty already looks like the intended contract given the indexType == null check in the planner.
   • The stress test passing 120 rounds does not prove this is closed, since the window is only a few instructions wide.

3. Empty catch blocks that swallow IndexException. They are commented, which is fine, but plannableIndexes and buildIndexSearchDescriptor now silently drop any IndexException, including unrelated ones (e.g. a corrupted index). Consider catching only the "not valid" case or logging at FINE level.

Minor / style

• TypeIndex.java:53: the alignment of the valid field declaration is off after adding volatile (the other fields are column-aligned).
• plannableIndexes allocates a new list on every plan. That is cheap compared to planning, but you could skip the copy in the common case where all indexes are valid.
• buildIndexSearchDescriptor was split into a wrapper plus ...Internal just for the try/catch. Acceptable, though a single try/catch inside the existing method would be a smaller diff.
• The new Javadoc references the issue number; per CLAUDE.md prefer comments that explain the constraint rather than the issue (the explanation is already there, so the (issue #8855) suffixes could go).

Tests

• SelectPlanningDuringIndexDdlTest is a good regression test and follows the AssertJ style. Consider adding @Timeout as a hang detector for the join() loop, since a deadlock would stall CI.
• It is probabilistic by nature. It would help to also add a deterministic unit test, e.g. a TypeIndex with an empty sub-index list: getType() and getNullStrategy() should throw IndexException (or return null), never NPE, and plannableIndexes should skip it.
• Only the < range query on a UNIQUE index is exercised. A full-text index create/drop cycle would cover the new buildIndexSearchDescriptorForFulltextSafely path, which has no test now.

I did not run the tests locally; the PR description reports 3030 related tests passing.

🤖 Generated with Claude Code

Review

The approach is sound: a ConcurrentHashMap for indexesByProperties, a copy-on-write sub-index list, a volatile valid, and a planner that snapshots plannable indexes and tolerates IndexException. The firstOrNull() helper is also a good fix for the isEmpty() then getFirst() check-then-act race. Findings below, ordered by importance.

1. Other planner paths still read the unfiltered index list (incomplete fix)

Only findBestIndexFor goes through plannableIndexes(). Other places in SelectExecutionPlanner walk type.getAllIndexes(true) and call methods that throw IndexException on a TypeIndex that is still being populated or was just invalidated:

• findIndexForProperty (~L872) calls index.getPropertyNames() on every index (used by the min/max-style optimizations).
• The ORDER BY index path (~L3885-3921) calls idx.getPropertyNames() and idx.getNullStrategy(). getPropertyNames() goes through getFirstUnderlyingIndex(), which throws when the index is empty.

A query such as SELECT FROM Person ORDER BY id or SELECT min(id) FROM Person should hit the same "unrelated query fails during index DDL" scenario from issue 8855. The regression test only runs WHERE id < ?, so it does not cover these. Suggest applying plannableIndexes() (hoisted into a shared helper) wherever the planner iterates getAllIndexes, and adding an ORDER BY and a min()/max() query to the race test reader loop.

2. Unguarded getType() after the snapshot

In findBestIndexFor, the prefer-full-text step does d.index.getType().equals(FULL_TEXT) (~L4671). That is outside the new try/catch, so a drop between plannableIndexes() and this line gives an NPE (getType() returns null once the list is emptied) or an IndexException (once valid is false). It is a narrow window (only when descriptors.size() > 1) but it is the same class of bug being fixed. Use FULL_TEXT == d.index.getType() (null-safe) and handle IndexException, or capture the type in the descriptor when it is built.

3. The drop() comment is not quite accurate

The comment says "a concurrent reader that sees the list empty must also see the index invalid". But the loop just above (type.getSchema().dropIndex(...) per sub-index) already removes sub-indexes from the list via removeIndexOnBucket, so a reader can still see an empty list while valid is true. The reorder is harmless, but the stated invariant does not hold; the getType() != null check in plannableIndexes() is what actually covers that window. Please soften the comment or confirm the reorder is needed.

4. Minor

• plannableIndexes() allocates a new list on every findBestIndexFor call, i.e. once per OR branch. Snapshot once in the caller (~L4160) and pass the list down.
• isValid() followed by getType() are two reads that can still disagree; the catch covers it, but a short comment would help.
• firstOrNull() relying on NoSuchElementException is correct (CopyOnWriteArrayList.getFirst() reads the array once) and only costs on the empty path.
• A plan can still reference a TypeIndex dropped before execution. That is inherent and out of scope here, but execution-time IndexException remains possible.

Tests

• Good: a real concurrent regression test covering NOTUNIQUE and FULL_TEXT, failures grouped by exception kind, and @Timeout sized as a hang detector.
• It is probabilistic, so the query shapes from item 1 would improve its power.
• The FULL_TEXT variant never issues a CONTAINSTEXT predicate, so buildIndexSearchDescriptorForFulltext is only reached through the getType() filter. A CONTAINSTEXT reader would exercise the new ...Safely wrapper.
• typeIndexWithoutSubIndexesIsNeverPlannedOn checks the exceptions the TypeIndex throws, not that the planner skips such an index. A deterministic test that registers an empty TypeIndex on a type and runs a SELECT would pin that behaviour without relying on timing.
• Consider whether the 120-round, 6-reader test is fast enough for the default lane, or needs @Tag("slow") per CLAUDE.md.

Conventions

Style follows CLAUDE.md (final params, no new deps, single-statement ifs without braces).

Overall: a good targeted fix, but item 1 means the symptoms from issue 8855 can still reproduce with ORDER BY or aggregate queries. I would address that before merging.

Checked head 33d0e382f7 (and 60c84c0ff9 before it) against the base 3625409603 with DdlRace from #8855 (8 readers beside 200 create/drop cycles of an unrelated index), embedded, laptop, Temurin 25 with compact object headers and Temurin 21:

60c84c0ff9 was also at zero on every run. The wider stress run (readers doing lookups, SQL, openCypher, and a parallel aggregate beside index DDL and writes, then a close) ends with the right count and no exception on the head, where the base still threw #8855's NPEs.

One thing I noticed, in case it is not expected: during the DDL the head's readers answer 64 queries per second, where the base answered about 119 per second successfully plus about 55 that failed fast. Without DDL the two are the same (1,501 to 1,548 queries per second here against 1,555 to 1,616 on the base, 10 s each). So the queries now seem to wait out each index build instead of failing, which looks like the right trade.

Review of #8857

Well-scoped fix for #8855 with a real regression test that reproduces the race. The approach (concurrent map, copy-on-write sub-index list, snapshot of plannable indexes, IndexException treated as 'not a candidate') fits the existing weakly-consistent-read conventions in LocalDocumentType. A few points worth addressing.

Issues

1. Misplaced Javadoc in SelectExecutionPlanner: the 'Snapshot of the indexes of a type that can be planned on...' comment is now attached to isFullText(Index), while plannableIndexes(...) right below has no doc. Move the comment onto plannableIndexes.

2. TypeIndex.drop() still has a window where the index looks valid but its sub-indexes are already dropped. The loop calling type.getSchema().dropIndex(...) runs first and valid = false is set only after it. A reader passing isValid() in plannableIndexes() during that loop sees a valid TypeIndex whose bucket indexes are being dropped. The comment acknowledges it is best effort, and the IndexException catches cover most of it, but this relies on reading a dropped sub-index's metadata (getType(), getPropertyNames()) throwing only IndexException and not some file/page-manager error. If dropIndex on a sub-index does not require the parent to still be valid, setting valid = false before the loop would close the window almost entirely. Worth checking.

3. Other consumers of getAllIndexes(true) are not hardened, e.g. SaveElementStep (around line 139, getAllIndexes(true).stream().filter(TypeIndex::isUnique)). An INSERT racing a CREATE INDEX can hit the same half-created index. Out of scope for a SELECT-focused fix, but worth a follow-up issue.

Smaller points

• Performance: plannableIndexes() allocates a new list and calls isValid()/getType() for every index on every call, from several planner paths per statement. Not blocking, but for types with many indexes the snapshot could be computed once per statement and passed down.
• CopyOnWriteArrayList is a good fit (rare writes, hot reads), and the callers I checked (for loops, new ArrayList<>(...), get(i), toArray) are compatible. Several TypeIndex accessors (around lines 518/654/663/710) return the live list, so callers can no longer use Iterator.remove() on it. I found none, but it is a behavior change.
• LocalDocumentType: only indexesByProperties becomes concurrent; bucketIndexesByBucket and custom are still plain HashMaps, so the schema-level race is only partially closed.
• firstOrNull() using try/catch is the right choice (an isEmpty() check would not be atomic); no change needed.

Tests

• SelectPlanningDuringIndexDdlTest is a good regression test (6 readers vs 120 create/drop cycles, both NOTUNIQUE and FULL_TEXT), and typeIndexWithoutSubIndexesIsNeverPlannedOn pins the invariant the planner relies on.
• The race test is inherently probabilistic, but the PR notes it failed with all four exception kinds before the fix, which is the right bar.
• Two tests x 120 DDL cycles with 6 busy reader threads could be slow on small CI runners. Per CLAUDE.md consider @tag("slow") if it takes more than a few seconds. The 5-minute @timeout is fine as a hang detector.
• Cleanup is handled correctly (finally stops and joins readers).

Verdict

No blocking problems found. Please fix the misplaced Javadoc and consider the drop() ordering (point 2). I did not run the test suite in this review; the '3030 related tests pass' claim is taken from the PR description.

Review

Overall this is a sound, well-scoped fix. The root cause (planner reading a live HashMap and the metadata of indexes that are not populated yet or already invalidated) is addressed at both ends: safe containers on the DDL side, and a snapshot plus IndexException tolerance on the planner side. I found no blocking issues.

What looks good

• indexesByProperties as ConcurrentHashMap: the existing uses I checked (values().iterator().remove() in removeTypeIndexInternal, get/put/remove in addIndexInternal) are all supported, and getAllIndexes(true) returning a live view is now safe to iterate.
• TypeIndex: CopyOnWriteArrayList plus the single-read firstOrNull() removes the isEmpty() then getFirst() TOCTOU in get(), keyedCursor(), getType(), isResultApproximate() and getIndexesByKeys(). valid being volatile is correct. The write rate on indexesOnBuckets is DDL only, so COW cost is irrelevant.
• drop() now sets valid = false before clear(), which narrows the window sensibly. The comment is honest that it is best effort.
• The planner reads type, property names and null strategy once up front inside the try/catch (handleClassWithIndexForSortOnly) instead of re-reading across the loop body. Good.
• The regression test covers both the plain and FULL_TEXT paths, uses the preferred assertThat(...) style, and @Timeout is used as a hang detector rather than a latency bound, which fits CLAUDE.md.

Suggestions (non-blocking)

1. Remaining TOCTOU in TypeIndex.getMetadata() / getMetadataForNewFile() (!indexesOnBuckets.isEmpty() then getFirstUnderlyingIndex()). It now surfaces as an IndexException rather than an NPE/CME, so it is not harmful, but using firstOrNull() there would be consistent with the rest of the change and would return null, which these methods already do for the empty case.
2. bucketIndexesByBucket is still a plain HashMap mutated by DDL (computeIfAbsent/add in addIndexInternal, removeBucketIndexInternal). If any query-time path reads it, it has the same class of race. If it is only read under the schema lock this is fine; worth a quick check or a follow-up issue.
3. Planner catch-and-skip pattern is repeated (findIndexForProperty, handleClassWithIndexForSortOnly, isFullText, buildIndexSearchDescriptorForFulltextSafely, the buildIndexSearchDescriptor wrapper/Internal split). It is readable, and the plannableIndexes Javadoc explains the rationale well; just be aware that a new planner path reading index metadata must follow the same convention (a short note near plannableIndexes saying so would help).
4. Performance: plannableIndexes() allocates a list per planning call and calls isValid()/getType() per index. Negligible next to planning itself; just the first place to look if uncached planning on types with many indexes ever shows up in profiles.
5. Test:
   • The race is probabilistic, so green is evidence, not proof. That is acceptable for this kind of bug.
   • 2 tests x 120 create/drop cycles with 6 spinning readers: if this takes more than a few seconds on CI, consider @Tag("slow") per CLAUDE.md.
   • typeIndexWithoutSubIndexesIsNeverPlannedOn pins the contract the planner relies on (null type, IndexException on property names / null strategy), which is useful.

Security

No concerns: no new inputs, no new dependencies.

Nice fix, and thanks for the clear PR description.

Review of #8857

Solid, well-scoped fix for #8855. The root cause is correctly identified (planner walking a plain HashMap and reading metadata of half-built or half-dropped TypeIndex instances), and the layered approach (concurrent map, snapshot of plannable indexes, firstOrNull() single-read helper, volatile valid) is coherent. I checked the other users of indexesByProperties and indexesOnBuckets (iterator remove() on values() in LocalDocumentType, toArray, direct returns of the list): all are compatible with ConcurrentHashMap / CopyOnWriteArrayList, so I see no correctness regression from the type swaps.

Points worth considering

1. drop() still leaves the long window open (TypeIndex.java ~L394-401). valid = false is set only after the loop that calls schema.dropIndex(...) on every sub-index, which is the slow part. A reader arriving during that loop sees isValid() == true while sub-indexes are being closed and deleted. The reorder only narrows the window between clear() and valid = false. If Schema.dropIndex on a sub-index does not require the parent to still be valid, setting valid = false before the loop would close most of the race. If it does require it, a short comment saying so would help, since "a good chance" reads as an unexplained half-measure.

2. Catching IndexException broadly can hide real problems. buildIndexSearchDescriptor, findIndexForProperty, handleClassWithIndexForSortOnly and buildIndexSearchDescriptorForFulltextSafely now swallow every IndexException and silently fall back to a worse plan (full scan). IndexException is also thrown for genuine corruption or misconfiguration, which would now degrade performance with no trace. Consider a FINE/DEBUG LogManager message in those catch blocks, or only swallowing when index.isValid() is false.

3. Repeated try/catch pattern. The "read metadata, treat IndexException as not a candidate" logic now appears five or six times, and the Javadoc on plannableIndexes has to warn that every future planner path must remember it. An immutable per-plan snapshot (type, property names, null strategy, supportsOrderedIterations, holdsFoldedKeys captured once inside plannableIndexes) would make the contract structural instead of a convention, and avoid calling getType() / checkIsValid() repeatedly per index. Not a blocker, fine as a follow-up.

4. firstOrNull() uses exception-based control flow. getFirst() on an empty CopyOnWriteArrayList throws NoSuchElementException. It is correct and cheap in the common case, but final Iterator<IndexInternal> it = indexesOnBuckets.iterator(); return it.hasNext() ? it.next() : null; is equally race-free (the COW iterator is a snapshot) and has no exception on the empty path. Minor.

5. Index-based loops such as TypeIndex.equals (for i < size(); get(i), ~L489-497) can now observe a size change mid-loop and throw IndexOutOfBoundsException under concurrent DDL. Not a regression (equally unsafe before), but iterating a snapshot would be consistent with the intent of this PR.

Tests

• SelectPlanningDuringIndexDdlTest is a good race regression and follows the repo style (assertThat, @Timeout as a hang detector, no wall-clock assertions).
• It runs 2 x 120 create/drop cycles with 6 busy-looping readers and a 5 minute timeout. If it takes more than a few seconds, please add @Tag("slow") per CLAUDE.md, and confirm the runtime on a small CI runner, since 6 spinning readers plus DDL can starve it.
• A race test is probabilistic: it fails reliably before the fix, but a pass is only evidence. typeIndexWithoutSubIndexesIsNeverPlannedOn is a nice deterministic complement. Consider also a deterministic test that an invalidated TypeIndex registered on the type is skipped by plannableIndexes (the plan still succeeds), independent of thread timing.
• Only the SQL SELECT planner is exercised; other callers such as SaveElementStep (getAllIndexes(true)...filter(TypeIndex::isUnique)) are not covered. They likely do not read type or null strategy, so fine, just noting scope.

Performance / security

• CopyOnWriteArrayList writes copy the array, but sub-index counts equal bucket counts (small) and writes only happen at DDL. Reads stay lock-free.
• plannableIndexes allocates one list per planning call; acceptable (see point 3).
• No security concerns.

Overall: good fix, mergeable once the drop() ordering question (1) is answered and the test runtime is checked.

Review

The approach is sound: a snapshot of plannable indexes, a copy-on-write sub-index list, and tolerating IndexException for an index that disappears mid-plan. It follows the codebase lock-free read pattern (as with properties and the bucket lists), and I found no blocking issue. I read the diff and surrounding code but did not run the tests here. Notes, roughly by importance:

1. TypeIndex.drop() ordering only narrows the window
The new comment says the flag is set "before the sub-indexes are cleared". But the type.getSchema().dropIndex(...) loop just above already drops every sub-index, and that is the main window where a reader sees a valid wrapper with dying children. The reorder of valid = false / clear() is a small improvement, but the real protection is the planner IndexException handling, so I would reword the comment to say that. As written it overstates what the reorder does.

2. Blanket catch (IndexException) can hide real failures
buildIndexSearchDescriptor, findIndexForProperty and handleClassWithIndexForSortOnly now swallow every IndexException and silently plan without the index. That is right for the drop/rebuild race. But a genuinely broken index (corrupt metadata, for example) now degrades to a full scan with no signal instead of a visible error. Consider a debug-level log in the catch blocks, or checking that the index is no longer valid before swallowing. findBestIndexFor already does this properly: it only swallows when stillValid.size() != descriptors.size(), and otherwise rethrows. The other sites could follow the same rule.

3. Redundant handling in plannableIndexes
isPlannable already catches IndexException, so the try/catch around it in plannableIndexes is dead code. Likewise index.getType() != null already goes through checkIsValid() for a TypeIndex, so the isValid() check only matters for other IndexInternal implementations. Harmless, but easy to trim.

4. Same race class left elsewhere
bucketIndexesByBucket is still a plain HashMap (LocalDocumentType line ~92) and is mutated by the same DDL paths. If any query or insert path reads it without the schema lock, it has the same CME risk. Other readers of TypeIndex.getType()/getPropertyNames() outside the SQL planner (OpenCypher, Studio and HTTP schema endpoints, other getAllIndexes consumers) may still hit the NPE/IndexException from the issue. Out of scope here, but worth a follow-up issue.

5. firstOrNull()
Using the exception is correct and race-free (isEmpty() + getFirst() would be racy), and it only costs anything in the rare empty case. A one-line comment saying why would stop a future "simplification".

6. Tests

• The regression race test is good: deterministic setup, threads joined in finally, and a failure histogram so a regression names the exception kind. It is probabilistic by nature, but the PR reports it failing on all four exception kinds before the fix, which is the right evidence.
• typeIndexWithoutSubIndexesIsNeverPlannedOn is misnamed: it only checks TypeIndex behaviour with no children (null type, IndexException on the getters), not that the planner skips it. Rename it (e.g. typeIndexWithoutSubIndexesReportsNoTypeAndThrowsOnMetadata) or add a planner-level assertion.
• Please check the wall time of the two race tests (120 create/drop rounds against 6 readers). If they take more than a few seconds in CI, add @Tag("slow") per CLAUDE.md. The 5-minute @Timeout is fine as a hang detector.
• Minor style: indexKind.equals("FULL_TEXT") is evaluated twice per round; a final boolean fullText would be cleaner.

7. Performance and security
plannableIndexes allocates one small list per plan and makes a few volatile reads per index, which is negligible next to planning. CopyOnWriteArrayList copies only on DDL, which is rare, and reads stay lock-free. No security concerns.

Overall: good fix, with small cleanups suggested (items 1 to 3 and 6).

🤖 Generated with Claude Code

Review

Overall this is a solid, well-scoped fix. The approach (concurrent map, copy-on-write sub-index list, a planning snapshot of valid and populated indexes, plus a tolerant catch of IndexException at the read sites) matches the root cause in #8855. The regression test reproduces the race with a real create/drop loop. I found nothing blocking, only the points below.

Correctness and design

• TypeIndex.firstOrNull() is the right way to avoid the isEmpty() then getFirst() TOCTOU against clear(). The remaining first-element reads all go through it. getFirstUnderlyingIndex() still throws IndexException on empty, which is what the planner catches.
• CopyOnWriteArrayList is fine here (writes only happen on DDL). I checked the remaining usages in TypeIndex (iteration, get(i), toArray, add/remove) and none rely on Iterator.remove() or sort(), which COW lists do not support. Accessors that return the live list now hand out a COW list, so callers can no longer hit a CME.
• indexesByProperties as ConcurrentHashMap: the existing it.remove() in removeTypeIndexInternal is supported, and I see no null keys or values. getAllIndexes(false) is now a weakly consistent view, which is the intent.
• drop() ordering (valid = false before clear()) closes the window where a reader sees a valid wrapper with no sub-indexes. A narrow window still exists between the first sub-index drop and valid = false, but it is covered by the IndexException / null-type handling, and the Javadoc on isPlannable documents the contract.
• findBestIndexFor retry loop: it terminates because the list either shrinks or the exception is rethrown. OK.

Suggestions (non-blocking)

1. Same race outside the SELECT planner. SaveElementStep:139 does docType.getAllIndexes(true).stream().filter(TypeIndex::isUnique) and caches the result. isUnique() goes through the first sub-index, so an INSERT/UPDATE/UPSERT running concurrently with CREATE INDEX / DROP INDEX on the same type can probably still hit an IndexException for an index that is not populated yet. FetchFromSchemaTypesStep:183 (sorts by name, reads metadata) is in a similar situation. Worth a follow-up issue, or a shared helper next to the planner one.
2. Redundant try/catch: plannableIndexes() wraps isPlannable() in a try/catch, but isPlannable() already catches IndexException and returns false, so the inner catch is dead code.
3. Duplicated catch-and-log blocks: the "Index '%s' skipped while planning" log is repeated in four places. A tiny helper would reduce noise. Not required.
4. Allocation per plan: plannableIndexes() copies the index collection on every SELECT plan (previously an unmodifiable view). Small cost and acceptable for correctness, but it is on the planning hot path.

Tests

• The race test is good and covers both the plain and FULL_TEXT paths, plus the composite-index tie case. Two things to consider:
  • If the runtime turns out to be noticeable in CI (120 DDL rounds x 2 tests, 2,000 vertices), tag the race tests @Tag("slow") per CLAUDE.md. The @Timeout is fine as a hang detector.
  • Readers only count exceptions; they never check results. Ids 0..1999 are static, so count(*) WHERE id < ? has a known answer. Asserting it would also catch a plan that silently picks a half-built index and returns wrong data, not just throws.
• typeIndexWithoutSubIndexesIsNeverPlannedOn only asserts the null/throw contract of an empty wrapper, so a name like typeIndexWithoutSubIndexesHasNoTypeAndThrows would be more accurate.

No security concerns. Thanks for the thorough PR description and the before/after failure counts.

Review of #8857

Overall this is a solid, well-reasoned fix. Snapshotting plannable indexes, treating IndexException and a null type as "not a candidate", and the copy-on-write sub-index list address the race in #8855 directly. The regression test is realistic: concurrent readers against create/drop cycles for both NOTUNIQUE and FULL_TEXT, and it also checks result correctness. I found one gap and a few small points.

Potential bug: same unguarded pattern remains in WhereClause.estimate

engine/src/main/java/com/arcadedb/query/sql/parser/WhereClause.java:110-135 is on the planning path (it estimates the cost of the WHERE clause):

final Collection<TypeIndex> indexes = oClass.getAllIndexes(true);
...
for (final Index index : indexes) {
  if (index.getType().equals(Schema.INDEX_TYPE.FULL_TEXT))   // NPE when getType() == null
    continue;
  final List<String> indexedFields = index.getPropertyNames(); // IndexException when no sub-indexes

It reads the live collection, not a snapshot. An index being created (wrapper registered, no sub-indexes yet, so getType() is null) gives an NPE here, and a dropped-but-still-registered index gives an IndexException. This is the same failure mode the PR fixes in SelectExecutionPlanner. It is only reached when the estimated count is above the threshold, which may be why the test does not hit it. Please route it through the same filtering (e.g. move plannableIndexes to a shared helper), and add a test with a type large enough to pass that threshold.

Other getAllIndexes(true) readers in the query layer (SelectExecutor, OpenCypher StatisticsProvider, MergeStep, MatchNodeStep, SaveElementStep) have similar exposure. Probably out of scope here, but worth a follow-up issue.

Nits

1. Misplaced Javadoc in SelectExecutionPlanner: the "Snapshot of the indexes of a type that can be planned on..." comment sits on logSkippedIndex, not on plannableIndexes/isPlannable. Move it.
2. TypeIndex.drop() comment is hard to follow. Say plainly: sub-indexes dropped, then valid = false, then clear(). Note that between the first sub-index drop and valid = false readers still see a valid wrapper with some sub-indexes missing, which is why the try/catch guards are needed.
3. while (true) retry loop in findBestIndexFor: it terminates (each pass returns, rethrows, or strictly shrinks the list), but a one-line comment saying so would help the next reader.
4. ConcurrentHashMap iteration order differs from HashMap, so tie-breaks between equally ranked indexes could pick a different index than before. Likely fine since the 3030 related tests pass, and I found no null key/value put that ConcurrentHashMap would reject.
5. Test: catch Throwable in the reader loop so an Error in a reader thread is not silently lost. Consider @Tag("slow") if 2 x 120 rounds are noticeably long in CI (per CLAUDE.md). A race test cannot prove absence of the bug, but failing on all four exception kinds before the fix is good evidence it exercises it.

Performance

CopyOnWriteArrayList makes mutation O(n buckets), only on DDL, so negligible; reads stay lock-free and firstOrNull() removes the isEmpty() + getFirst() race cheaply. plannableIndexes adds one small ArrayList per planning call, acceptable.

Security

No concerns.

Verdict

Good to merge once the WhereClause.estimate path is guarded (or explicitly tracked as a follow-up) and the misplaced Javadoc is moved.

Review

The approach is sound: make the shared structures safe to read concurrently (ConcurrentHashMap for indexesByProperties, CopyOnWriteArrayList plus volatile valid in TypeIndex), then have the planner work on a filtered snapshot and treat an IndexException as "not a candidate". The firstOrNull() helper removes the isEmpty() then getFirst() check-then-act window. Reordering drop() so that valid = false is set before clear() is correct, and the comment explains why the sub-indexes must be dropped first. I read through the changed files and checked the other users of indexesByProperties and indexesOnBuckets. I did not run the tests.

Correctness / robustness

• Other getAllIndexes(true) consumers are still unguarded. The fix covers SelectExecutionPlanner and WhereClause.estimate. The same race (no sub-indexes yet, or already invalidated) can still reach these:

  • SaveElementStep:139 (filter(TypeIndex::isUnique)) is on the write path, so an INSERT/UPDATE racing a CREATE INDEX or DROP INDEX could hit it. isUnique() goes through getFirstUnderlyingIndex(), which throws on an empty wrapper.
  • SelectExecutor:422, SQLFunctionGeoPredicate:126,165, SQLFunctionSearchFields(More), StatisticsProvider:121, and the OpenCypher MatchNodeStep, MergeStep and CypherExecutionPlan.

  This is fine as a follow-up, but the PR title and the "unrelated queries failed" framing suggest more coverage than there is. Consider either a follow-up issue or moving the "ready" filter into one shared helper, so these paths do not each need their own try/catch. For example, a DocumentType method that returns only ready indexes would avoid every caller repeating the isReadyForQueries() and catch IndexException pattern.

• Broad catch (IndexException). buildIndexSearchDescriptor and findIndexForProperty now swallow any IndexException and log it at FINE. That covers the intended "dropped mid-plan" case, but it also hides real failures such as a corrupt or unreadable index. Where practical, only skip when !isPlannable(index) after the catch, as the findBestIndexFor retry loop already does, and rethrow otherwise.

• findBestIndexFor retry loop. The termination argument in the comment holds. pickBestDescriptor takes descriptors as a non-final parameter and reassigns it, which is easy to misread. A small local copy would be clearer.

• isPlannable for non-TypeIndex. The Index branch is only reached via d.index in the retry loop, which is fine but untested.

• isReadyForQueries() is best effort. The Javadoc says so, which is good. It reads first.getType(), and the type can still be invalidated after that call. The remaining window is covered by the catches, so a note in the planner on which call sites rely on that is enough.

Performance

• CopyOnWriteArrayList is the right choice here, because the list changes only on DDL and is read on every query. plannableIndexes() allocates one small ArrayList per plan step. That is negligible, but handleClassWithIndexForSortOnly and findIndexForProperty could filter inline instead of copying.
• getAllIndexes(true) for a non-polymorphic type returns a live view over the ConcurrentHashMap values. That is weakly consistent and fine, but it is not a snapshot, so callers that iterate twice can see different content.

Tests

• SelectPlanningDuringIndexDdlTest is a good stress regression. It covers the NOTUNIQUE and FULL_TEXT variants plus MATCH and ORDER BY, and it asserts exact counts, so a wrong plan on a half-built index is detected and not just an exception. Setting stop and joining in finally is correct.
• Race tests are probabilistic. Please make sure it is stable on slow CI runners. At 120 create/drop cycles over 2,000 rows with a 5-minute @Timeout, it may need @Tag("slow") if it takes more than a few seconds. @Timeout is a hang detector here, which is the correct use.
• There is no deterministic test for the new pieces. A cheap one would be a TypeIndex unit test for isReadyForQueries() (no sub-indexes, valid with sub-indexes, after drop()), and one for the new drop() ordering. The existing typeIndexWithoutSubIndexesHasNoTypeAndThrows covers only the empty case. A deterministic planner test would also help: a TypeIndex with no sub-indexes registered on the type must be skipped without failing the query.
• Nit: runRace(final String indexKind) compares strings with equals("FULL_TEXT") three times. A boolean fullText parameter would be simpler.

Style (CLAUDE.md)

• Mostly consistent. Imports are used instead of fully qualified names, and empty catch blocks carry a comment.
• WhereClause.estimate swallows IndexException with no log, while the planner logs at FINE. Consider making them consistent.

Overall this is a solid fix for the reported failure modes. I would like to see the unguarded write-path consumer (SaveElementStep) addressed or tracked, and a deterministic unit test for isReadyForQueries().

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 of #8857

Overall this is a solid, well-scoped fix. The root cause (a plain HashMap plus half-built or half-dropped TypeIndex wrappers read by the planner) is correctly identified, and the new test fails before the fix and passes after, according to the PR description. I read the diff and the surrounding LocalDocumentType and TypeIndex code. I did not run the tests. Findings below, most important first.

Correctness / design

1. TypeIndex.drop() ordering. The new order is drop sub-indexes, then valid = false, then clear(). The comment explains it well. One residual window: between the first sub-index drop and valid = false, isReadyForQueries() can return true, since it checks only the first sub-index, which may be the one already dropped. The downstream IndexException catches handle this, as the PR says. It does mean correctness depends on every planner read being wrapped in try/catch, which is fragile. The Javadoc on plannableIndexes documents this, which is good. A cleaner long-term option would be a single immutable per-index snapshot (type, properties, null strategy) taken once, so later reads cannot throw.

2. WhereClause.estimate calls index.isReadyForQueries() outside the try block. The Javadoc says "never throws", yet isPlannable in the planner wraps the same call in a try/catch. Either trust the contract in both places or defend in both. Moving the call inside the existing try is a one-line change.

3. Other readers of the same state are still exposed.

   • SaveElementStep (around line 139) does getAllIndexes(true).stream().filter(TypeIndex::isUnique) and caches the result per type name. isUnique() goes through getFirstUnderlyingIndex(), which throws IndexException for an index that is mid-create or mid-drop. This affects DML with skipDuplicateKey, not SELECT, so it is likely out of scope for SQL queries fail (NullPointerException, IndexException, ConcurrentModificationException) while another thread creates or drops an index on the same type: 21-31% of an unrelated indexed query during 200 create/drop cycles (26.8.1 to main) #8855, but worth a follow-up issue.
   • LocalDocumentType.bucketIndexesByBucket is still a HashMap<Integer, List<IndexInternal>> with ArrayList values, mutated in addIndexInternal and removeBucketIndexInternal and read on the write path by getPolymorphicBucketIndexByBucketId. The same class of race (CME or torn read) can occur there. Consider covering it or filing a follow-up.

4. getAllIndexes(false) now returns an unmodifiable view over a live ConcurrentHashMap. That is fine (weakly consistent) and the planner now snapshots it, but note it is not a snapshot for other callers that iterate it twice (e.g. TruncateTypeStatement, FetchFromSchemaTypesStep).

Performance (CLAUDE.md: performance and GC pressure first)

5. firstOrNull() allocates a COWIterator on every call, and it is used on hot paths (get(keys), get(keys, limit), keyedCursor, getType(), isResultApproximate()). Escape analysis may remove it, but that is not guaranteed. On Java 21, CopyOnWriteArrayList.getFirst() reads the backing array once, so it is race-free and allocation-free:

   private IndexInternal firstOrNull() {
     try {
       return indexesOnBuckets.getFirst();
     } catch (final NoSuchElementException e) {
       return null;
     }
   }

   The exception is only thrown in the rare empty case. The original !isEmpty() then getFirst() was racy only because of the two separate reads.

6. plannableIndexes() allocates a new ArrayList per call, up to about four times per plan, plus a getType() per index. This is negligible next to planning cost; noting it only. COW write cost (O(n) per addIndexOnBucket) is fine for typical bucket counts.

Style / conventions

7. Several new comments are multi-line blocks that explain the story of the fix. CLAUDE.md asks for no comments unless the WHY is non-obvious. The drop() ordering comment earns its place. The "Dropped or rebuilt while reading it: not a candidate" comment is repeated four or five times and could be trimmed.
8. logSkippedIndex in SelectExecutionPlanner and the inline LogManager call in WhereClause duplicate the same message pattern. Minor.

Tests

• Good coverage: deterministic tests for isReadyForQueries() and for an empty TypeIndex (null type, throws on metadata), plus the stress test for both the regular and the full-text path. Asserting known counts for id < ? and id = ? also guards against silently wrong plans, not only exceptions.
• The race test is probabilistic by nature (6 busy-loop readers against 120 DDL cycles). That is acceptable for a regression test, but if its runtime is more than a few seconds in CI, consider @Tag("slow") per CLAUDE.md.
• The PR description mentions rebuilding, but the test does not exercise REBUILD INDEX concurrent with queries. Adding a rebuild step to the loop would cover the valid flag flip on that path.

Verdict

No blocking issues. I would apply #5 (allocation-free firstOrNull) and #2 (consistent try block) before merge, and open follow-ups for #3 (SaveElementStep, bucketIndexesByBucket).

Review

The fix addresses the root cause of issue 8855 in the right places: a concurrent indexesByProperties, a copy-on-write sub-index list, valid made volatile, and a planner that works on a snapshot of ready indexes and tolerates IndexException for indexes dropped mid-plan. The drop() reordering (valid=false before clear()) is well reasoned and the comment explains why the wrapper must stay valid during the sub-index drops. The regression test is a real race reproducer and also asserts correct counts, not just "no exception". Nothing here looks like a blocker.

Suggestions

1. TypeIndex.firstOrNull() uses exceptions for control flow. The comment says "no allocation", but on an empty list it throws and catches a NoSuchElementException (with a stack trace). The empty state is now a normal state for any index being created or dropped, and getType()/isReadyForQueries() hit it on every planning pass. Probably cheap enough in practice, but a single snapshot read that does not throw would avoid it while keeping atomicity. At minimum, fix the comment so it does not claim zero allocation.

2. Other getAllIndexes(true) consumers are still unguarded. The PR hardens SelectExecutionPlanner and WhereClause.estimate, but the same race can hit SelectExecutor (query/select, ~line 422), SaveElementStep (cached unique indexes), and the OpenCypher paths (MatchNodeStep, MergeStep, CypherExecutionPlan, StatisticsProvider), which read getType()/getPropertyNames() on indexes that may be half-built. Consider a follow-up, or note it in the PR description so the issue is not read as closing the whole class of problem. Moving the "ready" filter into a shared helper (rather than the private plannableIndexes in the planner) would make that follow-up trivial and fits the "reuse existing components" guideline.

3. Planner allocation on the hot path. plannableIndexes() allocates a new ArrayList per call, and findIndexForProperty, handleClassWithIndexForSortOnly and handleTypeAsTargetWithIndex each call it, so one query can build several copies. Small next to the rest of planning, but given the GC emphasis in CLAUDE.md it may be worth computing the snapshot once per plan.

4. findBestIndexFor retry loop. The while (true) termination argument in the comment is correct (each retry strictly shrinks the list, otherwise it rethrows), but it is subtle for a planner method. A bounded loop, or filtering with isPlannable once before pickBestDescriptor, would be easier to reason about.

5. ConcurrentHashMap semantics. It rejects null keys/values and iteration is weakly consistent. I found no null put in LocalDocumentType, and getAllIndexes(false) now returns a weakly consistent live view instead of a fail-fast one, which is what you want here. Since the field is protected, worth a quick grep for writers in other modules.

Tests

• Good: the race test covers LSM and full-text, composite-index tie-breaking, and MATCH estimation, plus two deterministic unit tests for isReadyForQueries() and the empty TypeIndex.
• The race test is probabilistic (6 readers x 120 DDL cycles). Please check its duration in CI; if it takes more than a few seconds, tag it @Tag("slow") per CLAUDE.md.
• Missing: a deterministic test for the findBestIndexFor retry path (an index invalidated between plannableIndexes and ranking); the race test only reaches it by luck.
• Missing: no test covers the polymorphic case (supertype indexes) under concurrent DDL.

Style / security

Conventions look followed (final params, imports instead of FQNs, no System.out, assertj style). No new dependencies and no security concerns.

Review

Overall this is a solid, well-scoped fix. The root cause (planner walking a plain HashMap and reading type/null strategy of half-built or already-invalidated indexes) is addressed at the right layers: a concurrent map, a copy-on-write sub-index list, a volatile valid, a planner-side snapshot, and IndexException tolerance at the points where metadata is read. The regression test reproduces the race with real DDL threads and a correctness check on the results, which is better than the usual "no exception" test.

What looks good

• TypeIndex.drop() ordering (drop sub-indexes, then valid = false, then clear) is explained, and firstOrNull() makes every "check empty then getFirst()" pair a single read. That was a real TOCTOU in several methods (get, keyedCursor, getType, isResultApproximate, getIndexesByKeys).
• isReadyForQueries() never throws and is reused by both the planner and WhereClause.estimate, so there is one definition of "plannable".
• The retry loop in findBestIndexFor terminates (each pass returns, rethrows, or strictly shrinks the candidate list), and the comment says why.
• indexesByProperties as ConcurrentHashMap: I checked all usages in LocalDocumentType/LocalSchema and none put a null key or value, so the stricter null contract is safe. getAllIndexes(true) copies into a HashSet from a weakly consistent view, so the CME is gone there too.

Points to consider

1. Partial sub-index window can give silent incomplete results, not just an exception. In drop(), until valid = false is set, a reader sees a valid wrapper whose sub-index list is shrinking. isReadyForQueries() only checks that the first sub-index exists, so a plan can pick a wrapper that has lost some bucket indexes. If execution then runs against the partial list before valid flips, it returns fewer rows rather than throwing. The comment in drop() says readers get "an IndexException or a null type", which is not fully true for this case. The window is tiny and the index is being dropped anyway, so this may be acceptable, but it is worth either documenting accurately or closing (e.g. compare the sub-index count to the type's bucket count in isReadyForQueries).
2. firstOrNull() relies on CopyOnWriteArrayList#getFirst() reading the array once. That holds on JDK 21+ where COWAL overrides getFirst(); the default List#getFirst() is isEmpty() + get(0) and would reintroduce the race. Fine for this branch, but the comment is the only thing protecting it. try { return indexesOnBuckets.get(0); } catch (IndexOutOfBoundsException e) {...} would make the single-read property independent of the JDK override. Minor.
3. Residual unsynchronized maps in LocalDocumentType. bucketIndexesByBucket (HashMap of ArrayList) is mutated by addIndexInternal/removeBucketIndexInternal during the same DDL and read via getIndexesByBucketId. The planner paths in this PR do not touch it, so this is out of scope, but the same class of CME/NPE could show up on record-write paths running concurrently with index DDL. Probably a follow-up issue.
4. Per-plan allocation. plannableIndexes() allocates a new list on every call and it is called from several planner paths. Negligible next to planning cost, just noting it given the project's GC-sensitivity guidance.
5. Test runtime / tagging. SelectPlanningDuringIndexDdlTest does 120 rounds of create/drop (each index over 2,000 records, plus composite indexes in the non-full-text variant) against 6 reader threads, twice. If it takes more than a few seconds on CI, please tag the two race tests @Tag("slow") per CLAUDE.md. As a probabilistic race test it is also worth confirming it stays green over many repeated runs on a loaded machine (the PR description says it failed before and passed after, which is the right signal).
6. Test nit. The catch (Throwable) in the reader threads also folds AssertionError/OutOfMemoryError into the failure counter; fine here since they fail the final assertion anyway.

Style

Conforms to CLAUDE.md (final params, no FQNs, single-statement ifs without braces, assertThat(...) style). No debug System.out left.

None of the above blocks the merge; (1) is the only one I would want a conscious decision on.

Review of #8857

Solid, well-scoped fix for #8855. The root causes are correctly identified (non-thread-safe HashMap, planner reading metadata of half-built or invalidated indexes), the changes are minimal, and the regression test reproduces the original failure modes (6 readers vs 120 create/drop cycles). I found no blocking issues. Notes below, mostly minor.

Correctness / design

• TypeIndex.drop() ordering (valid = false then clear()) plus volatile valid and CopyOnWriteArrayList is sound. The comment on why the wrapper must stay valid while sub-indexes are dropped is useful. I checked the other users of indexesOnBuckets (getSubIndexes(), getIndexesOnBuckets(), addIndexOnBucket/removeIndexOnBucket): none sort, use ListIterator.remove, or rely on mutation features COW lists do not support.
• firstOrNull() uses get(0) and catches IndexOutOfBoundsException. It is correct and race-free. Exceptions as control flow are slow if the empty state ever becomes common, but it is rare today, so fine.
• isReadyForQueries() is documented as best-effort, and every planner path that reads metadata afterwards catches IndexException. Good layering. The while (true) retry in findBestIndexFor terminates as the comment argues (it rethrows or strictly shrinks the candidate list).
• Possible remaining gaps (not necessarily in scope): SelectExecutionPlanner around line 5564 iterates index.getSubIndexes() (safe structurally now thanks to COW, but a per-sub-index metadata read there can still hit a dropped index), and LocalDocumentType.bucketIndexesByBucket is still a plain HashMap mutated by DDL. If any reader path touches it concurrently it has the same class of problem.
• getAllIndexes(true) with supertypes builds a HashSet from the CHM values: weakly consistent, fine.

Performance

• plannableIndexes() allocates a list per planning call and isPlannable does a getType() per index. Planner-time only and small; acceptable. CopyOnWriteArrayList copies only on DDL (n = number of buckets). Read paths stay lock-free.

Style (CLAUDE.md)

• Consistent with existing code; the test uses the assertThat(...) style. The multi-line comment on TypeIndex.drop() is longer than usual, but the ordering invariant is non-obvious, so I think it earns its place.

Tests

• SelectPlanningDuringIndexDdlTest covers WHERE, ORDER BY via index, min/max, CONTAINSTEXT, MATCH and a full-text variant, and checks result correctness for the count queries, not only the absence of exceptions.
• As a race test it can only show absence of failures probabilistically. Consider @Tag("slow") if it takes more than a few seconds on CI (the 5 min @Timeout is fine as a hang detector).
• The retry loop in findBestIndexFor is only exercised by chance. A deterministic test (invalidate an index between candidate build and ranking) would be a nice addition.

Verdict: LGTM. Nothing blocking; optional follow-ups are the bucketIndexesByBucket map and a deterministic test for the retry loop.

Codecov Report

❌ Patch coverage is 76.66667% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.38%. Comparing base (1f41772) to head (6265409).
⚠️ Report is 38 commits behind head on main.

@@             Coverage Diff              @@
##               main    #8857      +/-   ##
============================================
+ Coverage     76.35%   76.38%   +0.02%     
- Complexity     3777     3780       +3     
============================================
  Files          2102     2104       +2     
  Lines        205880   206213     +333     
  Branches      43382    43453      +71     
============================================
+ Hits         157193   157506     +313     
- Misses        31730    31746      +16     
- Partials      16957    16961       +4     

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