23eb3f5233
Задания, отчёты и патчи, лежавшие в C:\Users\Ochenstarik\projects и в домашней папке, перенесены в agents/. Разложено по агентам там, где имя файла позволяло определить автора; остальное — в _salvage-2026-08-18/ и разбирается вручную. Патчи в notes/salvage-2026-08-18/ — незакоммиченная работа из брошенных рабочих копий: она существовала только на диске. Тяжёлое (релизные архивы, инсталляторы, наборы данных) в репозиторий не попало: оно лежит рядом, в Agent_projects/_archive и Agent_projects/_data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
125 lines
17 KiB
Markdown
125 lines
17 KiB
Markdown
# Ревью пакета 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.
|