diff --git a/docs/adr/0001-no-connection-context-provider.md b/docs/adr/0001-no-connection-context-provider.md index 3711ba6..3dce6fb 100644 --- a/docs/adr/0001-no-connection-context-provider.md +++ b/docs/adr/0001-no-connection-context-provider.md @@ -1,26 +1,11 @@ # No connection `ContextProvider` for arq's `ctx` -**Decision:** this integration registers no connection provider. A task that needs job metadata -reads its `ctx` argument directly. - -Most modern-di integrations bind the framework object a unit of work carries — a Starlette -`Request`, a FastStream message, a gRPC `ServicerContext` — into a `ContextProvider`, so a -dependency deep in the graph can declare it and have it resolved rather than threaded through by -hand. The symmetry argument says arq's per-job `ctx` should be bound the same way. - -It should not, for two reasons that do not apply to those frameworks. First, `ctx` is not an -object: arq builds it per job as `{**worker.ctx, **job_ctx}`, a `dict[str, Any]` holding `job_id`, -`job_try`, `enqueue_time`, `score`, `redis`, and whatever the worker seeded — a bag with no type -worth declaring a dependency on, so the thing a `ContextProvider` normally buys (a named type a -factory can ask for) is absent. Second, `ctx` is not out of reach: arq calls a task as -`coroutine(ctx, *args, **kwargs)`, so every task already holds it as its first positional -parameter, unconditionally. Injecting it would add a second path to a value that is never missing -from the first one, and the integration would then own a provider whose whole job is to hand back -an argument the caller already has. - -`modern-di-celery` and `modern-di-typer` decline for the same reason. Not registering one is the -pattern for a framework whose per-unit context is a bag rather than an object. - -**Revisit trigger:** arq replaces the `ctx` dict with a structured context type, or a real request -arrives to resolve job metadata inside a factory several levels below the task — which is the case -that threading `ctx` by hand actually becomes painful, and the case that has not appeared yet. +Most modern-di integrations bind the framework object a unit of work carries, a Starlette `Request` +or a FastStream message, into a `ContextProvider` so a factory deep in the graph can declare it +instead of having it threaded through by hand, and the symmetry argument says arq's per-job `ctx` +should be bound the same way. Neither half of that argument holds here. arq builds `ctx` per job as +`{**worker.ctx, **job_ctx}`, a `dict[str, Any]` of `job_id`, `job_try`, `enqueue_time`, `score` and +`redis`, so there is no named type a factory could ask for, and it calls every task as +`coroutine(ctx, *args, **kwargs)`, so a task already holds it. A provider would only add a second +path to a value that is never missing from the first. What would reopen this is a factory several +levels below the task needing job metadata, which has not appeared. diff --git a/docs/adr/0002-no-auto-inject-sweep.md b/docs/adr/0002-no-auto-inject-sweep.md index 38de7b0..5868813 100644 --- a/docs/adr/0002-no-auto-inject-sweep.md +++ b/docs/adr/0002-no-auto-inject-sweep.md @@ -1,26 +1,12 @@ # No auto-inject sweep over `WorkerSettings.functions` -**Decision:** `@inject` is applied per task, explicitly. `setup_di` does not walk -`WorkerSettings.functions` wrapping what it finds. - -Wrapping every registered task at `setup_di` time is the obvious convenience, and it is what makes -`setup_di` a one-liner in integrations whose handler registry is homogeneous. arq's is not. -`functions` is a heterogeneous list by design: a plain coroutine, an `arq.func(...)` wrapper -carrying its own name/timeout/retry configuration, or an import string arq resolves later. A sweep -has to recognise each shape, unwrap it, wrap the coroutine inside, and rebuild the surrounding -object without dropping the configuration it carried — for an import string, without importing -anything, since arq's own resolution of it is what decides which module is loaded and when. - -That is three format-specific code paths against a pre-1.0 library, each of which fails by -silently not injecting rather than by raising, and each of which has to be revisited whenever arq -adds a fourth accepted shape. `@inject` at the definition site costs one line, fails loudly at -decoration when a task's signature cannot support it, and is visible in the task's own source to -anyone reading it. - -The convenience is also smaller here than it looks: `inject` already returns a task with no -`FromDI` parameter unchanged, so the sweep would be saving a decorator line only on the tasks that -actually take dependencies. - -**Revisit trigger:** arq narrows `functions` to a single shape, or gives its entries a documented -accessor for the underlying coroutine that survives a rewrite — at which point one code path, not -three, is enough, and the sweep can be added without a per-shape guess. +`@inject` is applied per task at the definition site: `setup_di` wraps the four lifecycle hooks and +never walks `WorkerSettings.functions` wrapping what it finds. A sweep is the obvious convenience, +and a one-liner in integrations whose handler registry is homogeneous, but arq's is not. An entry is +a plain coroutine, a `Function` record from `arq.func(...)` carrying its own name, timeout, result +retention and retry settings, or an import string `arq.func` resolves at worker construction, so a +sweep must recognise each shape, reach the coroutine inside, and rebuild the record without dropping +any of its six fields, against a pre-1.0 library, with every path failing by silently not injecting +rather than by raising. `@inject` costs one line, fails loudly at decoration, and is visible in the +task's own source; it also returns a task with no `FromDI` parameter unchanged, so a sweep would +save a line only where dependencies are declared. diff --git a/docs/adr/0003-per-job-child-teardown-is-reference-counted.md b/docs/adr/0003-per-job-child-teardown-is-reference-counted.md index 66346f7..2c5e2b5 100644 --- a/docs/adr/0003-per-job-child-teardown-is-reference-counted.md +++ b/docs/adr/0003-per-job-child-teardown-is-reference-counted.md @@ -1,33 +1,13 @@ # Per-job child teardown is reference-counted, not owned -**Decision:** `inject`'s wrapper tracks an open depth on the job's `ctx` and closes the per-job -child on the 1→0 transition. It does not decide, on entry, whether it is the call that owns the -child. - -Ownership is the smaller and more obvious mechanism, and it was the first one shipped here: a -boolean taken on entry (then, `owns = child.closed`), closing in `finally` only `if owns`. It reads -correctly and it handles the case that motivated moving teardown out of `on_job_end` — an outer -`@inject` task awaiting an inner `@inject` function over the same `ctx`, where the inner call must -not close a child the outer one is still using. - -It is wrong as soon as two `@inject` calls are concurrent rather than nested, and the fault is the -entry-time claim itself, not the state it happened to read. A job coroutine that is not itself -`@inject` can `asyncio.gather` two decorated helpers over the same `ctx`. Both enter before either -exits, so exactly one of them takes the claim and the other defers to it — and the claimant is the -one that entered first, which says nothing about which one exits last. When it is the faster -sibling, its `finally` runs the `Scope.REQUEST` finalizers while the slower sibling is still -awaiting with a resolved handle to what was just finalized. Nothing raises. The symptom is a -resource used after teardown, in the sibling that happened to be slower, which is why an -adversarial review rather than the suite found it. - -A count has no such entry-time question to get wrong: the child is closed by whichever call is last -to exit, whether the calls nested or overlapped, and whether or not it is the one that opened it. -Both transitions run with no `await` between the read and the write, so the event loop cannot -interleave another call into the middle of either. The invariant is -`test_concurrent_inject_fanout_shares_one_child` in `tests/test_jobs.py`, which is red under the -boolean version and green under the count. - -**Revisit trigger:** the child's lifetime stops being derived from `@inject` spans — arq gaining a -hook that is guaranteed to run after the task on every path, or modern-di growing a per-scope -context manager the wrapper could enter instead. Either removes the bookkeeping rather than -simplifying it, and the count goes with it. +`inject`'s wrapper keeps an open depth on the job's `ctx` and closes the `Scope.REQUEST` child on +the 1 to 0 transition. The boolean claim it replaced, `owns = child.closed` taken on entry and +honoured in `finally`, is smaller and handles the nested case that moved teardown out of +`on_job_end`, but it is wrong as soon as two `@inject` calls overlap rather than nest: a job +coroutine that is not itself decorated can `asyncio.gather` two decorated helpers over one `ctx`, +the claim goes to whichever entered first, and when that one also exits first it runs the +REQUEST-scoped finalizers while its sibling still holds a resolved handle, with nothing raising. A +count has no entry-time question to get wrong, and each transition reads and writes with no `await` +between, so the loop cannot interleave; `on_job_end` then closes the child only as a safety net for +a job that ran no wrapper. `tests/test_jobs.py::test_concurrent_inject_fanout_shares_one_child` is +red under the boolean and green under the count.