feat(signaling): extract a one-method transport seam (C7) #59
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "c7/transport-seam"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
C7 / Track 4 — the last item on the OSS plan. ⚠ Unverified C++ — build queued behind C5.
The escape hatch for anyone whose broker is not a chromium-network-service WebSocket. Today
SignalingWsClientis the only way an envelope can leave this peer. That is a fine default and stays the default — but it is not a reasonable requirement to impose on a third party, or on a test that wants to drive the offerer without a network stack.One virtual. That's the whole interface.
The plan said "SignalingWsClient already has the exact four methods", implying a four-virtual extraction. I checked what the consumer actually uses rather than copying the class surface:
cb_offerer_driver.cccalls exactly one method —Send()— at four sites (788, 809, 818, 1192).Connect,Disconnect,is_connected,is_closingare called only by the embedder, which owns the concrete client anyway and needs no abstraction to reach it.Exporting all six would also leak implementation detail into a contract third parties then depend on:
SendTextexists so tests can inject malformed frames, which is not something a transport contract should promise. Adding a second virtual later is a smaller change than removing four that were never used.Two deliberate omissions
Inbound is not here. Envelopes arrive via
SignalingClientObserver, which is already abstract and already the seam for that direction. Duplicating it would create two ways to say one thing.The
from-rewrite is stated as a contract obligation, not pretended to be enforceable.SignalingWsClient::SendrewritesfromtokBrowserbefore emitting, and the broker rewrites server-side too — so an implementation forwarding a caller-suppliedfromverbatim disagrees with both. An interface can't enforce that; it can at least say so, which the header does.Wiring
SignalingWsClient : public SignalingTransport,Send()becomesoverrideCbOffererDrivertakes and storesSignalingTransport*:cb_wire_envelopetarget — same kind of artifact (pure contract, no I/O), and its only dependency is that codec, so a separatesource_setwould be a target with no.ccand one depVerification
✅
make verify,cxx-include-lintclean (122 files) · ❌ compiles: unverifiedStacked on
verify/c4-with-testsfor the same reason as #58.The build I fired to prove cb_wire_envelope_unittests passes did not get that far — it failed to COMPILE the test, with: FAILED: cb_wire_envelope_test.o ../../base/memory/raw_ptr_exclusion.h:11:10: fatal error: 'partition_alloc/pointers/raw_ptr_exclusion.h' file not found Root cause: :cb_wire_envelope listed //base in private `deps`. But cb_wire_envelope.h includes "base/values.h" in its PUBLIC interface (ProbeResultPayload holds a base::DictValue), so any target that includes that header needs //base's include dirs transitively — and //base is what pulls in partition_alloc's generated headers. The source_set itself compiled fine (it sees //base directly); only DEPENDENTS broke. With no dependent building, nothing could observe it. That is gn's deps-vs-public_deps distinction exactly: a dep whose headers appear in your own public headers is part of your interface, not an implementation detail. Then the obvious question — is this one target or a class? Scanned every source_set in capture/ for "public header includes base/ AND //base is a private dep". NINETEEN targets, including: pointer_state, input_dispatch_ime, input_dispatch_touch, input_dispatch_drag, input_dispatch_clipboard, cursor_client, cursor_xy_join, embedder, framesink_capture, stats_relay, clipboard_relay, file_upload_relay, active_webcontents_resolver, cb_audio_lifecycle, cb_signaling_ws_client, cb_offerer_driver, cb_dc_host, cb_signaling_reconnect, cb_wire_envelope Note which names appear there: pointer_state and input_dispatch. Their test binaries — cloud_browser_pointer_state_unittests, cloud_browser_input_dispatch_unittests — are two of the five that have never been built in any lane. This is very likely why: whoever last tried to add them hit this same wall and dropped the target rather than the dep. I cannot prove that from here (no record of the attempt), but the shape matches. All nineteen moved to public_deps. The worker binary links them all transitively either way, which is why nothing was broken in production — the defect is only visible to a target that depends on ONE of them in isolation, i.e. a unit test. That is the whole point of unit tests having narrow deps, and it is why this stayed hidden while the tests stayed unbuilt. Verified locally: brace/bracket balance on both BUILD.gn files, no target left with //base in both deps and public_deps, re-scan reports zero remaining affected targets, make verify + cxx-include-lint clean. NOT verified: that it compiles. A t7 build follows — that is the point. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Found while verifying C4's t7 build: my new wire-envelope tests COMPILED and never executed. Neither did the ones that were already there — including the negative test whose own comment says it exists "so a maintainer has to argue the contract change explicitly". I cited that test as a guarantee earlier today. It has never run in this lane. Two independent gaps, same shape: 1. CHROMELESS_BUILD_TARGETS named two unittest targets. The tree declares SEVEN: cb_wire_envelope_unittests <- never built cloud_browser_adm_unittests <- never built cloud_browser_encoder_unittests built cloud_browser_framesink_capturer_unittests built cloud_browser_input_dispatch_unittests <- never built cloud_browser_pcf_unittests (t2-only, deliberate) cloud_browser_pointer_state_unittests <- never built Earlier today I restored encoder+framesink to close a 10-week silent-skip and did not notice I was restoring a two-item list into a seven-item tree. 2. STEP 7 invoked those two binaries BY NAME. So even adding a target to the lane would have changed nothing here — it would build and sit there. The mismatch is invisible: the build goes green either way. That is the same failure shape as the silent-skip STEP 7's fatal-on-missing check was added to close, one level up — there the binary was absent, here it exists and nobody invokes it. STEP 7 now DERIVES the list from CHROMELESS_BUILD_TARGETS (any label whose target name ends in _unittests), so a target added to the lane is automatically executed and cannot drift out of the runner again. The existing fatal-on-missing behaviour is preserved and now applies to every derived binary; a lane with no test targets at all logs a loud WARN rather than passing silently. Adds cb_wire_envelope_unittests to the x264-t7 lane. That is the contract this repo publishes to third parties, so it is the one that most needs to run. The other three unbuilt targets (adm, input_dispatch, pointer_state) are NOT added here: nothing has compiled them in ~10 weeks, they may have bitrotted, and finding that out is its own change rather than something to discover while landing a codec fix. Verified locally (this part is shell, so it is verifiable without Chromium): bash -n clean, and the derivation exercised against a realistic target string — picks exactly the two _unittests labels, correctly ignores cloud_browser_worker and headless:resource_pack_data. NOT verified: that cb_wire_envelope_unittests passes. A t7 build is running to find out; that is the entire point of the change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>localoutside a function killed STEP 7 silently — plus a lintMy own bug, from the previous commit on this branch. The STEP 7 rewrite used local -a test_binaries=() inside an `if` block at SCRIPT TOP LEVEL. bash rejects that outright: bash: local: can only be used in a function Under `set -e` the step aborts instantly and — the expensive part — prints NOTHING of its own. The log reads: === STEP 6/10 autoninja OK (745s) === === STEP 7/10 unit tests START === <end of file> which looks exactly like an infrastructure kill. I spent the first pass chasing reapers, node pressure and hostPath contention before reading my own diff. The compile had SUCCEEDED; only my four extra characters failed. `bash -n` does not catch this. It is a runtime error, not a syntax error, so the script passed every check I ran locally and the real feedback took 27 minutes of t7 build to arrive. Two changes: 1. Drop the three `local` keywords. Top-level variables are already global; the keyword bought nothing. Verified every remaining `local` in the file is genuinely inside a function. 2. tools/lint/shell_toplevel_local_lint.py — because "a four-character mistake costs a quarter hour of Chromium" is exactly the feedback-loop gap this repo's lint directory exists to close, and it is the same argument that produced lint-cxx (a missing #include surfacing hours later in an unwatched lane). Checked against the whole tree before landing, per CONTRIBUTING: clean on all 35 shell scripts, and confirmed to FAIL on the real bug by reintroducing it on a copy. It tracks brace depth, handles both function syntaxes, and skips heredoc bodies so `local` appearing as text in an embedded script is not flagged. Wired into CI's Lint job next to the other host-runnable checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>⚠ UNVERIFIED C++ — capture/ cannot be compiled here (no local Chromium; a build is 4-8h cold). A t7 build is running against this branch; do not merge until it is green. The research first, because the original plan was wrong about two things: 1. Physics is NOT a second dialect. webrtc_signaling.rs's SignalingEnvelope is #[serde(tag="type", rename_all="snake_case")] over variants Offer / Answer / Ice / …, so it emits exactly "offer", "answer", "ice" — byte identical to this codec's accept-list. The `sdp_offer` strings in that file are LOG LABELS (lines 1072, 1096), not wire tags. The plan's "delete physics's serde aliases" step has nothing to delete. 2. The PORTAL is the real second dialect, and it is genuinely incompatible. portal/src/canvas/browser_screencast_webrtc.rs:48-53: {"type":"sdp_offer", "sdp":"v=0..."} {"type":"sdp_answer", "sdp":"v=0..."} {"type":"ice_candidate", "candidate":"...", "sdpMid":"0", …} Flat: no `from`, payload inlined as siblings of `type`. Decode() rejected it on all three counts. It works today only because physics translates in the middle (pattern_c_envelope_to_portal_msg, screencast_ws.rs:6909). This change teaches Decode() the flat dialect and normalizes it to the same Envelope struct. Encode is untouched: we still always EMIT canonical, because an emitter that picked a dialect per-peer would need to know which peer it is addressing, which this codec deliberately does not. What this does NOT do, stated because the plan implied otherwise: it does not make pattern_c_envelope_to_portal_msg deletable. That translator is LOSSY in the other direction — it returns None for bye, request_renegotiate, probe_result and session_unhealthy, so those four tags never reach the portal at all. Removing it needs the portal to learn those tags first. "Liberalize the decoder" and "delete the translator" are two changes, not one. Design notes: * PortalTagFromString is a SEPARATE function from TagFromString, and the test asserts the two tag spaces stay disjoint. Decode() consults both; that is where the widening lives. Keeping it out of TagFromString is what stops "canonical" quietly becoming "anything we accept". * The accept-list stays CLOSED — three tags wider, not open. A new negative test uses `restart_ice`, well-formed in every respect except its tag. * `from` is absent in this dialect and is inferred as kClient. That is a statement (the portal is the browser-facing UI and the only producer of these frames), not a default — a future flat-but-not-client producer must send canonical instead. Callers do switch on `from`, so leaving it indeterminate was not an option. * A flat `ice_candidate` with no `candidate` field is the end-of-candidates marker: canonical says that with `data: null`, which has no flat equivalent because the payload IS the frame. The old RejectsHistoricSdpOfferTag test called sdp_offer "the historic v0 tag" and existed so that flipping the contract required an explicit argument rather than a quiet edit. The framing was wrong on the facts — it is neither historic nor v0, it is live — and this commit is that argument, made in the place the tripwire pointed at. conformance/suites/signaling-wire.mjs moves in lockstep: sdp_offer leaves REJECTED_TAGS for a new PORTAL_TAGS export. Without that the kit would assert the opposite of the code it ships beside. Verified locally: make verify passes, cxx-include-lint clean (121 files), conformance 23/23. NOT verified: that any of this compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>⚠ UNVERIFIED C++ — a t7 build follows. The escape hatch for anyone whose broker is not a chromium-network-service WebSocket. Today SignalingWsClient is the only way an envelope can leave this peer; that is a fine default and stays the default, but it is not a reasonable REQUIREMENT to impose on a third party — or on a test that wants to drive the offerer without a network stack. ONE virtual method. That is the whole interface: class SignalingTransport { virtual bool Send(const Envelope&) = 0; }; The plan described this as "SignalingWsClient already has the exact four methods", implying a four-virtual interface. I checked what the consumer actually uses instead of copying the class surface: cb_offerer_driver.cc calls exactly ONE method, Send(), at four sites (788, 809, 818, 1192). Connect, Disconnect, is_connected and is_closing are called only by the embedder — the code that owns the concrete client anyway and needs no abstraction to reach it. Exporting all six would also leak implementation detail into a contract third parties then depend on: SendText exists so tests can inject malformed frames, which is not something a transport contract should promise. Adding a second virtual later is a smaller change than removing four that were never used. Inbound is deliberately NOT in this interface. Envelopes arrive through SignalingClientObserver, which is already abstract and already the seam for that direction; duplicating it here would create two ways to say one thing. The `from`-rewrite requirement is stated as a contract obligation on the comment rather than pretended to be enforceable: SignalingWsClient::Send rewrites `from` to kBrowser before emitting, the broker rewrites server-side too, and an implementation that forwards a caller-supplied `from` verbatim will disagree with both. An interface cannot enforce that; it can at least say it. Wiring: SignalingWsClient now `: public SignalingTransport`, its Send() is an `override`, and CbOffererDriver takes/stores SignalingTransport* instead of SignalingWsClient*. The header joins the :cb_wire_envelope target — it is the same kind of artifact (a pure contract, no I/O) and its only dependency IS that codec, so a separate source_set would be a target with no .cc and one dep. Verified locally: make verify, cxx-include-lint clean (122 files). NOT verified: that it compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>✅ Build green on t7
Three binaries invoked, three passed. Checked the invocation lines rather than the summary.
That the wire-envelope tests still pass is the relevant signal here:
CbOffererDrivernow holds aSignalingTransport*instead of aSignalingWsClient*, andSignalingWsClient::Sendbecame anoverride— the codec round-trips are what would break if the vtable wiring were wrong.Not verified: that a second implementation actually works, because there isn't one. The interface is exercised only by
SignalingWsClient. A fake transport in a test would prove the seam is usable rather than merely compilable — worth doing when something needs it, not before. Building an unused fake to prove an unused interface would be ceremony.