Fix/detector readiness - #12
Merged
Merged
Conversation
The first end-to-end run in production failed after exactly 240 seconds: 13:32:50 guardrail begins 13:36:50 API call failed on attempt 1: stream timeout That is the platform's ingress limit, and the finding is that it applies to internal ingress too. Moving the caller off the HTTP path was not sufficient: the worker still reaches the detector over HTTP, so somebody was still holding a connection open across a 13-minute cold start. llm_guardrail_timeout_s of 900 never applied, because the proxy severs the connection first -- the same way requestTimeout never applied on the public side. The worker now establishes readiness by repeating a short request until the detector answers, then screens against a warm server, where the call returns in seconds. No single request spans the cold start. The probe doubles as the activation, since a request to an app at zero replicas is what starts it. A job taken while the detector stays unreachable is recorded as failed rather than attempted, because attempting it would spend the whole request budget on a call that cannot succeed and end in the same failure. Records in infra/gemma/README.md that the 240s limit is not specific to public ingress, and that cost scales with wake-ups rather than screenings: the same fifty screenings cost about EUR 2 batched and about EUR 50 scattered.
…blished Two defects found reviewing the async split. abandon() overwrote any job, including one already carrying a result. A worker that records an outcome and then dies before deleting its message leaves that message to be redelivered; on the delivery that exceeds the limit, drain() abandons the job and rewrites the row, so a caller that had already read a result would later be told the screening failed. Only a job still PENDING is settled now. The queue-size validator counted raw UTF-8 bytes, but the fields are published inside a JSON document, where a control character occupies six bytes rather than one. A transcript of 24,000 such characters passed every cap and produced a 144,107-byte message against a 64 KiB ceiling. The rejection then arrived from the transport after the job row existed, so the caller was told to retry a body that can never be published. The validator now measures the escaped size, which is the size that actually travels.
llm_guardrail_timeout_s was 900 seconds, which could never fire: ingress severs any single request at 240 seconds, internal routes included. The call ended as a transport error from the proxy rather than a timeout naming the endpoint, which is what made the first production failure read as an unexplained stream timeout. It no longer has to cover the cold start either. The worker establishes that the detector is serving by repeating a short probe before screening anything, so this bounds a call to an endpoint already known to be up. That also closes the budget flagged in review. Worst case per job was 900 + 900 + 60 = 1860 seconds, above both the queue's visibility timeout and replicaTimeout, so a message could reappear while the execution holding it still ran. It is now 900 + 200 + 60 = 1160.
.codex/ was swept in by a broad add. It is local tooling config, not part of the detector readiness fix, and belongs in its own change if it is to be tracked.
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughChangesDetector readiness and job handling
JSON-aware request sizing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant DetectorClient
participant Detector
participant ScreenService
Worker->>DetectorClient: Probe /models
DetectorClient->>Detector: GET /models
Detector-->>DetectorClient: HTTP status
DetectorClient-->>Worker: Readiness result
alt Detector ready
Worker->>ScreenService: Screen queued job
else Detector unavailable
Worker->>ScreenService: Abandon job with DetectorUnavailable
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/worker.py (1)
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare
httpxas a direct dependency.
app/worker.pyand multiple tests importhttpx. Addhttpxtoproject.dependenciesinpyproject.tomland regenerateuv.lock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/worker.py` around lines 13 - 17, Declare httpx as a direct project dependency in pyproject.toml under project.dependencies, then regenerate uv.lock so the lockfile reflects the updated dependency metadata. Preserve the existing imports and dependency versions unless resolution requires changes.tests/unit/test_models.py (1)
81-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the full queue payload stays within 64 KiB.
Use
encode_messageand UTF-8 byte length intest_a_request_at_the_byte_limit_is_accepted. This covers the job identifier and JSON envelope that_json_string_bytesdoes not measure. The queue client sends raw text by default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_models.py` around lines 81 - 102, Update test_a_request_at_the_byte_limit_is_accepted to measure the complete queue payload using encode_message, then assert its UTF-8 byte length equals MAX_REQUEST_BYTES. Replace the _json_string_bytes-only calculation so the job identifier and JSON envelope are included, matching the queue client’s raw-text payload behavior.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/adapters/detector_readiness.py`:
- Around line 55-70: Update the readiness polling function containing the
waited/interval_s/deadline_s loop to use a monotonic clock and elapsed time
rather than incrementing waited only after sleep. Recalculate remaining time
before each probe and sleep, stop immediately once the deadline has elapsed, and
preserve returning True when probe succeeds. Add a test that advances or
consumes monotonic time during probe execution and verifies polling stops at the
configured deadline.
In `@app/config.py`:
- Around line 42-51: Update the Settings documentation for
llm_guardrail_timeout_s to remove the stale claim that the timeout covers GPU
cold starts, keeping the explanation that readiness polling handles startup
before screening and that the value remains below ingress limits.
In `@app/domain/service.py`:
- Around line 118-123: Make pending-only abandonment atomic by replacing the
separate get/status check and fail sequence in the abandonment method with a
JobStore contract operation such as fail_if_pending(job_id, reason). Implement
this operation atomically in InMemoryJobStore._settle() under its lock and with
a conditional ETag update in Azure Table Storage, preserving the skip behavior
for already-settled jobs. Add an interleaving test that completes the job
between abandonment’s attempted read and write and verifies the completed result
is not overwritten.
In `@infra/gemma/README.md`:
- Around line 375-379: Update the fenced log block containing
“worker_job_started” and “guardrail begins” to specify the text language on its
opening fence, preserving the log contents unchanged.
---
Nitpick comments:
In `@app/worker.py`:
- Around line 13-17: Declare httpx as a direct project dependency in
pyproject.toml under project.dependencies, then regenerate uv.lock so the
lockfile reflects the updated dependency metadata. Preserve the existing imports
and dependency versions unless resolution requires changes.
In `@tests/unit/test_models.py`:
- Around line 81-102: Update test_a_request_at_the_byte_limit_is_accepted to
measure the complete queue payload using encode_message, then assert its UTF-8
byte length equals MAX_REQUEST_BYTES. Replace the _json_string_bytes-only
calculation so the job identifier and JSON envelope are included, matching the
queue client’s raw-text payload behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f63e596-fea5-40df-be8e-46e7ebf4e5b0
📒 Files selected for processing (11)
app/adapters/detector_readiness.pyapp/config.pyapp/domain/models.pyapp/domain/service.pyapp/worker.pyinfra/gemma/README.mdtests/unit/test_config.pytests/unit/test_detector_readiness.pytests/unit/test_models.pytests/unit/test_service_async.pytests/unit/test_worker.py
…cally Four findings from review of the readiness change. The readiness deadline counted only the intervals between probes, not the time the probes themselves took. A probe against an endpoint that is not listening consumes its full timeout before failing, so at the worker's values the wait could reach 2,730 seconds against a configured 900 -- past the limits the deadline exists to stay inside. It now measures elapsed time on a monotonic clock. abandon() still read the job and wrote the failure as two operations, so a completion landing between them was overwritten. The decision moves into the store as fail_if_pending: one operation, guarded by a lock in memory and by a conditional write on the row's etag in Table Storage. The table's test double now models etags, since one that accepted any write would hide exactly the failure being guarded against. Also corrects the Settings documentation, which still described the guardrail timeout as covering a cold start, and labels a fenced block in the runbook.
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 by CodeRabbit