test(rekognition): fix intermittent full-suite CI failure on the off-loop read test (remediates #1205) - #1208
test(rekognition): fix intermittent full-suite CI failure on the off-loop read test (remediates #1205)#1208groupthinking wants to merge 3 commits into
Conversation
boto3 is a synchronous SDK. Every Rekognition call in AWSRekognition was issued directly inside an `async def`, so each one blocked the event loop for a full network round-trip. `_wait_for_job_completion` is the worst case: it polls every 5s for up to 600s, so a single video analysis could stall the loop up to 120 times. All 14 boto3 calls now dispatch via `await asyncio.to_thread(...)`, and the local-image read in `_prepare_image_input` goes through a new module-level `_read_file_bytes` helper on the same path. - 89 pre-existing tests pass with zero edits - 6 new heartbeat tests (`TestRekognitionDoesNotBlockEventLoop`); 5 of the 6 discriminate, proven by reverting both dimensions simultaneously (5 targeted failures / 90 passed) - ruff: exact parity with origin/main (8 pre-existing findings, 0 added) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…elapsed ticks The heartbeat form of this one test failed on CI. Unlike the four boto3 tests, which drive a controllable 0.12s mock, the local file read is a few microseconds of real work, so "did the loop tick while it ran" is a load-sensitive proxy rather than a property. Assert the property directly instead: record `threading.get_ident()` inside `_read_file_bytes` and require it to differ from the thread running the event loop. That is exactly what "dispatched off the loop" means, needs no sleeps, and cannot flake under runner contention. - 95 tests pass (89 pre-existing, unmodified, + 6 new) - Non-vacuity: calling `_read_file_bytes` directly instead of via `asyncio.to_thread` yields exactly 1 targeted failure / 94 passed - Suite runtime for the file drops to 0.85s (the 0.12s sleep is gone) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…mported alias
The off-loop assertion for the local image read failed intermittently in the
full CI suite ("_read_file_bytes was never called") while passing in isolation
and in the single-file run.
Root cause: the test re-imported the provider module inside the test body and
patched that alias, then called `_prepare_image_input` on a provider built from
the module-level import. Under EventRelay's dual-import hazard
(`src.youtube_extension` vs `youtube_extension` resolve to two distinct module
objects with independent dicts), the alias can differ from the dict backing the
running coroutine's bare `_read_file_bytes` lookup — so the patch silently
no-ops, the real read runs, and the recorder never fires.
Fix: derive the patch target from the provider we actually call
(`type(provider)._prepare_image_input.__globals__`). A function's `__globals__`
is frozen to its defining module at def-time and is exactly the dict the
name lookup uses, making the patch identical to that dict by construction and
immune to import-path / module-identity mismatch. The off-loop property
assertion is unchanged.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: BLOCKEDReasons: Machine-readable verdict{
"details": {
"invalid_fields": [
"policy.agent_login",
"policy.run_id"
]
},
"reasons": [
"invalid_payload"
],
"verdict": "blocked"
} |
Remediation status (head
|
|
Update: the Generated by Claude Code |
|
Contained as a duplicate repair. Its proven |
2 participants
Canonical issue
Closes #1204 (remediates the failing
testCI gate on #1205)This branch is a superset of #1205 (
perf/rekognition-offloop): the same two Rekognition off-loop commits plus one test-only fix. It exists because the automation is constrained to develop onclaude/determined-maxwell-u91xgxand may not push to #1205's head branch. A maintainer should either merge this in place of #1205, or apply the single-file fix below to #1205's branch and close whichever becomes redundant — the two must not both land.Outcome
The Rekognition provider's local-image read is asserted to run off the event loop by a deterministic thread-id check. That assertion was flaky in the full CI suite (
_read_file_bytes was never called) while passing in isolation and in the single-file run. This makes the assertion robust so thetestgate is green, with no change to production behaviour.Scope
tests/unit/test_aws_rekognition_provider.py—test_local_image_read_runs_off_the_event_loop_threadnow patches the running method's own__globals__instead of a separately re-imported module alias.aws_rekognition.pyor any production code; the off-loop property assertion itself is unchanged.Root cause
The test re-imported the provider module inside the test body and patched that alias, then invoked
_prepare_image_inputon a provider built from the module-level import. Under EventRelay's documented dual-import hazard (src.youtube_extensionvsyoutube_extensionresolve to two distinct module objects with independent dicts), the alias can differ from the dict backing the running coroutine's bare_read_file_byteslookup. The patch then silently no-ops, the real read runs, and the recorder never fires — exactly the observedassert 'thread_id' in {}.The fix derives the patch target from the provider actually under test —
type(provider)._prepare_image_input.__globals__. A function's__globals__is frozen to its defining module atdef-time and is precisely the dict the name lookup uses, so the patch is identical to that dict by construction and immune to import-path / module-identity mismatch.Risk
Verification
Tied to head
2997873:TestRekognitionDoesNotBlockEventLoop(6) and full file (95) pass locally.__globals__the method resolves in (recorder fires, off-loop, bytes intact).testnow that the sole failure is fixed;gitleaks,dependency-review, and the agent-completion gates are pre-existing repo-wide reds tracked by other PRs (fix(security): scope uv.lock checksum false-positive allowlist (#1141) #1142/fix(ci): stop gitleaks flagging uv.lock PyPI package hashes #1197, fix(deps): drop phantom python-jose to clear unfixable ecdsa advisory #1156, fix(ci): report Agent Lock gate as neutral when trust policy is unprovisioned #1151/fix: scope agent gate applicability to real dispatch evidence #1154), not caused by this diff.Production evidence
Not applicable — test-only change. The underlying perf change's production import path is documented in #1205.
Agent handoff
main)Generated by Claude Code