mirror of
https://github.com/jhd3197/ServerKit.git
synced 2026-11-09 05:24:49 +09:00
Juan DenisandClaude Opus 5
3e4bd69ab8
The 500 handler shares the app-context-scoped db.session with the view that just failed, which breaks two ways. The one that matters: when the crash IS a database error, that session is already in a failed transaction, so record_error's very first query raises PendingRollbackError -- swallowed by its own never-raise contract. Database- caused 500s were therefore the one class of crash the error tracker could never show, and they are the class most worth seeing. 72 API views commit with no try/except anywhere in the function, so this is not a narrow path. The second: record_error's commit() flushes whatever ORM state the crashed view left pending, persisting a half-applied write on the way out. Rolling back first fixes both. Flask-SQLAlchemy's teardown rolls this session back moments later anyway, so a clean request is unaffected. Reviewed as an alternative to a partial unique index on the dedup fingerprint, which was rejected: record_error swallows everything and returns None, so an IntegrityError would DROP the error report rather than dedupe it; set_resolved commits unguarded, so un-resolving an old row sharing a fingerprint would 500 a legitimate admin action; and CREATE UNIQUE INDEX would abort the migration on exactly the installs that already accumulated duplicates. The race is real but its blast radius is duplicate rows in an admin list -- nothing reads fingerprint for uniqueness, and the lookup is .first(), not .one(). Both new tests fail with the rollback removed (mutation-checked by line number). The first draft of the DB-error test did NOT: a hand-built IntegrityError never poisons the session, so it has to be a real failed flush. Race reported as High by Copilot on the error-tracker PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>