From acd1df1112e6a2c15e8844e35f36067ec83feba8 Mon Sep 17 00:00:00 2001 From: Dennis Soong Date: Wed, 20 May 2026 02:41:00 +0800 Subject: [PATCH] fix: time out hung browser api requests --- static/panels.js | 2 +- static/ui.js | 6 +- static/workspace.js | 87 ++++++++++++----- tests/test_api_timeout.py | 194 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 264 insertions(+), 25 deletions(-) create mode 100644 tests/test_api_timeout.py diff --git a/static/panels.js b/static/panels.js index 198b6b37..b40252e7 100644 --- a/static/panels.js +++ b/static/panels.js @@ -6364,7 +6364,7 @@ async function checkUpdatesNow(){ if(label) label.textContent=t('settings_checking'); if(status) status.textContent=''; try { - const data=await api('/api/updates/check?force=1'); + const data=await api('/api/updates/check?force=1',{timeoutMs:60000}); if(data.disabled){ if(status){status.textContent=t('settings_updates_disabled');status.style.color='var(--muted)';} } else { diff --git a/static/ui.js b/static/ui.js index 5ab149f7..e07373a0 100644 --- a/static/ui.js +++ b/static/ui.js @@ -4393,7 +4393,7 @@ async function showWhatsNewSummary(target){ } _renderUpdateSummaryPanel({summary:'Writing a simple summary…'},data,target); try{ - const res=await api('/api/updates/summary',{method:'POST',body:JSON.stringify({updates:scopedUpdates,target:target||null})}); + const res=await api('/api/updates/summary',{method:'POST',body:JSON.stringify({updates:scopedUpdates,target:target||null}),timeoutMs:60000}); _rememberGeneratedSummary(target,res,data); _renderUpdateSummaryPanel(res,data,target); _renderUpdateWhatsNewLinks(data,{mode:'summary'}); @@ -4514,7 +4514,7 @@ async function applyUpdates(){ if(window._updateData?.agent?.behind>0) targets.push('agent'); try{ for(const target of targets){ - const res=await api('/api/updates/apply',{method:'POST',body:JSON.stringify({target})}); + const res=await api('/api/updates/apply',{method:'POST',body:JSON.stringify({target}),timeoutMs:120000}); if(!res.ok){ _showUpdateError(target,res); resetApplyButton(0); @@ -4563,7 +4563,7 @@ async function forceUpdate(btn){ const errEl=$('updateError'); if(errEl){errEl.style.display='none';} try{ - const res=await api('/api/updates/force',{method:'POST',body:JSON.stringify({target})}); + const res=await api('/api/updates/force',{method:'POST',body:JSON.stringify({target}),timeoutMs:120000}); if(!res.ok){ if(errEl){errEl.textContent='Force update failed: '+(res.message||'unknown error');errEl.style.display='block';} btn.disabled=false;btn.textContent='Force update'; diff --git a/static/workspace.js b/static/workspace.js index 1511a70a..5309addc 100644 --- a/static/workspace.js +++ b/static/workspace.js @@ -2,39 +2,84 @@ async function api(path,opts={}){ // Strip leading slash so URL resolves relative to location.href (supports subpath mounts) const rel = path.startsWith('/') ? path.slice(1) : path; const url=new URL(rel,document.baseURI||location.href); + const timeoutMs=Object.prototype.hasOwnProperty.call(opts,'timeoutMs')?opts.timeoutMs:30000; // Retry up to 2 times on network errors (e.g. stale keep-alive after long idle). - // Server errors (4xx/5xx) are NOT retried — only connection failures. + // Server errors (4xx/5xx) and client-side timeouts are NOT retried. let lastErr; for(let attempt=0;attempt<3;attempt++){ + let controller=null; + let timeoutId=null; + let didTimeout=false; + let upstreamSignal=null; + let upstreamAbort=null; try{ - const res=await fetch(url.href,{credentials:'include',headers:{'Content-Type':'application/json'},...opts}); - if(!res.ok){ - // 401 means the auth session expired. Redirect to login so the user can - // re-authenticate. This is especially important for iOS PWA (standalone mode) - // and for subpath mounts like /hermes/, where /login escapes to the site root. - if(res.status===401){window.location.href='login?next='+encodeURIComponent(window.location.pathname+window.location.search);return;} - const text=await res.text(); - // Parse JSON error body and surface the human-readable message, - // rather than showing raw JSON like {"error":"Profile 'x' does not exist."} - let message=text; - try{const j=JSON.parse(text);message=j.error||j.message||text;}catch(e){} - // Attach the raw HTTP context so callers can branch on status (404 stale-session - // cleanup, 401 redirect, 503 retry, etc.) without re-parsing the message string. - const err=new Error(message); - err.status=res.status; - err.statusText=res.statusText; - err.body=text; - throw err; + const fetchOpts={...opts}; + delete fetchOpts.timeoutMs; + const useTimeout=Number.isFinite(Number(timeoutMs))&&Number(timeoutMs)>0; + if(useTimeout&&typeof AbortController!=='undefined'){ + controller=new AbortController(); + upstreamSignal=fetchOpts.signal||null; + if(upstreamSignal){ + upstreamAbort=()=>controller.abort(upstreamSignal.reason); + if(upstreamSignal.aborted) upstreamAbort(); + else upstreamSignal.addEventListener('abort',upstreamAbort,{once:true}); + } + fetchOpts.signal=controller.signal; } - const ct=res.headers.get('content-type')||''; - return ct.includes('application/json')?res.json():res.text(); + const requestPromise=(async()=>{ + const res=await fetch(url.href,{credentials:'include',headers:{'Content-Type':'application/json'},...fetchOpts}); + if(!res.ok){ + // 401 means the auth session expired. Redirect to login so the user can + // re-authenticate. This is especially important for iOS PWA (standalone mode) + // and for subpath mounts like /hermes/, where /login escapes to the site root. + if(res.status===401){window.location.href='login?next='+encodeURIComponent(window.location.pathname+window.location.search);return;} + const text=await res.text(); + // Parse JSON error body and surface the human-readable message, + // rather than showing raw JSON like {"error":"Profile 'x' does not exist."} + let message=text; + try{const j=JSON.parse(text);message=j.error||j.message||text;}catch(e){} + // Attach the raw HTTP context so callers can branch on status (404 stale-session + // cleanup, 401 redirect, 503 retry, etc.) without re-parsing the message string. + const err=new Error(message); + err.status=res.status; + err.statusText=res.statusText; + err.body=text; + throw err; + } + const ct=res.headers.get('content-type')||''; + return ct.includes('application/json')?await res.json():await res.text(); + })(); + return useTimeout?await Promise.race([ + requestPromise, + new Promise((_,reject)=>{ + timeoutId=setTimeout(()=>{ + didTimeout=true; + if(controller) controller.abort(); + const err=new Error('Request timed out. Please try again.'); + err.name='TimeoutError'; + err.timeout=true; + reject(err); + },Number(timeoutMs)); + }) + ]):await requestPromise; }catch(e){ lastErr=e; + const isTimeout=didTimeout||(e&&(e.timeout===true||e.name==='TimeoutError')); + if(isTimeout){ + const err=(e&&e.name==='TimeoutError')?e:new Error('Request timed out. Please try again.'); + err.name='TimeoutError'; + err.timeout=true; + if(typeof showToast==='function') showToast('Request timed out. Please try again.',5000,'error'); + throw err; + } // Only retry on network errors (TypeError from fetch), not on HTTP errors // that were already thrown above. Re-throw 401 redirects immediately. if(e.message&&/401/.test(e.message)) throw e; if(attempt<2 && e instanceof TypeError) continue; throw e; + }finally{ + if(timeoutId) clearTimeout(timeoutId); + if(upstreamSignal&&upstreamAbort) upstreamSignal.removeEventListener('abort',upstreamAbort); } } throw lastErr; diff --git a/tests/test_api_timeout.py b/tests/test_api_timeout.py new file mode 100644 index 00000000..3f89f71a --- /dev/null +++ b/tests/test_api_timeout.py @@ -0,0 +1,194 @@ +"""Regression coverage for #2539 client-side api() timeout handling.""" + +from __future__ import annotations + +import json +import re +import subprocess +import textwrap +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +WORKSPACE_JS = ROOT / "static" / "workspace.js" +SESSIONS_JS = ROOT / "static" / "sessions.js" +UI_JS = ROOT / "static" / "ui.js" +PANELS_JS = ROOT / "static" / "panels.js" + + +def _source(path: Path) -> str: + return path.read_text(encoding="utf-8") + + +def _extract_js_function(src: str, name: str) -> str: + marker = f"async function {name}(" + start = src.find(marker) + assert start >= 0, f"{name}() function must exist" + # The api() signature contains a default object literal (`opts={}`), so the + # function-body brace is the first `{` after the balanced parameter list. + paren_depth = 0 + close_paren = -1 + for idx in range(start + len(f"async function {name}"), len(src)): + ch = src[idx] + if ch == "(": + paren_depth += 1 + elif ch == ")": + paren_depth -= 1 + if paren_depth == 0: + close_paren = idx + break + assert close_paren > start, f"{name}() parameter list must close" + brace = src.find("{", close_paren) + assert brace > close_paren, f"{name}() function body must start with {{" + depth = 0 + in_string: str | None = None + escaped = False + in_line_comment = False + in_block_comment = False + for idx in range(brace, len(src)): + ch = src[idx] + nxt = src[idx + 1] if idx + 1 < len(src) else "" + if in_line_comment: + if ch == "\n": + in_line_comment = False + continue + if in_block_comment: + if ch == "*" and nxt == "/": + in_block_comment = False + continue + if in_string: + if escaped: + escaped = False + elif ch == "\\": + escaped = True + elif ch == in_string: + in_string = None + continue + if ch == "/" and nxt == "/": + in_line_comment = True + continue + if ch == "/" and nxt == "*": + in_block_comment = True + continue + if ch in ("'", '"', "`"): + in_string = ch + continue + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + if depth == 0: + return src[start : idx + 1] + raise AssertionError(f"could not extract {name}() body") + + +def _node_eval(script: str, timeout: float = 2.0) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["node", "-e", script], + cwd=ROOT, + text=True, + capture_output=True, + timeout=timeout, + check=False, + ) + + +def test_api_rejects_hung_fetch_with_timeout_and_toast(): + """A hung fetch must reject quickly and surface a recognizable timeout toast.""" + api_fn = _extract_js_function(_source(WORKSPACE_JS), "api") + script = textwrap.dedent( + f""" + const events=[]; + global.document={{baseURI:'http://example.test/hermes/'}}; + global.location={{href:'http://example.test/hermes/',pathname:'/hermes/',search:''}}; + global.window={{location:global.location}}; + global.showToast=(msg,ms,type)=>events.push({{msg:String(msg),ms,type}}); + global.fetch=(url,opts)=>new Promise(()=>{{ + if(opts&&opts.signal)opts.signal.addEventListener('abort',()=>events.push({{aborted:true}})); + }}); + {api_fn} + api('/api/sessions',{{timeoutMs:20}}) + .then(()=>{{console.error('resolved unexpectedly');process.exit(2);}}) + .catch(err=>{{ + console.log(JSON.stringify({{message:String(err&&err.message||err),events}})); + process.exit(0); + }}); + setTimeout(()=>{{console.error('api did not reject after timeoutMs');process.exit(3);}},250); + """ + ) + result = _node_eval(script, timeout=1.0) + assert result.returncode == 0, result.stderr or result.stdout + payload = json.loads(result.stdout.strip()) + assert "timed out" in payload["message"].lower() + assert any(event.get("aborted") for event in payload["events"]), payload + assert any("request timed out" in event.get("msg", "").lower() for event in payload["events"]), payload + assert any(event.get("type") == "error" for event in payload["events"]), payload + + +def test_api_rejects_stalled_response_body_with_timeout(): + """The timeout must stay active through JSON/text body consumption, not only headers.""" + api_fn = _extract_js_function(_source(WORKSPACE_JS), "api") + script = textwrap.dedent( + f""" + const events=[]; + global.document={{baseURI:'http://example.test/hermes/'}}; + global.location={{href:'http://example.test/hermes/',pathname:'/hermes/',search:''}}; + global.window={{location:global.location}}; + global.showToast=(msg,ms,type)=>events.push({{msg:String(msg),ms,type}}); + global.fetch=(url,opts)=>Promise.resolve({{ + ok:true, + headers:{{get:()=> 'application/json'}}, + json:()=>new Promise(()=>{{ + if(opts&&opts.signal)opts.signal.addEventListener('abort',()=>events.push({{aborted:true}})); + }}), + text:()=>Promise.resolve('') + }}); + {api_fn} + api('/api/sessions',{{timeoutMs:20}}) + .then(()=>{{console.error('resolved unexpectedly');process.exit(2);}}) + .catch(err=>{{ + console.log(JSON.stringify({{message:String(err&&err.message||err),events}})); + process.exit(0); + }}); + setTimeout(()=>{{console.error('api body read did not reject after timeoutMs');process.exit(3);}},250); + """ + ) + result = _node_eval(script, timeout=1.0) + assert result.returncode == 0, result.stderr or result.stdout + payload = json.loads(result.stdout.strip()) + assert "timed out" in payload["message"].lower() + assert any(event.get("aborted") for event in payload["events"]), payload + + +def test_api_has_default_timeout_and_per_call_override_contract(): + src = _source(WORKSPACE_JS) + body = _extract_js_function(src, "api") + assert "timeoutMs" in body, "api() must accept opts.timeoutMs as a per-call override" + assert "30000" in body, "api() must default browser API calls to a 30s timeout" + assert "AbortController" in body, "api() must abort hung fetches with AbortController" + assert "delete fetchOpts.timeoutMs" in body, "api() must strip timeoutMs before calling fetch()" + fetch_call = re.search(r"fetch\(url\.href,\{.*?\.\.\.fetchOpts.*?\}\)", body, re.DOTALL) + assert fetch_call, "api() must call fetch() with sanitized fetchOpts" + assert "...opts" not in fetch_call.group(0), "api() must not spread raw opts into fetch()" + assert "timeoutMs" not in fetch_call.group(0), "api() must not forward timeoutMs to fetch()" + + +def test_update_flows_keep_explicit_longer_timeouts(): + """Legitimately long update flows should not inherit the generic 30s guard.""" + src = _source(UI_JS) + panels = _source(PANELS_JS) + assert "api('/api/updates/check?force=1',{timeoutMs:60000})" in panels + assert "api('/api/updates/summary',{method:'POST',body:JSON.stringify({updates:scopedUpdates,target:target||null}),timeoutMs:60000})" in src + assert "api('/api/updates/apply',{method:'POST',body:JSON.stringify({target}),timeoutMs:120000})" in src + assert "api('/api/updates/force',{method:'POST',body:JSON.stringify({target}),timeoutMs:120000})" in src + + +def test_new_session_inflight_cleanup_still_runs_after_api_rejects(): + """newSession() must keep its finally cleanup path so timeout rejections unpin the UI.""" + src = _source(SESSIONS_JS) + start = src.find("async function newSession") + assert start >= 0, "newSession() must exist" + finally_idx = src.find("}finally{", start) + assert finally_idx > start, "newSession() must keep a finally cleanup block" + block = src[finally_idx : src.find("\n}", finally_idx) + 2] + assert "_newSessionInFlight=null" in block + assert "_setNewSessionPending(false)" in block