Stop large embedding backlogs from stalling on the CPU-only embedder

[JSv4]

What

EMBEDDER_BATCH_REQUEST_TIMEOUT_SECONDS was 60s while a batch of 100 long texts — whole ordinance sections and statute chapters, as produced by authority-pack ingestion — routinely takes longer than that on a CPU-only embedder. On timeout the client retries the entire batch three times, so each task burns ~3 minutes and requeues having made no progress.

How it surfaced

Ingesting a Fort Worth authority pack (881 sections) plus a 55-document corpus:

• a ~2,800-task queue draining at ~1 task/min with a continuous retry storm
• Hybrid search: no results from either arm on every query, because no annotation ever received an embedding
• the corpus agent silently fell back to general knowledge and answered a local-code question wrong

That last one is the reason this is filed as a bug rather than tuning: the failure is invisible at the API surface. Nothing errors. The agent just quietly stops being grounded.

Change

Constant	Before	After
EMBEDDER_BATCH_REQUEST_TIMEOUT_SECONDS	60	300
MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE	100	32
EMBEDDING_API_BATCH_SIZE	50	32

A slow batch now completes once instead of being redone three times. EMBEDDING_API_BATCH_SIZE drops to keep it <= the cap and satisfy the documents.E001 system check.

Measured, same queue, one worker

	before	after
retries in 4 min	continuous	0
tasks completed in 4 min	4	117

Direct timing for calibration: 100 short texts embed in 25.6s, so the old 60s ceiling left almost no headroom for long legal text.

Tests

manage.py check passes. Embedder suites run (138 tests).

opencontractserver/tests/test_batch_embedding.py and test_embedding_manager.py are intermittently failing on this machine (3 failures, vector dim 1536 != 3072) — verified pre-existing by stashing this change and re-running the same suites, which alternate FAILED/OK either way. Untouched here.

Also adds docs/test_scripts/fort_worth_homeowner_corpus.md, the manual procedure this surfaced under.

[claude]

Review

This is a small, well-scoped config change with strong evidence behind it (real ingest measurements, before/after retry counts). A few notes:

Code quality

• The comments added to opencontractserver/constants/document_processing.py explain the why clearly (worst-case sizing, blast-radius rationale) rather than restating the what — matches the repo's comment conventions well.
• EMBEDDING_API_BATCH_SIZE dropping to exactly 32 (equal to the new MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE) correctly keeps documents.E001 green, and test_api_batch_size_matches_service_cap / test_exceeding_max_batch_size_raises in test_batch_embedding.py derive from the constants directly, so they stay correct without edits.

Potential considerations (not blockers)

• Compounding retry math at the new timeout. sent_transformer_microservice.py's shared session configures a urllib3-level Retry(total=3, read=3, backoff_factor=1.0, ...) underneath Celery's own autoretry_for (max_retries=3, countdown=60, per embeddings_task.py). If a genuinely stuck/overloaded microservice (not just "long batch") causes repeated read timeouts, the worst case is now roughly 3x300s at the urllib3 layer plus Celery's 3x(60s + task time) on top — noticeably longer than the old worst case before the task is finally marked failed/dead-lettered. Reasonable tradeoff for the common case this PR targets, but worth a callout (or follow-up issue) that a genuinely-hung microservice now means tasks stuck ~15-20 min each instead of fast-failing.
• Batch-size/throughput tradeoff isn't quantified. Dropping MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE from 100 to 32 means ~3x more HTTP round-trips for the same corpus even in the fast/short-text case (fixed per-request overhead). The before/after numbers clearly show the retry-storm fix dominates, but it'd be worth confirming small-batch, all-short-text corpora (the common case) didn't regress meaningfully in throughput — likely fine given the 25s/100-texts baseline, but not explicitly re-measured at batch size 32.
• The comment at sent_transformer_microservice.py:122-124 ("server-side cap ... default 100") is still accurate post-change since the client cap (32) just sends smaller batches under the server's default ceiling — no inconsistency, just noting it didn't need updating and wasn't.

Test coverage

• No new automated test pins the specific tuned values (300s timeout / 32 batch size) beyond the existing <= relationship check — reasonable, since these are tuned magic numbers rather than invariants.
• PR body is transparent about pre-existing, unrelated intermittent failures in test_batch_embedding.py/test_embedding_manager.py (vector dim 1536 != 3072), verified pre-existing via stash/re-run — good practice per CLAUDE.md's "don't touch old tests without permission."
• docs/test_scripts/fort_worth_homeowner_corpus.md is a solid, detailed manual test script per the repo's docs/test_scripts/ convention, including a "Recorded run" section with pass/fail results. It also documents an unrelated finding (a cross-corpus reachability gap for authority corpora) — valuable, but arguably out of scope for this PR's stated fix; consider splitting into its own tracked issue so it doesn't get lost inside a test-script doc for an unrelated config change.

Security

No concerns — this is a constants-only change with no new input handling, auth, or permission surface.

Overall: solid, well-justified fix with good measurement backing it up. Nothing blocking.

[claude]

Review: Stop large embedding backlogs from stalling on the CPU-only embedder

Solid, well-diagnosed fix — the "retrying an already-slow batch 3x makes it worse, not better" root cause is convincing, and the before/after measurement (4 → 117 tasks/4min) is good evidence. A couple of things worth addressing before/after merge:

1. Stale docs/comments now describe a false invariant (docs/deployment/performance_tuning.md, sent_transformer_microservice.py:122-126)

MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE drops from 100 to 32, but the code comment right above where it's consumed still says:

# The local microservice caps batch size at MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE
# (server-side env var ``MAX_TEXTS_PER_BATCH``, default 100). Raising
# past it causes the service to 400 with "exceeds maximum"; pin
# api_batch_size to the cap so we use the full per-call capacity.
api_batch_size = MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE

(opencontractserver/pipeline/embedders/sent_transformer_microservice.py:122-126)

"pin ... to the cap so we use the full per-call capacity" is no longer true — the client cap (32) is now well below the documented server-side default cap (100), by design, to bound retry blast radius. That's a reasonable tradeoff, but the comment should say so instead of claiming the opposite.

Same drift in docs/deployment/performance_tuning.md:

• Line 59: MicroserviceEmbedder | 100 (= service cap) | 2 | ... matching saturates without queueing.
• Line 161: EMBEDDING_API_BATCH_SIZE (fallback) | 50
• Line 162: MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE | 100 | Set in tandem with the service-side MAX_TEXTS_PER_BATCH env var; raising one without the other 400s

There's also a test whose docstring encodes the old assumption — test_batch_embedding.py::TestMicroserviceEmbedderHardening.test_api_batch_size_matches_service_cap. The assertion itself still passes (it only compares two constants that were changed together), but the docstring ("api_batch_size must equal the service-side MAX_TEXTS_PER_BATCH cap") is no longer accurate now that the two are deliberately decoupled. Per CLAUDE.md's doc-maintenance rule ("keep docs current... update the relevant doc as part of the same change"), worth fixing all three in this PR so a future reader doesn't reintroduce 100 while "restoring" the documented invariant.

2. Worth double-checking: does the 300s timeout compound with existing retry layers more than the PR description accounts for?

sent_transformer_microservice.py's shared session mounts a urllib3 Retry with read=3, connect=3, total=3, backoff_factor=1.0 (lines 67-76). urllib3's read retry budget also covers read-timeout errors (not just 5xx/429 — those are gated separately via status_forcelist), so a single session.post(..., timeout=EMBEDDER_BATCH_REQUEST_TIMEOUT_SECONDS) call that keeps timing out can itself retry up to 3x at the urllib3 layer before the exception even reaches the Celery task. Stacked with the outer autoretry_for=(Exception,), retry_kwargs={"max_retries": 3, "countdown": 60} on the Celery task (embeddings_task.py), a batch that's genuinely stuck (not just slow) could now block a worker for up to roughly 300s × 3 (urllib3) × 4 (celery attempts) — tens of minutes in the worst case, versus ~3 minutes under the old 60s timeout described in the PR body. That's still strictly better for the measured case (slow-but-succeeds batches, which is the actual problem here), but it does change the worst-case pathological-input blast radius pretty significantly and isn't mentioned in the PR description. Might be worth a short note in the constant's comment (or a follow-up) acknowledging the compounding, even if no code change is needed right now.

Minor

• EMBEDDING_API_BATCH_SIZE (32) and MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE (32) are now equal, which pairs with the documents.E001 check being a strict > — valid, but it leaves zero headroom; any future bump to one without the other trips E001 immediately. Probably the intended tripwire, just flagging in case slack was meant to be left.
• No new/updated automated test asserts the new numeric values (300 / 32 / 32) directly the way test_api_batch_size_matches_service_cap pins api_batch_size to the constant — that indirection still holds so existing tests won't regress, but nothing would catch an accidental revert of the new values. Not blocking given this is a constants-only change plus a thorough manual test script.
• docs/test_scripts/fort_worth_homeowner_corpus.md is a valuable addition, but it documents an unrelated, still-open finding (the "authority corpus reachability gap" / tool-call omission) as a to-do for a different problem. Worth confirming that's tracked somewhere (issue) so it doesn't only live in a test-script file.

Overall: the core fix (raise timeout, shrink batch) is directionally right and well-justified by the measurements in the PR description — main ask is reconciling the now-stale "matches the service cap" language before merge.

[claude]

Review

Small, well-scoped fix backed by real production evidence (queue draining at ~1 task/min, silent fallback to general knowledge). Verified the change against the codebase:

Correctness

• EMBEDDING_API_BATCH_SIZE = 32 and MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE = 32 satisfy the documents.E001 system check (opencontractserver/documents/checks.py:248, <= not <), so manage.py check stays green.
• opencontractserver/tests/test_batch_embedding.py references MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE dynamically (e.g. line 633: range(MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE + 1)) rather than hardcoding 100, so it is not broken by the constant change. The other 50s in that file are literal args to _batch_embed_text_annotations, independent of the constant.
• The retried call sites all use retry_kwargs={"max_retries": 3, "countdown": 60} (opencontractserver/tasks/embeddings_task.py:308-361,727-728,957-958,1083-1084), consistent with the "retried three times" framing in the PR description.
• No hard Celery task_time_limit/soft_time_limit is configured (CELERY_TASK_TIME_LIMIT is commented out in config/settings/base.py), so bumping the HTTP read timeout to 300s will not get preempted by a task-level timeout and turn into a worse failure mode (e.g. SoftTimeLimitExceeded mid-request).

Design / tradeoffs worth naming explicitly (not blockers)

• Dropping the batch cap 100→32 while also raising the timeout 60s→300s is belt-and-suspenders — either alone would likely address the immediate symptom. Doing both is reasonable given the goal (bound the retry blast radius and give slow batches enough room), but it does mean a future tuning pass cannot tell from this PR alone which knob mattered more. Just flagging for whoever revisits this, not asking for a change.
• 32 is picked pragmatically ("worst realistic input" reasoning in the comment is good) rather than derived from a formula — fine for a hotfix, but it is a fixed constant regardless of embedder backend. If a GPU-backed microservice deployment is ever added, this same cap would be unnecessarily conservative there. Possibly worth a follow-up issue if multi-backend deployments are on the roadmap; out of scope here.

Test coverage

• No new automated test asserts the new constant values or exercises a slow-batch-vs-timeout scenario directly — the PR relies on the existing embedder suite (138 tests, said to pass) plus the manual verification script and measured throughput numbers. That is a reasonable bar for a pure constant tune guarded by the existing documents.E001 invariant check, but a small regression test (e.g. asserting the timeout is large enough relative to a fake slow embedder, or simply pinning the constant values) would make a future accidental revert-to-60s show up in CI instead of only in production.
• The intermittent test_batch_embedding.py / test_embedding_manager.py failures are called out and credibly shown to be pre-existing (verified by stashing the change) — good diligence, appropriately left untouched since unrelated.

Docs

• docs/test_scripts/fort_worth_homeowner_corpus.md and run_gold_questions.py are a very thorough manual test record, consistent with the "Manual Test Scripts" convention in CLAUDE.md. The write-up goes notably beyond this fixs scope — it documents a separate retrieval/grounding gap (authority corpora reachable only via resolved citation edges) as a follow-up architecture finding. That is valuable, but given its size and independence from the timeout fix, it might be worth splitting into its own tracked issue so it is not buried inside a doc file attached to an unrelated config PR.
• The changelog fragment (changelog.d/embedder-batch-timeout.fixed.md) follows the required naming/format convention correctly.

Security / perf

• No security concerns — this only changes HTTP client timeout and batch-size constants for a local/internal embedding microservice call.
• Perf impact is the intended fix: smaller batches mean more round trips at steady state, but the PRs own measurement (4 → 117 tasks/4min) shows the retry-storm elimination dominates decisively.

Overall: a solid, well-evidenced fix with good reasoning documented in the code comments. No blocking issues found.

[claude]

Review

Solid, narrowly-scoped fix — the diagnosis (60s timeout < realistic CPU-embedder latency for long legal text → whole-batch retries → queue stall) is well-documented in the PR description and commit messages, and the before/after measurement (4 → 117 tasks/4min, continuous retries → 0) is convincing. The changelog fragment follows the changelog.d/ convention correctly, and the manual test script under docs/test_scripts/ is a nice artifact per the repo's own guidance.

Comments worth a look

1. Stale rationale in MicroserviceEmbedder comment + test docstring (not touched by this PR, but now inconsistent with it)

• opencontractserver/pipeline/embedders/sent_transformer_microservice.py:122-126: the comment says "pin api_batch_size to the cap so we use the full per-call capacity", referencing a server-side MAX_TEXTS_PER_BATCH default of 100.
• opencontractserver/tests/test_batch_embedding.py:1412-1417 (test_api_batch_size_matches_service_cap) has a matching docstring: "pinning to the cap uses the full per-call capacity without ever asking for more than the service will accept."

Both of these describe MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE as tracking the server's cap. After this change, the client cap (32) is deliberately well below the server's actual cap (still 100 per the comment) — the constant now encodes a timeout/blast-radius budget, not "full per-call capacity." The test still passes (it's tautological — comparing the class attr to the constant it's assigned from), but the comment and docstring will mislead the next person who touches this file into thinking 32 is the service limit. Worth a follow-up doc tweak to say explicitly "capped below the service limit to bound worst-case batch latency," per the good comment already added in document_processing.py.

2. Compounding retry latency at the new timeout — worth confirming, not necessarily a blocker
sent_transformer_microservice.py layers two retry mechanisms: urllib3-level Retry(read=3, backoff_factor=1.0) on the shared session (lines 67-76), underneath Celery's own autoretry_for=(Exception,) with max_retries=3, countdown=60 at the task level. A ReadTimeoutError is retried by urllib3's Retry(read=3) before it ever surfaces as an exception to Celery. With the timeout now at 300s, a batch that's genuinely too slow (not just transiently slow) could block a single Celery task attempt for up to ~3×300s at the urllib3 layer alone, before Celery's outer retry (up to 3 more full attempts) even engages — a worst case of well over an hour of a worker thread being tied up on one pathological batch, versus the ~3×60s = 3 min described in the PR's "before" state. The new MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE=32 cap reduces how often this worst case is hit, but doesn't eliminate it (a single oversized row is still possible). Might be worth a quick gut-check on whether the urllib3-level read retry count should come down now that the per-attempt timeout is 5x larger, so the two retry layers don't multiply.

3. Test coverage — the empirical, real-ingest measurement in the PR description is good evidence, but there's no automated regression test pinning the new relationship between the three constants (beyond the existing documents.E001 system check, which only asserts EMBEDDING_API_BATCH_SIZE <= MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE, not the timeout). Not blocking — these are tuning constants, not logic — but a cheap test asserting EMBEDDER_BATCH_REQUEST_TIMEOUT_SECONDS is comfortably above some documented "worst realistic batch" benchmark could prevent future regressions from a well-intentioned "let's lower this back to 60" edit.

Nits

• The pre-existing intermittent test_batch_embedding.py / test_embedding_manager.py failures are correctly called out as unrelated (verified via stash) — good diligence, no action needed here.

Overall: low-risk, well-justified config change with real production evidence behind it. The two comment/docstring spots above are the only things I'd actually touch before merging; the retry-compounding point is more of a "worth knowing" than a required fix.

[claude]

Review

Solid, well-diagnosed fix — the write-up connecting the 60s timeout → whole-batch retry → silent grounding failure is exactly the kind of root-cause analysis this repo's CLAUDE.md asks for, and the before/after throughput numbers (4 → 117 tasks/4min) are convincing.

Correctness — looks right

• EMBEDDING_API_BATCH_SIZE (32) ≤ MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE (32) still satisfies documents.E001 (opencontractserver/documents/checks.py:248).
• OpenAIEmbedder.api_batch_size = 256 (opencontractserver/pipeline/embedders/openai_embedder.py:54) and MicroserviceEmbedder.api_batch_size = MICROSERVICE_EMBEDDER_MAX_BATCH_SIZE (opencontractserver/pipeline/embedders/sent_transformer_microservice.py:126) both override the class default, so the lowered global fallback doesn't quietly throttle the hosted OpenAI path — only the CPU-only microservice is affected, which is the actual target.
• EMBEDDER_BATCH_REQUEST_TIMEOUT_SECONDS is only read by sent_transformer_microservice.py:324, so it's scoped to the microservice embedder and doesn't touch OpenAI request timeouts.
• Existing tests (test_api_batch_size_matches_service_cap in test_batch_embedding.py:1412) assert the invariant generically against the constants rather than hardcoding 100/50, so they should keep passing without modification — consistent with what the PR description reports.

One thing worth a second look — worst-case task blocking time

sent_transformer_microservice.py wraps requests in a urllib3 Retry(total=3, backoff_factor=1.0, status_forcelist=(429, 502, 503, 504)) (lines 67-73). That retry budget also covers read timeouts on the underlying session.post(..., timeout=EMBEDDER_BATCH_REQUEST_TIMEOUT_SECONDS) call. Raising the timeout 60s → 300s means a genuinely hung (not just slow) microservice call can now block a Celery worker for up to ~3×300s = 15 minutes per task, versus ~3 minutes before. I didn't find a task_time_limit/soft_time_limit on the embedding task or in config/settings/base.py that would preempt this. Given there's no hard time-limit backstop, it might be worth either:

• adding a Celery soft_time_limit to the embedding task, or
• confirming this worst case is acceptable given how rare "hung but TCP-connected" failures are in practice (vs. connection-refused, which fails fast and isn't affected).

Not a blocker — the measured win is real and the previous 60s ceiling was clearly too aggressive — just flagging the tradeoff since the PR description doesn't discuss it.

Minor / non-blocking

• Scope of the doc additions: docs/test_scripts/fort_worth_homeowner_corpus.md is ~330 lines and documents several findings beyond the timeout fix (tool-call omission on gpt-4.1, grounding dilution, authority-corpus reachability gaps, preferred_llm pinning). Per CLAUDE.md's manual-test-script convention this is the right place for it, but it may be worth splitting into its own PR/doc next time so git blame/history on the embedder-timeout fix isn't tangled with an unrelated retrieval-architecture investigation. Not asking for a change here, just a note for next time.
• docs/test_scripts/Which Law Applies - A Fort Worth Homeowner's Map.txt — filename has spaces and an apostrophe, which is atypical for this repo's otherwise snake_case/kebab-case test-script naming. Harmless, but can be mildly annoying in shell one-liners/CI globs.
• No new regression test pins the specific values (300 / 32) the way test_api_batch_size_matches_service_cap pins the batch-size relationship — given how invisible this failure mode was in production (no errors, just silent fallback to ungrounded answers), a small test asserting EMBEDDER_BATCH_REQUEST_TIMEOUT_SECONDS >= <some floor> could guard against a future well-intentioned "let's tune this back down" regression. Optional.

Test coverage

Per the PR description, manage.py check passes and the embedder suite (138 tests) runs; the two intermittent failures were verified pre-existing (vector-dim mismatch, reproducible with the change stashed). That's a reasonable bar for a constants-only change — no new test scenarios strictly required since existing tests already assert the size/relationship invariants generically.

Overall: good fix, correctly scoped to the CPU-only embedder, backed by real measurements. The retry/timeout worst-case is the only thing I'd want confirmed before merging.