Skip to content

use aclose for arq pool shutdown#387

Merged
SoundMindsAI merged 2 commits into
SoundMindsAI:mainfrom
Yashi248:fix-arq-pool-aclose
Jun 2, 2026
Merged

use aclose for arq pool shutdown#387
SoundMindsAI merged 2 commits into
SoundMindsAI:mainfrom
Yashi248:fix-arq-pool-aclose

Conversation

@Yashi248
Copy link
Copy Markdown
Contributor

@Yashi248 Yashi248 commented Jun 2, 2026

Summary

  • Replaced deprecated arq_pool.close() with arq_pool.aclose() in API lifespan shutdown and worker shutdown.
  • Added regression coverage asserting aclose() is awaited and close() is not called.

Verification

  • uv run pytest -o cache_dir=.pytest-cache-local backend/tests/unit/test_main_lifespan.py backend/tests/unit/test_workers.py - 14 passed
  • uv run ruff check backend/app/main.py backend/workers/all.py backend/tests/unit/test_main_lifespan.py backend/tests/unit/test_workers.py - passed
  • rg -n "arq_pool\.close\(\)" backend/app backend/workers - no matches of deprecated calls

@Yashi248 Yashi248 requested a review from SoundMindsAI as a code owner June 2, 2026 01:20
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces deprecated arq_pool.close() calls with arq_pool.aclose() in both the application lifespan and worker shutdown hooks, and adds corresponding unit tests. The feedback highlights that in the new tests, asserting that close is not called is currently unreachable because calling and awaiting a non-async MagicMock would raise a TypeError first. To make these assertions robust, the reviewer suggests mocking close as an AsyncMock as well.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread backend/tests/unit/test_main_lifespan.py
Comment thread backend/tests/unit/test_workers.py
Signed-off-by: Yashi248 <yashveldanda2811@gmail.com>
@Yashi248 Yashi248 force-pushed the fix-arq-pool-aclose branch from 39ffb5b to 492942c Compare June 2, 2026 01:27
@SoundMindsAI
Copy link
Copy Markdown
Owner

Review adjudication — ready to merge

Replacing the deprecated arq_pool.close() with aclose() is correct: arq 0.28's ArqRedis extends redis-py 5.3.1's Redis, where close() is decorated @deprecated_function(version="5.0.1", reason="Use aclose() instead"). Both pool-shutdown sites are covered (backend/app/main.py lifespan + backend/workers/all.py on_shutdown) — no other sites missed, and the openai_client.close() calls are correctly left alone (the OpenAI SDK's close() is not deprecated).

Gemini Code Assist findings

# File / line Finding Verdict Notes
1 test_main_lifespan.py:86 close.assert_not_called() is shadowed by a TypeError in a regression scenario (awaiting a non-async MagicMock) Accept — addressed by author fake_pool.close now mocked as AsyncMock, so the negative assertion fails cleanly instead of crashing.
2 test_workers.py:120 Same issue in the worker-shutdown test Accept — addressed by author Same fix applied.

Both findings resolved by the author in 492942c. Verified locally on the updated head: 14/14 unit tests pass, ruff clean, DCO sign-off intact, single Conventional Commit.

CI

All code-validating jobs green (backend lint/typecheck/tests/coverage, frontend, docker buildx, license, DCO, secrets-defense). The smoke job is red only because forked PRs cannot access the OPENAI_API_KEY_TEST repository secret — an environmental fork limitation, not a code defect, and a non-required check. Branch was updated onto current main (30a4d0b9) before this run.

Merging.

@SoundMindsAI SoundMindsAI merged commit 2e49ac9 into SoundMindsAI:main Jun 2, 2026
12 of 13 checks passed
@Yashi248 Yashi248 deleted the fix-arq-pool-aclose branch June 2, 2026 01:53
SoundMindsAI added a commit that referenced this pull request Jun 2, 2026
…-refs (#411)

Surfaced merging PR #387 (chore_arq_pool_aclose_deprecation): the smoke
job hard-fails on every external-fork PR because forked PRs can't read
OPENAI_API_KEY_TEST and the secret sanity-check exit 1s. Captured as
infra_smoke_fork_pr_secret_skip; cross-referenced bidirectionally with
infra_smoke_reseed_runtime_budget (sibling smoke-red issue) and added a
shipped postscript + forward-ref to chore_arq_pool_aclose_deprecation.

Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

2 participants