Skip to content

Commit 6c0a603

Browse files
rustyconoverclaude
andcommitted
protocol_version: enforce on HTTP dispatch path + conformance coverage
HTTP transport doesn't route through RpcServer.serve_one — it has its own dispatch loop in vgi_rpc/http/server/_app_unary.py and _app_stream.py. The initial commit's dispatch-boundary check was wired only in serve_one, so HTTP clients with mismatched protocol_version dispatched straight to the worker handler with mismatched schemas. Caught by senior review, fixed here. Adds the check at the same point in both HTTP unary and stream init — after _read_request populates _current_request_metadata, before _deserialize_params / handler dispatch. __describe__ is exempt (diagnostic path for mismatched clients to discover the server's version). Mismatch raises ProtocolVersionError → existing (VersionError, RpcError) catch wraps as _RpcHttpError(BAD_REQUEST) → client sees IOException with the directional message text intact. Verified by mutation test against the HTTP integration suite (edit VGI_PROTOCOL_VERSION → "2.0.0", rebuild, ATTACH fails with full directional text). Adds: - 4 HTTP transport unit tests in tests/test_protocol_version.py (matched success, mismatched directional error, describe bypass, undeclared opt-out). These are the tests that would have caught the blocker had they existed before. - protocol_version conformance category: ConformanceService now declares protocol_version="1.0.0", forcing cross-language ports to support the new metadata key. Two describe tests (surfaces_declared_version, format) and one round-trip (matched_dispatch_succeeds) across every transport. Mismatch scenarios stay in unit tests because the conformance runner is parameterised on a single proxy — spawning two servers at different versions doesn't fit the fixture model. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 416c0d1 commit 6c0a603

5 files changed

Lines changed: 178 additions & 3 deletions

File tree

tests/test_protocol_version.py

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,3 +387,89 @@ def serve() -> None:
387387
finally:
388388
client_t.close()
389389
server_t.close()
390+
391+
392+
# ---------------------------------------------------------------------------
393+
# HTTP transport — the path external clients actually use.
394+
# ---------------------------------------------------------------------------
395+
396+
397+
class TestHttpTransportVersionCheck:
398+
"""HTTP unary and stream init must enforce the dispatch-boundary version check.
399+
400+
HTTP doesn't route through ``RpcServer.serve_one`` — it has its own
401+
dispatch loop in ``vgi_rpc/http/server/_app_unary.py`` and ``_app_stream.py``.
402+
Without an HTTP-specific check, a mismatched HTTP client would dispatch
403+
straight to the worker handler with the wrong schemas, and the only
404+
defense would be the Arrow column-count check (silent corruption in some
405+
cases). These tests gate that HTTP-path regression.
406+
"""
407+
408+
def test_matched_versions_succeed(self) -> None:
409+
"""HTTP unary RPC succeeds when client and server speak the same protocol_version."""
410+
from vgi_rpc.http import http_connect, make_sync_client
411+
412+
server = RpcServer(_ProtoV100, _Impl())
413+
client = make_sync_client(server, token_key=b"test")
414+
try:
415+
with http_connect(_ProtoV100, client=client) as proxy:
416+
assert proxy.greet(name="x") == "hi x"
417+
finally:
418+
client.close()
419+
420+
def test_mismatched_versions_raise_with_direction(self) -> None:
421+
"""HTTP unary RPC fails with directional message when versions mismatch.
422+
423+
The C++ extension's user-visible error path runs through this: a
424+
DuckDB user issuing ``ATTACH`` against a worker on the wrong version
425+
sees the directional text inline, not a generic 'request failed'.
426+
"""
427+
from vgi_rpc.http import http_connect, make_sync_client
428+
429+
server = RpcServer(_ProtoV200, _Impl())
430+
# Client built against the 1.0.0 Protocol talks to a 2.0.0 server.
431+
client = make_sync_client(server, token_key=b"test")
432+
try:
433+
with http_connect(_ProtoV100, client=client) as proxy, pytest.raises(RpcError) as exc_info:
434+
proxy.greet(name="x")
435+
err_text = str(exc_info.value) + " " + exc_info.value.error_message
436+
assert "Client: 1.0.0" in err_text
437+
assert "Server: 2.0.0" in err_text
438+
assert "upgrade the VGI extension/client" in err_text
439+
finally:
440+
client.close()
441+
442+
def test_describe_bypass_over_http(self) -> None:
443+
"""HTTP __describe__ must dispatch regardless of client/server version mismatch."""
444+
from vgi_rpc.http import http_introspect, make_sync_client
445+
446+
server = RpcServer(_ProtoV200, _Impl(), enable_describe=True)
447+
client = make_sync_client(server, token_key=b"test")
448+
try:
449+
# http_introspect is the framework-internal discovery path; like
450+
# the pipe-transport introspect(), it doesn't carry a client
451+
# protocol_version. The server must serve describe anyway so
452+
# mismatched clients can introspect the expected version.
453+
desc = http_introspect(client=client)
454+
assert desc.protocol_name == "_ProtoV200"
455+
assert desc.protocol_version == "2.0.0"
456+
finally:
457+
client.close()
458+
459+
def test_undeclared_server_does_not_check_over_http(self) -> None:
460+
"""Server without protocol_version -> HTTP path is opt-out too.
461+
462+
Symmetry with the pipe path. A Protocol that hasn't opted into
463+
versioning must not gain the check just because the transport
464+
happens to be HTTP.
465+
"""
466+
from vgi_rpc.http import http_connect, make_sync_client
467+
468+
server = RpcServer(_ProtoNoVersion, _Impl())
469+
# Client declares 2.0.0; server opts out → check doesn't fire.
470+
client = make_sync_client(server, token_key=b"test")
471+
try:
472+
with http_connect(_ProtoV200, client=client) as proxy:
473+
assert proxy.greet(name="x") == "hi x"
474+
finally:
475+
client.close()

vgi_rpc/conformance/_protocol.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313

1414
import datetime as _dt
1515
from decimal import Decimal
16-
from typing import Annotated, Protocol
16+
from typing import Annotated, ClassVar, Protocol
1717

1818
import pyarrow as pa
1919

@@ -35,7 +35,15 @@
3535

3636

3737
class ConformanceService(Protocol):
38-
"""Wire-protocol conformance service exercising all framework capabilities."""
38+
"""Wire-protocol conformance service exercising all framework capabilities.
39+
40+
Declares ``protocol_version`` so cross-language conformant implementations
41+
must support the per-request ``vgi_rpc.protocol_version`` metadata key
42+
that vgi-rpc enforces at the dispatch boundary. Bump major when adding
43+
or removing methods; bump minor for additive parameter changes.
44+
"""
45+
46+
protocol_version: ClassVar[str] = "1.0.0"
3947

4048
# ------------------------------------------------------------------
4149
# Unary: Scalar Echo

vgi_rpc/conformance/_runner.py

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1819,6 +1819,67 @@ def _test_desc_protocol_hash_format(desc: ServiceDescription) -> None:
18191819
assert all(c in "0123456789abcdef" for c in desc.protocol_hash), "protocol_hash must be lowercase hex"
18201820

18211821

1822+
# ---------------------------------------------------------------------------
1823+
# Application-protocol-version tests
1824+
# ---------------------------------------------------------------------------
1825+
# The mismatch / directional-error scenarios for protocol_version don't fit
1826+
# the single-proxy-per-transport shape of the conformance runner — they require
1827+
# spawning two servers at different versions. Those cases live in
1828+
# ``tests/test_protocol_version.py`` (which now covers pipe AND HTTP). The
1829+
# conformance tests below cover the wire-shape contract every transport must
1830+
# satisfy: describe surfaces the field, request batches carry the metadata key,
1831+
# and matched-version dispatch succeeds across all transports.
1832+
1833+
1834+
@_describe_test(category="describe_protocol_version", name="surfaces_declared_version")
1835+
def _test_desc_protocol_version_surfaces(desc: ServiceDescription) -> None:
1836+
"""ConformanceService declares protocol_version, so describe must surface it.
1837+
1838+
Round-trip through ``__describe__`` response custom_metadata to
1839+
``ServiceDescription.protocol_version``. Cross-language ports that
1840+
don't yet read the ``vgi_rpc.protocol_version`` key fail this test,
1841+
which is the intended forcing function.
1842+
"""
1843+
assert desc.protocol_version, "protocol_version must be present in describe response"
1844+
# ConformanceService.protocol_version is "1.0.0"; the field is the source
1845+
# of truth, so don't hardcode the value here — fetch it.
1846+
from vgi_rpc.conformance._protocol import ConformanceService
1847+
1848+
expected = vars(ConformanceService).get("protocol_version")
1849+
assert desc.protocol_version == expected, (
1850+
f"describe surfaced protocol_version={desc.protocol_version!r}, "
1851+
f"expected {expected!r} from ConformanceService.protocol_version"
1852+
)
1853+
1854+
1855+
@_describe_test(category="describe_protocol_version", name="format")
1856+
def _test_desc_protocol_version_format(desc: ServiceDescription) -> None:
1857+
"""The protocol_version field must be canonical semver MAJOR.MINOR.PATCH."""
1858+
parts = desc.protocol_version.split(".")
1859+
assert len(parts) == 3, f"expected MAJOR.MINOR.PATCH, got {desc.protocol_version!r}"
1860+
for component in parts:
1861+
assert component.isdigit(), f"non-numeric semver component in {desc.protocol_version!r}"
1862+
# No leading zeros except literal "0".
1863+
assert component == "0" or not component.startswith("0"), (
1864+
f"leading zero in semver component of {desc.protocol_version!r}"
1865+
)
1866+
1867+
1868+
@_conformance_test(category="protocol_version", name="matched_dispatch_succeeds")
1869+
def _test_proto_version_matched_dispatch(proxy: ConformanceService, logs: LogCollector) -> None:
1870+
"""A normal dispatched RPC exercises the dispatch-boundary check on the matched-version path.
1871+
1872+
The conformance client and server are both built against the same
1873+
ConformanceService, so their declared protocol_version matches. A
1874+
successful echo proves the server's dispatch-boundary check accepted
1875+
the client's ``vgi_rpc.protocol_version`` metadata across whatever
1876+
transport the runner picked. (Mismatch scenarios live in unit tests —
1877+
see ``tests/test_protocol_version.py``.)
1878+
"""
1879+
result = proxy.echo_string(value="versioned")
1880+
assert result == "versioned"
1881+
1882+
18221883
# ---------------------------------------------------------------------------
18231884
# Runner
18241885
# ---------------------------------------------------------------------------

vgi_rpc/http/server/_app_stream.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020

2121
from vgi_rpc.external import predict_externalize_bytes_for_collector, resolve_external_location
2222
from vgi_rpc.log import Message
23-
from vgi_rpc.metadata import CANCEL_KEY, STATE_KEY, strip_keys
23+
from vgi_rpc.metadata import CANCEL_KEY, PROTOCOL_VERSION_KEY, STATE_KEY, strip_keys
2424
from vgi_rpc.rpc import (
2525
_EMPTY_SCHEMA,
2626
_TICK_BATCH,
@@ -50,6 +50,7 @@
5050
CallStatistics,
5151
HookToken,
5252
_current_call_stats,
53+
_current_request_metadata,
5354
_current_stream_id,
5455
_DispatchHook,
5556
_record_input,
@@ -205,6 +206,14 @@ def _run_stream_init_sync(
205206
f"Method name mismatch: URL path has '{method_name}' but Arrow IPC "
206207
f"custom_metadata 'vgi_rpc.method' has '{ipc_method}'. These must match."
207208
)
209+
# Application-protocol-version gate (mirror of RpcServer.serve_one). HTTP
210+
# stream init dispatches directly here; the check has to be wired in
211+
# independently. Stream methods include no synthetic ``__describe__``,
212+
# but the same exemption is kept for consistency with the unary path.
213+
# Server opts out by not declaring ``protocol_version`` on its Protocol.
214+
if app._server._protocol_version_parts is not None and method_name != "__describe__":
215+
md = _current_request_metadata.get()
216+
app._server._check_protocol_version(md.get(PROTOCOL_VERSION_KEY) if md is not None else None)
208217
_deserialize_params(kwargs, info.param_types, app._server.ipc_validation)
209218
_validate_params(info.name, kwargs, info.param_types)
210219
except (pa.ArrowInvalid, TypeError, StopIteration, RpcError, VersionError) as exc:

vgi_rpc/http/server/_app_unary.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
from pyarrow import ipc
1616

1717
from vgi_rpc.external import predict_externalize_bytes_for_batch
18+
from vgi_rpc.metadata import PROTOCOL_VERSION_KEY
1819
from vgi_rpc.rpc import (
1920
CallContext,
2021
RpcError,
@@ -37,6 +38,7 @@
3738
CallStatistics,
3839
HookToken,
3940
_current_call_stats,
41+
_current_request_metadata,
4042
_DispatchHook,
4143
_record_output,
4244
)
@@ -77,6 +79,15 @@ def _run_unary_sync(
7779
f"Method name mismatch: URL path has '{method_name}' but Arrow IPC "
7880
f"custom_metadata 'vgi_rpc.method' has '{ipc_method}'. These must match."
7981
)
82+
# Application-protocol-version gate (mirror of RpcServer.serve_one). HTTP
83+
# transport dispatches directly here instead of going through serve_one,
84+
# so the check has to be wired in independently. ``__describe__`` is
85+
# exempt — it's the diagnostic path a mismatched client uses to learn
86+
# the server's expected version. Server opts out by not declaring
87+
# ``protocol_version`` on its Protocol class.
88+
if app._server._protocol_version_parts is not None and method_name != "__describe__":
89+
md = _current_request_metadata.get()
90+
app._server._check_protocol_version(md.get(PROTOCOL_VERSION_KEY) if md is not None else None)
8091
_deserialize_params(kwargs, info.param_types, app._server.ipc_validation)
8192
_validate_params(info.name, kwargs, info.param_types)
8293
except (pa.ArrowInvalid, TypeError, StopIteration, RpcError, VersionError) as exc:

0 commit comments

Comments
 (0)