Skip to content

Reuse a per-process Redis connection in the ingest worker #1493

Description

@dark-sorceror

Summary

The ingest worker opens a fresh Redis connection for every batch it publishes. _publish_live_spans calls redis_lib.from_url(...) (backend/worker/ingest_tasks.py:37) and closes the connection at the end of each Celery task (:63). The in-code comment (:34-36) explains why: clients created before fork() crash Celery's prefork workers on macOS. The workaround is correct but costs a TCP connect + auth handshake per ingest batch in production.

How to replicate

  1. Run sustained ingest and watch redis-cli CLIENT LIST (or MONITOR): one new connection + auth per batch published.

Root cause (confirmed)

The fork-safety problem is real but overstated by the workaround: it only requires that the connection be created after fork — not per task. shared.redis.get_redis_client() is already a lazy singleton (backend/shared/redis.py:16-26): nothing constructs it at import time, and the Celery master never runs tasks, so first use inside a task creates the client post-fork in each child. It also already ships the reconnect story — retry_on_error=[ConnectionError, TimeoutError] and health_check_interval=30 (:23-24). (Dev additionally sets OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES for the worker, tmux_tools/launcher.py:298-301.)

Preferred fix: reuse get_redis_client() and update the stale comment. Alternative if explicit init is wanted: Celery's worker_process_init signal (fires once per child post-fork) — note the repo currently has no precedent for it (celery_app.py only uses worker_ready).

Impact

  • One TCP connect + auth per ingest batch — pure overhead on the hot publish path that grows linearly with ingest volume.
  • Extra connection churn on the Redis server under load.

Acceptance criteria

  • The worker uses one Redis connection per process (lazy post-fork singleton or worker_process_init); no per-task connect. The stale fork-safety comment at ingest_tasks.py:34-36 is updated to explain the lazy-post-fork reasoning.
  • A dropped connection is re-established on the next publish (free via get_redis_client()'s retry_on_error/health_check_interval), not left broken until worker restart.
  • The existing guarantee holds: a Redis/publish failure never fails the ingest task.
  • A new test patches the client and asserts publish behavior — today tests/worker/test_ingest_tasks.py neither mocks nor asserts on Redis, and _publish_live_spans swallows all exceptions, so existing tests pass no matter what; "existing tests pass" alone proves nothing here.
  • Works in macOS development (prefork) and production.

Same anti-pattern nearby, follow-up or fold in: detector_tasks.py:45-51 (_get_redis()) also connects per call (:382).

Metadata

Metadata

Assignees

Labels

needs approvalPending core-team approval — wait for this label to be removed before starting work.

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions