M2 batch B: <input type=file> works, plus the M1 client gaps (audio, fullscreen, keyboard, token refresh) #109
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "capture/batch-b-file-chooser"
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?
<input type=file>works end to end, verified live on a real guest —tests/interactive --only uploadsis 9/9 oncr7727-f9deaaa7efc6: the page receives a realFile, with the original filename, the right size, and contents matching byte for byte.Full suite on that image plus a gateway rebuilt from this branch: 82 PASS, 1 FAIL, and the one failure is the pre-existing audio-after-re-arm defect — established, not assumed: audio passed with 62 KB on a fresh worker, the pod had served 17 sessions, and restarting it made audio pass again (8361 bytes).
Lane: 639 steps, 0 failed, 0 undefined symbols.
The feature
Both halves were missing, which is presumably why neither had been done:
RunFileChooseroverride — chromium's default auto-cancels, so the page was told "no file selected" before anything could ask the viewer for a byte.fileschannel was bound to a relay whose WebSocket backend was never built (url="off", permanentlydisabled(), dropping every frame).CbFileUploadReceiverterminates the v1 protocol in the browser process: chunks decoded onto a blocking pool task into<profile>/Uploads, the finished file read back off disk to verify the client's sha256, the result resolving the parkedcontent::FileSelectListener. The dead relay's 35 KB is deleted.What it cost: five image rolls, and why
Every one of the three real defects was a silent path, not a wrong answer:
base::AppendToFileopensO_WRONLY|O_APPENDwith noO_CREAT— it cannot create the file it appends towrite_failedfrom a different function, four steps laterreturn, and aVLOG(1)result.firstmoved into argument 3 and read in argument 5, and C++ leaves argument evaluation order unspecifiedThe last one was masked by an error message naming a cause that never happened:
"could not read the file back", about a read the logs eventually proved had never failed once. That string steered the search through the filesystem, permissions, the task runner, and a genuineend/write race before the line itself.Two theories were refuted and are recorded as such in
docs/findings/file-upload-chunk-vanishes.md— the task-runner swap fixed nothing, and theend/write race was real (its fix stands) but was not this. "Refuted" is the half of a diagnosis that vanishes if only the winning answer gets written down.Four lints, each from a defect that got past every local check
lint-cxx-use-after-movelint-cxx-orphan.hand defined in no.cc— strictly stronger than the linker, which needs a call to noticelint-yaml-dupe-keysbuild-job-x264-t7.yamlargued for 64Gi in twenty lines of comment while every build got the 72Gi on the next line, for three weeksEach checked against the whole tree before landing, with negative controls run both ways. The orphan lint found two false positives first (
RTC_GUARDED_BY,ABSL_GUARDED_BY); the use-after-move lint's first version produced 14, all false, because it ignored statement boundaries.Also: the kaniko push deadline was 300s against a 210 MB layer that now takes longer — two pushes died at 82% with the registry logging
client disconnected during blob PATCH. Raised to 900s with the measurement replacing the old "~30-60s" belief, and aDeadlineExceededfailure now says so instead of looking like a credential error.M1 client gaps, same branch
<video muted>with no control meant every byte was decoded and discarded.fullscreen_changedeventcontrol.tslogged and dropped since it was written.clipboard_pasteis in the guest'skKnownInputTypes, so it was accepted and handled nowhere.TokenRefresherfinally has a caller — tokens live 5-15 min, the login cookie 12 h, and the socket froze the token into its URL at construction.statsevent has fired once a second since T82 with no subscriber. Now9 fps / 1.5 Mbps / 1280x720on screen./jsonand threw it away.Client tests 333, new ones mutation-checked.
A file input on a streamed page did nothing at all, and it took two independent fixes to explain why: 1. CbWebContentsDelegate had no RunFileChooser override. Chromium's default auto-cancels, so the page was told "no file selected" before anything could ask the viewer for a byte. 2. The "files" DataChannel was bound to CbFileUploadRelay, which forwarded frames to a WebSocket bridge (capture/file-bridge/) over a client constructed with url="off" — permanently disabled(), dropping every frame. Same shape as the clipboard relay's four-month outage, and the log line even said so. Either fix alone changes nothing, which is presumably why neither happened. Both land here. CbFileUploadReceiver terminates docs/protocols/file-upload.md v1 in the browser process: chunks are base64-decoded onto a MayBlock sequence into <profile>/Uploads, the finished file is read BACK off disk to verify the client's sha256 (what is verified is what the page will get), and the result resolves the content::FileSelectListener that RunFileChooser parked. The sidecar is deployed by nothing today, so nothing regresses; its 35 KB of unreferenced source is deleted. Read from the pinned tree rather than remembered, and two drafts were wrong until it was: * NativeFileInfo::display_name is what the page reads as File.name. Left empty, blink falls back to the base of file_path — our sanitised "<id>__<name>" — so a site echoing the filename or checking its extension would see the wrong thing. The raw name is now carried alongside the on-disk one. * FileSelected's base_dir is empty except for kUploadFolder (file_select_listener.h:24). We reject that mode, so it is always empty; passing uploads_dir_ advertised an enumeration root that described nothing. Two more defects found by re-reading, not by the compiler: * Upload::received was never incremented, so progress always reported 0 and the cumulative size guard tested 0 > declared_size — i.e. never fired. It now counts DECODED bytes; an earlier draft passed the base64 length through, which overstates by a third. * Every abandon path left partial bytes on disk. Since the session quota only credits uploads that SUCCEED, a client retrying a bad hash could fill the guest's disk without ever tripping it. All six paths now go through AbandonUpload (erase + delete + reply). The listener contract (resolve exactly once, or chromium CHECKs on release) is held from three directions, because no one of them is enough: the receiver runs its own 5-minute deadline (the control request's is cancelled the moment the viewer answers, so a viewer who says "sending" and then vanishes would strand it); re-arm and teardown cancel before destroying; and the client answers every branch, including when its own picker throws. Client: control.ts learns kind=file_chooser and main.ts shows a picker bar, since a file dialog needs a user gesture and this request arrives from the network. Five new tests, two mutation-checked. Docs: the protocol spec's server half was describing the sidecar. Its MIME allowlist and virus scan do not exist here and are now marked NOT IMPLEMENTED rather than left as claims; target_selector is ignored; attached_via can no longer be domSetFileInputFiles; the error-code table is replaced with the codes actually emitted. UNVERIFIED: capture/ does not compile locally. The build lane has not run on this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>build-job-x264-t7.yaml carried this, and had since 2026-08-21: memory: "64Gi" memory: "72Gi" Twenty lines of comment above the first argue — with measurements from two OOM evictions — for 64Gi. YAML's last-key-wins gave every build 72Gi. The 64Gi edit had been inserted ABOVE the existing line instead of replacing it. Nothing surfaced it in three weeks. kubectl apply accepts a duplicate mapping key silently, PyYAML accepts it silently, both values schedule on triform-7, so every build worked and the manifest simply did not mean what it said. No other manifest in the repo has one — the other five build jobs have exactly two `memory:` keys, which is requests + limits. The class matters more than the instance. This repo's manifests are heavily commented precisely BECAUSE the numbers are load-bearing and hard-won (CLAUDE.md's OutOfcpu/OutOfmemory traps are about these exact lines), and a silently discarded value makes every one of those comments untrustworthy. Same shape as "a guard whose skipped path is silent is not a guard": the discarded value reports identically to the applied one. So: lint-yaml-dupe-keys, wired into `make lint`. Checked against the whole tree before landing (CLAUDE.md's rule) — zero findings across 59 files once t7 is fixed, and the 14 Go-templated Helm files are skipped BY NAME with the skip reported, so the lint can never quietly check nothing. Negative control run both ways: reintroducing the exact defect fails it at the right line; removing it passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>`InputChannel.attach` wired the browser's `paste` event to a `clipboard_paste` envelope on the INPUT channel, while `main.ts` separately sent `clipboard_offer` on the CLIPBOARD channel. Two envelopes per paste, one of which does nothing. Nothing surfaced it, and the reason is worth writing down: the guest lists `clipboard_paste` in `kKnownInputTypes`, so the dispatcher accepts it and does NOT report it through OnInputEventUnknownType. It is dropped silently by a lookup table that says it is known. `cb_input_dispatch_clipboard.h` documents the gap on purpose ("we leave the dispatch surface unclaimed here") and the M4 rank that was to claim it never landed. Removed the listener, not the sender: `sendClipboardPaste` stays for anyone driving the channel directly, now carrying a comment saying where it goes (nowhere). `clipboard_copy_request` stays wired — that one IS consumed, arming the guest's copy window (cb_input_dispatch_composite.cc:77), so removing the copy listener would have broken a working feature while chasing a dead one. The spec claimed "the server writes this into the remote clipboard and synthesizes a paste action". It does not, and has never. Marked as specified-but-unimplemented with a pointer to `clipboard_offer`, which is what works. The envelope itself stays in v1 — removing a type is a wire break, and a future implementation may still claim it. Two tests, mutation-checked by putting the listener back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>The build lane failed on exactly one thing across 638 compile steps: cb_file_upload_receiver.cc:183:74: error: too few arguments to function call, expected at least 2, have 1 base::JSONReader::ReadDict(json) `options` has no default at 7727. Every other ReadDict caller in this tree already passes base::JSON_PARSE_RFC — cb_control_channel.cc:257, cb_clipboard_relay.cc:109, cb_devtools_agent.cc:664 and :750. This is the check CLAUDE.md prescribes ("grep for existing uses of it in capture/ before using a Chromium API"), and I ran it for AppendToFile, crypto:#️⃣:Sha256, GetDeleteFileCallback, FileSelected and the chooser mojom, and skipped it for this one line. Everything else in batch B compiled: cb_web_contents_delegate.o and cloud_browser_browser_main_parts.o both built clean, so the RunFileChooser override, the receiver wiring, the re-arm/teardown cancellation and the new GN target are all confirmed by the compiler. Re-verified the remaining APIs against the pinned tree afterwards (JSONWriter::Write, ThreadPool::CreateSequencedTaskRunner, OneShotTimer::Start, PostTaskAndReplyWithResult, base::DictValue). Also in this commit: TokenRefresher finally has a caller. It has existed, implemented and unit-tested, with none. Tokens carry a 5-15 minute TTL while the gateway's login cookie lasts 12 hours, and ReconnectingWebSocket froze the signaling URL — token and all — at construction. So any session that outlived one TTL and then had to redial presented an EXPIRED credential; the broker rejects that at the handshake, which is indistinguishable from "the broker is down", and it gets more likely the longer a session runs. Precisely when reconnecting matters. The socket now takes a URL SUPPLIER (a plain string still works, so every existing caller is unchanged) resolved per dial, and the session adopts the token it already fetched rather than issuing a second one. TokenRefresher gained adopt() + scheduleNext() for that. It is stopped on teardown: a live refresher after disconnect keeps minting credentials every 15 minutes for a session nobody holds. Four tests. The redial one is mutation-checked by re-freezing the URL at construction, which reproduces the original bug exactly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>The lane caught this in ~13 minutes: ld.lld: error: undefined symbol: cloud_browser::CbFileUploadReceiver::OnFinalised(...) My HandleEnd rewrite replaced a range that ran from one function to the next and took two OTHER bodies with it. OnFinalised and ResolveChooserWith were still declared, still called, defined nowhere. Both restored verbatim from546f14b. `make lint-cxx` was clean — it checks includes. `make verify` was clean — it cannot compile capture/. So a defect a script can find in milliseconds cost a full lane cycle, on the one component where a cycle is 20 minutes plus an image roll. So: lint-cxx-orphan. It is strictly stronger than the linker, which only notices when a CALL exists — this also catches a method declared, defined nowhere, and not yet called, which links today and breaks whoever adds the first caller. Checked against the whole tree before landing (CLAUDE.md's rule). It found two false positives on the first run — RTC_GUARDED_BY and ABSL_GUARDED_BY, thread-safety macros that trail a member declaration and parse exactly like a call — which are now excluded by suffix. Clean across all 53 header/impl pairs after that. Negative control run both ways: deleting OnFinalised's body fails it at the right symbol, restoring it passes. It deliberately under-reports (skipping inline bodies, pure virtuals, = default/delete, templates) because a false positive costs more trust than a missed defect costs time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>THE cause, found by instrumenting rather than theorising: CV2-UPLOAD: WriteFile -> ok, 27 bytes; file is now 27 at .../Uploads/... CV2-UPLOAD: finalising ...; hashing .../Uploads/... CV2-UPLOAD: upload ... failed: could not read the file back The write succeeded. The file was 27 bytes on disk. And the ERROR I had added to HashFile's read-failure path never printed once — so the read succeeded too. Both halves worked and the caller still saw failure. self->OnFinalised(std::move(id), std::move(p), std::move(result.first), result.second, !result.first.empty()); // argument 5 `result.first` is moved into argument 3 and read in argument 5. C++ leaves argument evaluation order UNSPECIFIED; on this toolchain argument 3 won, so `ok` was computed from a moved-from empty string. Every upload reported failure while the file sat correct on disk. Compute the flag into a named local first. Never read a value in the same call that moves it. Five image rolls went into this, and the reason is that the error message named a cause that had not happened: "could not read the file back" sent me through the filesystem, the task runner, and a genuine end/write race (that one was real, and its fix stands) before the line itself. So the message is now `hash_failed` and says only what it knows, with the read logging its own failure separately. The spec's code table records the old lie. Two things landed to make the class cheap next time: - `lint-cxx-use-after-move`. Only the UNSEQUENCED case — a move and a read of one name inside a single argument list. A first version that ignored statement boundaries produced 14 findings, ALL false (`auto x = std::move(m_); m_.clear();` is legal and deliberate), and a second matched `chunk_bytes` as a read of `bytes`. Both fixed; clean across 76 files, negative control reintroduces the bug and it fails at the right line. - The AppendChunk / HashFile / FinaliseUpload logging that produced the three facts above in ONE run, after four cycles of reasoning from the outside had produced none. UNVERIFIED: the lane has not run on this. The uploads suite should reach 9/9; that is the next roll. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Two pushes of the same image failed in a row today, and neither was a push error. The Job event says DeadlineExceeded and the registry says: error msg="client disconnected during blob PATCH" contentLength=210758335 copied=172730490 error="unexpected EOF" 82% of a 210 MB layer transferred, then activeDeadlineSeconds severed it. The comment above that setting asserted "cold layer push is ~30-60s"; the eight successful pushes earlier the same day took 109s-4m21s, so the headroom was already thin and something — registry load, node egress — pushed it over. 300s -> 900s, which is ~4x the slowest observed success. The deadline still stops a genuinely wedged job from holding node capacity; it just no longer kills one that is merely slow. Two things that made this read as a code problem, both fixed: - The script's `wait --for=condition=complete` was 600s, UNDER the Job's own deadline, so a slow push returned "indeterminate" before the Job had decided anything. - A DeadlineExceeded failure printed the same "✗ kaniko-push FAILED" as a credential or Dockerfile error. It now names the reason and points at the registry log line above. The tell is that the last log line is "Pushing image to ..." with no error after it. Found while chasing a deploy of my own that I should also record: I read a failed push as still-running from tail output, then piped an empty digest capture into `kubectl set image`, which the API accepted and left the worker in InvalidImageName for ~2 minutes. Restored from the previous digest. The script's verdict line was in the file I was already reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>01fe71180388dea1eb51