Track vgi-rpc 0.36: token split, 401 reasons, CORS, introspection, access log - #1
Merged
Merged
Conversation
The upstream TestSticky group gained three tests covering the ways a sticky session must be refused — expiry, a token presented to the wrong worker, and a token replayed under a different principal. Each is gated on a runner-supplied fixture, so until now all three skipped here and Java's session-loss paths went unexercised despite being implemented. --token-key and --sticky-ttl already existed, and RpcServer mints a random server_id per process, so the peer pair differs without any new flag. The only worker addition is --sticky-auth: an Authenticator that resolves the principal named in X-Conformance-Principal and stays anonymous when the header is absent (the suite probes /health and the capability endpoint before it authenticates anything). Declaring that lambda inline means this module names HttpServletRequest at compile time, which vgirpc keeps on `implementation` — hence the compileOnly servlet-api entry rather than a new runtime dependency. Verified the tests can actually fail: dropping --sticky-auth from the fixture makes test_cross_principal_replay_rejected fail with bob resuming alice's session, which is the whole point of the group. TestSticky: 19 passed, 0 skipped. Full suite: 1066 passed, 7 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion, access log Brings the Java port up to the reference's 0.36 protocol surface. Each piece below is pinned by the shared cross-language conformance suite unless noted. **Call/cursor token split.** A stream's state now travels as two tokens: a CallToken minted once by /init carrying the resolved schemas and stream id, and a StateToken cursor re-minted per turn. Cursor v5 -> v6; call token v1. The cursor is opened first so its call id is authenticated before it keys the CallStateCache, then the call resolves from cache or from the client's echoed token. **Token payload compression**, inside the seal, under a codec tag. **Standardized 401s.** The reason is read off the AuthException subtype rather than guessed from message text, so the existing bearer and mTLS authenticators classify correctly without being touched. VGI-Auth-Reason, Cache-Control, the JSON envelope, and a proxy note derived from configuration. **CORS, implemented from scratch** — this port emitted no Access-Control-* headers at all. Origin allowlist, preflight, the expose list built from the same conditions as the capability headers, and Cross-Origin-Resource-Policy. **Token introspection.** Off unless enabled. The endpoint previously 500'd: with an empty prefix the servlet is mapped /*, so every POST path was dispatched as a method name and a JSON body reached the Arrow reader. Paths that name no method now answer 404, which also fixes any wrong-prefix or nested POST path. **Access log**: trace correlation, deterministic per-call sampling, egress accounting (deferred via a per-request scope so response_bytes is the compressed size), key-based claim redaction that fails closed — claims were not emitted at all before — and `truncated: "payload_omitted"`. The scope also supplies http_status, request_id and remote_addr, none of which this port emitted. **X-Request-ID** is now echoed or minted, and exposed. Fixes a latent defect the new access-log gate surfaced: request batches were serialized with no DictionaryProvider, so every enum-taking method silently dropped `request_data` — a required field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`serializeRequestBatch` ran after `reader.drain()`, and draining mutates the reader's root — so every record carried a zero-row batch. The kwargs snapshot immediately above already guarded against this and says so in a comment; the request_data capture did not follow it. Invisible until 0.36.1, whose validator round-trips the field instead of only checking it is present. Java is the one port that emits request_data at all — Go, Rust and TypeScript default to `payload_omitted`, so the new check never inspects anything there and passes vacuously. 121 of 133 records violated the schema before this; all pass now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vgi-rpc 0.37.0 added `--require-request-data`. This port already passes it — payloads are on by default here, and `--access-log-no-payloads` is the inverse flag — which is exactly why Java was the one port that caught a real request_data bug this week while the others passed vacuously by never emitting the field. So this is the CI half only. The step runs the validator unfiltered, so the zero-parameter methods stay in scope: a no-arg call sends an empty schema and no row, and a validator demanding one row unconditionally rejects it — the bug 0.36.1 shipped, which reported this correct port as non-conformant. `--access-log-debug` is accepted as a deliberate no-op. Java gates payloads with a builder flag rather than a logger level, so there is nothing for it to switch, but the arg parser exits 2 on unknown flags, which made the porting guide's literal command fail on Java and only Java. It must stay a no-op: inverting it to mirror `--access-log-no-payloads` would cost this port the default that lets it catch these bugs. Access-log conformance drifted precisely because verification was manual. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three gaps, one shared cause: every Java error path serializes the exception into the response body and then returns normally, so neither the HTTP layer nor the access-log hook could learn from control flow that a call had failed. CallOutcome is a thread-local set at the one choke point every error passes through, Wire.errorMetadata, and opened by RouterServlet.service and RpcServer.serveOne. Two readers depend on it. X-VGI-RPC-Error is now sent (reference 0.37.1's TestErrorHeader asserts it is sent, not merely CORS-exposed). More seriously, the same missing signal meant the access log reported status: "ok" for every raising call on *every* transport, not just HTTP -- the subprocess lane went from 0 error records to 10. HTTP stream calls now emit access-log records. HttpStreamHandler fires the dispatch hook once per turn -- one record per /init, one per /exchange -- matching spec section 1 and the reference's _dispatch_telemetry. The turn opens only once a request is a genuine dispatch, so a malformed body or unopenable cursor still logs nothing, the same boundary Python draws. stream_id is minted at init before mintInitTokens (so a producer finishing in one turn still gets one) and travels in the CallToken, so continuations recover it with no server state. DispatchInfo gains requestState/responseState carrying the decrypted cursor per section 4.4. Also supplies the optional conformance_http_access_log fixture so TestRequestId's correlation case runs instead of skipping, and adds TestHttpStreamAccessLog, which asserts records *exist* with the right shape before validating them -- a validator passing over records that were never emitted is not evidence, which is how this shipped. Known gap left alone: cap-overshoot records still say ok, because writeResponseCapError runs after onDispatchEnd computed the status. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
writeResponseCapError runs after onDispatchEnd has computed the record's status, so a hard cap overshoot was logged as status: "ok" while the response correctly carried X-VGI-RPC-Error: true and the client got an RpcError. Same shape as the raising-call bug in 169967a, different path. It survived because --strict and --access-log had never been combined: the strict-cap fixture writes no log, and the access-log fixture sets no cap. AccessLogScope.close now re-states each parked record from CallOutcome.currentError before stamping response_bytes/http_status. That works because RouterServlet.service opens CallOutcome outside AccessLogScope, so the error is still installed when the scope emits -- a nesting order that is now load-bearing and commented as such. restate only promotes ok -> error, so a record that already named its failure keeps it. Reuses the 169967a choke point rather than adding a second mechanism. Only hard caps promote. A producer soft overshoot stays ok, because continuation tokens cover it and nothing failed. The exchange /init record also stays ok -- promoting the whole call would blame the turn that succeeded. Side effect: sampledIn runs after restate, so an overshoot is now exempt from sampling, as spec section 5bb requires of any error. TestHttpResponseCapAccessLog covers unary overshoot, exchange overshoot (asserting /init stays ok) and the producer soft cap as a control against over-eager re-stating. Watched failing against the stashed fix: 2 failed, 1 passed, with a message naming the real defect. Found while verifying, not fixed here: - max_externalized_response_bytes is advertised and never enforced -- the field is only ever read to emit a header. A worker capped at 512 uploads 200,336 bytes and answers success. The shared suite has no enforcement test for it, so no port is checked. - error_type in the log is the Java class name (RuntimeException) while the wire sends the mapped Python name (RuntimeError), so consumers joining log records to client errors by type will not match. - http_status for an overshoot: Java logs 200 (what went on the wire), Python logs 500 (pre-conversion). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the Java port up to the reference's 0.36 protocol surface.
What's here
CallToken+StateTokencursor, cursor v5→v6, cursor-first resolution, boundedCallStateCacheAuthExceptionsubtype rather than guessed from message text, so the existing bearer and mTLS authenticators classify correctly without being touchedAccess-Control-*headers at allpayload_omitted; the new per-request scope also supplieshttp_status,request_idandremote_addrTwo defects fixed along the way
/__introspect_token__answered 500. With an empty prefix the servlet is mapped/*, so every POST path was dispatched as a method name and a JSON body reached the Arrow reader. Paths naming no method now answer 404 — which also repairs any wrong-prefix or nested POST path, not just this one.DictionaryProvider, so every enum-taking method silently droppedrequest_data— a required field. Surfaced by the new access-log gate onecho_enum.Verification
1118 passed, 7 skipped(the 7 are suite-declared transport limitations)../gradlew buildgreen. Access log validated withvgi-rpc-test --access-logover pipe and HTTP, including the sampling / async / no-payload record shapes.Known gaps (pre-existing, not introduced here)
input_bytes/output_byteshave never been emitted —CallStatisticsis never populated in this port.truncated: trueis unreachable and only"payload_omitted"is emitted.vgi-rpc-test --access-login.🤖 Generated with Claude Code