lekss361/gendesign

Fork 0

fix(site-finder): cascade InFailedSqlTransaction — velocity internal rollback #155

Merged

lekss361 merged 2 commits from fix/velocity-internal-rollback-cascade into main

2026-05-15 05:52:39 +00:00

lekss361 commented

2026-05-15 05:42:03 +00:00

(Migrated from github.com)

Root cause (final)

POST /analyze всё ещё крашится 500 после PR #154 SAVEPOINT wrap. Глубже investigation:

velocity.py имеет 3 internal try/except (lines 77, 149, 268) которые:

Ловят SQL exception ✓
Логируют ✓
return None БЕЗ db.rollback() ❌

Из-за return None exception НЕ propagates за пределы velocity. PR #154 SAVEPOINT не triggers rollback (no exception to catch). Но Postgres transaction УЖЕ aborted внутри savepoint. Caller (analyze_parcel) видит velocity = None нормально, но следующая SQL query (geotech_risk → osm_noise_sources_ekb) → cascade InFailedSqlTransaction.

Fix

velocity.py — db.rollback() в 3 catch блоках

После logger.exception/warning делаем db.rollback() (с try/except защитой на double-rollback case).

parcels.py — SAVEPOINT для zoning + success_recommendation

Defensive — те же try/except без rollback patterns. Wrap в with db.begin_nested().

Test plan

После merge → deploy → POST /analyze работает (velocity gracefully degrades к None, остальные fields normal).

PR #154 был partial — fixed SAVEPOINT wrap но не internal velocity error swallow.

Refs: user reports cascade InFailedSqlTransaction

## Root cause (final) POST /analyze всё ещё крашится 500 после PR #154 SAVEPOINT wrap. Глубже investigation: `velocity.py` имеет **3 internal try/except** (lines 77, 149, 268) которые: 1. Ловят SQL exception ✓ 2. Логируют ✓ 3. **return None БЕЗ db.rollback()** ❌ Из-за `return None` exception НЕ propagates за пределы velocity. PR #154 SAVEPOINT не triggers rollback (no exception to catch). Но Postgres transaction УЖЕ aborted внутри savepoint. Caller (analyze_parcel) видит `velocity = None` нормально, но **следующая SQL query** (geotech_risk → osm_noise_sources_ekb) → cascade `InFailedSqlTransaction`. ## Fix ### velocity.py — db.rollback() в 3 catch блоках После logger.exception/warning делаем `db.rollback()` (с try/except защитой на double-rollback case). ### parcels.py — SAVEPOINT для zoning + success_recommendation Defensive — те же try/except без rollback patterns. Wrap в `with db.begin_nested()`. ## Test plan После merge → deploy → POST /analyze работает (velocity gracefully degrades к None, остальные fields normal). ## Related PR #154 был partial — fixed SAVEPOINT wrap но не internal velocity error swallow. Refs: user reports cascade InFailedSqlTransaction

lekss361 commented

2026-05-15 05:45:28 +00:00

(Migrated from github.com)

Bot review (SHA `c3cafef`, CI ✅)

Follow-up к моему PR #154 ревью. Author более глубоко investigated и нашёл что #154 был partial.

Корректный root cause analysis

✅ velocity.py 3 internal try/except swallow'или exception: return None без db.rollback(). PR #154's outer SAVEPOINT не triggers rollback потому что no exception propagates.
✅ Postgres tx уже aborted внутри savepoint, но savepoint commit'нулся cleanly → outer tx остаётся aborted → next query (_geotech_risk line 828) → cascade InFailedSqlTransaction.

Fix scope

✅ velocity.py: 3 catch блока теперь делают try: db.rollback() except: pass после log + перед return None. Double-protection (inner try) защищает от double-rollback edge cases.
✅ parcels.py: zoning + success_recommendation queries имели похожий "try/except no rollback" anti-pattern — wrapped в with db.begin_nested(): defensively.
✅ CI green, +76/-57.

Subtle concern (запросил cross-check от агента)

db.rollback() внутри compute_velocity вызывается из caller'а который обернул вызов в with db.begin_nested(): (PR #154). Теоретическая question: SQLAlchemy 2.0 Session.rollback() semantics inside savepoint context — rolls back savepoint only или entire outer tx?

Если entire tx → savepoint released → caller's with db.begin_nested(): exit может крашиться trying to commit released savepoint.

Empirically CI passes, double-try protects от rollback() exceptions. Но это subtle. Запросил независимое мнение через general-purpose агента — обновлю если найдёт что-то.

Лучший альтернативный fix (рассмотреть в follow-up)

Cleaner был бы: убрать internal try/except в velocity, let exceptions propagate. Caller's with db.begin_nested(): тогда автоматически roll-back-нёт savepoint + transaction state clean. Меньше surface, меньше edge cases.

Но текущий fix должен работать т.к. double-try в velocity защищает от любых rollback issues. Принимаю.

Минор

parcels.py lines 1693-1735 имеют огромный indent shift — это просто with wrap, content идентичен. Diff визуально шумный, но логически тривиальный.

Жду агент

Если он подтвердит concerns про savepoint semantics — обновлю review. Иначе approve+merge на следующем тике.

## Bot review (SHA `c3cafef`, CI ✅) Follow-up к моему PR #154 ревью. Author более глубоко investigated и нашёл что #154 был partial. ### Корректный root cause analysis ✅ **velocity.py 3 internal try/except swallow'или exception**: `return None` без `db.rollback()`. PR #154's outer SAVEPOINT не triggers rollback потому что no exception propagates. ✅ Postgres tx уже aborted внутри savepoint, но savepoint commit'нулся cleanly → outer tx остаётся aborted → next query (`_geotech_risk` line 828) → cascade `InFailedSqlTransaction`. ### Fix scope ✅ `velocity.py`: 3 catch блока теперь делают `try: db.rollback() except: pass` после log + перед return None. Double-protection (inner try) защищает от double-rollback edge cases. ✅ `parcels.py`: zoning + success_recommendation queries имели похожий "try/except no rollback" anti-pattern — wrapped в `with db.begin_nested():` defensively. ✅ CI green, +76/-57. ### Subtle concern (запросил cross-check от агента) `db.rollback()` внутри `compute_velocity` вызывается из caller'а который обернул вызов в `with db.begin_nested():` (PR #154). Теоретическая question: SQLAlchemy 2.0 `Session.rollback()` semantics inside savepoint context — rolls back **savepoint only** или **entire outer tx**? Если entire tx → savepoint released → caller's `with db.begin_nested():` __exit__ может крашиться trying to commit released savepoint. Empirically CI passes, double-try protects от rollback() exceptions. Но это subtle. Запросил независимое мнение через `general-purpose` агента — обновлю если найдёт что-то. ### Лучший альтернативный fix (рассмотреть в follow-up) Cleaner был бы: убрать internal try/except в velocity, let exceptions propagate. Caller's `with db.begin_nested():` тогда автоматически roll-back-нёт savepoint + transaction state clean. Меньше surface, меньше edge cases. Но текущий fix должен работать т.к. double-try в velocity защищает от любых rollback issues. Принимаю. ### Минор `parcels.py` lines 1693-1735 имеют огромный indent shift — это просто `with` wrap, content идентичен. Diff визуально шумный, но логически тривиальный. ### Жду агент Если он подтвердит concerns про savepoint semantics — обновлю review. Иначе approve+merge на следующем тике.

lekss361 commented

2026-05-15 05:46:23 +00:00

(Migrated from github.com)

⚠️ UPDATE: Bot review — cross-checked, fix incorrect

После независимого cross-check'а через general-purpose agent (анализ SQLAlchemy 2.0 semantics) — этот PR не нужно мерджить как есть, он вводит худший bug чем решает.

Root cause проблемы fix'а

db.rollback() внутри compute_velocity вызывается из caller'а который обернул вызов в with db.begin_nested(): (PR #154). И тут крайне subtle SQLAlchemy 2.0 поведение:

Session.rollback() rolls back ENTIRE outer transaction, не savepoint. SQLAlchemy pops all SAVEPOINTs и rollback'ает root. Чтобы откатить только savepoint, нужно позволить context manager сделать это через exception propagation (что и происходит при with begin_nested(): + raise).
Когда velocity catch вызывает db.rollback():
- Root transaction обнуляется (✓ это очищает InFailedSqlTransaction)
- НО caller's with db.begin_nested(): savepoint становится orphaned — SessionTransaction больше не active
- На __exit__ SQLAlchemy пытается RELEASE SAVEPOINT на already-deactivated tx → raises ResourceClosedError / InvalidRequestError: This SessionTransaction is no longer active
Net effect: даже если caller's outer try/except swallow'ит это, все последующие queries в analyze_parcel выполняются вне tx или в broken state (root tx был nuked under them). Получается тот же класс bugs, который PR пытается решить, только тоньше.
CI green только потому что savepoint-orphan path не покрыт тестами — unit tests мockирующие Session не triggers реальный SAVEPOINT lifecycle. Prod упадёт на первом velocity SQL exception.

Правильный fix

Mirror тот pattern, который PR #155 уже применил правильно для zoning + success_recommendation queries в parcels.py — обернуть каждый db.execute(...) в velocity.py в свой with db.begin_nested()::

# velocity.py — Option A (cleanest):
try:
    with db.begin_nested():  # ← SAVEPOINT per query
        comp_rows = (
            db.execute(text("""..."""), {...})
            .mappings()
            .all()
        )
except Exception:
    logger.exception("velocity: competitor query failed for wkt=%s", parcel_geom_wkt[:80])
    return None
    # savepoint auto-rollback'нут __exit__'ом при exception
    # outer tx остаётся clean — exactly то что нужно

Заменить так все 3 catch блока в velocity.py (lines ~126, ~193, ~323), убрать db.rollback() calls.

Альтернатива (option B, проще): убрать internal try/except из velocity.py вообще, let exceptions propagate. Caller's with db.begin_nested(): (из PR #154) автоматически handle'ит. Меньше surface, меньше edge cases. Но теряем custom log message внутри velocity.

Recommendation

Request changes:

Заменить 3 db.rollback() блока в velocity.py на with db.begin_nested(): wrappers (или option B — убрать internal try/except)
parcels.py zoning + success_recommendation changes — OK, паттерн правильный
После push нового SHA — re-review

Извинения

Я в interim review (15 min назад) предположил "double-try защищает, CI green, accept". Это было слишком оптимистично — агент справедливо разобрал SQLAlchemy semantics глубже, я этот edge case недостаточно проверил.

Это именно тот тип subtle tx-state bug которым стоит спорить с CI green'ом. Не мерджу.

## ⚠️ UPDATE: Bot review — cross-checked, **fix incorrect** После независимого cross-check'а через `general-purpose` agent (анализ SQLAlchemy 2.0 semantics) — **этот PR не нужно мерджить как есть**, он вводит худший bug чем решает. ### Root cause проблемы fix'а `db.rollback()` внутри `compute_velocity` вызывается из caller'а который обернул вызов в `with db.begin_nested():` (PR #154). И тут крайне subtle SQLAlchemy 2.0 поведение: 1. **`Session.rollback()` rolls back ENTIRE outer transaction, не savepoint.** SQLAlchemy pops all SAVEPOINTs и rollback'ает root. Чтобы откатить только savepoint, **нужно** позволить context manager сделать это через exception propagation (что и происходит при `with begin_nested():` + raise). 2. **Когда velocity catch вызывает `db.rollback()`**: - Root transaction обнуляется (✓ это очищает `InFailedSqlTransaction`) - НО caller's `with db.begin_nested():` savepoint становится **orphaned** — SessionTransaction больше не active - На `__exit__` SQLAlchemy пытается `RELEASE SAVEPOINT` на already-deactivated tx → raises `ResourceClosedError` / `InvalidRequestError: This SessionTransaction is no longer active` 3. **Net effect**: даже если caller's outer try/except swallow'ит это, **все последующие queries в analyze_parcel выполняются вне tx или в broken state** (root tx был nuked under them). Получается тот же класс bugs, который PR пытается решить, только тоньше. 4. **CI green только потому что savepoint-orphan path не покрыт тестами** — unit tests мockирующие Session не triggers реальный SAVEPOINT lifecycle. Prod упадёт на первом velocity SQL exception. ### Правильный fix Mirror тот pattern, который PR #155 **уже применил правильно** для zoning + success_recommendation queries в parcels.py — обернуть каждый `db.execute(...)` в `velocity.py` в свой `with db.begin_nested():`: ```python # velocity.py — Option A (cleanest): try: with db.begin_nested(): # ← SAVEPOINT per query comp_rows = ( db.execute(text("""..."""), {...}) .mappings() .all() ) except Exception: logger.exception("velocity: competitor query failed for wkt=%s", parcel_geom_wkt[:80]) return None # savepoint auto-rollback'нут __exit__'ом при exception # outer tx остаётся clean — exactly то что нужно ``` Заменить так все 3 catch блока в velocity.py (lines ~126, ~193, ~323), убрать `db.rollback()` calls. **Альтернатива** (option B, проще): убрать internal try/except из velocity.py вообще, let exceptions propagate. Caller's `with db.begin_nested():` (из PR #154) автоматически handle'ит. Меньше surface, меньше edge cases. Но теряем custom log message внутри velocity. ### Recommendation Request changes: - Заменить 3 `db.rollback()` блока в `velocity.py` на `with db.begin_nested():` wrappers (или option B — убрать internal try/except) - `parcels.py` zoning + success_recommendation changes — **OK**, паттерн правильный - После push нового SHA — re-review ### Извинения Я в interim review (15 min назад) предположил "double-try защищает, CI green, accept". Это было слишком оптимистично — агент справедливо разобрал SQLAlchemy semantics глубже, я этот edge case недостаточно проверил. Это именно тот тип subtle tx-state bug которым стоит спорить с CI green'ом. Не мерджу.

lekss361 commented

2026-05-15 05:52:33 +00:00

(Migrated from github.com)

Final review (SHA `37fd4b5`, CI ✅)

Cross-checked через general-purpose agent — fix полностью корректен и адресует bug из моего предыдущего review.

Что проверено агентом

✅ velocity.py: все 3 db.rollback() удалены, каждый db.execute(...) wrapped в with db.begin_nested(): per-query
✅ Pattern correctness: try → with begin_nested(): → execute → savepoint __exit__ auto-rollback ТОЛЬКО savepoint на exception → outer tx clean → return None
✅ parcels.py: zoning + success_recommendation wrappers preserved
✅ Inline comments документируют SQLAlchemy 2.0 SAVEPOINT semantics (ссылка на bot review)
✅ No side-effects: control flow (graceful return None) сохранён

Минор (не блокер)

Нет regression test для tx-cascade scenario — был бы ценен (mock outer SAVEPOINT + failing inner query → проверить что caller's tx остаётся clean), но не blocker для этого fix'а.

CI

backend: ✅ SUCCESS
frontend: ✅ SUCCESS

approve merge

## Final review (SHA `37fd4b5`, CI ✅) Cross-checked через `general-purpose` agent — fix полностью корректен и адресует bug из моего предыдущего review. ### Что проверено агентом ✅ **`velocity.py`**: все 3 `db.rollback()` удалены, каждый `db.execute(...)` wrapped в `with db.begin_nested():` per-query ✅ **Pattern correctness**: try → `with begin_nested():` → execute → savepoint `__exit__` auto-rollback ТОЛЬКО savepoint на exception → outer tx clean → `return None` ✅ **`parcels.py`**: zoning + success_recommendation wrappers preserved ✅ **Inline comments** документируют SQLAlchemy 2.0 SAVEPOINT semantics (ссылка на bot review) ✅ **No side-effects**: control flow (graceful `return None`) сохранён ### Минор (не блокер) Нет regression test для tx-cascade scenario — был бы ценен (mock outer SAVEPOINT + failing inner query → проверить что caller's tx остаётся clean), но не blocker для этого fix'а. ### CI - backend: ✅ SUCCESS - frontend: ✅ SUCCESS approve merge