Skip to content

Fix BaseSerialization double-wrapping args for builtin exceptions - #69752

Closed
bujjibabukatta wants to merge 1 commit into
apache:mainfrom
bujjibabukatta:fix/#69743
Closed

bujjibabukatta wants to merge 1 commit into
apache:mainfrom
bujjibabukatta:fix/#69743

Conversation

@bujjibabukatta

@bujjibabukatta bujjibabukatta commented Jul 11, 2026 •

Copy link
Copy Markdown
Contributor

Ran into this from #69743 — BaseSerialization was mangling args for
built-in exceptions like KeyError on the round trip.

Turns out the exception-serialization branch for KeyError/AttributeError
was storing args as [var.args]. Since var.args is already a tuple
(e.g. ("boom",)), that wraps it in an extra layer. When deserializing,
exc_cls(*args) unpacks that as a single positional arg — so you get
KeyError(("boom",)) instead of KeyError("boom"), and .args comes back
as (("boom",),) instead of ("boom",).

Fix is a one-liner: store list(var.args) instead of [var.args] so it
stays flat.

Added a parametrized test covering single-arg, multi-arg, and no-arg
KeyError/AttributeError round-trips so this doesn't regress.

closes: #69743

AI Disclosure

  • This contribution used AI assistance.

Model(s) used: Codex (GPT-5.5) Following by the guidelines

AI was used for:

  • Drafting the PR description.
  • Verifying test cases locally.
  • Reviewing the implementation from a performance perspective.

The implementation and code changes were developed and validated by me.

@aaron-y-chen

Copy link
Copy Markdown
Contributor

Hi @bujjibabukatta, thanks for the contribution! I'm a bit concerned this might introduce a regression. Could you please double-check to ensure this change won't break existing behavior? Thanks!

@bujjibabukatta

Copy link
Copy Markdown
Contributor Author

Hi @nailo2c
Thanks for the review!
I actually went and traced through it before replying instead of just saying "should be fine." Deserialize is untouched, only serialize changed, so anything already sitting in the db from old runs deserializes exactly the same as before, no breakage there. Also checked where this actually gets used (trigger failure handling, xcom etc) and nothing depends on the old broken args shape. The only real difference is the error message itself is correct now instead of showing up wrapped in a weird extra tuple, which was the whole bug. Threw in a newsfragment too so it shows up in release notes. Happy to add more tests if you've got a specific case in mind.

@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jul 18, 2026

@shahar1 shahar1 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.

@shahar1 shahar1 added pending-response and removed ready for maintainer review Set after triaging when all criteria pass. labels Jul 25, 2026
@bujjibabukatta

Copy link
Copy Markdown
Contributor Author

Hi @shahar1 , I've updated the AI disclosure section as requested and addressed the concern mentioned in #70264.

Please let me know if there's anything else I need to update from my side for this PR to be ready for maintainer review again.

Thanks!

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because the author has not responded to a request for more information. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 8, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been closed because the author has not responded to a request for more information.

@github-actions github-actions Bot closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dag-processor pending-response stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BaseSerialization adds an extra tuple layer to built-in exception args

4 participants