Skip to content

Fix session teardown deadlocks and bound POSIX shell cleanup - #75

Open
xela92 wants to merge 1 commit into
dwservice:masterfrom
xela92:fix/session-shell-cleanup
Open

xela92 wants to merge 1 commit into
dwservice:masterfrom
xela92:fix/session-shell-cleanup

Conversation

@xela92

@xela92 xela92 commented Oct 16, 2026 •

Copy link
Copy Markdown

Problem and reproduction

Closing a shell can make a running agent unresponsive, including its status/heartbeat path. Two cleanup problems compound each other:

  1. Agent.close_session() holds _sessions_semaphore while invoking app callbacks. The shell callback takes the ShellManager condition. Concurrent shell open/close takes that condition first, then takes _sessions_semaphore through Session.inc_activities_value() / dec_activities_value(): an inverse lock order.
  2. LinuxMac.terminate() can call waitpid(pid, 0) while the manager condition is held. If collection stalls, session teardown waits for the manager while retaining the global sessions lock. Closing the Python streams also leaves the PTY master open because they use closefd=False.

This is more than a suspected explanation from logs. On a live macOS agent, an immediate shell open/close reproduced a WebSocket reader blocked in LinuxMac.terminate() / native wait4, a session-close task waiting for the manager condition, and activity/status readers waiting for the global sessions lock. The process remained alive but stopped making status progress. This reproduction establishes the failure mechanism; it does not prove the precise trigger of the earlier incident that prompted the investigation.

Changes

  • Mark closing sessions under the global lock, then release it before session/app teardown. Keep closing sessions registered until callbacks finish, so updates and idle checks cannot mistake unfinished cleanup for an idle agent. Duplicate or stale closes cannot remove a replacement session or call cleanup twice.
  • Snapshot app instances under the app registry lock and invoke callbacks after releasing it. Coordinate idle app unloading and new-session publication with the same apps → sessions lock order. Never wait for an update while holding the app registry lock. Initialize the optional connection timeout and correct the update-wait elapsed condition along that path.
  • Make POSIX terminal cleanup idempotent, track activity registration, and take ownership of the child/PTY before initialization steps that may fail. Explicitly close the PTY master.
  • Stop the current foreground job before closing the owned PTY: TERM, up to 0.5 s grace, then KILL if it is still the foreground group. Reject the agent's own group and groups in another session. A root integration test exposed a foreground sleep surviving shell teardown without this step.
  • Replace blocking child collection with WNOHANG polling: up to 0.5 s after TERM and 0.5 s after KILL. Retain unreaped children in one lazy, nonblocking reaper shared across shell app reloads. It polls once per second only while children are pending, sends no signals, and exits when empty. No periodic worker runs when idle.

These grace periods bound deliberate polling waits, not arbitrary OS scheduling latency or every possible blocking native operation.

Regression tests included

python3 -B tests/test_shell_lifecycle.py

See tests/README.md for setup, limitations and negative controls. The standard-library harness compiles the actual method/class bodies from this checkout, replacing service/bootstrap/network/native dependencies. It does not read installed configuration or require a DWService account.

  • 33 passed with bundled CPython 3.10.5 on macOS, including a separate UID 0 run against the exact production source bytes subsequently installed. The repository-adapted suite was rerun successfully on this PR checkout.
  • Covers lock inversion, blocked callbacks, heartbeat/new-session progress during unreapable-child cleanup, duplicate/stale close, callback failure/mutation, idle-unload/new-open races, update waits, 650 concurrent registry session lifecycles, EINTR/ECHILD, partial initialization, descriptor ownership, and a shared reaper that exits when empty.
  • Real process tests verify shell and foreground-child disappearance before fallback cleanup, including ignored TERM and foreground jobs ignoring both HUP and TERM. In the root run, 30 immediate PTY open/close cycles left descriptors 4 → 4; maximum close time was 0.805 s.
  • Four targeted tests fail on the unmodified base and pass with this patch: callback/global lock release, bounded reaping, PTY master closure, and initialization-failure cleanup. The negative controls were also rerun using the repository-adapted suite.

Additional validation performed outside the repository suite

The following are completed integration experiments, not additional tests invoked by the command above:

  • 16/16 cases through the real DWService relay and a patched root agent: ten immediate shell open/close cycles, command completion with zero and nonzero exit status, end of input, timeout, and interrupted connections. Actual shell PIDs were observed in every case; tested foreground jobs were also observed. After cleanup: no remaining tested shell/job processes, no open PTY masters, zero shell activity/session counts, advancing heartbeat, and an online agent with the same process identity throughout. Abrupt disconnection exercised the existing remote recovery interval rather than immediate orderly cleanup.
  • 5/5 isolated TCP/WebSocket network-fault scenarios, exercising actual DWAgent shell/session method bodies with real PTYs: normal transport; delayed writes at 600 ms and 3 s per write; interrupted TCP; and a blackhole with no disconnect notification. All ended with no tested shell/foreground-child processes, managers, pending children or shell activity before emergency cleanup. Existing recovery/inactivity intervals remained 20/40 s; measured remote cleanup was approximately 20–23 s on the interrupted paths and 40 s for the blackhole.

The live relay tests originated on the same Mac that hosted the remote agent. They do not establish results for a separate originating host. Account/relay bootstrap was substituted only in the isolated network harness.

Scope

POSIX process cleanup was validated on macOS; Linux runtime and Windows terminal cleanup were not exercised. The Windows terminal implementation is unchanged; the shared Agent session-lock changes apply across platforms. Cleanup targets the owned shell and its current foreground process group, not arbitrary daemonized/background descendants. No watchdog, machine reboot policy, installer, credentials or copied deployment configuration is included.

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.

1 participant