ADFA-3822 Fix race condition and add DB guard for LSP indexing#1245
ADFA-3822 Fix race condition and add DB guard for LSP indexing#1245hal-eisen-adfa wants to merge 7 commits intostagefrom
Conversation
…n to BackgroundIndexer to fully close the race condition window between in-flight DB
📝 WalkthroughRelease NotesCore Changes
Risks & Best Practice Violations
Testing Recommendations
WalkthroughSQLiteIndex, BackgroundIndexer, and JvmSymbolIndex had their shutdown/lifecycle logic hardened: SQLiteIndex guards operations with a volatile Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BackgroundIndexer
participant SupervisorJob as Supervisor
participant WorkerJob as Worker
participant SQLiteIndex as Index
participant DB
Caller->>BackgroundIndexer: close()
BackgroundIndexer->>Supervisor: cancelAndJoin()
Supervisor-->>WorkerJob: cancel (in-flight)
WorkerJob->>Index: stop using DB
Caller->>Index: close() (may be concurrent)
Index->>Index: acquire mutex, set closed
Index->>DB: db.close()
DB-->>Index: closed
Index-->>Caller: closed
BackgroundIndexer-->>Caller: closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt (2)
161-185:insertAllouter loop is not guarded.The batch guard (
ifOpen { insertBatch(batch) }) is correct, but the outerfor (entry in entries)loop keeps pulling from a potentially expensiveSequenceeven after the DB is closed — each iteration just accumulates intobatchuntil the nextifOpensilently drops it. For a large provider that's already running at close time, this wastes CPU and delays the coroutine's exit.Consider a short-circuit check so a closed index stops draining the sequence:
♻️ Optional refactor
override suspend fun insertAll(entries: Sequence<T>) = withContext(Dispatchers.IO) { val batch = mutableListOf<T>() for (entry in entries) { + if (closed) return@withContext batch.add(entry) if (batch.size >= batchSize) { ifOpen { insertBatch(batch) } batch.clear() } } if (batch.isNotEmpty()) { ifOpen { insertBatch(batch) } } }This also helps
BackgroundIndexer.close()complete faster since the coroutine iterating the sequence will exit more promptly (standardSequenceiteration has no suspension points, so cancellation alone won't interrupt it).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt` around lines 161 - 185, The outer loop in insertAll keeps consuming a Sequence even after the index closes; fix by iterating via an iterator and short-circuiting when the index is closed (use the same open-check used by ifOpen/isOpen) so you stop pulling items as soon as the DB is closed — e.g. obtain val it = entries.iterator(); while (it.hasNext()) { if (!isOpen) break; batch.add(it.next()); ... } and ensure leftover batch is only passed to insertBatch via ifOpen; update insertAll to use this iterator+isOpen pattern referencing insertAll, insertBatch and ifOpen/isOpen so the Sequence stops being drained after close.
110-148: Safe-default semantics: ensure callers don't need to distinguish "closed" from "empty/absent".The read-side guards now silently return
emptySequence(),null,false,0onceclosed. Per theReadableIndexcontract (Index.kt:13-91), this is not a breaking change, andBackgroundIndexer.indexSource(lines 82-122) only relies on these returns for control flow, so closed-vs-absent ambiguity is tolerable here.Just flagging for awareness: if any future caller wants to distinguish "index is shut down" from "no results", they'll need an explicit API (e.g., an
isClosedaccessor) rather than catching exceptions. Fine to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt` around lines 110 - 148, The read-side guards (methods query, get, containsSource, distinctValues using ifOpen) intentionally return safe defaults when the index is closed, which hides "closed" vs "empty/absent" from callers; to allow callers to distinguish these states add an explicit isClosed/isShutdown accessor to the ReadableIndex interface and implement it in SQLiteIndex (so SQLiteIndex can report the closed state rather than relying on ifOpen defaults), then update callers that need to differentiate (e.g., BackgroundIndexer.indexSource) to check ReadableIndex.isClosed before relying on returned empty/null/false values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/util/BackgroundIndexer.kt`:
- Around line 167-170: The close() implementation currently blocks with
runBlocking { job.cancelAndJoin() } which can ANR on the main thread; change it
to wait with a bounded timeout and a fallback cancel: add a companion constant
CLOSE_TIMEOUT_MS = 5_000L, replace the blocking cancelAndJoin with a timed wait
(e.g., withTimeoutOrNull(CLOSE_TIMEOUT_MS) { job.cancelAndJoin() }) and if that
returns null call job.cancel() (or job.cancel(CancellationException(...))) to
force cancellation, then clear activeJobs as before; this leverages the existing
SQLiteIndex.closed and ifOpen guards so abandoned coroutines safely no-op on DB
calls.
---
Nitpick comments:
In
`@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt`:
- Around line 161-185: The outer loop in insertAll keeps consuming a Sequence
even after the index closes; fix by iterating via an iterator and
short-circuiting when the index is closed (use the same open-check used by
ifOpen/isOpen) so you stop pulling items as soon as the DB is closed — e.g.
obtain val it = entries.iterator(); while (it.hasNext()) { if (!isOpen) break;
batch.add(it.next()); ... } and ensure leftover batch is only passed to
insertBatch via ifOpen; update insertAll to use this iterator+isOpen pattern
referencing insertAll, insertBatch and ifOpen/isOpen so the Sequence stops being
drained after close.
- Around line 110-148: The read-side guards (methods query, get, containsSource,
distinctValues using ifOpen) intentionally return safe defaults when the index
is closed, which hides "closed" vs "empty/absent" from callers; to allow callers
to distinguish these states add an explicit isClosed/isShutdown accessor to the
ReadableIndex interface and implement it in SQLiteIndex (so SQLiteIndex can
report the closed state rather than relying on ifOpen defaults), then update
callers that need to differentiate (e.g., BackgroundIndexer.indexSource) to
check ReadableIndex.isClosed before relying on returned empty/null/false values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d82c6ad-f996-463f-9c57-de23337e151a
📒 Files selected for processing (3)
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.ktlsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/util/BackgroundIndexer.ktlsp/jvm-symbol-index/src/main/kotlin/org/appdevforall/codeonthego/indexing/jvm/JvmSymbolIndex.kt
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…ake BackgroundIndexer.close wait for cancellation completion and update misleading log text
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/util/BackgroundIndexer.kt (1)
47-51: Consider plumbingparentScopethrough production and test call sites.The default
CoroutineScope(Dispatchers.Default)preserves source compatibility, but creates practical issues:
Resource leak: Only 1 of 9 BackgroundIndexerTest tests calls
close(); all 15 JvmSymbolIndexTest tests create BackgroundIndexer viamakeIndex()without closing. The default scope's outerJobis never cancelled externally — relying entirely onclose(), which isn't called.Virtual time incompatibility: Tests use
runTest { … }, but BackgroundIndexer runs onDispatchers.Default, sorunTest's virtual time and task draining don't coordinate with the indexer's coroutines. Existing tests happen to pass because they either call.join()on jobs or assert post-close()state.Pass an explicit
parentScopefromJvmSymbolIndex.create()(from a scope owned by the index/registry) and from test methods (theTestScopefromrunTest). The default can remain for ergonomics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/util/BackgroundIndexer.kt` around lines 47 - 51, The BackgroundIndexer constructor currently defaults parentScope to CoroutineScope(Dispatchers.Default), causing leaked coroutines and test virtual-time mismatches; update call sites to pass an explicit parentScope instead of relying on the default: change JvmSymbolIndex.create (and any factory like makeIndex) to accept a CoroutineScope parameter and pass the owning scope (the index/registry scope) into BackgroundIndexer(parentScope = ...), and update tests to pass their runTest TestScope into makeIndex/create so the indexer's job and scope are children of the test/production scope (you may keep the default for ergonomics but ensure production factories and tests explicitly forward the scope).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt`:
- Around line 191-199: Add a main-thread safety check at the top of
SQLiteIndex.close(): detect Looper.getMainLooper() == Looper.myLooper() and emit
a warning via the existing logger (or add one) that closing on the main thread
may block/ANR, then continue with the current guarded close logic (runBlocking {
mutex.withLock { if (closed) return@withLock; closed = true; db.close() } }).
Update the method to reference SQLiteIndex.close(), mutex, closed, and db so
reviewers can locate the change; the check should mirror BackgroundIndexer’s
defensive warning behavior without altering the existing close semantics.
---
Nitpick comments:
In
`@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/util/BackgroundIndexer.kt`:
- Around line 47-51: The BackgroundIndexer constructor currently defaults
parentScope to CoroutineScope(Dispatchers.Default), causing leaked coroutines
and test virtual-time mismatches; update call sites to pass an explicit
parentScope instead of relying on the default: change JvmSymbolIndex.create (and
any factory like makeIndex) to accept a CoroutineScope parameter and pass the
owning scope (the index/registry scope) into BackgroundIndexer(parentScope =
...), and update tests to pass their runTest TestScope into makeIndex/create so
the indexer's job and scope are children of the test/production scope (you may
keep the default for ergonomics but ensure production factories and tests
explicitly forward the scope).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba7288f1-3685-45ee-ae3e-cc6307ec40df
📒 Files selected for processing (2)
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.ktlsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/util/BackgroundIndexer.kt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt (1)
43-49:⚠️ Potential issue | 🟡 MinorUpdate the class doc — WAL concurrency claim is now misleading.
The doc still states the index "Uses WAL journal mode for concurrent read/write performance," but with the new global
Mutexevery read and write is serialized throughifOpen { ... }. The underlying SQLite WAL mode no longer translates to concurrent access at this layer (this trade-off was explicitly acknowledged in the prior review when moving fromReentrantReadWriteLocktoMutex). Worth tightening the doc so future readers don't assume readers and writers can proceed in parallel.📝 Suggested doc tweak
- * Uses WAL journal mode for concurrent read/write performance. - * Inserts are batched inside transactions for throughput. + * The underlying database is opened in WAL journal mode, but all + * operations on this index are serialized through an internal [Mutex] + * to coordinate safe shutdown via [close]. Inserts are batched inside + * transactions for throughput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt` around lines 43 - 49, Update the SQLiteIndex class KDoc to remove the claim that WAL enables concurrent read/write at this layer and instead state that although the DB uses WAL, access is serialized by the global Mutex used in ifOpen, so reads and writes are effectively serialized (previously handled by ReentrantReadWriteLock); also clarify that query and distinctValues still eagerly collect results and that callers must run on an appropriate thread (e.g., Dispatchers.IO) as before.
🧹 Nitpick comments (1)
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt (1)
116-129: Optional: mirror the main-thread warning inquery()/distinctValues().Both methods now use
runBlocking { ifOpen(...) { ... } }. Because the mutex is global, a caller invoking these on the main thread can block on an in-flightinsertAllbatch (each batch holds the mutex while running a transaction) or onclose()itself, with the same ANR characteristics that motivated the warning inclose(). The class kdoc already requires callers to dispatch off the main thread, but a defensiveLooper-based warn would surface accidental misuse symmetrically withclose(). Feel free to skip if you'd rather rely solely on the doc contract.♻️ Optional symmetry fix
+ private fun warnIfMainThread(op: String) { + if (Looper.getMainLooper() == Looper.myLooper()) { + log.warn("SQLiteIndex.$op called on the main thread; may block on the index mutex and cause ANR") + } + } + override fun query(query: IndexQuery): Sequence<T> = runBlocking { + warnIfMainThread("query()") ifOpen(emptySequence()) { ... } } override fun distinctValues(fieldName: String): Sequence<String> = runBlocking { + warnIfMainThread("distinctValues()") ifOpen(emptySequence()) { ... } }(And refactor
close()to use the same helper for consistency.)Also applies to: 156-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt` around lines 116 - 129, Add the same main-thread Looper-based warning used in close() to query() and distinctValues(): detect if Looper.getMainLooper() == Looper.myLooper() and log a warning before calling runBlocking/ifOpen so accidental main-thread callers are warned; reuse the helper or warning message used by close() (and consider refactoring close() to call that shared helper) to keep behavior and wording consistent across the methods (target symbols: query(), distinctValues(), close(), ifOpen, runBlocking).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt`:
- Around line 43-49: Update the SQLiteIndex class KDoc to remove the claim that
WAL enables concurrent read/write at this layer and instead state that although
the DB uses WAL, access is serialized by the global Mutex used in ifOpen, so
reads and writes are effectively serialized (previously handled by
ReentrantReadWriteLock); also clarify that query and distinctValues still
eagerly collect results and that callers must run on an appropriate thread
(e.g., Dispatchers.IO) as before.
---
Nitpick comments:
In
`@lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt`:
- Around line 116-129: Add the same main-thread Looper-based warning used in
close() to query() and distinctValues(): detect if Looper.getMainLooper() ==
Looper.myLooper() and log a warning before calling runBlocking/ifOpen so
accidental main-thread callers are warned; reuse the helper or warning message
used by close() (and consider refactoring close() to call that shared helper) to
keep behavior and wording consistent across the methods (target symbols:
query(), distinctValues(), close(), ifOpen, runBlocking).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbcfa9e8-e09a-46e4-a097-e69928a1921f
📒 Files selected for processing (1)
lsp/indexing/src/main/kotlin/org/appdevforall/codeonthego/indexing/SQLiteIndex.kt
2 participants
Fix for Sentry APPDEVFORALL-S6