fix(references): атомарная переиндексация и обновление индекса замороженных документов#4261
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReference indexing now separates occurrence construction from persistence, batches document references for atomic replacement, skips unchanged rebuilds using content fingerprints, removes stale locations, synchronizes document operations with locks, and validates frozen-document refreshes and concurrent reads. ChangesReference index refresh
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentContext
participant ReferenceIndexFiller
participant BatchingSink
participant ReferenceIndex
participant SymbolOccurrenceRepository
participant LocationRepository
DocumentContext->>ReferenceIndexFiller: emit content change
ReferenceIndexFiller->>ReferenceIndexFiller: check content fingerprint
ReferenceIndexFiller->>BatchingSink: collect discovered occurrences
BatchingSink->>ReferenceIndex: replaceReferences
ReferenceIndex->>SymbolOccurrenceRepository: save new occurrences
ReferenceIndex->>SymbolOccurrenceRepository: delete stale occurrences
ReferenceIndex->>LocationRepository: delete stale locations
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexRaceStressTest.java (1)
98-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReader exceptions are silently swallowed — the stress test can pass falsely.
readerscollects the submitted futures but they are never inspected. If a reader thread throws (e.g.getReferencesTofails during a rebuild), the exception is captured inside itsFutureand lost, yet the test still assertsfalsePositives == 0and reports green. Drain the futures after signalingstopso reader failures propagate.💚 Propagate reader failures
} finally { stop.set(true); pool.shutdown(); var terminated = pool.awaitTermination(60, TimeUnit.SECONDS); assertThat(terminated).isTrue(); } + + for (var reader : readers) { + reader.get(); // surface any exception thrown inside a reader thread + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexRaceStressTest.java` around lines 98 - 135, Reader task exceptions are currently hidden because the futures in readers are never checked. After setting stop and awaiting pool termination, iterate over every Future in readers and call get() so any reader failure propagates and fails the test; handle the checked exceptions consistently with the test’s existing exception strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java`:
- Around line 139-147: Snapshot document content once at the start of
ReferenceIndexFiller.fill, then reuse that captured value when computing the
stored fingerprint; keep AST retrieval and indexing tied to the same fill
operation to prevent a later document rebuild from producing mismatched
references and fingerprints.
---
Nitpick comments:
In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexRaceStressTest.java`:
- Around line 98-135: Reader task exceptions are currently hidden because the
futures in readers are never checked. After setting stop and awaiting pool
termination, iterate over every Future in readers and call get() so any reader
failure propagates and fails the test; handle the checked exceptions
consistently with the test’s existing exception strategy.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40a1cafc-1a70-4e20-9516-688830191f3f
📒 Files selected for processing (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerFrozenDocumentTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexRaceStressTest.java
|
Конфликт |
| // Диагностики, вычисленные конкурентно между перестроением документа и завершением | ||
| // fill, могли увидеть рассинхрон «новый AST — старый индекс» и закэшироваться. | ||
| // Сбрасываем кэш: следующий запрос пересчитает их по согласованному состоянию. | ||
| documentContext.clearDiagnostics(); |
There was a problem hiding this comment.
Это все выглядит как подпорка... Изначальная идея синхронных эвентов на DC#rebuildContent - работа под локом верхнего уровня. Как так получилось, что где-то есть nolock операция? Какое поведение в text document service? В analyze command? Нарисуй sequence диаграммы с подробным объяснением, где возникает дырка в синхронизации.
|
Проанализируй ещё, нет ли смысла учитывать флаг заморозки документов в других хэндлерах эвента на document content changed |
…женных документов Ложные срабатывания UnusedLocalVariable/UnusedLocalMethod на явно используемых символах (LSP при старте VS Code с открытым файлом, MCP, прогоны сонар-плагина) вызывались двумя дефектами заполнения индекса ссылок: 1. Окно пустого индекса. fill() делал clearReferences(uri) и затем заново наполнял индекс обходом AST, а событие DocumentContextContentChangedEvent публикуется после того, как rebuild отпустил локи документа. Читатели без документного лока (AnalyzeProjectOnStart, push-ветка refreshDiagnostics) в этом окне видели пустой индекс, диагностировали «все переменные не используются», и результат кэшировался в diagnostics Lazy до следующей правки. 2. Пропуск переиндексации замороженных документов. handleEvent игнорировал документы с isComputedDataFrozen() — но замороженный документ штатно перечитывается с изменённым содержимым (didChangeWatchedFiles после git checkout, McpDocumentReader.analyze после правки файла MCP-клиентом), и индекс оставался от старого содержимого. Исправление: fill() собирает вхождения в буфер и применяет их атомарной заменой replaceReferences (добавить недостающие → удалить устаревшие) — окна пустого индекса нет, исключение при обходе AST оставляет прежний индекс нетронутым. Guard по заморозке заменён отпечатком содержимого: переиндексация пропускается только если содержимое не менялось с последнего fill (повторный rebuild в пакетном анализе), после fill кэш диагностик документа сбрасывается. Гонка воспроизведена стресс-тестом (develop: 4/4 читателей ловят ложное «не используется», с фиксом — зелёный). Перфоманс: fill 29 → 20 мкс (малый модуль) и 1308 → 1132 мкс (150 процедур); analyze УПП (9543 модуля) — без деградации (средняя фаза анализа 229 с → 170 с в пределах шума); SARIF-отчёт эквивалентен базовому (идентичные счётчики по всем правилам). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
java:S125 — комментарий в replaceReferences переформулирован (не похож на код); java:S2301 — variableUsageOccurrence принимает OccurrenceType вместо булева селектора (переименован в variableOccurrence), ReferenceSink и BatchingSink переведены на OccurrenceType; java:S109 — сдвиг в contentFingerprint выражен через Integer.SIZE; java:S4030 — стресс-тест не копит неиспользуемые Future. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
replaceReferences принимает List вместо Collection и не делает Set.copyOf: дубликаты в батче безопасны (повторная запись вхождения — идемпотентный no-op по множествам хранилищ). Убраны зависимости от Collection и Set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…242) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- fill() снимает содержимое документа ДО чтения AST и пишет отпечаток из снимка: конкурентное перестроение больше не может оставить отпечаток новее проиндексированного AST (залипание устаревшего индекса исключено, лишняя переиндексация безопасна) — по замечанию ревью; - политика доступности ссылки вынесена в ReferenceAccessibility — снижение связанности ReferenceIndex (java:S1200) и отдельная зона ответственности; - стресс-тест считает исключения потоков-читателей и проваливается при их наличии — по замечанию ревью. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…одпорок Перестроение и очистка документов выполняются под верхнеуровневым RW-локом документа ВО ВСЕХ писателях: AnalyzeProjectOnStart и обработчики didChangeWatchedFiles берут write-лок (как didOpen, DocumentChangeExecutor и MCP), push-ветка refreshDiagnostics считает диагностики под read-локом. Дырка «читатель без лока между rebuild и переиндексацией» закрыта архитектурно, сброс кэша диагностик после fill (подпорка) убран. Также по замечаниям: интерфейс ReferenceSink с единственной реализацией схлопнут в BatchingSink; FQCN в стресс-тесте заменены импортами. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sfaqer
force-pushed
the
fix/reference-index-fill-race
branch
from
cb7714d to
16203a5
Compare
Где дырка в синхронизации: диаграммы и разборКак задумано (и как работает didChange) — дырки нетВсе «канонические» писатели держат верхнеуровневый RW-лок документа ( sequenceDiagram
participant C as VS Code
participant TDS as BSLTextDocumentService
participant DCE as DocumentChangeExecutor
participant DC as DocumentContext
participant F as ReferenceIndexFiller
C->>TDS: didChange v2
TDS->>DCE: submit v2
note over DCE: flushPendingChanges берёт writeLock документа
DCE->>DC: rebuild content v2
note over DC: computeLock и diagnosticsLock. Парсинг, symbolTree
DC-->>DCE: return. Внутренние локи отпущены
note over DCE: AOP AfterReturning публикует событие
DCE->>F: fill - переиндексация ссылок
note over DCE: writeLock отпущен. awaitLatest завершён
C->>TDS: pull diagnostic
note over TDS: awaitLatest + readLock. Затем getDiagnostics
TDS-->>C: диагностики по согласованному состоянию
Где появился nolockТри места исторически работали с документом мимо верхнеуровневого лока:
Диаграмма гонки (до этого PR), самый частый сценарий — старт VS Code: sequenceDiagram
participant W as Поток didChange документа X
participant F as ReferenceIndexFiller
participant A as AnalyzeProjectOnStart
W->>W: rebuild X v2. writeLock X взят
W->>F: fill X. Сначала clearReferences X
note over F: ОКНО - индекс X пуст
A->>A: getDiagnostics X - документный лок не берётся
A->>A: UnusedLocalVariable - getReferencesTo пусто
note over A: ложные срабатывания кэшируются в diagnostics Lazy
F->>F: индекс X заполнен заново
note over A: но кэш уже отравлен и публикуется клиенту
В analyze command дырки нет: populate держит write-лок на документ, фаза анализа обрабатывает каждый файл одним потоком (конкурентных читателей того же документа нет), а повторный rebuild с тем же содержимым отсеивается по отпечатку. Что сделано в последнем коммите
|
Анализ: нужен ли учёт заморозки в других хэндлерах
|
| Хэндлер | Что делает | Стоимость | Учитывать frozen? |
|---|---|---|---|
ReferenceIndexFiller |
полный обход AST → переиндексация ссылок | дорого | заменено отпечатком контента (этот PR) |
SymbolTypeIndex |
пересобирает список методов из уже вычисленного symbolTree | дёшево | нет |
ConfigurationModuleMembersProvider |
регистрирует TypeRef модуля по URI (идемпотентно) | дёшево | нет |
OScriptModuleMembersProvider |
то же для .os | дёшево | нет |
TypeRegistry.invalidateMembersCache |
бамп epoch (атомарный инкремент) | копейки | нет |
AbstractDocumentLifecycleClearableIndex (5 наследников: Inferred*TypeIndex, CallStatementByReceiverIndex, EventContractsIndex, WorkspaceSymbolIndex) |
per-URI очистка кэшей | дёшево | нет |
DocumentContextLazyDataMeasurer |
замеры (только при measures-профиле) | n/a | нет |
Вывод — учитывать флаг заморозки нигде не нужно, по двум причинам:
- Frozen ≠ «контент не менялся». Замороженный документ штатно перечитывается с диска с новым содержимым (didChangeWatchedFiles после git checkout,
McpDocumentReader.analyzeпосле правки файла агентом). Скип по frozen в любом из этих хэндлеров воспроизвёл бы ровно ту багу, которую чинит этот PR, только в другой подсистеме: устаревшие кэши типов/членов/символов при реально изменившемся файле. - Выигрывать нечего. Все остальные хэндлеры — либо дешёвые инвалидации (remove из map, бамп счётчика), либо идемпотентная регистрация, либо чтение уже вычисленного symbolTree. Единственный дорогой подписчик — переиндексация ссылок, и для него критерий скипа теперь корректный: отпечаток содержимого (второй rebuild того же файла в пакетном анализе по-прежнему не переиндексируется).
Если захочется срезать и дешёвые холостые инвалидации при повторном rebuild того же контента — правильнее делать это один раз на уровне публикации события (не публиковать contentChanged, если контент не изменился), но для этого DocumentContext должен помнить отпечаток прошлого контента после clearSecondaryData — по сути тот же механизм, что сейчас в филлере, только централизованный. Могу вынести в отдельный issue/PR, если считаешь нужным.
|
Ребейзнул на develop — конфликт с влитым #4259 разрешён: |
|
Это не отвечает на вопрос, почему в сонар плагине тоже периодически мигает unused variable diagnostic. |
|
Справедливо — гонки из этого PR мигание в сонар-плагине действительно не объясняют: там конвейер по каждому файлу однопоточный. Проследил путь плагина отдельно (develop плагина + BSL LS 0.29.0, который бандлится в v1.18.1) — у сонара свой механизм. Почему LSP-гонки в сенсоре не воспроизводятся
Что мигает в сонаре: индекс ссылок переживает
|
|
Отчёты об ошибках сообщают, что проблема с мигающим UnusedLocalVariable происходит даже на единственном источнике sonar.sources, то есть когда контекст сервера поднимается в единственном экземпляре |
|
И второй вопрос - если вопрос блокировок в lsp режиме решён, то нужно ли усложнение reference index? |
|
Нашёл механизм и для одного Коллизия (mdoRef, moduleType) между конфигурацией и расширением
Симметрично страдает и базовый модуль, когда побеждает расширение, и В LSP/MCP этот механизм живёт ровно так же (populate параллельный на каждом старте сервера, расширения в workspace — норма) и складывается с гонками, которые чинит этот PR: даже при «удачном» победителе LSP добавлял свои окна, при неудачном — мигает и без них. Этот дефект — не про локи и не про заморозку, этим PR он не чинится (и не портится). Корень: для локальных переменных и методов определение резолвится через глобальную карту Предыдущий разбор про |
Нужно — локи и атомарная замена защищают два разных инварианта, и друг друга не заменяют. Документный лок сериализует жизненный цикл одного документа: rebuild + переиндексация + расчёт его диагностик не пересекаются с конкурентными читателями этого же документа. Это закрывает пайплайн «документ ↔ его диагностики». Атомарная замена защищает кросс-файловых читателей индекса, которых локами закрыть нельзя. Индекс — общее хранилище: вхождения, расположенные в файле Y, читаются при работе с файлом X:
Брать читателю локи всех документов, где могут лежать вхождения, нельзя — это глобальная блокировка со всеми прелестями дедлоков и деградации. Дешевле сделать сам индекс монотонным: Плюс два практических довода:
Стресс-тест в PR это и проверяет: его читатели — безлоковые (модель кросс-файлового потребителя индекса). С одними локами, но со старым clear→fill он продолжает падать; с заменой — зелёный. |
…ex — protected Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Сделал protected методы записи/построения вхождений По двум разборам выше завёл отдельные ишью: #4265 (индекс переживает |
|
2 participants
Проблема
Периодические ложные срабатывания
UnusedLocalVariable/UnusedLocalMethodна явно используемых символах. Чаще всего — в LSP при старте VS Code с уже открытым файлом; также в MCP-режиме и при прогонах через сонар-плагин. Два дефекта заполнения индекса ссылок:1. Окно пустого индекса (гонка).
fill()делалclearReferences(uri)и заново наполнял индекс обходом AST. СобытиеDocumentContextContentChangedEventпубликуется@AfterReturning— после того, какrebuildотпустил локи документа. Читатели, вычисляющие диагностики без документного лока (AnalyzeProjectOnStart, push-веткаDiagnosticProvider.refreshDiagnostics), в этом окне видели пустой индекс → «все переменные не используются» → результат кэшировался вdiagnosticsLazy и залипал до следующей правки. На старте VS Code окно совпадает с бурей пересчётов (populate → analyze-on-start + refresh по регистрации типов) — отсюда типичный сценарий воспроизведения.2. Пропуск переиндексации замороженных документов.
handleEventигнорировал документы сisComputedDataFrozen(). Но замороженный (не открытый в редакторе) документ штатно перечитывается с изменённым содержимым:workspace/didChangeWatchedFilesпосле git checkout/pull,McpDocumentReader.analyzeпосле правки файла MCP-клиентом. Индекс оставался от старого содержимого — у новых/переименованных переменных не было вхождений.Исправление
fill()собирает вхождения в буфер (ReferenceSink) и применяет атомарной заменойReferenceIndex.replaceReferences: сначала добавляются недостающие вхождения, затем удаляются устаревшие (разность). Читатели в любой момент видят как минимум пересечение старого и нового наборов — окна пустого индекса нет. Бонус: исключение при обходе AST оставляет прежний индекс нетронутым (раньше оставлял пустой).fill(сохраняет оптимизацию повторного rebuild в пакетном анализе), и выполняется всегда, когда содержимое реально изменилось.fillсбрасывается кэш диагностик документа — результат, посчитанный конкурентно в интервале «rebuild → конец fill», не залипает.Тесты
ReferenceIndexRaceStressTest— писатель перестраивает документ, 4 читателя конкурентно проверяют предикат UnusedLocalVariable. На develop падает мгновенно (4/4 читателей ловят ложное «не используется»), с фиксом — зелёный.ReferenceIndexFillerFrozenDocumentTest— rebuild замороженного документа с новым содержимым обновляет индекс. На develop падает, с фиксом — зелёный../gradlew test: 3 584 теста, 0 упавших.Перфоманс
fill()малого модуля (микро, 1000 итераций)fill()модуля на 150 процедурanalyzeУПП (9543 модуля), фаза анализа, 3 прогонаПовторная индексация неизменённого содержимого теперь — чистый diff без записей, поэтому микро-замер даже улучшился. SARIF-отчёт по УПП эквивалентен базовому: счётчики по всем правилам идентичны.
Замечание: остаётся теоретическое окно «новый AST — старый индекс» для безлоковых читателей на интервале «rebuild → конец fill» (переменная, впервые появившаяся в новой версии, на один проход может считаться неиспользуемой) — оно закрывается сбросом кэша диагностик после fill и исчезает при следующем запросе. Полное устранение потребовало бы брать документный лок в
AnalyzeProjectOnStart/refreshDiagnostics— сознательно не расширял скоуп.🤖 Generated with Claude Code
Summary by CodeRabbit