Skip to content

fix: Fix bad requests when retrying with streams in multi-part requests#212

Merged
rbell517 merged 8 commits into
masterfrom
users/rbell/fix-notebook-integration-test
May 29, 2026
Merged

fix: Fix bad requests when retrying with streams in multi-part requests#212
rbell517 merged 8 commits into
masterfrom
users/rbell/fix-notebook-integration-test

Conversation

@rbell517
Copy link
Copy Markdown
Collaborator

@rbell517 rbell517 commented May 28, 2026

What does this Pull Request accomplish?

This change introduces a request handler that seeks any seekable stream Part for multi-part requests to the start of the stream when the stream is being retried, via a decorator.

This change also adds 429 retries to the notebook artifacts client to align to other clients, and implementing the same stream seek to start on retry fix.

Why should this Pull Request be merged?

The current behavior for multi-part stream requests that are retried does not seek the stream back to the beginning of the stream. This results in retries continuing from however far into the stream the request got to before an error, often resulting in malformed requests.

This was evident in intermittent notebook client integration tests that hit and retried 429 errors. These tests would often fail with a cryptic malformed Part error.

What testing has been done?

New tests have been added that explicitly covers the retry cases on 429 errors and verifies the content arrives correctly.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Comment thread tests/integration/file/test_file_client.py Outdated
Comment thread tests/integration/notebook/test_notebook_client.py Outdated
Comment thread nisystemlink/clients/core/_uplink/_multipart_retry.py Outdated
Comment thread nisystemlink/clients/core/_uplink/_multipart_retry.py
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment thread nisystemlink/clients/core/_uplink/_multipart_retry.py
Comment thread nisystemlink/clients/core/_uplink/_multipart_retry.py
@rbell517 rbell517 changed the title Fix bad requests when retrying with seekable streams fix: Fix bad requests when retrying with seekable streams May 29, 2026
@rbell517 rbell517 changed the title fix: Fix bad requests when retrying with seekable streams fix: Fix bad requests when retrying with streams in multi-part requests May 29, 2026
@rbell517 rbell517 enabled auto-merge (squash) May 29, 2026 19:35
@rbell517 rbell517 merged commit 4e34914 into master May 29, 2026
12 checks passed
@rbell517 rbell517 deleted the users/rbell/fix-notebook-integration-test branch May 29, 2026 19:35
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.

3 participants