feat: guaranteed per-job child teardown (3.1.0) - #6
Merged
Conversation
Add unit tests exercising the per-job child's lifecycle directly against the wrapped on_job_start/on_job_end hooks and the inject wrapper: teardown must happen even when on_job_end is skipped (return and raise paths), a nested @Inject call must not close a child it doesn't own, and on_job_end's close must remain a no-op safety net when the wrapper already closed it. Update test_setup_di_composes_with_user_hooks for the new contract: the child is closed after on_job_start (built unopened) and open only within an @Inject wrapper's span, so resolve_job now goes through @Inject instead of resolving directly against the child. These fail against the current implementation, which opens the child in on_job_start and closes it unconditionally in on_job_end.
on_job_start built and opened the per-job REQUEST child, and on_job_end closed it unconditionally — but arq's run_job doesn't call on_job_end in a finally, so an unpicklable job result/exception (after the task, before on_job_end) or a user on_job_start hook that resolves-then-raises could leak the child's REQUEST-scope finalizers. Move open/close ownership into the inject wrapper, which runs inside arq's own caught try and is therefore guaranteed to run its finally even when on_job_end is later skipped: - on_job_start now builds the child unopened. - inject's wrapper opens the child only if it's closed (ownership) and closes it in a finally only if it opened it — safe under nested @Inject, since an inner call sees an already-open child and doesn't own it. - on_job_end becomes a safety net: it closes the child only if still open, which is now a no-op in the ordinary case. Contract change: the per-job child is open only within an @Inject task body, not across the whole on_job_start->on_job_end span, so a user on_job_start/on_job_end hook can no longer resolve from it (the private _CHILD_CONTAINER_KEY was never a supported path; @Inject is). Public API (setup_di, inject, FromDI, fetch_di_container) is unchanged.
Promote the new per-job child lifecycle (unopened at on_job_start, owned open/close in the inject wrapper, on_job_end as a safety net) into architecture/dependency-injection.md and README.md, and add the 3.1.0 release notes plus a lightweight planning change bundle.
Was still describing on_job_start/on_job_end as building and closing the child; update to describe the child as unopened, owned by @Inject's wrapper, with on_job_end as a safety net only.
Reproduces a use-after-finalize: a non-@Inject job coroutine that asyncio.gathers two @Inject helpers over the same ctx lets the faster sibling's boolean owns=child.closed check believe it alone opened the child, so it closes it (running REQUEST finalizers) while the slower sibling is still mid-body. Fails against the current boolean-ownership wrapper.
Replace the boolean owns=child.closed check in inject's wrapper with a per-job depth counter on ctx (_CHILD_DEPTH_KEY): open on the 0->1 transition, close on the 1->0 transition. The check/open/increment (and decrement/close) run with no await between them, so a single @Inject call's entry/exit is atomic from asyncio's point of view. This keeps nested @Inject safe (as before) and additionally makes concurrent @Inject fan-out via asyncio.gather over the same ctx safe: the child is closed exactly once, by whichever sibling is last to exit, regardless of which one opened it or finished first.
Update architecture/dependency-injection.md's "Per-job scope" section, README.md's usage summary, planning/releases/3.1.0.md's Fix note, and the guaranteed-child-teardown change bundle summary to describe the reference-counted open/close (nested and concurrent @Inject fan-out share one open child, closed once by the last to exit) instead of the earlier boolean-ownership model, which only handled nesting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
on_job_startopened the per-jobScope.REQUESTchild andon_job_endclosed it unconditionally. arq'srun_jobdoes not callon_job_endinside afinally, so two narrow paths could skip it and leak the child's REQUEST-scope finalizers:on_job_end.on_job_starthook resolves-then-raises.This moves open/close ownership into
inject's wrapper, which is the task body and runs inside arq's own caughttry, so afinallythere is guaranteed to run even whenon_job_endis later skipped:on_job_startnow builds the child unopened.inject's wrapper opens the child only if it's closed (owns = child.closed) and closes it in afinallyonly if it opened it — this ownership check is what makes nested@injectsafe (an inner call sees an already-open child, doesn't own it, doesn't close it).on_job_endbecomes a safety net: it closes the child only if still open, normally a no-op.Contract change (documented in
architecture/dependency-injection.mdand the 3.1.0 release notes): the per-job child is now open only during an@injecttask body, not across the wholeon_job_start→on_job_endspan, so a useron_job_start/on_job_endhook can no longer resolve from it directly._CHILD_CONTAINER_KEYwas always private;@injectremains the supported path.Public API (
setup_di,inject,FromDI,fetch_di_container) is unchanged. This isplanning/releases/3.1.0.md— nopyproject.tomlbump (tag-driven versioning).Test plan
tests/test_jobs.py: guaranteed teardown withouton_job_end(return and raise paths), nested@inject(inner doesn't close a child it doesn't own),on_job_endsafety-net path.tests/test_lifespan.py::test_setup_di_composes_with_user_hooksupdated for the new contract (child closed aton_job_start/on_job_endtime).just test-ci— 18 tests, 100% line coverage.just lint-ci—ruff,ty,planning/index.py --checkclean.🤖 Generated with Claude Code