Skip to content

fix(sqlite): don't parse a non-QueryResponse body as JSON - #141

Open
ashishtanwer wants to merge 1 commit into
mainfrom
fix/query-http-non-json-body
Open

fix(sqlite): don't parse a non-QueryResponse body as JSON#141
ashishtanwer wants to merge 1 commit into
mainfrom
fix/query-http-non-json-body

Conversation

@ashishtanwer

Copy link
Copy Markdown
Contributor

What's wrong

_query_http parses the response with a bare response.json():

response = self.client.request("POST", f"/resources/sqlite/{name}/query", json=...)
return QueryResponse(**response.json())   # no status check, no shape check

So anything that isn't 200-with-JSON surfaces as either

  • JSONDecodeError: Expecting value: line 1 column 1 (char 0) — a non-JSON body
    (text/plain 500, nginx HTML 502, empty body), or
  • a pydantic ValidationError — a {"detail": ...} error body from a 401/404

…with no status code, no URL and no body text in the exception.

Why it mattered

This cost a full grading session on 2026-07-30 (see
theseus#21168).

A BLOB column made an env runner raise during response serialization, so Starlette
answered 500 Internal Server Error as text/plain (21 bytes). The resulting
char-0 JSONDecodeError reached the generated multi-app verifier, whose "app route is
dead" heuristic failed the run ENVIRONMENT_NOT_READY and threw away ~90 minutes of
completed rollout.

The environment was healthy the whole time — the runner served five 200s before the
500 in each of the four retries and was never restarted:

POST /ramp/api/v1/env/resources/sqlite/current/query   200  358
POST ...                                               200  386
POST ...                                               200  370
POST ...                                               200  290
POST ...                                               200  290
POST ...                                               500   21   <- text/plain

Diagnosing it required the cluster's edge access logs, purely because the exception
the SDK produced named neither the status nor the body. A caller cannot tell "app
returned 500" from "route is gone" — and here the difference decided whether a
session was scored or discarded.

The change

Raise FleetEnvironmentError with status_code, content-type and a 500-char body
snippet, mirroring the _describe_http precedent a few lines above. Shared by the
sync and async paths (from ...resources.sqlite import _raise_for_non_query_response)
so they can't drift, and it covers exec() as well as query().

Only paths that already raised change. A 200-with-JSON body is untouched, and a
failed query (200 + success=False) is still returned as data rather than raised
test_successful_query_is_untouched and
test_sql_error_response_still_returned_as_data_not_raised pin both, and they pass
with and without the patch.

The body snippet is deliberate, not incidental: callers classify unreachability on
the reason phrase in the body ("Bad Gateway", "Service Unavailable"), so gateway
failures keep classifying correctly through the new exception
(test_gateway_body_is_preserved_for_unavailability_classification).

Tests

8 new tests in tests/test_query_http_non_json_body.py, covering the text/plain 500,
the nginx 502 HTML, {"detail": ...} bodies at 401/404, an empty 200, the two
must-not-change paths, and the write path. 6 of the 8 fail without the patch.

Full suite: identical failure/error set before and after (5 pre-existing
tests/track/test_mcp_install.py failures and 3 pre-existing
test_sqlite_resource_dual_mode.py errors, all failing on main too).

One consequence worth a decision, not worked around here

An app-side 500 no longer matches the char-0 heuristic in theseus's
orchestrator/tasks/activities/verifier.py _app_unavailable(). With this merged,
such a response would be scored 0 instead of raising ENVIRONMENT_NOT_READY.

Silently scoring 0 for an env-side fault is worse than failing loudly, so that
heuristic likely wants an explicit "app-route 5xx ⇒ unavailable" rule. Gateway codes
(502/503/504) are unaffected — their bodies still carry the phrases it matches. I've
flagged this on the theseus PR rather than quietly changing grading semantics from
inside the SDK.

🤖 Generated with Claude Code

`_query_http` did a bare `response.json()`, so anything that wasn't
200-with-JSON surfaced as an opaque

    JSONDecodeError: Expecting value: line 1 column 1 (char 0)

for a non-JSON body, or a pydantic ValidationError for a `{"detail": ...}` error
body -- carrying no status code, no URL and no body text.

That cost a full grading session on 2026-07-30 (theseus#21168). A BLOB column made
an env runner return `500 Internal Server Error` as text/plain; the char-0
JSONDecodeError reached the generated multi-app verifier, whose "app route is
dead" heuristic failed the run ENVIRONMENT_NOT_READY and discarded ~90 minutes of
completed rollout -- while the environment was in fact healthy and served five
200s before each 500. The diagnosis needed the cluster's edge access logs, because
the exception the SDK produced named neither the status nor the body.

Raise FleetEnvironmentError with status_code, content-type and a 500-char body
snippet instead, mirroring the `_describe_http` precedent alongside it. Shared by
the sync and async paths so they can't drift, and it covers `exec()` too.

Only paths that already raised change: a 200-with-JSON body is untouched, and a
failed *query* (200 + success=False) is still returned as data rather than raised.
Two tests pin that.

The body snippet is included deliberately, not incidentally: callers classify
unreachability on the reason phrase in the body ("Bad Gateway", "Service
Unavailable"), so gateway failures keep classifying correctly through the new
exception.

Heads-up for theseus: an app-side 500 no longer matches that char-0 heuristic, so
`_app_unavailable` in orchestrator/tasks/activities/verifier.py would score such a
response 0 instead of raising ENVIRONMENT_NOT_READY. Silently scoring 0 for an
env-side fault is worse than failing loudly, so that heuristic likely wants an
explicit app-route-5xx rule. Called out rather than worked around here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ashishtanwer

Copy link
Copy Markdown
Contributor Author

Two sequencing notes for whoever merges this, both discovered while wiring up the theseus side.

1. Merge the theseus classifier fix first. fleet-ai/theseus#21306.

Before this PR, an env-side 500 reached the generated multi-app verifier as JSONDecodeError: Expecting value: line 1 column 1 (char 0) and matched its char-0 "app route is dead" rule — which is why the incident session failed loudly with ENVIRONMENT_NOT_READY rather than silently scoring 0. This PR replaces that with a descriptive FleetEnvironmentError("... status_code=500 ..."), which matches none of the existing rules. Probed directly against theseus main:

exception main classifies as unavailable
JSONDecodeError ... (char 0) True
FleetEnvironmentError("... status_code=500 ...") False ← would score 0

So merging this alone converts a loud environment fault into a quiet 0 on the agent's scorecard. #21306 adds the app-route-5xx rules that keep it loud; it's independently useful, since a 500 carried on a response object is already misclassified today.

2. This PR targets main, which isn't where releases come from. fleet-python-v0.2.132 sits on release/fleet-python-v0.2.132, and main's pyproject.toml still says 0.2.124 — so main is behind what's on PyPI, and merging here does not by itself produce a consumable release. Either the fix needs carrying onto the next release branch, or it should ride the canonicalization work in flight on andrews/plat-424-canonicalize-fleet-sdk-main-and-automate-high-assurance-pypi. Flagging rather than guessing a version — I deliberately left the pyproject.toml version alone.

Once a release contains it, the theseus-side pins are two one-liners, with different blast radii:

  • shared/template-gen/pyproject.toml (currently 0.2.132) — the cluster grader (fleet/verifier-runtime); rebuilding that one image deploys it.
  • shared/verifier/pyproject.toml (currently 0.2.115, 17 versions behind) — the in-env grader; only reaches an environment when that env image is rebuilt.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant