Fix node hang issues: chrome not killed on action_timeout + threadpool/HTTP hangs#719
Conversation
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed The timeout fix is directionally good, but the new cleanup is too broad: it can terminate unrelated Selenium sessions, not just the hung one that triggered the timeout. → Security ReviewStatus: ✅ Completed No security findings in this diff. The change is a timeout-recovery improvement that force-kills hung Selenium browser processes, and I did not see new injection, authz, secrets, or exposure issues introduced. → Performance ReviewStatus: ✅ Completed No performance issues worth flagging in this PR diff; the added process cleanup runs only on timeout/error paths and is not a hot-path regression. → Testing ReviewStatus: ✅ Completed The timeout cleanup change is not covered by a regression test, so the new browser-kill behavior could silently regress.
|
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed One concurrency concern stands out: the timeout recovery now force-kills Selenium sessions by scanning global driver registries, which can impact other in-flight work in the same process. The other changes are directionally sound and don’t show obvious correctness regressions from the diff alone. → Security ReviewStatus: ✅ Completed No security findings in this PR diff. The changes address reliability/timeouts and do not introduce a clear injection, auth, secrets, or data-exposure issue. → Performance ReviewStatus: ✅ Completed One performance regression stands out: the step-report upload now creates a fresh thread pool per step and shuts it down without waiting, which can accumulate background threads and resource pressure if uploads stall. The other timeout changes look like net wins for hang prevention. → Testing ReviewStatus: ✅ Completed The PR fixes two hang-related regressions, but it adds no automated coverage for the new timeout paths. I’d add regression tests for both the browser-kill-on-timeout behavior and the new auth request timeouts.
|
Summary
Two related fixes for nodes getting stuck/hung, bundled together per request:
1. Chrome/chromedriver not killed on action_timeout
action_timeoutpreviously only abandoned the stuck worker thread when a browser action hung, leaving the underlying chromedriver/Chrome OS processes running indefinitely._force_kill_hung_browser_sessions()/_kill_process_tree(), invoked from bothTimeoutErrorbranches in_run_action_with_timeout, which walks each tracked Selenium driver's process tree (viadriver.service.process.pid+psutil) and terminates it (SIGTERM, then SIGKILL after a grace period) whenever an action_timeout aborts a hung action.psutildependency, only ever touches PIDs reachable through the current process's own driver registries, so it cannot affect a co-located node's Chrome sessions, and never raises out of the timeout handler.2. Node event-loop hang from ThreadPoolExecutor + unbounded HTTP calls
run_all_test_steps_in_a_test_caseusedThreadPoolExecutor()as a context manager around a singlesubmit()call --__exit__blocks (waits) for that thread to finish before thewithblock can exit, which can stall the step loop if the report upload is slow. Switched to an explicit executor withshutdown(wait=False)so the upload runs fire-and-forget instead of blocking step progression.renew_token()andlogin()inRequestFormatter.pyhad no timeout on theirrequests.postcalls, so a slow/unresponsive server could hang the node indefinitely on auth. Addedtimeout=70.Test plan
python3 -m py_compileon all changed filessequential_actions.pyandBuiltInFunctions.py🤖 Generated with Claude Code