fix(cache): prevent stale RLM BSL index reads - #326
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesThe change adds deterministic source-root generation and stores it with workspace index status. Index readiness checks generation, source root, database path, and file existence. RLM execution rejects stale indexes before and after execution, retires sessions, and schedules deduplicated maintenance. Source-generation-bound index lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceService
participant WorkspaceIndexService
participant RLMTransport
participant IndexMaintenance
WorkspaceService->>WorkspaceIndexService: validate source generation and index readiness
WorkspaceIndexService-->>WorkspaceService: return ready or stale status
WorkspaceService->>RLMTransport: execute RLM request
RLMTransport-->>WorkspaceService: return execution output
WorkspaceService->>WorkspaceIndexService: recheck generation after execution
WorkspaceService->>IndexMaintenance: schedule deduplicated maintenance when stale
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (3)
crates/unica-coder/src/infrastructure/workspace_services.rs (2)
6579-6579: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert against
SOURCE_GENERATION_STALE_STATUSinstead of the literal string.The constant is already imported at Line 10 and the production code uses it at Line 1724. Using the literal
"stale (source generation)"here lets the constant and the test drift apart.♻️ Proposed change
- assert_eq!(response.error.as_deref(), Some("stale (source generation)")); + assert_eq!( + response.error.as_deref(), + Some(SOURCE_GENERATION_STALE_STATUS) + );🤖 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 `@crates/unica-coder/src/infrastructure/workspace_services.rs` at line 6579, Update the assertion in the relevant test to compare response.error against the imported SOURCE_GENERATION_STALE_STATUS constant instead of the hard-coded "stale (source generation)" literal, preserving the existing assertion behavior.
5453-5473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the index lock path and payload from the production definitions.
write_active_rlm_index_lockhardcodeslocks/bsl_index.lockand hand-builds the lock JSON. The production lock path and lock schema live inworkspace_index.rs(lock_path, the lock record type). If either changes, this fixture stops creating an active lock and the test fails with a confusing readiness mismatch instead of a compile error.Expose a test-only helper from
workspace_index(for example apub(crate) fn lock_pathor a#[cfg(test)]lock writer) and call it here.🤖 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 `@crates/unica-coder/src/infrastructure/workspace_services.rs` around lines 5453 - 5473, Update write_active_rlm_index_lock to use a test-only helper exposed by workspace_index for both the production lock path and lock-record serialization. Remove the hardcoded locks/bsl_index.lock path and hand-built JSON, delegating lock creation to the existing lock_path or lock-writer symbol so fixture data stays aligned with production definitions.crates/unica-coder/tests/platform/issue_89_workspace_service.rs (1)
551-574: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the last observed status and yield the CPU between polls.
Two improvements make this helper more useful:
- The panic message names only the path. When this wait times out on CI, the operator cannot tell whether the status was
building,failed, or missing. Include the last read file content.- The loop calls
thread::yield_now()and re-reads the file on every iteration for up toRESPONSE_DEADLINE. That competes for CPU with the index subprocess this helper waits for.Fixture::finishalready uses a 10 ms sleep for the same kind of wait.♻️ Proposed change
fn wait_for_index_ready(&self, timeout: Duration) { let status_path = self.cache.join("caches/bsl_index_status.json"); let deadline = Instant::now() + timeout; + let mut last = String::new(); while Instant::now() < deadline { - let ready = fs::read_to_string(&status_path) - .ok() + let text = fs::read_to_string(&status_path).unwrap_or_default(); + if !text.is_empty() { + last = text.clone(); + } + let ready = Some(text) + .filter(|text| !text.is_empty()) .and_then(|text| serde_json::from_str::<Value>(&text).ok()) .is_some_and(|status| { status["status"] == "ready" && status["source_generation"].is_u64() && status["db_path"] .as_str() .is_some_and(|path| Path::new(path).is_file()) }); if ready { return; } - thread::yield_now(); + thread::sleep(Duration::from_millis(10)); } panic!( - "timed out waiting for generation-bound RLM index status at {}", - status_path.display() + "timed out waiting for generation-bound RLM index status at {}; last status: {last}", + status_path.display() ); }🤖 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 `@crates/unica-coder/tests/platform/issue_89_workspace_service.rs` around lines 551 - 574, Update Fixture::wait_for_index_ready to retain the last successfully read status file content and include it in the timeout panic alongside the status path, while preserving useful output when the file is missing or unreadable. Replace thread::yield_now() with a 10 ms sleep between polls, matching Fixture::finish and reducing CPU contention with the index subprocess.
🤖 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 `@crates/unica-coder/tests/platform/issue_89_workspace_service.rs`:
- Around line 254-260: Separate setup timing from cleanup timing in the test
around the existing started timestamp and index-readiness setup. Record a new
Instant immediately after wait_for_index_ready returns, then use that timestamp
for the 8-second cleanup-bound assertion while leaving the setup operations
unchanged.
---
Nitpick comments:
In `@crates/unica-coder/src/infrastructure/workspace_services.rs`:
- Line 6579: Update the assertion in the relevant test to compare response.error
against the imported SOURCE_GENERATION_STALE_STATUS constant instead of the
hard-coded "stale (source generation)" literal, preserving the existing
assertion behavior.
- Around line 5453-5473: Update write_active_rlm_index_lock to use a test-only
helper exposed by workspace_index for both the production lock path and
lock-record serialization. Remove the hardcoded locks/bsl_index.lock path and
hand-built JSON, delegating lock creation to the existing lock_path or
lock-writer symbol so fixture data stays aligned with production definitions.
In `@crates/unica-coder/tests/platform/issue_89_workspace_service.rs`:
- Around line 551-574: Update Fixture::wait_for_index_ready to retain the last
successfully read status file content and include it in the timeout panic
alongside the status path, while preserving useful output when the file is
missing or unreadable. Replace thread::yield_now() with a 10 ms sleep between
polls, matching Fixture::finish and reducing CPU contention with the index
subprocess.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7683c6bb-32fc-4ea5-b143-ff2de2139a28
📒 Files selected for processing (6)
crates/unica-coder/src/infrastructure/source_roots.rscrates/unica-coder/src/infrastructure/workspace_index.rscrates/unica-coder/src/infrastructure/workspace_services.rscrates/unica-coder/tests/platform/issue_89_workspace_service.rsdocs/design/2026-08-03-issue-286-rlm-source-generation-design.mddocs/plans/2026-08-03-issue-286-rlm-source-generation.md
|
Механика правильная: связать индекс с состоянием исходников и перестать верить 1. Перебазировать на
|
…-freshness # Conflicts: # crates/unica-coder/tests/platform/issue_89_workspace_service.rs
zeegin
left a comment
zeegin
left a comment
There was a problem hiding this comment.
Направление верное, реализация аккуратная, тесты содержательные. Локально на ветке PR: workspace_services::tests 95/95, issue_89_workspace_service 2/2, clippy -p unica-coder --lib -- -D warnings чист.
Ниже — то, что стоит решить до мержа.
Существенное
1. Четыре полных обхода дерева на каждый RLM-вызов (~4 с на реальной конфигурации)
source_generation в crates/unica-coder/src/infrastructure/source_roots.rs:201 — рекурсивный read_dir + metadata по всему дереву. Замер на реальном дампе 8.3.27 (43 299 файлов .bsl/.xml), release-сборка, прогретый кеш, APFS:
run 0: 1.255s run 1: 1.039s run 2: 1.039s run 3: 1.038s
~1.04 с за один вызов. Вызовов на один unica.code.definition:
| Точка | Обходов |
|---|---|
handle_rlm_ready → start_for_workspace_cancellable (workspace_index.rs:267) |
1 |
handle_rlm_ready → ready_index_cancellable (workspace_index.rs:376) |
1 |
handle_rlm_mcp → pre_execution_generation (workspace_services.rs:1672) |
1 |
handle_rlm_mcp → post_execution_generation (workspace_services.rs:1715) |
1 |
До PR обход был один (только для инвалидации тёплой сессии в handle_rlm_mcp). Стало четыре → +3 с чистого stat-а на вызов на этом корпусе, и это macOS; на Windows stat заметно дороже. При RLM_EXECUTE_TIMEOUT = 45 s это ещё не таймаут, но latency инструмента меняется на порядок.
Отдельно: и pre_execution_generation, и post_execution_generation считаются под удержанным self.rlm mutex — self.rlm.lock() берётся строкой выше. Два секундных обхода сериализуют все параллельные RLM-вызовы поверх и без того узкой rlm_lane.
Предложение: считать поколение один раз на запрос и протаскивать значение (handle_rlm_ready вычисляет его дважды подряд для одного и того же корня — чистое дублирование), плюс мемоизация per-source_root с коротким TTL. Как минимум post_execution_generation можно считать после drop(rlm).
2. Тихие обрезания в source_generation стали дырой в корректности
if depth > 8 (source_roots.rs:210) и .take(20_000) (source_roots.rs:246) — код перенесённый, не новый. Но его роль изменилась: раньше это была best-effort эвристика для сброса тёплых сессий, теперь это авторитетное доказательство свежести. Пропущенный файл больше не «лишний перезапуск сессии», а ровно тот дефект, который PR чинит.
На том же реальном корпусе:
- максимальная глубина
.bsl— ровно 8, то есть запас нулевой; - 7 реальных XML уже за границей, на глубине 10 — вложенные подсистемы вида
Subsystems/ЭлектронноеВзаимодействие/Subsystems/.../Subsystems/КонвертацияЭлектронныхДокументовСлужебный.xml.
Достаточно, чтобы source_root разрешился на уровень выше корня выгрузки — и весь BSL уходит за depth 8, поколение перестаёт меняться, fresh снова проходит, #286 воспроизводится молча. Просьба либо снять/поднять предел, либо при срабатывании обрезания помечать поколение недостоверным и не отдавать Ready.
3. Терминальный failed-маркер превратился в тупик без выхода
Тест переименован: fresh_info_replaces_matching_failed_marker → fresh_info_preserves_matching_failed_marker, и теперь утверждает status == "failed" и runner.backgrounds.borrow().is_empty().
Раньше fresh от info писал ready-статус и тем самым перезатирал терминальный маркер — это был единственный автоматический путь восстановления. Теперь ready-маркер пишет только фоновая операция, а matching_failed блокирует её старт в обеих точках (workspace_index.rs:279 и workspace_index.rs:383). Круг замкнут: фоновое задание не стартует, потому что маркер failed; маркер failed не снимается, потому что его снимает только фоновое задание.
Маркер достижим (recovery_exhausted && Stale в workspace_index.rs:996 — например, битая БД или переполненный диск в момент recovery build). После этого definition/search/profile навсегда отдают failed, и лечится только ручным удалением bsl_index_status.json. Нужен явный выход — самое естественное: снимать терминальный маркер, когда текущее поколение исходников отличается от того, при котором он записан (сейчас terminal_failure пишет source_generation: None, так что для этого его придётся заполнять).
4. Гонка mtime+size остаётся ровно на симптоме #286
Поколение строится из имени, размера и mtime. Правка Процедура Тест(А) → Процедура Тест(Б) не меняет размер; на ФС с посекундной гранулярностью mtime (сетевые монтирования, ext3, часть CI-образов) в пределах одной секунды поколение не изменится → fresh пройдёт → вернётся старая сигнатура. На APFS/NTFS с наносекундами практически не стреляет, но раз уж вся конструкция — про доказательство свежести, ограничение стоит проговорить в дизайн-записке (или взять хеш содержимого для BSL, что ещё сильнее ударит по п. 1).
Помельче
-
DefaultHasherтеперь персистится на диск. Std явно не гарантирует стабильность его хеша между релизами Rust. Направление отказа безопасное (несовпадение → переиндексация), но пересборка Unica на новом тулчейне разом инвалидирует все маркеры у всех пользователей. Стоит либо зафиксировать стабильный хеш, либо записать это как осознанный выбор. -
Поток обслуживания не отслеживается.
request_rlm_index_maintenance(workspace_services.rs:1621) клонирует токен запроса.OperationGuard::dropтолько снимает операцию с регистрации, не отменяя токен — значитbegin_shutdownуже не сможет отменить обслуживание, стартовавшее под завершившийся запрос; и наоборот,cancel_operation, попавший в узкое окно до возврата, убьёт только что запланированное обновление. Логичнее собственный токен, привязанный к жизни рантайма. -
Пост-проверка использует
pre_execution_generation. Если фоновое задание успело записать более новое поколение, пока шёл execute, корректный результат будет отброшен. Направление консервативное и безопасное, но неочевидное — просится комментарий. -
Дизайн-записка сама себе противоречит. Альтернатива №2 отклонена в том числе формулировкой «полный обход перед каждым чтением отдаёт политику производительности внешнему инструменту» (
docs/design/2026-08-03-issue-286-rlm-source-generation-design.md:60). Выбранный подход делает полный обход перед каждым чтением, четырежды, внутри процесса. Обоснование стоит переписать честно: цена перенесена в Unica, а не устранена. -
Ветка «метаданные недоступны» в
hash_source_pathхеширует0_u8и выходит, не подмешав путь, — в отличие от остальных веток. Сегодня безопасно, потому чтоstored_path_matchesотдельно пришивает маркер к корню, но асимметрия неочевидна.
Тесты
Покрытие хорошее и по делу: обе временны́е границы (...before_starting_session, ...before_fake_execute_returns), приоритет активной блокировки, дедупликация обслуживания, чтение legacy-маркера, фиксация поколения до команды. Блокирующая fixture на rustc с файловым рандеву тяжеловата, но для проверки «изменение исходников ровно в середине execute» альтернатив немного.
Чего не хватает: теста на выход из терминального маркера (п. 3) и стража на п. 2 — например, что файл глубже предела не проходит незамеченным.
Интеграционные тесты теперь ждут wait_for_index_ready с RESPONSE_DEADLINE = 10 s и зависят от реального планирования фонового задания. Локально проходят за 4.7 с, но это новая точка потенциальной флакости на нагруженной CI-машине.
Итог
Блокирующими считаю п. 1 (4× обход, ~+3 с на вызов, два из них под мьютексом) и п. 3 (невосстановимый терминальный маркер). П. 2 — не регрессия этого PR, но именно он делает старое ограничение опасным, так что закрыть его логичнее здесь же.
Ревью IngvarConsulting#326: обход стал корректным доказательством свежести, поэтому тихие обрезания убраны, а его цена снижена и измерена. - source_generation: без следования симлинкам, глубина 64 вместо 8, без потолка в 20 000 записей, имена вместо абсолютных путей, file_type из перечисления каталога вместо лишнего stat, фан-аут по верхнеуровневым детям (883 мс -> 438 мс на 43k файлов) - WorkspaceIndexService мемоизирует обход на время запроса: три обхода на вызов вместо четырёх - терминальный маркер пишет своё поколение и перестаёт блокировать перезапуск при изменении исходников - обслуживание индекса получило собственный токен отмены
|
Положил в ветку PR коммит Что измененоП. 1 — стоимость проверки. Обход упирается в метаданные ФС, поэтому работал по двум направлениям. Замер числа воркеров на том же корпусе: 1 → 883 мс, 2 → 541, 4 → 438, 8 → 496, 16 → 610. Дальше четырёх масштабирование отрицательное из-за контеншена на метаданных, поэтому предел зафиксирован на четырёх, а не на Плюс Итого: 1.13 с → 0.40 с за обход и 4 → 3 обхода на вызов, порядка 4.5 с → 1.2 с на П. 2 — тихие обрезания. Симлинки не обходятся, поэтому дерево не может зациклиться; глубина 64 вместо 8; потолок в 20 000 записей убран. Три регрессионных теста: правка ниже прежнего предела меняет поколение, симлинк-петля не ломает обход, фан-аут даёт стабильное значение. П. 3 — терминальный маркер. Пишет поколение, к которому относится, и перестаёт блокировать автоматический перезапуск, когда исходники изменились. Маркер без поколения считается неблокирующим и даёт ровно одну попытку — иначе воркспейсы, уже застрявшие на старом маркере, остались бы застрявшими навсегда. Два новых теста; восемь существующих фикстур переведены на хелпер Мелочи. Обслуживание индекса получило собственный токен отмены, привязанный к жизни рантайма и отменяемый в Дизайн-записка. Добавлен раздел «Стоимость проверки» с замерами, раздел про ограничение mtime+size (п. 4 — закрыть его полностью можно только хешированием содержимого, что несовместимо со стоимостью обхода), правила про симлинки и глубину, правило про освобождение терминального маркера. Обоснование альтернативы №2 переписано: цена обхода перенесена в Unica, а не устранена. Проверка
Локально в полном параллельном прогоне падают Что осталось незакрытымДаже после этого проверка стоит порядка 1.2 с на вызов на конфигурации такого размера — это нижняя граница для модели «полный обход перед каждым чтением». Уйти ниже можно только сменой модели обнаружения изменений: подписка на события ФС либо переиспользование собственного сигнала инвалидации workspace-сервиса. Это отдельная работа за рамками issue; зафиксировал в разделе «Стоимость проверки». |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@crates/unica-coder/src/infrastructure/source_roots.rs`:
- Around line 367-379: Update hash_source_file so any failure reading or
converting the modification timestamp—metadata.modified() or
duration_since(UNIX_EPOCH)—hashes UNREADABLE_ENTRY before returning. Preserve
hashing the file length and the existing seconds/nanoseconds timestamp values
when both operations succeed.
- Around line 603-625: Update the test source-generation fixture in
source_generation_is_stable_across_repeated_walks so all 64 generated
Module{index} directories are created directly under source_root rather than
beneath CommonModules. Keep the repeated source_generation calls and stability
assertion unchanged, ensuring source_root has enough immediate children for
child_digests to select the parallel fan-out path.
- Around line 360-365: Update is_source_file_name to normalize the extracted
extension to a consistent case before matching, so bsl, xml, yaml, and yml are
accepted regardless of capitalization while preserving the existing
supported-extension set.
In `@docs/design/2026-08-03-issue-286-rlm-source-generation-design.md`:
- Around line 59-65: Update the design’s post-execution validation flow for each
definition call to avoid two full source-tree traversals: retain the required
post-check, but reuse the existing workspace invalidation signal or another
bounded check for the second validation. Revise the related cost section to
document the resulting traversal count and timing, including the impact on RLM
lock duration.
- Around line 169-174: Revise the generation-fingerprint design to avoid
claiming unconditional freshness from filename, size, and coarse mtime alone.
Either use a stronger source-revision signal or explicitly narrow the freshness
guarantee, and update the readiness behavior for definition, search, and
meta.profile accordingly; add a regression test covering same-size edits within
one-second mtime granularity.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 642a1c2a-1c3b-4224-a669-f74f3261bbb5
📒 Files selected for processing (4)
crates/unica-coder/src/infrastructure/source_roots.rscrates/unica-coder/src/infrastructure/workspace_index.rscrates/unica-coder/src/infrastructure/workspace_services.rsdocs/design/2026-08-03-issue-286-rlm-source-generation-design.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/unica-coder/src/infrastructure/workspace_services.rs
- crates/unica-coder/src/infrastructure/workspace_index.rs
| fn is_source_file_name(name: &OsStr) -> bool { | ||
| Path::new(name) | ||
| .extension() | ||
| .and_then(|value| value.to_str()) | ||
| .is_some_and(|extension| matches!(extension, "bsl" | "xml" | "yaml" | "yml")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match source extensions case-insensitively.
is_source_file_name compares the extension with a case-sensitive matches!. A file named Module.BSL or Form.XML therefore fails the filter and is dropped from the fingerprint. Edits to such a file never change the generation, which is the stale-read class this walk prevents. Windows-authored and tool-generated 1C exports can carry uppercase extensions.
🐛 Proposed fix for the extension filter
fn is_source_file_name(name: &OsStr) -> bool {
Path::new(name)
.extension()
.and_then(|value| value.to_str())
- .is_some_and(|extension| matches!(extension, "bsl" | "xml" | "yaml" | "yml"))
+ .is_some_and(|extension| {
+ ["bsl", "xml", "yaml", "yml"]
+ .iter()
+ .any(|candidate| extension.eq_ignore_ascii_case(candidate))
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn is_source_file_name(name: &OsStr) -> bool { | |
| Path::new(name) | |
| .extension() | |
| .and_then(|value| value.to_str()) | |
| .is_some_and(|extension| matches!(extension, "bsl" | "xml" | "yaml" | "yml")) | |
| } | |
| fn is_source_file_name(name: &OsStr) -> bool { | |
| Path::new(name) | |
| .extension() | |
| .and_then(|value| value.to_str()) | |
| .is_some_and(|extension| { | |
| ["bsl", "xml", "yaml", "yml"] | |
| .iter() | |
| .any(|candidate| extension.eq_ignore_ascii_case(candidate)) | |
| }) | |
| } |
🤖 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 `@crates/unica-coder/src/infrastructure/source_roots.rs` around lines 360 -
365, Update is_source_file_name to normalize the extracted extension to a
consistent case before matching, so bsl, xml, yaml, and yml are accepted
regardless of capitalization while preserving the existing supported-extension
set.
| fn hash_source_file(hasher: &mut DefaultHasher, entry: &fs::DirEntry) { | ||
| let Ok(metadata) = entry.metadata() else { | ||
| UNREADABLE_ENTRY.hash(hasher); | ||
| return; | ||
| }; | ||
| metadata.len().hash(hasher); | ||
| if let Ok(modified) = metadata.modified() { | ||
| if let Ok(duration) = modified.duration_since(std::time::UNIX_EPOCH) { | ||
| duration.as_secs().hash(hasher); | ||
| duration.subsec_nanos().hash(hasher); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Emit UNREADABLE_ENTRY when the modification time cannot be read.
The constant at lines 216-218 states that the walk marks anything it knows exists but cannot read. hash_source_file does not follow that rule for the timestamp. If metadata.modified() fails, or duration_since(UNIX_EPOCH) fails, the file contributes only its length and no marker. Losing timestamp access then reads as "nothing changed" for every same-length edit to that file. modified() returns an error on platforms and filesystems that do not expose the field, so this is reachable.
🐛 Proposed fix for the unmarked timestamp failure
fn hash_source_file(hasher: &mut DefaultHasher, entry: &fs::DirEntry) {
let Ok(metadata) = entry.metadata() else {
UNREADABLE_ENTRY.hash(hasher);
return;
};
metadata.len().hash(hasher);
- if let Ok(modified) = metadata.modified() {
- if let Ok(duration) = modified.duration_since(std::time::UNIX_EPOCH) {
- duration.as_secs().hash(hasher);
- duration.subsec_nanos().hash(hasher);
- }
- }
+ match metadata
+ .modified()
+ .ok()
+ .and_then(|modified| modified.duration_since(std::time::UNIX_EPOCH).ok())
+ {
+ Some(duration) => {
+ duration.as_secs().hash(hasher);
+ duration.subsec_nanos().hash(hasher);
+ }
+ None => UNREADABLE_ENTRY.hash(hasher),
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn hash_source_file(hasher: &mut DefaultHasher, entry: &fs::DirEntry) { | |
| let Ok(metadata) = entry.metadata() else { | |
| UNREADABLE_ENTRY.hash(hasher); | |
| return; | |
| }; | |
| metadata.len().hash(hasher); | |
| if let Ok(modified) = metadata.modified() { | |
| if let Ok(duration) = modified.duration_since(std::time::UNIX_EPOCH) { | |
| duration.as_secs().hash(hasher); | |
| duration.subsec_nanos().hash(hasher); | |
| } | |
| } | |
| } | |
| fn hash_source_file(hasher: &mut DefaultHasher, entry: &fs::DirEntry) { | |
| let Ok(metadata) = entry.metadata() else { | |
| UNREADABLE_ENTRY.hash(hasher); | |
| return; | |
| }; | |
| metadata.len().hash(hasher); | |
| match metadata | |
| .modified() | |
| .ok() | |
| .and_then(|modified| modified.duration_since(std::time::UNIX_EPOCH).ok()) | |
| { | |
| Some(duration) => { | |
| duration.as_secs().hash(hasher); | |
| duration.subsec_nanos().hash(hasher); | |
| } | |
| None => UNREADABLE_ENTRY.hash(hasher), | |
| } | |
| } |
🤖 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 `@crates/unica-coder/src/infrastructure/source_roots.rs` around lines 367 -
379, Update hash_source_file so any failure reading or converting the
modification timestamp—metadata.modified() or duration_since(UNIX_EPOCH)—hashes
UNREADABLE_ENTRY before returning. Preserve hashing the file length and the
existing seconds/nanoseconds timestamp values when both operations succeed.
| #[test] | ||
| fn source_generation_is_stable_across_repeated_walks() { | ||
| let context = fixture(&[("main", "CONFIGURATION", "src")]); | ||
| let source_root = context.workspace_root.join("src"); | ||
| for index in 0..64 { | ||
| let module = source_root.join(format!("CommonModules/Module{index}/Ext/Module.bsl")); | ||
| fs::create_dir_all(module.parent().unwrap()).unwrap(); | ||
| fs::write( | ||
| &module, | ||
| format!("Процедура Тест{index}()\nКонецПроцедуры\n"), | ||
| ) | ||
| .unwrap(); | ||
| } | ||
|
|
||
| let first = source_generation(&source_root); | ||
|
|
||
| assert_eq!( | ||
| source_generation(&source_root), | ||
| first, | ||
| "the fan-out walk must fold worker results back in a stable order" | ||
| ); | ||
| cleanup(&context); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The determinism test does not reach the parallel path.
child_digests computes workers as the minimum of available_parallelism, children.len(), and MAX_SOURCE_WALK_WORKERS. children here means the children of the source root only. This fixture puts all 64 modules under one directory, CommonModules, so the root has about one child. workers is then 1 and child_digests returns through the sequential branch at lines 291-293. The assertion message at line 622 claims the fan-out walk is covered, but the scatter and gather code never runs.
Create the modules as direct children of source_root so the root fans out.
💚 Proposed fix to exercise the fan-out path
for index in 0..64 {
- let module = source_root.join(format!("CommonModules/Module{index}/Ext/Module.bsl"));
+ let module = source_root.join(format!("CommonModules{index}/Module/Ext/Module.bsl"));
fs::create_dir_all(module.parent().unwrap()).unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn source_generation_is_stable_across_repeated_walks() { | |
| let context = fixture(&[("main", "CONFIGURATION", "src")]); | |
| let source_root = context.workspace_root.join("src"); | |
| for index in 0..64 { | |
| let module = source_root.join(format!("CommonModules/Module{index}/Ext/Module.bsl")); | |
| fs::create_dir_all(module.parent().unwrap()).unwrap(); | |
| fs::write( | |
| &module, | |
| format!("Процедура Тест{index}()\nКонецПроцедуры\n"), | |
| ) | |
| .unwrap(); | |
| } | |
| let first = source_generation(&source_root); | |
| assert_eq!( | |
| source_generation(&source_root), | |
| first, | |
| "the fan-out walk must fold worker results back in a stable order" | |
| ); | |
| cleanup(&context); | |
| } | |
| #[test] | |
| fn source_generation_is_stable_across_repeated_walks() { | |
| let context = fixture(&[("main", "CONFIGURATION", "src")]); | |
| let source_root = context.workspace_root.join("src"); | |
| for index in 0..64 { | |
| let module = source_root.join(format!("CommonModules{index}/Module/Ext/Module.bsl")); | |
| fs::create_dir_all(module.parent().unwrap()).unwrap(); | |
| fs::write( | |
| &module, | |
| format!("Процедура Тест{index}()\nКонецПроцедуры\n"), | |
| ) | |
| .unwrap(); | |
| } | |
| let first = source_generation(&source_root); | |
| assert_eq!( | |
| source_generation(&source_root), | |
| first, | |
| "the fan-out walk must fold worker results back in a stable order" | |
| ); | |
| cleanup(&context); | |
| } |
🤖 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 `@crates/unica-coder/src/infrastructure/source_roots.rs` around lines 603 -
625, Update the test source-generation fixture in
source_generation_is_stable_across_repeated_walks so all 64 generated
Module{index} directories are created directly under source_root rather than
beneath CommonModules. Keep the repeated source_generation calls and stability
assertion unchanged, ensuring source_root has enough immediate children for
child_digests to select the parallel fan-out path.
| внутренние настройки закреплённой версии поставщика, а ограниченная выборка не | ||
| доказывает свежесть каждого файла большого проекта. | ||
|
|
||
| Полный обход перед каждым чтением остаётся и в выбранном подходе — разница в | ||
| том, что политику обхода задаёт Unica, а не закреплённая версия поставщика. Цена | ||
| обхода не устранена, а перенесена внутрь; она измерена в разделе «Стоимость | ||
| проверки». |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid two full post-execution traversals per definition call.
The design records about 1.3 seconds per call for a 43,299-file workspace and keeps the RLM lock across both checks. The PR objective requires the post-check, but it also requires avoiding the second full source-tree traversal. Reuse the existing workspace invalidation signal or another bounded check, then update this cost section.
Also applies to: 122-141
🤖 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 `@docs/design/2026-08-03-issue-286-rlm-source-generation-design.md` around
lines 59 - 65, Update the design’s post-execution validation flow for each
definition call to avoid two full source-tree traversals: retain the required
post-check, but reuse the existing workspace invalidation signal or another
bounded check for the second validation. Revise the related cost section to
document the resulting traversal count and timing, including the impact on RLM
lock duration.
| Поколение строится из имени, размера и времени изменения. Правка, не меняющая | ||
| размер файла, на файловой системе с посекундной гранулярностью `mtime` (сетевые | ||
| монтирования, ext3, часть CI-образов) в пределах одной секунды не изменит | ||
| поколение. На APFS и NTFS с наносекундным `mtime` ограничение не проявляется. | ||
| Закрыть его полностью можно только хешированием содержимого, что несовместимо со | ||
| стоимостью обхода из раздела выше. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not ship an unconditional freshness guarantee with this fingerprint.
On filesystems with one-second mtime granularity, a same-size BSL edit within one second produces the same generation. The readiness check can then accept an old RLM result, which conflicts with the objective that definition, search, and meta.profile reflect the current source revision. Use a stronger signal or narrow the guarantee and add a regression test for this case.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~170-~170: Прилагательное не согласуется с существительным по роду.
Context: ...е с посекундной гранулярностью mtime (сетевые монтирования, ext3, часть CI-образов) в пределах одн...
(Unify_Adj_NN_gender)
🤖 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 `@docs/design/2026-08-03-issue-286-rlm-source-generation-design.md` around
lines 169 - 174, Revise the generation-fingerprint design to avoid claiming
unconditional freshness from filename, size, and coarse mtime alone. Either use
a stronger source-revision signal or explicitly narrow the freshness guarantee,
and update the readiness behavior for definition, search, and meta.profile
accordingly; add a regression test covering same-size edits within one-second
mtime granularity.
Страж ADR-0009 запрещает cfg(unix) и std::os вне platform-фасада. create_dir_symlink_for_test уже покрывает обе ОС, поэтому тест ещё и перестал быть unix-only: на Windows без привилегии он возвращает None и тест штатно выходит.
2 participants
Что меняется
Fixes #286. RLM-backed чтение больше не принимает
freshот внешнего индексатора как достаточное доказательство актуальности: готовый индекс связывается с поколением BSL/XML-исходников, а workspace-сервис повторно проверяет эту связь непосредственно до и после RLM-вызова. Устаревший результат отбрасывается, сессия завершается, обновление индекса запускается асинхронно и дедуплицируется.unica.code.outlineостаётся на current-file AST-пути ADR-0020; исправление охватывает оставшиеся RLM-backed consumers (definition, индексную частьsearch,meta.profile).Архитектурный слой
Затронутые записи реестра:
INV-CACHE-ORCHESTRATOR-OWNED,INV-CACHE-WORKTREE-ISOLATION,INV-APP-LAZY-HIDDEN-SERVICES(контракты сохранены).Решение (ADR), если публичный или архитектурный контракт меняется: нет — публичная поверхность
unica.*и архитектурный контракт не меняются; реализация уточняет существующую модель ADR-0018 и сохраняет ADR-0020.Пройден чек-лист изменений в части, относящейся к этому изменению.
Проверка
rlm_execute_rechecks_generation_after_readiness_before_starting_session,rlm_execute_discards_output_when_source_changes_before_fake_execute_returns,rlm_execute_preserves_active_index_lock_priority_after_source_changeиrlm_execute_returns_stale_responses_and_deduplicates_blocked_maintenance.Локально затронутый модуль проходит 94/94, интеграционный target issue #89 — 2/2. Полная кроссплатформенная проверка подтверждена GitHub Actions run 30858736899.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation