server-monitor-manager/agents/_salvage-2026-08-18/smm-deliverables/Старые задачи/smm-task2-blockB-review-2026-08-03.md

Ревью пакета 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):

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.