You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/10/21 00:06:16 UTC
[PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
mikebridge opened a new pull request, #45551:
URL: https://github.com/apache/superset/pull/45551
### SUMMARY
Restoring a dashboard version can fail when, at the same moment, one of the charts it is putting back or removing gets permanently deleted (for example by the archive's retention cleanup or a manual permanent delete). Nothing is half-restored, since the whole restore rolls back, but the user only saw a generic "Dashboard could not be updated." error, with no hint that simply trying again works.
**Why it happens.** The restore first works out which charts to link and unlink, then writes the change in one go. If one of those charts is permanently deleted in between, the write fails: linking it violates the `dashboard_slices` foreign key, and unlinking it finds that the row is already gone.
**What this changes.** When the restore's write to `dashboard_slices` fails in one of those two ways, and a fresh read after the rollback confirms that one of the charts is gone, the restore endpoint returns **409** with a message asking the user to try again. This follows the existing convention for retryable conflicts ("retry the same request"). The retry restores the version without the deleted chart. Any other failure keeps the existing 422, because retrying would not help. Successful restores are unchanged.
Also included: the 409 response in the OpenAPI spec, the new message in `messages.pot`, and a sentence in the version history docs.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (API behaviour only).
### TESTING INSTRUCTIONS
- `pytest tests/integration_tests/dashboards/version_restore_tests.py -k concurrent_chart_purge`. It runs a real permanent delete on a separate connection between the restore's read and its write, once for a chart being linked and once for one being unlinked. It checks for a 409, that nothing was restored, and that a retry restores the version. A third case checks that an unrelated foreign key failure still returns 422.
- `pytest tests/unit_tests/versioning/test_db_errors.py -k foreign_key` and `pytest tests/unit_tests/versioning/test_restore.py -k chart_purge_race`
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45551:
URL: https://github.com/apache/superset/pull/45551#issuecomment-6304655433
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 3b6867a7e96b3ce949c71395fab73a1cb4eb54da |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ad801f81da0a743d55c78af |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-45551--superset-docs-preview.netlify.app](https://deploy-preview-45551--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLDoAgDADRu3QtCah84g08RqWgCxVScEGMd5fdTPJeyJyuXGEBGGBjvP3Ru3ihRme0FRxKTRxEfngP_Sq3DinkM7WVOjVITqroFKFEO0-ktbcOI3w_) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45551--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #45551:
URL: https://github.com/apache/superset/pull/45551#discussion_r4351254031
##########
superset/commands/dashboard/restore_version.py:
##########
@@ -43,3 +53,35 @@ class RestoreDashboardVersionCommand(BaseRestoreVersionCommand):
not_found_exc = DashboardNotFoundError
forbidden_exc = DashboardForbiddenError
failed_exc = DashboardUpdateFailedError
+
+ def run(self) -> RestoreResult:
+ """Restore, confirming a reported chart-purge race before surfacing it.
+
+ The transaction has rolled back by the time the race reaches here,
+ so a fresh read sees the purge's commit. Only when one of the charts
+ whose membership the restore was changing is really gone is the
+ retryable :class:`ChartPurgedDuringRestoreError` raised. Otherwise,
+ or when the confirming read itself fails, the restore fails the
+ ordinary way.
+ """
+ try:
+ return super().run()
+ except ChartPurgedDuringRestoreError as ex:
+ slices: sa.Table = Slice.__table__
+ try:
+ remaining: set[int] = set(
+ db.session.execute(
+ sa.select(slices.c.id).where(slices.c.id.in_(ex.chart_ids))
Review Comment:
Thanks for flagging the identity edge. It's possible with SQLite's reused maximum ROWID, or with explicitly reused IDs on PostgreSQL/MySQL. Normal sequence and auto-increment allocation on PostgreSQL and InnoDB doesn't recycle deleted IDs. The check at `restore_version.py:74-80` runs after rollback, so even REPEATABLE READ doesn't hide a committed replacement: the user gets a 422 ("Dashboard could not be updated"), and that request changes nothing.
I'm keeping the conservative fallback here. Changing only this check to a 409 would promise a safe retry while the retry still resolves membership by integer ID, so UUID-aware membership reconstruction belongs in a separate follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #45551:
URL: https://github.com/apache/superset/pull/45551#issuecomment-6373783982
_AI-assisted reply on Mike's behalf._
Thanks, Matt. The comment and coverage nits are addressed:
- The 409 comment covers both linking and unlinking and says that purge removes the chart's membership history, so the retry does not include it in `skipped_slice_ids`.
- Added a test using a real failed SQLAlchemy flush against in-memory SQLite. It verifies that the entity is expired and that the log preserves the original `IntegrityError`, entity ID and transaction ID. Changing the logger back to `entity.id` makes the test fail with `PendingRollbackError`.
- The unrelated-FK case's docstring explicitly calls the injected error simulated.
The focused suite passes all 104 tests, and changed-file hooks pass. There are no executable production changes in this follow-up. I left ID reuse unchanged, as you suggested. I also deferred the optional uncommitted-purge interleaving; that needs a separate PostgreSQL/MySQL concurrency test and is not covered by this addition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45551:
URL: https://github.com/apache/superset/pull/45551#issuecomment-6328509975
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge; the restore rolls back on a detected chart-purge race and offers a retry.
- A rare chart ID reuse between purge and confirmation could turn the retryable conflict into an ordinary failure, but would not restore partial changes.
**Assessed commit:** `39a5d54e20bf`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45551:
URL: https://github.com/apache/superset/pull/45551#issuecomment-6304971362
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45551?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `97.29730%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 83.97%. Comparing base ([`6dc0641`](https://app.codecov.io/gh/apache/superset/commit/6dc0641c91a7ae2d696d133041d807266a5e1fc8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`39a5d54`](https://app.codecov.io/gh/apache/superset/commit/39a5d54e20bf8ddee3d64a790abb4bc0fb7a85ef?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45551?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/dashboard/restore\_version.py](https://app.codecov.io/gh/apache/superset/pull/45551?src=pr&el=tree&filepath=superset%2Fcommands%2Fdashboard%2Frestore_version.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGFzaGJvYXJkL3Jlc3RvcmVfdmVyc2lvbi5weQ==) | 90.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45551?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45551 +/- ##
==========================================
- Coverage 83.98% 83.97% -0.02%
==========================================
Files 3055 3055
Lines 201017 201080 +63
Branches 46949 46958 +9
==========================================
+ Hits 168830 168855 +25
- Misses 28942 28970 +28
- Partials 3245 3255 +10
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45551/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45551/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.88% <17.56%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/45551/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/45551/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.88% <74.32%> (+0.02%)` | :arrow_up: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45551/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.64% <17.56%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45551/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.17% <97.29%> (-0.03%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/45551/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/45551/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.40% <82.43%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45551?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45551:
URL: https://github.com/apache/superset/pull/45551#discussion_r4351257786
##########
superset/commands/dashboard/restore_version.py:
##########
@@ -43,3 +53,35 @@ class RestoreDashboardVersionCommand(BaseRestoreVersionCommand):
not_found_exc = DashboardNotFoundError
forbidden_exc = DashboardForbiddenError
failed_exc = DashboardUpdateFailedError
+
+ def run(self) -> RestoreResult:
+ """Restore, confirming a reported chart-purge race before surfacing it.
+
+ The transaction has rolled back by the time the race reaches here,
+ so a fresh read sees the purge's commit. Only when one of the charts
+ whose membership the restore was changing is really gone is the
+ retryable :class:`ChartPurgedDuringRestoreError` raised. Otherwise,
+ or when the confirming read itself fails, the restore fails the
+ ordinary way.
+ """
+ try:
+ return super().run()
+ except ChartPurgedDuringRestoreError as ex:
+ slices: sa.Table = Slice.__table__
+ try:
+ remaining: set[int] = set(
+ db.session.execute(
+ sa.select(slices.c.id).where(slices.c.id.in_(ex.chart_ids))
Review Comment:
โ
**Customized review instruction saved!**
**Instruction:**
> Do not recommend changing the post-rollback chart-ID check to return a retryable 409 unless membership reconstruction is UUID-aware; integer-ID lookup can resolve a replacement chart.
**Applied to:**
- `superset/commands/dashboard/restore_version.py`
---
๐ก *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45551:
URL: https://github.com/apache/superset/pull/45551#discussion_r4342873593
##########
superset/commands/dashboard/restore_version.py:
##########
@@ -43,3 +53,35 @@ class RestoreDashboardVersionCommand(BaseRestoreVersionCommand):
not_found_exc = DashboardNotFoundError
forbidden_exc = DashboardForbiddenError
failed_exc = DashboardUpdateFailedError
+
+ def run(self) -> RestoreResult:
+ """Restore, confirming a reported chart-purge race before surfacing it.
+
+ The transaction has rolled back by the time the race reaches here,
+ so a fresh read sees the purge's commit. Only when one of the charts
+ whose membership the restore was changing is really gone is the
+ retryable :class:`ChartPurgedDuringRestoreError` raised. Otherwise,
+ or when the confirming read itself fails, the restore fails the
+ ordinary way.
+ """
+ try:
+ return super().run()
+ except ChartPurgedDuringRestoreError as ex:
+ slices: sa.Table = Slice.__table__
+ try:
+ remaining: set[int] = set(
+ db.session.execute(
+ sa.select(slices.c.id).where(slices.c.id.in_(ex.chart_ids))
Review Comment:
**Suggestion:** If a chart id is reused after purge, this treats the replacement chart as the original, returning 422 instead of the retryable 409.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Incorrect condition logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=be4a2c702062408f9cae27919f65d94c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=be4a2c702062408f9cae27919f65d94c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dashboard/restore_version.py
**Line:** 74:74
**Comment:**
*Incorrect Condition Logic: If a chart id is reused after purge, this treats the replacement chart as the original, returning 422 instead of the retryable 409.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45551&comment_hash=c6bca6e3b4d1ef6307bd9f03100044cfc4be014a91bd182a790fb1347647a3a0&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45551&comment_hash=c6bca6e3b4d1ef6307bd9f03100044cfc4be014a91bd182a790fb1347647a3a0&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): retryable error when a dashboard restore races a chart purge [superset]
Posted by "rebenitez1802 (via GitHub)" <gi...@apache.org>.
rebenitez1802 merged PR #45551:
URL: https://github.com/apache/superset/pull/45551
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org