Skip to content

fix(grpc-web): round-2 review fixes — REST under Pyodide, OIDC refresh backoff, batch exit semantics - #2137

Merged
g-despot merged 6 commits into
feat/grpc-web-wasm-transportfrom
feat/grpc-web-review-fixes-round2
Aug 18, 2026
Merged

fix(grpc-web): round-2 review fixes — REST under Pyodide, OIDC refresh backoff, batch exit semantics#2137
g-despot merged 6 commits into
feat/grpc-web-wasm-transportfrom
feat/grpc-web-review-fixes-round2

Conversation

@g-despot

@g-despot g-despot commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #2056 — merges into feat/grpc-web-wasm-transport. Fixes from a second deep review of that PR at 524a27a6 (code review of both the companion and the base-client changes, plus hands-on probes in Pyodide 314.0.4 against Weaviate 1.39.0).

What's here

  • REST under Pyodide crashed on every HEAD/204 (a344c896) — the companion deferred to Pyodide's bundled httpx transport, which reads Response.body unconditionally (null for HEAD and 204), so data.exists/update/delete_by_id/reference_delete and tenants.exists raised a raw AttributeError; it also doesn't enforce sub-5 s REST timeouts end to end. The package transport is now always installed under Emscripten, builds stream-backed responses (reference_add_many reads .elapsed), takes the deadline from the read timeout only, and rounds the AbortSignal up with an int32 cap. The import hook only says "install weaviate-client-web" when the companion itself is missing. The e2e gains a transport self-check plus HEAD/204 and reference steps — an A/B run with the previous transport fails them.
  • Framing / deadline hardening + diagnostics (e9c81bec) — non-finite deadlines send no grpc-timeout; units escalate m → S → M and never H (Weaviate's transcoder rejects anything above 8H); LF-only trailer lines and non-ASCII keys are accepted; an unknown frame flag, a message after the trailer, or more than one message frame in a unary response is INTERNAL instead of silently mis-read; metadata with CR/LF/NUL is rejected before I/O. An HTTP 405 and a missing grpc_path_prefix under Emscripten both name the fix; a truncated body is reported as truncated, not as a wrong path.
  • README (1ffa2a05) — use_async_with_custom(..., grpc_path_prefix="/v1/grpc-web") on the REST port is the example; the previous use_async_with_local one cannot reach core-native grpc-web (no prefix → the shim POSTs to the native gRPC port → 63 s of retries). Full list of headers a CORS allow-list must carry; no more skip_init_checks recommendation.
  • OIDC refresh backoff, close(), timeouts (b950e39a, all platforms) — a permanent OAuth failure (invalid_grant) made the async refresher POST to the IdP every second until close(), while the sync thread died silently on the same error. Both colours now retry with a capped exponential backoff (1 s → 60 s, reset on success; Con001 reworded). Async close() awaits the cancelled task; the sync thread waits on the Event it was started with, so close() ends it promptly and a reconnect leaves no second refresher. Non-finite timeouts mean "no deadline" for every REST/gRPC hand-off (the Emscripten connect-timeout arm is gone — the package transport reads only read); the grpc-web prefix guards run at construction; a RuntimeError maps to WeaviateClosedClientError only when the http client is actually closed.
  • Batch stream exit (c099eb3c) — leaving with/async with client.batch.stream() never replaces the caller's exception (CancelledError included) with a background failure; both colours raise WeaviateBatchStreamError when the workers failed or data was left unsent, saying how much (the sync colour previously swallowed this); _BatchStreamShutdownError joins the taxonomy; WeaviateBatchError carries the gRPC details only.
  • CI (8a18975e) — pyright and flake8 now cover packages/web.

Behaviour changes worth a second look

Sync refresher catches any exception and backs off (was: HTTPError only, then dies); sync batch.stream() exit raises like async; Timeout(...=inf) means no deadline; the prefix guard fires at construction instead of inside connect().

Key areas for review

  • packages/web/…/_httpx_fetch.py — the ByteStream hand-off and _pick_timeout reading only read (nothing in the client relies on connect/pool standing in for a missing read timeout).
  • weaviate/connect/v4.py_cancel_background_token_refresh / async close(): same-loop task is awaited with return_exceptions=True, an other-loop task is only scheduled for cancel; _token_refresh_backoff and both refresh loops (reset on success, never below 1 s).
  • weaviate/collections/batch/batch_wrapper.py __exit__/__aexit__ — the e is not exc_val identity check is what stops the propagating exception being logged twice; sync.py:_wait copies results before raising so failed_objects stays inspectable.
  • packages/web/…/_channel.py_encode_timeout unit bounds and the frame-error → diagnostic mapping (unknown flag vs truncated decides what a user sees).
  • weaviate/__init__.py — the ModuleNotFoundError.name == "weaviate_client_web" test lets a companion with a broken dependency surface its own error.

Verification

554 unit + mock + proto tests (from 521) and 141 companion tests (from 87); ruff, flake8 and pyright clean including packages/web; every fix has a test that fails without it (mutation-checked). Real-Pyodide e2e 22/22 against Weaviate 1.39.0 core-native /v1/grpc-web. Hands-on under Pyodide: the HEAD/204 calls above pass, Timeout(query|insert|init=inf) work, Timeout(query=0.001) on REST now raises WeaviateTimeoutError, a 30 MB insert_many is no longer cut at ~5 s, wrong-prefix and 405 errors are diagnosable. On CPython (native gRPC): OIDC backoff observed as 4 refresh POSTs in 12 s (1/2/4/8 s), close() prompt in both colours; sync + async smoke and batch-stream context exit unchanged.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HZqnMkJBKrb7mUxhzCWmeB

…ripten

Pyodide's bundled httpx transport dereferences Response.body unconditionally,
which is null for HEAD and 204 responses, so data.exists/update/delete_by_id
and tenants.exists crashed with a raw AttributeError; it also does not enforce
sub-5s REST timeouts end to end. Install the package transport whenever the
platform is Emscripten instead of deferring, build the response as a stream so
httpx stamps .elapsed (reference_add_many read it), take the request deadline
from the read timeout only, and round the AbortSignal up with an int32 cap.

The import hook now raises the install hint only when the companion itself is
missing and chains any other ImportError, so a broken companion surfaces its
own error. The Pyodide e2e gains a transport self-check plus HEAD/204 and
reference steps so CI pins the fix.
Non-finite deadlines send no grpc-timeout header and no client-side wait;
finite ones escalate m -> S -> M to stay within the spec's 8 digits and never
use H, which Weaviate's transcoder rejects above 8H (values past ~190 years
mean no deadline). Trailer blocks accept LF-only lines and non-ASCII keys, an
unknown frame flag or a message after the trailer is a framing error, and a
unary response carrying more than one message frame is INTERNAL rather than
silently truncated. Metadata with CR/LF/NUL is rejected before any I/O.

Diagnostics: an HTTP 405 and a missing grpc_path_prefix under Emscripten both
name the fix, and a truncated grpc-web body is reported as truncated instead of
as a wrong path. Stale cross-references in comments trimmed.
use_async_with_custom(..., grpc_path_prefix="/v1/grpc-web") on the REST port
is the primary example; use_async_with_local/weaviate_cloud take no prefix and
only work with a transcoder at the gRPC host:port root. Drop the
skip_init_checks recommendation (the health check runs over grpc-web), list
every header the client sends in the CORS section, state the tested Pyodide
version, and correct the message-size and REST-transport wording.
…anitise timeouts

A permanent OAuth failure (expired or revoked refresh token) made the async
refresher POST to the IdP every second; the sync thread died silently on the
same error. Both colours now catch any exception and retry with a capped
exponential backoff (1s doubling to 60s, reset on success); Con001 says so.
The async close() awaits the cancelled task, and the sync thread waits on the
Event it was started with, so close() ends it promptly and a reconnect leaves
no second refresher. A test drives a real refresher death through connect()
to pin the done-callback wiring.

Non-finite timeouts mean "no deadline" for every REST and gRPC hand-off, the
Emscripten connect-timeout arm is gone (the package transport reads only the
read timeout), the grpc-web prefix guards run at construction with a message
that explains the CPython testing mode, and a RuntimeError is rewritten to
WeaviateClosedClientError only when the http client is actually closed.
Native connection errors keep the observed gRPC status line; the duplicated
no-prefix hint lives in the transport only.
…rity

Leaving `with`/`async with client.batch.stream()` no longer replaces the
exception raised in the block (CancelledError included) with the background
failure — the caller's wins and the background one is logged, unless it is the
same exception already propagating. Both colours raise
WeaviateBatchStreamError when the workers failed or the stream ended with data
still queued, with a message that says how much was unsent; the sync colour
previously swallowed this. _BatchStreamShutdownError joins the taxonomy, the
shutdown wait tolerates an infinite insert timeout, and WeaviateBatchError
carries the gRPC details only.
pyright now includes packages/web/src and the lint job's flake8 run covers the
package alongside ruff, so the companion is held to the same bar as the client.
@g-despot
g-despot requested a review from a team as a code owner August 17, 2026 18:40

@orca-security-eu orca-security-eu Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@g-despot
g-despot merged commit 1943edb into feat/grpc-web-wasm-transport Aug 18, 2026
129 of 130 checks passed
@g-despot
g-despot deleted the feat/grpc-web-review-fixes-round2 branch August 18, 2026 06:23
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