[dotnet] Terminate driver process when the parent application exits - #17971
[dotnet] Terminate driver process when the parent application exits#17971ashrafiucse wants to merge 1 commit into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
|
||
| // Driver services that are still running. Used to terminate leftover driver | ||
| // processes when the application exits without disposing them. | ||
| private static readonly ConcurrentDictionary<DriverService, byte> ActiveServices = new(); |
There was a problem hiding this comment.
Instead of one global shared state I propose to create dedicated "KillOnCloseJobObject" per DriverService. If it is still performant.
| // A job object terminated on close makes the operating system kill | ||
| // the driver (and browser) processes even when this process is killed | ||
| // abruptly, e.g. stopping a debugging session in an IDE, when no exit | ||
| // event is raised and no cleanup code can run. Failure to set this up | ||
| // must not prevent the driver from starting: the lifetime tracking via | ||
| // the exit event above remains in place either way. |
There was a problem hiding this comment.
I remember user is able to start driver service process and say like "keep browser running even if parent process exits". Is it still possible?
| { | ||
| if (_logger.IsEnabled(LogEventLevel.Debug)) | ||
| { | ||
| _logger.Debug($"Unable to track the driver process in a job object: {ex.Message}"); |
There was a problem hiding this comment.
So low-level, let's move it to Trace level.
| /// Finalizes this instance, terminating the driver process as a best effort | ||
| /// when the user forgot to dispose of this <see cref="DriverService"/>. |
There was a problem hiding this comment.
Pull request overview
Restores .NET driver and browser process cleanup when the parent application exits.
Changes:
- Adds Windows kill-on-close job-object management.
- Adds process-exit tracking and finalizer cleanup.
- Adds a Windows job-object integration test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
dotnet/src/webdriver/DriverService.cs |
Integrates process lifetime tracking and cleanup. |
dotnet/src/webdriver/Internal/KillOnCloseJobObject.cs |
Implements Windows job-object handling. |
dotnet/test/webdriver/KillOnCloseJobObjectTests.cs |
Verifies kill-on-close behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // Driver services that are still running. Used to terminate leftover driver | ||
| // processes when the application exits without disposing them. | ||
| private static readonly ConcurrentDictionary<DriverService, byte> ActiveServices = new(); |
| /// This class is a no-op safety net on non-Windows platforms and must only be | ||
| /// used there; callers are responsible for platform checks. |
|
In general I support the fix for Windows. Let's find hidden stones. Please address feedback and we will look closer. |
d87e0f5 to
f8aed74
Compare
|
Thanks @nvborisenko and @Copilot for the review! All feedback is addressed in the updated commit. 1. Dedicated A side benefit: closing the job handle when the service stops now also cleans up a browser left behind by a driver that was terminated without ending its session first. 2. "Keep browser running even if parent process exits" — is it still possible? — There has never been an explicit API for this in the .NET bindings. Before 4.36 the driver inherited the parent's console and died with it (exactly the behavior #17095 asks to restore); since #16782 it silently survives the parent, which is the reported leak. With this change the coupling is restored on Windows by default. The supported way to keep a driver alive beyond the application remains starting it out-of-process and attaching with 3. Copilot: the finalizer never ran because 4. Low-level logging → 5. Finalizer doc — Reworded generically. 6. Verified on Linux (where job tracking is skipped and behavior is unchanged): |
Code Review by Qodo
1. Job query failure is silent
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| } | ||
| finally | ||
| { | ||
| this.DisposeDriverProcessJob(); |
There was a problem hiding this comment.
1. leavebrowserrunning browsers are killed 📘 Rule violation ≡ Correctness
Normal DriverService shutdown now closes the kill-on-close job, terminating Chromium descendants even when the public LeaveBrowserRunning (detach) option promises the browser will remain running. This incompatibly changes existing public functionality without deprecation or an alternative.
Agent Prompt
## Issue description
On Windows, normal driver disposal closes a kill-on-close job containing the driver and its browser descendants, which breaks the public `LeaveBrowserRunning` behavior.
## Issue Context
`ChromiumOptions.LeaveBrowserRunning` is documented to keep Chromium running after ChromeDriver exits and is serialized as `detach: true`. Preserve that contract while retaining abrupt-parent-exit cleanup for ordinary sessions.
## Fix Focus Areas
- dotnet/src/webdriver/DriverService.cs[404-438]
- dotnet/src/webdriver/DriverService.cs[441-479]
- dotnet/src/webdriver/Chromium/ChromiumOptions.cs[97-100]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| using (KillOnCloseJobObject jobObject = new()) | ||
| { | ||
| jobObject.AddProcess(process); | ||
| } |
There was a problem hiding this comment.
2. Tests bypass driverservice contract 📘 Rule violation ☼ Reliability
The new tests exercise KillOnCloseJobObject directly, but never cover DriverService startup/disposal or the supported detached-browser path affected by the integration. Consequently, they cannot detect the introduced LeaveBrowserRunning regression.
Agent Prompt
## Issue description
The regression tests validate only the job-object primitive and do not exercise the production `DriverService` integration or preserve detached-browser behavior.
## Issue Context
Add focused Windows coverage through the actual service lifecycle, including graceful disposal and a Chromium `LeaveBrowserRunning` case. Keep the lower-level kernel-contract test if useful, but do not rely on it as the sole behavioral coverage.
## Fix Focus Areas
- dotnet/test/webdriver/KillOnCloseJobObjectTests.cs[28-125]
- dotnet/test/webdriver/DriverServiceTests.cs[30-60]
- dotnet/src/webdriver/DriverService.cs[281-284]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (!AssignProcessToJobObject(this.jobHandle, process.Handle)) | ||
| { | ||
| if (_logger.IsEnabled(LogEventLevel.Trace)) | ||
| { | ||
| _logger.Trace($"Unable to add process {process.Id} to the driver job object: {new Win32Exception().Message}"); | ||
| } |
There was a problem hiding this comment.
3. Assignment failure loses tracking 🐞 Bug ☼ Reliability
When AssignProcessToJobObject fails, AddProcess only logs and returns, while DriverService installs no AppDomain.ProcessExit handler or other fallback tracking. In nested-job or already-exited assignment cases, an application exit without explicit service disposal can therefore still leave the driver and browser running—the leak this fallback is meant to prevent.
Agent Prompt
## Issue description
A failed job-object assignment is swallowed after logging, leaving the driver process without the promised application-exit cleanup fallback. If the application exits without disposing the service, the driver and browser can remain alive.
## Issue Context
`DriverService.TrackDriverProcessLifetime` only creates the Windows job and calls `AddProcess`; there is no `AppDomain.CurrentDomain.ProcessExit` subscription or process registry. The finalizer is only best-effort garbage-collection cleanup and does not replace explicit process-exit tracking.
## Fix Focus Areas
- dotnet/src/webdriver/Internal/KillOnCloseJobObject.cs[89-112]
- dotnet/src/webdriver/DriverService.cs[404-429]
- dotnet/src/webdriver/DriverService.cs[204-210]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (!SetInformationJobObject(this.jobHandle, JobObjectExtendedLimitInformationClass, ref information, (uint)Marshal.SizeOf<JOBOBJECT_EXTENDED_LIMIT_INFORMATION>())) | ||
| { | ||
| throw new Win32Exception("Unable to configure the job object to terminate processes on close"); | ||
| } |
There was a problem hiding this comment.
4. Failed configuration delays handle cleanup 🐞 Bug ☼ Reliability
If SetInformationJobObject fails after CreateJobObject succeeds, the constructor throws without disposing the newly owned SafeFileHandle. Repeated configuration failures can retain native job handles until GC finalizes the safe handles, despite the caller catching the exception and continuing normally.
Agent Prompt
## Issue description
The constructor does not deterministically release the job handle when job configuration fails. This can accumulate native handles until garbage collection under repeated failures.
## Issue Context
`CreateJobObject` has already returned an owning `SafeFileHandle` before `SetInformationJobObject` runs. Since construction throws, callers cannot invoke `KillOnCloseJobObject.Dispose`, and `DriverService` intentionally catches the exception and continues.
## Fix Focus Areas
- dotnet/src/webdriver/Internal/KillOnCloseJobObject.cs[64-78]
- dotnet/src/webdriver/DriverService.cs[418-429]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
f8aed74 to
bd3cf69
Compare
|
Thanks for the follow-up review — findings #1 and #4 were valid and are fixed in the updated commit; #2 and #3 are addressed as explained below. #1
#4 constructor leaks the job handle when #3 assignment failure loses tracking — accepted as best-effort. #2 tests bypass the No behavior change on Linux/macOS (job tracking is skipped there); |
| // Entries are never removed — the set holds one small handle per started | ||
| // service and the operating system releases all of them at process exit. | ||
| private static readonly ConcurrentDictionary<KillOnCloseJobObject, byte> DriverProcessJobs = new(); |
There was a problem hiding this comment.
1. Job handles accumulate indefinitely 📘 Rule violation ☼ Reliability
Every Windows driver start adds a strongly rooted KillOnCloseJobObject to the static DriverProcessJobs dictionary, while shutdown only clears the instance field and never removes or disposes the entry, permanently retaining a managed object, native handle, and kernel job object even after all job processes exit. Long-running applications that repeatedly create or restart driver sessions can therefore exhaust process handles and unexpectedly break existing driver-start behavior.
Agent Prompt
## Issue description
The static `DriverProcessJobs` collection permanently accumulates a `KillOnCloseJobObject`, native handle, and kernel job object for every Windows driver start because shutdown paths never remove or dispose dictionary entries. This causes unbounded handle growth in applications that repeatedly create or restart driver sessions.
## Issue Context
A job must remain open while it still contains a detached browser, including when preserving `ChromiumOptions.LeaveBrowserRunning`, but retaining it after the driver exits and the job has no active processes no longer protects anything. Query the job's active-process count after shutdown, remove and dispose empty jobs, preserve jobs containing processes intended to outlive the service, and clean up newly created jobs that never successfully track a process.
## Fix Focus Areas
- dotnet/src/webdriver/DriverService.cs[35-46]
- dotnet/src/webdriver/DriverService.cs[415-449]
- dotnet/src/webdriver/DriverService.cs[450-493]
- dotnet/src/webdriver/Internal/KillOnCloseJobObject.cs[56-145]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit bd3cf69 |
bd3cf69 to
803a9a7
Compare
|
Valid point — fixed in the updated commit. The static registry no longer grows unboundedly:
So accumulation is now bounded by the number of live tracked processes, not by the number of started services. Covered by a new Windows test ( Still no behavior change on Linux/macOS; |
| if (!QueryInformationJobObject(this.jobHandle, JobObjectBasicAccountingInformationClass, out JOBOBJECT_BASIC_ACCOUNTING_INFORMATION accounting, (uint)Marshal.SizeOf<JOBOBJECT_BASIC_ACCOUNTING_INFORMATION>(), IntPtr.Zero)) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
1. Job query failure is silent 📘 Rule violation ◔ Observability
When QueryInformationJobObject fails, TryDisposeIfEmpty silently returns false, leaving the job handle retained without any diagnostic explaining why cleanup failed. This makes process-lifetime tracking and native-handle retention failures opaque to users troubleshooting the new behavior.
Agent Prompt
## Issue description
`TryDisposeIfEmpty` silently returns when `QueryInformationJobObject` fails, so users cannot diagnose why a job handle was not released.
## Issue Context
Capture the Win32 error immediately after the failed native call and log it at the repository-appropriate diagnostic level before returning `false`.
## Fix Focus Areas
- dotnet/src/webdriver/Internal/KillOnCloseJobObject.cs[144-147]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 803a9a7 |
Driver services no longer attach to the parent's console, so on Windows the driver (and the browser it spawns) survive when the application is terminated, e.g. by stopping a debugging session in an IDE. Restore the lifetime coupling with three mechanisms: - place the driver process in a job object terminated on close, so the operating system kills the driver and browser even when the parent is killed abruptly and no cleanup code can run - terminate leftover drivers when the application exits gracefully without disposing them - add a finalizer terminating the driver process when the service is garbage collected without being disposed Fixes SeleniumHQ#17095
|
Fixed in the updated commit: a failed |
803a9a7 to
21d3641
Compare
|
Code review by qodo was updated up to the latest commit 21d3641 |
🔗 Related Issues
Fixes #17095
🔄 Types of changes
📝 Description
Root cause. Since #16782, driver services start with
CreateNoWindow = true, so the driver server executable no longer attaches to the parent application's console. On Windows, that console was what tied the driver's lifetime to the application: terminating the console (e.g. stopping a debugging session in an IDE) used to take the driver — and the browser it spawned — down with it. Now nothing does: when the application is killed abruptly, no dispose/finalizer/exit code runs at all, and the driver and browser processes are leaked.This restores the lifetime coupling with three complementary mechanisms:
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE(Windows, primary fix). The driver process is placed in a job object whose handle is held by the application process. When the application exits — including when it is terminated abruptly viaTerminateProcess— the kernel closes the handle and terminates every process in the job, along with their children (the browser). This is the only mechanism that works when the application is killed without any chance to run cleanup code, which is the exact scenario reported in the issue.AppDomain.CurrentDomain.ProcessExithandler. Terminates leftover driver processes when the application exits gracefully without disposing its services (e.g.Environment.Exit). Works on all platforms.DriverService. Best-effort synchronous termination when a service is garbage collected without being disposed, as suggested in the issue.Notes:
Version note. While the issue report mentions 4.36.0, the regression commit (#16782) first shipped in 4.40.0 (git:
selenium-4.36.0…selenium-4.39.0do not contain it) — the reported intermediate-version testing was likely affected by cached binaries.🧪 How has this been tested?
KillOnCloseJobObjectTests(Windows-only,[Platform("Win")]) verifies the actual kernel contract: a process added to the job object is terminated when the job object is disposed — no mocks, a realpingchild process.DriverServiceTestspass unchanged.net462,netstandard2.0,net8.0) build via bazel;./scripts/format.sh --pre-commitpasses.new ChromeDriver()from an IDE before it disposes) still needs a Windows machine — the unit test covers the same contract continuously on Windows CI.Manual repro steps (Windows):
Before this change:
chromedriver.exeandchrome.exeremain running. After: both are terminated by the operating system.📚 Notes for reviewers