Skip to content

Fix Stage E concurrency cap: hold advisory lock on a dedicated connection - #453

Merged
WilfordGrimley merged 1 commit into
masterfrom
fix-stage-e-concurrency-connection
Jul 25, 2026
Merged

Fix Stage E concurrency cap: hold advisory lock on a dedicated connection#453
WilfordGrimley merged 1 commit into
masterfrom
fix-stage-e-concurrency-connection

Conversation

@WilfordGrimley

Copy link
Copy Markdown

Description

Production-verified fix for the Stage E dispatch concurrency cap (PR #450, MPCAutofill/cardpicker/stage_e_concurrency.py), which did not bind in its first live shakedown.

Incident (2026-07-25T00:25Z shakedown): envtrip-20260725T002504-73e1eb6d, {'ceiling': 7.0, 'load_avg': 11.4013671875}. Zero throttled-concurrency-cap outcomes despite 8 concurrent django-q workers, plus 8 occurrences of the module's own pg_advisory_unlock reported slot N was not held by this connection warning.

Root cause, confirmed by reading the installed django_q package: the original module held its Postgres advisory lock on django.db.connection (Django's shared per-thread connection), on the strength of a claim (checked against the wrong code path — django_q.worker's connection recycling, which only happens between tasks) that a single dispatch_micro_batch call always runs as one uninterrupted segment on one connection. The real trigger: cardpicker.stage_e_signals's post_save receivers fire during Stage C's persist_evidence, inside the locked region, and call django_q.tasks.async_task(...), which synchronously calls the installed ORM broker's enqueue. django_q.brokers.orm.ORM.get_connection() calls django.db.close_old_connections() unconditionally whenever not inside an atomic block — and this project's DATABASES["default"] has no CONN_MAX_AGE override, so Django's own default (0) applies, meaning the connection is treated as already-expired the first time anything asks. A closed connection auto-releases every advisory lock its session held, so every worker then found every slot "free".

Fix: stage_e_concurrency.py now opens a dedicated psycopg2 connection (autocommit=True) it alone owns for the lifetime of one try_acquire_dispatch_slot() call — never django.db.connection. Explicitly pg_advisory_unlocks AND close()s in a finally, so the lock is released even if the explicit unlock itself somehow fails. The "not held by this connection" warning guard is kept unchanged — it's what caught this bug, and should now never fire again. Connection-creation failure fails CLOSED (dispatch treated as throttled) rather than proceeding uncapped, since an uncapped dispatch is exactly the failure this incident was.

Observability (Tron gate anomaly 4): throttled dispatches previously wrote no ledger row and emitted only a log line, so the runbook's "tune the cap against the observed throttle rate" instruction had nothing queryable to check. Added StageEThrottleCounter — a singleton, always-one-row, atomically-incremented counter (migration 0081_stageethrottlecounter), visible in Django admin. Deliberately not a per-event row (would write-amplify under exactly the failure shape this module guards against).

Zero cardpicker.* imports preserved in stage_e_concurrency.py (extractable-primitives ledger updated).

Checklist

  • I have installed pre-commit and installed the hooks with pre-commit install before creating any commits.
  • I have updated any related tests for code I modified or added new tests where appropriate.
  • I have manually tested my changes as follows:
    • pytest cardpicker/tests/test_stage_e_concurrency.py cardpicker/tests/test_stage_e_dispatch.py -q → 39 passed (real testcontainers Postgres).
    • Proved the new regression tests (TestRegressionDedicatedConnectionSurvivesFollowOnEnqueue) catch the bug: temporarily restored the pre-fix module (django.db.connection-based lock) and confirmed both new tests fail (assert False is True — a genuinely separate session found the slot free when it shouldn't be) against it, then restored the fix and confirmed all 39 pass again.
    • pre-commit run mypy --files <changed .py files> → passed.
    • pre-commit run --files <all changed files> (ruff/isort/black/mypy/prettier) → passed.
    • .github/scripts/docs_lint.py → clean.
  • I have updated any relevant documentation or created new documentation where appropriate.
    • docs/features/stage-e-operations.md (Concurrency cap section: dedicated-connection fix, fail-closed choice, throttle-observability counter)
    • docs/troubleshooting.md (new symptom-first entry with the literal warning string)
    • docs/lessons.md (terse entry: a static "no connection pool" review doesn't prove connection stability across a code region's own side effects)
    • docs/upstreaming/extractable-primitives.md (updated the existing CLEAN row for the connection-lifecycle change)

@WilfordGrimley
WilfordGrimley merged commit d7d55e3 into master Jul 25, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant