Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task - #68539

Closed
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main
Closed

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task#68539
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main

Conversation

@Shushankranjan

Copy link
Copy Markdown

When the Execution API rejects a variable write (any non-2xx response —
e.g. a 403 authorization denial or a server error), Variable.set() and
Variable.delete() were catching AirflowRuntimeError and only logging
it. The task still finished as success and the variable was left
unchanged.

This is inconsistent with Variable.get(), which already raises on error
(and since #66575 even refuses secrets-backend fallback on a 401/403).
Reads fail loudly on denial; writes silently succeeded.

Changes

  • Variable.set(): Remove the try/except AirflowRuntimeError block
    that swallowed the error. Any API rejection now propagates and fails the
    task.
  • Variable.delete(): Same fix.
  • Remove the now-unused logging import and log variable from
    variable.py.
  • Add test_var_set_raises_on_error and test_var_delete_raises_on_error
    to cover the error propagation path.

Root cause

# Before (broken)try:
_set_variable(...)
exceptAirflowRuntimeErrorase:
log.exception(e) # swallowed — task reports success# After (fixed)_set_variable(...) # AirflowRuntimeError propagates — task fails

closes: #68537
Related: #66575 (fixed the analogous read / secrets-backend path)

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@onlyarnavonlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain the removal of logging and AirflowRuntimeError workflow from variable.py?

@Shushankranjan

Shushankranjan commented Jun 15, 2026

Copy link
Copy Markdown
Author

What Changed in variable.py

The change (from your previous conversation) was specifically in Variable.set() and Variable.delete(). Here's the before/after:

Variable.set() - Before

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
importloggingfromairflow.sdk.execution_time.contextimport_set_variabletry:
_set_variable(key, value, description, serialize_json=serialize_json)
exceptAirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently

Variable.set() - After (current)

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
fromairflow.sdk.execution_time.contextimport_set_variable_set_variable(key, value, description, serialize_json=serialize_json)

The same pattern applied to Variable.delete().


Why the Change Was Made

1. Silent Failure is Dangerous

The old try/exceptcaught and suppressedAirflowRuntimeError from the Execution API. If the API server rejected a set() or delete() (e.g., due to permissions, network issues, or server errors), the task would:

  • Log a warning
  • Continue executing as if the write succeeded

This is a correctness bug: downstream tasks could read stale variable values or operate on data that was never actually written.

2. Inconsistency with Variable.get()

Variable.get() already let AirflowRuntimeError propagate (with one deliberate exception: VARIABLE_NOT_FOUND + a provided default). The old set()/delete() had no such intentional swallowing - it was just a side effect of defensive coding that went too far.

3. The try/except Was an Overcorrection

There's no legitimate reason to swallow a write error. The correct behavior is:

  • Write fails → task fails → Airflow marks the task instance as failed → the user investigates

This matches Airflow's fault model: tasks are expected to be retried or alarmed on, not silently succeed.


What the Tests Verify Now

The two new tests confirm the corrected behavior:

TestWhat it asserts
test_var_set_raises_on_errorVariable.set() re-raises AirflowRuntimeError when the API rejects the write
test_var_delete_raises_on_errorVariable.delete() re-raises AirflowRuntimeError when the API rejects the delete

The logging import was also removed as a cleanup - it was only there to service the now-gone except block.


Summary: The try/except + logging in set()/delete() was silently eating API errors, letting tasks proceed as if writes succeeded when they hadn't. Removing it makes write failures immediately visible as task failures, consistent with how get() already behaves.

@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdkready for maintainer reviewSet after triaging when all criteria pass.staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task SDK: Variable.set/delete swallow API errors — a rejected write does not fail the task

3 participants

@Shushankranjan@onlyarnav@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task - #68539

Closed
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main
Closed

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task#68539
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main

Conversation

@Shushankranjan

Copy link
Copy Markdown

When the Execution API rejects a variable write (any non-2xx response —
e.g. a 403 authorization denial or a server error), Variable.set() and
Variable.delete() were catching AirflowRuntimeError and only logging
it. The task still finished as success and the variable was left
unchanged.

This is inconsistent with Variable.get(), which already raises on error
(and since #66575 even refuses secrets-backend fallback on a 401/403).
Reads fail loudly on denial; writes silently succeeded.

Changes

  • Variable.set(): Remove the try/except AirflowRuntimeError block
    that swallowed the error. Any API rejection now propagates and fails the
    task.
  • Variable.delete(): Same fix.
  • Remove the now-unused logging import and log variable from
    variable.py.
  • Add test_var_set_raises_on_error and test_var_delete_raises_on_error
    to cover the error propagation path.

Root cause

# Before (broken)try:
_set_variable(...)
exceptAirflowRuntimeErrorase:
log.exception(e) # swallowed — task reports success# After (fixed)_set_variable(...) # AirflowRuntimeError propagates — task fails

closes: #68537
Related: #66575 (fixed the analogous read / secrets-backend path)

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@onlyarnavonlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain the removal of logging and AirflowRuntimeError workflow from variable.py?

@Shushankranjan

Shushankranjan commented Jun 15, 2026

Copy link
Copy Markdown
Author

What Changed in variable.py

The change (from your previous conversation) was specifically in Variable.set() and Variable.delete(). Here's the before/after:

Variable.set() - Before

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
importloggingfromairflow.sdk.execution_time.contextimport_set_variabletry:
_set_variable(key, value, description, serialize_json=serialize_json)
exceptAirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently

Variable.set() - After (current)

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
fromairflow.sdk.execution_time.contextimport_set_variable_set_variable(key, value, description, serialize_json=serialize_json)

The same pattern applied to Variable.delete().


Why the Change Was Made

1. Silent Failure is Dangerous

The old try/exceptcaught and suppressedAirflowRuntimeError from the Execution API. If the API server rejected a set() or delete() (e.g., due to permissions, network issues, or server errors), the task would:

  • Log a warning
  • Continue executing as if the write succeeded

This is a correctness bug: downstream tasks could read stale variable values or operate on data that was never actually written.

2. Inconsistency with Variable.get()

Variable.get() already let AirflowRuntimeError propagate (with one deliberate exception: VARIABLE_NOT_FOUND + a provided default). The old set()/delete() had no such intentional swallowing - it was just a side effect of defensive coding that went too far.

3. The try/except Was an Overcorrection

There's no legitimate reason to swallow a write error. The correct behavior is:

  • Write fails → task fails → Airflow marks the task instance as failed → the user investigates

This matches Airflow's fault model: tasks are expected to be retried or alarmed on, not silently succeed.


What the Tests Verify Now

The two new tests confirm the corrected behavior:

TestWhat it asserts
test_var_set_raises_on_errorVariable.set() re-raises AirflowRuntimeError when the API rejects the write
test_var_delete_raises_on_errorVariable.delete() re-raises AirflowRuntimeError when the API rejects the delete

The logging import was also removed as a cleanup - it was only there to service the now-gone except block.


Summary: The try/except + logging in set()/delete() was silently eating API errors, letting tasks proceed as if writes succeeded when they hadn't. Removing it makes write failures immediately visible as task failures, consistent with how get() already behaves.

@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdkready for maintainer reviewSet after triaging when all criteria pass.staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task SDK: Variable.set/delete swallow API errors — a rejected write does not fail the task

3 participants

@Shushankranjan@onlyarnav@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task - #68539

Closed
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main
Closed

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task#68539
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main

Conversation

@Shushankranjan

Copy link
Copy Markdown

When the Execution API rejects a variable write (any non-2xx response —
e.g. a 403 authorization denial or a server error), Variable.set() and
Variable.delete() were catching AirflowRuntimeError and only logging
it. The task still finished as success and the variable was left
unchanged.

This is inconsistent with Variable.get(), which already raises on error
(and since #66575 even refuses secrets-backend fallback on a 401/403).
Reads fail loudly on denial; writes silently succeeded.

Changes

  • Variable.set(): Remove the try/except AirflowRuntimeError block
    that swallowed the error. Any API rejection now propagates and fails the
    task.
  • Variable.delete(): Same fix.
  • Remove the now-unused logging import and log variable from
    variable.py.
  • Add test_var_set_raises_on_error and test_var_delete_raises_on_error
    to cover the error propagation path.

Root cause

# Before (broken)try:
_set_variable(...)
exceptAirflowRuntimeErrorase:
log.exception(e) # swallowed — task reports success# After (fixed)_set_variable(...) # AirflowRuntimeError propagates — task fails

closes: #68537
Related: #66575 (fixed the analogous read / secrets-backend path)

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@onlyarnavonlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain the removal of logging and AirflowRuntimeError workflow from variable.py?

@Shushankranjan

Shushankranjan commented Jun 15, 2026

Copy link
Copy Markdown
Author

What Changed in variable.py

The change (from your previous conversation) was specifically in Variable.set() and Variable.delete(). Here's the before/after:

Variable.set() - Before

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
importloggingfromairflow.sdk.execution_time.contextimport_set_variabletry:
_set_variable(key, value, description, serialize_json=serialize_json)
exceptAirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently

Variable.set() - After (current)

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
fromairflow.sdk.execution_time.contextimport_set_variable_set_variable(key, value, description, serialize_json=serialize_json)

The same pattern applied to Variable.delete().


Why the Change Was Made

1. Silent Failure is Dangerous

The old try/exceptcaught and suppressedAirflowRuntimeError from the Execution API. If the API server rejected a set() or delete() (e.g., due to permissions, network issues, or server errors), the task would:

  • Log a warning
  • Continue executing as if the write succeeded

This is a correctness bug: downstream tasks could read stale variable values or operate on data that was never actually written.

2. Inconsistency with Variable.get()

Variable.get() already let AirflowRuntimeError propagate (with one deliberate exception: VARIABLE_NOT_FOUND + a provided default). The old set()/delete() had no such intentional swallowing - it was just a side effect of defensive coding that went too far.

3. The try/except Was an Overcorrection

There's no legitimate reason to swallow a write error. The correct behavior is:

  • Write fails → task fails → Airflow marks the task instance as failed → the user investigates

This matches Airflow's fault model: tasks are expected to be retried or alarmed on, not silently succeed.


What the Tests Verify Now

The two new tests confirm the corrected behavior:

TestWhat it asserts
test_var_set_raises_on_errorVariable.set() re-raises AirflowRuntimeError when the API rejects the write
test_var_delete_raises_on_errorVariable.delete() re-raises AirflowRuntimeError when the API rejects the delete

The logging import was also removed as a cleanup - it was only there to service the now-gone except block.


Summary: The try/except + logging in set()/delete() was silently eating API errors, letting tasks proceed as if writes succeeded when they hadn't. Removing it makes write failures immediately visible as task failures, consistent with how get() already behaves.

@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdkready for maintainer reviewSet after triaging when all criteria pass.staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task SDK: Variable.set/delete swallow API errors — a rejected write does not fail the task

3 participants

@Shushankranjan@onlyarnav@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task - #68539

Closed
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main
Closed

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task#68539
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main

Conversation

@Shushankranjan

Copy link
Copy Markdown

When the Execution API rejects a variable write (any non-2xx response —
e.g. a 403 authorization denial or a server error), Variable.set() and
Variable.delete() were catching AirflowRuntimeError and only logging
it. The task still finished as success and the variable was left
unchanged.

This is inconsistent with Variable.get(), which already raises on error
(and since #66575 even refuses secrets-backend fallback on a 401/403).
Reads fail loudly on denial; writes silently succeeded.

Changes

  • Variable.set(): Remove the try/except AirflowRuntimeError block
    that swallowed the error. Any API rejection now propagates and fails the
    task.
  • Variable.delete(): Same fix.
  • Remove the now-unused logging import and log variable from
    variable.py.
  • Add test_var_set_raises_on_error and test_var_delete_raises_on_error
    to cover the error propagation path.

Root cause

# Before (broken)try:
_set_variable(...)
exceptAirflowRuntimeErrorase:
log.exception(e) # swallowed — task reports success# After (fixed)_set_variable(...) # AirflowRuntimeError propagates — task fails

closes: #68537
Related: #66575 (fixed the analogous read / secrets-backend path)

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@onlyarnavonlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain the removal of logging and AirflowRuntimeError workflow from variable.py?

@Shushankranjan

Shushankranjan commented Jun 15, 2026

Copy link
Copy Markdown
Author

What Changed in variable.py

The change (from your previous conversation) was specifically in Variable.set() and Variable.delete(). Here's the before/after:

Variable.set() - Before

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
importloggingfromairflow.sdk.execution_time.contextimport_set_variabletry:
_set_variable(key, value, description, serialize_json=serialize_json)
exceptAirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently

Variable.set() - After (current)

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
fromairflow.sdk.execution_time.contextimport_set_variable_set_variable(key, value, description, serialize_json=serialize_json)

The same pattern applied to Variable.delete().


Why the Change Was Made

1. Silent Failure is Dangerous

The old try/exceptcaught and suppressedAirflowRuntimeError from the Execution API. If the API server rejected a set() or delete() (e.g., due to permissions, network issues, or server errors), the task would:

  • Log a warning
  • Continue executing as if the write succeeded

This is a correctness bug: downstream tasks could read stale variable values or operate on data that was never actually written.

2. Inconsistency with Variable.get()

Variable.get() already let AirflowRuntimeError propagate (with one deliberate exception: VARIABLE_NOT_FOUND + a provided default). The old set()/delete() had no such intentional swallowing - it was just a side effect of defensive coding that went too far.

3. The try/except Was an Overcorrection

There's no legitimate reason to swallow a write error. The correct behavior is:

  • Write fails → task fails → Airflow marks the task instance as failed → the user investigates

This matches Airflow's fault model: tasks are expected to be retried or alarmed on, not silently succeed.


What the Tests Verify Now

The two new tests confirm the corrected behavior:

TestWhat it asserts
test_var_set_raises_on_errorVariable.set() re-raises AirflowRuntimeError when the API rejects the write
test_var_delete_raises_on_errorVariable.delete() re-raises AirflowRuntimeError when the API rejects the delete

The logging import was also removed as a cleanup - it was only there to service the now-gone except block.


Summary: The try/except + logging in set()/delete() was silently eating API errors, letting tasks proceed as if writes succeeded when they hadn't. Removing it makes write failures immediately visible as task failures, consistent with how get() already behaves.

@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdkready for maintainer reviewSet after triaging when all criteria pass.staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task SDK: Variable.set/delete swallow API errors — a rejected write does not fail the task

3 participants

@Shushankranjan@onlyarnav@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task - #68539

Closed
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main
Closed

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task#68539
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main

Conversation

@Shushankranjan

Copy link
Copy Markdown

When the Execution API rejects a variable write (any non-2xx response —
e.g. a 403 authorization denial or a server error), Variable.set() and
Variable.delete() were catching AirflowRuntimeError and only logging
it. The task still finished as success and the variable was left
unchanged.

This is inconsistent with Variable.get(), which already raises on error
(and since #66575 even refuses secrets-backend fallback on a 401/403).
Reads fail loudly on denial; writes silently succeeded.

Changes

  • Variable.set(): Remove the try/except AirflowRuntimeError block
    that swallowed the error. Any API rejection now propagates and fails the
    task.
  • Variable.delete(): Same fix.
  • Remove the now-unused logging import and log variable from
    variable.py.
  • Add test_var_set_raises_on_error and test_var_delete_raises_on_error
    to cover the error propagation path.

Root cause

# Before (broken)try:
_set_variable(...)
exceptAirflowRuntimeErrorase:
log.exception(e) # swallowed — task reports success# After (fixed)_set_variable(...) # AirflowRuntimeError propagates — task fails

closes: #68537
Related: #66575 (fixed the analogous read / secrets-backend path)

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@onlyarnavonlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain the removal of logging and AirflowRuntimeError workflow from variable.py?

@Shushankranjan

Shushankranjan commented Jun 15, 2026

Copy link
Copy Markdown
Author

What Changed in variable.py

The change (from your previous conversation) was specifically in Variable.set() and Variable.delete(). Here's the before/after:

Variable.set() - Before

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
importloggingfromairflow.sdk.execution_time.contextimport_set_variabletry:
_set_variable(key, value, description, serialize_json=serialize_json)
exceptAirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently

Variable.set() - After (current)

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
fromairflow.sdk.execution_time.contextimport_set_variable_set_variable(key, value, description, serialize_json=serialize_json)

The same pattern applied to Variable.delete().


Why the Change Was Made

1. Silent Failure is Dangerous

The old try/exceptcaught and suppressedAirflowRuntimeError from the Execution API. If the API server rejected a set() or delete() (e.g., due to permissions, network issues, or server errors), the task would:

  • Log a warning
  • Continue executing as if the write succeeded

This is a correctness bug: downstream tasks could read stale variable values or operate on data that was never actually written.

2. Inconsistency with Variable.get()

Variable.get() already let AirflowRuntimeError propagate (with one deliberate exception: VARIABLE_NOT_FOUND + a provided default). The old set()/delete() had no such intentional swallowing - it was just a side effect of defensive coding that went too far.

3. The try/except Was an Overcorrection

There's no legitimate reason to swallow a write error. The correct behavior is:

  • Write fails → task fails → Airflow marks the task instance as failed → the user investigates

This matches Airflow's fault model: tasks are expected to be retried or alarmed on, not silently succeed.


What the Tests Verify Now

The two new tests confirm the corrected behavior:

TestWhat it asserts
test_var_set_raises_on_errorVariable.set() re-raises AirflowRuntimeError when the API rejects the write
test_var_delete_raises_on_errorVariable.delete() re-raises AirflowRuntimeError when the API rejects the delete

The logging import was also removed as a cleanup - it was only there to service the now-gone except block.


Summary: The try/except + logging in set()/delete() was silently eating API errors, letting tasks proceed as if writes succeeded when they hadn't. Removing it makes write failures immediately visible as task failures, consistent with how get() already behaves.

@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdkready for maintainer reviewSet after triaging when all criteria pass.staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task SDK: Variable.set/delete swallow API errors — a rejected write does not fail the task

3 participants

@Shushankranjan@onlyarnav@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task - #68539

Closed
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main
Closed

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task#68539
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main

Conversation

@Shushankranjan

Copy link
Copy Markdown

When the Execution API rejects a variable write (any non-2xx response —
e.g. a 403 authorization denial or a server error), Variable.set() and
Variable.delete() were catching AirflowRuntimeError and only logging
it. The task still finished as success and the variable was left
unchanged.

This is inconsistent with Variable.get(), which already raises on error
(and since #66575 even refuses secrets-backend fallback on a 401/403).
Reads fail loudly on denial; writes silently succeeded.

Changes

  • Variable.set(): Remove the try/except AirflowRuntimeError block
    that swallowed the error. Any API rejection now propagates and fails the
    task.
  • Variable.delete(): Same fix.
  • Remove the now-unused logging import and log variable from
    variable.py.
  • Add test_var_set_raises_on_error and test_var_delete_raises_on_error
    to cover the error propagation path.

Root cause

# Before (broken)try:
_set_variable(...)
exceptAirflowRuntimeErrorase:
log.exception(e) # swallowed — task reports success# After (fixed)_set_variable(...) # AirflowRuntimeError propagates — task fails

closes: #68537
Related: #66575 (fixed the analogous read / secrets-backend path)

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@onlyarnavonlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain the removal of logging and AirflowRuntimeError workflow from variable.py?

@Shushankranjan

Shushankranjan commented Jun 15, 2026

Copy link
Copy Markdown
Author

What Changed in variable.py

The change (from your previous conversation) was specifically in Variable.set() and Variable.delete(). Here's the before/after:

Variable.set() - Before

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
importloggingfromairflow.sdk.execution_time.contextimport_set_variabletry:
_set_variable(key, value, description, serialize_json=serialize_json)
exceptAirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently

Variable.set() - After (current)

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
fromairflow.sdk.execution_time.contextimport_set_variable_set_variable(key, value, description, serialize_json=serialize_json)

The same pattern applied to Variable.delete().


Why the Change Was Made

1. Silent Failure is Dangerous

The old try/exceptcaught and suppressedAirflowRuntimeError from the Execution API. If the API server rejected a set() or delete() (e.g., due to permissions, network issues, or server errors), the task would:

  • Log a warning
  • Continue executing as if the write succeeded

This is a correctness bug: downstream tasks could read stale variable values or operate on data that was never actually written.

2. Inconsistency with Variable.get()

Variable.get() already let AirflowRuntimeError propagate (with one deliberate exception: VARIABLE_NOT_FOUND + a provided default). The old set()/delete() had no such intentional swallowing - it was just a side effect of defensive coding that went too far.

3. The try/except Was an Overcorrection

There's no legitimate reason to swallow a write error. The correct behavior is:

  • Write fails → task fails → Airflow marks the task instance as failed → the user investigates

This matches Airflow's fault model: tasks are expected to be retried or alarmed on, not silently succeed.


What the Tests Verify Now

The two new tests confirm the corrected behavior:

TestWhat it asserts
test_var_set_raises_on_errorVariable.set() re-raises AirflowRuntimeError when the API rejects the write
test_var_delete_raises_on_errorVariable.delete() re-raises AirflowRuntimeError when the API rejects the delete

The logging import was also removed as a cleanup - it was only there to service the now-gone except block.


Summary: The try/except + logging in set()/delete() was silently eating API errors, letting tasks proceed as if writes succeeded when they hadn't. Removing it makes write failures immediately visible as task failures, consistent with how get() already behaves.

@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdkready for maintainer reviewSet after triaging when all criteria pass.staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task SDK: Variable.set/delete swallow API errors — a rejected write does not fail the task

3 participants

@Shushankranjan@onlyarnav@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task - #68539

Closed
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main
Closed

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task#68539
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main

Conversation

@Shushankranjan

Copy link
Copy Markdown

When the Execution API rejects a variable write (any non-2xx response —
e.g. a 403 authorization denial or a server error), Variable.set() and
Variable.delete() were catching AirflowRuntimeError and only logging
it. The task still finished as success and the variable was left
unchanged.

This is inconsistent with Variable.get(), which already raises on error
(and since #66575 even refuses secrets-backend fallback on a 401/403).
Reads fail loudly on denial; writes silently succeeded.

Changes

  • Variable.set(): Remove the try/except AirflowRuntimeError block
    that swallowed the error. Any API rejection now propagates and fails the
    task.
  • Variable.delete(): Same fix.
  • Remove the now-unused logging import and log variable from
    variable.py.
  • Add test_var_set_raises_on_error and test_var_delete_raises_on_error
    to cover the error propagation path.

Root cause

# Before (broken)try:
_set_variable(...)
exceptAirflowRuntimeErrorase:
log.exception(e) # swallowed — task reports success# After (fixed)_set_variable(...) # AirflowRuntimeError propagates — task fails

closes: #68537
Related: #66575 (fixed the analogous read / secrets-backend path)

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@onlyarnavonlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain the removal of logging and AirflowRuntimeError workflow from variable.py?

@Shushankranjan

Shushankranjan commented Jun 15, 2026

Copy link
Copy Markdown
Author

What Changed in variable.py

The change (from your previous conversation) was specifically in Variable.set() and Variable.delete(). Here's the before/after:

Variable.set() - Before

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
importloggingfromairflow.sdk.execution_time.contextimport_set_variabletry:
_set_variable(key, value, description, serialize_json=serialize_json)
exceptAirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently

Variable.set() - After (current)

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
fromairflow.sdk.execution_time.contextimport_set_variable_set_variable(key, value, description, serialize_json=serialize_json)

The same pattern applied to Variable.delete().


Why the Change Was Made

1. Silent Failure is Dangerous

The old try/exceptcaught and suppressedAirflowRuntimeError from the Execution API. If the API server rejected a set() or delete() (e.g., due to permissions, network issues, or server errors), the task would:

  • Log a warning
  • Continue executing as if the write succeeded

This is a correctness bug: downstream tasks could read stale variable values or operate on data that was never actually written.

2. Inconsistency with Variable.get()

Variable.get() already let AirflowRuntimeError propagate (with one deliberate exception: VARIABLE_NOT_FOUND + a provided default). The old set()/delete() had no such intentional swallowing - it was just a side effect of defensive coding that went too far.

3. The try/except Was an Overcorrection

There's no legitimate reason to swallow a write error. The correct behavior is:

  • Write fails → task fails → Airflow marks the task instance as failed → the user investigates

This matches Airflow's fault model: tasks are expected to be retried or alarmed on, not silently succeed.


What the Tests Verify Now

The two new tests confirm the corrected behavior:

TestWhat it asserts
test_var_set_raises_on_errorVariable.set() re-raises AirflowRuntimeError when the API rejects the write
test_var_delete_raises_on_errorVariable.delete() re-raises AirflowRuntimeError when the API rejects the delete

The logging import was also removed as a cleanup - it was only there to service the now-gone except block.


Summary: The try/except + logging in set()/delete() was silently eating API errors, letting tasks proceed as if writes succeeded when they hadn't. Removing it makes write failures immediately visible as task failures, consistent with how get() already behaves.

@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdkready for maintainer reviewSet after triaging when all criteria pass.staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task SDK: Variable.set/delete swallow API errors — a rejected write does not fail the task

3 participants

@Shushankranjan@onlyarnav@potiuk
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task - #68539

Closed
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main
Closed

Task SDK: Fix Variable.set/delete swallowing API errors — propagate AirflowRuntimeError to fail the task#68539
Shushankranjan wants to merge 1 commit into
apache:mainfrom
Shushankranjan:main

Conversation

@Shushankranjan

Copy link
Copy Markdown

When the Execution API rejects a variable write (any non-2xx response —
e.g. a 403 authorization denial or a server error), Variable.set() and
Variable.delete() were catching AirflowRuntimeError and only logging
it. The task still finished as success and the variable was left
unchanged.

This is inconsistent with Variable.get(), which already raises on error
(and since #66575 even refuses secrets-backend fallback on a 401/403).
Reads fail loudly on denial; writes silently succeeded.

Changes

  • Variable.set(): Remove the try/except AirflowRuntimeError block
    that swallowed the error. Any API rejection now propagates and fails the
    task.
  • Variable.delete(): Same fix.
  • Remove the now-unused logging import and log variable from
    variable.py.
  • Add test_var_set_raises_on_error and test_var_delete_raises_on_error
    to cover the error propagation path.

Root cause

# Before (broken)try:
_set_variable(...)
exceptAirflowRuntimeErrorase:
log.exception(e) # swallowed — task reports success# After (fixed)_set_variable(...) # AirflowRuntimeError propagates — task fails

closes: #68537
Related: #66575 (fixed the analogous read / secrets-backend path)

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@onlyarnavonlyarnav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain the removal of logging and AirflowRuntimeError workflow from variable.py?

@Shushankranjan

Shushankranjan commented Jun 15, 2026

Copy link
Copy Markdown
Author

What Changed in variable.py

The change (from your previous conversation) was specifically in Variable.set() and Variable.delete(). Here's the before/after:

Variable.set() - Before

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
importloggingfromairflow.sdk.execution_time.contextimport_set_variabletry:
_set_variable(key, value, description, serialize_json=serialize_json)
exceptAirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently

Variable.set() - After (current)

@classmethoddefset(cls, key, value, description=None, serialize_json=False) ->None:
fromairflow.sdk.execution_time.contextimport_set_variable_set_variable(key, value, description, serialize_json=serialize_json)

The same pattern applied to Variable.delete().


Why the Change Was Made

1. Silent Failure is Dangerous

The old try/exceptcaught and suppressedAirflowRuntimeError from the Execution API. If the API server rejected a set() or delete() (e.g., due to permissions, network issues, or server errors), the task would:

  • Log a warning
  • Continue executing as if the write succeeded

This is a correctness bug: downstream tasks could read stale variable values or operate on data that was never actually written.

2. Inconsistency with Variable.get()

Variable.get() already let AirflowRuntimeError propagate (with one deliberate exception: VARIABLE_NOT_FOUND + a provided default). The old set()/delete() had no such intentional swallowing - it was just a side effect of defensive coding that went too far.

3. The try/except Was an Overcorrection

There's no legitimate reason to swallow a write error. The correct behavior is:

  • Write fails → task fails → Airflow marks the task instance as failed → the user investigates

This matches Airflow's fault model: tasks are expected to be retried or alarmed on, not silently succeed.


What the Tests Verify Now

The two new tests confirm the corrected behavior:

TestWhat it asserts
test_var_set_raises_on_errorVariable.set() re-raises AirflowRuntimeError when the API rejects the write
test_var_delete_raises_on_errorVariable.delete() re-raises AirflowRuntimeError when the API rejects the delete

The logging import was also removed as a cleanup - it was only there to service the now-gone except block.


Summary: The try/except + logging in set()/delete() was silently eating API errors, letting tasks proceed as if writes succeeded when they hadn't. Removing it makes write failures immediately visible as task failures, consistent with how get() already behaves.

@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:task-sdkready for maintainer reviewSet after triaging when all criteria pass.staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task SDK: Variable.set/delete swallow API errors — a rejected write does not fail the task

3 participants

@Shushankranjan@onlyarnav@potiuk