embeddevio/awg-dash
1
0
Fork 0
Code Issues 40 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

BUG (#20/PR #50): apply — BaseException не покрыт (нет revert и failed-записи аудита); TOCTOU в set_geoip_schedule #55

New Issue
Open
opened 2026-10-22 00:20:37 +03:00 by embeddevio · 3 comments
embeddevio commented 2026-10-22 00:20:37 +03:00
Owner
Copy Link
Copy Source

Найдено на ревью PR #50 (head 7e10ffaf8ad436f10b4195fddc4120ce576b2c7d). Не блокирует merge само по себе, но правки нужны в этом же цикле. Refs #20, #50.

D-1 (средне): except Exception вокруг apply не покрывает BaseException

ApplyEngine.apply ловит Exception. При BaseException (в asyncio это asyncio.CancelledError, плюс KeyboardInterrupt/SystemExit) транзакция БД откатывается (contextmanager ловит BaseException), но:

  • Applier.revert(previous) не вызывается → внешняя подсистема (awg/iptables) может остаться в новом состоянии, пока БД уже откатилась;
  • в audit_log не пишется терминальная запись failed → остаётся «висящая» запись staged без исхода, ApplyError(restored=…) наружу не поднимается.

Фактический вывод пробы (внешний применитель поднимает KeyboardInterrupt):

BaseException из Applier -> KeyboardInterrupt (не ApplyError); revert вызван 0x;
аудит apply=['staged']; состояние БД восстановлено=True

Ожидаемое: любое прерывание применения конфигурации должно приводить к revert внешней подсистемы и к записи failed в аудит (минимум — не оставлять staged без исхода); отмена запроса не должна оставлять расхождение БД и iptables.

Проверка: тест с применителем, поднимающим BaseException/CancelledError — ожидается revert вызван и в аудите есть failed.

D-2 (низко): TOCTOU в Store.set_geoip_schedule

Расписание читается (self.get_geoip_schedule()) до открытия транзакции, затем пишется INSERT ... ON CONFLICT DO UPDATE. При конкурентном обновлении old_value в аудите может быть устаревшим: журнал заявит не то «старое» значение, которое реально было заменено. Мутации upsert_*/mark_config_applied читают и пишут внутри одной транзакции — там проблемы нет; здесь нужно перенести чтение внутрь with self._db.transaction().

Проверка: тест «два конкурентных set_geoip_schedule → каждое изменение имеет корректный old_value, журнал не противоречит состоянию».

Воспроизведено пробой F1 (tools/probe_qa.py) в воркспейсе задачи t_2dec61bb; полный отчёт — qa_report_pr50.md.

Найдено на ревью PR #50 (head `7e10ffaf8ad436f10b4195fddc4120ce576b2c7d`). Не блокирует merge само по себе, но правки нужны в этом же цикле. Refs #20, #50. ## D-1 (средне): `except Exception` вокруг apply не покрывает `BaseException` `ApplyEngine.apply` ловит `Exception`. При `BaseException` (в asyncio это `asyncio.CancelledError`, плюс `KeyboardInterrupt`/`SystemExit`) транзакция БД откатывается (contextmanager ловит `BaseException`), но: - `Applier.revert(previous)` **не вызывается** → внешняя подсистема (awg/iptables) может остаться в новом состоянии, пока БД уже откатилась; - в `audit_log` не пишется терминальная запись `failed` → остаётся «висящая» запись `staged` без исхода, `ApplyError(restored=…)` наружу не поднимается. Фактический вывод пробы (внешний применитель поднимает `KeyboardInterrupt`): ``` BaseException из Applier -> KeyboardInterrupt (не ApplyError); revert вызван 0x; аудит apply=['staged']; состояние БД восстановлено=True ``` Ожидаемое: любое прерывание применения конфигурации должно приводить к `revert` внешней подсистемы и к записи `failed` в аудит (минимум — не оставлять `staged` без исхода); отмена запроса не должна оставлять расхождение БД и iptables. Проверка: тест с применителем, поднимающим `BaseException`/`CancelledError` — ожидается `revert` вызван и в аудите есть `failed`. ## D-2 (низко): TOCTOU в `Store.set_geoip_schedule` Расписание читается (`self.get_geoip_schedule()`) **до** открытия транзакции, затем пишется `INSERT ... ON CONFLICT DO UPDATE`. При конкурентном обновлении `old_value` в аудите может быть устаревшим: журнал заявит не то «старое» значение, которое реально было заменено. Мутации `upsert_*`/`mark_config_applied` читают и пишут внутри одной транзакции — там проблемы нет; здесь нужно перенести чтение внутрь `with self._db.transaction()`. Проверка: тест «два конкурентных `set_geoip_schedule` → каждое изменение имеет корректный old_value, журнал не противоречит состоянию». Воспроизведено пробой F1 (`tools/probe_qa.py`) в воркспейсе задачи t_2dec61bb; полный отчёт — `qa_report_pr50.md`.
embeddevio added the backendqa labels 2026-10-22 00:20:37 +03:00
embeddevio commented 2026-10-28 21:14:25 +03:00
Author
Owner
Copy Link
Copy Source

Исправлено в feat/20-storage @ c4661c9db97ce4ed2c2c3471af06b18b69d99162

D-1 — BaseException теперь покрыт

ApplyEngine.apply (apply.py):

except BaseException as exc:
    restored_ok = self._rollback_failed_apply(plan, exc)
    if not isinstance(exc, Exception):
        raise                      # семантика отмены сохранена
    raise ApplyError(..., restored=restored_ok, audit_id=...) from exc

_rollback_failed_apply принимает BaseException, Applier.revert(previous) вызывается всегда (best-effort: падение revert не мешает восстановить БД; причина падения revert добавляется в detail, а исходное исключение не теряется), затем в одной транзакции восстанавливается состояние и пишется запись apply/failed. Сверка состояния после отката и _last_failed_audit_id читают через Database.read().

Тест: test_base_exception_from_applier_reverts_and_records_failure[KeyboardInterrupt | SystemExit | asyncio.CancelledError] — ожидания: pytest.raises(<тот же BaseException>), reverted == [previous], capture_state == before, верхняя запись аудита status='failed' с именем типа исключения в detail, new_value == previous.

# было (7e10ffaf):  BaseException -> KeyboardInterrupt; revert вызван 0x; аудит apply=['staged']
# стало (c4661c9d): BaseException -> KeyboardInterrupt; revert вызван 1x; аудит apply=['failed','staged']
#                   состояние восстановлено=True; причина отмены в detail

Старые сценарии не сломаны: ApplyError(restored=True) для Exception, падение revert всё равно восстанавливает БД (test_failed_apply_with_failing_revert_still_restores_db — теперь ещё и с причиной в detail).

D-2 — TOCTOU в set_geoip_schedule

repository.py: чтение текущего расписания перенесено внутрь with self._db.transaction(), значение для аудита (current) — тот же снимок, который заменяется; new_value — то же значение, что записано в БД (включая updated_at).

Тест: test_concurrent_geoip_updates_keep_audit_chain_consistent — два потока одновременно вызывают set_geoip_schedule; проверяется: 2 записи аудита, first.old_value.cron == исходное, first.new_value.cron == second.old_value.cron, second.new_value.cron == get_geoip_schedule().cron, множество новых значений == оба запрошенных cron. На 7e10ffaf тест падает ('0 2 * * *' != '0 4 * * *' — устаревший old_value), на c4661c9d — проходит.

Гейт на новом head: ruff check/format чисто, mypy strict чисто, pytest 105 passed (PR-only, coverage 97.79 %) / 107 passed с файлами #47 (97.80 %); пробы из ревью — probe_qa.py 40/40, probe_conc2.py 0 крашей/потерь.

## Исправлено в `feat/20-storage` @ `c4661c9db97ce4ed2c2c3471af06b18b69d99162` ### D-1 — `BaseException` теперь покрыт `ApplyEngine.apply` (`apply.py`): ```python except BaseException as exc: restored_ok = self._rollback_failed_apply(plan, exc) if not isinstance(exc, Exception): raise # семантика отмены сохранена raise ApplyError(..., restored=restored_ok, audit_id=...) from exc ``` `_rollback_failed_apply` принимает `BaseException`, `Applier.revert(previous)` вызывается **всегда** (best-effort: падение revert не мешает восстановить БД; причина падения revert добавляется в `detail`, а исходное исключение не теряется), затем в одной транзакции восстанавливается состояние и пишется запись `apply/failed`. Сверка состояния после отката и `_last_failed_audit_id` читают через `Database.read()`. Тест: `test_base_exception_from_applier_reverts_and_records_failure[KeyboardInterrupt | SystemExit | asyncio.CancelledError]` — ожидания: `pytest.raises(<тот же BaseException>)`, `reverted == [previous]`, `capture_state == before`, верхняя запись аудита `status='failed'` с именем типа исключения в `detail`, `new_value == previous`. ``` # было (7e10ffaf): BaseException -> KeyboardInterrupt; revert вызван 0x; аудит apply=['staged'] # стало (c4661c9d): BaseException -> KeyboardInterrupt; revert вызван 1x; аудит apply=['failed','staged'] # состояние восстановлено=True; причина отмены в detail ``` Старые сценарии не сломаны: `ApplyError(restored=True)` для `Exception`, падение revert всё равно восстанавливает БД (`test_failed_apply_with_failing_revert_still_restores_db` — теперь ещё и с причиной в `detail`). ### D-2 — TOCTOU в `set_geoip_schedule` `repository.py`: чтение текущего расписания перенесено внутрь `with self._db.transaction()`, значение для аудита (`current`) — тот же снимок, который заменяется; `new_value` — то же значение, что записано в БД (включая `updated_at`). Тест: `test_concurrent_geoip_updates_keep_audit_chain_consistent` — два потока одновременно вызывают `set_geoip_schedule`; проверяется: 2 записи аудита, `first.old_value.cron == исходное`, `first.new_value.cron == second.old_value.cron`, `second.new_value.cron == get_geoip_schedule().cron`, множество новых значений == оба запрошенных cron. На `7e10ffaf` тест падает (`'0 2 * * *' != '0 4 * * *'` — устаревший old_value), на `c4661c9d` — проходит. Гейт на новом head: ruff check/format чисто, mypy strict чисто, pytest 105 passed (PR-only, coverage 97.79 %) / 107 passed с файлами #47 (97.80 %); пробы из ревью — `probe_qa.py` 40/40, `probe_conc2.py` 0 крашей/потерь.
embeddevio commented 2026-10-28 21:27:05 +03:00
Author
Owner
Copy Link
Copy Source

QA (ревью раунд 2, PR #50 @ c4661c9d): D-1 и D-2 закрыты.

D-1 (BaseException): собственная проба на KeyboardInterrupt, SystemExit, asyncio.CancelledError — все три: тип исключения проброшен без изменения, Applier.revert(previous) вызван, БД восстановлена побайтово, в audit_log ровно одна failed-запись с причиной в detail, «висящих» staged без исхода нет. Мутация «except Exception вместо BaseException» → все три параметра теста падают (reverted == []).

D-2 (TOCTOU set_geoip_schedule): old_value читается внутри транзакции; 4 конкурентных обновления → цепочка old→new связна, итоговое состояние == последний new. Мутация «чтение вне транзакции» в 1 из 2 прогонов ломает тест (разрыв цепочки) — тест чувствителен к регрессии, хотя гонка узкая.

Гейт на merge-состоянии (main 063ecf18 + PR): ruff/format чисто, mypy strict 21 файл чисто, 107 passed, coverage 97.80 %.

Merge — после #17 (порядок #17 → #50); issue закрывается владельцем.

**QA (ревью раунд 2, PR #50 @ `c4661c9d`): D-1 и D-2 закрыты.** D-1 (`BaseException`): собственная проба на `KeyboardInterrupt`, `SystemExit`, `asyncio.CancelledError` — все три: тип исключения проброшен без изменения, `Applier.revert(previous)` вызван, БД восстановлена побайтово, в `audit_log` ровно одна `failed`-запись с причиной в `detail`, «висящих» `staged` без исхода нет. Мутация «`except Exception` вместо `BaseException`» → все три параметра теста падают (`reverted == []`). D-2 (TOCTOU `set_geoip_schedule`): `old_value` читается внутри транзакции; 4 конкурентных обновления → цепочка `old→new` связна, итоговое состояние == последний `new`. Мутация «чтение вне транзакции» в 1 из 2 прогонов ломает тест (разрыв цепочки) — тест чувствителен к регрессии, хотя гонка узкая. Гейт на merge-состоянии (`main` 063ecf18 + PR): ruff/format чисто, mypy strict 21 файл чисто, 107 passed, coverage 97.80 %. Merge — после #17 (порядок #17 → #50); issue закрывается владельцем.
embeddevio commented 2026-10-28 21:35:55 +03:00
Author
Owner
Copy Link
Copy Source

Закрыто: исправлено в PR #50 (head c4661c9d), влито в main.

  • D-1: apply теперь ловит BaseException — cleanup выполняется всегда: Exception → ApplyError, KeyboardInterrupt/SystemExit/CancelledError → Applier.revert + запись failed в аудите + проброс исходного типа. Тест: test_base_exception_from_applier_reverts_and_records_failure[KeyboardInterrupt/SystemExit/CancelledError]; проверено ревьюером (revert вызван, failed в аудите, состояние БД восстановлено).
  • D-2: old_value в set_geoip_schedule читается внутри транзакции — цепочка old→new связна при конкурентных обновлениях. Тест: test_concurrent_geoip_updates_keep_audit_chain_consistent (4 конкурентных обновления).

Мутационный откат каждого фикса валит соответствующие новые тесты.

Закрыто: исправлено в PR #50 (head `c4661c9d`), влито в `main`. - **D-1:** `apply` теперь ловит `BaseException` — cleanup выполняется всегда: `Exception` → `ApplyError`, `KeyboardInterrupt`/`SystemExit`/`CancelledError` → `Applier.revert` + запись `failed` в аудите + проброс исходного типа. Тест: `test_base_exception_from_applier_reverts_and_records_failure[KeyboardInterrupt/SystemExit/CancelledError]`; проверено ревьюером (revert вызван, `failed` в аудите, состояние БД восстановлено). - **D-2:** `old_value` в `set_geoip_schedule` читается внутри транзакции — цепочка `old→new` связна при конкурентных обновлениях. Тест: `test_concurrent_geoip_updates_keep_audit_chain_consistent` (4 конкурентных обновления). Мутационный откат каждого фикса валит соответствующие новые тесты.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
backend
Backend related work
 blocker
Blocking issue
 devops
Infra and deployment
 epic
Large feature or project
 frontend
Frontend related work
 hotfix
Urgent hotfix
 qa
Testing and QA
 release
Release request
 status/backlog
Kanban: не начато / бэклог
 status/blocked
Kanban: заблокировано, нужна эскалация
 status/in-progress
Kanban: в работе (есть ветка/PR)
 status/ready
Kanban: готово к работе, есть AC и зависимости
 status/review
Kanban: на ревью/QA
No Label backend qa
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
embeddevio
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: embeddevio/awg-dash#55
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?