mirror of
https://github.com/nesquena/hermes-webui.git
synced 2026-05-25 11:10:18 +00:00
fix: time out hung browser api requests
This commit is contained in:
Dennis Soong
+1
-1
@@ -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 {
|
||||
|
||||
+3
-3
@@ -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';
|
||||
|
||||
+66
-21
@@ -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;
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user