Skip to content

refactor(api): route every async dispatch through one off-loop helper - #162

Merged
Valtora merged 1 commit into
mainfrom
refactor/off-loop-task-dispatch
Jul 28, 2026
Merged

refactor(api): route every async dispatch through one off-loop helper#162
Valtora merged 1 commit into
mainfrom
refactor/off-loop-task-dispatch

Conversation

@Valtora

@Valtora Valtora commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Pull Request

Description

Completes the follow-up ADR-0007 named, and tidies the test seams so the suite exercises one dispatch path rather than six. No new dependencies. Behaviour is unchanged and re-measured.

v2.1.0 bounded how long an unreachable Redis can block a dispatch, and moved one call site off the event loop. The other 33 were bounded but still inline, so during an outage each stalled its own request for up to ~6s.

What changed

One helper, backend/core/task_dispatch.py. dispatch_task for work whose result the response depends on, dispatch_task_best_effort for work the caller must not fail on. Every dispatch reachable from a request handler now goes through it.

That covers the 33 remaining sites, plus two helpers that were synchronous but only ever called from async code, which the original count missed:

  • enqueue_model_preparation — blocked twice, because it also wrote download progress to Redis inline.
  • _enqueue_push_channel_refresh — its own docstring says it exists so "API paths stay fast", which the blocking publish quietly undermined.

asyncio.to_thread rather than Starlette's run_in_threadpool, for two reasons beyond taste:

  • calendar_service is imported by the worker, whose image ships no ASGI stack (there is already an explicit try/except ModuleNotFoundError around its FastAPI import). A Starlette import in a shared helper would not resolve there.
  • The loop's default executor is separate from the anyio limiter that serves sync route handlers, so a Redis outage can no longer consume the threads those handlers need. That closes the threadpool residual ADR-0007 recorded, rather than merely shrinking it.

Best-effort dispatch is now a named function rather than a bare except Exception repeated at each site, so the choice to swallow is visible where it is made instead of inferred from a try block.

Worker-side code still dispatches inline, deliberately. It has no event loop to protect, and the guard below only inspects async def.

Enforced, not remembered

test_no_api_code_dispatches_celery_work_on_the_event_loop walks the tree and fails on any send_task, apply_async or delay inside an async def, naming file and line. That is what stops this regressing the next time someone adds an endpoint.

Test alignment

Four test files reached for a module's celery_app re-export (system.celery_app, routes_chat.celery_app, cli_oauth.celery_app) and broke the moment the import moved. That is the same import-time binding mistake that made the suite slow in #159, in a different costume. They now patch the shared app object, which holds however the caller reached it. test_cli_oauth_api.py also had hand-rolled save/restore around those attributes; the references are repointed so the pattern is consistent with the rest.

Two more stubbed enqueue_model_preparation with sync lambdas that are now awaited.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that changes existing behaviour)
  • Documentation update

Refactor. No behaviour change intended or measured; the externally visible contract is the one ADR-0007 already set.

Checks run

  • Backend tests: source .venv/bin/activate && pytest
  • Python quality: python scripts/check.py (Ruff lint, format check, mypy, doc and Alembic validators)
  • Frontend lint: cd frontend && npm run lint
  • Frontend unit tests: cd frontend && npm run test
  • Frontend build: cd frontend && npm run build
  • Docs validation: python3 scripts/validate_docs.py
  • Alembic validation: python3 scripts/validate_alembic.py

1151 tests pass and the gate is green end to end. No frontend file is touched.

OK: lint, format, whitespace, filesize, heldpins, typecheck, docs, alembic, tests

Migration impact

  • No database migration in this PR.
  • Adds an Alembic migration.

Documentation impact

  • No documentation change required.

  • Updated the relevant guide(s) in the same PR.

  • ADR-0007 gains a dated Update section recording that the first two residuals are closed. The decision is unchanged, so the original text stands as written rather than being quietly rewritten to claim something it did not say at the time.

  • docs/ARCHITECTURE.md — the API/worker boundary now describes both halves, the off-loop dispatch and the bounded retries.

  • docs/DEVELOPMENT.md — a backend convention bullet, in the same list as the ML-import rule, so the next contributor meets the rule where they would look for it.

Security impact

  • No security-sensitive change.
  • Touches auth, tokens, encryption, capture ownership, or exposure.

No auth, token, encryption or ownership boundary is touched. As with #160 it slightly reduces a denial-of-service surface, since no request handler can now be blocked by a slow broker.

Manual verification

The guard catches a reintroduced inline dispatch. Reverting one call site to celery_app.send_task inside its async def:

E   AssertionError: Celery dispatched inline from an async def. Use
    backend.core.task_dispatch.dispatch_task instead:
E     backend/api/v1/endpoints/documents.py:123

It names the file and line. The violation was reverted after the check and is not in the diff.

The mechanism swap is behaviour-preserving. Re-ran the same uvicorn harness from #160 against a black-holed broker (10.255.255.1, packets dropped), now going through asyncio.to_thread:

Metric #160 (run_in_threadpool) This PR (asyncio.to_thread)
dispatch request 6.03s 6.03s
concurrent /ping, worst 0.00s 0.00s
concurrent /ping, median 0.0010s 0.0009s
pings stalled > 1s 0 of 60 0 of 60

A static sweep confirms the property holds tree-wide, both directions: no send_task/apply_async/delay remains inside any async def, and no await was introduced into a sync function.

The throwaway harness was removed afterwards; nothing from it is in the diff.

Screenshots (if relevant)

Not applicable.

ADR-0007 bounded how long an unreachable Redis can block a dispatch and
moved one call site off the event loop, leaving the other 33 bounded but
still inline. This finishes that follow-up and makes the property
enforceable rather than remembered.

Add backend/core/task_dispatch.py with dispatch_task and
dispatch_task_best_effort, and route every dispatch reachable from a
request handler through it. That covers the 33 remaining sites plus two
helpers that were synchronous but only ever called from async code.
enqueue_model_preparation is one, and it blocked twice, since it also wrote
download progress to Redis inline. _enqueue_push_channel_refresh is the
other, and its own docstring said it existed to keep API paths fast, which
the blocking publish undermined.

The helper uses asyncio.to_thread rather than Starlette's
run_in_threadpool, for two reasons beyond taste. calendar_service is
imported by the worker, whose image ships no ASGI stack, so a Starlette
import there would not resolve. And the loop's default executor is separate
from the anyio limiter that serves sync route handlers, so a Redis outage
can no longer consume the threads those handlers need. That closes the
threadpool residual ADR-0007 recorded rather than merely shrinking it.

Best-effort dispatch is now a named function rather than a bare except at
each site, so the choice to swallow is visible where it is made instead of
being inferred from a try block. Worker-side code still dispatches inline,
deliberately, having no event loop to protect.

A test walks the tree and fails on any send_task, apply_async or delay
inside an async def, naming file and line. Verified by reintroducing one
inline dispatch, which it caught at documents.py:123.

Align the tests on one seam while here. Four files reached for a module's
celery_app re-export, which is the same import-time binding mistake that
made the suite slow, and they broke as soon as the import moved. They now
patch the shared app object. Two more stubbed enqueue_model_preparation
with sync lambdas that are now awaited.

Behaviour is unchanged and re-measured against an unreachable broker: 6.03s
to fail a dispatch, with 60 concurrent no-op requests at a 0.9ms median and
a 0.00s worst case, matching the run before the mechanism swap. 1151 tests
pass.

Refs: docs/adr/0007-bounded-fail-fast-task-dispatch.md, docs/ARCHITECTURE.md
@Valtora
Valtora merged commit d3daa1e into main Jul 28, 2026
19 of 20 checks passed
@Valtora
Valtora deleted the refactor/off-loop-task-dispatch branch July 28, 2026 18:38
Valtora added a commit that referenced this pull request Jul 30, 2026
Bump docs/VERSION to 2.2.0 so the tag validates, and fill in the
release-notes template for this range instead of hand-editing the
published body afterwards, which is how v2.1.0 was done.

A minor bump rather than a patch: #168 adds a capture action that did
not exist before, stopping and processing a paused recording.

Refs: #162, #163, #165, #167, #168, #169, #170, #171
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