fix: read the etag where the SDK keeps it, and say what the probe is … - #13
Conversation
…doing The first production run of the readiness change failed with "IfNotModified must be specified with etag". The etag is metadata on a table entity, not one of its properties, so copying the entity into a plain dict dropped it and the conditional write was sent without one. The test double had put the etag among the properties, so the tests passed against a client that behaves differently from the real one. It now mirrors the SDK, and the test that covers this fails without the fix. The readiness loop logged nothing, so fifteen minutes of polling and a hang were indistinguishable in the logs. Each attempt is now recorded with how long it has waited and why the endpoint was not usable. Raises the probe timeout from 30 to 180 seconds. The platform holds a request open while it starts a replica for an app scaled to zero, so a probe that gives up in thirty seconds can abandon that start before a replica exists -- the endpoint is then never reached however often it is retried. 180 stays below the 240-second limit at which ingress severs any request. The per-job budget is now asserted by a test rather than described in a comment.
📝 WalkthroughWalkthroughThe changes add structured detector readiness logs, use Azure Table entity metadata for conditional etags, and align detector and queue timeouts. Unit tests cover readiness logging, SDK-style etags, and timeout constraints. ChangesWorker reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes 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: 2
🧹 Nitpick comments (3)
app/worker.py (1)
52-55: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winApply
VISIBILITY_TIMEOUT_StoAzureJobQueue.
VISIBILITY_TIMEOUT_Sis not passed whenmainconstructsAzureJobQueue. The actual lease therefore remains controlled by the adapter default. A future default change can break the worker budget while the worker constant and its test still pass.Pass
visibility_timeout=int(VISIBILITY_TIMEOUT_S)when constructingAzureJobQueue.🤖 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 52 - 55, Update the AzureJobQueue construction in main to pass visibility_timeout=int(VISIBILITY_TIMEOUT_S), ensuring the queue uses the worker’s configured visibility timeout instead of the adapter default.tests/unit/test_detector_readiness.py (1)
135-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the structured readiness context.
The test verifies event names only. It does not verify
attempt,waited_s, orreason. A regression in the new structured log contract would pass this test.Capture
record.contextand assert the not-ready attempt contains{"attempt": 1, "waited_s": 0, "reason": "not_serving"}. Also assert the ready event has the expected second-attempt context.🤖 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_detector_readiness.py` around lines 135 - 160, Update the _Collect handler in the readiness test to capture each record’s context alongside its message, then assert the detector_not_ready_yet event from attempt 1 has attempt 1, waited_s 0, and reason "not_serving". Also assert detector_ready contains the expected context for attempt 2, including its attempt and waited_s values and the ready reason.tests/unit/test_job_store_table.py (1)
127-127: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the fake enforce
IfNotModifiedwith the ETag.When
match_conditionis omitted, Azure SDK defaults toUnconditionallyand rejects an explicit ETag. The fake currently accepts the update when the ETag matches.🤖 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_job_store_table.py` at line 127, Update the fake’s conditional handling around the etag comparison to enforce IfNotModified semantics: require an explicit matching condition when an ETag is provided, and reject requests that omit match_condition even when the ETag matches. Preserve unconditional updates when no ETag is supplied.
🤖 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 81-90: Update the deadline check in the readiness loop around
now(), waited, and the detector_never_ready branch to use the raw elapsed
duration for retry-budget evaluation, while retaining the rounded waited value
in the context used for logging. Ensure values near the deadline cannot permit
an extra sleep due to rounding.
In `@tests/unit/test_worker.py`:
- Around line 170-175: Update the budget calculation in the relevant worker test
to include DETECTOR_PROBE_TIMEOUT_S alongside DETECTOR_READY_DEADLINE_S and the
existing LLM timeouts, so the assertion verifies the complete worst-case job
duration remains below VISIBILITY_TIMEOUT_S.
---
Nitpick comments:
In `@app/worker.py`:
- Around line 52-55: Update the AzureJobQueue construction in main to pass
visibility_timeout=int(VISIBILITY_TIMEOUT_S), ensuring the queue uses the
worker’s configured visibility timeout instead of the adapter default.
In `@tests/unit/test_detector_readiness.py`:
- Around line 135-160: Update the _Collect handler in the readiness test to
capture each record’s context alongside its message, then assert the
detector_not_ready_yet event from attempt 1 has attempt 1, waited_s 0, and
reason "not_serving". Also assert detector_ready contains the expected context
for attempt 2, including its attempt and waited_s values and the ready reason.
In `@tests/unit/test_job_store_table.py`:
- Line 127: Update the fake’s conditional handling around the etag comparison to
enforce IfNotModified semantics: require an explicit matching condition when an
ETag is provided, and reject requests that omit match_condition even when the
ETag matches. Preserve unconditional updates when no ETag is supplied.
🪄 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: ae54793c-779f-4890-90eb-95809fea5226
📒 Files selected for processing (6)
app/adapters/detector_readiness.pyapp/adapters/job_store_table.pyapp/worker.pytests/unit/test_detector_readiness.pytests/unit/test_job_store_table.pytests/unit/test_worker.py
| waited = round(now() - started) | ||
| context = {"attempt": attempt, "waited_s": waited} | ||
| if ready: | ||
| logger.info("detector_ready", extra={"context": context}) | ||
| return True | ||
| if now() - started + interval_s > deadline_s: | ||
| logger.info( | ||
| "detector_not_ready_yet", extra={"context": {**context, "reason": reason}} | ||
| ) | ||
| if waited + interval_s > deadline_s: | ||
| logger.error("detector_never_ready", extra={"context": context}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use unrounded elapsed time for the deadline check.
Line 81 rounds elapsed time before Line 89 evaluates the retry budget. A value such as 58.6 seconds with a 60 second deadline and a 1 second interval rounds down and permits another sleep after the deadline.
Keep waited_s rounded for logs. Use the raw elapsed value for deadline control.
Proposed fix
- waited = round(now() - started)
+ elapsed = now() - started
+ waited = round(elapsed)
context = {"attempt": attempt, "waited_s": waited}
@@
- if waited + interval_s > deadline_s:
+ if elapsed + interval_s > deadline_s:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| waited = round(now() - started) | |
| context = {"attempt": attempt, "waited_s": waited} | |
| if ready: | |
| logger.info("detector_ready", extra={"context": context}) | |
| return True | |
| if now() - started + interval_s > deadline_s: | |
| logger.info( | |
| "detector_not_ready_yet", extra={"context": {**context, "reason": reason}} | |
| ) | |
| if waited + interval_s > deadline_s: | |
| logger.error("detector_never_ready", extra={"context": context}) | |
| elapsed = now() - started | |
| waited = round(elapsed) | |
| context = {"attempt": attempt, "waited_s": waited} | |
| if ready: | |
| logger.info("detector_ready", extra={"context": context}) | |
| return True | |
| logger.info( | |
| "detector_not_ready_yet", extra={"context": {**context, "reason": reason}} | |
| ) | |
| if elapsed + interval_s > deadline_s: | |
| logger.error("detector_never_ready", extra={"context": context}) |
🤖 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/adapters/detector_readiness.py` around lines 81 - 90, Update the deadline
check in the readiness loop around now(), waited, and the detector_never_ready
branch to use the raw elapsed duration for retry-budget evaluation, while
retaining the rounded waited value in the context used for logging. Ensure
values near the deadline cannot permit an extra sleep due to rounding.
| budget = ( | ||
| DETECTOR_READY_DEADLINE_S | ||
| + settings.llm_guardrail_timeout_s | ||
| + settings.llm_timeout_s | ||
| ) | ||
| assert budget < VISIBILITY_TIMEOUT_S |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Include the final probe timeout in the job budget.
wait_until_ready checks its deadline after await probe(). A probe that starts just before DETECTOR_READY_DEADLINE_S can still consume DETECTOR_PROBE_TIMEOUT_S. The current assertion omits that time, so it does not prove that one job fits inside the queue lease.
Proposed fix
budget = (
DETECTOR_READY_DEADLINE_S
+ + DETECTOR_PROBE_TIMEOUT_S
+ settings.llm_guardrail_timeout_s
+ settings.llm_timeout_s
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| budget = ( | |
| DETECTOR_READY_DEADLINE_S | |
| + settings.llm_guardrail_timeout_s | |
| + settings.llm_timeout_s | |
| ) | |
| assert budget < VISIBILITY_TIMEOUT_S | |
| budget = ( | |
| DETECTOR_READY_DEADLINE_S | |
| DETECTOR_PROBE_TIMEOUT_S | |
| settings.llm_guardrail_timeout_s | |
| settings.llm_timeout_s | |
| ) | |
| assert budget < VISIBILITY_TIMEOUT_S |
🤖 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_worker.py` around lines 170 - 175, Update the budget
calculation in the relevant worker test to include DETECTOR_PROBE_TIMEOUT_S
alongside DETECTOR_READY_DEADLINE_S and the existing LLM timeouts, so the
assertion verifies the complete worst-case job duration remains below
VISIBILITY_TIMEOUT_S.
1 participant
The first production run of the readiness change failed with "IfNotModified must be specified with etag". The etag is metadata on a table entity, not one of its properties, so copying the entity into a plain dict dropped it and the conditional write was sent without one.
The test double had put the etag among the properties, so the tests passed against a client that behaves differently from the real one. It now mirrors the SDK, and the test that covers this fails without the fix.
The readiness loop logged nothing, so fifteen minutes of polling and a hang were indistinguishable in the logs. Each attempt is now recorded with how long it has waited and why the endpoint was not usable.
Raises the probe timeout from 30 to 180 seconds. The platform holds a request open while it starts a replica for an app scaled to zero, so a probe that gives up in thirty seconds can abandon that start before a replica exists -- the endpoint is then never reached however often it is retried. 180 stays below the 240-second limit at which ingress severs any request.
The per-job budget is now asserted by a test rather than described in a comment.
Summary by CodeRabbit
Improvements
Tests