Skip to content

test: fix L0_lifecycle test_shutdown_dynamic exit timeout - #8889

Merged
Vinya567 merged 1 commit into
mainfrom
vinyak/nvbug-6450088-l0-lifecycle-shutdown-timeout
Jul 23, 2026
Merged

test: fix L0_lifecycle test_shutdown_dynamic exit timeout#8889
Vinya567 merged 1 commit into
mainfrom
vinyak/nvbug-6450088-l0-lifecycle-shutdown-timeout

Conversation

@Vinya567

@Vinya567 Vinya567 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What does the PR do?

Fix L0_lifecycle failures on Luna where LifeCycleTest.test_shutdown_dynamic passes pytest but the QA script aborts during server cleanup.

Root cause

  • test_shutdown_dynamic starts the server without --exit-timeout-secs, so Triton uses the default 30s shutdown window.
  • The test sends 6 async inferences with execute_delay_ms=5000, then SIGINT while they are in flight; draining can exceed 30s on Luna (~80% failure rate).
  • The server logs Exit timeout expired and exits non-zero; wait $SERVER_PID under set -e aborts the rest of L0_lifecycle (silent failure in the main log).

Fix

  • qa/L0_lifecycle/test.sh:
    • Set --exit-timeout-secs=${SERVER_TIMEOUT} (120s) for the test_shutdown_dynamic server launch so draining has room to complete.
    • Use wait $SERVER_PID || true after cleanup so a non-zero server exit does not abort the suite.

Previous revisions of this branch also touched qa/L0_lifecycle/lifecycle_test.py (gRPC error-string assertions and pre-existing flake8 lints). Those changes are redundant with #8888, which landed on main after this PR was opened, so this PR now carries only the test.sh fix.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.

Commit Type:

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

Where should the reviewer start?

  • qa/L0_lifecycle/test.shtest_shutdown_dynamic server args (--exit-timeout-secs) and cleanup wait.

Test plan:

  • GitLab Luna: L0_lifecycle--base green at commit 721e9c07 (job 368596258, pipeline 58809167); log shows test_shutdown_dynamic ran and *** Test Passed ***.
  • Earlier Luna runs on same pipeline: d41ea55 failed on (111) assertion mismatch (job 368365178); 8ba9e9fb and 38c233da also green.
  • CI used TRITON_SERVER_BRANCH_NAME=vinyak/nvbug-6450088-l0-lifecycle-shutdown-timeout and TRITON_THIRD_PARTY_REPO_TAG=4d8c8cec to work around a transient master py3 build break (gRPC patch); that pin is CI-only, not part of this PR.
  • CI Pipeline ID: 58809167

Caveats:

Background

  • Linear TRI-1580 / NVBUG 6450088 / DLIS-8700
  • Same shutdown-timeout family as Jetson lifecycle work (TRI-1330), but a different test (L0_lifecycle) and platform (Luna x86).

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • Relates to NVBUG 6450088

@Vinya567
Vinya567 requested review from mc-nv and yinggeh July 20, 2026 20:41
@Vinya567 Vinya567 added the PR: test Adding missing tests or correcting existing test label Jul 20, 2026
Comment thread qa/L0_lifecycle/lifecycle_test.py Outdated
Comment thread qa/L0_lifecycle/lifecycle_test.py Outdated
suffix = msg[len(self._GRPC_CONNECTION_REFUSED_PREFIX) :].strip()
self.assertIn(suffix, self._GRPC_CONNECTION_REFUSED_SUFFIXES, msg)

def _assert_shutdown_rejects_new_request(self, ex):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's only called once, I'm not sure this function is necessary.

@Vinya567 Vinya567 Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed , it was only used in one place, so the helper wasn't worth it. Removed in 721e9c07; test_shutdown_sequence now inlines the check (allow CANCELLED, otherwise expect the connection-refused error string), same as the other call sites.

Comment thread qa/L0_lifecycle/lifecycle_test.py
@Vinya567
Vinya567 requested a review from yinggeh July 21, 2026 18:55
Comment thread qa/L0_lifecycle/lifecycle_test.py Outdated
@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes flaky L0_lifecycle failures on Luna by addressing two issues in test.sh around the test_shutdown_dynamic test. The root cause was that 6 in-flight inferences each sleeping 5 s could exceed the default 30 s Triton shutdown window, causing the server to exit non-zero and abort the entire suite under set -e.

  • Adds --exit-timeout-secs=${SERVER_TIMEOUT} (120 s, sourced from util.sh) to the test_shutdown_dynamic server args so the shutdown window is wide enough to drain all in-flight requests.
  • Changes wait $SERVER_PID to wait $SERVER_PID || true in the cleanup path so a non-zero server exit (expected when the exit-timeout path fires) no longer aborts the rest of the test suite.

Confidence Score: 5/5

Safe to merge — two minimal, well-scoped shell fixes that directly address the documented root cause without touching production code.

Both changes are in test infrastructure only. The 120 s exit timeout sourced from util.sh is consistent with the value already used for model-stability waits throughout the same script. The || true on wait is narrowly placed in the cleanup path after the test has already validated its assertions, so no real failure can be silently swallowed.

No files require special attention.

Important Files Changed

Filename Overview
qa/L0_lifecycle/test.sh Adds --exit-timeout-secs=${SERVER_TIMEOUT} (120 s) to test_shutdown_dynamic server args and makes `wait $SERVER_PID

Sequence Diagram

sequenceDiagram
    participant SH as test.sh
    participant TS as Triton Server
    participant PT as pytest (test_shutdown_dynamic)

    SH->>TS: "start with --exit-timeout-secs=120s"
    SH->>PT: run test (set +e)
    PT->>TS: "send 6 async inferences (execute_delay_ms=5000)"
    PT->>TS: SIGINT
    note over TS: drains in-flight requests (up to 120 s window)
    TS-->>SH: exit (may be non-zero if timeout fires)
    PT-->>SH: pytest result
    SH->>SH: check_unit_test (set -e)
    SH->>SH: grep server log for gRPC connection count
    SH->>TS: "kill $SERVER_PID || true (safety net)"
    SH->>SH: "wait $SERVER_PID || true (non-zero exit tolerated)"
Loading

Reviews (5): Last reviewed commit: "test: fix L0_lifecycle test_shutdown_dyn..." | Re-trigger Greptile

Comment thread qa/L0_lifecycle/lifecycle_test.py Outdated
@Vinya567
Vinya567 requested a review from yinggeh July 23, 2026 00:53
Comment thread qa/L0_lifecycle/lifecycle_test.py Outdated
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

# flake8: noqa

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-commit #8897 fixed flake8 arg parsing, and it now enforces rules that were silently disabled before. That surfaced ~30 pre-existing lints in this file (unused vars, imports after sys.path.append, etc.) ,none from this fix. Added # flake8: noqa to unblock CI without turning this PR into a lint cleanup. Happy to drop it and clean them up in a separate PR if you'd rather.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clean it up in this PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you install pre-commit locally, does it automatically reformat for you?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No isort and black auto-fix formatting, but flake8 is a reporter. E402/F841/E721/E712/E711/F401 all need manual edits.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clean it up in this PR?

Cleaned up in f36a9bd — removed the # flake8: noqa and fixed the pre-existing lints (E402/F841/E266/E721/E712/E711/F401).

Comment thread qa/L0_lifecycle/lifecycle_test.py Outdated
self.assertTrue(
msg.startswith(self._GRPC_CONNECTION_REFUSED_PREFIX), msg
)
self.assertStartsWith(self._GRPC_CONNECTION_REFUSED_PREFIX, msg)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized self.assertStartsWith is available in 3.14. Our test environment might not be supporting it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'll revert back to self.assertTrue(msg.startswith(...)) - okay?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or assertIn, up to you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with self.assertTrue(msg.startswith(...)) in f36a9bd.

@Vinya567
Vinya567 requested review from pskiran1 and yinggeh July 23, 2026 02:23
pskiran1
pskiran1 previously approved these changes Jul 23, 2026

@pskiran1 pskiran1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Extend the exit-timeout window for the test_shutdown_dynamic server
launch to SERVER_TIMEOUT (120s) and tolerate a non-zero wait exit so
"set -e" does not abort the rest of L0_lifecycle when the exit timeout
fires while draining in-flight dynamic-batch work.

Root cause: the test issues 6 async inferences with execute_delay_ms=5000
and then SIGINTs the server while they are in flight. The default 30s
Triton exit timeout is not enough for the drain on Luna, so the server
logs "Exit timeout expired" and exits non-zero, and "wait $SERVER_PID"
under "set -e" aborts the whole suite.

The related lifecycle_test.py assertion-string and lint changes from
earlier revisions of this branch are now redundant with #8888 on main,
so this PR carries only the shutdown-timeout fix in test.sh.
@Vinya567
Vinya567 force-pushed the vinyak/nvbug-6450088-l0-lifecycle-shutdown-timeout branch from f36a9bd to 0256d33 Compare July 23, 2026 17:55
@Vinya567
Vinya567 requested a review from pskiran1 July 23, 2026 17:58
@yinggeh yinggeh changed the title test: fix L0_lifecycle test_shutdown_dynamic exit timeout (NVBUG 6450088) test: fix L0_lifecycle test_shutdown_dynamic exit timeout Jul 23, 2026
@yinggeh

yinggeh commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

I removed your NVBUGS ID from title.

@Vinya567
Vinya567 merged commit 12a3052 into main Jul 23, 2026
4 checks passed
@Vinya567
Vinya567 deleted the vinyak/nvbug-6450088-l0-lifecycle-shutdown-timeout branch July 23, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: test Adding missing tests or correcting existing test

Development

Successfully merging this pull request may close these issues.

3 participants