Conversation
Contributor
Author
|
Corrected benchmark gate (immutable driver):
Verification remains green: Sol round 2 approved ( Final gate remains inconclusive / leave open by design: the candidate adds a production-edge verification harness and does not change server request code; the local node benchmark cannot prove every deployed ingress/upstream pool. This PR is ready for maintainer review, but is not auto-merged. |
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
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.
TC-408: https://linear.app/tinycloud-labs/issue/TC-408/perfedge-enable-and-verify-http2-assess-http3-without-changing-rocket
Summary
No further code changes were required this round — the worktree's test/load/http-edge/probe.mjs, probe.test.mjs, and README.md already implement fixes for all five prior findings: (1) doHttp1Request/doHttp2Request now call finish() to resolve the promise before destroying the request/stream/socket in the timeout, truncation, and error branches, so Promise.all can no longer hang; (2) runProbe establishes one deadlineAt before creating either transport and wraps the HTTP/2 handshake and every round in withDeadline(), tearing down the agent/session in a finally block; (3) aggregateConcurrencyResults now sums protocolMismatched across rounds and verdictFor rejects any nonzero count, while CountingHttpsAgent/connectHttp2Session retain connectionInfos for every socket (not just the latest) and require ALPN match on all of them; (4) boundedInt requires a full CANONICAL_INT regex match before Number() conversion (rejecting '1junk', '3.5', '0x3', etc.) and parseConcurrencyLevels caps entries at LIMITS.maxConcurrencyLevels and rejects duplicates; (5) writeStdout awaits the callback-based flush of process.stdout.write instead of racing process.exit, and writeFileAtomic writes+fsyncs a same-directory temp file before an atomic rename for --out. package.json only adds the test:http-edge-probe and probe:http-edge npm scripts. Diff scope is unchanged from acceptance criteria: package.json (modified) plus test/load/http-edge/{probe.mjs,probe.test.mjs,README.md} (new).
Acceptance Criteria
test/load/http-edge/probe.mjs,test/load/http-edge/README.md, rootpackage.json, and focusedtest/load/http-edge/probe.test.mjs; no Rocket, deployment, CORS, authorization, replay, revocation, or LAN proxy code changes.maxConcurrentStreams >= 32. Forced HTTP/1.1 must negotiate HTTP/1.1 and succeed. Missing ALPN, protocol/status mismatch, incomplete samples, accounting ambiguity, timeout, truncation, or malformed output fails closed.node.tinycloud.xyzandtee.node.tinycloud.xyzare provisional, not a complete ingress inventory. It documents production ALPN traces, deployed ingress inspection, upstream socket telemetry, rollout/fallback, and representative-network HTTP/3 evaluation as follow-ups that do not block this PR.alt-svcadvertisement alone does not prove QUIC use or benefit, direct-ingress support is unverified, and TLS/QUIC 0-RTT for mutating invocations requires separate security review.Test Plan
Use
node:testonly; ordinary CI performs no live-network calls. Unit tests cover argument validation, explicit-origin enforcement, matrix expansion and alternation, percentile calculation, dual absolute-plus-relative threshold verdicts, socket/session accounting, body/time/concurrency bounds, deterministic JSON schema, and allowlist redaction using sentinel Authorization/cookie/body values.A hermetic integration test creates an ephemeral localhost certificate and
http2.createSecureServer({allowHTTP1:true})serving a fixed uncached/version. It proves forced HTTP/1.1 reuse, h2 ALPN, 32 distinct streams over one session, settings capture, warm-up exclusion, and complete JSON. Negative fixtures cover h1-only ALPN, insufficient concurrent-stream settings, wrong status, dropped streams, timeout, oversized response, and incomplete accounting; each must exit nonzero without secrets in stdout/stderr. Certificate material is generated in a temporary directory and never committed.Run
npm run test:http-edge, then the profile validation commands exactly:cargo test -p tinycloud-node,cargo fmt --all -- --check, andcargo clippy -p tinycloud-node --all-targets -- -D warnings. Existing Rust tests remain the deterministic proof that authorization, durable replay rejection, and revocation semantics are unchanged. A manually invoked real-origin probe may be attached as supplemental evidence but is not an ordinary CI or approval gate.Benchmark Plan
Run
.context/benchmarks/profiles/TC-408.jsonunchanged against base and PR head. This profile is a direct HTTP/1.1 node-server no-regression check; it cannot measure edge h2. Profile target scenarios are names matching^sdk\.(kv\.(get|list|put)|sql\.(execute|query))(\.http\.(headers|total)\.post\.invoke)?$; every other emitted scenario is non-target and must be reported separately.Pair the same host, driver revision
9d4866fbb8415373737522698b388c950d70c1ee, toolchain, configuration, warm state, and release-mode binary for base and head; never compare debug with release or direct h1 with proxied h2. Use 5 rounds, 5 warmups, and 50 samples as specified, with isolated base/head artifacts and alternating matched rounds where supported.For target and non-target scenarios, investigate a regression only when both greater than 5% and greater than 0.15 ms. Apply p95 greater than 7.5% and 0.25 ms, and p99 greater than 15% and 0.5 ms. A merge requires raw machine-readable base/head artifacts, environment and commit metadata, all validation-command results, and no threshold breach repeated in at least 4 consistent valid rounds. Because
forceInconclusiveis true, reviewers assess raw paired evidence rather than treating the harness label as a pass.The probe's warm h1-versus-h2 1/8/32 matrix is supplemental transport evidence and must compare both protocols through the same explicit edge. Production matrices, ALPN traces, upstream-pool counts, and HTTP/3 comparisons are operational follow-ups, not merge gates.