Skip to content

fix(tests): eliminate CI-only embedding batch race by waiting for terminal status - #1025

Draft
seonghobae wants to merge 5 commits into
mainfrom
fix/embedding-batch-race-wait-timeout
Draft

fix(tests): eliminate CI-only embedding batch race by waiting for terminal status#1025
seonghobae wants to merge 5 commits into
mainfrom
fix/embedding-batch-race-wait-timeout

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Summary

  • Fix a CI-only race in tests/test_provider_embedding_batch_backend.py: test_unknown_tokenizer_uses_authoritative_provider_usage and test_unknown_tokenizer_byte_bound_never_becomes_recorded_usage called CostRoutingCoordinator.complete_embeddings_batch() against a provider (async, ThreadPoolExecutor-backed) embedding agent without wait_timeout, then asserted on the returned document's total_tokens immediately.
  • ProviderEmbeddingBatchBackend.submit() hands the job to a background ThreadPoolExecutor and returns immediately; without wait_timeout, complete_embeddings_batch() polls the job document right away. Under light/isolated load the background thread usually wins the race, but under CI's heavier concurrent load (full suite, thousands of tests) it sometimes hadn't finished, so the poll returned a non-terminal document shape that omits total_tokens entirely, producing KeyError: 'total_tokens'.
  • This is exactly the pattern sibling tests in the same file already guard against correctly (test_runtime_added_remote_embedding_member_uses_provider_backend, test_provider_embedding_requests_are_sharded_by_the_existing_token_limit both pass wait_timeout=1 for the same provider-backend-plus-immediate-check shape).

Root cause and diagnosis

  • Confirmed both tests pass reliably in isolation on current main (single-file run, single-test run), matching a race rather than a deterministic logic bug.
  • Reproduced deterministically by temporarily adding an artificial delay to ProviderEmbeddingBatchBackend._run_job (job execution): both tests failed with the exact same KeyError: 'total_tokens' seen on CI (RED), and passed once wait_timeout=1 was added to the complete_embeddings_batch() calls (GREEN), with the artificial delay still in place. The delay was reverted before committing — the only committed change is the two wait_timeout=1 additions plus a CHANGELOG entry.
  • This surfaced while triaging PR feat(discovery): add bounded OpenRouter free canary #985's failed "Full unit and contract suite" check: that PR is docs-only (CHANGELOG.md, README.md, docs/product-technical-gap-baseline.md), so the failure could not be its own defect. Once merged, the fix will be ported into feat(discovery): add bounded OpenRouter free canary #985 (and any other PR hitting the same flake) by merging main in.

Verification

  • pytest tests/test_provider_embedding_batch_backend.py -q -k "test_unknown_tokenizer_uses_authoritative_provider_usage or test_unknown_tokenizer_byte_bound_never_becomes_recorded_usage" — RED with the artificial delay before the fix, GREEN after.
  • pytest tests/test_provider_embedding_batch_backend.py tests/test_batch_routing.py tests/test_cost_router.py -q — 72 passed (no artificial delay).

🤖 Generated with Claude Code

https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4


Generated by Claude Code

…minal status

complete_embeddings_batch() against a provider (ThreadPoolExecutor-backed,
asynchronous) embedding agent returns as soon as the job is submitted unless
a wait_timeout is passed. Two tests in
test_provider_embedding_batch_backend.py asserted on the returned document's
total_tokens immediately, without wait_timeout, racing the background
executor thread. Reproduced deterministically by adding an artificial delay
to job execution: the assertion failed with KeyError: 'total_tokens' (the
non-terminal document shape) exactly as seen on CI, and passed once
wait_timeout=1 was added -- matching the pattern already used by sibling
tests in the same file for this exact shape.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae seonghobae added area: ci-cd CI, GitHub Actions, checks, release, or supply chain priority: high status: draft type: maintenance Maintenance, build, dependency, or operational upkeep labels Sep 2, 2026 — with ChatGPT Codex Connector
Preserve the exact provider embedding batch wait-timeout regression fix while
adopting current protected main non-destructively. Record the change in the
current CHANGELOG.d fragment convention instead of carrying the stale
monolithic CHANGELOG.md edit.

Copy link
Copy Markdown
Contributor Author

Protected-base reconciliation completed non-destructively.

  • previous PR head: 0b1477fc2f935370669a259b1b9bdabd38bcbdd0
  • protected main: 464da4715b495b5eaaa593eba3796e2d976ee0c9
  • reconciled exact head: dcfd3ec3d2ae9985b00b99e19fe967dfe6c11d61
  • ancestry: the new commit is a two-parent descendant of both the prior PR head and current protected main; no force-push or destructive rebase was used.

Semantic preservation was checked before the ref move: intervening main changes did not touch tests/test_provider_embedding_batch_backend.py. The prior monolithic CHANGELOG.md edit was intentionally not carried because protected main has moved this stacked work to CHANGELOG.d; the equivalent release note now lives at CHANGELOG.d/embedding-batch-race-wait-timeout.md.

Fresh base→head diff is exactly two files: the changelog fragment and the provider embedding batch regression test (+2/-2 in the test). PR is mechanically mergeable and remains Draft until exact-head required checks/reviews are terminal GREEN.

seonghobae and others added 2 commits September 3, 2026 00:32
Adopt main@212ff437 without rewriting history; the main admin-console delta is disjoint from the provider embedding batch test repair.
Bring in main's already-landed fix for the pre-existing
tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events
NameError (missing 'import json', commit 39a4348 / #1035) that PR #1025 was
still behind on. PR #1025's own diff (CHANGELOG.d fragment +
tests/test_provider_embedding_batch_backend.py) is untouched by this merge.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4

Copy link
Copy Markdown
Contributor Author

Triage: "Full unit and contract suite" failure on run 33649256377 (head 1797a0d0)

Not this PR's defect. The only failure was:

FAILED tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events
NameError: name 'json' is not defined. Did you forget to import 'json'
1 failed, 3350 passed, 2 skipped

tests/test_admin_contract.py is untouched by this PR's diff (only CHANGELOG.d/embedding-batch-race-wait-timeout.md and tests/test_provider_embedding_batch_backend.py change here). The bug was introduced on main at 212ff437 ("fix(admin): refresh audit after model-group mutations", merged during queue saturation without a clean check run) — a missing import json plus two other JS-extraction bugs in that same test. This PR's branch had merged main at exactly that commit, so it inherited the break.

Confirmed independently by reproducing the exact NameError on an isolated clone of this PR's then-head (1797a0d0) running only tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events.

Already fixed on main by 39a4348e (#1035), merged 2026-09-03T02:22Z — adds the missing import json/shutil/subprocess and corrects the JS source-extraction boundaries. This PR was simply behind that fix (mergeable_state: behind, no real conflicts — verified with git merge-tree).

Shared root cause with #1027: the parallel investigation on #1027 found the same pre-existing tests/test_admin_contract.py NameError, already fixed on main via the same 39a4348e/#1035. Both PRs were just behind main the same way.

Fix applied

Merged current main (394bf4ed) into this branch with an ordinary two-parent merge commit (b2c58154, non-force push) — no conflicts, no changes to this PR's own diff (still exactly the changelog fragment + the two wait_timeout=1 additions).

Verification (isolated worktree at the new head, Python 3.11 since this sandbox's egress policy blocks the fast-mlsirm GitHub-archive dependency that's gated to Python ≥3.12 anyway):

  • pytest tests/test_admin_contract.py3 passed (previously 1 failed here)
  • pytest tests/test_provider_embedding_batch_backend.py tests/test_batch_routing.py tests/test_cost_router.py72 passed (this PR's own targeted tests, unaffected)
  • Full suite pytest -q3385 passed, 2 skipped, only 2 failures, both attributable to this sandbox lacking fast-mlsirm/Python 3.12 (ModuleNotFoundError: fast_mlsirm and a downstream usage_source fallback assertion) — not present in the original CI run's 3350-passed baseline and not related to this change.

Fresh required checks are queued on the new head (b2c581546ec51f7b1efb9afc2fc6943b24864c94) as of this push.


_Generated by Claude Code


Generated by Claude Code

@seonghobae

Copy link
Copy Markdown
Contributor Author

Adjudication evidence (host 1 session, 2026-09-06 KST; full report with commands in #1080). Nothing here closes, flips, or retargets anything — the decision is the opener's.

Near-duplicate of #1044 — not verified as superseded, so not closable on this evidence. Both make the same wait_timeout=1 edits in tests/test_provider_embedding_batch_backend.py; the reverse-apply fails both ways on #1044's comment block (:127-137) and on this PR's CHANGELOG.d/embedding-batch-race-wait-timeout.md, which is this PR's only unique delta. Cheapest resolution: narrow this PR to that fragment (or fold the fragment into #1044) and then retire one of the two. Draft hold respected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci-cd CI, GitHub Actions, checks, release, or supply chain maintenance priority: high status: draft type: maintenance Maintenance, build, dependency, or operational upkeep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants