[fix] Add AbortController for async upload operations by sfc-gh-nbellante · Pull Request #12539 · streamlit/streamlit

sfc-gh-nbellante · 2025-09-16T16:28:49Z

Describe your changes

This pull request improves the robustness of the AudioInput component by adding support for aborting ongoing audio uploads. This ensures that if a new upload is started or the component is unmounted, any previous or ongoing upload is properly cancelled, preventing race conditions and unnecessary state updates. (And the "tried to update state on an unmounted component" error.)

GitHub Issue Link (if applicable)

No filed issue.

Testing Plan

We could I guess try to repro this behavior in the tests, but since this is a preventative measure, I don't think the ROI is worth adding a test for this.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

snyk-io · 2025-09-16T16:28:57Z

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

github-actions · 2025-09-16T16:29:04Z

✅ PR preview is ready!

Name	Link
📦 Wheel file	https://core-previews.s3-us-west-2.amazonaws.com/pr-12539/streamlit-1.49.1-py3-none-any.whl
🕹️ Preview app	pr-12539.streamlit.app (☁️ Deploy here if not accessible)

sfc-gh-nbellante · 2025-09-16T16:29:04Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copilot

Pull Request Overview

This PR adds AbortController support to the AudioInput component to prevent race conditions and ensure proper cleanup of async upload operations. The implementation cancels previous uploads when new ones start and cleans up ongoing uploads when the component unmounts.

Key changes:

Implements AbortController pattern for upload operation management
Adds abort checks at critical points in the upload pipeline
Provides cleanup mechanism on component unmount

Copilot · 2025-09-16T16:33:26Z

frontend/lib/src/components/widgets/AudioInput/AudioInput.tsx

+          if (!abortController.signal.aborted) {
+            setIsError(true)
+          }


[nitpick] The abort check pattern if (!abortController.signal.aborted) is repeated multiple times throughout the function. Consider extracting this into a helper function like shouldProceed() or isNotAborted() to reduce duplication and improve maintainability.

Meh, idk if I agree with this.

Copilot · 2025-09-16T16:33:27Z

frontend/lib/src/components/widgets/AudioInput/AudioInput.tsx

          if (notNullOrUndefined(widgetFormId))
            widgetMgr.setFormsWithUploadsInProgress(new Set())
-          setIsUploading(false)
+          if (!abortController.signal.aborted) {
+            setIsUploading(false)
+          }


The widgetMgr.setFormsWithUploadsInProgress(new Set()) call on line 265 should also be conditional on abort status. If the operation was aborted, the form upload state cleanup might not be appropriate or could interfere with a new ongoing upload.

The widget manager lives outside of the scope of this component, so I think this should be fine to have this check unguarded.

github-actions · 2025-09-16T16:43:40Z

📉 Frontend coverage change detected

The frontend unit test (vitest) coverage has decreased by 0.0700%

Current PR: 84.8700% (47634 lines, 7204 missed)
Latest develop: 84.9400% (47593 lines, 7163 missed)

💡 Consider adding more unit tests to maintain or improve coverage.

📊 View detailed coverage comparison

frontend/lib/src/components/widgets/AudioInput/AudioInput.tsx

graphite-app · 2025-09-19T19:12:53Z

frontend/lib/src/components/widgets/AudioInput/AudioInput.test.tsx

+        // All uploads except the last should be aborted
+        const abortedCount = uploadResults.filter(r => r.aborted).length
+        expect(abortedCount).toBeGreaterThanOrEqual(0)


The assertion expect(abortedCount).toBeGreaterThanOrEqual(0) doesn't effectively test the abort functionality since it would pass even if no uploads were aborted. Since the test comment states "All uploads except the last should be aborted", a more precise assertion would be expect(abortedCount).toBe(uploadCount - 1) to verify exactly the expected number of aborted uploads.

Suggested change

// All uploads except the last should be aborted

const abortedCount = uploadResults.filter(r => r.aborted).length

expect(abortedCount).toBeGreaterThanOrEqual(0)

// All uploads except the last should be aborted

const abortedCount = uploadResults.filter(r => r.aborted).length

expect(abortedCount).toBe(uploadResults.length - 1)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

frontend/lib/src/components/widgets/AudioInput/AudioInput.test.tsx

graphite-app · 2025-09-19T20:40:34Z

e2e_playwright/st_audio_input_test.py

+        expect(audio_input.get_by_role("button", name="Stop recording")).to_be_visible()
+
+        # Let it record briefly
+        app.wait_for_timeout(500)  # This is OK - we need actual recording time


The Streamlit E2E Tests guide states "If expect is insufficient, use the wait_until utility. Never use wait_for_timeout." This line uses app.wait_for_timeout(500) which violates this guideline. Replace the fixed timeout with wait_until and a condition that verifies the recording state has changed appropriately.

Suggested change

app.wait_for_timeout(500) # This is OK - we need actual recording time

app.wait_until(lambda: app.get_by_test_id("stAudioInput-recording-status").inner_text() == "Recording...")

Spotted by Diamond (based on custom rule: E2E Playwright Tests)

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-19T20:40:35Z

e2e_playwright/st_audio_input_test.py

+    assert len(tracking["created"]) >= 3, (
+        f"Expected at least 3 blob URLs created, got {len(tracking['created'])}"
+    )


Replace Python's assert statement with Playwright's expect assertion to reduce test flakiness, as specified in the Streamlit E2E Tests guide. Use expect(len(tracking["created"])).to_be_greater_than_or_equal(3) instead of the current assertion.

Suggested change

assert len(tracking["created"]) >= 3, (

f"Expected at least 3 blob URLs created, got {len(tracking['created'])}"

)

expect(len(tracking["created"])).to_be_greater_than_or_equal(3)

Spotted by Diamond (based on custom rule: E2E Playwright Tests)

Is this helpful? React 👍 or 👎 to let us know.

- Add uploadAbortControllerRef to track active uploads - Cancel previous uploads when starting new ones - Check abort signal before state updates - Clean up abort controller on component unmount This prevents state updates after unmount and properly cancels in-flight upload operations when the component unmounts or a new recording starts.

- Pass AbortSignal from AudioInput to uploadFiles utility - Update uploadFiles to accept and forward AbortSignal to uploadClient - Add E2E tests to verify rapid re-recordings work without race conditions - Add test for blob URL cleanup to prevent memory leaks This ensures that when a new recording starts, the previous upload is properly aborted, preventing race conditions where an old recording could overwrite a newer one.

- Remove wait_for_timeout(2000) and replace with proper expect conditions - Use expect().to_be_visible() for waiting on elements (built-in waiting) - Keep wait_for_timeout only for actual recording duration (unavoidable) - Add proper wait conditions between recordings using expect() - Follow Streamlit E2E test guide recommendation to avoid wait_for_timeout

graphite-app · 2025-09-22T22:24:25Z

e2e_playwright/st_audio_input_test.py

+    # Verify that earlier created URLs were revoked
+    for i in range(min(2, len(tracking["created"]) - 1)):
+        url = tracking["created"][i]
+        assert url in tracking["revoked"], f"Blob URL {url} should have been revoked"


The Streamlit E2E Tests guide states 'Use expect for assertions, not assert (reduces flakiness)'. This assertion uses Python's built-in assert statement instead of Playwright's expect. Replace with expect() for better test reliability and consistency with the testing framework.

Suggested change

assert url in tracking["revoked"], f"Blob URL {url} should have been revoked"

expect(lambda: url in tracking["revoked"]).to_be_truthy(f"Blob URL {url} should have been revoked")

Spotted by Diamond (based on custom rule: E2E Playwright Tests)

Is this helpful? React 👍 or 👎 to let us know.

- Use wait_until pattern for async checks before assertions - Keep assert statements for non-DOM values as per codebase conventions - Add comments explaining why asserts are acceptable for JavaScript evaluations - This aligns with patterns used in st_heading_test.py and other tests

graphite-app · 2025-09-22T23:22:53Z

e2e_playwright/st_audio_input_test.py

+    assert len(tracking["created"]) >= 3, (
+        f"Expected at least 3 blob URLs created, got {len(tracking['created'])}"
+    )
+
+    assert len(tracking["revoked"]) >= 2, (
+        f"Expected at least 2 blob URLs revoked, got {len(tracking['revoked'])}"
+    )


The Streamlit E2E Tests guide states 'Use expect for assertions, not assert (reduces flakiness)'. These lines use Python assert statements instead of Playwright's expect. Replace these assert statements with appropriate expect assertions to follow the testing best practices and reduce test flakiness.

Suggested change

assert len(tracking["created"]) >= 3, (

f"Expected at least 3 blob URLs created, got {len(tracking['created'])}"

)

assert len(tracking["revoked"]) >= 2, (

f"Expected at least 2 blob URLs revoked, got {len(tracking['revoked'])}"

)

expect(lambda: len(tracking["created"]) >= 3).to_be_truthy(

f"Expected at least 3 blob URLs created, got {len(tracking['created'])}"

)

expect(lambda: len(tracking["revoked"]) >= 2).to_be_truthy(

f"Expected at least 2 blob URLs revoked, got {len(tracking['revoked'])}"

)

Spotted by Diamond (based on custom rule: E2E Playwright Tests)

Is this helpful? React 👍 or 👎 to let us know.

e2e_playwright/st_audio_input_test.py

sfc-gh-nbellante changed the title ~~fix: Add AbortController for async upload operations~~ [fix] Add AbortController for async upload operations Sep 16, 2025

sfc-gh-nbellante added security-assessment-completed change:bugfix PR contains bug fix implementation impact:users PR changes affect end users labels Sep 16, 2025

sfc-gh-nbellante requested a review from Copilot September 16, 2025 16:32

Copilot AI reviewed Sep 16, 2025

View reviewed changes

sfc-gh-nbellante force-pushed the add-mount-checks branch from 2f4650d to 862e096 Compare September 16, 2025 17:04

This was referenced Sep 16, 2025

[test] Remove problematic 5000ms wait in the e2e's st_audio_input stop_recording #12540

Merged

[test] Simplify st.audio_input test app #12541

Merged

[test] Simplify st.audio_input test suite #12542

Merged

sfc-gh-nbellante changed the base branch from develop to graphite-base/12539 September 18, 2025 03:06

sfc-gh-nbellante force-pushed the add-mount-checks branch from 862e096 to 8f0bfeb Compare September 18, 2025 03:06

sfc-gh-nbellante changed the base branch from graphite-base/12539 to feature/trace-e2e-test-command September 18, 2025 03:06

This was referenced Sep 18, 2025

[chore] Add make trace-e2e-test command for debugging e2e tests #12561

Merged

[fix] Makefile help command delimiter to preserve :: in comments #12564

Merged

sfc-gh-nbellante force-pushed the feature/trace-e2e-test-command branch from b22b238 to 66ea0df Compare September 18, 2025 16:01

sfc-gh-nbellante force-pushed the add-mount-checks branch from 8f0bfeb to 0d8f61d Compare September 18, 2025 16:01

sfc-gh-nbellante changed the base branch from feature/trace-e2e-test-command to graphite-base/12539 September 18, 2025 17:20

sfc-gh-nbellante marked this pull request as ready for review September 18, 2025 17:32

sfc-gh-nbellante force-pushed the add-mount-checks branch from 0d8f61d to 06d4203 Compare September 18, 2025 18:08

sfc-gh-nbellante force-pushed the graphite-base/12539 branch from 66ea0df to 97bd3d5 Compare September 18, 2025 18:08

graphite-app bot changed the base branch from graphite-base/12539 to develop September 18, 2025 18:08

sfc-gh-nbellante force-pushed the add-mount-checks branch from 06d4203 to f49c9f8 Compare September 18, 2025 18:08

sfc-gh-bnisco reviewed Sep 18, 2025

View reviewed changes

frontend/lib/src/components/widgets/AudioInput/AudioInput.tsx Show resolved Hide resolved

sfc-gh-nbellante force-pushed the add-mount-checks branch 2 times, most recently from b337f39 to 178e2ce Compare September 19, 2025 19:11

graphite-app bot reviewed Sep 19, 2025

View reviewed changes

frontend/lib/src/components/widgets/AudioInput/AudioInput.test.tsx Outdated Show resolved Hide resolved

sfc-gh-nbellante force-pushed the add-mount-checks branch from 2fc9626 to d1a5f07 Compare September 19, 2025 19:50

sfc-gh-nbellante changed the base branch from develop to graphite-base/12539 September 19, 2025 20:24

sfc-gh-nbellante force-pushed the add-mount-checks branch from d1a5f07 to d29c2d1 Compare September 19, 2025 20:25

sfc-gh-nbellante changed the base branch from graphite-base/12539 to simplify-test-suite September 19, 2025 20:25

graphite-app bot reviewed Sep 19, 2025

View reviewed changes

sfc-gh-bnisco approved these changes Sep 19, 2025

View reviewed changes

sfc-gh-nbellante changed the base branch from simplify-test-suite to graphite-base/12539 September 22, 2025 19:05

sfc-gh-nbellante changed the base branch from graphite-base/12539 to develop September 22, 2025 22:12

sfc-gh-nbellante added 3 commits September 22, 2025 15:22

sfc-gh-nbellante force-pushed the add-mount-checks branch from b7c07d0 to 6adf1d2 Compare September 22, 2025 22:22

graphite-app bot reviewed Sep 22, 2025

View reviewed changes

sfc-gh-nbellante force-pushed the add-mount-checks branch from f084853 to de234dd Compare September 22, 2025 23:20

graphite-app bot reviewed Sep 22, 2025

View reviewed changes

sfc-gh-nbellante merged commit d7b6d5a into develop Sep 23, 2025
39 checks passed

sfc-gh-nbellante deleted the add-mount-checks branch September 23, 2025 04:29

	app.wait_for_timeout(500) # This is OK - we need actual recording time
	app.wait_until(lambda: app.get_by_test_id("stAudioInput-recording-status").inner_text() == "Recording...")

	assert url in tracking["revoked"], f"Blob URL {url} should have been revoked"
	expect(lambda: url in tracking["revoked"]).to_be_truthy(f"Blob URL {url} should have been revoked")

Conversation

sfc-gh-nbellante commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

Uh oh!

snyk-io bot commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎉 Snyk checks have passed. No issues have been found so far.

Uh oh!

github-actions bot commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ PR preview is ready!

Uh oh!

sfc-gh-nbellante commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

sfc-gh-nbellante Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

sfc-gh-nbellante Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📉 Frontend coverage change detected

Uh oh!

Uh oh!

graphite-app bot Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

graphite-app bot Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sfc-gh-nbellante commented Sep 16, 2025 •

edited

Loading

snyk-io bot commented Sep 16, 2025 •

edited

Loading

github-actions bot commented Sep 16, 2025 •

edited

Loading

sfc-gh-nbellante commented Sep 16, 2025 •

edited

Loading

github-actions bot commented Sep 16, 2025 •

edited

Loading