Skip to content

test: Safely handle filesystem exception#8736

Merged
yinggeh merged 6 commits into
mainfrom
yinggeh/tri-873-psirt-triton-denial-of-service-via-uncaught
Apr 20, 2026
Merged

test: Safely handle filesystem exception#8736
yinggeh merged 6 commits into
mainfrom
yinggeh/tri-873-psirt-triton-denial-of-service-via-uncaught

Conversation

@yinggeh
Copy link
Copy Markdown
Contributor

@yinggeh yinggeh commented Apr 14, 2026

What does the PR do?

Catch exceptions raised by std::filesystem::canonical and std::filesystem::weakly_canonical.

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.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • test

Related PRs:

triton-inference-server/client#887
triton-inference-server/core#491

Where should the reviewer start?

Test plan:

  • CI Pipeline ID:
    48465353

Caveats:

Background

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

  • closes GitHub issue: #xxx

@yinggeh yinggeh self-assigned this Apr 14, 2026
@yinggeh yinggeh force-pushed the yinggeh/tri-873-psirt-triton-denial-of-service-via-uncaught branch from 48a2328 to 9b03645 Compare April 14, 2026 09:02
whoisj
whoisj previously approved these changes Apr 14, 2026
Copy link
Copy Markdown
Contributor

@whoisj whoisj left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread qa/L0_http/http_test.py
)
# TODO: [TRI-958] Status code 400 is more appropriate here
self.assertEqual(
500,
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.

but HTTP(400) is Bad Request while HTTP(500) is Internal Error.

I would think returning a 400 is more accurate.

Copy link
Copy Markdown
Contributor Author

@yinggeh yinggeh Apr 17, 2026

Choose a reason for hiding this comment

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

The status code is dropped here and any error is responded as 500 server error.
https://github.com/triton-inference-server/core/blob/45d5a3d92119b5f21a25aa290e9ee3d9b2641ba8/src/model_repository_manager/model_repository_manager.cc#L1347-L1351. That's why I added a TODO and created a ticket for fixing it.

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.

huh. 🤔 this is one of those clean up things we'll defer to a future PR then, thanks..

@yinggeh yinggeh requested a review from whoisj April 18, 2026 00:08
@yinggeh yinggeh added the PR: test Adding missing tests or correcting existing test label Apr 18, 2026
Comment thread qa/L0_http/http_test.py
long_path = "file:" + ("A" * 256)
payload = {
"parameters": {
long_path: "YQ==",
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.

long_path: "YQ==",
What does this code do?

Copy link
Copy Markdown
Contributor Author

@yinggeh yinggeh Apr 18, 2026

Choose a reason for hiding this comment

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

Some random placeholder generated by AI. In practice, it's the binary data of custom model file.

@yinggeh yinggeh merged commit 9144042 into main Apr 20, 2026
2 checks passed
@yinggeh yinggeh deleted the yinggeh/tri-873-psirt-triton-denial-of-service-via-uncaught branch April 20, 2026 18:27
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