Repository navigation
fix(preview): reconnect viewers when their tickets expire - #602
Conversation
|
What: Independent review found one blocking mobile picture-in-picture lifecycle regression at Blocking finding[P2] Preserve the mobile picture-in-picture player when the viewer expiresLocation: Trigger: open a qualified remote browser viewer on mobile, enter picture-in-picture, and let the effective viewer-ticket deadline arrive, including while the app is backgrounded. The new timer correctly retires the socket, but its unconditional
Consequently a normal ticket expiry tears down the floating mobile viewer instead of continuing its stream with fresh access. Preserve the active mobile player while replacing stream access, and add a focused lifecycle regression that observes the player surviving a deadline-triggered refresh. The added tests replace only the shared stream client from a mocked callback, so they do not exercise this real consumer's WebView/player lifetime. Inspected scope and cleared concerns
LimitsRead-only source review only. As instructed, I ran no tests, dev servers, browsers, devices, live reproduction or repo-wide checks, and changed no repository files. Native OS PiP presentation was not exercised; the finding is supported by the explicit WebView replacement, capture-track teardown and background visibility paths above. This verdict is one independent review of this exact head, not a complete CI or merge-readiness verdict. Elon recordRequirements and who asked: User requested one independent exact-head review of #588/#602, including real web/desktop/mobile/PiP consumers, lifecycle and the earlier-expiry reconnect acceptance; remote browser continuity supports the Objective through #379/#383. Reviewer: gpt-6.1-sol, high effort, Codex harness. |
|
What: Independent review found one blocking mobile viewer-renewal lifecycle defect at Required fix[P2] Keep same-viewer admission alive through the socket replacementLocation: Trigger: one qualified mobile browser viewer, with no concurrent upload/download retaining its lease, reaches its effective ticket deadline. This includes active background picture-in-picture.
Preserve a valid admitted lifetime until replacement admission succeeds, or obtain a fresh lease while keeping the document/player alive. Merely moving the renewal call ahead of Inspected scope and cleared concerns
Proof and limitsThe task/PR reports the original two failing-before deadline cases, the native regression failing on the prior candidate, 40 passing focused tests, package typechecks, scoped lint/format/diff checks and fork validation. I inspected the actual test implementations and server consumer contract, but did not independently execute those commands or inspect raw local run transcripts. CI was still in progress when inspected, so this verdict does not claim complete CI or merge readiness. As instructed, this was read-only source review: no repository edits, tests, checkout changes, dev servers, browsers, devices, live runs, screenshots, repo-wide checks, version changes, further delegation or merge actions. Native OS PiP was not exercised. The finding is supported by the explicit close-before-renew path and server admission invariants, not a claimed live reproduction. Elon recordRequirements and who asked: User requests independent exact-head review of corrected #588/#602, including real web/desktop/mobile/PiP consumers, earlier-expiry reconnect, cleanup and prior-finding resolution; continuity contributes through #379/#383 to the Objective. Reviewer: gpt-6.1-sol, high effort, Codex harness. |
|
What: Independent review found no blocking findings at Verdict and identityNo supported actionable blocking finding remains in the inspected scope. Reviewed the complete cumulative ten-file diff from original base Read #588 and all comments, the current PR body and comments, the inline-comment endpoint, both prior reviews (first, second), repository AGENTS.md, and the operations verification/code-review procedures. The earlier reviewed heads were Cleared mechanisms
Proof inspected and limitsInspected the actual focused regressions: both earlier-expiry orders and next deadline, retry/terminal/stop cleanup, legacy tickets, native fresh-refresh/no-old-renewal with stable mounted document and exact source identity under background PiP, explicit retry rebuild, browser video/track/active PiP element identity, real atom refresh opening a distinct viewer, and real SessionStore expiry followed by last-transport release, rejected old renewal, then successful fresh issuance/admission/recheck. The parent reports failing-before evidence, 40 passing client/mobile cases plus three real SessionStore lifetime cases (43 total), client-runtime/mobile package typechecks, targeted lint/format/diff checks and fork validation. I inspected source and test assertions, but did not execute those commands or independently inspect raw local run transcripts. GitHub CI still had pending checks when inspected; this is not a complete CI or merge-readiness verdict. This review made no repository edits and ran no tests, checkout changes, dev servers, browsers, devices, live sessions, screenshots, repo-wide checks, version changes, additional delegation or merge actions. The browser regression uses jsdom and fake timers; native OS PiP and real background scheduling remain unverified, as explicitly required by the task's live-run prohibition. Elon recordRequirements and who asked: User requests independent exact-head review of corrected #588/#602 against earlier-expiry reconnect acceptance and real web/desktop/mobile/PiP lifecycle and authority owners; remote browser continuity contributes through #379/#383 to the Objective. Reviewer: gpt-6.1-sol, high effort, Codex harness. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
|
What: This deployment thread now owns following PR #602 through its remaining CI gate, merge readiness and the authorized bestia shipment. Current candidate: Reran only Test Server 5 and its dependent jobs using Deployment constraints carry forward: bestia only; cavallo and rocky remain untouched. The held c6f94f6 stage is not to be re-armed: a shipped candidate must include the merged #605 fix ( User also requests settlement of this deployment thread and Requirements and who asked: User now asks to ensure #602 ships, beyond the original PR-only implementation endpoint; it supplies viewer reconnection needed by #379/#383. |
|
What: The user explicitly authorized overnight completion: PR #602 merged to main, deployed and verified on bestia, with the delivery threads settled if possible afterward. Exact scope remains toolboxmd/chromeria PR #602 -> main and bestia's installed Chromeria service. Existing authorization covers routine fixes, retries and replacement delivery needed for this same outcome, with required proof/review for each candidate. Do not deploy the old c6f94f6 stage because it lacks #605. Do not touch cavallo or rocky, perform logins, expose secrets or introduce persistent services outside the established procedure. Read the runbook bestia Chromeria procedure, shared retention, latest machine records and persistent-host-automation ADR before the live cut. Keep database snapshots, settings-aware rollback, detached swap/observer and parent continuation. If strict idle is unavailable, use only the runbook's cut-and-continue conditions with exact continuation links posted before restart, preserving unsafe tools/background work and unrelated Codex runs. Required live acceptance remains: running artifact identity and finite stability observer; CfT 154.0.8037.92 installed and actually serving agent tabs with expected headless/Vulkan/reduced-UA flags; three preview_open timing samples; Sannysoft and CreepJS webdriver/plugins/WebGL/page+worker UA/headless score; owned-tab cleanup and process inventory. Record actual results and any gaps, update/push dated bestia runbook and CHANGES.md, and post bestia state on #572. The user requests both the #379 planner thread and this deployment thread settled only after successful shipment/acceptance, if possible. Settlement is conditional on completion, not merely PR merge; preserve the thread IDs and tool limitations in the earlier ownership comment. The user is asleep; no deadline guarantee replaces required proof. |
|
What: PR #602 is merged to main as Merge used the repository's supported squash path with The completed original writer had no active/queued work. Its clean exact task worktree was removed normally after a root inode/canary holder scan found no holder or unexpected error. Local and remote task branches were removed without force, and remote absence checked. No unrelated worktree or main's dirty generated files changed. One clean detached Linux build of the merged SHA completed at 22:37:15 CEST. Immutable archive SHA-256 The next bestia runbook record is |
What: Browser viewers refresh access and reconnect when their effective viewer ticket expires.
Why: The shared stream client waited for socket closure, leaving an open but expired remote relay stuck despite the earlier expiry returned by #596.
So what: Web, desktop and mobile receive the shared fix; independent exact-head review passed, and this PR remains open for human merge approval.
Problem
#596 capped renewal metadata at the earlier source/destination ticket expiry, but the client never scheduled against it. A destination-first expiry could leave a browser viewer on a dead relay until another socket event occurred.
Change
DeviceHubAccess.expiresAtfor viewer tickets, then retire the socket and invoke the existing surface-owned access refresh path. Web/desktop and picture-in-picture use their existing access owner. Mobile refreshes its access owner for a fresh viewer lease and passes that access into the existing document, replacing only the socket while preserving its picture-in-picture player and capture track. Explicit retry and process recovery can still rebuild a failed document.docs/map.md.Verification
node_modules/.bin/vp test run packages/client-runtime/src/preview/serverBrowserStream.test.ts -t 'earlier .* expiry': both regression cases failed with zero refresh calls at the effective deadline.node_modules/.bin/vp test run packages/client-runtime/src/preview/serverBrowserStream.test.ts packages/client-runtime/src/state/deviceHubAccess.test.ts apps/mobile/src/features/browser/PreviewStreamWebView.test.tsx apps/mobile/src/features/browser/preview-stream.browser.test.ts apps/mobile/src/features/browser/preview-stream-document.test.ts: 40 passed, using fake timers with no sleeps or polling.node_modules/.bin/tsc-rs --noEmit -p packages/client-runtime/tsconfig.jsonandnode_modules/.bin/tsc-rs --noEmit -p apps/mobile/tsconfig.json: passed (existing suggestions only).node_modules/.bin/vp test run apps/mobile/src/features/browser/PreviewStreamWebView.test.tsx -t 'renews expired access'failed on the first candidate with zero same-viewer renewal calls. It now verifies fresh access refresh (without attempting to renew a retired lease), stable document/source identity during background PiP, and explicit retry recovery. It also fails on the second candidate when that candidate calls the retired same-viewer renewal. The bundled-browser test separately observes the same video, capture track and active PiP element across deadline-driven socket replacement and the next expiry.node_modules/.bin/vp test run apps/server/src/auth/SessionStore.test.ts -t 'admitted .* transport|admits a fresh preview viewer': 3 passed with the real SessionStore and test clock. The new case releases the expired socket first, verifies same-viewer renewal fails, then admits a fresh lease successfully. Shared access tests also verify refresh opens a distinct viewer without sending the retired viewerId. Total focused proof: 43 tests passed.vp lint: passed with one pre-existing inline-schema compilation warning in the test file. Targetedvp fmt --checkandgit diff --check: passed.bash scripts/fork-check.sh --upstream upstream: passed on the committed candidate; 59 registered features.7e289ae1f7and open PR/Issue searches had no applicable viewer expiry fix; details and baseline evidence are in #588.Elon record
Requirements and who asked: User requests the remaining #588 client acceptance: reconnect at the earlier viewer expiry, shared client coverage, failing-before proof, independent exact-head review, and a PR to main without merging; this advances #379/#383 remote browser operation for the Objective.
Deleted: New services, per-client expiry timers, ticket lifetime changes, Spec, version bumps and live verification. Mobile uses its existing fresh-opening owner and a small access-update bridge into its document, preserving the player. Same-viewer renewal after transport retirement is removed.
Bottleneck: The existing shared browser stream client never schedules against viewer access expiry; one shared deadline timer reuses access refresh, while mobile must obtain a valid fresh lease after closing the expired socket, without replacing the document that owns PiP.
Checked myself: Read #588 and all comments, checked #596 and upstream, traced web/desktop/mobile/picture-in-picture consumers, observed the baseline failures, accepted and fixed both independent mobile lifecycle findings against the real server admission contract, and ran focused tests, lint, package typechecks and the fork check.
Closes #588
Model: gpt-6.1-sol. Harness: Codex.