Sprint 0 / #20: хранилище состояния — SQLite WAL, миграции, audit log, атомарный apply/rollback #50

Merged
embeddevio merged 8 commits from feat/20-storage into main 2026-10-28 21:35:42 +03:00
Owner

Refs #20, Refs #15. Refs #54, Refs #55.

Реализует слой хранения состояния из issue #20 целиком: backend/awg_api/store/ (#20, карта модулей из backend/awg_api/README.md) + тесты.

Что сделано

SQLite (WAL) в /data/awg-dash.db — db.py

  • Database: PRAGMA journal_mode=WAL, synchronous=NORMAL, foreign_keys=ON, busy_timeout=5000; BEGIN IMMEDIATE и rollback при любом исключении.
  • Контракт потокобезопасности (#54): одно соединение на процесс, все операции (записи через transaction() и чтения через read()) — под одним RLock. Прямой доступ к соединению (connection/raw()) — только для однопоточного кода.
  • Путь: аргумент → env AWG_DASH_DB → /data/awg-dash.db; каталог создаётся автоматически (пустой /data — ок).
  • Схема v1: routing_rules, rule_exceptions, routing_mode, geoip_schedule, configs, devices, audit_log, state_snapshots, schema_migrations.

Версионированные миграции + проверка схемы при старте — migrations.py

  • Список Migration(version, name, statements); каждая версия применяется в своей транзакции вместе с записью в schema_migrations → падение миграции не оставляет «полуприменённой» схемы.
  • verify_schema() при старте: проверяет версию (новее ожидаемой → SchemaError, не мигрирована → SchemaError), наличие таблиц и обязательных колонок. Откат схемы не автоматизируется намеренно (ADR-006).

Аудит «кто/когда/что/old→new» на все мутации — audit.py + repository.py

  • Store: upsert_rule/delete_rule, upsert_exception/delete_exception, set_mode, set_geoip_schedule, upsert_config/mark_config_applied, register_device/delete_device; каждая мутация и её запись в audit_log — одна транзакция (не могут разойтись).
  • Запись содержит ts/actor/action/entity/entity_id/old_value/new_value/status/detail; old/new — JSON. Пустой actor — ошибка.
  • old_value читается внутри той же транзакции (#55 D-2): конкурентные обновления не рвут цепочку old→new.

Атомарное validate → stage → apply → rollback — apply.py, state.py, validation.py

  • validate — чистые функции (domain/CIDR/ipset, MAC, cron, режим, политика, приоритет, пути конфигов), не трогают БД.
  • stage — снимок текущего состояния в state_snapshots (durable) + запись аудита staged.
  • apply — мутация БД, вызов внешнего Applier (awg/iptables) и запись аудита ok в одной транзакции; падение применителя → rollback транзакции, Applier.revert(previous), запись аудита failed и ApplyError(restored=True).
  • apply ловит BaseException (#55 D-1): asyncio.CancelledError/KeyboardInterrupt/SystemExit тоже дают revert и failed в журнале — «висящих» staged не остаётся; падение самого revert попадает в detail.
  • rollback(plan_id) — явный возврат к снимку + запись аудита.

Acceptance criteria (#20)

AC Статус Проверка
Пустой /data → миграции и схема v1 без ручных действий ✅ test_empty_data_dir_creates_schema, test_seed_rows_present
Мутация пишет в audit_log old/new и время ✅ test_audit_actor_recorded_per_mutation, test_upsert_rule_updates_by_name_and_records_old_new, test_no_audit_row_when_transaction_rolls_back
Ошибочный apply (мок падения iptables) → предыдущее состояние + ошибка ✅ test_failed_apply_restores_previous_state, test_failed_apply_with_failing_revert_still_restores_db, test_read_during_failing_apply_never_sees_discarded_state
Рестарт контейнера → правила/конфиг на месте ✅ test_state_survives_container_restart, test_wal_file_next_to_database

Раунд 2 — правки по ревью QA (#54, #55)

Head: c4661c9db97ce4ed2c2c3471af06b18b69d99162.

Замечание Что сделано Тест
#54 (critical): незащищённые чтения на общем соединении → TypeError/IndexError, грязные чтения, выдача откаченного состояния Выбран вариант 1: Database.read() берёт тот же RLock; все 12 методов чтения Store переведены на него; counts() — один снимок на все таблицы; контракт задокументирован в db.py и store/README.md test_concurrent_upserts_and_reads_are_consistent, test_reads_never_see_uncommitted_rows, test_read_during_failing_apply_never_sees_discarded_state
#55 D-1: except Exception не покрывает BaseException → нет revert и failed в аудите apply ловит BaseException: cleanup выполняется всегда, Exception → ApplyError, прочие пробрасываются как есть test_base_exception_from_applier_reverts_and_records_failure[KeyboardInterrupt/SystemExit/CancelledError]
#55 D-2: TOCTOU в set_geoip_schedule чтение old_value перенесено внутрь транзакции test_concurrent_geoip_updates_keep_audit_chain_consistent

На коде до правки все 7 новых тестов падают (воспроизведено: InterfaceError: bad parameter or other API misuse, reader_saw_rules=['baseline','uncommitted'], reader_saw_rules=['new'] при ApplyError(restored=True), reverted == []); после правки — проходят.

Проверка (раунд 2, реальный прогон)

ruff check backend            -> All checks passed!          (exit 0)
ruff format --check backend   -> 13 files already formatted  (exit 0)
mypy backend                  -> Success: 21 source files (дерево с файлами #47)
mypy backend --explicit-package-bases -> Success: 18 source files (PR-only дерево)
pytest (PR-only дерево)       -> 105 passed, coverage 97.79 % (порог 70 %)
pytest (дерево с файлами #47) -> 107 passed, coverage 97.80 %

Покрытие: state/service/models/errors/clock/__init__ 100 %, validation 99 %, apply/audit/db 98 %, repository 97 %, migrations 94 %. Новые строки db.py/repository.py/apply.py покрыты регрессионными тестами.

Пробы QA из ревью, прогнанные против исправленной ветки: probe_qa.py — 40/40 pass (было 39/40, падал H2); probe_conc.py — writes_plus_reads: 100/100 rules, 100/100 audit, 0 ошибок; незаблокированное чтение во время открытой транзакции больше не возвращает данные; probe_conc2.py — 5/5 прогонов без крашей и потерь, читатель во время падающего apply видит ['keep'] (прежнее состояние), final_rules=['keep'].

Зависимости и известные ограничения

  1. CI: pipeline появится в #17, в main его пока нет — гейт «CI green» на этой ветке технически недостижим до #17. Локальный полный гейт прогнан и приложен выше.
  2. Каркас (#16, PR #47 не влит): в ветке нет pyproject.toml (конфиг ruff/mypy/pytest) — он приходит из #47. На PR-only дереве mypy backend падает на N-1 (Source file found twice..., нет backend/awg_api/__init__.py), типовая чистка при этом подтверждена --explicit-package-bases; в дереве с файлами #47 — чисто. Порядок влития: #47 → #17 → #50.
  3. RUF001/RUF002: правила срабатывают на кириллических токенах целиком из «конфузных» букв (отдельное с, а, Все, его, ОС). Код проходит ruff check под неизменённым конфигом из #16.
  4. Аудит ретеншен: audit_log растёт неограниченно — очистка/ротация вынесена в #28 (ADR-006).

Гейты

  • Тесты: 107 passed (105 на PR-only дереве), покрытие 97.79–97.80 % ≥ 70 %
  • Lint/mypy: ruff check + ruff format чисто; mypy strict чисто (18/21 файлов)
  • CI green — зависит от #17
  • Ревью: раунд 2 после CHANGES REQUESTED; в Gitea запрос ревьюера технически невозможен (единственный пользователь — автор PR), ревью через Kanban t_2dec61bb (qa-engineer)

Все правки — через Gitea MCP. Содержимое ветки совпадает с проверенным локально деревом по git-blob SHA: 4 изменённых файла сверены по sha из Gitea, новый тест — по sha (19 файлов в PR, +3692/−0).

Refs #20, Refs #15. Refs #54, Refs #55. Реализует слой хранения состояния из issue #20 целиком: `backend/awg_api/store/` (`#20`, карта модулей из `backend/awg_api/README.md`) + тесты. ## Что сделано **SQLite (WAL) в `/data/awg-dash.db`** — `db.py` - `Database`: PRAGMA `journal_mode=WAL`, `synchronous=NORMAL`, `foreign_keys=ON`, `busy_timeout=5000`; `BEGIN IMMEDIATE` и rollback при любом исключении. - **Контракт потокобезопасности (#54)**: одно соединение на процесс, **все** операции (записи через `transaction()` и чтения через `read()`) — под одним `RLock`. Прямой доступ к соединению (`connection`/`raw()`) — только для однопоточного кода. - Путь: аргумент → env `AWG_DASH_DB` → `/data/awg-dash.db`; каталог создаётся автоматически (пустой `/data` — ок). - Схема v1: `routing_rules`, `rule_exceptions`, `routing_mode`, `geoip_schedule`, `configs`, `devices`, `audit_log`, `state_snapshots`, `schema_migrations`. **Версионированные миграции + проверка схемы при старте** — `migrations.py` - Список `Migration(version, name, statements)`; каждая версия применяется в своей транзакции вместе с записью в `schema_migrations` → падение миграции не оставляет «полуприменённой» схемы. - `verify_schema()` при старте: проверяет версию (новее ожидаемой → `SchemaError`, не мигрирована → `SchemaError`), наличие таблиц и обязательных колонок. Откат схемы не автоматизируется намеренно (ADR-006). **Аудит «кто/когда/что/old→new» на все мутации** — `audit.py` + `repository.py` - `Store`: `upsert_rule/delete_rule`, `upsert_exception/delete_exception`, `set_mode`, `set_geoip_schedule`, `upsert_config/mark_config_applied`, `register_device/delete_device`; каждая мутация и её запись в `audit_log` — одна транзакция (не могут разойтись). - Запись содержит `ts/actor/action/entity/entity_id/old_value/new_value/status/detail`; `old/new` — JSON. Пустой `actor` — ошибка. - `old_value` читается **внутри** той же транзакции (#55 D-2): конкурентные обновления не рвут цепочку `old→new`. **Атомарное `validate → stage → apply → rollback`** — `apply.py`, `state.py`, `validation.py` - `validate` — чистые функции (domain/CIDR/ipset, MAC, cron, режим, политика, приоритет, пути конфигов), не трогают БД. - `stage` — снимок текущего состояния в `state_snapshots` (durable) + запись аудита `staged`. - `apply` — мутация БД, вызов внешнего `Applier` (awg/iptables) и запись аудита `ok` в одной транзакции; падение применителя → rollback транзакции, `Applier.revert(previous)`, запись аудита `failed` и `ApplyError(restored=True)`. - `apply` ловит `BaseException` (#55 D-1): `asyncio.CancelledError`/`KeyboardInterrupt`/`SystemExit` тоже дают `revert` и `failed` в журнале — «висящих» `staged` не остаётся; падение самого `revert` попадает в `detail`. - `rollback(plan_id)` — явный возврат к снимку + запись аудита. ## Acceptance criteria (#20) | AC | Статус | Проверка | |---|---|---| | Пустой `/data` → миграции и схема v1 без ручных действий | ✅ | `test_empty_data_dir_creates_schema`, `test_seed_rows_present` | | Мутация пишет в `audit_log` old/new и время | ✅ | `test_audit_actor_recorded_per_mutation`, `test_upsert_rule_updates_by_name_and_records_old_new`, `test_no_audit_row_when_transaction_rolls_back` | | Ошибочный apply (мок падения iptables) → предыдущее состояние + ошибка | ✅ | `test_failed_apply_restores_previous_state`, `test_failed_apply_with_failing_revert_still_restores_db`, `test_read_during_failing_apply_never_sees_discarded_state` | | Рестарт контейнера → правила/конфиг на месте | ✅ | `test_state_survives_container_restart`, `test_wal_file_next_to_database` | ## Раунд 2 — правки по ревью QA (#54, #55) Head: `c4661c9db97ce4ed2c2c3471af06b18b69d99162`. | Замечание | Что сделано | Тест | |---|---|---| | **#54** (critical): незащищённые чтения на общем соединении → `TypeError`/`IndexError`, грязные чтения, выдача откаченного состояния | Выбран вариант 1: `Database.read()` берёт тот же `RLock`; все 12 методов чтения `Store` переведены на него; `counts()` — один снимок на все таблицы; контракт задокументирован в `db.py` и `store/README.md` | `test_concurrent_upserts_and_reads_are_consistent`, `test_reads_never_see_uncommitted_rows`, `test_read_during_failing_apply_never_sees_discarded_state` | | **#55 D-1**: `except Exception` не покрывает `BaseException` → нет `revert` и `failed` в аудите | `apply` ловит `BaseException`: cleanup выполняется всегда, `Exception` → `ApplyError`, прочие пробрасываются как есть | `test_base_exception_from_applier_reverts_and_records_failure[KeyboardInterrupt/SystemExit/CancelledError]` | | **#55 D-2**: TOCTOU в `set_geoip_schedule` | чтение `old_value` перенесено внутрь транзакции | `test_concurrent_geoip_updates_keep_audit_chain_consistent` | На коде до правки все 7 новых тестов падают (воспроизведено: `InterfaceError: bad parameter or other API misuse`, `reader_saw_rules=['baseline','uncommitted']`, `reader_saw_rules=['new']` при `ApplyError(restored=True)`, `reverted == []`); после правки — проходят. ## Проверка (раунд 2, реальный прогон) ``` ruff check backend -> All checks passed! (exit 0) ruff format --check backend -> 13 files already formatted (exit 0) mypy backend -> Success: 21 source files (дерево с файлами #47) mypy backend --explicit-package-bases -> Success: 18 source files (PR-only дерево) pytest (PR-only дерево) -> 105 passed, coverage 97.79 % (порог 70 %) pytest (дерево с файлами #47) -> 107 passed, coverage 97.80 % ``` Покрытие: `state/service/models/errors/clock/__init__` 100 %, `validation` 99 %, `apply`/`audit`/`db` 98 %, `repository` 97 %, `migrations` 94 %. Новые строки `db.py`/`repository.py`/`apply.py` покрыты регрессионными тестами. **Пробы QA из ревью, прогнанные против исправленной ветки:** `probe_qa.py` — 40/40 pass (было 39/40, падал H2); `probe_conc.py` — `writes_plus_reads`: 100/100 rules, 100/100 audit, 0 ошибок; незаблокированное чтение во время открытой транзакции больше не возвращает данные; `probe_conc2.py` — 5/5 прогонов без крашей и потерь, читатель во время падающего apply видит `['keep']` (прежнее состояние), `final_rules=['keep']`. ## Зависимости и известные ограничения 1. **CI**: pipeline появится в #17, в `main` его пока нет — гейт «CI green» на этой ветке технически недостижим до #17. Локальный полный гейт прогнан и приложен выше. 2. **Каркас (#16, PR #47 не влит)**: в ветке нет `pyproject.toml` (конфиг ruff/mypy/pytest) — он приходит из #47. На PR-only дереве `mypy backend` падает на N-1 (`Source file found twice...`, нет `backend/awg_api/__init__.py`), типовая чистка при этом подтверждена `--explicit-package-bases`; в дереве с файлами #47 — чисто. Порядок влития: #47 → #17 → #50. 3. **RUF001/RUF002**: правила срабатывают на кириллических токенах целиком из «конфузных» букв (отдельное `с`, `а`, `Все`, `его`, `ОС`). Код проходит `ruff check` под неизменённым конфигом из #16. 4. **Аудит ретеншен**: `audit_log` растёт неограниченно — очистка/ротация вынесена в #28 (ADR-006). ## Гейты - [x] Тесты: 107 passed (105 на PR-only дереве), покрытие 97.79–97.80 % ≥ 70 % - [x] Lint/mypy: ruff check + ruff format чисто; mypy strict чисто (18/21 файлов) - [ ] CI green — зависит от #17 - [ ] Ревью: раунд 2 после CHANGES REQUESTED; в Gitea запрос ревьюера технически невозможен (единственный пользователь — автор PR), ревью через Kanban t_2dec61bb (qa-engineer) Все правки — через Gitea MCP. Содержимое ветки совпадает с проверенным локально деревом по git-blob SHA: 4 изменённых файла сверены по `sha` из Gitea, новый тест — по `sha` (19 файлов в PR, +3692/−0).
embeddevio added 6 commits 2026-10-21 23:46:05 +03:00
embeddevio added the backendstatus/review labels 2026-10-21 23:46:13 +03:00
Author
Owner

Backend Engineer — заметки к ревью PR #50.

Воспроизведение полного гейта (до влития #16/#17): взять ветку feat/20-storage, локально подложить pyproject.toml/requirements*.txt из feat/16-repo-skeleton (PR #47), затем ruff check backend && ruff format --check backend && mypy backend && pytest → локально: All checks passed, 15 files formatted, no issues in 20 sources, 100 passed / coverage 97.95 %.

Пункты для владельца роли

  1. Порядок влития: #47 (#16, каркас) → #17 (CI) → этот PR; без #17 гейт «CI green» нечего запускать, без #47 в ветке нет конфигов линтера/тестов.
  2. RUF001/RUF002 (включены через RUF в конфиге #16) срабатывают только на кириллических токенах, целиком состоящих из конфузных букв: одиночные а/с, слова Все, его, ОС. Прошлое утверждение автора, что это ломает модуль backend/awg_agent/__init__.py из #16, неверно — проверено: ruff check backend/awg_agent/__init__.py → All checks passed. Правка #16 не нужна, рекомендация снята; практический вывод для команды — избегать таких токенов в русских docstring.
  3. audit_log не ротируется — осознанно вынесено в #28.
  4. Applier — Protocol (apply/revert); реальный iptables/awg-применитель подключается в агентской части (#21/#18), NullApplier — dry-run.
  5. Запрос ревьюера в Gitea добавить нельзя: единственный пользователь embeddevio — автор PR («poster of pr can't be reviewer»). Ревью — через Kanban, карточка t_2dec61bb.
Backend Engineer — заметки к ревью PR #50. **Воспроизведение полного гейта** (до влития #16/#17): взять ветку `feat/20-storage`, локально подложить `pyproject.toml`/`requirements*.txt` из `feat/16-repo-skeleton` (PR #47), затем `ruff check backend && ruff format --check backend && mypy backend && pytest` → локально: All checks passed, 15 files formatted, no issues in 20 sources, 100 passed / coverage 97.95 %. **Пункты для владельца роли** 1. Порядок влития: #47 (#16, каркас) → #17 (CI) → этот PR; без #17 гейт «CI green» нечего запускать, без #47 в ветке нет конфигов линтера/тестов. 2. `RUF001`/`RUF002` (включены через `RUF` в конфиге #16) срабатывают только на кириллических токенах, целиком состоящих из конфузных букв: одиночные `а`/`с`, слова `Все`, `его`, `ОС`. Прошлое утверждение автора, что это ломает модуль `backend/awg_agent/__init__.py` из #16, **неверно** — проверено: `ruff check backend/awg_agent/__init__.py` → All checks passed. Правка #16 не нужна, рекомендация снята; практический вывод для команды — избегать таких токенов в русских docstring. 3. `audit_log` не ротируется — осознанно вынесено в #28. 4. `Applier` — Protocol (`apply`/`revert`); реальный iptables/awg-применитель подключается в агентской части (#21/#18), `NullApplier` — dry-run. 5. Запрос ревьюера в Gitea добавить нельзя: единственный пользователь `embeddevio` — автор PR («poster of pr can't be reviewer»). Ревью — через Kanban, карточка t_2dec61bb.
Author
Owner

QA / ревью-ран — t_2dec61bb: FAIL (changes requested), PR помечен blocking

Ревью проведено на отпечатке ветки, идентичном опубликованному: head 7e10ffaf…, blob SHA 18/18 совпадают (по index-строкам диффа PR ↔ локальный git hash-object). Прогон — Python 3.13.5 / ruff 0.17.1 / mypy 2.4.0 / pytest 9.1.1.

Гейты — воспроизведены (независимо)

Гейт Дерево с файлами #16 (как у автора) Только файлы PR #50 + конфиг #16
ruff check backend All checks passed All checks passed
ruff format --check 15 files formatted 13 files formatted
mypy backend (strict) Success: 20 файлов exit 2 — 1 ошибка (store vs awg_api.store: в ветке нет backend/awg_api/__init__.py из #47)
pytest 100 passed, 97.95 % 98 passed, 97.94 % (827 stmt)

Из «100 passed» 98 тестов — собственные тесты PR, 2 — test_skeleton.py из #16. AC-тесты проверены по содержанию: реальные ассерты поведения (сверка capture_state до/после, mock.reverted == [before], запись failed), тавтологий нет.

Собственный набор проб: 40 проверок — 39 pass / 1 FAIL

AC1 (пустой /data → схема v1 + WAL), AC2 (аудит на все 9 типов мутаций, old→new, одна транзакция), AC3 однопоточно (падение применителя → ApplyError(restored=True), состояние байт-в-байт прежнее, revert вызван, падение revert тоже восстанавливает), AC4 (переоткрытие → правила/конфиг/аудит на месте), schema-guard (новее/немиграция/нет колонки/нет таблицы → SchemaError), SQL-инъекции и path traversal (заблокированы) — всё PASS.

FAIL — конкурентный доступ (блокирует merge):

  • 4 потока × 25 upsert без чтений: rules=100/100, audit=100/100, 0 ошибок — запись сериализована корректно.
  • 4 потока × 25 upsert + чтения list_rules()/get_mode(): rules=94/100, errors=1 → TypeError: int() ... not 'NoneType' в repository._rule_from_row; в 5× серии 3/5 прогонов упали (потеряно 4–38 мутаций).
  • Детерминированно: чтение на том же соединении видит незафиксированную строку другого потока (["baseline", "uncommitted"]).
  • AC3 нарушается при конкуренции: во время падающего apply читатель видит ["new"], после ApplyError(restored=True) фактически ["keep"] — наружу отдаётся конфигурация, которая не была применена и была откачена.

Причина: Database._lock берётся только в transaction()/migrate()/close(); все чтения Store используют общий connection без блокировки при check_same_thread=False. sqlite3.threadsafety=3 защищает память, но не даёт изоляции. Это проявится как только появится API с threadpool (#19/#21).

Баги с шагами воспроизведения

  • #54 — critical / blocker: незащищённые чтения на общем соединении (3 воспроизведения, скрипты приложены).
  • #55: except Exception вокруг apply не покрывает BaseException (нет revert и failed-записи в аудите; в аудите остаётся staged без исхода) + TOCTOU в set_geoip_schedule.

Что должно попасть в повторное ревью

  1. Правка конкурентного доступа (вариант — на выбор владельца: лок на чтения / соединение на поток / явный запрет многопоточности) + тест N потоков × M upsert со сверкой rules == audit_rows == N*M.
  2. Тест: чтение во время открытой транзакции другого потока не видит незафиксированных строк.
  3. BaseException/CancelledError из Applier → revert вызван и в аудите есть failed.
  4. Чтение в set_geoip_schedule внутри транзакции.

Merge только после повторного прогона гейта на новом head-SHA. Полный отчёт: qa_report_pr50.md (приложен к карточке t_2dec61bb). Гейт «CI green» по-прежнему недостижим без #47/#17 — это не блокирует данные правки, но фиксирую как открытый пункт DoD.

Ревью оформлено комментарием, а не REQUEST_CHANGES в Gitea: единственный пользователь — автор PR (self-review API отклоняет).

## QA / ревью-ран — t_2dec61bb: **FAIL (changes requested), PR помечен blocking** Ревью проведено на отпечатке ветки, идентичном опубликованному: head `7e10ffaf…`, blob SHA **18/18 совпадают** (по `index`-строкам диффа PR ↔ локальный `git hash-object`). Прогон — Python 3.13.5 / ruff 0.17.1 / mypy 2.4.0 / pytest 9.1.1. ### Гейты — воспроизведены (независимо) | Гейт | Дерево с файлами #16 (как у автора) | Только файлы PR #50 + конфиг #16 | |---|---|---| | `ruff check backend` | All checks passed | All checks passed | | `ruff format --check` | 15 files formatted | 13 files formatted | | `mypy backend` (strict) | Success: 20 файлов | **exit 2 — 1 ошибка** (`store` vs `awg_api.store`: в ветке нет `backend/awg_api/__init__.py` из #47) | | `pytest` | 100 passed, **97.95 %** | **98 passed**, **97.94 %** (827 stmt) | Из «100 passed» 98 тестов — собственные тесты PR, 2 — `test_skeleton.py` из #16. AC-тесты проверены по содержанию: реальные ассерты поведения (сверка `capture_state` до/после, `mock.reverted == [before]`, запись `failed`), тавтологий нет. ### Собственный набор проб: 40 проверок — 39 pass / 1 FAIL AC1 (пустой `/data` → схема v1 + WAL), AC2 (аудит на все 9 типов мутаций, old→new, одна транзакция), AC3 однопоточно (падение применителя → `ApplyError(restored=True)`, состояние байт-в-байт прежнее, `revert` вызван, падение `revert` тоже восстанавливает), AC4 (переоткрытие → правила/конфиг/аудит на месте), schema-guard (новее/немиграция/нет колонки/нет таблицы → `SchemaError`), SQL-инъекции и path traversal (заблокированы) — **всё PASS**. **FAIL — конкурентный доступ (блокирует merge):** - `4 потока × 25 upsert` **без чтений**: `rules=100/100`, `audit=100/100`, 0 ошибок — запись сериализована корректно. - `4 потока × 25 upsert` **+ чтения** `list_rules()/get_mode()`: `rules=94/100`, `errors=1` → `TypeError: int() ... not 'NoneType'` в `repository._rule_from_row`; в 5× серии **3/5 прогонов** упали (потеряно 4–38 мутаций). - Детерминированно: чтение на том же соединении видит **незафиксированную** строку другого потока (`["baseline", "uncommitted"]`). - AC3 нарушается при конкуренции: во время падающего `apply` читатель видит `["new"]`, после `ApplyError(restored=True)` фактически `["keep"]` — наружу отдаётся конфигурация, которая не была применена и была откачена. Причина: `Database._lock` берётся только в `transaction()`/`migrate()`/`close()`; все чтения `Store` используют общий `connection` без блокировки при `check_same_thread=False`. `sqlite3.threadsafety=3` защищает память, но не даёт изоляции. Это проявится как только появится API с threadpool (#19/#21). ### Баги с шагами воспроизведения - **#54 — critical / blocker:** незащищённые чтения на общем соединении (3 воспроизведения, скрипты приложены). - **#55:** `except Exception` вокруг `apply` не покрывает `BaseException` (нет `revert` и `failed`-записи в аудите; в аудите остаётся `staged` без исхода) + TOCTOU в `set_geoip_schedule`. ### Что должно попасть в повторное ревью 1. Правка конкурентного доступа (вариант — на выбор владельца: лок на чтения / соединение на поток / явный запрет многопоточности) + тест N потоков × M upsert со сверкой `rules == audit_rows == N*M`. 2. Тест: чтение во время открытой транзакции другого потока не видит незафиксированных строк. 3. `BaseException`/`CancelledError` из `Applier` → `revert` вызван и в аудите есть `failed`. 4. Чтение в `set_geoip_schedule` внутри транзакции. Merge только после повторного прогона гейта на новом head-SHA. Полный отчёт: `qa_report_pr50.md` (приложен к карточке t_2dec61bb). Гейт «CI green» по-прежнему недостижим без #47/#17 — это не блокирует данные правки, но фиксирую как открытый пункт DoD. _Ревью оформлено комментарием, а не `REQUEST_CHANGES` в Gitea: единственный пользователь — автор PR (self-review API отклоняет)._
embeddevio added qablockerstatus/blocked and removed status/review labels 2026-10-22 00:20:51 +03:00
embeddevio added status/in-progress and removed qablockerstatus/blocked labels 2026-10-28 20:57:08 +03:00
embeddevio added 1 commit 2026-10-28 21:12:11 +03:00
Закрывает замечания ревью QA по PR #50:

* #54: все чтения Store идут через Database.read() под тем же RLock, что
  транзакции; контракт «одно соединение, все операции под локом»
  задокументирован в db.py и README модуля. Устраняет TypeError/IndexError
  на общем соединении, грязные чтения и выдачу состояния, которое затем
  откатывается падающим apply.
* #55 D-1: apply ловит BaseException — отмена/CancelledError/
  KeyboardInterrupt вызывают Applier.revert(previous) и пишут failed в
  audit_log (без «висящей» записи staged); падение revert попадает в detail.
* #55 D-2: set_geoip_schedule читает old_value внутри той же транзакции.

Refs #20, Refs #54, Refs #55
embeddevio added 1 commit 2026-10-28 21:12:36 +03:00
Новый файл backend/tests/test_store_concurrency.py:

* N потоков × M upsert с чтениями → 0 ошибок, rules == audit_rows == N*M;
* чтение во время открытой транзакции другого потока не видит незафиксированных
  строк (ждёт коммита);
* чтение во время падающего apply не видит состояния, которое будет откачено
  (AC «не частично применённая конфигурация»);
* BaseException (KeyboardInterrupt/SystemExit/CancelledError) из Applier →
  revert вызван, failed в audit_log, БД восстановлена;
* два конкурентных set_geoip_schedule → связная цепочка old→new в журнале.

На коде до правки все 7 тестов падают; после — проходят.

Refs #20, Refs #54, Refs #55
Author
Owner

Раунд 2 — правки по ревью, новый head c4661c9d

Закрыты #54 и #55; гейт прогнан заново на новом отпечатке ветки.

Что изменено

Замечание Правка Файл
#54 незащищённые чтения на общем соединении Добавлен Database.read() — тот же RLock, что у transaction(); все чтения Store (12 методов) переведены на него, counts() — один снимок на все таблицы; journal_mode/schema_version/verify_schema тоже под локом. Выбран вариант 1 из #54 (сериализация чтений). Контракт задокументирован: «одно соединение, все операции под локом; прямой connection/raw() — только однопоточный код» db.py, repository.py, store/README.md
#55 D-1 BaseException не покрыт apply ловит BaseException: revert(previous) вызывается всегда, failed пишется в audit_log; Exception → ApplyError(restored=…), CancelledError/KeyboardInterrupt/SystemExit пробрасываются как есть (семантика отмены сохранена); падение revert попадает в detail apply.py
#55 D-2 TOCTOU в set_geoip_schedule old_value читается внутри той же транзакции, что и INSERT … ON CONFLICT repository.py

Также _last_failed_audit_id и сверка состояния после отката читают через read().

Новые тесты (backend/tests/test_store_concurrency.py, 7 тестов)

  • test_concurrent_upserts_and_reads_are_consistent — 4 потока × 25 upsert + list_rules/get_mode/get_rule_by_name → 0 ошибок, rules == audit_rows == 100;
  • test_reads_never_see_uncommitted_rows — при открытой транзакции другого потока чтение ждёт коммита, незафиксированная строка не видна;
  • test_read_during_failing_apply_never_sees_discarded_state — во время падающего apply снаружи не видно «нового» состояния, после ApplyError(restored=True) — прежнее (AC «не частично применённая конфигурация»);
  • test_base_exception_from_applier_reverts_and_records_failure[3 варианта] — KeyboardInterrupt/SystemExit/asyncio.CancelledError: revert вызван 1× с прежним payload, аудит failed, БД восстановлена;
  • test_concurrent_geoip_updates_keep_audit_chain_consistent — два конкурентных set_geoip_schedule → связная цепочка old→new, финал совпадает с состоянием.

На 7e10ffaf все 7 падают (реальные сообщения: InterfaceError: bad parameter or other API misuse, ['baseline','uncommitted'], ['new'], reverted == [], разрыв geoip-цепочки); на c4661c9d — проходят (10 прогонов подряд без флапов).

Гейт (Python 3.13.5, ruff 0.17.1, mypy 2.4.0, pytest 9.1.1)

ruff check backend                       -> All checks passed!            exit 0
ruff format --check backend              -> 13 files already formatted    exit 0
mypy backend (дерево с файлами #47)      -> Success: 21 source files      exit 0
mypy backend --explicit-package-bases    -> Success: 18 source files      exit 0
pytest (PR-only дерево)                  -> 105 passed, coverage 97.79 %
pytest (дерево с файлами #47)            -> 107 passed, coverage 97.80 %

Пробы из ревью против исправленной ветки: probe_qa.py — 40/40 pass (было 39/40, падал H2); probe_conc.py — expected=100 rules_in_db=100 rule_audit_rows=100 errors=0 (writes+reads), читатель во время открытой транзакции результатов не получает; probe_conc2.py — runs_with_worker_crash=0, runs_with_silent_loss=0 на 5/5 прогонов с чтениями, читатель во время падающего apply видит ['keep'].

Идентичность артефакта

Содержимое ветки сверено с деревом, на котором прогонялся гейт, по git-blob SHA (Gitea sha ↔ git hash-object): db.py bf03f2d0, repository.py a21dcb2b, apply.py 3ecc2fe1, store/README.md 631e124e, новый test_store_concurrency.py 1b879cf4 — 5/5; остальные 14 файлов не менялись (размеры совпадают с 7e10ffaf).

## Раунд 2 — правки по ревью, новый head `c4661c9d` Закрыты #54 и #55; гейт прогнан заново на новом отпечатке ветки. ### Что изменено | Замечание | Правка | Файл | |---|---|---| | **#54** незащищённые чтения на общем соединении | Добавлен `Database.read()` — тот же `RLock`, что у `transaction()`; все чтения `Store` (12 методов) переведены на него, `counts()` — один снимок на все таблицы; `journal_mode`/`schema_version`/`verify_schema` тоже под локом. Выбран **вариант 1** из #54 (сериализация чтений). Контракт задокументирован: «одно соединение, все операции под локом; прямой `connection`/`raw()` — только однопоточный код» | `db.py`, `repository.py`, `store/README.md` | | **#55 D-1** `BaseException` не покрыт | `apply` ловит `BaseException`: `revert(previous)` вызывается всегда, `failed` пишется в `audit_log`; `Exception` → `ApplyError(restored=…)`, `CancelledError`/`KeyboardInterrupt`/`SystemExit` пробрасываются как есть (семантика отмены сохранена); падение `revert` попадает в `detail` | `apply.py` | | **#55 D-2** TOCTOU в `set_geoip_schedule` | `old_value` читается внутри той же транзакции, что и `INSERT … ON CONFLICT` | `repository.py` | Также `_last_failed_audit_id` и сверка состояния после отката читают через `read()`. ### Новые тесты (`backend/tests/test_store_concurrency.py`, 7 тестов) - `test_concurrent_upserts_and_reads_are_consistent` — 4 потока × 25 upsert + `list_rules`/`get_mode`/`get_rule_by_name` → 0 ошибок, `rules == audit_rows == 100`; - `test_reads_never_see_uncommitted_rows` — при открытой транзакции другого потока чтение ждёт коммита, незафиксированная строка не видна; - `test_read_during_failing_apply_never_sees_discarded_state` — во время падающего apply снаружи не видно «нового» состояния, после `ApplyError(restored=True)` — прежнее (AC «не частично применённая конфигурация»); - `test_base_exception_from_applier_reverts_and_records_failure[3 варианта]` — `KeyboardInterrupt`/`SystemExit`/`asyncio.CancelledError`: `revert` вызван 1× с прежним payload, аудит `failed`, БД восстановлена; - `test_concurrent_geoip_updates_keep_audit_chain_consistent` — два конкурентных `set_geoip_schedule` → связная цепочка `old→new`, финал совпадает с состоянием. На `7e10ffaf` все 7 падают (реальные сообщения: `InterfaceError: bad parameter or other API misuse`, `['baseline','uncommitted']`, `['new']`, `reverted == []`, разрыв geoip-цепочки); на `c4661c9d` — проходят (10 прогонов подряд без флапов). ### Гейт (Python 3.13.5, ruff 0.17.1, mypy 2.4.0, pytest 9.1.1) ``` ruff check backend -> All checks passed! exit 0 ruff format --check backend -> 13 files already formatted exit 0 mypy backend (дерево с файлами #47) -> Success: 21 source files exit 0 mypy backend --explicit-package-bases -> Success: 18 source files exit 0 pytest (PR-only дерево) -> 105 passed, coverage 97.79 % pytest (дерево с файлами #47) -> 107 passed, coverage 97.80 % ``` Пробы из ревью против исправленной ветки: `probe_qa.py` — **40/40 pass** (было 39/40, падал H2); `probe_conc.py` — `expected=100 rules_in_db=100 rule_audit_rows=100 errors=0` (writes+reads), читатель во время открытой транзакции результатов не получает; `probe_conc2.py` — `runs_with_worker_crash=0, runs_with_silent_loss=0` на 5/5 прогонов с чтениями, читатель во время падающего apply видит `['keep']`. ### Идентичность артефакта Содержимое ветки сверено с деревом, на котором прогонялся гейт, по git-blob SHA (Gitea `sha` ↔ `git hash-object`): `db.py` `bf03f2d0`, `repository.py` `a21dcb2b`, `apply.py` `3ecc2fe1`, `store/README.md` `631e124e`, новый `test_store_concurrency.py` `1b879cf4` — 5/5; остальные 14 файлов не менялись (размеры совпадают с `7e10ffaf`).
embeddevio added status/review and removed status/in-progress labels 2026-10-28 21:14:25 +03:00
Author
Owner

QA-ревью раунд 2 — вердикт: APPROVE (head c4661c9d)

Линза раунда 2 — execution: самостоятельный checkout артефакта и прогон, не перечитывание handoff'а. Ничего не мержено, в main не пушилось (ADVISORY).

Идентичность артефакта

Архив ветки, отданный Gitea по точному head-SHA c4661c9d (git fetch по HTTP в этой сети падает: bad pack header), сверен с API: размеры 19/19 файлов = get_dir_contents, blob-префиксы 5 изменённых файлов = заявленные (bf03f2d0, a21dcb2b, 3ecc2fe1, 631e124e, 1b879cf4).

Гейт на реальном merge-состоянии (main 063ecf18, #16 уже влит, + 19 файлов PR)

ruff check backend            -> All checks passed!            exit 0
ruff format --check backend   -> 17 files already formatted    exit 0
mypy backend (strict)         -> Success: 21 source files      exit 0
pytest                        -> 107 passed, coverage 97.80 %  (порог 70 %)

PR-only дерево: ruff/format чисто, mypy --explicit-package-bases 18 файлов чисто, 105 passed / 97.79 %; mypy без флага — N-1 из #16, PR его не вносит. Флейки: concurrency-файл 10/10, полный набор 3/3 зелёные.

Требования раунда 1 — закрыты

  • #54: Database.read() под общим RLock (db.py:152-162), все 12 чтений Store на read(), counts() — один снимок; контракт в db.py и store/README.md.
  • #55 D-1: apply() ловит BaseException, revert(previous) вызывается всегда, failed в аудите, Exception → ApplyError, остальные пробрасываются.
  • #55 D-2: old_value читается внутри транзакции (repository.py:373-391).
  • Обязательные тесты присутствуют все 4.

Независимые пробы QA (написаны по AC, не из тестов PR): 55/55 PASS

AC1 (пустой /data → WAL + схема + все таблицы), AC2 (аудит на мутации, откат не оставляет следа), AC3 (ApplyError(restored=True), revert с previous, БД побайтово прежняя, failed + audit_id), AC4 (рестарт), schema-guard (SchemaError), #54 6 потоков × 30 upsert × 5 прогонов → rules == audit == 180, 0 ошибок; нет грязных чтений (ждал ≥0.3 с); во время падающего apply снаружи ['keep']; #55 D-1 KeyboardInterrupt/SystemExit/CancelledError → тип сохранён, revert вызван, failed в аудите, «висящих» staged нет; #55 D-2 4 конкурентных обновления → цепочка old→new связна.

Мутационная проверка тестов (anti-tautology)

Откат трёх фиксов в копии дерева → новый файл тестов падает: 6/7 в первом прогоне, 7/7 во втором (TypeError/InterfaceError, ['baseline','uncommitted'], ['new'] при restored=True, reverted == [] ×3, разрыв цепочки geoip). Тесты проверяют поведение. Оговорка: geoip-тест в RED-состоянии срабатывает не всегда (узкая гонка), на исправленном коде 10/10 зелёный.

Безопасность: 28/28 PASS

11 инъекционных payload'ов × 6 полей → все ValidationError; таблицы и audit_log целы; пустой actor отклонён; хардкода секретов нет.

Неблокирующие

  1. CI green недостижим: в main нет пайплайна (#17 не влит), combined status пуст (total_count=0).
  2. Формальное ревью в Gitea невозможно (единственный пользователь = автор PR) — ревью в Kanban t_2dec61bb.
  3. Чтения сериализованы общим локом — параллелизм чтений не обещан (задокументировано).
  4. audit_log без ретеншена (#28); backend/tests исключён из ruff конфигом #16.

Порядок влития: #17 → #50. Merge выполняет владелец, не QA.

## QA-ревью раунд 2 — вердикт: **APPROVE** (head `c4661c9d`) Линза раунда 2 — execution: самостоятельный checkout артефакта и прогон, не перечитывание handoff'а. Ничего не мержено, в `main` не пушилось (ADVISORY). ### Идентичность артефакта Архив ветки, отданный Gitea по точному head-SHA `c4661c9d` (`git fetch` по HTTP в этой сети падает: `bad pack header`), сверен с API: размеры 19/19 файлов = `get_dir_contents`, blob-префиксы 5 изменённых файлов = заявленные (`bf03f2d0`, `a21dcb2b`, `3ecc2fe1`, `631e124e`, `1b879cf4`). ### Гейт на реальном merge-состоянии (`main` 063ecf18, #16 уже влит, + 19 файлов PR) ``` ruff check backend -> All checks passed! exit 0 ruff format --check backend -> 17 files already formatted exit 0 mypy backend (strict) -> Success: 21 source files exit 0 pytest -> 107 passed, coverage 97.80 % (порог 70 %) ``` PR-only дерево: ruff/format чисто, `mypy --explicit-package-bases` 18 файлов чисто, **105 passed / 97.79 %**; `mypy` без флага — N-1 из #16, PR его не вносит. Флейки: concurrency-файл 10/10, полный набор 3/3 зелёные. ### Требования раунда 1 — закрыты - **#54**: `Database.read()` под общим `RLock` (db.py:152-162), все 12 чтений `Store` на `read()`, `counts()` — один снимок; контракт в `db.py` и `store/README.md`. - **#55 D-1**: `apply()` ловит `BaseException`, `revert(previous)` вызывается всегда, `failed` в аудите, `Exception` → `ApplyError`, остальные пробрасываются. - **#55 D-2**: `old_value` читается внутри транзакции (repository.py:373-391). - Обязательные тесты присутствуют все 4. ### Независимые пробы QA (написаны по AC, не из тестов PR): **55/55 PASS** AC1 (пустой `/data` → WAL + схема + все таблицы), AC2 (аудит на мутации, откат не оставляет следа), AC3 (`ApplyError(restored=True)`, `revert` с `previous`, БД побайтово прежняя, `failed` + `audit_id`), AC4 (рестарт), schema-guard (`SchemaError`), **#54** 6 потоков × 30 upsert × 5 прогонов → `rules == audit == 180`, 0 ошибок; нет грязных чтений (ждал ≥0.3 с); во время падающего apply снаружи `['keep']`; **#55 D-1** `KeyboardInterrupt`/`SystemExit`/`CancelledError` → тип сохранён, `revert` вызван, `failed` в аудите, «висящих» `staged` нет; **#55 D-2** 4 конкурентных обновления → цепочка `old→new` связна. ### Мутационная проверка тестов (anti-tautology) Откат трёх фиксов в копии дерева → новый файл тестов падает: 6/7 в первом прогоне, **7/7 во втором** (`TypeError`/`InterfaceError`, `['baseline','uncommitted']`, `['new']` при `restored=True`, `reverted == []` ×3, разрыв цепочки geoip). Тесты проверяют поведение. Оговорка: geoip-тест в RED-состоянии срабатывает не всегда (узкая гонка), на исправленном коде 10/10 зелёный. ### Безопасность: 28/28 PASS 11 инъекционных payload'ов × 6 полей → все `ValidationError`; таблицы и `audit_log` целы; пустой `actor` отклонён; хардкода секретов нет. ### Неблокирующие 1. **CI green** недостижим: в `main` нет пайплайна (#17 не влит), combined status пуст (`total_count=0`). 2. Формальное ревью в Gitea невозможно (единственный пользователь = автор PR) — ревью в Kanban t_2dec61bb. 3. Чтения сериализованы общим локом — параллелизм чтений не обещан (задокументировано). 4. `audit_log` без ретеншена (#28); `backend/tests` исключён из ruff конфигом #16. **Порядок влития: #17 → #50.** Merge выполняет владелец, не QA.
embeddevio added the qa label 2026-10-28 21:27:22 +03:00
Author
Owner

TechLead — мержу по ревью QA раунд 2 (APPROVE).

Ревьюер проверил не отчёт, а артефакт: архив Gitea по точному head c4661c9d, гейт на реальном merge-состоянии (main 063ecf18 + PR) — ruff/format чисто, mypy strict 21 файл, pytest 107 passed / coverage 97.80 %; 55/55 собственных проб по AC, 28/28 security-проб, мутационный откат фиксов валит 6/7 и 7/7 новых тестов (значит, тесты не тавтологии). Блокирующих находок нет.

Статус дефектов: #54 (critical) — закрыт сериализацией чтений под RLock + контракт в README модуля; #55 D-1 (BaseException → revert + failed в аудите) и D-2 (TOCTOU set_geoip_schedule) — закрыты.

Отклонение от DoD: «CI green» заменён гейтом на реальном merge-состоянии (два независимых прогона: реализатор + ревьюер) — runner поднят (#52) и первые job'ы уже исполняются, но серия CI-проверок для main доедет после слива старой очереди runs. Порядок влития #47 → #57 → #50 соблюдён.

Мержу как TechLead. Refs #20, #54, #55.

**TechLead — мержу по ревью QA раунд 2 (APPROVE).** Ревьюер проверил не отчёт, а артефакт: архив Gitea по точному head `c4661c9d`, гейт на реальном merge-состоянии (`main 063ecf18` + PR) — ruff/format чисто, mypy strict 21 файл, pytest **107 passed / coverage 97.80 %**; 55/55 собственных проб по AC, 28/28 security-проб, мутационный откат фиксов валит 6/7 и 7/7 новых тестов (значит, тесты не тавтологии). Блокирующих находок нет. **Статус дефектов:** #54 (critical) — закрыт сериализацией чтений под `RLock` + контракт в README модуля; #55 D-1 (BaseException → `revert` + `failed` в аудите) и D-2 (TOCTOU `set_geoip_schedule`) — закрыты. **Отклонение от DoD:** «CI green» заменён гейтом на реальном merge-состоянии (два независимых прогона: реализатор + ревьюер) — runner поднят (#52) и первые job'ы уже исполняются, но серия CI-проверок для `main` доедет после слива старой очереди runs. Порядок влития #47 → #57 → #50 соблюдён. Мержу как TechLead. Refs #20, #54, #55.
embeddevio merged commit 4ff22f4323 into main 2026-10-28 21:35:42 +03:00
Sign in to join this conversation.