Repository navigation
feat(s16): guest desktop session with pinned Chromium + desktop lease beat (#410 slice 2b, closes #454) - #455
Conversation
[Medium impact] The reviewed changes appear safe to merge. SummaryAdds a pinned guest browser and desktop session, with a separate provisioning lease and a recurring desktop lease beat.
|
…l; installer verifies the running session (Greptile #455)
|
@greptileai please re-review head f0dff38 (fixes for the 2 P1s and the P2). |
chayapats
left a comment
There was a problem hiding this comment.
Review of f0dff38 (#410 slice 2b + #454)
No blocker in the diff. One SHOULD. CI on this head is still red, so this is not a merge.
SHOULD — a transient inspect during reattach never arms the beat
QemuVmManagement.reattached (src/s15/vm/qemu.ts:484-487) calls inspect() and only then armDesktop(). reattachOne (src/s15/vm/service.ts:504) swallows any throw from reattached ("The VM works without its desktop lease") and does not fence.
health() just before that also calls inspect(), then does SSH identity and egress probes while QMP is free. A snapshot that takes the QMP socket in that window holds it for up to 300s. inspect's qmp times out at 30s as vm_management_unavailable (src/s15/vm/qmp.ts), so reattached's inspect() throws before the timer exists. The VM stays ready. No lease is issued and nothing retries until the next controller restart. That misses #454: a transient inspect failure must skip a beat, not prevent the beat from existing.
Fix: arm the beat before inspect(), and handle a throw the same way desktopBeat does (boundary unproven: revoke and keep the timer; anything else: skip). A VM that is not running should still end the beat.
NIT — the ready check's pgrep -f is an unanchored regex
src/s15/vm/qemu.ts:922. pgrep -f treats the pattern as an ERE and does not anchor it. Probed locally: a process whose argv contains .../chrome_crashpad_handler matches, and so does chrome-157X0X8103X50/chrome because . is any character. The three-in-a-row check makes this narrow, but it can write ready-v1 without the pinned browser. Escape the pattern and anchor it (^.../chrome( |$)), or match /proc/<pid>/exe.
Merge gate, not a code defect
Run 43035519562 on f0dff38 is red. The test jobs die in apt-get (Temporary failure resolving security.ubuntu.com). The deterministic job is only test "failure" = success after those failures. typecheck and Greptile passed. Do not merge until a rerun is green. I could not re-run bun test here (Cannot find module 'schway/host').
Checked, not findings
- Both pins match the published archives, hashed 2026-11-11: linux64
d17167bc092eb495a3b768c7467f27e35f625aa03834233a1663af67dcb668c9(203,527,334 bytes), arm649539c1612121eb09c4b787f4f9d0c732357772eb22583620ffed64d50dd6c4cc. - The writer rejects
$(and newline, quote, backslash). Ran the real program. desktopBeatmatches the #454 ruling for the in-process timer: renew, re-issue, skip a transient throw, revoke on unproven or notproxy-only, end the beat when not running. A qmp timeout isvm_management_unavailable, notvm_boundary_unproven.- Real linux64 zip: no
..entries;chromeis mode 0755. Thedeb.depsstring passed toapt-get satisfyparses. On resolute,libgtk-3-0t64andlibasound2t64Providethe old names, so that part ofdeb.depsis not an obvious 26.04 miss. - The guest program shell-quotes and parses. The forwarder's CRLF literals are real CRLF. It connects only to
192.168.5.100:3128. - Sensitive-host list unchanged (Champ's question). The install lease is unfiltered, as the contract says. The test's 403 on
accounts.google.comis the private DNS answer; the lookup is recorded.
Live Xorg under PAMName=login, and the AppArmor userns profile, still belong to #431. I did not find a path that issues a lease if that health check fails.
Open, not filed: the install lease is 512 MiB and the zip alone is 194 MiB, so apt on that same lease has about 318 MiB left. I did not measure the apt closure, so I am not calling it a defect.
|
⚖️ คำตัดสิน (Tria): เข้าข้าง Duo ยังไม่ผ่านที่ เหตุผล
ทำต่อ
ความมั่นใจ: สูง |
…the session's chrome pgrep (Duo/Tria #455) QemuVmManagement.reattached no longer inspects before armDesktop(): a transient inspect failure (QMP held by a live snapshot) used to throw before the beat existed, leaving a ready desktop VM without a lease or beat until the next controller restart. openDesktopLease already swallows inspect failures and refuses unproven networks; the beat ends itself on a non-running VM. New test fails when the fix is reverted. The installer's pgrep is now anchored and escaped so chrome_crashpad_handler no longer counts as the browser.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Fixes for Tria's ruling (#455 (comment)) in 60104b7:
Local results: |
|
@greptileai please re-review head 60104b7. |
chayapats
left a comment
There was a problem hiding this comment.
Delta review of 60104b7 (f0dff38..60104b7)
Previous SHOULD and NIT are fixed. No new findings. CI on this head is green (run 43044244466, attempt 8). From this review the delta is shippable; Tria still rules.
SHOULD — closed
QemuVmManagement.reattached (src/s15/vm/qemu.ts:486-488) calls armDesktop() and then desktopTurn(() => openDesktopLease(signal)). The inspect() that used to run first is gone, so a throw can no longer happen before the beat exists.
openDesktopLease already swallows an inspect() throw (src/s15/vm/qemu.ts:387: try { if (!this.isolated(await this.inspect())) return; } catch { return; }). A VM that is proven not running ends the beat in desktopBeat (src/s15/vm/qemu.ts:448). repeat is setInterval at 600s, so the first beat does not race the reattach delivery.
The new test (tests/s16-desktop-lease.test.ts:255) throws vm_management_unavailable from qmp for the whole of reattached(), then expects one live beat and a lease on the next beat. Putting the old if ((await this.inspect()).state !== "running") return; back makes reattached() throw before armDesktop(), so await m.reattached() rejects and beats stays empty. CI ran that test: x64 shard 5, job 130674274974, (pass) #455: a transient inspect failure while re-attaching still arms the beat; the next beat issues the lease [311.11ms] inside 644 pass / 0 fail. arm64 shard 5 passed too. I did not re-run the revert locally (no bun on this machine); the fail-on-revert claim is from that control flow, and the pass is from CI.
NIT — closed
The ready check is now pgrep -f '^'+re.escape(B+'/chrome')+'( |$)' (src/s15/vm/qemu.ts:923). Probed with procps-ng 4.0.2: that pattern matches a process whose argv starts with /opt/jit-desktop/chrome-157.0.8103.50/chrome and does not match .../chrome_crashpad_handler or chrome-157X0X8103X50/chrome. The session script execs @B@/chrome as argv0, so the anchor matches the process the installer starts. Live Xorg remains #431.
CI — not this diff, and now green
Attempts 1–7 failed in sudo apt-get update before bun test (that step was skipped). Attempt 7, job 130661520282 (tests x64, 0): Temporary failure resolving 'azure.archive.ubuntu.com', archive.ubuntu.com, and security.ubuntu.com. Compare 51377d1...60104b7 does not touch a workflow file. The guest apt-get is a string that runs inside the VM, not on the runner, so this PR cannot cause that DNS failure.
Attempt 8 started 2026-11-11T17:01:48Z and concluded success at 17:07:39Z: typecheck, all 12 test shards, deterministic, and Greptile. https://github.com/chayapats/jit/actions/runs/43044244466
Checked, not findings
- The only production caller of
reattachedisreattachOne(src/s15/vm/service.ts:504), and only afterinspectplushealthalready showed the VM running. A stop in that window arms a beat that issues nothing and ends itself on the next tick. That is the order Tria required. - The 512 MiB measurement item is on #431 (comment 2026-11-11T15:02:04Z).
- Greptile has no new comments on this commit; its check passed.
No blockers. No new SHOULD.
|
⚖️ Tria ruling — PR #455 @ 60104b7: ✅ PASS (nothing to fix now)
Next: Mono may merge under Champ's rule: every check green on the latest head, Greptile has no open comments, and every review comment is addressed. Before merging, confirm the head is still Confidence: high |
Summary
DESK16-01 (#410) slice 2b, per ADR 0091 decisions 1 and 6. It also closes every REQUIRED item and NIT of #454, which Tria ruled must land before 2b merges.
Guest desktop session (ADR 0091 decisions 1, 6)
qemuDesktopProvisioningis a fixed, idempotent guest program that runs as root over SSH through a separate bounded provisioning lease. The lease is unfiltered, like the Docker install, and is always closed afterwards. The program does this:jit-desktopwith no sudo and home/var/lib/jit-desktop./opt/jit-desktop/chrome-<version>.deb.depslist, plus Xorg (modesetting), libinput, xinit, fluxbox and fonts from Ubuntu's archive.usernsto that browser binary only, so the Chromium sandbox stays on. There is no--no-sandbox.jit-desktop-proxy.service: a loopback forwarder on 127.0.0.1:3129, running asjit-desktop, that addsProxy-Authorizationfrom/etc/jit-desktop/proxy.env. It re-reads that file on every connection, so a re-issued lease applies without a restart. This is needed because Chromium takes no credentials in a proxy URL.jit-desktop.service: Xorg, fluxbox and the browser on vt7 asjit-desktop(PAM login session). The browser is restarted if it exits.qemuDesktopBrowser): Chrome for Testing 157.0.8103.50, Google's version-pinned Chromium build with no auto-updater. It is published for both linux64 and linux-arm64.chrome-linux64.zip:d17167bc092eb495a3b768c7467f27e35f625aa03834233a1663af67dcb668c9chrome-linux-arm64.zip:9539c1612121eb09c4b787f4f9d0c732357772eb22583620ffed64d50dd6c4ccchrome --versionreportsGoogle Chrome for Testing 157.0.8103.50.desktopwas on, the first desktop beat of a later boot installs it through the newprovisionLeasehost option, whichstartLocalVmControlcomposes only when the desktop lease exists. A failed install is tried once per boot and never fails the start. A headless controller installs nothing.#454: one desktop beat per boot (every 10 min)
inspect()throw (QMP busy with a live snapshot, management timeout): skip that beat and keep the lease and the timer.vm_boundary_unproven: revoke the lease (fail closed) and keep the beat.jit-desktopgroup timing: provisioning issues the lease again after cloud-init and the desktop install, so a first boot whose group did not exist yet still ends with a lease. This uses the "re-issue after cloud-init" option that #410 slice 2b required residual: desktop lease beat (renew/retry/skip-transient) + NITs from #453 #454 allows.s15-working-vm-v1contract now says each renewal starts a fresh 512 MiB allowance per period.$, because systemd expands it in an environment file.Docs: updated the
s15-working-vm-v1contract (desktop lease beat and the new "Desktop session" section),s16-computer-screen-v1(the screen now shows the desktop), ADR 0090 "Pending" and ADR 0091 status and decision 6 (which build is pinned).Closes #454
Refs #410
Refs #392
Tests
Tested head:
f0dff38. It fixes Greptile's 2 P1s and 1 P2 onfb2eb92, listed under "Greptile fixes" below.bun test tests/s16-desktop-lease.test.ts: 20 pass / 0 fail. New or changed tests:inspect()fails asqmpExchangedoes after its 30s timeout. The beat is skipped, the lease is kept with no renewal and no cancel, and the next beat renews it.provisionLease.provisionLeaseexists only with the desktop lease and is unfiltered.$.--no-sandboxand has no sudoers entry.Proxy-Authorizationwith exactly one header built fromproxy.env, and returns 503 with nothing sent upstream when no lease file exists.inspect()shows the VM running with networkproxy-only. If the network is unproven, the beat revokes the lease and keeps running, so a VM that is proven again gets a new lease. The new test adds an extra-device./opt/jit-desktop/ready-v1marker first and writes it last. The lease writer requires that marker to name the pinned version and reports "absent" otherwise, so a partial or outdated install is installed again. The new test runs the real writer with only the paths patched.bun test --timeout 30000on s16-desktop-lease, s15-vm-qemu, s15-vm-provisioning, s16-computer-screen, s16-screen-input, s15-vm-proxy, s15-vm-local, s15-vm-vmm, s15-vm-enrollment and s15-vm-pool atf0dff38: 159 / 0.tests/s15-vm.test.tshas 1 local failure onfb2eb92:descriptor-anchored search preserves real rg …. It needs the realrgbinary, which this machine does not have. It is unrelated to this change, and CI installs ripgrep.tsc --noEmit -p .: clean.git diff --check: clean.Honest pending evidence
satisfyofdeb.depson 26.04, rootless Xorg under thePAMName=loginunit on vt7, fluxbox, the AppArmorusernsprofile, and the browser showing on the screen) is checked only by the deterministic tests and the local forwarder and extraction runs above. Live evidence belongs to LIVE16-01 ([Frontier] [LIVE16-01] Collect exact-build Linux, account and native-device evidence #431): the screen shows the browser, the owner signs in to a trusted fixture site, hands back, and the session survives a stop and start.publicWebon the proxy (the default), because storage.googleapis.com is not a registry host.