Repository navigation
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| private async closeConnect(): Promise<void> { | ||
| // Let any background compaction finish before releasing the connection. | ||
| await this.pending |
There was a problem hiding this comment.
WARNING: await this.pending only awaits the compaction promise that is current when closeConnect() runs. queueCompact()'s finally (line 611) can chain another compaction into this.pending when a write crosses the threshold while the first compaction is still running. closeConnect() resumes as soon as the promise it captured settles, so the chained compaction is never awaited and can execute after this.table = null / db.close(), i.e. a use-after-close (or it can reopen the connection through getDb()). That defeats the guarantee stated in the comment above this line. Consider awaiting a stable drain (e.g. loop while a per-instance in-flight flag is set, or track all chained runs) instead of a single promise snapshot.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| throw new Error(`Invalid point id format: ${bad}`) | ||
| // Serialize the delete+add commit against other stores' native work so | ||
| // concurrent batches cannot race each other or a compaction. | ||
| await native(async () => { |
There was a problem hiding this comment.
SUGGESTION: native() is backed by the module-level nativeQueue (line 18), shared by every LanceDBVectorStore instance in the process. Routing each upsert's delete+add through it (and the background compaction at line 625) serializes DB work across all workspaces, not just within one store. If cross-workspace serialization isn't intended, consider a per-instance queue so one workspace's write/compaction backlog can't stall another's indexing.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of The reworked regression test now creates only capitalized source dirs ( Targeted re-checks of the unchanged PR files found no carried-forward defects: Files Reviewed (1 PR file changed since the last review)
Previous Review Summaries (8 snapshots, latest commit caeb36c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit caeb36c)Status: No Issues Found | Recommendation: Merge Incremental review of Previously raised findings were re-verified against current code and remain resolved: Files Reviewed (0 PR files changed since the last review)
Previous review (commit 7cb5843)Status: No Issues Found | Recommendation: Merge Incremental review of Previously raised findings were re-verified against current code and remain resolved: Files Reviewed (1 file changed since the last review)
Previous review (commit d812f62)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (1 file changed since the last review)
Previous review (commit 857393d)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (1 file changed since the last review)
Previous review (commit bd2899e)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (2 files changed since the last review)
Previous review (commit 1ff7204)Status: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous review (commit 1fd4268)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (9 files)
Previous review (commit 01a6200)Status: 2 Issues Found | Recommendation: Address before merge The ignore-glob expansion ( Fix these issues in Kilo Cloud Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (9 files)
Reviewed by deepseek-flash-latest · Input: 37.7K · Output: 6K · Cached: 347.9K Review guidance: REVIEW.md from base branch |
github-actions
Bot
added
community
2358993 to
1fd4268
Compare
The scanner passed bare ignore folder names (node_modules, dist, ...) straight to glob's `ignore` option, where they match nothing at depth, so every ignored directory was walked and its paths materialized before the TypeScript filter dropped them. Expand bare names into anchored prune globs in one shared helper and use it from the scanner, watcher, and ignore-file discovery. LanceDB's optimizeTable pruned with a zero-length retention window (cleanupOlderThan: new Date()), which installs an auto-cleanup that races concurrent writers, logs "manifest not found", and can corrupt the store. Prune with a 10-minute retention instead, compact in the background every ~40 modification operations (Lance recommends ~20) without overlapping itself or the write path, and serialize writes/compaction through the native queue. Fixes Kilo-Org#14705
1fd4268 to
1ff7204
Compare
| nodir: true, | ||
| dot: false, | ||
| ignore: FileIgnore.PATTERNS, | ||
| ignore: FileIgnore.globs(), |
There was a problem hiding this comment.
Thanks for addressing these indexing issues. One regression needs fixing before merge: on macOS, glob defaults to case-insensitive matching, while FileIgnore.match() checks folder names case-sensitively. The new prune patterns therefore exclude valid sources such as src/Build/Main.ts, src/Target/Main.ts, and Desktop/App.ts, which were indexed before this change. I reproduced an incremental scan removing four such existing files from a real LanceDB index (five rows became one).
Please preserve the existing case-sensitive semantics, for example with nocase: false in the scanner glob, and add a scanner regression test covering capitalized source folders alongside lowercase ignored folders. The current suite passes but does not cover this case.
There was a problem hiding this comment.
Addressed. The scanner glob now passes nocase: false (packages/kilo-indexing/src/indexing/processors/scanner.ts:160) so the anchored FileIgnore.globs() patterns keep the same case-sensitive semantics as FileIgnore.match(). Added the scanner regression test keeps capitalized source folders that match ignored dirs case-insensitively, covering src/Build, src/Target, and Desktop alongside lowercase build directories. It no longer skips case-insensitive volumes, so it exercises the macOS path directly; I confirmed glob on platform: "darwin" prunes src/Build/Main.ts by default and keeps it with nocase: false. Branch is synced with main; kilo-indexing typecheck and tests pass.
github-actions
Bot
added
awaiting-contributor
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
Bot
added
needs-triage
|
Sweep note: branch is current with main (0 behind) and the review findings are resolved, but the latest CI run has a failure unrelated to this diff — |
…nup' into fix/indexing-memory-lancedb-cleanup
What & why
Two defects behind codebase-indexing trouble on large repos.
Scanner walked ignored directories (memory / OOM).
scanner.tspassedFileIgnore.PATTERNSstraight toglob'signoreoption. Most entries are bare directory names (node_modules,dist,build,target,vendor, …), and a bare name matches nothing at depth, so ignored directories were never pruned — every file under them was enumerated and retained before the TS-level filter dropped them. On a repo with a largenode_modulesthis is a large, avoidable memory/time cost. In the reported incident the indexing process was the one the WSL OOM killer SIGKILLed (exit 137;dmesg:Out of memory: Killed process … (kilo) anon-rss:6844612kB).Zero-retention LanceDB cleanup.
optimizeTable()usedcleanupOlderThan: new Date(), i.e. a 0 ms window. Lance treats that as an auto-cleanup policy that prunes every non-latest version; with concurrent writers it races, spamslance::io::commit … auto_cleanup_hook … manifest not found, and can leave the dataset unusable. This implements requested fix (2) in Code index LanceDB store corrupted by concurrent initialize()+optimize() across sessions/workers (no per-workspace lock) #14544 — use a real retention window instead of a zero/default policy.Changes
src/file/ignore.ts: addFileIgnore.globs()— expands bare folder names into anchored prune globs (x/**,**/x,**/x/**).src/indexing/processors/scanner.ts,src/indexing/processors/file-watcher.ts: useFileIgnore.globs()so ignored dirs are pruned, not walked.src/indexing/shared/load-ignore.ts:discovery()delegates toFileIgnore.globs()(single source of truth; no behavior change for ignore-file discovery).src/indexing/vector-store/lancedb-vector-store.ts:cleanupOlderThan: new Date(Date.now() - 10 min)instead ofnew Date();native()queue;close()awaits any pending compaction..changeset/quiet-lions-index.md(patch,@kilocode/kilo-indexing).Testing
bun run typecheck(kilo-indexing): clean. Full workspacebun typecheckincl.@kilocode/kilo-jetbrains: passing.bun test --timeout 30000(kilo-indexing): 515 pass / 0 fail, including newFileIgnore.globs()tests and updated compaction tests (background scheduling, non-blocking, coalescing, retention window).oxlinton changed files: 0 errors.Not included
This does not add the cross-process, per-workspace index lock requested as fix (1) in #14544. Separate OS processes targeting the same workspace can still race; this PR only removes the unsafe zero-retention prune and makes the in-process path safe (native-queue serialization). #14544 should stay open for the lock.
Relates to #14544
Fixes #14705
kilo-auto/efficient on behalf of maphew