fix(fc3i): single-line celery.signals import + INT32 bounds on target_id + dict.fromkeys + rewrite retry test as direct _finalize unit
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -22,9 +22,7 @@ import logging
|
||||
from datetime import UTC, datetime
|
||||
|
||||
from celery.exceptions import SoftTimeLimitExceeded
|
||||
from celery.signals import (
|
||||
task_failure, task_postrun, task_prerun, task_retry,
|
||||
)
|
||||
from celery.signals import task_failure, task_postrun, task_prerun, task_retry
|
||||
|
||||
from .models import TaskRun
|
||||
from .tasks._sync_engine import sync_session_factory
|
||||
@@ -44,6 +42,15 @@ _MAX_ERROR_MESSAGE_LEN = 2000
|
||||
_MAX_ARGS_SUMMARY_LEN = 255
|
||||
_MAX_WORKER_HOSTNAME_LEN = 128
|
||||
|
||||
# PostgreSQL Integer is signed 32-bit. Tasks called with a first-arg
|
||||
# int outside this range (e.g. an absurdly large mock value, or a
|
||||
# string-of-digits coercible to int but bigger than 2^31-1) would crash
|
||||
# the INSERT with NumericValueOutOfRange. Bound the recorded value to
|
||||
# the column's range; values outside become None (target_id is
|
||||
# nullable, so this is safe).
|
||||
_INT32_MAX = 2_147_483_647
|
||||
_INT32_MIN = -2_147_483_648
|
||||
|
||||
|
||||
def _queue_for(task) -> str:
|
||||
"""Reverse the task→queue routing from celery_app.task_routes.
|
||||
@@ -68,14 +75,18 @@ def _queue_for(task) -> str:
|
||||
|
||||
|
||||
def _target_id_from_args(args) -> int | None:
|
||||
"""Best-effort: if the first positional arg parses as int, record
|
||||
it as target_id (image_id, source_id, etc.). Never raises."""
|
||||
"""Best-effort: if the first positional arg parses as int AND fits
|
||||
in the column's signed-32-bit range, record it as target_id
|
||||
(image_id, source_id, etc.). Never raises."""
|
||||
if not args:
|
||||
return None
|
||||
try:
|
||||
return int(args[0])
|
||||
value = int(args[0])
|
||||
except (TypeError, ValueError):
|
||||
return None
|
||||
if value < _INT32_MIN or value > _INT32_MAX:
|
||||
return None
|
||||
return value
|
||||
|
||||
|
||||
def _truncate(s, limit: int) -> str | None:
|
||||
|
||||
@@ -45,7 +45,7 @@ def _reset_caches(monkeypatch):
|
||||
async def test_queues_returns_all_known_queues(client, monkeypatch):
|
||||
def _fake():
|
||||
return {
|
||||
"queues": {q: 0 for q in activity_module._QUEUE_NAMES},
|
||||
"queues": dict.fromkeys(activity_module._QUEUE_NAMES, 0),
|
||||
"fetched_at": datetime.now(UTC).isoformat(),
|
||||
}
|
||||
monkeypatch.setattr(activity_module, "_read_queues_sync", _fake)
|
||||
@@ -75,7 +75,7 @@ async def test_queues_cached_within_ttl(client, monkeypatch):
|
||||
async def test_queues_redis_failure_returns_null_per_failing_queue(client, monkeypatch):
|
||||
"""_read_queues_sync's per-queue try/except returns None on failure."""
|
||||
def _partial():
|
||||
out = {q: 0 for q in activity_module._QUEUE_NAMES}
|
||||
out = dict.fromkeys(activity_module._QUEUE_NAMES, 0)
|
||||
out["ml"] = None # simulate broker hiccup on the ml queue
|
||||
return {"queues": out, "fetched_at": "x"}
|
||||
monkeypatch.setattr(activity_module, "_read_queues_sync", _partial)
|
||||
|
||||
@@ -202,35 +202,46 @@ def test_target_id_extracted_from_first_int_arg(args, expected):
|
||||
assert _target_id_from_args(args) == expected
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_retry_signal_records_retry_status(db_sync, monkeypatch):
|
||||
"""task_retry should mark the running row as 'retry' status."""
|
||||
from backend.app.tasks.thumbnail import generate_thumbnail
|
||||
def test_finalize_sets_retry_status_with_retry_count(db_sync):
|
||||
"""Direct unit-shape test of _finalize's retry path. We can't
|
||||
reliably trigger Celery's task_retry signal under eager mode with
|
||||
monkeypatched .run (the bind=True 'self' injection breaks), so
|
||||
test the finalize helper directly — that's the code the retry
|
||||
signal handler delegates to anyway."""
|
||||
from datetime import UTC, datetime
|
||||
|
||||
call_count = [0]
|
||||
from backend.app.celery_signals import _finalize
|
||||
from backend.app.models import TaskRun
|
||||
|
||||
def _retry_then_succeed(self_, *a, **k):
|
||||
call_count[0] += 1
|
||||
if call_count[0] == 1:
|
||||
raise self_.retry(exc=RuntimeError("first attempt"), countdown=0)
|
||||
return {"status": "ok"}
|
||||
|
||||
monkeypatch.setattr(
|
||||
"backend.app.tasks.thumbnail.generate_thumbnail.run",
|
||||
_retry_then_succeed,
|
||||
# Seed a 'running' row that _finalize can target.
|
||||
row = TaskRun(
|
||||
celery_task_id="retry-test-tid",
|
||||
queue="ml",
|
||||
task_name="backend.app.tasks.fake.t",
|
||||
target_id=999_992,
|
||||
started_at=datetime.now(UTC),
|
||||
status="running",
|
||||
)
|
||||
try:
|
||||
generate_thumbnail.delay(999_992).get(timeout=10, propagate=False)
|
||||
except Exception:
|
||||
pass
|
||||
db_sync.add(row)
|
||||
db_sync.commit()
|
||||
|
||||
# Expect multiple rows for the same target_id: one with status='retry'
|
||||
# for the first attempt, one with status='ok' for the second.
|
||||
statuses = db_sync.execute(
|
||||
select(TaskRun.status).where(TaskRun.target_id == 999_992)
|
||||
.order_by(TaskRun.id.asc())
|
||||
).scalars().all()
|
||||
assert "retry" in statuses
|
||||
_finalize(
|
||||
"retry-test-tid", status="retry",
|
||||
error_type="RuntimeError",
|
||||
error_message="first attempt",
|
||||
retry_count=1,
|
||||
)
|
||||
|
||||
# Column-select assertions — _finalize mutated the row via its own
|
||||
# session; re-read fresh state through db_sync.
|
||||
db_sync.expire_all()
|
||||
status, retry_count, error_type = db_sync.execute(
|
||||
select(TaskRun.status, TaskRun.retry_count, TaskRun.error_type)
|
||||
.where(TaskRun.celery_task_id == "retry-test-tid")
|
||||
).one()
|
||||
assert status == "retry"
|
||||
assert retry_count == 1
|
||||
assert error_type == "RuntimeError"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
Reference in New Issue
Block a user