hil: only fail /json check on the actual double-send signature - #8
Conversation
First hardware run (ESPresense pipeline 974) exposed the check as too brittle:
2 of 4 nodes passed, 2 failed on `NoneType is not iterable` and
`IncompleteRead` — neither of which is the double-send bug. IncompleteRead is
*fewer* bytes than Content-Length, the opposite of a double-send's extra
bytes; the NoneType was the check itself crashing on a `null` body. The bug's
actual signature (a 200 answering the wrong URL) never appeared, i.e. the
firmware was fine and the check was crying wolf and reddening main.
Now the only hard failure is that signature: a 200 whose document doesn't
match the requested URL, or a 200 carrying the 429 text. Everything a
memory-constrained node legitimately does under a burst — 429s, dropping a
keep-alive mid-body, refusing a connection, the odd garbled body — is counted
as load-shedding, logged ("N/M shed under load"), and the worker reconnects.
Dropped to 2 workers; a constrained node need not survive a stampede for us to
prove it doesn't double-send.
test_json_check.py gains a third mock, `flaky`, that sheds load exactly like
the real nodes did; it must pass. That is the regression this commit fixes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013LgPhX92D1RCmZgYQwNAFN
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe JSON concurrency checker now distinguishes contract violations from load-shedding, rejecting malformed or non-object 200 responses and 503 responses. The test harness adds OOM-style null responses and stale 503 responses while retaining flaky 429 behavior. ChangesJSON concurrency validation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 1
🧹 Nitpick comments (1)
scripts/hil_monitor.py (1)
144-148: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_runsilently swallows unexpected worker exceptions.
_runonly catches_Bug. Any other unanticipated exception insideworker()kills that thread silently (no bug, no drop recorded), andjson_endpoint_checkwill still report "passed" with a shorter-than-expected effective sample. For a check that gates HIL/CI, an unexpected failure disappearing without a trace undermines its reliability.🛡️ Proposed fix
def _run(fn, n, bugs, drops): try: fn(n, drops) except _Bug as e: bugs.append(str(e)) + except Exception as e: # safety net: never silently lose a worker + drops.append(f"worker {n} crashed unexpectedly: {e!r}")🤖 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 `@scripts/hil_monitor.py` around lines 144 - 148, Update _run to catch unexpected exceptions from fn(n, drops) in addition to _Bug, and record or propagate them so worker failures cannot disappear silently and json_endpoint_check cannot report success with missing samples. Preserve the existing _Bug handling while ensuring unexpected exceptions produce an observable failure.
🤖 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 `@scripts/hil_monitor.py`:
- Around line 57-60: Update the response-handling logic in the worker’s
resp.status == 429 branch to append the request or failure detail to drops
before continuing. Preserve the existing reconnect-and-continue behavior while
ensuring 429 responses are included in the final shed-load count and logging.
---
Nitpick comments:
In `@scripts/hil_monitor.py`:
- Around line 144-148: Update _run to catch unexpected exceptions from fn(n,
drops) in addition to _Bug, and record or propagate them so worker failures
cannot disappear silently and json_endpoint_check cannot report success with
missing samples. Preserve the existing _Bug handling while ensuring unexpected
exceptions produce an observable failure.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb118729-154f-4a87-a590-7b6b3a72f5b4
📒 Files selected for processing (2)
scripts/hil_monitor.pyscripts/test_json_check.py
| Everything else a memory-constrained node does under a burst of concurrent requests — | ||
| 429s, dropping a keep-alive connection mid-body (IncompleteRead), refusing to connect — | ||
| is acceptable load-shedding, not a firmware bug. Those are counted and logged, never | ||
| failed on; the worker just reconnects and continues. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
429s aren't counted in drops, contradicting the docstring.
The docstring says 429s are "counted and logged" as load-shedding, but the resp.status == 429 branch just continues without appending to drops — unlike every other shed-load path (transport errors, non-200/429 statuses, unparseable bodies). This makes the final "X/Y shed under load" summary undercount the most common shedding case, weakening the diagnostic value of the check for future CI triage.
🩹 Proposed fix
if resp.status == 429:
+ drops.append(f"HTTP 429 on {path}")
continueAlso applies to: 82-86
🤖 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 `@scripts/hil_monitor.py` around lines 57 - 60, Update the response-handling
logic in the worker’s resp.status == 429 branch to append the request or failure
detail to drops before continuing. Preserve the existing reconnect-and-continue
behavior while ensuring 429 responses are included in the final shed-load count
and logging.
…ated Follow-up within this PR after the firmware side (ESPresense#2428) landed the proper fix. The first cut here tolerated a 200 with a `null` body as "load shedding" — but that is exactly the firmware bug, not acceptable behaviour. Tighten the rule: refusing to serve is fine (429 busy, 503 low-heap, or a TCP drop/reset), but any *200* that is null, non-object, unparseable, or the wrong document fails the build. That is what validates ESPresense#2428, which makes serveJson return 503 under low heap instead of a 200-null. test gains an `oom` mock (200-null under pressure -> must be caught) and its `flaky` mock now sheds load the correct way (drops + 503 -> must pass).
- _run now catches non-_Bug exceptions instead of swallowing them: a worker that dies unexpectedly is logged and counted as a drop rather than vanishing while the check still reports "passed" (CodeRabbit on #8). - Match the firmware low-heap refusal to 429 (ESPresense#2428 uses 429, not 503): flaky mock and messages updated. 503 stays tolerated for forward-compat.
The firmware contract is 200 (success) or 429 (busy or low-heap); the low-heap path was deliberately changed from 503 to 429 (ESPresense#2428). On a direct connection to the device there is no proxy, so a 503 can only come from the firmware itself and means wrong/old firmware — a contract violation, not load-shedding. Stop whitelisting 503; fail on it. Adds a `stale503` test mock (503 under pressure -> must be caught).
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/hil_monitor.py (3)
105-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original exception (
B904).
raise _Bug(...)insideexcept ValueError:drops the original traceback/context. Chain it for easier debugging of CI failures.🩹 Proposed fix
try: doc = json.loads(body) - except ValueError: + except ValueError as e: conn.close() raise _Bug(f"GET {path} returned a 200 with an unparseable body " - f"({len(body)}B, starts {body[:60]!r}) — truncated or garbled") + f"({len(body)}B, starts {body[:60]!r}) — truncated or garbled") from e🤖 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 `@scripts/hil_monitor.py` around lines 105 - 110, Update the ValueError handler in the JSON parsing flow to chain the original exception when raising _Bug, preserving the existing message and connection cleanup while retaining the underlying traceback context.Source: Linters/SAST tools
67-69: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuarantee
conncleanup on unexpected worker exceptions.
_runnow expectsworkerto potentially raise arbitrary exceptions (line 168-170), butworker's only cleanup path is after the loop completes normally (line 126-127). An unexpected exception outside the already-handled paths leaks the open connection instead of being closed.🩹 Proposed fix
def worker(n, drops): conn = None - for i in range(JSON_CHECK_REQUESTS): - path = "/json/devices" if i % 2 else "/json" - ... - if conn is not None: - conn.close() + try: + for i in range(JSON_CHECK_REQUESTS): + path = "/json/devices" if i % 2 else "/json" + ... + finally: + if conn is not None: + conn.close()Also applies to: 126-127
🤖 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 `@scripts/hil_monitor.py` around lines 67 - 69, Update the worker function to guarantee cleanup of conn when any unexpected exception interrupts the request loop, while preserving the existing handling and normal completion behavior. Move or wrap the loop and its current cleanup so conn.close() executes through a finally path, including for exceptions propagated to _run.
151-152: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish worker crashes from load-shedding drops.
Worker crashes are appended into the same
dropslist used for legitimate 429s/connection drops (line 169), then folded into the "X/Y shed under load" summary (line 152). A crash isn't shedding — it's likely a bug in the checker itself — and lumping it in there also masks that the crashed worker's remaining planned iterations were never attempted (yettotaldoesn't account for that), understating how much of the intended coverage actually ran.Consider tracking crashes in a separate list and reporting them distinctly (e.g. "X/Y shed, Z worker crashes") so a masked script bug doesn't read as an innocuous "clean pass with shedding."
Also applies to: 163-170
🤖 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 `@scripts/hil_monitor.py` around lines 151 - 152, Separate worker-crash tracking from the drops list in the monitor’s worker execution flow around the crash handling at lines 163-170. Keep only legitimate 429/connection-drop events in drops, track crashes separately, and update the summary built from total and shed to report both shed counts and worker-crash counts distinctly, while preserving accurate planned-iteration coverage when a worker terminates early.
🤖 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.
Nitpick comments:
In `@scripts/hil_monitor.py`:
- Around line 105-110: Update the ValueError handler in the JSON parsing flow to
chain the original exception when raising _Bug, preserving the existing message
and connection cleanup while retaining the underlying traceback context.
- Around line 67-69: Update the worker function to guarantee cleanup of conn
when any unexpected exception interrupts the request loop, while preserving the
existing handling and normal completion behavior. Move or wrap the loop and its
current cleanup so conn.close() executes through a finally path, including for
exceptions propagated to _run.
- Around line 151-152: Separate worker-crash tracking from the drops list in the
monitor’s worker execution flow around the crash handling at lines 163-170. Keep
only legitimate 429/connection-drop events in drops, track crashes separately,
and update the summary built from total and shed to report both shed counts and
worker-crash counts distinctly, while preserving accurate planned-iteration
coverage when a worker terminates early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d77027ff-eddb-4623-a139-eb66be5d1743
📒 Files selected for processing (2)
scripts/hil_monitor.pyscripts/test_json_check.py
CodeRabbit feedback on #8: - Wrap the worker loop in try/finally so the keep-alive connection is closed even if an unexpected exception propagates (previously only closed on normal loop exit). Removes the scattered conn.close() before each raise. - Track worker crashes in their own list, separate from load-shedding drops, and surface them distinctly ("N worker crash(es)") — a harness bug must not read as a clean pass. A crash now also fails the check rather than hiding in the tally. - Chain the ValueError -> _Bug raise (B904) to keep the original traceback. - Docstring: 429 is silently accepted (the designed refusal), not counted as shedding — matches the actual code instead of overclaiming. test_json_check.py gains a crash-routing assertion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LgPhX92D1RCmZgYQwNAFN
There was a problem hiding this comment.
Pull request overview
Refines the HIL /json concurrency probe to distinguish acceptable load-shedding (connection drops / refusal statuses) from true /json contract violations (false 200s such as wrong document, truncated/unparseable JSON, or non-object JSON), and expands the local raw-socket test harness to cover these cases.
Changes:
- Tighten
/jsonvalidation: only a 200 must be a correct JSON object matching the requested endpoint; other behaviors are treated as shedding and tallied. - Improve observability by tracking “shed under load” and routing worker crashes separately from drops/bugs.
- Extend
scripts/test_json_check.pywith additional mock modes and assertions for shedding vs contract-violation scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| scripts/hil_monitor.py | Updates /json endpoint check behavior, reporting, and worker error routing under concurrent load. |
| scripts/test_json_check.py | Expands raw-socket mock coverage for the /json checker (shedding, OOM/null-body, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The mock split only the current recv() chunk on \r\n\r\n, assuming each recv returned whole requests. TCP is a stream: a request can span recv() calls (and several can arrive in one), so a chunk without the delimiter was silently dropped and the worker hung on getresponse() until its 10s timeout — intermittent, nondeterministic. Buffer across recv() calls and process a request only once its terminating blank line has arrived; keep the remainder. Per-request logic factored into respond() so the read loop stays a plain buffer-and-dispatch.
Adds a
/jsonconcurrency check tohil_monitor.pyso HIL covers the web endpoint, not just the serial console — and hardened over review to be precise about what fails the build vs. what is acceptable load-shedding.The contract it enforces
The firmware answers
/jsonwith exactly two things: 200 (a complete, correct JSON object for the URL asked) or 429 (busy, or refusing under low heap — see ESPresense#2428). The check is built around that:Fails the build (a 200 that lies about success, or a contract violation):
null/ non-object — the low-heap null-body pathTolerated (refusing to serve is fine; never fails):
How it detects the double-send
The bug leaves an extra response in the socket buffer, shifting every later response on that connection by one — each still a valid 200. So each worker reuses one keep-alive connection (to expose the shift) and alternates
/json(nodeviceskey) against/json/devices(has one), asserting each response answers the request that was actually made.6Test —
scripts/test_json_check.py, no hardwareRaw-socket mocks (a
BaseHTTPRequestHandlercan't send two responses to one request, which is the whole point), with a stream-correct reader that buffersrecv()until the request delimiter:Needs a
v1.0.6tag after merge to move the floating:1image. Pairs with ESPresense#2428 (the firmware low-heap 429 guard) — the check only goes green on hardware once that fix is flashed.🤖 Generated with Claude Code
https://claude.ai/code/session_013LgPhX92D1RCmZgYQwNAFN