Skip to content

fix: ensure Python backend shm cleanup on forced shutdown#450

Open
Vinya567 wants to merge 4 commits into
mainfrom
vinyak/fix-jetson-shm-shutdown-cleanup
Open

fix: ensure Python backend shm cleanup on forced shutdown#450
Vinya567 wants to merge 4 commits into
mainfrom
vinyak/fix-jetson-shm-shutdown-cleanup

Conversation

@Vinya567

@Vinya567 Vinya567 commented Jul 18, 2026

Copy link
Copy Markdown

What does the PR do?

Runtime fix for orphaned triton_python_backend_shm_region_* files when Python backend stub shutdown exceeds the server exit timeout. Pairs with the QA-side change in server#8881.

Ticket

Root cause

  • stub-timeout-seconds was parsed at backend init but not applied inside TerminateStub — the wait on the stub was effectively unbounded.
  • Parent-owned shm regions were only removed via the SharedMemoryManager destructor, so a forced/abnormal exit could skip cleanup and leak /dev/shm/triton_python_backend_shm_region_*.

Fix

  • Wire stub_timeout_seconds_ through StubLauncher::TerminateStub with a bounded pop + WaitForStubProcessWithTimeout; fall back to KillStubProcess on timeout (src/stub_launcher.{cc,h}).
  • Add explicit SharedMemoryManager::RemoveShmRegion() and call it from TerminateStub (src/shm_manager.{cc,h}).
  • Register parent-owned shm regions for std::atexit cleanup as a safety net for forced exits.

Test plan

  • IGX-Orin GitLab job 366443271: L0_backend_python--IGX-Orin lifecycle PASSED with this PR + server#8881; /dev/shm shm-region count unchanged before/after run.
  • x86: no regression on L0_backend_python/lifecycle.

Checklist

  • PR title reflects the change and is of format <type>: <description>
  • Changes are described in the pull request.
  • Related PRs are referenced.
  • Added test plan and verified test passes.

Commit Type

  • fix

Register parent-owned shm regions for atexit cleanup, remove regions
explicitly in TerminateStub, and honor stub-timeout-seconds during stub
teardown to avoid orphaned regions when server exit times out.
- Bump copyright to 2021-2026 on src/shm_manager.{cc,h}.
- Collapse wrapped stub_timeout_seconds_ assignment per clang-format.
@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes two root causes of orphaned /dev/shm/triton_python_backend_shm_region_* files after forced/timeout-triggered server shutdown: an effectively unbounded wait in TerminateStub (the stub timeout was parsed but never applied) and shm regions that were only removed via the SharedMemoryManager destructor (skipped on abnormal exits).

  • Bounded teardown (stub_launcher.cc): TerminateStub now shares a single stub_timeout_seconds_ budget between the Pop (finalize response) and WaitForStubProcessWithTimeout (process exit polling), falling back to KillStubProcess on timeout. All three issues flagged in the first review round (double timeout, final-sleep off-by-one, int64_tint narrowing) are addressed.
  • Explicit shm removal (shm_manager.cc/.h): RemoveShmRegion() is called unconditionally at the end of TerminateStub, and parent-owned regions are tracked in a global set with an atexit handler as a last-resort safety net for exits that bypass normal cleanup.

Confidence Score: 5/5

Safe to merge; the core teardown logic is correct and all three issues from the first review round are addressed.

The bounded teardown path (single shared budget, correct remaining-seconds accounting, final waitpid recheck) and the idempotent RemoveShmRegion flow are logically sound. The two remaining comments are about the atexit handler behaviour in a shared-library unload scenario and a missing flag-reset on an exceedingly rare atexit failure — neither affects normal operation or the primary fix.

src/shm_manager.cc — the atexit registration block warrants a second look if Triton ever dynamically unloads backends via dlclose mid-session.

Important Files Changed

Filename Overview
src/shm_manager.cc Adds anonymous-namespace globals for atexit-based shm cleanup: RegisterParentShmRegion, UnregisterParentShmRegion, CleanupParentShmRegions. Splits the destructor into destructor + RemoveShmRegion() (idempotent). Logic is sound; atexit handler has a theoretical safety concern in shared-library contexts.
src/shm_manager.h Adds RemoveShmRegion() public declaration. Minimal, clean change.
src/stub_launcher.cc Wires stub_timeout_seconds_ through TerminateStub, adds WaitForStubProcessWithTimeout (bounded poll-sleep loop with a final recheck to avoid killing an already-exiting stub), and calls RemoveShmRegion unconditionally. All three reviewer-raised issues from the previous round are addressed.
src/stub_launcher.h Adds WaitForStubProcessWithTimeout declaration and stub_timeout_seconds_ field. Clean, consistent with implementation.

Sequence Diagram

sequenceDiagram
    participant Server
    participant StubLauncher
    participant Stub
    participant ShmManager

    Server->>StubLauncher: TerminateStub()
    alt is_healthy
        StubLauncher->>Stub: Push(PYTHONSTUB_FinalizeRequest)
        StubLauncher->>StubLauncher: Pop(pop_timeout_ms, success)
        alt success within timeout
            StubLauncher->>StubLauncher: compute remaining_seconds
            StubLauncher->>StubLauncher: WaitForStubProcessWithTimeout(remaining_seconds)
            alt stub exited in time
                StubLauncher-->>StubLauncher: OK
            else timeout
                StubLauncher->>Stub: KillStubProcess() SIGKILL
            end
        else timeout / no response
            StubLauncher->>Stub: KillStubProcess() SIGKILL
        end
    else not healthy
        StubLauncher->>Stub: KillStubProcess() SIGKILL
    end
    StubLauncher->>ShmManager: RemoveShmRegion() idempotent
    ShmManager->>ShmManager: shm_unlink + UnregisterParentShmRegion
    Note over ShmManager: atexit(CleanupParentShmRegions) fires at exit for any regions not cleaned up by TerminateStub
Loading

Reviews (3): Last reviewed commit: "polish: log atexit registration failure;..." | Re-trigger Greptile

Comment thread src/stub_launcher.cc Outdated
Comment thread src/stub_launcher.cc
Comment thread src/stub_launcher.cc Outdated
Vinya567 added 2 commits July 23, 2026 19:00
- Enforce stub_timeout_seconds_ as a single total budget across the
  finalize-wait and process-exit-wait phases; healthy teardown previously
  could take up to 2 * stub_timeout_seconds_.
- Clamp the Pop timeout to INT_MAX to avoid narrowing when passing int64_t
  into MessageQueue::Pop(int const&).
- Re-poll waitpid once after the final sleep window in
  WaitForStubProcessWithTimeout so a stub that exits during that second is
  not force-killed unnecessarily.
- Log a warning to stderr if std::atexit registration fails so the loss of
  the last-resort shm cleanup is visible; primary cleanup path is unaffected.
- Tighten inline comments to explain only intent/constraints, not mechanics.
@Vinya567
Vinya567 requested review from mc-nv, pskiran1 and yinggeh July 23, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant