From e659faec514f1f74a7b990d8bbdcebf405f42d68 Mon Sep 17 00:00:00 2001 From: Rusty Conover Date: Thu, 17 Sep 2026 10:56:41 -0400 Subject: [PATCH 1/3] fix: a scalar stream is an exchange, and must say so on the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `InitScalar` returned `RpcStream(..., InputSchema: null)`. A scalar call is an EXCHANGE — DuckDB pushes one batch of argument columns per turn and reads one batch of results back — and the only marker on the wire that says so is a non-empty `InputSchema`. The canonical Python worker sets exactly this (`input_schema = request.bind_call.input_schema`, in `Worker._init_stream`'s `ScalarFunctionGenerator` branch); this port had the decoded schema in hand two lines earlier and dropped it. vgi-rpc's HTTP dispatch classifies a stream with `InputSchema is not { FieldsList.Count: > 0 }` as a PRODUCER, and folds a producer's first tick into the `/init` request itself, driving it with a zero-COLUMN batch. So every scalar function served over HTTP was called before DuckDB had sent a single argument row, and threw out of `RecordBatch.Column(0)`: System.ArgumentOutOfRangeException: Index was out of range. at Apache.Arrow.RecordBatch.Column(Int32 i) at ...ExampleWorker.Scalar.DoubleFunction.Process(...) at ...Internal.ScalarStreamState.ExchangeAsync(...) at ...Http.RpcHttpEndpoints.HandleStreamInitAsync(...) 26 integration files failed there first, across scalar/, settings/, global_functions/, overload/, aggregate/, cache/, connection_string.test and unary_error_propagation.test. The pipe/launcher transport never synthesizes a turn — it only delivers batches the client actually sent — so the launcher lane was green throughout, which is why a published release carries this. The regression test asserts the invariant twice: once as the schema it should declare, and once spelled exactly as the transport spells the predicate, because that predicate is what actually decides. Co-Authored-By: Claude Opus 5 (1M context) --- src/QueryFarm.Vgi/Internal/VgiServiceImpl.cs | 15 ++- .../Internal/ScalarStreamShapeTests.cs | 98 +++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 test/QueryFarm.Vgi.Tests/Internal/ScalarStreamShapeTests.cs diff --git a/src/QueryFarm.Vgi/Internal/VgiServiceImpl.cs b/src/QueryFarm.Vgi/Internal/VgiServiceImpl.cs index 5ae5772..d79f23d 100644 --- a/src/QueryFarm.Vgi/Internal/VgiServiceImpl.cs +++ b/src/QueryFarm.Vgi/Internal/VgiServiceImpl.cs @@ -847,7 +847,20 @@ private RpcStream InitScalar(string identity, IReadOnlyList }; var state = new ScalarStreamState(function, outputSchema, bindRequest.Arguments, bindRequest.Settings, bindRequest.Secrets); - return new RpcStream(outputSchema, state, InputSchema: null, Header: header); + + // InputSchema MUST be the bound argument-column schema, never null: a scalar call is an + // EXCHANGE stream (DuckDB pushes one batch of argument columns per turn), and the wire + // contract's only marker for "exchange, not producer" is a non-empty InputSchema — the + // canonical Python worker sets exactly this (`input_schema = request.bind_call.input_schema` + // in Worker._init_stream's ScalarFunctionGenerator branch). Declaring null here made every + // scalar call read as a producer on the HTTP transport, where vgi-rpc folds a producer's + // first tick into /init and hands the state a zero-COLUMN tick batch: the function then + // indexed Column(0) on it and threw ArgumentOutOfRangeException before DuckDB had sent a + // single argument row — 26 integration files failed there first, across scalar/, + // settings/, global_functions/, overload/, aggregate/, cache/, connection_string.test and + // unary_error_propagation.test. The pipe/launcher transport never ticks — it only delivers + // batches the client actually sent — which is why the launch lane stayed green and hid it. + return new RpcStream(outputSchema, state, InputSchema: inputSchema, Header: header); } private RpcStream InitTable(ITableFunction function, BindRequest bindRequest, InitRequest request) diff --git a/test/QueryFarm.Vgi.Tests/Internal/ScalarStreamShapeTests.cs b/test/QueryFarm.Vgi.Tests/Internal/ScalarStreamShapeTests.cs new file mode 100644 index 0000000..baf619a --- /dev/null +++ b/test/QueryFarm.Vgi.Tests/Internal/ScalarStreamShapeTests.cs @@ -0,0 +1,98 @@ +using Apache.Arrow; +using Apache.Arrow.Types; +using QueryFarm.Vgi.Attributes; +using QueryFarm.Vgi.Internal; +using QueryFarm.Vgi.Protocol; +using QueryFarm.Vgi.Scalar; +using Xunit; + +namespace QueryFarm.Vgi.Tests.Internal; + +/// +/// The declared WIRE SHAPE of a scalar function's stream. +/// +/// A scalar call is an exchange: DuckDB pushes one batch of argument columns per turn and +/// reads one batch of results back. The only thing on the wire that says so is the stream's +/// InputSchema — QueryFarm.VgiRpc.Http.RpcHttpEndpoints.HandleStreamInitAsync +/// classifies a stream with stream.InputSchema is not { FieldsList.Count: > 0 } as a +/// PRODUCER, and a producer's first tick is folded into the /init request itself and driven +/// with a zero-COLUMN batch. This port declared InputSchema: null, so every scalar function +/// over HTTP was ticked with that batch before DuckDB had sent a single argument row, and the +/// function threw out of RecordBatch.Column(0) — 26 integration files failed there first, +/// across scalar/, settings/, global_functions/, overload/, +/// aggregate/, cache/, connection_string.test and +/// unary_error_propagation.test. +/// +/// Nothing caught it because the pipe/launcher transport never synthesizes a turn — it only +/// delivers batches the client actually sent — and the repo's integration lane ran only that +/// transport. An HTTP lane now runs alongside it (ci/run-integration.sh TRANSPORT=http); +/// this test is the fast, DuckDB-free guard on the same invariant. +/// +public class ScalarStreamShapeTests +{ + private sealed class DoublingScalar : ScalarFn + { + public override string Name => "doubling"; + + private void Compute([Param] Int64Array value, Int64Array.Builder result) + { + for (var row = 0; row < value.Length; row++) + { + result.Append(value.GetValue(row) * 2); + } + } + } + + private static async Task<(VgiServiceImpl Service, byte[] Attach)> NewAttachedServiceAsync() + { + var registry = new CatalogRegistry(); + registry.RegisterScalar(new DoublingScalar()); + var service = new VgiServiceImpl(registry); + var attach = await service.CatalogAttachAsync(new CatalogAttachRequest { Name = "example" }); + return (service, attach.AttachOpaqueData ?? []); + } + + [Fact] + public async Task InitAsync_DeclaresTheScalarsArgumentColumns_AsTheStreamsInputSchema() + { + var (service, attach) = await NewAttachedServiceAsync(); + var arguments = new Schema([new Field("value", Int64Type.Default, nullable: true)], metadata: null); + var bindRequest = new BindRequest + { + FunctionName = "doubling", + FunctionType = FunctionType.Scalar, + Arguments = [], + InputSchema = SchemaIpc.WriteSchemaOnly(arguments), + AttachOpaqueData = attach, + }; + + var stream = await service.InitAsync(new InitRequest { BindCall = EmbeddedIpc.Encode(bindRequest) }); + + Assert.NotNull(stream.InputSchema); + Assert.Equal(["value"], stream.InputSchema!.FieldsList.Select(f => f.Name)); + Assert.Equal(ArrowTypeId.Int64, stream.InputSchema.GetFieldByIndex(0).DataType.TypeId); + } + + /// The predicate above, spelled exactly as the transport spells it. A scalar stream + /// that answers here is one every HTTP worker will tick as a producer, + /// whatever the schema on it happens to look like. + [Fact] + public async Task InitAsync_ScalarStreamIsNeverClassifiedAsAProducerByTheHttpTransport() + { + var (service, attach) = await NewAttachedServiceAsync(); + var arguments = new Schema([new Field("value", Int64Type.Default, nullable: true)], metadata: null); + var bindRequest = new BindRequest + { + FunctionName = "doubling", + FunctionType = FunctionType.Scalar, + Arguments = [], + InputSchema = SchemaIpc.WriteSchemaOnly(arguments), + AttachOpaqueData = attach, + }; + + var stream = await service.InitAsync(new InitRequest { BindCall = EmbeddedIpc.Encode(bindRequest) }); + + var readsAsProducer = stream.InputSchema is not { FieldsList.Count: > 0 }; + Assert.False(readsAsProducer); + } +} From dcfb464351e996cf411c2fd454c2feffa8504841 Mon Sep 17 00:00:00 2001 From: Rusty Conover Date: Thu, 17 Sep 2026 10:56:52 -0400 Subject: [PATCH 2/3] ci: run the integration suite over HTTP too, and refuse a lane that is green without running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The launcher lane reported 333/334 while **81 of 334 files were red over HTTP** on the same worker build. A worker's two transports are separate dispatch implementations — the pipe server loop and the HTTP endpoint handlers share the function code and nothing else — so one lane is not evidence about the other. `TRANSPORT=launch|http` picks; CI runs both as a matrix, neither fail-fast. Two things differ by lane, both for reasons intrinsic to the test: `database_worker/package.test` packages `$VGI_TEST_WORKER` as an EXECUTABLE and runs it, which a URL is not, so it is excluded on http (and still required on launch); `VGI_REQUIRE_LAUNCHER_TRANSPORT` is set only on launch. Then the guards, which matter more than the lane. A failed `require`/`require-env` is a SKIP, not a failure, so "all tests passed" alone proves nothing ran — and on an HTTP lane there is a second, sharper version of that hazard: DuckDB's sqllogictest runner defaults `ignore_error_messages` to {"HTTP", "Unable to connect"}, so a worker answering 500 produces an error message containing "HTTP" and the statement is SKIPPED rather than failed. That is not theoretical — an intermediate state of this work turned 118 assertions into silent skips while the summary still read `0 failed`, and two files (`table_in_out/echo/all_types`, `echo/union_tags`) had been red on that lane for as long as it existed while reading as "2 skipped". The lane now fails on: no test cases matched; an unreadable or below-floor assertion count; the http worker dying mid-run; and ANY assertion lost to that default — zero, not a tolerance, established by re-running the whole suite once with the directive disabled and finding nothing that legitimately errors with an HTTP-containing message. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 14 +++-- ci/README.md | 91 +++++++++++++++++++++++----- ci/run-integration.sh | 124 +++++++++++++++++++++++++++++++++++---- 3 files changed, 196 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6b3746a..3cc2411 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,12 +53,17 @@ jobs: # projects and needs no such exclusion. - run: dotnet format vgi-csharp.slnx --verify-no-changes - # See ci/README.md: a single (subprocess) lane against a prebuilt haybarn-unittest + the - # signed community vgi extension, no C++ build from source. First-pass, not yet run against - # a real haybarn-unittest binary — see that doc's "Scope of this first version" section. + # See ci/README.md: runs the canonical suite against a prebuilt haybarn-unittest + the signed + # community vgi extension, no C++ build from source. BOTH transports, because they are two + # separate dispatch implementations that have diverged in production: a launcher-only lane + # reported 333/334 green while 81 files were red over HTTP. integration: runs-on: ubuntu-latest needs: build-test + strategy: + fail-fast: false + matrix: + transport: [launch, http] steps: - uses: actions/checkout@v4 - uses: actions/setup-dotnet@v4 @@ -100,7 +105,8 @@ jobs: env: GH_TOKEN: ${{ github.token }} - - name: Run integration suite + - name: Run integration suite (${{ matrix.transport }}) run: ci/run-integration.sh env: VGI_SRC: ${{ github.workspace }}/vgi-upstream + TRANSPORT: ${{ matrix.transport }} diff --git a/ci/README.md b/ci/README.md index 76e8e15..93fe116 100644 --- a/ci/README.md +++ b/ci/README.md @@ -1,11 +1,11 @@ # CI: the vgi integration suite -[`.github/workflows/integration.yml`](../.github/workflows/integration.yml) runs the canonical +The `integration` job in [`ci.yml`](../.github/workflows/ci.yml) runs the canonical [Query-farm/vgi](https://github.com/Query-farm/vgi) integration sqllogictest suite against this -repo's C# example worker on every push/PR. The same `.test` files run against the Python, Go, -Rust, and Java ports, so a green run here is real wire-compatibility evidence. +repo's C# example worker on every push/PR, once per transport. The same `.test` files run against +the Python, Go, Rust, and Java ports, so a green run here is real wire-compatibility evidence. -(The separate [`ci.yml`](../.github/workflows/ci.yml) covers build/test/format.) +(The same workflow's `build-test` and `lint` jobs cover build/test/format.) ## How it works (no C++ build) @@ -24,19 +24,73 @@ from): [`preprocess-require.awk`](preprocess-require.awk) rewrites each `require ` into an explicit signed `INSTALL FROM {community,core}; LOAD ;`. `require-env` and everything else pass through. -5. **Run** — [`run-integration.sh`](run-integration.sh) stages the preprocessed tree and places - the main worker behind DuckDB's `launch:` AF_UNIX pool, so repeated ATTACHes reuse one warm - .NET process. The small stateful and incompatible-protocol fixture workers remain isolated - subprocesses. The harness `FORCE INSTALL`s the vgi extension (so the run uses what users can - install today), then runs the suite in a single `haybarn-unittest` invocation. +5. **Run** — [`run-integration.sh`](run-integration.sh) stages the preprocessed tree and runs the + suite in a single `haybarn-unittest` invocation, after `FORCE INSTALL`ing the vgi extension (so + the run uses what users can install today). The small stateful and incompatible-protocol + fixture workers remain isolated subprocesses on both lanes. -## Scope of this version +## Two transport lanes -This is deliberately a **single launcher lane** with no coverage collection, no skip-reason -allowlist, and no executed-case floor — unlike `vgi-go`'s CI, which covers stdio/launch/shm/http -lanes and guards against a whole-suite silent skip (a failed `require`/`require-env` is a *skip*, -not a failure, so "all tests passed" alone isn't proof anything ran). That hardening is a natural -follow-up if this lane ever needs it. +`TRANSPORT=launch` (the default) puts the main worker behind DuckDB's `launch:` AF_UNIX pool, so +repeated ATTACHes reuse one warm .NET process. `TRANSPORT=http` boots the same worker as +`--http` on an ephemeral port and ATTACHes `http://localhost:`. CI runs both as a matrix. + +**Both are required, because they are two separate dispatch implementations.** A worker's pipe +path and its HTTP path share the function code but not the server loop, and they have diverged in +production: with only the launcher lane, this repo published a release whose CI said 333/334 while +**81 of 334 files were red over HTTP** — a scalar stream declaring no input schema (so HTTP +classified it as a producer and drove it with a zero-column tick), plus three metadata bugs in +`vgi-rpc-csharp`'s HTTP dispatch. Nothing about those was visible from the launcher lane, by +construction: the pipe transport never synthesizes a turn and never re-encodes batch metadata. + +Two things differ by lane, both for reasons intrinsic to the test rather than to the worker: + +- `database_worker/package.test` is excluded on **http**. It packages `$VGI_TEST_WORKER` as an + executable artifact into a DuckDB table and runs it; a URL is not an executable. It runs, and + must pass, on the launch lane. +- `VGI_REQUIRE_LAUNCHER_TRANSPORT` is set only on **launch**. + +`VGI_HTTP_TRANSPORT` is **not** set on either lane yet, which leaves five HTTP-only files +skipped. Four of them — `http/capability_probe`, `http/producer_turns`, +`http/small_body_encoding`, `cache/partition_scope_identity` — were verified to pass on this +lane as-is. The fifth, `cache/identity_isolation.test`, needs the example worker to answer as a +*named* principal: the reference fixture server maps `vgi-test-alice`→alice and +`vgi-test-bob`→bob as optional bearer auth (see vgi-python's `_test_fixtures/http_server.py`). +`Worker.RunHttpAsync` here exposes no `authenticate` hook at all — a worker written against this +port cannot authenticate an HTTP caller — so there is nowhere to wire that map. Adding the hook +is a product API change and belongs in its own commit; when it lands, set `VGI_HTTP_TRANSPORT=1` +on the http lane and all five run. + +## Guards against a lane that is green without running + +A failed `require`/`require-env` is a *skip*, not a failure, so "all tests passed" alone is not +proof anything ran. `run-integration.sh` therefore fails the lane on: + +- **no test cases matched** — an empty stage still exits 0; +- **fewer than `MIN_ASSERTIONS` (9000) assertions executed** — the blunt floor against a + largely-skipped run; +- **the http worker dying mid-run** — every result after that point is untrustworthy; +- **any assertion skipped by DuckDB's `ignore_error_messages` default** (`MAX_HTTP_SWALLOWED`, + default 0). + +That last one deserves its own note. DuckDB's sqllogictest runner defaults +`ignore_error_messages` to `{"HTTP", "Unable to connect"}`: a statement whose error text contains +`HTTP` is **skipped, not failed**. On a lane that reaches the worker over HTTP, that is a live +hazard rather than a convenience — a worker answering 500 produces an error message containing +`HTTP`, so a whole class of server-side crashes reads as "skipped" and the lane exits 0. This was +not theoretical: during the work that added this lane, an intermediate state of the worker turned +**118 assertions into silent skips** while the summary line still read `0 failed`, and two files +(`table_in_out/echo/all_types`, `echo/union_tags`) had been red on this lane for as long as it +existed while reading as "2 skipped". The suite's own files narrow the default where it matters to +them (`bearer_auth/bearer_token.test` sets `ignore_error_messages Unable to connect`; +`http/no_compression.test` clears it), which is upstream agreeing that the HTTP entry is wrong for +VGI tests — but it cannot be cleared from outside a `.test` file, so the count guard stands in for +it here. + +The threshold is **zero**, not a tolerance. That was established rather than assumed: the whole +suite was re-run once with `set ignore_error_messages Unable to connect` injected into every +staged file, turning each swallowed skip into a visible failure. With the worker fixed, nothing in +the suite legitimately errors with an HTTP-containing message on this lane. **This lane has real value beyond the local suite**: this environment doesn't have the DuckDB `spatial` extension built, so `require spatial`-gated files (e.g. `table/expression_filter.test`) @@ -68,7 +122,12 @@ that local verification when actually changing worker behavior. dotnet build -c Release VGI_SRC=~/Development/vgi \ HAYBARN_UNITTEST=/path/to/haybarn-unittest \ - ci/run-integration.sh + ci/run-integration.sh # launch lane (default) + +VGI_SRC=~/Development/vgi \ +HAYBARN_UNITTEST=/path/to/haybarn-unittest \ +TRANSPORT=http \ + ci/run-integration.sh # http lane ``` Download `haybarn-unittest` for your platform from the latest Haybarn release: diff --git a/ci/run-integration.sh b/ci/run-integration.sh index 24caa08..494ee18 100755 --- a/ci/run-integration.sh +++ b/ci/run-integration.sh @@ -3,16 +3,17 @@ # example worker, using a prebuilt standalone `haybarn-unittest` and the signed # community vgi extension — no C++ build from source. See ci/README.md. # -# Ported from vgi-go's ci/run-integration.sh, trimmed to a single launcher -# transport lane — vgi-go's version additionally covers stdio/shm/http lanes -# with a skip-reason allowlist and an executed-case -# floor to catch silent whole-suite skips; that hardening is a natural -# follow-up here once this lane is proven green in real CI (see ci/README.md). +# Ported from vgi-go's ci/run-integration.sh. Runs ONE transport per invocation; +# both must pass. A worker's two transports are separate dispatch implementations +# (the pipe/launcher server loop vs. the HTTP endpoint handlers) and they DO +# diverge: running only the launcher lane let a scalar-stream shape bug and three +# HTTP metadata bugs reach a published release with CI reporting 333/334 green. # # Required environment: # VGI_SRC path to a Query-farm/vgi checkout (contains test/sql/integration) # HAYBARN_UNITTEST path to the haybarn-unittest binary # Optional: +# TRANSPORT launch (default) | http — see the lane setup below # CONFIGURATION build configuration the worker binaries were built in (default: Release) # STAGE scratch dir for the preprocessed test tree (default: mktemp) set -euo pipefail @@ -24,6 +25,11 @@ HERE="$(cd "$(dirname "$0")" && pwd)" REPO="$(cd "$HERE/.." && pwd)" STAGE="${STAGE:-$(mktemp -d)}" CONFIGURATION="${CONFIGURATION:-Release}" +TRANSPORT="${TRANSPORT:-launch}" +case "$TRANSPORT" in + launch|http) ;; + *) echo "::error::TRANSPORT must be 'launch' or 'http', got '$TRANSPORT'"; exit 1 ;; +esac INTEGRATION="$VGI_SRC/test/sql/integration" [ -d "$INTEGRATION" ] || { echo "::error::no test/sql/integration under VGI_SRC=$VGI_SRC"; exit 1; } @@ -71,16 +77,30 @@ done # Revisit if a future community-extension publish # catches up. # --------------------------------------------------------------------------- -echo "Staging preprocessed tests into $STAGE ..." +AWK_HTTP=0 +# database_worker/package.test packages $VGI_TEST_WORKER as an EXECUTABLE artifact into a DuckDB +# table and then runs it (test/support/database_worker_fixture.sh is a wrapper that execs it). On +# the http lane VGI_TEST_WORKER is a URL, so the fixture fails with +# `/bin/sh: http://localhost:NNNNN: No such file or directory`. The test's premise is a local +# worker binary; there is nothing for an HTTP worker to package. Excluded on that lane only — it +# runs, and must pass, on the launch lane. +EXTRA_EXCLUDES=() +if [ "$TRANSPORT" = http ]; then + AWK_HTTP=1 + EXTRA_EXCLUDES=(-not -path './database_worker/package.test') +fi + +echo "Staging preprocessed tests into $STAGE (transport=$TRANSPORT) ..." mkdir -p "$STAGE/test/sql/integration" ( cd "$INTEGRATION" find . -name '*.test' \ -not -path './writable/*' \ -not -name 'nested_type_combinations.test' \ -not -path './cache/secret_ineligible.test' \ - -not -path './macro/macros.test' | while read -r f; do + -not -path './macro/macros.test' \ + ${EXTRA_EXCLUDES[@]+"${EXTRA_EXCLUDES[@]}"} | while read -r f; do mkdir -p "$STAGE/test/sql/integration/$(dirname "$f")" - awk -f "$HERE/preprocess-require.awk" "$f" > "$STAGE/test/sql/integration/$f" + awk -v http="$AWK_HTTP" -f "$HERE/preprocess-require.awk" "$f" > "$STAGE/test/sql/integration/$f" done ) # The database-worker tests package this executable through a path relative to @@ -95,11 +115,41 @@ mkdir -p "$STAGE/test/support" install -m 0755 "$DATABASE_WORKER_FIXTURE" \ "$STAGE/test/support/database_worker_fixture.sh" -# Pool the main worker behind DuckDB's AF_UNIX launcher. The suite opens many -# connections and ATTACHes the same worker repeatedly; a bare path starts a new -# .NET process for each connection, while launch: reuses one warm process. -export VGI_TEST_WORKER="launch:$WORKER" -export VGI_REQUIRE_LAUNCHER_TRANSPORT=1 +HTTP_PID="" +if [ "$TRANSPORT" = launch ]; then + # Pool the main worker behind DuckDB's AF_UNIX launcher. The suite opens many + # connections and ATTACHes the same worker repeatedly; a bare path starts a new + # .NET process for each connection, while launch: reuses one warm process. + export VGI_TEST_WORKER="launch:$WORKER" + export VGI_REQUIRE_LAUNCHER_TRANSPORT=1 +else + # One long-lived HTTP server for the whole lane, on an ephemeral port it reports + # on stdout as `PORT:`. VGI_REQUIRE_LAUNCHER_TRANSPORT is deliberately NOT set. + HTTP_LOG="$STAGE/http-worker.log" + ( cd "$STAGE" && exec "$WORKER" --http ) > "$HTTP_LOG" 2>&1 & + HTTP_PID=$! + trap '[ -n "$HTTP_PID" ] && kill -TERM "$HTTP_PID" 2>/dev/null || true' EXIT + port="" + for _ in $(seq 1 120); do + kill -0 "$HTTP_PID" 2>/dev/null || { echo "::error::http worker exited before reporting a port"; cat "$HTTP_LOG"; exit 1; } + port="$(sed -n 's/.*PORT:\([0-9]*\).*/\1/p' "$HTTP_LOG" | head -1)" + [ -n "$port" ] && break + sleep 0.5 + done + [ -n "$port" ] || { echo "::error::http worker never reported a port"; cat "$HTTP_LOG"; exit 1; } + echo "http worker pid=$HTTP_PID port=$port" + export VGI_TEST_WORKER="http://localhost:${port}" + # DELIBERATELY NOT SET: VGI_HTTP_TRANSPORT, which would un-gate the suite's five HTTP-only + # files. Four of them (http/capability_probe, http/producer_turns, http/small_body_encoding, + # cache/partition_scope_identity) were verified to pass here as-is; the fifth, + # cache/identity_isolation.test, needs the example worker to answer as a NAMED principal — + # the reference fixture server maps `vgi-test-alice`->alice / `vgi-test-bob`->bob as optional + # bearer auth (vgi-python's `_test_fixtures/http_server.py`). This port's `Worker.RunHttpAsync` + # exposes no authenticate hook at all, so there is nowhere to wire that map; adding one is a + # product API change, not a harness change, and belongs in its own commit. Setting the variable + # without it would leave a permanently red file, and excluding that file would be the exact + # dishonesty the guards below exist to prevent. See ci/README.md. +fi # Keep the small stateful and deliberately-incompatible fixtures isolated. # They account for only a handful of tests, and process isolation prevents @@ -139,5 +189,53 @@ if grep -q 'No test cases matched\|No tests ran' "$log"; then rc=1 fi +# ----------------------------------------------------------------------------- +# Guards against a lane that is green because it did not really run. +# +# DuckDB's sqllogictest runner defaults `ignore_error_messages` to +# {"HTTP", "Unable to connect"} — a statement whose error text contains "HTTP" is +# SKIPPED, not failed. On a lane that reaches the worker over HTTP that default is +# a live hazard rather than a convenience: a worker returning 500 produces an error +# message containing "HTTP", so a whole class of server-side crashes reads as +# "skipped" and the lane still exits 0. Observed directly: an intermediate state of +# this port turned 118 assertions into silent skips this way while the summary line +# still said 0 failed. The floor is ZERO, not a tolerance: the whole suite was re-run +# once with `set ignore_error_messages Unable to connect` injected into every staged +# file, and with the worker fixed NOTHING in it legitimately errors with an +# HTTP-containing message. Every such skip is a bug in hiding. +# +# The assertion floor catches the same failure mode from the other side — a run that +# skipped most of the suite for any reason at all. +if [ "$TRANSPORT" = http ]; then + swallowed="$(sed -n "s/^skip on error_message matching 'HTTP': \([0-9]*\)$/\1/p" "$log" | head -1)" + swallowed="${swallowed:-0}" + if [ "$swallowed" -gt "${MAX_HTTP_SWALLOWED:-0}" ]; then + echo "::error::$swallowed assertions were skipped by DuckDB's ignore_error_messages 'HTTP' default (max ${MAX_HTTP_SWALLOWED:-0}). A server-side 500 hides here — read the worker log at $HTTP_LOG." + rc=1 + fi + + if ! kill -0 "$HTTP_PID" 2>/dev/null; then + echo "::error::the shared http worker died during the run — every result after that point is untrustworthy." + echo "--- http worker log tail ---"; tail -80 "$HTTP_LOG" + rc=1 + fi +fi + +# Catch2 prints two different summary shapes, and the FULLY GREEN one is the shape this guard +# most needs to read: "All tests passed (N assertions in M test cases)" when nothing failed, and +# a "assertions: N | ..." table when something did. Match both — reading only the table would +# make the floor unreachable on exactly the runs it exists to catch. +assertions="$(sed -n 's/^assertions: *\([0-9]*\) .*/\1/p' "$log" | tail -1)" +if [ -z "$assertions" ]; then + assertions="$(sed -n 's/^All tests passed .*[( ]\([0-9][0-9]*\) assertions.*/\1/p' "$log" | tail -1)" +fi +if [ -z "$assertions" ]; then + echo "::error::could not read an assertion count out of the runner summary — the guard below cannot do its job, so fail rather than assume." + rc=1 +elif [ "$assertions" -lt "${MIN_ASSERTIONS:-9000}" ]; then + echo "::error::only $assertions assertions ran (floor ${MIN_ASSERTIONS:-9000}) — the suite was largely skipped, not passed." + rc=1 +fi + rm -f "$log" exit "$rc" From b4ddac8a0112c13019cdc3c8e0df3ee0f6f21c97 Mon Sep 17 00:00:00 2001 From: Rusty Conover Date: Thu, 17 Sep 2026 11:38:04 -0400 Subject: [PATCH 3/3] =?UTF-8?q?deps:=20require=20QueryFarm.VgiRpc=200.10.2?= =?UTF-8?q?=20=E2=80=94=20a=20correctness=20floor,=20not=20a=20compile=20f?= =?UTF-8?q?loor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scalar-stream fix in this repo closes 26 of the 81 integration files that were red over HTTP. The other 55 were four defects in the transport, fixed in QueryFarm.VgiRpc 0.10.2: HTTP dispatch never read `OutputCollector.EmittedMetadata` (so `vgi.cache.*`, `vgi_partition_values#b64`, `vgi_batch_index` and `vgi_rpc.parent_row#b64` were dropped on that transport alone, and the extension rejected the batches that had declared those features); the `/init` tick carried null input metadata (so conditional revalidation never saw `vgi.cache.if_none_match`); the transport's framing keys crossed into user metadata in one direction and let a worker overwrite its own continuation cursor in the other; and `ValueCodec.EmptyRow` — written as the continuation-token sentinel — threw for FixedSizeBinary, Union and Interval columns and for any ENUM whose dictionary index was not Int16. Worth stating plainly in the pin comment, because the usual signal is absent: this repo BUILDS against 0.10.1 and its unit tests PASS. Only the HTTP integration lane fails, which is exactly why that lane now exists. Lowering the pin to make a restore resolve would put the silent-drop behaviour back. Verified against the published package, not a sibling source build: QueryFarm.VgiRpc/0.10.2 resolves as `package` in project.assets.json, and the full integration suite is green on both lanes from that restore. Co-Authored-By: Claude Opus 5 (1M context) --- Directory.Packages.props | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 32d883a..02f77fe 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -16,8 +16,8 @@ on IArrowArray/Array), fixed in vgi-rpc-csharp 0.4.0 (see that repo's third_party/apache-arrow-dotnet/README.md's "Published as QueryFarm.Arrow" section). This package's own NuGet publish must not happen before 0.4.0 is live on nuget.org. --> - - + + - + keeps the name it already answered to. + MUST be >= 0.10.2, and this one is a CORRECTNESS floor rather than a compile floor: on + 0.10.1 and earlier, HTTP dispatch never read `OutputCollector.EmittedMetadata`, so every + per-batch key this worker emits — the whole `vgi.cache.*` family, + `vgi_partition_values#b64`, `vgi_batch_index`, `vgi_rpc.parent_row#b64` — was silently + dropped over HTTP and the extension rejected the batches that declared those features; + `ValueCodec.EmptyRow` also threw (500) for FixedSizeBinary/Union/Interval columns and for + any ENUM whose dictionary index was not Int16. This repo builds and its unit tests pass + against 0.10.1; it is the HTTP integration lane that does not, which is precisely why + that lane now exists (ci/README.md). Do not lower this pin to make a restore resolve. --> +