# Ревью пакета Task 2 / Block B (2026-08-03) Пакет: `C:\Users\Ochenstarik\Сюда\Panel control\Task2-Block-B-2026-08-03\` База: `ba14d29` (PR #10 — Block A, уже в `main`) Ветка: `hermes/task2-b-link-reconciliation`, 14 файлов, 550/108 ## Вердикт: **доработать до commit** Код, который написан, написан хорошо и оба независимых review по нему корректны. Но **центральное требование Block B не выполнено**, и оба review этого не увидели, потому что проверяли диф на внутреннюю непротиворечивость, а не на соответствие постановке задачи. Ниже — доказательство. --- ## 1. Целостность пакета `SHA256SUMS` не сходится: | Файл | Статус | |---|---| | `block-b-current.diff` | ✅ совпадает | | `block-b-modified-files.zip` | ✅ совпадает | | `INDEPENDENT_REVIEW_CHANGES_REQUESTED.txt` | ✅ совпадает | | `REPORT.md` | ❌ `effa0b90…` в списке против `a3b74e5b…` фактически | | `TEST_EVIDENCE.md` | ❌ `52c154de…` против `3c7d01c6…` | | `INDEPENDENT_REPAIR_REVIEW_APPROVE.txt` | ❌ отсутствует в `SHA256SUMS` | Полезная нагрузка (диф и zip) верифицируется — содержательно пакет целый. Расхождение объясняется тем, что суммы посчитали до финального обновления отчётов. Но: следующий блок задачи 2 (Block C) целиком про то, что контрольные суммы и manifest должны быть достоверны. Пересчитывать `SHA256SUMS` последним действием, после всех правок отчётов, и включать в него все файлы пакета. --- ## 2. Что сделано хорошо - `link-status` — строгий вывод `active`/`disabled`, `lookup_node_ip` перед статусом, отказ при неизвестном действии и при лишних аргументах (есть тест). - `rule_exists` теперь **fail-closed**: `nft list chain` с ненулевым кодом приводит к `fail`, а не к «правил нет». Раньше `grep -Fq` на пустом выводе молча означал бы «правила нет» → ложный disconnect. Это правильное и важное изменение, покрытое контрактной проверкой в `test-bootstrap-contract.sh`. - Exit code больше не считается доказательством применения политики: после каждой мутации идёт `VerifyFactualStateAsync`, состояние в БД отражает пробу, а не факт запуска процесса. Тест `LinkServiceDoesNotPersistCommandSuccessAsFactualSuccess` бьёт ровно в это. - Реконсиляция перестала быть «только для Disabled» — `ReconcileLinksForNodeAsync` приводит Link к его desired-состоянию в обе стороны. - Node-локи с глобальной сортировкой, освобождением частично захваченных при отмене и порядком node → link. Цикла блокировок нет — проверил все четыре пути (`CreateAsync`, `DisableAsync`, `ReconcileLinksForNodeAsync`, `ReenrollAgentAsync`): ни один не берёт link-gate раньше node-lock. Вывод repair-review здесь верен. - Idempotency-replay больше не «съедает» прерванную привилегированную операцию: ранний возврат только для завершённых состояний. - Acceptance-скрипт получил `expect_factual_status` и проверяет desired+actual+lastError, а не только `actualState`. Проверки добавлены и в ветки `SMM_ACCEPT_RESTORE` и `SMM_ACCEPT_REBOOT`. - Desktop показывает desired/actual/drift/error/version, контракт закреплён в `Test-DesktopContracts.ps1`. Обе HIGH-находки первого review закрыты по существу, а не косметически, и на каждую есть регрессионный тест. --- ## 3. Главное: P0-2 закрыт только наполовину **Реконсиляция по-прежнему запускается ровно из одного места** — обработчика heartbeat: ``` Program.cs:353 if (mutation.RequiresReconciliation) await linkService.ReconcileLinksForNodeAsync(heartbeat.NodeId, ct); ``` Условие взведения флага (`ControlStore.RecordHeartbeatAsync`): ```csharp var reconnectThreshold = TimeSpan.FromSeconds(Math.Max(60, nextHeartbeatSeconds * 3)); // 90 с var requiresReconciliation = previousStatus != "Online" || previousLastSeenAt is null || now - previousLastSeenAt >= reconnectThreshold; ``` То есть реконсиляция происходит **только если Node пропадал из виду ≥ 90 секунд**. Ни `IHostedService` со стартовым проходом, ни периодического прохода в дифе нет; `ExpireDueLinksAsync` работает исключительно по выборке `ListExpiredLinksAsync` (TTL), здоровый `Active`-Link с `TtlMinutes = 0` не пересматривается никогда. **Незакрытые сценарии — те самые, ради которых блок и затевался:** 1. **`ochenstarik-smm-emergency firewall-restore`.** Команда удаляет и пересоздаёт `inet ochenstarik_smm`, все accept-правила исчезают, **ни один Node не уходит в offline**. Флаг не взводится, реконсиляция не запускается. В БД и в новом красивом UI — `Desired=Active`, `Actual=Active`, «Расхождение: нет». Фактически трафик заблокирован. `docs/linux-bootstrap.md:111` прямо обещает, что Control переприменит правила — обещание по-прежнему не выполняется. 2. **`systemctl restart ochenstarik-smm-firewall`** — то же самое, руками или из будущего update-сценария. 3. **Быстрый перезапуск Hub.** Если простой Control меньше 90 секунд (перезапуск сервиса, быстрый reboot VM, `update-control`), `previousStatus` остаётся `Online`, разрыв меньше порога — реконсиляции не будет. Правила при этом уже стёрты юнитом firewall при загрузке. 4. **Link между двумя Node, ни один из которых не переподключался** — например, Hub перезагрузился, Node A отчитался и был реконсилирован, Node B продолжал слать heartbeat без разрывов. Links, где B — единственный участник, останутся неприменёнными. Сформулировано в задаче это было пунктами **B2.1** («при старте Control **и далее периодически**, интервал `Control__LinkReconciliationSeconds`») и **B4** (маркер от emergency-команды). Ни то, ни другое не реализовано, и в `REPORT.md` это не отмечено как сознательный перенос. Почему это пропустили оба review: первый смотрел «безопасно ли коммитить этот диф», второй — «закрыты ли две HIGH». Полноту относительно постановки не проверял никто. На будущее — в промпт независимого review стоит класть текст задачи, а не только диф. **Что доделать (это и есть остаток Block B):** - `LinkReconciliationBackgroundService : BackgroundService` — проход при старте Control и далее раз в `Control__LinkReconciliationSeconds` (валидация 30…3600, по умолчанию 300), по всем Link, а не по одному Node. - Отдельная трактовка «таблицы нет» ≠ «правил ноль». Сейчас при отсутствии таблицы `link-status` падает с кодом 78, исключение ловится и каждый Link по очереди получает `Failed`/`Partial` с текстом ошибки от helper'а. Оператор видит N разных ошибок вместо одного внятного «mesh firewall не загружен». Нужен код возврата helper'а, отличающий отсутствие таблицы, и событие `mesh.firewall-missing` + перевод всех затронутых Link в `Partial` без попытки массового переприменения. - Маркер от `ochenstarik-smm-emergency firewall-restore`, по которому фоновый сервис делает внеочередной проход и снимает маркер после успеха. - Тест: правила стёрты, **ни один Node не переподключался** → в пределах одного интервала связность восстановлена. Именно этот тест и ловит текущий пробел. - В acceptance-скрипт: сценарий `firewall-restore` (или `nft delete table`) без reboot Node, затем `expect_reachable` после интервала реконсиляции. --- ## 4. Замечания среднего уровня **M1. Поиск Link по id через полное перечисление.** `CreateAsync` и `ConvergeDisabledCoreAsync` делают `(await store.ListLinksAsync(ct)).SingleOrDefault(c => c.Id == link.Id)`, `ReconcileLinksForNodeAsync` внутри цикла заново вызывает `ListEffectiveLinksForNodeAsync(nodeId)` для каждого кандидата. Это O(N) чтений и десериализаций на операцию, **под захваченной блокировкой**. При этом в `ControlStore.cs:1460` уже есть приватный `ReadLinkAsync(id)` — достаточно опубликовать `GetLinkAsync(string id, CancellationToken)` и заменить все три места. Иначе 100-node нагрузочный тест из Этапа 5 начнёт деградировать по мере роста числа политик. **M2. Семантика `LinkReconciliationResult.Reconciled` изменилась молча.** Раньше счётчик означал «привёл в порядок», теперь — «рассмотрел», включая Link, уже находящиеся в нужном состоянии. Это видно в изменённом тесте: `Assert.Equal(new LinkReconciliationResult(1, 0), disabledResult)` при `Assert.Equal(beforeReconnect, applier.DisconnectCalls)` — то есть «1 реконсилирован» при нуле мутаций. Если счётчик где-то попадёт в метрики или события, смысл будет ложным. Развести на `Examined` / `Converged` / `Failed`. **M3. Каждая проба статуса — отдельный `sudo`-процесс.** Реконсиляция N Link'ов теперь порождает до 3N процессов (`link-status`, мутация, повторный `link-status`). Для периодического прохода из п.3 это станет заметно. `link-apply-batch` (правила списком на stdin, одна транзакция `nft -f`) и пакетный `link-status` были в задаче — перенести в остаток блока. **M4. Links-страница стала журналом.** Фильтр `link.DesiredState != "Disabled" || link.ActualState == "Partial"` убран, теперь выводятся все строки таблицы `links` навсегда. Retention для `links` в `ControlMaintenance` нет (чистятся только `metric_samples`, `idempotency`, `audit`, токены). Через несколько месяцев список станет нечитаемым. Нужен либо фильтр по умолчанию «только действующие + расхождения» с переключателем, либо retention для завершённых Disabled-политик. **M5. Проба статуса для зарезервированного, но не активированного Node.** `status_rule` вызывает `lookup_node_ip` и падает, если Node есть в `nodes.tsv` со статусом `reserved` без адреса или отсутствует. Теперь это происходит не только при мутации, но и при каждой пробе — Link к такому Node будет регулярно перебрасываться в `Failed`. Стоит отличать «Node ещё не активирован в mesh» от «helper сломан» и не считать первое ошибкой применения политики. --- ## 5. Мелочи **L1.** `CompactError`: `Split(['\r', '\n'], …)` заменено на `Split([(char)13, '\n'], …)`. `(char)13` — это и есть `'\r'`. Изменение ничего не делает и ухудшает читаемость; похоже на артефакт инструмента, спотыкающегося о `\r` в патче. Вернуть как было. **L2.** `TEST_EVIDENCE.md` упоминает временный verifier `C:\Users\...\Temp\hermes-verify-t0w3drk4.sh`, «временно оставлен до commit checkpoint». Удалить до коммита; в отчёт такие файлы попадать не должны. **L3.** `Test-DesktopContracts.ps1` не был перезапущен после backend-правок («заблокирован approval timeout»). Формально backend-правки XAML не трогают, но перед PR прогнать. **L4.** Physical acceptance не выполнялся — это честно указано и не выдаётся за пройденное. Это правильно. Но критерий выхода Block B (`B6`) без него не закрывается, поэтому блок нельзя объявлять завершённым по факту merge — только по факту прогона `three-server-mesh.sh` с `SMM_ACCEPT_REBOOT=1` на реальной тройке. --- ## 6. Порядок действий 1. Убрать L1, L2. Пересчитать `SHA256SUMS` по всем файлам пакета последним шагом. 2. Доделать раздел 3 — фоновый реконсилятор, различение «таблицы нет», маркер emergency, тест «правила стёрты без переподключения Node». 3. M1 (`GetLinkAsync`) — дёшево и снимает риск деградации; M2 — переименование полей. 4. M3, M4, M5 — можно отдельным PR, но до объявления блока закрытым. 5. Прогнать `Test-DesktopContracts.ps1`, коммит, PR, CI. 6. Physical acceptance на тройке — выдать SSH/topology inputs, без этого критерий B6 остаётся открытым. Пункт 2 — обязательный для закрытия P0-2. Всё остальное из этого списка блокирующим не является. ## 7. Вне scope Block C (подписанный manifest, совместимость версий, пиннинг Actions) — по `smm-task-2-2026-07-31.md`, раздел 3. Начинать после закрытия пункта 2.