fix(api): break the import cycle, stop pinning DB connections, revive retries

[mariuspruvot]

Correctness findings from the 2026-08-01 audit. Every bug below was reproduced before being fixed, and the regression tests were checked against the old code to confirm they fail on it.

The import cycle the app was living with

Each module package re-exported its router, and core/dependencies.py imports identity.models. Importing any submodule therefore pulled the whole router graph back through a half-initialised core.dependencies:

$ python -c "import helprs.core.dependencies"
ImportError: cannot import name 'CurrentUser' from partially initialized module

The app booted only because main.py happens to import admin.views — which imports a model — before any router. An isort pass would have broken startup. It was also the reason identity/service.py and container/service.py each carried a function-level import.

The four __init__.py files are bare now, both deferred imports are hoisted, and tests/test_import_graph.py imports each module in a fresh interpreter — once anything pulls in helprs.main, everything is in sys.modules and a cycle is invisible.

Two paths pinned a database connection for minutes

FastAPI tears yield-dependencies down only after the streaming body completes. The SSE route took db: DbSession, and its authentication dependency took another — so a pooled connection, with an open transaction, stayed checked out for the life of the stream: up to CONTAINER_TTL_SECONDS (900s). At DB_POOL_SIZE + DB_MAX_OVERFLOW = 15 per worker, fifteen viewers starve every other request on that worker, and each stream is an idle-in-transaction backend blocking VACUUM. get_db_context's own docstring warns against exactly this.

finalize_session did the same thing, holding a session across asyncio.wait_for(docker.wait_container(...), timeout=900) — plus a lock on the session row.

The SSE route now takes neither dependency: it authenticates and authorizes inside a short get_db_context() that closes before streaming starts. get_current_user_for_stream is split into stream_token(request) and authenticate_token(session, settings, token) so the route can call them directly. finalize_session is three phases — read, wait with nothing held, write.

Client disconnect silently truncated the scorecard

On disconnect, finalization was detached but the drain was not. The generator closed, stream_and_persist stopped consuming, and finalize_session then built its scorecard from a truncated event list and posted that to the PR — quietly defeating the thing detaching finalization exists to protect, and only in the common case of someone closing the tab during a ten-minute session.

Both are detached together now. Re-reading the log from the start is deliberate and cheap: _persist is idempotent through ON CONFLICT DO NOTHING. The stream also gained an except Exception arm that emits an event: error frame — previously a mid-stream failure produced a truncated body with no done, and native EventSource reconnected straight back into it.

The enum was stored in the wrong form

SQLAlchemy's Enum persists the member name unless told otherwise, so the ORM wrote 'RUNNING' while the column's server_default is 'pending' and the API returns 'running':

python RUNNING  -> stored 'RUNNING'
stored 'running' -> LookupError: 'running' is not among the defined enum values

Any row created without an explicit status was permanently unreadable through the ORM. Fixed with values_callable plus a data migration, verified end-to-end against a real Postgres: a legacy RUNNING row comes back running after alembic upgrade head.

Three query parameters returned 500

per_page=0 divided by zero computing total_pages; per_page=-5 reached Postgres as a negative LIMIT; ?status=bogus raised ValueError before any DB call. The handler validated by hand with min()/max() and a bare ContainerStatus(status). All three are constrained in the signature now, so FastAPI answers 422 before the handler runs.

Contract change worth flagging: per_page=500 used to be silently clamped to 100 and is now a 422. The frontend only ever sends 20 (dashboardApi.ts:78), so nothing in-tree breaks, but an external caller relying on the clamp would notice.

A failed container start left no trace

start_container set FAILED and flushed, then raised — and the exception unwound through get_db, which rolls back, discarding the status and the session row created earlier in the same request. The user got a 502 and a dashboard showing nothing had ever happened. The row is committed before the container is touched, and the failure status is committed rather than flushed. The new test rolls the request session back explicitly; it fails against the old code.

The retry policy was fiction

mark_failed wrote status="failed", and get_replayable_events only ever selected pending/processing. Nothing moved a row back, so a single handler exception dropped the delivery permanently, retry_count could never exceed 1, and both MAX_RETRY_COUNT = 5 and the entire abandoned branch were unreachable code documented as a working policy.

failed is now replayable and claimable — selecting it without teaching mark_processing to accept it would just have the reaper pick the same rows forever. Idleness is measured from updated_at so retries are spaced by the grace period, and mark_processing gained the retry_count guard so abandonment sticks.

Also

• core/dependencies.py ran its own select(GitHubUser) over identity's table; it goes through identity.repository.get_by_id now, and parses the UUID once instead of parsing then discarding it.
• stream_token requires a well-formed Bearer value. The old code used removeprefix, so a bare Authorization: <token> was accepted there but rejected by the header-only path — two notions of a valid header in one module.
• Two function-level exception imports in test_service.py hoisted.

Verification

• 389 tests pass (was 369), ruff + ruff format + mypy clean
• Migration applied against a real Postgres, including the legacy-row conversion and its downgrade
• Every regression test was run against the pre-fix code to confirm it fails there

Deferred, not forgotten

cleanup_all_running and reconcile_stale_sessions are not scoped to the worker that owns a session, so one worker restarting cancels live sessions belonging to its peers and marks the survivors FAILED. Unlike the reaper these are not idempotent — they are actively destructive to other workers. Fixing it properly needs an ownership marker (a boot id on the container labels, or a column), which is a bigger change than belongs in this PR.

[helprs-prod]

helPRs session created for this PR.

Skill: challenge-me | Open session