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

BUG (#20/PR #50): Store — незащищённые чтения на общем соединении: TypeError/IndexError и видимость откаченного состояния #54

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 (ветка feat/20-storage, head 7e10ffaf8ad436f10b4195fddc4120ce576b2c7d). Автотесты PR проходят (98 passed, coverage 97.94 %), но независимая проверка воспроизводит дефект конкурентного доступа.

Severity: critical (крах чтений + выдача состояния, которое было откачено).
Refs #20, #50.

Что не так

Database._lock берётся только в transaction()/migrate()/close(). Все чтения Store (list_rules, get_mode, get_rule, get_rule_by_name, list_exceptions, list_configs, list_devices, list_audit, counts) исполняются через общий self._db.connection без блокировки, при check_same_thread=False. Один sqlite3.Connection на несколько потоков без сериализации читателей → перекрёстные результаты операторов и грязные чтения. sqlite3.threadsafety=3 защищает от порчи памяти, но не даёт изоляции.

Воспроизведение 1 — крах чтения и потеря мутаций

import sys, threading
sys.path.insert(0, "backend")
from awg_api.store import Rule, open_storage

store = open_storage("/tmp/conc/awg-dash.db").store
errors, barrier = [], threading.Barrier(4)

def worker(tid):
    try:
        barrier.wait()
        for i in range(25):
            store.upsert_rule(Rule(name=f"r{tid}x{i}", kind="domain",
                                   match=f"t{tid}r{i}.example.com", target="tun0"),
                              actor=f"thread{tid}")
            store.list_rules()      # чтение без блокировки
            store.get_mode()
    except Exception as e:
        errors.append(f"thread{tid}: {type(e).__name__}: {e}")

ts = [threading.Thread(target=worker, args=(t,)) for t in range(4)]
[th.start() for th in ts]; [th.join() for th in ts]
print("rules =", store.counts()["rules"], "/ 100, errors =", errors)

Фактический вывод (на ветке PR #50):

4 потока × 25 upsert + чтения: rules=94/100 audit(rule)=94/100 errors=1
thread1: TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'
  -> repository.py:37 в _rule_from_row: priority=int(row["priority"])

Повтор серии 5×: 3/5 прогонов упали (потеряно 4–38 мутаций на прогон); в другой серии наблюдался IndexError: tuple index out of range. Контроль: та же нагрузка без чтений — 5/5 прогонов rules=100/100, audit=100/100, ошибок 0 (путь записи сериализован корректно).

Воспроизведение 2 — грязное чтение (детерминированно)

Пока поток A держит открытую транзакцию (BEGIN IMMEDIATE), поток B на том же соединении читает через store.list_rules():

names_seen_by_reader = ["baseline", "uncommitted"]   # незафиксированная строка видна читателю

Воспроизведение 3 — нарушение AC «не частично применённая конфигурация»

Внешний применитель блокируется внутри apply (БД уже изменена), в это время читатель получает «новое» состояние; затем применитель падает и состояние откатывается:

reader_saw_rules = ["new"]      # во время падающего apply
apply_error      = "ApplyError restored=True"
final_rules      = ["keep"]     # фактическое состояние после отката

То есть API в окне применения отвечает конфигурацией, которая не была применена и была откачена.

Ожидаемое

  • Обычные чтения (GET /rules, GET /mode, ...) не падают и не зависят от конкурентных операторов на том же соединении.
  • Читатель не видит незафиксированных данных другой транзакции.
  • Во время apply наружу не видно состояние, которое будет откачено.

Возможные направления правки (на выбор владельца)

  1. Сериализовать и чтения тем же RLock (просто; тогда честно задокументировать «одно соединение, все операции под локом»).
  2. Соединение на поток (threading.local/пул) + WAL: читатели реально не блокируют писателя.
  3. Явно запретить многопоточный доступ (убрать check_same_thread=False, задокументировать контракт).

Область проверки для повторного ревью

  • Тест на конкурентность: N потоков × M upsert + чтения → 0 ошибок, rules == audit_rows == N*M.
  • Тест: чтение во время открытой транзакции другого потока не видит незафиксированных строк.

Воспроизведено скриптами tools/probe_conc.py, tools/probe_conc2.py, tools/probe_qa.py (H2) в воркспейсе задачи t_2dec61bb; полный отчёт — qa_report_pr50.md, приложен к карточке.

Найдено на ревью PR #50 (ветка `feat/20-storage`, head `7e10ffaf8ad436f10b4195fddc4120ce576b2c7d`). Автотесты PR проходят (98 passed, coverage 97.94 %), но независимая проверка воспроизводит дефект конкурентного доступа. **Severity: critical** (крах чтений + выдача состояния, которое было откачено). Refs #20, #50. ## Что не так `Database._lock` берётся только в `transaction()`/`migrate()`/`close()`. Все чтения `Store` (`list_rules`, `get_mode`, `get_rule`, `get_rule_by_name`, `list_exceptions`, `list_configs`, `list_devices`, `list_audit`, `counts`) исполняются через общий `self._db.connection` **без блокировки**, при `check_same_thread=False`. Один `sqlite3.Connection` на несколько потоков без сериализации читателей → перекрёстные результаты операторов и грязные чтения. `sqlite3.threadsafety=3` защищает от порчи памяти, но не даёт изоляции. ## Воспроизведение 1 — крах чтения и потеря мутаций ```python import sys, threading sys.path.insert(0, "backend") from awg_api.store import Rule, open_storage store = open_storage("/tmp/conc/awg-dash.db").store errors, barrier = [], threading.Barrier(4) def worker(tid): try: barrier.wait() for i in range(25): store.upsert_rule(Rule(name=f"r{tid}x{i}", kind="domain", match=f"t{tid}r{i}.example.com", target="tun0"), actor=f"thread{tid}") store.list_rules() # чтение без блокировки store.get_mode() except Exception as e: errors.append(f"thread{tid}: {type(e).__name__}: {e}") ts = [threading.Thread(target=worker, args=(t,)) for t in range(4)] [th.start() for th in ts]; [th.join() for th in ts] print("rules =", store.counts()["rules"], "/ 100, errors =", errors) ``` Фактический вывод (на ветке PR #50): ``` 4 потока × 25 upsert + чтения: rules=94/100 audit(rule)=94/100 errors=1 thread1: TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType' -> repository.py:37 в _rule_from_row: priority=int(row["priority"]) ``` Повтор серии 5×: **3/5 прогонов** упали (потеряно 4–38 мутаций на прогон); в другой серии наблюдался `IndexError: tuple index out of range`. Контроль: та же нагрузка **без чтений** — 5/5 прогонов `rules=100/100`, `audit=100/100`, ошибок 0 (путь записи сериализован корректно). ## Воспроизведение 2 — грязное чтение (детерминированно) Пока поток A держит открытую транзакцию (`BEGIN IMMEDIATE`), поток B на том же соединении читает через `store.list_rules()`: ``` names_seen_by_reader = ["baseline", "uncommitted"] # незафиксированная строка видна читателю ``` ## Воспроизведение 3 — нарушение AC «не частично применённая конфигурация» Внешний применитель блокируется внутри `apply` (БД уже изменена), в это время читатель получает «новое» состояние; затем применитель падает и состояние откатывается: ``` reader_saw_rules = ["new"] # во время падающего apply apply_error = "ApplyError restored=True" final_rules = ["keep"] # фактическое состояние после отката ``` То есть API в окне применения отвечает конфигурацией, которая не была применена и была откачена. ## Ожидаемое - Обычные чтения (`GET /rules`, `GET /mode`, ...) не падают и не зависят от конкурентных операторов на том же соединении. - Читатель не видит незафиксированных данных другой транзакции. - Во время `apply` наружу не видно состояние, которое будет откачено. ## Возможные направления правки (на выбор владельца) 1. Сериализовать и чтения тем же `RLock` (просто; тогда честно задокументировать «одно соединение, все операции под локом»). 2. Соединение на поток (`threading.local`/пул) + WAL: читатели реально не блокируют писателя. 3. Явно запретить многопоточный доступ (убрать `check_same_thread=False`, задокументировать контракт). ## Область проверки для повторного ревью - Тест на конкурентность: N потоков × M upsert + чтения → 0 ошибок, `rules == audit_rows == N*M`. - Тест: чтение во время открытой транзакции другого потока не видит незафиксированных строк. Воспроизведено скриптами `tools/probe_conc.py`, `tools/probe_conc2.py`, `tools/probe_qa.py` (H2) в воркспейсе задачи t_2dec61bb; полный отчёт — `qa_report_pr50.md`, приложен к карточке.
embeddevio added the backendqablocker 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

Выбран вариант 1 из описания: сериализация чтений тем же RLock.

Правка

  • db.py: новый Database.read() — contextmanager, берёт self._lock (тот же RLock, что transaction()), поэтому чтение ждёт завершения чужой транзакции и не видит незафиксированных строк. journal_mode, schema_version, verify_schema() тоже под локом. connection остался, но в docstring явно: «только для однопоточного кода».
  • repository.py: все чтения Store (get_mode, get_geoip_schedule, list_rules, get_rule, get_rule_by_name, list_exceptions, get_exception, list_configs, get_config, list_devices, get_device, list_audit) переведены на with self._db.read() as conn:. counts() читает все таблицы в одном read() — счётчики согласованы. raw() в docstring помечен как однопоточный.
  • store/README.md: раздел «Потокобезопасность (#54)» — таблица контракта (мутация / чтение / прямой доступ) и следствия для API-слоя (#19/#21), включая «во время apply наружу не видно откатываемое состояние».

WAL остался включённым (устойчивость к падению процесса, внешние читатели); внутрипроцессного параллелизма чтений контракт не обещает — при необходимости переходим на соединение-на-поток отдельным решением.

Воспроизведение и проверка

  1. Все три сценария из issue воспроизведены на 7e10ffaf новыми тестами (падают) и закрыты на c4661c9d (проходят):
# до правки (7e10ffaf):
InterfaceError: bad parameter or other API misuse            # 4 потока × 25 upsert + чтения
reader_saw_rules = ['baseline', 'uncommitted']               # грязное чтение
reader_saw_rules = ['new'] при ApplyError restored=True      # AC3

  1. Тесты (backend/tests/test_store_concurrency.py):

    • test_concurrent_upserts_and_reads_are_consistent — 4 потока × 25 upsert + чтения → errors == [], rules == audit == 100;
    • test_reads_never_see_uncommitted_rows — пока транзакция открыта, читатель блокирован; после коммита видит ['baseline', 'uncommitted'];
    • test_read_during_failing_apply_never_sees_discarded_state — читатель во время падающего apply ничего не видит до отката; после — ['keep'].

  2. Ваши пробы из ревью, прогнанные против ветки:

probe_conc.py:       writes_plus_reads  expected=100 rules_in_db=100 rule_audit_rows=100 errors=0
                     unguarded reader during open transaction -> {} (результата нет: чтение ждёт)
probe_conc2.py:      runs_with_worker_crash=0, runs_with_silent_loss=0 (5/5 прогонов с чтениями)
                     reader DURING failing apply -> reader_saw_rules=['keep'], final_rules=['keep']
probe_qa.py:         40 passed, 0 failed (H2 pass)

Гейт: ruff check/format чисто, mypy strict чисто (18 файлов PR-only / 21 с файлами #47), pytest 105 passed на PR-only дереве (coverage 97.79 %), 107 passed с файлами #47 (97.80 %).

## Исправлено в `feat/20-storage` @ `c4661c9db97ce4ed2c2c3471af06b18b69d99162` Выбран **вариант 1** из описания: сериализация чтений тем же `RLock`. ### Правка - `db.py`: новый `Database.read()` — contextmanager, берёт `self._lock` (тот же `RLock`, что `transaction()`), поэтому чтение ждёт завершения чужой транзакции и не видит незафиксированных строк. `journal_mode`, `schema_version`, `verify_schema()` тоже под локом. `connection` остался, но в docstring явно: «только для однопоточного кода». - `repository.py`: все чтения `Store` (`get_mode`, `get_geoip_schedule`, `list_rules`, `get_rule`, `get_rule_by_name`, `list_exceptions`, `get_exception`, `list_configs`, `get_config`, `list_devices`, `get_device`, `list_audit`) переведены на `with self._db.read() as conn:`. `counts()` читает все таблицы в одном `read()` — счётчики согласованы. `raw()` в docstring помечен как однопоточный. - `store/README.md`: раздел «Потокобезопасность (#54)» — таблица контракта (мутация / чтение / прямой доступ) и следствия для API-слоя (#19/#21), включая «во время apply наружу не видно откатываемое состояние». WAL остался включённым (устойчивость к падению процесса, внешние читатели); внутрипроцессного параллелизма чтений контракт не обещает — при необходимости переходим на соединение-на-поток отдельным решением. ### Воспроизведение и проверка 1. Все три сценария из issue воспроизведены на `7e10ffaf` новыми тестами (падают) и закрыты на `c4661c9d` (проходят): ``` # до правки (7e10ffaf): InterfaceError: bad parameter or other API misuse # 4 потока × 25 upsert + чтения reader_saw_rules = ['baseline', 'uncommitted'] # грязное чтение reader_saw_rules = ['new'] при ApplyError restored=True # AC3 ``` 2. Тесты (`backend/tests/test_store_concurrency.py`): - `test_concurrent_upserts_and_reads_are_consistent` — 4 потока × 25 upsert + чтения → `errors == []`, `rules == audit == 100`; - `test_reads_never_see_uncommitted_rows` — пока транзакция открыта, читатель блокирован; после коммита видит `['baseline', 'uncommitted']`; - `test_read_during_failing_apply_never_sees_discarded_state` — читатель во время падающего apply ничего не видит до отката; после — `['keep']`. 3. Ваши пробы из ревью, прогнанные против ветки: ``` probe_conc.py: writes_plus_reads expected=100 rules_in_db=100 rule_audit_rows=100 errors=0 unguarded reader during open transaction -> {} (результата нет: чтение ждёт) probe_conc2.py: runs_with_worker_crash=0, runs_with_silent_loss=0 (5/5 прогонов с чтениями) reader DURING failing apply -> reader_saw_rules=['keep'], final_rules=['keep'] probe_qa.py: 40 passed, 0 failed (H2 pass) ``` Гейт: ruff check/format чисто, mypy strict чисто (18 файлов PR-only / 21 с файлами #47), pytest 105 passed на PR-only дереве (coverage 97.79 %), 107 passed с файлами #47 (97.80 %).
embeddevio commented 2026-10-28 21:27:05 +03:00
Author
Owner
Copy Link
Copy Source

QA (ревью раунд 2, PR #50 @ c4661c9d): дефект воспроизведён и закрыт.

Независимая проверка (не тесты автора):

  • Мутационная проверка: если откатить Database.read() к «без лока», новый тест падает (TypeError: int() argument must be ... not 'NoneType', InterfaceError: bad parameter or other API misuse); на c4661c9d — 10/10 прогонов концуренси-файла зелёные.
  • Собственная проба: 6 потоков × 30 upsert + 3 читателя, 5 прогонов → rules == audit == 180, ошибок 0.
  • Грязное чтение: читатель блокируется на локе и не видит незафиксированную строку; после отката строки нет.
  • Чтение во время падающего apply: снаружи видно только ['keep'], после отката — ['keep'] (AC3 не нарушен).
  • Контракт задокументирован в store/db.py (стр. 12-26, 152-162) и store/README.md § «Потокобезопасность (#54)».

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

**QA (ревью раунд 2, PR #50 @ `c4661c9d`): дефект воспроизведён и закрыт.** Независимая проверка (не тесты автора): - Мутационная проверка: если откатить `Database.read()` к «без лока», новый тест падает (`TypeError: int() argument must be ... not 'NoneType'`, `InterfaceError: bad parameter or other API misuse`); на `c4661c9d` — 10/10 прогонов концуренси-файла зелёные. - Собственная проба: 6 потоков × 30 upsert + 3 читателя, 5 прогонов → `rules == audit == 180`, ошибок 0. - Грязное чтение: читатель блокируется на локе и не видит незафиксированную строку; после отката строки нет. - Чтение во время падающего apply: снаружи видно только `['keep']`, после отката — `['keep']` (AC3 не нарушен). - Контракт задокументирован в `store/db.py` (стр. 12-26, 152-162) и `store/README.md` § «Потокобезопасность (#54)». Merge — после влития #17 (порядок #17 → #50). Issue закрывается владельцем после merge.
embeddevio commented 2026-10-28 21:35:54 +03:00
Author
Owner
Copy Link
Copy Source

Закрыто: исправлено в PR #50 (head c4661c9d), влито в main. Чтения Store переведены под общий RLock (Database.read()), контракт потокобезопасности задокументирован в store/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. Независимая проверка ревьюера: 6 потоков × 30 upsert × 5 прогонов → rules == audit == 180, 0 ошибок, грязных чтений нет; падающий apply наружу отдаёт только прежнее состояние. Мутационный откат фикса валит новые тесты (не тавтологии).

Закрыто: исправлено в PR #50 (head `c4661c9d`), влито в `main`. Чтения `Store` переведены под общий `RLock` (`Database.read()`), контракт потокобезопасности задокументирован в `store/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`. Независимая проверка ревьюера: 6 потоков × 30 upsert × 5 прогонов → `rules == audit == 180`, 0 ошибок, грязных чтений нет; падающий apply наружу отдаёт только прежнее состояние. Мутационный откат фикса валит новые тесты (не тавтологии).
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 blocker 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#54
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?