Skip to content

fix(grpc-web): abort-based deadlines, lockstep extra, CORS diagnostics and review nits - #2175

Draft
g-despot wants to merge 1 commit into
mainfrom
fix/grpc-web-review-fixes
Draft

g-despot wants to merge 1 commit into
mainfrom
fix/grpc-web-review-fixes

Conversation

@g-despot

Copy link
Copy Markdown
Collaborator

What & why

Functional follow-ups from the deep review of #2142 (merged). The comment/docs pass landed there as 0ce4cd4; this PR carries the behaviour changes on top of it.

  • Deadlines without leaked timers. asyncio.wait_for left one live JS timer per gRPC call for its full timeout — Node kept the e2e process alive ~90 s after the suite finished, a browser page accumulates them — and overflowed setTimeout on pre-314 Pyodide. The deadline is now an AbortController with its timer cleared in finally, capped at 2³¹−1 ms; run.mjs exits explicitly.
  • No user-agent from the fetch transport. httpx's default was forwarded; Firefox honours it and preflights it, and core's REST allowlist does not include it.
  • [grpc-web] extra pinned in lockstep from setup.py via setuptools_scm. micropip does not backtrack, so an unpinned extra breaks micropip.install("weaviate-client[grpc-web]==X") for any non-latest X.
  • CORS diagnosability. Under Emscripten a fetch rejection carries a hint naming CORS_ALLOW_ORIGIN / CORS_ALLOW_HEADERS and the Weaviate Cloud setting. A transport failure on a channel that has not yet received any response is reported as UNKNOWN and fails at once (was: UNAVAILABLE, 63 s of retries); after the first response it stays UNAVAILABLE.
  • Framing: conflicting repeats of grpc-status / grpc-message inside one trailer frame are rejected as INTERNAL instead of letting the last value win.
  • Smaller fixes: call metadata validated against the gRPC spec before any I/O (ValueError, as grpcio does); path-prefix normalisation (" " → native, "//a//b/" → /a/b); the sync-client prefix error points at WeaviateAsyncClient(ConnectionParams.from_params(..., grpc_path_prefix=...)); portable Con006 wording; "REST endpoint" only when gRPC and HTTP addresses match, plus an Emscripten branch for hand-built params without a prefix; a clear ImportError when weaviate_client_web is imported on CPython; Pyodide terminology in runtime strings.
  • Tests and CI: cancellation propagates, HTTP 405 diagnosis, fake grpc version tied to the fallback, default-port Con006 case; the dead OSError branch in the PyPI version check is gone; the wheel check and the lockstep assert are shared (ci/pyodide-e2e/wheels.mjs, ci/assert-lockstep.sh).

Decisions to confirm

  1. UNKNOWN before the first response means a transient failure on the very first call is not retried when skip_init_checks=True.
  2. Metadata validation is strict (keys [0-9a-z_.-]+, non--bin values printable ASCII).
  3. All extras_require now live in setup.py (setuptools ignores setup.cfg extras once setup.py defines any), so agents moved too.
  4. Custom senders passed to set_sender must enforce timeout and raise TimeoutError.

Verification

Pyodide unit suite 169 passed (139 before). run.mjs e2e: ALL STEPS OK in 4 s (was ~94 s). Timer probe: +0 pending timers per 200 gRPC calls (was +200). pytest test mock_tests proto_test: 524 passed. ruff / flake8 / pyright clean. Real Chrome, cross-origin: 12/12 operations against local 1.39.3 and 1.40.0-rc.1, grpc-web health check against Weaviate Cloud. Wheel metadata carries weaviate-client-web==<same version> including when built from the sdist.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CCHAmyQF69W7K9CTEzCpEd

…s and review nits

Functional follow-ups from the deep review of the Pyodide transport:

- Enforce the gRPC deadline through an AbortController with the timer
  cleared in finally, capped at 2^31-1 ms. asyncio.wait_for left one live
  JS timer per call for its full timeout (Node kept running ~90 s after the
  e2e suite) and overflowed on pre-314 Pyodide. run.mjs now exits explicitly.
- Stop forwarding httpx's user-agent from the fetch transport (Firefox
  honours it and preflights it; core's REST allowlist does not cover it).
- Pin the [grpc-web] extra to the same version as the base package from
  setup.py via setuptools_scm; micropip does not backtrack.
- Add a CORS hint under Emscripten to transport errors and the grpc-web
  health-check error; report a transport failure before the first response
  as UNKNOWN so a blocked or misrouted endpoint fails at once instead of
  after the UNAVAILABLE retry loop.
- Reject conflicting repeats of grpc-status/grpc-message in one trailer.
- Validate call metadata against the gRPC spec before sending; normalise
  the grpc-web path prefix; fix the sync-client prefix error target, the
  Con006 wording, the endpoint wording in WeaviateGRPCUnavailableError and
  the companion's import error on CPython.
- Tests for cancellation, the HTTP 405 diagnosis, the fake grpc version tie,
  the default-port Con006 case; drop the dead OSError branch in the PyPI
  version check; share the wheel check and the lockstep assert between the
  harness scripts and CI.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CCHAmyQF69W7K9CTEzCpEd

@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 Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed SAST high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

This branch has not been deployed

No deployments
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