fix(orchestrator): закрыть два остатка на стыке guard и замка операций

Оба дефекта — в механизмах, введённых предыдущими коммитами, и оба относятся к гарантиям, ради которых эти механизмы вводились. 1. Отказ записи `auto-rollback-fired` оставался незамеченным. Инвариант фиксации "маркера нет и юниты inactive => guard не сработал" верен только при дополнительном условии "guard способен записать маркер". Пока `rc=0` стояло ПОСЛЕ создания маркера, отказ записи (заполненный tmpfs /run, read-only ФС) не влиял ни на что: скрипт успешно восстанавливал прежний firewall, завершался кодом 0, юнит уходил в inactive, маркера не было — и операция фиксировала успех после реально сработавшего отката. `rc` объявляется до первой операции, включая создание маркера, а ранний выход возвращает его вместо жёсткого `exit 0`. У факта срабатывания появилось два независимых канала: маркер и отказ юнита, потому что на пути фиксации успеха допустим ровно один ActiveState — inactive. Заодно маркер создаётся `touch`, а не `: >file`: двоеточие — special builtin POSIX, ошибка перенаправления на нём обязана завершить неинтерактивный shell целиком, и в dash скрипт умер бы ДО восстановления firewall. 2. Новая операция могла начаться, пока guard предыдущей ещё вооружён. Замок действует, пока жив процесс-держатель. Guard — отдельный объект systemd, переживающий свой процесс: A берёт замок -> применяет firewall -> вооружает guard на 45s A аварийно умирает B берёт замок и начинает менять production paths guard A срабатывает и возвращает firewall, который был ДО A Случай SIGTERM/SIGHUP хуже, чем kill -9: обработчик снимает замок сам, поэтому проверка живости держателя не видит вообще ничего, а таймер остаётся. Введён барьер покоя `assertNoPendingRollbackGuard`, через который проходит каждый захват замка — дважды, до и после, потому что между ними умирающая операция успевает вооружить guard, — и PHASE 0 установщика. Непокоем считаются active/activating/deactivating/reloading; `failed` и `inactive` — покой, иначе барьер блокировал бы `repair`, которым чинят последствия. Плюс P1: восстановление UnitFileState у nftables.service больше не обещает точности, которой не даёт. `enable --runtime` не удаляет постоянную ссылку, поэтому "восстановление" enabled-runtime оставляло юнит включённым в обоих scope. Восстанавливаются enabled/disabled — то, что операция реально меняет, — остальные состояния называются оператору и не трогаются. Тесты: поведенческая проверка раннего пути rollback-скрипта настоящим shell (ветка заканчивается до первой команды восстановления и безопасна для запуска), проверка двойного вызова барьера и снятия замка при его отказе, структурные инварианты. Приёмка и docs (D1h, уточнение D1f) — там же.
2026-08-31 03:30:14 +05:00
parent 0230f1ca99
commit 76d78ac71f
12 changed files with 820 additions and 30 deletions
@@ -71,12 +71,37 @@ export class FirewallGuardFiredError extends Error {
  }
 }

+const ROLLBACK_UNIT_PREFIX = "hy2xs-fw-rollback-";
+
+/**
+ * Состояния, в которых guard ещё СПОСОБЕН изменить систему.
+ *
+ * `failed` и `inactive` сюда не входят намеренно. Guard, который уже отработал
+ * (успешно или нет), больше ничего не сделает, а отказавший юнит — это как раз
+ * повод запустить `repair`. Барьер, отказывающий по `failed`, блокировал бы
+ * ровно тот инструмент, которым чинят последствия.
+ */
+const GUARD_PENDING_STATES = ["active", "activating", "deactivating", "reloading"] as const;
+
+/**
+ * Предыдущая операция мертва, но её асинхронный исполнитель ещё жив.
+ *
+ * Отдельный тип, потому что это единственный отказ, который не про текущую
+ * операцию: она не сделала ничего плохого, ей просто нельзя начинать.
+ */
+export class PendingRecoveryError extends Error {
+  constructor(message: string) {
+    super(message);
+    this.name = "PendingRecoveryError";
+  }
+}
+
 function rollbackRoot(opId: string): string {
  return `/run/hy2xs/rollback/${opId}`;
 }

 function rollbackUnit(opId: string): string {
-  return `hy2xs-fw-rollback-${opId}`;
+  return `${ROLLBACK_UNIT_PREFIX}${opId}`;
 }

 /**
@@ -321,6 +346,26 @@ export async function detectFirewallEntrypointKind(): Promise<FirewallEntrypoint
 *    оставалась успешная запись. Теперь каждая стадия независима, её отказ
 *    поднимает `rc`, и юнит честно уходит в `failed` с диагностикой в journal.
 *
+ * 3. Создание маркера входит в учёт `rc`, и это не мелочь, а второе плечо
+ *    инварианта фиксации. Инвариант
+ *
+ *        маркер отсутствует И юниты inactive => guard не сработал
+ *
+ *    верен только при дополнительном условии «guard способен записать маркер».
+ *    Пока `rc=0` стояло ПОСЛЕ создания маркера, отказ записи (заполненный
+ *    tmpfs /run, read-only ФС, ошибка ввода-вывода) не влиял ни на что: скрипт
+ *    успешно восстанавливал прежний firewall и завершался кодом 0, юнит уходил
+ *    в `inactive`, маркера не было — и операция фиксировала успех после
+ *    РЕАЛЬНО сработавшего отката.
+ *
+ *    Теперь у факта срабатывания два независимых канала:
+ *
+ *        маркер          — обычный;
+ *        отказ юнита     — аварийный, когда маркер записать не удалось.
+ *
+ *    Второй работает потому, что на пути фиксации успеха допустим ровно один
+ *    `ActiveState` — `inactive`, а `failed` фиксацию запрещает.
+ *
 * Состояние nftables.service скрипт СОЗНАТЕЛЬНО не восстанавливает: на Debian у
 * этого юнита `ExecStop=/usr/sbin/nft flush ruleset`, то есть остановка сервиса
 * стёрла бы только что восстановленные правила — прямо противоположно задаче
@@ -344,18 +389,38 @@ export function buildAutoRollbackScript(opId: string): string {

 root='${root}'

+# rc объявляется ДО первой операции, включая создание маркера срабатывания.
+#
+# Иначе отказ записи маркера не влиял бы ни на что: скрипт успешно восстановил
+# бы прежний firewall и завершился кодом 0, а операция, не увидев маркера и
+# увидев inactive-юнит, зафиксировала бы успех после реально сработавшего
+# отката. Отказ юнита — аварийный канал того же факта.
+rc=0
+
 # Маркер срабатывания — первым действием, до любой проверки. Операция обязана
 # узнать, что guard сработал, даже если восстановление ниже не удалось.
-mkdir -p "$root"
-: >"$root/${AUTO_ROLLBACK_FIRED_MARKER}"
+if ! mkdir -p "$root"; then
+  echo "hy2xs auto-rollback: failed to access the recovery root $root" >&2
+  rc=1
+fi
+# touch, а НЕ \`: >file\`.
+#
+# Двоеточие — special builtin POSIX, и ошибка перенаправления на нём обязана
+# завершить неинтерактивный shell целиком. В dash, который на Debian и есть
+# /bin/sh, это означало бы, что при недоступном /run скрипт умирает ДО
+# восстановления firewall — то есть guard перестаёт делать ровно то, ради чего
+# существует. touch — обычная внешняя команда, её код возврата просто
+# возвращается в if.
+if ! touch "$root/${AUTO_ROLLBACK_FIRED_MARKER}"; then
+  echo "hy2xs auto-rollback: failed to create the fired marker in $root" >&2
+  rc=1
+fi

 if [ ! -f "$root/prepared" ]; then
  echo 'hy2xs auto-rollback: prepared marker is absent, nothing to restore' >&2
-  exit 0
+  exit "$rc"
 fi

-rc=0
-
 # $1 — маркер существования, $2 — резервная копия, $3 — целевой путь.
 restore_file() {
  if [ -f "$1" ]; then
@@ -571,6 +636,81 @@ export async function assertEffectiveFirewallIsOurs(context: RuntimeContext): Pr
  }
 }

+/**
+ * Транзиентные юниты guard, которые сейчас известны systemd.
+ *
+ * Отказ самого запроса не считается доказательством наличия guard: без systemd
+ * не может быть и транзиентного таймера, а требование systemd живёт в
+ * preflight, где отказ будет понятнее и точнее.
+ */
+export async function listRollbackGuardUnits(): Promise<string[]> {
+  let listed: string;
+  try {
+    listed = await runReadOnly`systemctl list-units --all --plain --no-legend ${`${ROLLBACK_UNIT_PREFIX}*.timer`} ${`${ROLLBACK_UNIT_PREFIX}*.service`}`;
+  } catch (error) {
+    info(
+      `unable to list firewall rollback guard units: ${error instanceof Error ? error.message : String(error)}`
+    );
+    return [];
+  }
+
+  return listed
+    .split("\n")
+    .map((line) => line.trim().split(/\s+/)[0] ?? "")
+    .filter((unit) => unit.startsWith(ROLLBACK_UNIT_PREFIX));
+}
+
+/**
+ * Барьер покоя: у предыдущей операции не осталось асинхронных исполнителей.
+ *
+ * Замок операций и rollback guard вводились по отдельности и по отдельности же
+ * оставляли дыру на своём стыке. Замок защищает production paths, пока ЖИВ
+ * процесс-держатель. Guard — это отдельный systemd-объект, который переживает
+ * свой процесс:
+ *
+ *     A берёт замок -> применяет firewall -> взводит guard на 45s
+ *     A аварийно умирает
+ *     B берёт замок (либо снятый обработчиком сигнала, либо переиспользованный)
+ *     B начинает менять production paths
+ *     guard A срабатывает и возвращает firewall, который был ДО A
+ *
+ * Уникальные op-id здесь не помогают: каталоги копий разные, а
+ * /etc/nftables.conf, /etc/nftables.d/hy2xs.nft и ruleset в ядре — общие.
+ *
+ * Поэтому правильное условие для начала новой операции — не «PID предыдущей
+ * мёртв», а «у предыдущей не осталось исполнителей, способных изменить
+ * систему». Проверка обязательна при ЛЮБОМ захвате замка, а не только при
+ * переиспользовании устаревшего: обработчик сигналов снимает замок сам, и в
+ * этом случае stale-замка просто не будет, а таймер останется.
+ *
+ * Сознательно НЕ проверяются `hysteria-server`, `hy2xs-admin` и
+ * `nftables.service`: незавершённый `systemctl restart` ничего не откатывает,
+ * он лишь повторяет то, что новая операция сделает сама, а отказ по их
+ * переходным состояниям заблокировал бы `repair` ровно тогда, когда он нужен.
+ */
+export async function assertNoPendingRollbackGuard(): Promise<void> {
+  const pending: string[] = [];
+
+  for (const unit of await listRollbackGuardUnits()) {
+    const state = await readUnitProperty(unit, "ActiveState");
+    if ((GUARD_PENDING_STATES as readonly string[]).includes(state)) {
+      pending.push(`${unit} (${state})`);
+    }
+  }
+
+  if (pending.length === 0) {
+    return;
+  }
+
+  throw new PendingRecoveryError(
+    "previous HY2XS operation is no longer running, but its firewall rollback guard is still armed: " +
+      `${pending.join(", ")}. ` +
+      "Такой guard способен вернуть прежний firewall уже посреди новой операции. " +
+      "Дождитесь его завершения (окно — 45 секунд с момента применения firewall) и повторите; " +
+      "состояние guard видно в `hy2xs-orchestrator status` и в `journalctl -u 'hy2xs-fw-rollback-*'`."
+  );
+}
+
 function firewallRollbackIsInactive(context: RuntimeContext): boolean {
  return (
    context.options.skipFirewall ||
@@ -801,25 +941,32 @@ export async function rollbackFirewallNow(context: RuntimeContext): Promise<void
        info("nftables.service state was not captured for this operation; unit file state is left as is");
        return;
      }
+      // Восстанавливается ровно то, что операция могла изменить, и ровно так,
+      // как это можно сделать достоверно.
+      //
+      // Единственная мутация этого юнита в applyFirewall — постоянный
+      // `systemctl enable --now nftables`. Её точная отмена существует для двух
+      // состояний: `enabled` (ничего менять не нужно) и `disabled` (убрать
+      // добавленную нами постоянную ссылку).
+      //
+      // Остальные состояния сознательно НЕ трогаются, и это исправление
+      // прежнего поведения, а не пропуск. `enable --runtime` не удаляет
+      // постоянную ссылку, поэтому «восстановление» `enabled-runtime` таким
+      // вызовом оставляло юнит включённым в обоих scope'ах — то есть обещало
+      // точность, которой не давало. `masked`/`masked-runtime` восстанавливать
+      // не нужно вовсе: на замаскированном юните `enable --now` отказывает, и
+      // операция падает, ничего не изменив.
      switch (serviceState.unitFileState) {
        case "enabled":
          await runMutatingVisible`systemctl enable nftables`;
          return;
-        case "enabled-runtime":
-          await runMutatingVisible`systemctl enable --runtime nftables`;
-          return;
        case "disabled":
          await runMutatingVisible`systemctl disable nftables`;
          return;
-        case "masked":
-        case "masked-runtime":
-          await runMutatingVisible`systemctl mask nftables`;
-          return;
        default:
-          // static/indirect/generated/transient/пусто: у таких юнитов
-          // enable/disable либо бессмысленны, либо отказывают.
          info(
-            `nftables.service unit file state "${serviceState.unitFileState || "(empty)"}" is not restorable explicitly; skipped`
+            `nftables.service unit file state "${serviceState.unitFileState || "(empty)"}" is left as is: ` +
+              "точное восстановление этого состояния не гарантируется, а операция не могла его изменить"
          );
      }
    }