Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 }}
17 changes: 13 additions & 4 deletions Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -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. -->
<PackageVersion Include="QueryFarm.VgiRpc" Version="0.10.1" />
<PackageVersion Include="QueryFarm.VgiRpc.Http" Version="0.10.1" />
<PackageVersion Include="QueryFarm.VgiRpc" Version="0.10.2" />
<PackageVersion Include="QueryFarm.VgiRpc.Http" Version="0.10.2" />

<!-- Same sibling-source-vs-NuGet split as QueryFarm.VgiRpc above; only used when the
vgi-rpc-csharp checkout isn't present. QueryFarm.VgiRpc.Client's first-ever NuGet publish
Expand All @@ -34,8 +34,17 @@
declaration this port's `vgi.v2` needs (IVgiService carries it). 0.10.0 derived the
server's name from the C# type and offered no override at all, so against it this repo
does not compile. The bump is additive on the wire — a contract that declares nothing
keeps the name it already answered to. -->
<PackageVersion Include="QueryFarm.VgiRpc.Client" Version="0.10.1" />
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. -->
<PackageVersion Include="QueryFarm.VgiRpc.Client" Version="0.10.2" />

<PackageVersion Include="Microsoft.SourceLink.GitHub" Version="8.0.0" />

Expand Down
91 changes: 75 additions & 16 deletions ci/README.md
Original file line number Diff line number Diff line change
@@ -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)

Expand All @@ -24,19 +24,73 @@ from):
[`preprocess-require.awk`](preprocess-require.awk) rewrites each `require <ext>` into an
explicit signed `INSTALL <ext> FROM {community,core}; LOAD <ext>;`. `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:<port>`. 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`)
Expand Down Expand Up @@ -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:
Expand Down
Loading
Loading