Skip to content

Fix node hang issues: chrome not killed on action_timeout + threadpool/HTTP hangs#719

Merged
sazid merged 2 commits into
devfrom
fix-chrome-hang-on-timeout
Jul 11, 2026
Merged

Fix node hang issues: chrome not killed on action_timeout + threadpool/HTTP hangs#719
sazid merged 2 commits into
devfrom
fix-chrome-hang-on-timeout

Conversation

@mahbd

@mahbd mahbd commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two related fixes for nodes getting stuck/hung, bundled together per request:

1. Chrome/chromedriver not killed on action_timeout

  • action_timeout previously only abandoned the stuck worker thread when a browser action hung, leaving the underlying chromedriver/Chrome OS processes running indefinitely.
  • Orphaned Chrome instances accumulate over time and can exhaust host memory/disk (observed in production: a single 14h-hung Chrome instance filled a node server's disk via verbose debug logging, which then broke unrelated processes on the same host).
  • Adds _force_kill_hung_browser_sessions() / _kill_process_tree(), invoked from both TimeoutError branches in _run_action_with_timeout, which walks each tracked Selenium driver's process tree (via driver.service.process.pid + psutil) and terminates it (SIGTERM, then SIGKILL after a grace period) whenever an action_timeout aborts a hung action.
  • Best-effort and self-contained: uses the already-declared psutil dependency, 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_case used ThreadPoolExecutor() as a context manager around a single submit() call -- __exit__ blocks (waits) for that thread to finish before the with block can exit, which can stall the step loop if the report upload is slow. Switched to an explicit executor with shutdown(wait=False) so the upload runs fire-and-forget instead of blocking step progression.
  • renew_token() and login() in RequestFormatter.py had no timeout on their requests.post calls, so a slow/unresponsive server could hang the node indefinitely on auth. Added timeout=70.

Test plan

  • python3 -m py_compile on all changed files
  • Verified no circular import between sequential_actions.py and BuiltInFunctions.py
  • Reproduced the original chrome-hang bug in production and confirmed root cause
  • Reproduced the si node hang and confirmed root cause (thread-pool blocking shutdown + unbounded auth requests)
  • Exercise an action_timeout in a live node run to confirm the hung Chrome/chromedriver tree is fully killed and the next action starts clean

🤖 Generated with Claude Code

@riz-hossain

Copy link
Copy Markdown
Contributor

🔎 ZeuZ PR Review

Open the full report in ZeuZ: Review findings and apply suggestions

Overview Value
Agents ✅ 4 completed
Suggestions 💡 2

Agent breakdown

→ General Review

Status: ✅ Completed
Suggestions: 1 suggestion

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 Review

Status: ✅ Completed
Suggestions: 0 suggestions

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 Review

Status: ✅ Completed
Suggestions: 0 suggestions

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 Review

Status: ✅ Completed
Suggestions: 1 suggestion

The timeout cleanup change is not covered by a regression test, so the new browser-kill behavior could silently regress.

Open ZeuZ to inspect full findings, continue an agent conversation, or apply safe patch suggestions.

@mahbd
mahbd marked this pull request as draft July 11, 2026 06:05
@mahbd mahbd closed this Jul 11, 2026
@mahbd mahbd changed the title Fix force stop action on timeout not killing chrome instance Fix node hang issues: chrome not killed on action_timeout + threadpool/HTTP hangs Jul 11, 2026
@mahbd mahbd reopened this Jul 11, 2026
@mahbd
mahbd marked this pull request as ready for review July 11, 2026 06:12
@riz-hossain

Copy link
Copy Markdown
Contributor

🔎 ZeuZ PR Review

Open the full report in ZeuZ: Review findings and apply suggestions

Overview Value
Agents ✅ 4 completed
Suggestions 💡 3

Agent breakdown

→ General Review

Status: ✅ Completed
Suggestions: 1 suggestion

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 Review

Status: ✅ Completed
Suggestions: 0 suggestions

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 Review

Status: ✅ Completed
Suggestions: 1 suggestion

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 Review

Status: ✅ Completed
Suggestions: 1 suggestion

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.

Open ZeuZ to inspect full findings, continue an agent conversation, or apply safe patch suggestions.

@sazid
sazid merged commit d78b198 into dev Jul 11, 2026
7 checks passed
@sazid
sazid deleted the fix-chrome-hang-on-timeout branch July 11, 2026 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants