🌐 US-Proxy
class="logged-out env-production page-responsive" style="word-wrap: break-word;" >
Skip to content

[fix] Revoke disabled download-button callables - #17288

Merged
lukasmasuch merged 6 commits into
developfrom
lukasmasuch/disabled-deferred-export-fix
Oct 7, 2026
Merged

lukasmasuch merged 6 commits into
developfrom
lukasmasuch/disabled-deferred-export-fix

Conversation

@lukasmasuch

@lukasmasuch lukasmasuch commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Describe your changes

A disabled callable st.download_button no longer exposes an executable deferred file id. A client can run a registered data callable by sending the deferred-file request directly, even when the button is disabled in the UI. The server now skips registration for a disabled callable and drops any id an earlier enabled run registered for that button. That includes a later run that passes bytes or text, and a fragment rerun that moves the button while its element id stays the same. A key keeps that id stable if the label changes. Repeat downloads of an enabled button still invoke the callable.

A disabled button that is given bytes or text directly still publishes a /media/ URL. Those bytes are produced when the script runs, not when the button is clicked.

DownloadButton skips the source-URL check when url is empty. Checking "" re-fetches the current page.

GitHub Issue Link (if applicable)

Testing Plan

  • Unit Tests (JS and/or Python)
    • lib/tests/streamlit/elements/download_button_test.py — a disabled callable button does not register a file id; a later disabled render revokes the previous id, including a switch to static data and a public st.download_button call at a new delta path; re-enabling supports repeat downloads
    • lib/tests/streamlit/runtime/media_file_manager_test.py — removal is session-scoped, still applies after clear_session_refs, and leaves static files in place
    • DownloadButton.test.tsx — an empty URL does not trigger a source check

Contribution License Agreement

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

Disabled callable download buttons registered the generator and sent
its file id to the browser. Register the callable only while the
button is enabled, and drop an earlier entry when a later run renders
the button disabled.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 10:47
@lukasmasuch lukasmasuch added change:bugfix PR contains bug fix implementation impact:users PR changes affect end users labels Oct 6, 2026

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@snyk-io

snyk-io Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

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

Status Scan Engine Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@lukasmasuch lukasmasuch added the ai-review If applied to PR or issue will run AI review workflow label Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-17288/streamlit-1.65.0-py3-none-any.whl
📦 @streamlit/component-v2-lib Download from artifacts

@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High risk] Changes how disabled download buttons handle callable data.

The PR appears safe to merge based on the reviewed changes.

Summary

The PR prevents disabled callable download buttons from publishing or retaining executable deferred file IDs, while preserving static downloads and repeat downloads from enabled buttons. The changes since the previous review add a public-API test for revocation after a path change and clarify comments.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Render download button] --> B{Disabled?}
  B -- Yes --> C[Revoke matching deferred IDs]
  C --> D{Callable data?}
  D -- Yes --> E[Publish no URL or deferred ID]
  D -- No --> F[Publish static media URL]
  B -- No --> G{Callable data?}
  G -- Yes --> H[Register deferred ID]
  G -- No --> F
Loading

Reviews (6) · Last reviewed commit: "Document deferred revocation and test th..."

Comment thread lib/streamlit/runtime/media_file_manager.py Outdated
Comment thread lib/streamlit/elements/widgets/button.py Outdated
@github-actions github-actions Bot removed the ai-review If applied to PR or issue will run AI review workflow label Oct 6, 2026

@github-actions github-actions Bot left a comment

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.

Summary

All three expected reviews completed (gpt-5.6-terra-xhigh, claude-opus-5-5-medium, grok-4.7-xhigh). None failed to complete review. All three approve.

PR #17288 stops a disabled callable st.download_button from publishing a deferred file id. When a later run renders that button disabled, the server drops the callable left from the earlier enabled run, including after clear_session_refs has cleared the session coordinate map. Removal is limited to the active session. Enabled buttons still register a new id on each run, and repeat downloads of that id still run the callable.

Reviewers agreed this is a small, well-tested server-side bug fix with no API, compatibility, or performance concerns. The disagreements were optional: comment wording, a proposed scan shortcut, and how to phrase the PR title. Those are resolved below and do not block merge.

Product Alignment

This is a user-facing bug fix. disabled=True on a callable download button now holds on the server as well as in the browser. The public surface stays st.download_button(..., data=callable, disabled=True). There is no new parameter, default, command, or rendered control.

specs/AGENTS.md exempts bug fixes from the spec process. Merged specs that mention st.download_button (the on_change="ignore" and horizontal-wrap specs) do not govern deferred-callable revocation. The PR body describes this behavior and does not claim a separate product approval; none is required.

The change matches the existing disabled contract used by other widgets (same name, same behavior): a disabled control does not perform its action. Enabled deferred downloads, static downloads, and the on_click path stay as they are. Static data on a disabled button still publishes a media URL. All three reviewers treated that as existing behavior and outside this fix.

Code Quality

The implementation matches the existing media-file lifecycle and is proportionate to the bug.

  • marshall_file takes a keyword-only disabled flag. For a disabled callable it skips registration, sets url to "", and calls MediaFileManager.remove_deferred.
  • DeferredCallableEntry stores session_id. A full rerun calls clear_session_refs before the script runs, which drops the coordinate map and leaves deferred callables in place until end-of-run orphan cleanup so an in-flight download can finish. remove_deferred finds those unmapped entries by session and coordinates.
  • The coordinate map is updated only when the mapped id is one this call removed, so a static file at the same coordinates stays referenced. Lock handling matches add and add_deferred: the session id is read before the lock, and the dict is not mutated while it is iterated.

Reviewers agreed the extra field and the scan are the minimum needed once the coordinate index is gone.

One disagreement: claude-opus-5-5-medium suggested looking up session_files first and scanning _deferred_callables only when a stale entry might exist. That shortcut does not cover this bug. After clear_session_refs the session map is gone, and a second add_deferred at the same coordinates leaves the previous id executable but unmapped. Both cases are what the new tests pin down. The scan by session_id and coordinates is the right lookup. gpt-5.6-terra-xhigh and grok-4.7-xhigh called the scan proportionate; that assessment holds. The table is one entry per deferred download button per session, and the path runs only for a disabled callable button.

A second disagreement is the comment above download_button_proto.disabled = disabled (button.py line 1437). gpt-5.6-terra-xhigh read it as a real ordering constraint. marshall_file uses the disabled argument and does not read proto.disabled. claude-opus-5-5-medium and grok-4.7-xhigh are right that the "set disabled before registering" wording describes a coupling that is not there. It is a non-blocking nit, tracked inline.

Performance

No meaningful hot-path impact. remove_deferred runs only when a download button is both disabled and given a callable, and it scans the in-memory deferred-callable table under the existing media-manager lock. That table is off the element-enqueue, ForwardMsg, WebSocket flush, session-state, cache, and dataframe paths. Callable execution still happens outside the lock. There is no frontend change, so there are no new renders or remounts. A benchmark is not warranted.

Test Coverage

Unit coverage matches the bug, and all three reviewers judged it sufficient:

  • A disabled callable button enqueues no deferred_file_id, leaves url empty, and does not call the callable (download_button_test.py).
  • A later disabled render revokes the previous id after clear_session_refs, and execute_deferred then fails. Re-enabling registers a new id, and repeat downloads of that id still run the callable.
  • Removal is session-scoped, deletes every prior id at the same coordinates, leaves a static file mapped, and is a no-op for an unknown coordinate (media_file_manager_test.py).

E2E coverage is not required. The disabled control's rendered state is unchanged, and the browser already refuses the click. Existing deferred-download e2e coverage still exercises enabled callables.

Two optional gaps, both non-blocking: the enabled-then-disabled transition is driven through marshall_file rather than st.download_button, and test_remove_deferred_after_clear_session_refs checks that both ids leave _deferred_callables without calling execute_deferred. Neighboring tests already assert that a missing id cannot be executed.

Backwards Compatibility

The public signature, defaults, and return value are unchanged. marshall_file's new argument is keyword-only with a default, and its only caller is _download_button. DeferredCallableEntry is created only inside add_deferred, so the new session_id field is always present. Enabled callable buttons still publish a new id each run and still support repeat downloads. Static data on a disabled button still registers a file URL.

A client that replayed a deferred_file_id from a button that is now disabled will no longer run the callable. That id is an internal transport capability. Executing it would contradict disabled=True.

Security & Risk

The fix puts the authorization check on the server. A disabled callable button no longer sends an id, and a later disabled render at the same coordinates revokes the previous run's id for the active session. One session cannot revoke another session's callable. Routes, authentication, the WebSocket handshake, CORS, asset serving, dependencies, and client-side execution are unchanged. Backend-operation requests are still rejected when the request session id does not match the WebSocket session (app_session.py), and file ids remain uuid4 values. execute_deferred still runs whatever callable is registered for that id, so withholding and deleting the id is the right boundary.

Two narrow windows follow the existing orphan-cleanup design. No reviewer treated them as regressions or merge-blocking:

  • From clear_session_refs at the start of a full rerun until the disabled button renders, or until end-of-run remove_orphaned_files, the previous callable stays registered so an in-flight download can finish.
  • On a fragment rerun, media refs stay in place (script_runner.py skips clear_session_refs when a fragment id queue is set). Revocation uses the coordinates of the button rendered in that fragment. A stale mapping at a previous delta path remains until a full rerun. MediaFileManager already treats moving coordinates as outside the coordinate map's model. Switching data from a callable to static bytes leaves the old callable until end-of-run orphan cleanup, because remove_deferred runs only in the callable branch. Orphan cleanup still runs when a fragment run finishes.

External test recommendation

  • Recommend external_test: No
  • Triggered categories: None
  • Evidence:
    • lib/streamlit/elements/widgets/button.py: gates deferred-callable registration on disabled and calls remove_deferred. No route, cookie, handshake, iframe, CORS, or header changes.
    • lib/streamlit/runtime/media_file_manager.py: drops in-memory deferred callables for the active session and coordinates. Download endpoints, asset handlers, cache headers, and base paths are unchanged.
    • Tests are unit tests only (download_button_test.py, media_file_manager_test.py).
  • Suggested external_test focus areas: None. Registration and revocation stay inside the existing media manager and do not depend on proxy, iframe, CSP, or hosted-runtime boundaries.
  • Confidence: High
  • Assumptions and gaps: Deferred execution still uses the existing backend-operation message, which this PR does not modify. All three reviewers reached this recommendation.

Accessibility

No frontend change. The button is still exposed as a disabled control through the existing disabled proto field. Keyboard, name, and focus behavior are unchanged.

Readability

The line-level comment nit on button.py is in the inline comments. Remaining notes are optional polish. gpt-5.6-terra-xhigh found the new docstrings clear; grok-4.7-xhigh proposed tighter wording; claude-opus-5-5-medium flagged one pre-existing comment outside the diff.

lib/streamlit/runtime/media_file_manager.py

  • execute_deferred, lines 433–434, outside this diff. The comment says the callable is cleaned up when clear_session_refs() runs. clear_session_refs drops the coordinate map and leaves deferred callables in place. Cleanup is remove_orphaned_files(), and this PR adds remove_deferred() for the disabled path. Proposed rewrite:
# Keep the callable so users can download multiple times. It is removed by
# remove_orphaned_files() once no session references it, or by
# remove_deferred() when the button renders disabled.
  • remove_deferred docstring, lines 345–348. The text is accurate. The second sentence is dense. Optional rewrite:
"""Drop this session's deferred callables registered at ``coordinates``.

Also deletes entries that a preceding ``clear_session_refs`` already
unmapped, so a disabled button revokes the previous run immediately.
Does nothing when no entry matches.

Safe to call from any thread.
"""

lib/tests/streamlit/elements/download_button_test.py

  • test_disabling_callable_drops_previously_registered_entry, lines 332–337. "Unmaps session files", "orphan cleanup", and "generator" are local jargon. Optional rewrite:
"""Rendering the button disabled revokes the file id from the previous run.

A full rerun drops the session's coordinate map before widgets render, but
the callable stays registered until end-of-run cleanup. The disabled render
must delete it immediately so a download request cannot run the callable.
"""

lib/tests/streamlit/runtime/media_file_manager_test.py

  • test_remove_deferred_after_clear_session_refs, line 913. The docstring states a result the test does not show with execute_deferred. Optional rewrite: """remove_deferred deletes every prior id at these coordinates after clear_session_refs."""

PR title and description

Reviewers agreed the title [fix] Reject disabled deferred downloads is vague: "reject" can be read as an error response, and "deferred" does not name the API. Proposed title: [fix] Revoke disabled callable downloads.

The description explains the behavior. Two additions would make the scope obvious: the browser's disabled check is not what authorizes execute_deferred, and the fix covers callable data (static data on a disabled button is unchanged). The Testing Plan can record the E2E decision: unit tests cover the server-side id lifecycle, and an E2E test would not observe a click the browser already refuses.

Recommendations

  1. Optional: remove the ordering comment above download_button_proto.disabled = disabled in button.py (inline comment).
  2. Optional: update the execute_deferred cleanup comment so it matches remove_orphaned_files and remove_deferred.
  3. Optional: use the clearer PR title, and state the callable-only scope and the E2E decision in the description.
  4. Optional: in test_remove_deferred_after_clear_session_refs, assert that execute_deferred raises for both removed ids.

Verdict

APPROVED: All three reviews approve a correct, session-scoped fix that stops a disabled callable download button from exposing an executable file id, with no merge-blocking issues.


This is an automated AI review consolidated by grok-4.7-xhigh. Please verify the feedback and use your judgment.

This review also includes 2 inline comment(s) on specific code lines.

Comment thread lib/streamlit/elements/widgets/button.py
Comment thread lib/streamlit/elements/widgets/button.py Outdated
A disabled button that switches from a callable to static data left
the previous generator executable until orphan cleanup.

Co-authored-by: Cursor <cursoragent@cursor.com>
@lukasmasuch lukasmasuch added the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 6, 2026
@github-actions github-actions Bot removed the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 6, 2026

@github-actions github-actions Bot left a comment

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.

Summary

This PR makes disabled=True a server-side check for callable st.download_button data. A disabled callable is no longer registered and no longer publishes a deferred file id. A later disabled render revokes a callable that an earlier enabled render registered at the same coordinates, including after clear_session_refs and when this run passes bytes or text. Enabled buttons still run the callable on every download. A disabled button given bytes or text directly still publishes a /media/ URL, because those bytes are produced while the script runs.

All four expected reviews completed: gpt-5.6-sol-high, claude-opus-5-5-high, gemini-3.8-flash-high, and grok-4.7-xhigh. None failed. They agree the revocation logic, session scoping, and Python tests are sound. Votes split 2–2. claude-opus-5-5-high blocks on a frontend regression from the new empty-url state. gpt-5.6-sol-high blocks on revocation missing a button whose delta path changes during a fragment rerun. gemini-3.8-flash-high and grok-4.7-xhigh approve. The empty-url source check is a real regression and should be fixed before merge. The fragment coordinate gap is a follow-up to the existing media identity model.

Product Alignment

This is a user-facing bug fix. st.download_button(..., disabled=True) with callable data no longer leaves an executable file id. The public signature, defaults, docstring, and rendered disabled control stay the same. specs/AGENTS.md does not require a product spec for a bug fix, and no merged spec covers deferred-download revocation. The PR body documents the intended scope, including that static bytes still publish a /media/ URL, and the implementation matches that scope.

That matches principle 10 in specs/AGENTS.md (Same Name, Same Behavior): disabled=True means the widget action does not run. It also matches principle 9 (Match User Expectations): a disabled download should not run its callable, including for a client that kept an earlier file id. Disabled static data still exposes a URL because the bytes already exist from the script run. The PR adds no new public surface.

Code Quality

The change is small and follows the media manager's lock, session, and coordinate patterns. DeferredCallableEntry records session_id so remove_deferred can find callables after clear_session_refs drops the coordinate map. The unmap step deletes the coordinate only when the mapped id is one of the deferred ids just removed, so a static file at the same slot stays referenced. marshall_file takes disabled as a keyword-only argument with a default, which matches lib/AGENTS.md. Moving download_button_proto.disabled = disabled above marshall_file has no behavioral effect. A download already inside execute_deferred still finishes, because the callable is read and the lock released before it runs.

Reviewers agree on that structure. They disagree on a button that moves. gpt-5.6-sol-high treats delta-path revocation as merge-blocking: a fragment rerun skips clear_session_refs (script_runner.py), so a keyed button whose path changes keeps the old file id executable. claude-opus-5-5-high and grok-4.7-xhigh treat that as a follow-up. The full-run path still drops the old id: clear_session_refs unmaps every coordinate, and _remove_orphaned_deferred_callables deletes deferred ids no session references. The leftover window is a fragment rerun where the button's delta path changes. MediaFileManager already documents that coordinate identity misses elements that move. Rekeying by element id would change that lifetime for every deferred download and is a separate change. Same-coordinate revocation is the bug this PR fixes.

The disabled callable path is the merge blocker, and the mechanism is in the inline comment on button.py. marshall_file now sends url="" with no deferred_file_id. DownloadButton has always treated a missing deferred id as a static download and checks that URL on mount. gemini-3.8-flash-high saw the same fetch("") and called it a follow-up because a normal app document returns 200. That success path is real, and it still downloads the current page for every mounted disabled callable button. When the response is non-OK, checkSourceUrlResponse reports a Download Button client error to the host. That state is new for a mounted button, so the guard belongs in this PR.

Performance

All four reviews agree the cost is limited to disabled download buttons. remove_deferred holds the existing media-manager lock and scans _deferred_callables. Enabled buttons, media serving, and the backend hot paths in lib/streamlit/AGENTS.md are unchanged. No benchmark is warranted. An index by (session_id, coordinates) is optional and is noted inline.

The new empty URL adds one same-origin document fetch per mount of a disabled callable button. Effect dependencies stay stable across reruns when the URL and deferred id stay empty, so this is once per mount, outside the frontend hot paths in frontend/AGENTS.md. Skipping the check when url is empty removes that fetch.

Test Coverage

The Python tests are thorough, and all four reviews agree. They cover a disabled callable that is never registered, revocation after clear_session_refs, a switch from callable to disabled static data, re-enabling with repeat downloads, session isolation, preservation of a static file at the same coordinates, and a no-op for unknown coordinates.

The gap is the new frontend state. There is no unit test where url and deferredFileId are both empty, so the source check is unguarded. That test should land with the guard.

E2E coverage split the reviews. grok-4.7-xhigh is right that a click-through e2e test cannot see the server bug: the browser already ignores clicks on a disabled button, and the Python tests call execute_deferred directly. claude-opus-5-5-high wants an e2e case mainly to catch the client error. gpt-5.6-sol-high wants an enabled-to-disabled scenario marked @pytest.mark.external_test. A frontend unit test is the right check for the source-url regression. An e2e case is optional.

One new nested helper omits a return annotation; that nit is inline.

Backwards Compatibility

The public st.download_button signature is unchanged. Enabled deferred downloads still run the callable on every click. Disabled buttons that receive bytes, text, or file objects still publish a /media/ URL. marshall_file's new argument is keyword-only with a default, and its only caller is button.py. DeferredCallableEntry.session_id is in-memory only. A client that sends a file id from a button that has since rendered disabled at the same coordinates gets "Deferred file not found". That is the bug this PR fixes.

Security & Risk

This closes an authorization hole. The browser's disabled state was the only check, and DeferredFileHandler still runs any registered file id. Ids remain random uuid4().hex values. Revocation is limited to the active session, so one session cannot drop another's callable at the same coordinates.

execute_deferred still authorizes by file id alone. Entries now store session_id, so comparing it to the requesting session would be cheap defense in depth. That handler is unchanged and can stay a follow-up.

The fragment path-change window described under Code Quality remains until the next full run. It is the existing coordinate lifetime, narrower than the previous behavior where every disabled callable stayed executable.

No new dependencies, routes, cookies, CSP, HTML rendering, or download endpoints.

External test recommendation

  • Recommend external_test: No
  • Triggered categories: None
  • Evidence:
    • lib/streamlit/elements/widgets/button.py: skips registering a deferred callable when disabled=True and revokes the active session's entry at that button's coordinates. Routes, cookies, headers, and the download URL format for static data stay as they are.
    • lib/streamlit/runtime/media_file_manager.py: in-memory bookkeeping (session_id, remove_deferred). /media/ serving, execute_deferred, and the backend-operation handler are unchanged.
  • Suggested external_test focus areas:
    • None required for the server change. Registration and revocation run inside the process and do not depend on proxy, iframe, or CSP boundaries.
  • Confidence: High
  • Assumptions and gaps: gpt-5.6-sol-high recommended Yes under category 6 (service worker, uploads, and downloads) because deferred-download eligibility changes. The checklist hits upload/download endpoints, JWT or CSRF wrapping, and download-attribute behavior. Those are unchanged, so the category stays clear. The empty-url fetch("") can differ behind an auth proxy; that is the frontend guard in the inline comment, on the existing source-check path. With that guard, hosted and local behavior match.

Accessibility

No frontend rendering, focus, labels, keyboard handling, or ARIA changes. The button still receives the existing disabled proto field, and shortcut registration already honors disabled. The source-check guard does not change the accessible control.

Readability

lib/streamlit/runtime/media_file_manager.py

  • remove_deferred docstring, lines 345–348. The first sentence states the action. "No-op" does not say what the caller gets, and the second sentence leads with the edge case. Proposed rewrite:
"""Revoke this session's deferred callables at ``coordinates``.

Also deletes entries that ``clear_session_refs`` already unmapped, so a
disabled button drops the previous run's file id immediately. Does nothing
when no entry matches.

Safe to call from any thread.
"""
  • execute_deferred, lines 433–434, outside this diff. The comment says the callable is cleaned up when clear_session_refs() runs. That call drops the coordinate map and leaves the callable in place. Cleanup is remove_orphaned_files(), and this PR adds remove_deferred() for the disabled path. Proposed rewrite:
# Keep the callable so the user can download more than once.
# remove_orphaned_files() drops it once no session references it.
# remove_deferred() drops it when the button renders disabled.

lib/tests/streamlit/elements/download_button_test.py

  • test_disabling_callable_drops_previously_registered_entry, lines 332–337. "Unmaps session files", "orphan cleanup", and "generator" assume local vocabulary, and "entry" does not say what is revoked. Proposed name: test_disabling_callable_revokes_previous_file_id. Proposed docstring:
"""Rendering the button disabled revokes the file id from the previous run.

A full rerun drops the session's coordinate map before widgets render, but
the callable stays registered until end-of-run cleanup. The disabled render
must delete it immediately so a download request cannot run the callable.
"""

lib/tests/streamlit/runtime/media_file_manager_test.py

  • test_remove_deferred_after_clear_session_refs, line 913. The docstring says the previous callable is not executable. The test checks that both ids are gone from _deferred_callables and does not call execute_deferred. Proposed rewrite: """remove_deferred deletes every prior id at these coordinates after clear_session_refs."""

lib/streamlit/elements/widgets/button.py

  • The comment above remove_deferred (lines 1860–1862) already states the authorization rule and the static-data case. Leave it.

PR title and description

  • Title: [fix] Reject disabled deferred downloads. "Reject" reads like an error response, and "deferred" does not name the widget, so the title does not stand alone in a changelog. Proposed title: [fix] Revoke disabled download-button callables.
  • Description: The body leads with the mechanics and explains why static data still publishes a /media/ URL. It does not say why the server drops the id. The testing plan names test methods the diff already shows. Proposed rewrite:
## Describe your changes

A disabled `st.download_button` whose `data` is a callable no longer gives the browser a file id that can run that callable. The browser's disabled state is only a UI check; a client could previously request the id directly. When a later run renders the button disabled, the server drops the callable from the earlier enabled run, including when this run passes bytes or text. An enabled button still runs the callable on every download.

A disabled button that receives bytes or text directly still publishes a `/media/` URL, because those bytes are produced while the script runs.

## GitHub Issue Link (if applicable)

## Testing Plan

- [x] Unit Tests (JS and/or Python)
  - A disabled callable is not registered. A later disabled render revokes the previous id, including a switch to static data and after the session coordinate map is cleared. Re-enabling supports repeat downloads. Removal stays on the active session and leaves static files in place.
- [x] E2E Tests
  - Omitted. The browser already ignores clicks on a disabled button. These unit tests cover the server refusing a previously issued file id.

Recommendations

  1. Fix the empty-url source check and add the frontend unit test described in the inline comment on button.py. That is the merge blocker.
  2. Optional follow-up: key deferred entries by session and element id so revocation still works when a button moves during a fragment rerun.
  3. Optional: have execute_deferred reject a file id whose stored session_id differs from the requesting session.
  4. Optional: apply the docstring, test-name, and PR title rewrites in Readability.

Verdict

CHANGES REQUESTED: The server-side revocation is correct and well tested, and it should land with a frontend guard so a disabled callable download button does not fetch the current page or report a spurious client error.


This is an automated AI review by grok-4.7-xhigh (consolidation of gpt-5.6-sol-high, claude-opus-5-5-high, gemini-3.8-flash-high, and grok-4.7-xhigh). Please verify the feedback and use your judgment.

This review also includes 4 inline comment(s) on specific code lines.

Comment thread lib/streamlit/elements/widgets/button.py
Comment thread lib/streamlit/runtime/media_file_manager.py Outdated
Comment thread lib/streamlit/runtime/media_file_manager.py Outdated
Comment thread lib/tests/streamlit/runtime/media_file_manager_test.py Outdated
@github-actions github-actions Bot added the do-not-merge PR is blocked from merging label Oct 6, 2026
A disabled callable download has no URL. Checking that URL fetched
the current page and could report a client error.

Co-authored-by: Cursor <cursoragent@cursor.com>
@lukasmasuch lukasmasuch changed the title [fix] Reject disabled deferred downloads [fix] Revoke disabled download-button callables Oct 6, 2026
@lukasmasuch lukasmasuch added the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 6, 2026
@github-actions github-actions Bot removed the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 6, 2026

@github-actions github-actions Bot left a comment

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.

Summary

This PR stops a disabled callable st.download_button from registering a generator or publishing a deferred file id, revokes this session's earlier callable when a later run renders that button disabled (including when the later run passes bytes or text), and skips the download button's source-URL check when the URL is empty so fetch("") does not re-request the current page. Enabled buttons still keep the callable so repeat downloads run it again.

All four expected reviews completed: gpt-5.6-sol-high, claude-opus-5-5-high, gemini-3.8-flash-high, and grok-4.7-xhigh. None failed to complete. Three approve (claude-opus-5-5-high, gemini-3.8-flash-high, grok-4.7-xhigh). gpt-5.6-sol-high requests changes.

They agree this is a focused bug fix: session-scoped revocation, keyword-only disabled=False on marshall_file, static files left mapped at the same coordinates, strong same-path unit tests, no public API change, no accessibility change, and no external-test coverage. They also agree a disabled button given bytes or text still publishing a /media/ URL is the scope stated in the PR body.

They disagree on one security lifecycle case. gpt-5.6-sol-high treats delta-path revocation as merge-blocking when a button moves. claude-opus-5-5-high treats layout shift as acceptable because orphan cleanup runs at the end of the run. That cleanup does drop an unreferenced id after a full rerun, which calls clear_session_refs first. Fragment reruns skip that clear and keep the old coordinate mapping (script_runner.py), so orphan cleanup retains the id. gemini-3.8-flash-high and grok-4.7-xhigh do not discuss a moved path. The fragment case is a real hole in the authorization behavior this PR adds, and the same-coordinate tests cannot see it.

Product Alignment

This is a user-facing bug fix. A disabled callable download no longer exposes an executable file id, and an empty download URL no longer triggers a client fetch of the current page. It adds no public API, parameter, default, or rendered control. Bug fixes do not need a product spec. No merged spec covers deferred-callable revocation, and the PR body does not claim a separate product approval.

disabled=True means the control cannot be activated (specs/AGENTS.md, principle 10, "Same Name, Same Behavior"). A callable data value runs when the download is requested. Publishing that callable's file id while the button is disabled leaves a server action the UI presents as unavailable. Closing that gap matches the existing deferred-download design: enabled buttons still run the callable on every download.

The PR body states that a disabled button given bytes or text still publishes a /media/ URL, because those bytes already exist when the script runs. The implementation matches that boundary. The fragment delta-path gap below is an incomplete enforcement of this fix, not a new product decision.

Code Quality

The change is small and follows the media-file manager's locking and session map. DeferredCallableEntry.session_id keeps revocation inside the registering session. Unmapping a coordinate only when the mapped id was removed leaves a static file at those coordinates referenced. marshall_file(..., *, disabled=False) stays compatible with positional callers. The disabled callable path clears url and returns before add_deferred.

Revocation identity is the problem. remove_deferred selects entries by the button's current delta path. That path is the element's position (DeltaGenerator._get_delta_path_str), and a keyed download button's element id does not include it (key_as_main_identity=True). On a fragment rerun the previous path stays in _files_by_session_and_coord, so the earlier file id remains reachable after the button has rendered disabled at its new path. The inline comment has the concrete fix: remember the element id, match that id or the current coordinates, and unmap every coordinate that pointed at a removed id.

The full scan of _deferred_callables under the global media lock is acceptable for today's cardinality. The identity fix should look the entry up directly rather than preserve that scan.

Performance

No documented backend hot path (element enqueue, ForwardMsg hashing, delta flush, session state, caching, dataframe serialization) or frontend hot path (WebSocket decode, render tree, markdown, widget-state scans) is involved.

remove_deferred runs only for a disabled download button, then walks every session's deferred entries while holding the media-file lock. Three reviews treat that as negligible; claude-opus-5-5-high and gpt-5.6-sol-high note it is work proportional to all live deferred callables and contends with image, audio, and download registration. Typical apps have few of these entries, so the scan alone is not merge-blocking. Pair the identity fix with an index so a disabled render does not keep scanning the global table.

On the frontend, the new url effect dependency is a stable string already reflected in downloadUrl. Skipping the check removes a wasted fetch("") of the current page.

Test Coverage

Same-path coverage is thorough. download_button_test.py checks that a disabled callable is not registered or invoked, that a later disabled render revokes the previous id after clear_session_refs, that switching to static bytes revokes the generator and still publishes a /media/ URL, and that enabling the button again supports repeat downloads. media_file_manager_test.py checks session scope, multiple ids at one coordinate after clear_session_refs, a static file left mapped, and a missing coordinate as a no-op. DownloadButton.test.tsx checks that an empty URL with no deferred id does not call checkSourceUrlResponse.

Those tests always use one coordinate, so they pass while the moved-path fragment case stays open. A regression test needs to register a callable at one path, leave the session coordinate map in place (the fragment behavior), and disable the button at a different path. Assert the previous id is gone and execute_deferred raises. A new Playwright test is unnecessary: the browser already refuses a click on the disabled control, and this failure is server-side capability revocation.

Backwards Compatibility

Public st.download_button signatures, defaults, and return types are unchanged. marshall_file's new argument is keyword-only and defaults to False. Enabled deferred downloads still run on each click. Disabled bytes or text still get a media URL.

A client that previously called a disabled button's published file id will see that id stop working once revocation matches it. That is the intended fix. Until the identity gap is fixed, an id registered at another delta path on a fragment rerun keeps working.

Security & Risk

The PR closes a real capability leak for a stable delta path. execute_deferred runs the callable stored for a file id, DeferredFileHandler does not check the caller, and a disabled callable used to put that id on the protobuf anyway. The button now omits the id, and remove_deferred drops this session's entries stored under the coordinates of this render, including ids clear_session_refs has already unmapped. One session cannot revoke another session's callable. There is no new route, auth change, HTML rendering, eval, subprocess, cookie, or dependency.

That revocation misses the common fragment layout shift. ScriptRunner._run_script clears media session refs only when fragment_id_queue is empty, so download links outside the fragment keep working. _remove_orphaned_deferred_callables then deletes a callable only when no session coordinate map still references it. Sequence:

  1. An enabled keyed download button registers file id F at delta path A. The browser receives F.
  2. A fragment rerun renders conditional content above the button and renders the button disabled at delta path B.
  3. remove_deferred(B) finds no entry stored with coordinates B. The map still has A → F.
  4. Orphan cleanup keeps F. A client that saved F can still requestDeferredFile until some later full rerun.

A normal click is still blocked in the UI. The threat model this PR adopts is a client that retained the id, because the browser's disabled state is not the authorization check. The same pattern (a checkbox or other conditional block above a keyed download button inside @st.fragment) can persist for the whole session. Full reruns still drop the id at end-of-run orphan cleanup even when the path changed, because clear_session_refs has already cleared the map. The persistent leak is the fragment case.

claude-opus-5-5-high described layout shift as revoking a stale id early and called that acceptable. That description assumes the end-of-run orphan pass deletes the unmatched id. On a fragment rerun the old path is still referenced, so the pass keeps it.

External test recommendation

  • Recommend external_test: No
  • Triggered categories: None
  • Evidence:
    • lib/streamlit/elements/widgets/button.py: Disabled callable downloads skip add_deferred and clear a prior registration during marshalling. Routes, URL format, JWT/CSRF wrapping, and the HTML download attribute are unchanged.
    • lib/streamlit/runtime/media_file_manager.py: In-memory session and coordinate bookkeeping for deferred callables. Media serving and asset handlers are unchanged.
    • frontend/lib/src/components/widgets/DownloadButton/DownloadButton.tsx: Skips checkSourceUrlResponse when the download URL is empty. buildDownloadUrl, link creation, and cross-origin fetching are unchanged.
  • Suggested external_test focus areas:
    • None. Cover the moved-path fragment case with a backend test that preserves session refs.
  • Confidence: High
  • Assumptions and gaps: Deferred execution still uses the existing backend-operation message. Hosted and embedded apps see the same omitted file id. The lifecycle bug is process-local and shows up the same way outside a proxy or iframe.

Accessibility

No accessibility impact. Role, accessible name, shortcut handling, and the disabled state still come from the existing button props. The edit only skips a source check when there is no URL to check.

Readability

PR title

[fix] Revoke disabled download-button callables stands alone, uses the [fix] prefix, and fits the length limit. claude-opus-5-5-high preferred a more user-facing title ("Don't expose deferred downloads of disabled st.download_button"). gpt-5.6-sol-high and grok-4.7-xhigh accept the current title. The current title is clear enough for a changelog; no rewrite is required.

PR description

The body states the server behavior and why static bytes still publish a /media/ URL. It never says why revocation is required (a client can request a published file id while the control is disabled), and it omits the empty-URL client fix. gemini-3.8-flash-high and grok-4.7-xhigh both flag the missing frontend note; claude-opus-5-5-high and gpt-5.6-sol-high both want the authorization reason up front.

Proposed description:

A disabled `st.download_button` whose `data` is a callable no longer registers that callable or publishes a deferred file id. The browser's disabled state does not stop a client that already has the id, so a later run that renders the button disabled also drops the callable from the earlier enabled run, including when this run passes bytes or text. Repeat downloads of an enabled button still run the callable.

A disabled button that receives bytes or text directly still publishes a `/media/` URL, because those bytes are produced when the script runs.

The download button skips its source check when the URL is empty. `fetch("")` re-requests the current page and can report a client error.

Line-level comment rewrites are in the inline comments.

Recommendations

  1. Revoke a disabled button's earlier deferred callable when its delta path changed and the session coordinate map was preserved, and add the regression test described in Test Coverage. This is the merge blocker.
  2. Look that revocation up by session and element id (keeping a coordinate match for an in-place identity change) so the disabled path does not scan every session's deferred callables under the media lock.
  3. Optionally reject deferred execution when the request session does not own the file id, now that entries store session_id.
  4. Optionally apply the PR description rewrite above. The static-bytes /media/ URL behavior is already an explicit scope boundary and can stay as it is.

Verdict

CHANGES REQUESTED: Same-path revocation is correct and well tested, but a fragment rerun that moves a disabled callable download button leaves the previous file id executable, and that case needs a fix and a regression test before merge.


This is an automated consolidated AI review by grok-4.7-xhigh. Please verify the feedback and use your judgment.

This review also includes 4 inline comment(s) on specific code lines.

Comment thread lib/streamlit/runtime/media_file_manager.py Outdated
Comment thread lib/streamlit/runtime/media_file_manager.py
Comment thread lib/streamlit/elements/widgets/button.py Outdated
Comment thread frontend/lib/src/components/widgets/DownloadButton/DownloadButton.tsx Outdated
@lukasmasuch lukasmasuch added ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. ai-qa-test If applied to PR, an AI qa testing will run on the feature implemented in the PR. labels Oct 6, 2026
@github-actions github-actions Bot removed the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 6, 2026

@github-actions github-actions Bot left a comment

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.

Summary

This PR stops a disabled st.download_button whose data is callable from registering that callable or sending a deferred file id. When a later run renders the button disabled, the server revokes a callable registered on an earlier run, including when this run passes bytes or text. Enabled buttons still run the callable on every download. The frontend skips the source-URL check when there is no URL, so fetch("") does not reload the current page.

All four expected reviews completed (gpt-5.6-sol-high, claude-opus-5-5-high, gemini-3.8-flash-high, grok-4.7-xhigh). No expected model failed to complete a review. Claude and Gemini approved. GPT and Grok requested changes. All four agree that same-coordinate revocation is correct, session-scoped, and well tested, and that there is no breaking API change, material performance regression, or external-test gap. The split is the fragment lifecycle: revocation looks the callable up by delta path. A fragment rerun that moves the button leaves the previous generator executable. That bypass is real and merge-blocking.

Product Alignment

This is a user-facing bug fix. It changes whether a disabled callable download can still be executed. It adds no public parameter, return value, default, or config. specs/AGENTS.md does not require a product spec for bug fixes. No download-button spec is merged on develop. The PR body does not claim a separate product approval, and it does not need one.

The PR body records one preserved behavior, and the implementation matches it: a disabled button given bytes or text still publishes a /media/ URL, because those bytes are produced when the script runs. Enforcing disabled=True on the server matches Principle 10 (Same Name, Same Behavior): a disabled control must not run the action. The public docstring already points users at @st.fragment so a download click does not rerun the whole app, which is why the fragment gap below is part of normal use of this fix.

Code Quality

The change fits the media-file manager. session_id has to live on the deferred entry because a full run calls clear_session_refs before widgets render, so the coordinate map is already gone when a disabled button needs to revoke the previous id. The lock scope matches add_deferred and execute_deferred. Unmapping only an id this call removed is the right way to leave a static file at the same coordinates in place. marshall_file(..., *, disabled=False) stays compatible with existing positional callers.

The lookup key is the defect. remove_deferred in lib/streamlit/runtime/media_file_manager.py keeps entries whose stored delta path equals the path passed in. Delta paths are layout slots (DeltaGenerator._get_delta_path_str). The download button element id stays stable when disabled flips and when a sibling is inserted above it: disabled is not an input to compute_and_register_element_id, and the delta path is not part of that id. The inline comment on the lookup has the required change.

Performance

There is no meaningful regression. remove_deferred walks _deferred_callables under the media-manager lock once per disabled download button, including buttons whose data is already bytes or text. That dict holds one entry per deferred button per session, plus ids waiting for orphan cleanup. It is outside the backend hot paths (element enqueue, ForwardMsg hashing, session-state scans, cache replay). An early return when the table is empty is optional and is noted inline. The frontend adds url to an effect that already depends on downloadUrl, which is derived from url. The effect does not set state, so it does not cause an extra render. No new benchmark is warranted.

Test Coverage

Unit coverage of the stable-slot case is strong. The tests check that a disabled callable is not registered and is not invoked, that a later disabled render revokes the id after clear_session_refs, that turning the button back on still supports repeat downloads, that switching the slot to disabled static bytes revokes the generator and still publishes a /media/ URL, and that remove_deferred is session-scoped, deletes every prior id at those coordinates, leaves a static file mapped at the same coordinates, and ignores an unknown coordinate. The frontend test checks that an empty URL with no deferred id does not call checkSourceUrlResponse.

The case this bug needs is missing: keep session media references, register a callable at one coordinate, revoke it from the same element id at a different coordinate, and assert execute_deferred fails. A backend unit test is the right layer. The browser already blocks clicking the disabled control.

Backwards Compatibility

The public signature of st.download_button is unchanged. Enabled deferred downloads, including repeat clicks, behave as before. Disabled static data still receives a media URL. Invalidating a previously issued deferred id after the button becomes disabled is the intended bug fix. Callers that depended on invoking a disabled button's generator were relying on the bug.

Security & Risk

For a stable delta path, disabled is enforced on the server. The browser no longer receives a deferred file id, and execute_deferred fails for the id from the previous run. One session cannot revoke another's callable. Static bytes stay fetchable via /media/ when the button is disabled, which matches the behavior the PR describes.

Fragment reruns are the remaining hole. script_runner._run_script calls media_file_mgr.clear_session_refs() only when fragment_id_queue is empty, so download links outside the fragment do not 404. remove_orphaned_files then deletes a deferred callable only when no session map still points at its id. If the same fragment run disables the button and inserts an element above it, the delta path changes, remove_deferred misses the old entry, and the file id delivered while the button was enabled stays callable until a later full run. A full rerun does not keep this leak: session refs are cleared up front, and orphan cleanup drops the unreferenced callable at the end of that run.

A fragment rerun that omits the download button entirely never calls remove_deferred, so the previous callable stays mapped. That lifecycle is outside the behavior this PR describes. The element-id fix does not cover it. Fragment-scoped media unmap, following the dataframe-source pattern in script_runner.py, would.

execute_deferred still treats the uuid4 file id as a bearer capability. Entries now store session_id, and DeferredFileHandler.handle already receives the requesting session and ignores it. Checking ownership is worthwhile defense in depth and is a follow-up. No new endpoint, cookie, CSP, eval, or HTML-sanitization surface.

External test recommendation

  • Recommend external_test: No
  • Triggered categories: None
  • Evidence:
    • lib/streamlit/runtime/media_file_manager.py: in-memory deferred-callable registry only. No route, cache-header, CORS, cookie, or /media handler change.
    • lib/streamlit/elements/widgets/button.py: skips registering a callable when disabled=True and revokes by coordinates. No auth, session-transport, or SiS change.
    • frontend/lib/src/components/widgets/DownloadButton/DownloadButton.tsx: skips checkSourceUrlResponse when url is empty. No postMessage, iframe, storage, crossOrigin, or download-attribute change.
  • Suggested external_test focus areas:
    • None. Revocation and the empty-URL guard are server and component behavior. Embedded hosting does not take a different code path. The moved-path bug belongs in a backend unit test that preserves session references.
  • Confidence: High
  • Assumptions and gaps: Deferred-file execution stays on the existing websocket backend-operation channel, and /media URL construction is unchanged. The fragment delta-path bug is a server-side identity bug, not an embedded-host divergence.

Accessibility

No accessibility change. The download control is still a button, still receives disabled, and the shortcut hook already no-ops while disabled. Skipping the empty-URL fetch does not change the accessible name, focus order, or keyboard behavior.

Readability

PR title and description

The title [fix] Revoke disabled download-button callables stands alone and matches [type] Description. Three of the four reviews would keep it. Claude's alternative ([fix] Don't expose deferred file ids for disabled download buttons) is clearer to someone outside the media-file manager, and either title is fine.

The description leads with registration mechanics and omits why server-side revocation is required and the empty-URL frontend fix. The static-URL paragraph and the testing-plan bullets should stay. Proposed opening:

A disabled `st.download_button` whose `data` is callable no longer registers
the callable or publishes a deferred file id. A client can retain and request
an earlier id independently of the disabled browser control, so a later
disabled render also revokes that callable, including when this run passes
bytes or text. Enabled buttons still run their callable on every download.

A disabled button that is given bytes or text directly still publishes a
`/media/` URL. Those bytes are produced when the script runs, not when the
button is clicked.

The frontend skips the source-URL check when a button has neither a URL nor a
deferred id, because `fetch("")` re-requests the current page.

The empty "GitHub Issue Link" heading can be removed when there is no issue.

lib/streamlit/runtime/media_file_manager.py (execute_deferred, lines 434–436)

Three cleanup actors are packed into successive sentences. Proposed rewrite:

# Keep the callable so the user can download more than once.
# It is dropped when:
# - remove_orphaned_files() runs and no session references it
# - remove_deferred() runs because the button rendered disabled

Comment rewrites for marshall_file and the DownloadButton effect are in the inline comments.

Tests

Names and docstrings describe the behavior they cover. Optional renames, if those lines are already being edited: does not check the source url when a callable download has no url can be does not check an empty source URL (the fixture does not mark the button disabled), and test_disabling_callable_drops_previously_registered_entry can be test_disabling_callable_revokes_previous_file_id.

Recommendations

  1. Revoke by widget identity as well as delta path. Store the download button's element id on DeferredCallableEntry, delete this session's entries whose element_id or coordinates match, and unmap each removed entry's stored coordinates only when the session map still points at that file id. Add a unit test that preserves session references, registers at one coordinate, and revokes the same element id at another. See the inline comment on remove_deferred.
  2. Follow-up, separate from this PR's stated behavior: a fragment rerun that omits the download button never calls remove_deferred, and the previous callable stays mapped. Fragment-scoped media unmap would cover that.
  3. Follow-up: validate deferred-file ownership against the requesting session now that entries record session_id.
  4. Update the PR description so it states the server-side authorization reason and the empty-URL frontend fix.

Verdict

CHANGES REQUESTED: Same-coordinate revocation is correct and well tested, but a fragment rerun that moves a disabled callable download button leaves the previous generator executable.


This is an automated AI review consolidated by grok-4.7-xhigh. Please verify the feedback and use your judgment.

This review also includes 6 inline comment(s) on specific code lines.

Comment thread lib/streamlit/runtime/media_file_manager.py Outdated
Comment thread lib/streamlit/elements/widgets/button.py Outdated
Comment thread lib/streamlit/runtime/media_file_manager.py Outdated
Comment thread lib/streamlit/runtime/media_file_manager.py
Comment thread lib/streamlit/elements/widgets/button.py Outdated
Comment thread frontend/lib/src/components/widgets/DownloadButton/DownloadButton.tsx Outdated
@github-actions github-actions Bot removed the ai-qa-test If applied to PR, an AI qa testing will run on the feature implemented in the PR. label Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

QA Report: Disabled deferred download buttons

Summary

  • Date: 2026-10-06
  • Branch: PR 17288 (lukasmasuch/disabled-deferred-export-fix), detached HEAD 4e5660b26a ("Skip empty download URL source checks")
  • App URL: http://localhost:3001
  • Debug session: work-tmp/debug/20261006-133956-test_app.py-9314/
  • Total tests: 35
  • Passed: 35
  • Failed: 0
  • Blocked: 0

Overall status

PASS

Test results

Passed tests

ID Scenario Notes
F1 Disabled callable on first render Button disabled; server status registered=false, invocations 0; click does not download or rerun; no fetch of the page URL; button protobuf after the label has no deferred file id
F2 Enabled callable publishes a file id Isolated page protobuf contains the deferred id (8db79c69…) immediately after the label
F2b Repeat clicks invoke the callable again Same id produced toggle:5 then toggle:6 with on_click="ignore" (counts continue from an earlier QA pass in this process)
F3 Disable revokes the previous id Probe REVOKED:MediaFileStorageError; captured id matches; invocation count unchanged; click does not download
F4 Switch to disabled static bytes One selectbox rerun revokes the callable and publishes /media/81efb481….txt whose body is MODE_SWITCH_STATIC
F5 Re-enable supports repeat downloads New id; payloads toggle:7 and toggle:8
E6 Disable again That new id is revoked and is not executable
F6 Disabled static text and bytes Both buttons stay disabled and their /media/ URLs return the original payloads
E5 Disabled StringIO and BytesIO DISABLED_STRINGIO and DISABLED_BYTESIO are fetchable
E7 Static URL after another revoke Disabled static text still returns DISABLED_STATIC_TEXT after the toggle button is revoked
F7 Neighbors while toggle is enabled Buttons before and after the toggle both download
F7b Neighbors after revoke Both neighbors download again with the next counter value
F8 Enabled static download Body ENABLED_STATIC_TEXT, filename enabled_static.txt
E8 Enabled static after deferred history Mode-switch button downloads MODE_SWITCH_STATIC and is not deferred
F9 Failing callable Enabled click shows "Failed to generate file for download"; disabled button stays unregistered and shows no error
F10 on_click="ignore" Script run count stayed 1 across deferred downloads
F11 on_click callback Disabled click does not run the callback; enabled click downloads callback:2, returns true, and increments the callback once
F12 Bytes callable Body matches bytes:N, filename bytes.bin
F13 Lambda callable Body matches lambda:N, filename lambda.txt
E1 Empty-string callable Download succeeds with an empty body, filename empty.txt
F14 Fragment disable Fragment runs 8 → 9 while the full-script run count stays 8; previous id revoked; button disabled
F15 Sidebar Disabled sidebar callable is unregistered; enabled sidebar callable downloads
F16 Two sessions Distinct file ids; disabling session B revokes only B; session A’s second download is the next payload
E2 Inside st.form Existing error: st.download_button() can't be used in an st.form()
E3 Primary and tertiary disabled Both disabled and unregistered; generators not called
E4 Class and partial, disabled Both unregistered
E4b Enabled class callable Download body matches class_enabled:N
V1 Desktop 1280px Toggle button enabled; primary disabled button disabled. Screenshots v1_desktop.png, v1_toggle_enabled.png, v1_primary_disabled.png
V2 Narrow 390px Long-label, toggle, and primary buttons stay inside the viewport and keep their disabled state. Screenshot v2_narrow.png
I1 Checkbox "Disable toggle download" checks and disables the button
I1b Selectbox "disabled static" applies in one rerun
I2 Help tooltip Hover on the disabled callable shows "Help for a disabled callable download"
I3 Shortcut Ctrl+Alt+J No download while disabled; download of shortcut:N after enabling
L1 Backend log 4 tracebacks, all from the intentional failing callable (ValueError: intentional download failure). The form-layout error is the expected E2 exception
L2 Frontend / console No Client Error: Download Button

Failed tests

None.

Blocked tests

None.

Issues found

No user-facing functional bugs.

An initial leak check flagged 32-character hex strings in websocket frames that also contained the disabled button label. Those strings are the ForwardMsg hash and the active script hash, both of which sit before the label. The download-button protobuf after Disabled only is the element id, the label, disabled=true, and type=secondary, with no URL and no deferred file id. The same window on an enabled button contains the deferred id. The check was corrected and the full suite was run again.

Recommendations

  • No product change is required for this bugfix.
  • The disconnect console line below is not part of the download-button fix. It showed up when the QA page navigated between ?scenario= URLs and closed extra browser contexts.

Notes

  • Spec: none. Agent-wiki pull-requests/17288/: not present.
  • Expected behavior came from the PR body, marshall_file / remove_deferred, the download-button docstring, and the new unit tests.
  • Browser console recorded Cannot send rerun backMessage when disconnected from server while changing scenarios. It did not affect downloads, revocation, or the empty-URL check. Severity: low, non-blocking.
  • Screenshots: f1_disabled_only.png, f2_enabled_only.png, f3_toggle_disabled.png, f4_disabled_static.png, f9_error.png, f14_fragment_disabled.png, f16_session_a.png, f16_session_b.png, e2_form.png, i2_tooltip.png, i3_shortcut.png, v1_desktop.png, v1_toggle_enabled.png, v1_primary_disabled.png, v2_narrow.png, final_state.png.

This is an automated QA report by grok-4.7-xhigh.

A fragment rerun can disable a keyed download button at a new delta
path. Match the stable element id so the previous callable is dropped.

Co-authored-by: Cursor <cursoragent@cursor.com>
@lukasmasuch lukasmasuch added the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 6, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 1169e15. Configure here.

Comment thread lib/streamlit/runtime/media_file_manager.py
@github-actions github-actions Bot removed the ai-final-review If applied to PR or issue will run AI review workflow with the latest generation of AI models. label Oct 6, 2026

@github-actions github-actions Bot left a comment

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.

Summary

A disabled callable st.download_button no longer registers a deferred generator or publishes a file id, and a later run drops any callable an earlier enabled run registered for that button. Revocation is session-scoped and matches the current delta path or the button's stable element id, so a fragment rerun that moves the button still invalidates the previous id when that id is unchanged. Static bytes or text on a disabled button still get a /media/ URL. DownloadButton skips the source check when url is empty, which avoids fetch("") re-requesting the current page.

All four expected reviews completed (gpt-5.6-sol-high, claude-opus-5-5-high, gemini-3.8-flash-high, grok-4.7-xhigh) and approve. They agree this is a backwards-compatible bug fix: no new public API, no hot-path cost, unit tests cover the lifecycle, and external e2e coverage is unnecessary. Residual races and comment nits are optional follow-ups.

They differed on three points, resolved against the diff:

  • Specs. One review said nothing in specs/ covers deferred downloads. Merged specs do mention deferred st.download_button as an analogy (text-input validation, dynamic tabs). None of them define revocation for a disabled callable, so the bug-fix exception in specs/AGENTS.md still applies.
  • Clearing deferred_file_id. One review suggested ClearField("deferred_file_id") in case the proto is reused. _download_button always constructs a new DownloadButtonProto() before marshall_file, and the disabled callable path sets url = "" and returns before assigning deferred_file_id. The field stays unset.
  • How complete revocation is on fragment reruns. One review spelled out a remaining identity gap. It is real and narrow, and it is smaller than the bug this PR closes. Details are under Security & Risk.

Product Alignment

This is a user-facing bug fix. disabled=True now stops the server from running the button's data callable, including from a file id the server already sent. The public signature, defaults, rendered control, and error strings stay the same. Bug fixes do not need a product spec. No merged spec defines this revocation behavior.

The PR body explicitly keeps one related behavior: a disabled button given bytes or text still publishes a /media/ URL, because those bytes are produced when the script runs. marshall_file returns before add_deferred only when data is callable; the static branch still calls MediaFileManager.add. That matches the description.

The change fits "Same Name, Same Behavior" (specs/AGENTS.md, principle 10): disabled means the control does not perform its action. The st.download_button docstring already says a callable runs on click. Enabled buttons still run the callable on every download. The PR adds no permanent API surface.

Code Quality

The change fits the existing media-file model, and the extra session index is proportionate. clear_session_refs drops coordinate mappings before a disabled render needs to find prior callables, so a full-table scan would be the alternative. remove_deferred walks only that session's ids under the manager lock, drops entries at the current coordinates or with the same element id, and unmaps a coordinate only when the map still points at the id just removed. A static file or a newer deferred id at those coordinates stays referenced. _drop_deferred_id is the single removal path for revocation and orphan cleanup, and both callers hold the lock.

st.download_button computes its element id with key_as_main_identity=True and does not include the delta path. With a user key, label and the other command arguments are omitted too, so a keyed button that moves and changes its label still matches. An unkeyed button keeps the same id when it only moves.

Coordinate matching also drops a previous run's callable that occupied this path. If that other button renders later in the same run, it registers a new id. A click that still holds the old id fails until the browser receives the new delta. Full runs already dropped that old id at orphan cleanup; this moves the drop earlier. Fragment delta paths include the root container and stay unique among elements still on screen.

disabled is assigned on the proto before marshall_file. That matters because the disabled callable path returns early; leaving the assignment after the call would skip it.

Performance

No hot-path impact under the backend or frontend performance guidance. Registration still happens once per download button. Enabled buttons store an extra session_id and element_id. Each disabled button scans that session's deferred ids under the existing media-manager lock and returns immediately when the session has none. Callable execution stays outside the lock. The frontend effect gains a url dependency and skips a network check when the URL is empty; downloadUrl is already derived from url, so the extra dependency does not add renders. Typical sessions have few deferred callables, so the scan does not need a benchmark.

Test Coverage

Unit coverage matches the behavior that changed. A disabled callable registers nothing. A later disabled render revokes the previous id after clear_session_refs, and enabling it again supports repeat downloads. Switching that button to static bytes revokes the generator and still publishes a /media/ URL. The same element id at a new delta path revokes the old id while session refs are kept. Revocation is session-scoped, leaves a static file mapped at the same coordinates, leaves an unrelated callable executable, and is a no-op for an unknown coordinate. The download button does not call checkSourceUrlResponse when the URL is empty.

The first-render case goes through st.download_button. The enabled-then-disabled cases call marshall_file so they can clear session refs between renders. That leaves the disabled / element_id wiring in _download_button covered by inspection and by the first-render test, with a small gap called out inline. A new Playwright test is optional: the browser already blocks a disabled control, and the changed property is the server-side id lifecycle.

Backwards Compatibility

The public signature of st.download_button is unchanged. The new marshall_file and add_deferred arguments are keyword-only with defaults. Enabled deferred downloads still run the callable on each click. Disabled static downloads still expose a media URL. Clients that called execute_deferred on a file id after the button became disabled now get "Deferred file not found". That is the bug fix. Apps that use a disabled button as a non-interactive control keep working.

Security & Risk

The server, rather than the disabled control in the browser, now decides that a deferred callable cannot run. File ids remain uuid4 values. Routes, WebSocket transport, media serving, authentication, dependencies, and HTML handling are unchanged. Revocation is session-scoped and runs under the manager lock.

Two pre-existing boundaries stay as they are. DeferredFileHandler.handle still ignores its session_id argument, and execute_deferred runs any registered id. Binding execution to the stored session_id is a defense-in-depth follow-up; this PR does not weaken that boundary. A request already in flight can finish, because the callable runs outside the lock. Between the start of a full run and the moment the disabled button renders, the old id can still be executed. Both windows are short.

One narrower gap remains. Fragment reruns do not call media_file_mgr.clear_session_refs(). Revocation then depends on the current path or the current element id. An unkeyed button whose identity inputs change (for example the label) and whose delta path also changes matches neither, so the previous callable stays executable until the next full run. A stable key avoids that case, because the element id then ignores the label. A full rerun is already safe: clear_session_refs unmaps the old id, and orphan cleanup drops it if this render does not match it.

Disabled static data remains fetchable at its media URL. The PR description treats that as in scope.

External test recommendation

  • Recommend external_test: No
  • Triggered categories: None
  • Evidence:
    • lib/streamlit/runtime/media_file_manager.py: in-memory, session-scoped deferred-callable lifetime only. No route, cookie, websocket handshake, asset-handler, CORS, storage, or security-header changes.
    • lib/streamlit/elements/widgets/button.py: skips add_deferred for a disabled callable and revokes an earlier entry. Static files still use MediaFileManager.add.
    • frontend/lib/src/components/widgets/DownloadButton/DownloadButton.tsx: skips checkSourceUrlResponse when url is empty. Download link creation, the download attribute, and the deferred-file backend operation are unchanged.
  • Suggested external_test focus areas:
    • None. Revocation happens before a file id is sent, and the empty-URL guard is host-independent.
  • Confidence: High
  • Assumptions and gaps: DeferredFileHandler, media URL construction, and /media/ serving are unchanged, which the diff supports. Tests were not re-run.

Accessibility

The widget still renders a BaseButton with the existing disabled prop, label, and shortcut registration. The frontend change only skips a background source check when there is no URL. Focus, name, and keyboard behavior are unchanged. Skipping fetch("") also avoids a spurious download-source client error on a disabled callable.

Readability

PR title and description

  • Title: [fix] Revoke disabled download-button callables stands alone, uses the [fix] prefix, and fits the length limit. No change.
  • Description: The opening states the behavior change and the static-data exception. It leaves out why the change matters (a client can execute a deferred id directly) and omits the frontend empty-URL fix. The fragment case is also easy to miss. The empty "GitHub Issue Link" heading can be removed, or filled in if an issue exists.
  • Proposed rewrite of the description body:
A disabled callable `st.download_button` no longer exposes an executable deferred file id. A client can run a registered `data` callable by sending the deferred-file request directly, even when the button is disabled in the UI. The server now skips registration for a disabled callable and drops any id an earlier enabled run registered for that button. That includes a later run that passes bytes or text, and a fragment rerun that moves the button while its element id stays the same. A `key` keeps that id stable if the label changes. Repeat downloads of an enabled button still invoke the callable.

A disabled button that is given bytes or text directly still publishes a `/media/` URL. Those bytes are produced when the script runs, not when the button is clicked.

`DownloadButton` skips the source-URL check when `url` is empty. Checking `""` re-fetches the current page.

lib/streamlit/runtime/media_file_manager.py

  • Location: comment in remove_deferred, around lines 375–378
  • Issue: "That button re-registers if it renders later in this run" reads as if the same download keeps working. The old file id is dropped; a later render stores a new id. A click that still has the old id fails until the browser applies the new delta.
  • Proposed rewrite:
# Also drop a different button's callable from the previous run when this
# button now uses that path. If that button renders later in this run, it
# registers a new file id. A click that still holds the old id fails until
# the browser receives the new delta.
  • Location: _drop_deferred_id, around line 408
  • Issue: session_ids holds deferred file ids, not session ids.
  • Proposed rename: deferred_ids.

Other line-level comment and test-name notes are inline.

Recommendations

  1. Optional: apply the PR description rewrite above so the security reason, the fragment/element-id case, and the empty-URL frontend fix are visible.
  2. Optional follow-up: in execute_deferred or DeferredFileHandler, reject a deferred id whose stored session_id does not match the request. Ids are unguessable, so this can land separately.
  3. Optional follow-up: on a fragment rerun, an unkeyed callable whose label (or other element-id inputs) changes while its delta path also changes stays executable until the next full run. A stable key closes that case today.
  4. Optional: apply the coordinate-comment rewrite and the deferred_ids rename above.

Verdict

APPROVED: All four reviews approve, and the remaining fragment identity window, session-ownership check, and documentation nits are optional follow-ups.


This is an automated AI review consolidated by grok-4.7-xhigh. Please verify the feedback and use your judgment.

This review also includes 6 inline comment(s) on specific code lines.

Comment thread lib/tests/streamlit/elements/download_button_test.py
Comment thread lib/streamlit/runtime/media_file_manager.py
Comment thread lib/streamlit/elements/widgets/button.py Outdated
Comment thread lib/tests/streamlit/elements/download_button_test.py Outdated
Comment thread lib/streamlit/runtime/media_file_manager.py Outdated
Comment thread frontend/lib/src/components/widgets/DownloadButton/DownloadButton.tsx Outdated
@github-actions github-actions Bot removed the do-not-merge PR is blocked from merging label Oct 6, 2026

@github-actions github-actions Bot left a comment

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.

Auto-approved by AI final review (workflow run).

A moved button stays revoked only when element_id is passed through, so the new test calls st.download_button instead of marshall_file.

Co-authored-by: Cursor <cursoragent@cursor.com>
@lukasmasuch
lukasmasuch merged commit d8982b5 into develop Oct 7, 2026
38 checks passed
@lukasmasuch
lukasmasuch deleted the lukasmasuch/disabled-deferred-export-fix branch October 7, 2026 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:bugfix PR contains bug fix implementation impact:users PR changes affect end users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants