fix(desktop-update): acknowledge Windows progress completion
A delayed browser could miss the 900ms terminal event and spin forever after the updater exited. Retain terminal delivery until the page acknowledges it, bound unavailable-client teardown and failed requests, and preserve a truthful final display. Fixes #103747. Builds on OutThisLife and Teknium detached handoff work in #83634 and the #75895 quiet-window design. Continues Axl Ibiza Windows update investigation (#60233, #94107, #100763), including source/review contributions carried by merged #93353 and #85170. Existing #102373, #103140, #95719, #97299 and #103632 retain their separate scopes.
This commit is contained in:
Axl Ibiza, MBA
Teknium
100
apps/desktop/scripts/desktop-update-ui.test.mjs
Normal file
100
apps/desktop/scripts/desktop-update-ui.test.mjs
Normal file
@@ -0,0 +1,100 @@
|
||||
import assert from 'node:assert/strict'
|
||||
import fs from 'node:fs'
|
||||
import { JSDOM } from 'jsdom'
|
||||
import { afterEach, test, vi } from 'vitest'
|
||||
|
||||
// Execute the shipped page, including its inline script, rather than matching
|
||||
// source strings or testing a second implementation of the progress client.
|
||||
const html = fs.readFileSync(new URL('../../../scripts/desktop-update/ui.html', import.meta.url), 'utf8')
|
||||
const windows = []
|
||||
|
||||
function openPage(fetch) {
|
||||
vi.useFakeTimers()
|
||||
const dom = new JSDOM(html, {
|
||||
url: 'http://127.0.0.1:12345/',
|
||||
runScripts: 'dangerously',
|
||||
beforeParse(window) {
|
||||
window.fetch = fetch
|
||||
window.AbortController = AbortController
|
||||
window.setTimeout = setTimeout
|
||||
window.clearTimeout = clearTimeout
|
||||
window.requestAnimationFrame = () => 1
|
||||
window.cancelAnimationFrame = () => {}
|
||||
}
|
||||
})
|
||||
windows.push(dom.window)
|
||||
return dom.window.document
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
windows.splice(0).forEach(window => window.close())
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
test.each(['done', 'manual', 'error'])('renders %s before acknowledging terminal delivery', async status => {
|
||||
let document
|
||||
const requests = []
|
||||
const receipt = '550e8400-e29b-41d4-a716-446655440000'
|
||||
const fetch = vi.fn(async (url, options) => {
|
||||
requests.push(url)
|
||||
if (url.startsWith('/ack/')) {
|
||||
assert.equal(options.method, 'POST')
|
||||
assert.equal(document.body.className, status === 'error' ? 'error' : 'done')
|
||||
assert.notEqual(document.getElementById('title').textContent, 'Updating Hermes')
|
||||
return { ok: true }
|
||||
}
|
||||
return { ok: true, json: async () => ({ status, receipt, message: 'The updater result' }) }
|
||||
})
|
||||
document = openPage(fetch)
|
||||
await vi.advanceTimersByTimeAsync(1000)
|
||||
assert.equal(document.body.className, status === 'error' ? 'error' : 'done')
|
||||
assert.notEqual(document.getElementById('title').textContent, 'Updating Hermes')
|
||||
assert.deepEqual(requests, ['/progress', `/ack/${receipt}`])
|
||||
})
|
||||
|
||||
test.each(['disconnect', 'hung', 'hung-body', 'http', 'invalid'])('bounds %s progress failures without inventing an update outcome', async failure => {
|
||||
let attempts = 0
|
||||
const fetch = vi.fn((_url, options) => {
|
||||
attempts++
|
||||
if (attempts === 1) {
|
||||
return Promise.resolve({ ok: true, json: async () => ({ status: 'running', message: 'Installing dependencies' }) })
|
||||
}
|
||||
if (failure === 'hung' || failure === 'hung-body') {
|
||||
const pending = () => new Promise((_resolve, reject) => {
|
||||
options.signal?.addEventListener('abort', () => reject(new Error('timeout')), { once: true })
|
||||
})
|
||||
return failure === 'hung' ? pending() : Promise.resolve({ ok: true, json: pending })
|
||||
}
|
||||
if (failure === 'http') return Promise.resolve({ ok: false })
|
||||
if (failure === 'invalid') return Promise.resolve({ ok: true, json: async () => ({}) })
|
||||
return Promise.reject(new Error('connection refused'))
|
||||
})
|
||||
const document = openPage(fetch)
|
||||
await vi.advanceTimersByTimeAsync(20_000)
|
||||
assert.equal(document.body.className, 'disconnected')
|
||||
assert.equal(document.getElementById('title').textContent, 'Update status unavailable')
|
||||
assert.match(document.getElementById('line').textContent, /Check Hermes/)
|
||||
assert.ok(attempts <= 4, `unbounded retry loop: ${attempts}`)
|
||||
})
|
||||
|
||||
test.each(['legacy', 'transient', 'ack-failure'])('preserves terminal truth with %s servers', async mode => {
|
||||
let attempts = 0
|
||||
const fetch = vi.fn(async url => {
|
||||
if (url.startsWith('/ack/')) throw new Error('server already stopped')
|
||||
if (++attempts === 1 && mode === 'transient') throw new Error('temporary disconnect')
|
||||
return { ok: true, json: async () => ({ status: 'done', ...(mode === 'ack-failure' ? { receipt: 'test-receipt' } : {}) }) }
|
||||
})
|
||||
const document = openPage(fetch)
|
||||
await vi.advanceTimersByTimeAsync(20_000)
|
||||
assert.equal(document.body.className, 'done')
|
||||
assert.equal(document.getElementById('title').textContent, 'Update complete')
|
||||
})
|
||||
|
||||
test('continues displaying a healthy long update while progress remains reachable', async () => {
|
||||
const fetch = vi.fn(async () => ({ ok: true, json: async () => ({ status: 'running', message: 'Building Desktop' }) }))
|
||||
const document = openPage(fetch)
|
||||
await vi.advanceTimersByTimeAsync(60_000)
|
||||
assert.equal(document.body.className, '')
|
||||
assert.equal(document.getElementById('title').textContent, 'Updating Hermes')
|
||||
assert.equal(document.getElementById('line').textContent, 'Building Desktop')
|
||||
})
|
||||
@@ -99,8 +99,8 @@
|
||||
font-size: 11px;
|
||||
color: var(--foreground);
|
||||
}
|
||||
body.done #loader, body.error #loader { display: none; }
|
||||
body.done #glyph, body.error #glyph { display: flex; }
|
||||
body.done #loader, body.error #loader, body.disconnected #loader { display: none; }
|
||||
body.done #glyph, body.error #glyph, body.disconnected #glyph { display: flex; }
|
||||
</style>
|
||||
</head>
|
||||
<body>
|
||||
@@ -205,6 +205,7 @@
|
||||
const glyphEl = document.getElementById('glyph')
|
||||
const defaultLine = lineEl.textContent /* what a stage-less run says */
|
||||
let settled = false
|
||||
let failures = 0
|
||||
|
||||
const elapsedText = s =>
|
||||
s < 60 ? `${s}s elapsed` : `${Math.floor(s / 60)}m ${s % 60}s elapsed`
|
||||
@@ -226,7 +227,8 @@
|
||||
} else if (state.status === 'done') {
|
||||
settle('done')
|
||||
glyphEl.textContent = '\u2713'
|
||||
lineEl.textContent = 'Opening Hermes\u2026'
|
||||
titleEl.textContent = 'Update complete'
|
||||
lineEl.textContent = 'Opening Hermes\u2026\nYou can close this window.'
|
||||
} else if (state.status === 'manual') {
|
||||
// Update landed but Hermes will NOT reopen itself (package skew,
|
||||
// sandbox helper, launch rejected). The orchestrator leaves this
|
||||
@@ -243,13 +245,43 @@
|
||||
}
|
||||
}
|
||||
|
||||
async function request(url, options = {}) {
|
||||
const controller = new AbortController()
|
||||
const timeout = setTimeout(() => controller.abort(), 5000)
|
||||
try {
|
||||
const response = await fetch(url, { ...options, cache: 'no-store', signal: controller.signal })
|
||||
if (!response.ok) throw new Error('Progress request failed')
|
||||
// Keep the deadline active while reading the body too: receiving
|
||||
// headers alone does not prove that the progress server is responsive.
|
||||
return options.method === 'POST' ? null : await response.json()
|
||||
} finally {
|
||||
clearTimeout(timeout)
|
||||
}
|
||||
}
|
||||
|
||||
async function poll() {
|
||||
try {
|
||||
const res = await fetch('/progress', { cache: 'no-store' })
|
||||
if (res.ok) apply(await res.json())
|
||||
const state = await request('/progress')
|
||||
if (!state || !['running', 'done', 'manual', 'error'].includes(state.status)) {
|
||||
throw new Error('Invalid progress response')
|
||||
}
|
||||
failures = 0
|
||||
apply(state)
|
||||
// The Windows server can now wait for delivery instead of guessing
|
||||
// that one polling interval was enough. Older/POSIX servers omit this
|
||||
// receipt. Apply first so a failed acknowledgement cannot hide a result.
|
||||
if (settled && typeof state.receipt === 'string' && state.receipt) {
|
||||
await request(`/ack/${encodeURIComponent(state.receipt)}`, { method: 'POST' })
|
||||
}
|
||||
} catch {
|
||||
// Server gone: hold the last known state. The orchestrator owns
|
||||
// closing this window; the relaunched Desktop owns the result.
|
||||
if (!settled && ++failures >= 3) {
|
||||
// A vanished server is not evidence of success OR failure. Leave an
|
||||
// honest, finite state even if the browser refuses the close request.
|
||||
settle('disconnected')
|
||||
glyphEl.textContent = '!'
|
||||
titleEl.textContent = 'Update status unavailable'
|
||||
lineEl.textContent = 'The progress connection was lost.\nCheck Hermes for the update result. You can close this window.'
|
||||
}
|
||||
}
|
||||
if (!settled) setTimeout(poll, 400)
|
||||
}
|
||||
|
||||
@@ -108,6 +108,8 @@ $script:UiState = [hashtable]::Synchronized(@{
|
||||
status = "running" # running | done | manual | error
|
||||
message = $script:UiStage
|
||||
clock = $script:UiStopwatch
|
||||
receipt = $null
|
||||
acknowledged_receipt = $null
|
||||
})
|
||||
$script:UiServer = $null # @{ Listener; Runspace; PowerShell; Port; BrowserProc; Profile }
|
||||
|
||||
@@ -190,15 +192,26 @@ function Start-UiServer([string]$HtmlPath) {
|
||||
$request = $reader.ReadLine()
|
||||
# Drain headers so the client doesn't see a reset mid-send.
|
||||
while ($true) { $h = $reader.ReadLine(); if ($null -eq $h -or $h -eq "") { break } }
|
||||
if ($request -match "^GET /progress") {
|
||||
if ($request -match "^GET /progress HTTP/1\.[01]$") {
|
||||
$elapsed = [Math]::Floor($State.clock.Elapsed.TotalSeconds)
|
||||
$snapshot = @{
|
||||
status = $State.status
|
||||
message = $State.message
|
||||
elapsed_seconds = $elapsed
|
||||
receipt = $State.receipt
|
||||
} | ConvertTo-Json -Compress
|
||||
Send-Response $stream "200 OK" "application/json; charset=utf-8" ([System.Text.Encoding]::UTF8.GetBytes($snapshot))
|
||||
} elseif ($request -match "^GET / ") {
|
||||
} elseif ($request -match "^POST /ack/([^ /?]+) HTTP/1\.[01]$") {
|
||||
$receipt = $Matches[1]
|
||||
if ($State.status -in @("done", "manual", "error") -and $State.receipt -and $receipt -ceq $State.receipt) {
|
||||
# Flush acceptance before waking the owner that will
|
||||
# close the listener. No request body is needed.
|
||||
Send-Response $stream "204 No Content" "text/plain" ([byte[]]@())
|
||||
$State.acknowledged_receipt = $receipt
|
||||
} else {
|
||||
Send-Response $stream "409 Conflict" "text/plain" ([System.Text.Encoding]::ASCII.GetBytes("unknown terminal receipt"))
|
||||
}
|
||||
} elseif ($request -match "^GET / HTTP/1\.[01]$") {
|
||||
Send-Response $stream "200 OK" "text/html; charset=utf-8" $HtmlBytes
|
||||
} else {
|
||||
Send-Response $stream "404 Not Found" "text/plain" ([System.Text.Encoding]::ASCII.GetBytes("not found"))
|
||||
@@ -283,11 +296,26 @@ function Stop-UiServer([switch]$LeaveWindow) {
|
||||
}
|
||||
|
||||
function Publish-UiEvent([string]$Status, [string]$Message) {
|
||||
# The event the shim listens for. One beat of poll latency (400ms) before
|
||||
# teardown so the page actually renders the terminal state.
|
||||
# A background browser can miss a fixed 900ms delivery window. Retain the
|
||||
# terminal event until the page acknowledges applying this exact receipt.
|
||||
# Older/headless clients cannot acknowledge, so teardown remains bounded.
|
||||
$receipt = [Guid]::NewGuid().ToString('N')
|
||||
$script:UiState.receipt = $receipt
|
||||
$script:UiState.acknowledged_receipt = $null
|
||||
$script:UiState.message = $Message
|
||||
$script:UiState.status = $Status
|
||||
if ($script:UiServer) { Start-Sleep -Milliseconds 900 }
|
||||
if ($script:UiServer) {
|
||||
$deliveryWait = [System.Diagnostics.Stopwatch]::StartNew()
|
||||
while ($script:UiState.acknowledged_receipt -cne $receipt -and $deliveryWait.Elapsed.TotalSeconds -lt 10) {
|
||||
Start-Sleep -Milliseconds 50
|
||||
if ($script:Ui) { [System.Windows.Forms.Application]::DoEvents() }
|
||||
}
|
||||
if ($script:UiState.acknowledged_receipt -ceq $receipt) {
|
||||
Write-HandoffLog "shim: terminal state '$Status' acknowledged by the window"
|
||||
} else {
|
||||
Write-HandoffLog "shim: terminal state '$Status' was not acknowledged within 10s; closing the progress server"
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
function Get-UiElapsedText {
|
||||
@@ -309,6 +337,8 @@ function Publish-UiProgress([string]$Message) {
|
||||
$script:UiStage = $Message
|
||||
$script:UiState.message = $Message
|
||||
$script:UiState.status = "running"
|
||||
$script:UiState.receipt = $null
|
||||
$script:UiState.acknowledged_receipt = $null
|
||||
if ($script:Ui) {
|
||||
try {
|
||||
$script:Ui.Sub.Text = Get-UiProgressLine
|
||||
|
||||
92
tests/test_desktop_update_windows_ui_delivery.py
Normal file
92
tests/test_desktop_update_windows_ui_delivery.py
Normal file
@@ -0,0 +1,92 @@
|
||||
"""The real Windows update server retains terminal events until acknowledged."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import subprocess
|
||||
import time
|
||||
from contextlib import contextmanager
|
||||
from pathlib import Path
|
||||
from urllib.error import HTTPError
|
||||
from urllib.request import Request, urlopen
|
||||
|
||||
import pytest
|
||||
|
||||
pytestmark = pytest.mark.windows_only
|
||||
SCRIPT = Path(__file__).resolve().parents[1] / "scripts/desktop-update/windows.ps1"
|
||||
|
||||
|
||||
@contextmanager
|
||||
def _server(tmp_path: Path, *, failed: bool = False):
|
||||
powershell = shutil.which("powershell.exe")
|
||||
assert powershell, "Windows updater tests require Windows PowerShell."
|
||||
env = os.environ.copy()
|
||||
env.update(TEMP=str(tmp_path), TMP=str(tmp_path), HERMES_SELFTEST_HOLD_SECONDS="0")
|
||||
env.pop("HERMES_SELFTEST_FAIL", None)
|
||||
if failed:
|
||||
env["HERMES_SELFTEST_FAIL"] = "1"
|
||||
output_path = tmp_path / "ui-delivery.log"
|
||||
with output_path.open("wb") as output:
|
||||
process = subprocess.Popen(
|
||||
[powershell, "-NoProfile", "-ExecutionPolicy", "Bypass", "-File", str(SCRIPT), "-SelfTestUi", "-NoUi"],
|
||||
env=env,
|
||||
stdout=output,
|
||||
stderr=subprocess.STDOUT,
|
||||
creationflags=subprocess.CREATE_NO_WINDOW,
|
||||
)
|
||||
try:
|
||||
deadline = time.monotonic() + 30
|
||||
while time.monotonic() < deadline:
|
||||
text = output_path.read_text(encoding="utf-8", errors="replace")
|
||||
match = re.search(r"SELF-TEST: shim at (http://127\.0\.0\.1:\d+/)", text)
|
||||
if match:
|
||||
yield process, match.group(1)
|
||||
return
|
||||
if process.poll() is not None:
|
||||
break
|
||||
time.sleep(0.05)
|
||||
pytest.fail(f"Update server did not publish a serving URL: {text}")
|
||||
finally:
|
||||
if process.poll() is None:
|
||||
process.kill()
|
||||
process.wait(timeout=5)
|
||||
|
||||
|
||||
def _request(url: str, *, post: bool = False):
|
||||
request = Request(url, data=b"" if post else None, method="POST" if post else "GET")
|
||||
return urlopen(request, timeout=3)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("failed", [False, True], ids=["complete", "failed"])
|
||||
def test_delayed_client_receives_terminal_state_before_acknowledging(tmp_path: Path, failed: bool) -> None:
|
||||
with _server(tmp_path, failed=failed) as (process, url):
|
||||
# A background browser can miss the former 900ms terminal-state window.
|
||||
time.sleep(2)
|
||||
assert process.poll() is None, "the updater discarded its final state before the delayed client received it"
|
||||
with _request(url + "progress") as response:
|
||||
state = json.load(response)
|
||||
assert state["status"] == ("error" if failed else "done")
|
||||
receipt = state["receipt"]
|
||||
assert isinstance(receipt, str) and receipt
|
||||
|
||||
with pytest.raises(HTTPError) as wrong:
|
||||
_request(url + "ack/not-the-published-receipt", post=True)
|
||||
assert wrong.value.code == 409
|
||||
assert process.poll() is None, "an unrelated acknowledgement must not dispose the window's state"
|
||||
|
||||
for route, post in [("progress-other", False), ("ack/" + receipt, False), ("ack/" + receipt + "/extra", True)]:
|
||||
with pytest.raises(HTTPError) as invalid:
|
||||
_request(url + route, post=post)
|
||||
assert invalid.value.code == 404
|
||||
|
||||
with _request(url + "ack/" + receipt, post=True) as response:
|
||||
assert response.status == 204
|
||||
assert process.wait(timeout=15) == 0
|
||||
|
||||
|
||||
def test_no_client_does_not_keep_the_update_process_alive_forever(tmp_path: Path) -> None:
|
||||
with _server(tmp_path) as (process, _url):
|
||||
assert process.wait(timeout=25) == 0
|
||||
Reference in New Issue
Block a user