Isolate Sentry instrumentation from task run - #67505

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run
Closed

Isolate Sentry instrumentation from task run#67505
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run

Conversation

@potiuk

Copy link
Copy Markdown
Member

In ConfiguredSentry.enrich_errors, prepare_to_enrich_errors, add_tagging and add_breadcrumbs ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped run(...). An exception raised by any of them propagated through the wrapper, so:

  • A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
  • The wrapped run never executed — instrumentation could effectively prevent the task from running.

Reported as F-006 in the apache/tooling-agents L3 task-sdk sweep 0920c77.

Change

Wrap each instrumentation step (prepare_to_enrich_errors, add_tagging, add_breadcrumbs) in its own try/except and log a structured warning on failure (sentry_prepare_failed, sentry_add_tagging_failed, sentry_add_breadcrumbs_failed). The wrapped run still runs inside sentry_sdk.new_scope and its exceptions are captured via sentry_sdk.capture_exception(e) and re-raised — that path is unchanged.

Test plan

  • test_enrich_errors_isolates_instrumentation_from_task_run (parametrised across all three instrumentation steps) — asserts (a) the wrapped run() still executes when each step raises, (b) the structured warning is emitted on the task log.
  • test_enrich_errors_captures_and_reraises_task_failure — asserts the wrapped-run failure path is unchanged: sentry_sdk.capture_exception is called, exception re-raised.
  • prek run ruff clean.
  • prek run mypy-task-sdk clean.
  • Full test_sentry.py suite: 10 passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Generated-by: Claude Code (Opus 4.7) following the guidelines

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

Can those methods ever realistically fail?

Has this ever happened that we know of?

Comment on lines +157 to +162
except Exception:
log.warning("sentry_add_tagging_failed", exc_info=True)
try:
self.add_breadcrumbs(ti)
except Exception:
log.warning("sentry_add_breadcrumbs_failed", exc_info=True)

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.

A try/except for each is overkill.

Doubly so as these are all local operations. I don't think these can "ever" fail

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 7b93c6b to ab8fbb6CompareJune 5, 2026 01:10
@potiuk

Copy link
Copy Markdown
MemberAuthor

Good call on the granularity — collapsed it. prepare_to_enrich_errors stays in its own guard (it runs sentry_sdk.init()), and add_tagging + add_breadcrumbs now share a single try/except (one sentry_enrichment_failed warning) instead of one each.

On "can these ever fail / all local operations" though — two of the three aren't local:

  • add_breadcrumbs isn't local at all: it calls RuntimeTaskInstance.get_task_breadcrumbs(...), which does SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...)) — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
  • prepare_to_enrich_errors runs sentry_sdk.init(dsn=…) plus conf.getimport("sentry", "before_send"/"transport") — SDK/transport init and importing a configured callable can fail on misconfiguration.
  • add_tagging is the one that's genuinely local (scope.set_tag + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.

So this isn't purely defensive against the impossible — add_breadcrumbs in particular is a remote call we don't want taking down a task. Mind another look?


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ab8fbb6 to ca897a8CompareJune 5, 2026 14:03
Comment on lines +303 to +306
sentry_sdk_module = sys.modules["sentry_sdk"]
sentry_sdk_module.new_scope = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)

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.

  1. This leaks fixtures outside of this test.
  2. This already exist as mock_sentry_sdk fixture and is used in the test right above this one.

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ca897a8 to ac9dd53CompareJune 11, 2026 01:28
@potiuk
potiuk marked this pull request as draft June 11, 2026 01:36
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ac9dd53 to 2fd2f54CompareJune 11, 2026 01:37
In ``ConfiguredSentry.enrich_errors``, ``prepare_to_enrich_errors``,
``add_tagging`` and ``add_breadcrumbs`` ran inside (or alongside) the
same try-block as the wrapped ``run(...)``. An exception raised by any
of them propagated through the wrapper, so a failure in non-critical
Sentry instrumentation looked indistinguishable from a real task
failure and the wrapped ``run`` never executed.
Wrap each instrumentation step in its own ``try/except`` and log a
structured warning on failure. The wrapped ``run`` still runs inside
``sentry_sdk.new_scope`` and its exceptions are captured and
re-raised — that part is unchanged.
@potiuk
potiuk marked this pull request as ready for review June 11, 2026 17:33
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 2fd2f54 to 4822f54CompareJune 11, 2026 17:33
@potiuk

Copy link
Copy Markdown
MemberAuthor

Fair question — I don't have a production occurrence. This is defensive: add_tagging/add_breadcrumbs call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped run() from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.

@potiuk

Copy link
Copy Markdown
MemberAuthor

Closing then.

@potiukpotiuk closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@potiuk@ashb
, '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

Isolate Sentry instrumentation from task run - #67505

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run
Closed

Isolate Sentry instrumentation from task run#67505
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run

Conversation

@potiuk

Copy link
Copy Markdown
Member

In ConfiguredSentry.enrich_errors, prepare_to_enrich_errors, add_tagging and add_breadcrumbs ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped run(...). An exception raised by any of them propagated through the wrapper, so:

  • A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
  • The wrapped run never executed — instrumentation could effectively prevent the task from running.

Reported as F-006 in the apache/tooling-agents L3 task-sdk sweep 0920c77.

Change

Wrap each instrumentation step (prepare_to_enrich_errors, add_tagging, add_breadcrumbs) in its own try/except and log a structured warning on failure (sentry_prepare_failed, sentry_add_tagging_failed, sentry_add_breadcrumbs_failed). The wrapped run still runs inside sentry_sdk.new_scope and its exceptions are captured via sentry_sdk.capture_exception(e) and re-raised — that path is unchanged.

Test plan

  • test_enrich_errors_isolates_instrumentation_from_task_run (parametrised across all three instrumentation steps) — asserts (a) the wrapped run() still executes when each step raises, (b) the structured warning is emitted on the task log.
  • test_enrich_errors_captures_and_reraises_task_failure — asserts the wrapped-run failure path is unchanged: sentry_sdk.capture_exception is called, exception re-raised.
  • prek run ruff clean.
  • prek run mypy-task-sdk clean.
  • Full test_sentry.py suite: 10 passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Generated-by: Claude Code (Opus 4.7) following the guidelines

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

Can those methods ever realistically fail?

Has this ever happened that we know of?

Comment on lines +157 to +162
except Exception:
log.warning("sentry_add_tagging_failed", exc_info=True)
try:
self.add_breadcrumbs(ti)
except Exception:
log.warning("sentry_add_breadcrumbs_failed", exc_info=True)

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.

A try/except for each is overkill.

Doubly so as these are all local operations. I don't think these can "ever" fail

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 7b93c6b to ab8fbb6CompareJune 5, 2026 01:10
@potiuk

Copy link
Copy Markdown
MemberAuthor

Good call on the granularity — collapsed it. prepare_to_enrich_errors stays in its own guard (it runs sentry_sdk.init()), and add_tagging + add_breadcrumbs now share a single try/except (one sentry_enrichment_failed warning) instead of one each.

On "can these ever fail / all local operations" though — two of the three aren't local:

  • add_breadcrumbs isn't local at all: it calls RuntimeTaskInstance.get_task_breadcrumbs(...), which does SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...)) — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
  • prepare_to_enrich_errors runs sentry_sdk.init(dsn=…) plus conf.getimport("sentry", "before_send"/"transport") — SDK/transport init and importing a configured callable can fail on misconfiguration.
  • add_tagging is the one that's genuinely local (scope.set_tag + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.

So this isn't purely defensive against the impossible — add_breadcrumbs in particular is a remote call we don't want taking down a task. Mind another look?


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ab8fbb6 to ca897a8CompareJune 5, 2026 14:03
Comment on lines +303 to +306
sentry_sdk_module = sys.modules["sentry_sdk"]
sentry_sdk_module.new_scope = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)

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.

  1. This leaks fixtures outside of this test.
  2. This already exist as mock_sentry_sdk fixture and is used in the test right above this one.

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ca897a8 to ac9dd53CompareJune 11, 2026 01:28
@potiuk
potiuk marked this pull request as draft June 11, 2026 01:36
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ac9dd53 to 2fd2f54CompareJune 11, 2026 01:37
In ``ConfiguredSentry.enrich_errors``, ``prepare_to_enrich_errors``,
``add_tagging`` and ``add_breadcrumbs`` ran inside (or alongside) the
same try-block as the wrapped ``run(...)``. An exception raised by any
of them propagated through the wrapper, so a failure in non-critical
Sentry instrumentation looked indistinguishable from a real task
failure and the wrapped ``run`` never executed.
Wrap each instrumentation step in its own ``try/except`` and log a
structured warning on failure. The wrapped ``run`` still runs inside
``sentry_sdk.new_scope`` and its exceptions are captured and
re-raised — that part is unchanged.
@potiuk
potiuk marked this pull request as ready for review June 11, 2026 17:33
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 2fd2f54 to 4822f54CompareJune 11, 2026 17:33
@potiuk

Copy link
Copy Markdown
MemberAuthor

Fair question — I don't have a production occurrence. This is defensive: add_tagging/add_breadcrumbs call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped run() from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.

@potiuk

Copy link
Copy Markdown
MemberAuthor

Closing then.

@potiukpotiuk closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@potiuk@ashb
, '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

Isolate Sentry instrumentation from task run - #67505

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run
Closed

Isolate Sentry instrumentation from task run#67505
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run

Conversation

@potiuk

Copy link
Copy Markdown
Member

In ConfiguredSentry.enrich_errors, prepare_to_enrich_errors, add_tagging and add_breadcrumbs ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped run(...). An exception raised by any of them propagated through the wrapper, so:

  • A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
  • The wrapped run never executed — instrumentation could effectively prevent the task from running.

Reported as F-006 in the apache/tooling-agents L3 task-sdk sweep 0920c77.

Change

Wrap each instrumentation step (prepare_to_enrich_errors, add_tagging, add_breadcrumbs) in its own try/except and log a structured warning on failure (sentry_prepare_failed, sentry_add_tagging_failed, sentry_add_breadcrumbs_failed). The wrapped run still runs inside sentry_sdk.new_scope and its exceptions are captured via sentry_sdk.capture_exception(e) and re-raised — that path is unchanged.

Test plan

  • test_enrich_errors_isolates_instrumentation_from_task_run (parametrised across all three instrumentation steps) — asserts (a) the wrapped run() still executes when each step raises, (b) the structured warning is emitted on the task log.
  • test_enrich_errors_captures_and_reraises_task_failure — asserts the wrapped-run failure path is unchanged: sentry_sdk.capture_exception is called, exception re-raised.
  • prek run ruff clean.
  • prek run mypy-task-sdk clean.
  • Full test_sentry.py suite: 10 passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Generated-by: Claude Code (Opus 4.7) following the guidelines

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

Can those methods ever realistically fail?

Has this ever happened that we know of?

Comment on lines +157 to +162
except Exception:
log.warning("sentry_add_tagging_failed", exc_info=True)
try:
self.add_breadcrumbs(ti)
except Exception:
log.warning("sentry_add_breadcrumbs_failed", exc_info=True)

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.

A try/except for each is overkill.

Doubly so as these are all local operations. I don't think these can "ever" fail

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 7b93c6b to ab8fbb6CompareJune 5, 2026 01:10
@potiuk

Copy link
Copy Markdown
MemberAuthor

Good call on the granularity — collapsed it. prepare_to_enrich_errors stays in its own guard (it runs sentry_sdk.init()), and add_tagging + add_breadcrumbs now share a single try/except (one sentry_enrichment_failed warning) instead of one each.

On "can these ever fail / all local operations" though — two of the three aren't local:

  • add_breadcrumbs isn't local at all: it calls RuntimeTaskInstance.get_task_breadcrumbs(...), which does SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...)) — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
  • prepare_to_enrich_errors runs sentry_sdk.init(dsn=…) plus conf.getimport("sentry", "before_send"/"transport") — SDK/transport init and importing a configured callable can fail on misconfiguration.
  • add_tagging is the one that's genuinely local (scope.set_tag + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.

So this isn't purely defensive against the impossible — add_breadcrumbs in particular is a remote call we don't want taking down a task. Mind another look?


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ab8fbb6 to ca897a8CompareJune 5, 2026 14:03
Comment on lines +303 to +306
sentry_sdk_module = sys.modules["sentry_sdk"]
sentry_sdk_module.new_scope = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)

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.

  1. This leaks fixtures outside of this test.
  2. This already exist as mock_sentry_sdk fixture and is used in the test right above this one.

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ca897a8 to ac9dd53CompareJune 11, 2026 01:28
@potiuk
potiuk marked this pull request as draft June 11, 2026 01:36
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ac9dd53 to 2fd2f54CompareJune 11, 2026 01:37
In ``ConfiguredSentry.enrich_errors``, ``prepare_to_enrich_errors``,
``add_tagging`` and ``add_breadcrumbs`` ran inside (or alongside) the
same try-block as the wrapped ``run(...)``. An exception raised by any
of them propagated through the wrapper, so a failure in non-critical
Sentry instrumentation looked indistinguishable from a real task
failure and the wrapped ``run`` never executed.
Wrap each instrumentation step in its own ``try/except`` and log a
structured warning on failure. The wrapped ``run`` still runs inside
``sentry_sdk.new_scope`` and its exceptions are captured and
re-raised — that part is unchanged.
@potiuk
potiuk marked this pull request as ready for review June 11, 2026 17:33
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 2fd2f54 to 4822f54CompareJune 11, 2026 17:33
@potiuk

Copy link
Copy Markdown
MemberAuthor

Fair question — I don't have a production occurrence. This is defensive: add_tagging/add_breadcrumbs call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped run() from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.

@potiuk

Copy link
Copy Markdown
MemberAuthor

Closing then.

@potiukpotiuk closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@potiuk@ashb
, '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

Isolate Sentry instrumentation from task run - #67505

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run
Closed

Isolate Sentry instrumentation from task run#67505
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run

Conversation

@potiuk

Copy link
Copy Markdown
Member

In ConfiguredSentry.enrich_errors, prepare_to_enrich_errors, add_tagging and add_breadcrumbs ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped run(...). An exception raised by any of them propagated through the wrapper, so:

  • A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
  • The wrapped run never executed — instrumentation could effectively prevent the task from running.

Reported as F-006 in the apache/tooling-agents L3 task-sdk sweep 0920c77.

Change

Wrap each instrumentation step (prepare_to_enrich_errors, add_tagging, add_breadcrumbs) in its own try/except and log a structured warning on failure (sentry_prepare_failed, sentry_add_tagging_failed, sentry_add_breadcrumbs_failed). The wrapped run still runs inside sentry_sdk.new_scope and its exceptions are captured via sentry_sdk.capture_exception(e) and re-raised — that path is unchanged.

Test plan

  • test_enrich_errors_isolates_instrumentation_from_task_run (parametrised across all three instrumentation steps) — asserts (a) the wrapped run() still executes when each step raises, (b) the structured warning is emitted on the task log.
  • test_enrich_errors_captures_and_reraises_task_failure — asserts the wrapped-run failure path is unchanged: sentry_sdk.capture_exception is called, exception re-raised.
  • prek run ruff clean.
  • prek run mypy-task-sdk clean.
  • Full test_sentry.py suite: 10 passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Generated-by: Claude Code (Opus 4.7) following the guidelines

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

Can those methods ever realistically fail?

Has this ever happened that we know of?

Comment on lines +157 to +162
except Exception:
log.warning("sentry_add_tagging_failed", exc_info=True)
try:
self.add_breadcrumbs(ti)
except Exception:
log.warning("sentry_add_breadcrumbs_failed", exc_info=True)

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.

A try/except for each is overkill.

Doubly so as these are all local operations. I don't think these can "ever" fail

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 7b93c6b to ab8fbb6CompareJune 5, 2026 01:10
@potiuk

Copy link
Copy Markdown
MemberAuthor

Good call on the granularity — collapsed it. prepare_to_enrich_errors stays in its own guard (it runs sentry_sdk.init()), and add_tagging + add_breadcrumbs now share a single try/except (one sentry_enrichment_failed warning) instead of one each.

On "can these ever fail / all local operations" though — two of the three aren't local:

  • add_breadcrumbs isn't local at all: it calls RuntimeTaskInstance.get_task_breadcrumbs(...), which does SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...)) — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
  • prepare_to_enrich_errors runs sentry_sdk.init(dsn=…) plus conf.getimport("sentry", "before_send"/"transport") — SDK/transport init and importing a configured callable can fail on misconfiguration.
  • add_tagging is the one that's genuinely local (scope.set_tag + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.

So this isn't purely defensive against the impossible — add_breadcrumbs in particular is a remote call we don't want taking down a task. Mind another look?


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ab8fbb6 to ca897a8CompareJune 5, 2026 14:03
Comment on lines +303 to +306
sentry_sdk_module = sys.modules["sentry_sdk"]
sentry_sdk_module.new_scope = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)

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.

  1. This leaks fixtures outside of this test.
  2. This already exist as mock_sentry_sdk fixture and is used in the test right above this one.

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ca897a8 to ac9dd53CompareJune 11, 2026 01:28
@potiuk
potiuk marked this pull request as draft June 11, 2026 01:36
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ac9dd53 to 2fd2f54CompareJune 11, 2026 01:37
In ``ConfiguredSentry.enrich_errors``, ``prepare_to_enrich_errors``,
``add_tagging`` and ``add_breadcrumbs`` ran inside (or alongside) the
same try-block as the wrapped ``run(...)``. An exception raised by any
of them propagated through the wrapper, so a failure in non-critical
Sentry instrumentation looked indistinguishable from a real task
failure and the wrapped ``run`` never executed.
Wrap each instrumentation step in its own ``try/except`` and log a
structured warning on failure. The wrapped ``run`` still runs inside
``sentry_sdk.new_scope`` and its exceptions are captured and
re-raised — that part is unchanged.
@potiuk
potiuk marked this pull request as ready for review June 11, 2026 17:33
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 2fd2f54 to 4822f54CompareJune 11, 2026 17:33
@potiuk

Copy link
Copy Markdown
MemberAuthor

Fair question — I don't have a production occurrence. This is defensive: add_tagging/add_breadcrumbs call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped run() from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.

@potiuk

Copy link
Copy Markdown
MemberAuthor

Closing then.

@potiukpotiuk closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@potiuk@ashb
, '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

Isolate Sentry instrumentation from task run - #67505

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run
Closed

Isolate Sentry instrumentation from task run#67505
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run

Conversation

@potiuk

Copy link
Copy Markdown
Member

In ConfiguredSentry.enrich_errors, prepare_to_enrich_errors, add_tagging and add_breadcrumbs ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped run(...). An exception raised by any of them propagated through the wrapper, so:

  • A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
  • The wrapped run never executed — instrumentation could effectively prevent the task from running.

Reported as F-006 in the apache/tooling-agents L3 task-sdk sweep 0920c77.

Change

Wrap each instrumentation step (prepare_to_enrich_errors, add_tagging, add_breadcrumbs) in its own try/except and log a structured warning on failure (sentry_prepare_failed, sentry_add_tagging_failed, sentry_add_breadcrumbs_failed). The wrapped run still runs inside sentry_sdk.new_scope and its exceptions are captured via sentry_sdk.capture_exception(e) and re-raised — that path is unchanged.

Test plan

  • test_enrich_errors_isolates_instrumentation_from_task_run (parametrised across all three instrumentation steps) — asserts (a) the wrapped run() still executes when each step raises, (b) the structured warning is emitted on the task log.
  • test_enrich_errors_captures_and_reraises_task_failure — asserts the wrapped-run failure path is unchanged: sentry_sdk.capture_exception is called, exception re-raised.
  • prek run ruff clean.
  • prek run mypy-task-sdk clean.
  • Full test_sentry.py suite: 10 passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Generated-by: Claude Code (Opus 4.7) following the guidelines

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

Can those methods ever realistically fail?

Has this ever happened that we know of?

Comment on lines +157 to +162
except Exception:
log.warning("sentry_add_tagging_failed", exc_info=True)
try:
self.add_breadcrumbs(ti)
except Exception:
log.warning("sentry_add_breadcrumbs_failed", exc_info=True)

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.

A try/except for each is overkill.

Doubly so as these are all local operations. I don't think these can "ever" fail

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 7b93c6b to ab8fbb6CompareJune 5, 2026 01:10
@potiuk

Copy link
Copy Markdown
MemberAuthor

Good call on the granularity — collapsed it. prepare_to_enrich_errors stays in its own guard (it runs sentry_sdk.init()), and add_tagging + add_breadcrumbs now share a single try/except (one sentry_enrichment_failed warning) instead of one each.

On "can these ever fail / all local operations" though — two of the three aren't local:

  • add_breadcrumbs isn't local at all: it calls RuntimeTaskInstance.get_task_breadcrumbs(...), which does SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...)) — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
  • prepare_to_enrich_errors runs sentry_sdk.init(dsn=…) plus conf.getimport("sentry", "before_send"/"transport") — SDK/transport init and importing a configured callable can fail on misconfiguration.
  • add_tagging is the one that's genuinely local (scope.set_tag + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.

So this isn't purely defensive against the impossible — add_breadcrumbs in particular is a remote call we don't want taking down a task. Mind another look?


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ab8fbb6 to ca897a8CompareJune 5, 2026 14:03
Comment on lines +303 to +306
sentry_sdk_module = sys.modules["sentry_sdk"]
sentry_sdk_module.new_scope = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)

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.

  1. This leaks fixtures outside of this test.
  2. This already exist as mock_sentry_sdk fixture and is used in the test right above this one.

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ca897a8 to ac9dd53CompareJune 11, 2026 01:28
@potiuk
potiuk marked this pull request as draft June 11, 2026 01:36
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ac9dd53 to 2fd2f54CompareJune 11, 2026 01:37
In ``ConfiguredSentry.enrich_errors``, ``prepare_to_enrich_errors``,
``add_tagging`` and ``add_breadcrumbs`` ran inside (or alongside) the
same try-block as the wrapped ``run(...)``. An exception raised by any
of them propagated through the wrapper, so a failure in non-critical
Sentry instrumentation looked indistinguishable from a real task
failure and the wrapped ``run`` never executed.
Wrap each instrumentation step in its own ``try/except`` and log a
structured warning on failure. The wrapped ``run`` still runs inside
``sentry_sdk.new_scope`` and its exceptions are captured and
re-raised — that part is unchanged.
@potiuk
potiuk marked this pull request as ready for review June 11, 2026 17:33
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 2fd2f54 to 4822f54CompareJune 11, 2026 17:33
@potiuk

Copy link
Copy Markdown
MemberAuthor

Fair question — I don't have a production occurrence. This is defensive: add_tagging/add_breadcrumbs call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped run() from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.

@potiuk

Copy link
Copy Markdown
MemberAuthor

Closing then.

@potiukpotiuk closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@potiuk@ashb
, '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

Isolate Sentry instrumentation from task run - #67505

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run
Closed

Isolate Sentry instrumentation from task run#67505
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run

Conversation

@potiuk

Copy link
Copy Markdown
Member

In ConfiguredSentry.enrich_errors, prepare_to_enrich_errors, add_tagging and add_breadcrumbs ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped run(...). An exception raised by any of them propagated through the wrapper, so:

  • A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
  • The wrapped run never executed — instrumentation could effectively prevent the task from running.

Reported as F-006 in the apache/tooling-agents L3 task-sdk sweep 0920c77.

Change

Wrap each instrumentation step (prepare_to_enrich_errors, add_tagging, add_breadcrumbs) in its own try/except and log a structured warning on failure (sentry_prepare_failed, sentry_add_tagging_failed, sentry_add_breadcrumbs_failed). The wrapped run still runs inside sentry_sdk.new_scope and its exceptions are captured via sentry_sdk.capture_exception(e) and re-raised — that path is unchanged.

Test plan

  • test_enrich_errors_isolates_instrumentation_from_task_run (parametrised across all three instrumentation steps) — asserts (a) the wrapped run() still executes when each step raises, (b) the structured warning is emitted on the task log.
  • test_enrich_errors_captures_and_reraises_task_failure — asserts the wrapped-run failure path is unchanged: sentry_sdk.capture_exception is called, exception re-raised.
  • prek run ruff clean.
  • prek run mypy-task-sdk clean.
  • Full test_sentry.py suite: 10 passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Generated-by: Claude Code (Opus 4.7) following the guidelines

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

Can those methods ever realistically fail?

Has this ever happened that we know of?

Comment on lines +157 to +162
except Exception:
log.warning("sentry_add_tagging_failed", exc_info=True)
try:
self.add_breadcrumbs(ti)
except Exception:
log.warning("sentry_add_breadcrumbs_failed", exc_info=True)

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.

A try/except for each is overkill.

Doubly so as these are all local operations. I don't think these can "ever" fail

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 7b93c6b to ab8fbb6CompareJune 5, 2026 01:10
@potiuk

Copy link
Copy Markdown
MemberAuthor

Good call on the granularity — collapsed it. prepare_to_enrich_errors stays in its own guard (it runs sentry_sdk.init()), and add_tagging + add_breadcrumbs now share a single try/except (one sentry_enrichment_failed warning) instead of one each.

On "can these ever fail / all local operations" though — two of the three aren't local:

  • add_breadcrumbs isn't local at all: it calls RuntimeTaskInstance.get_task_breadcrumbs(...), which does SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...)) — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
  • prepare_to_enrich_errors runs sentry_sdk.init(dsn=…) plus conf.getimport("sentry", "before_send"/"transport") — SDK/transport init and importing a configured callable can fail on misconfiguration.
  • add_tagging is the one that's genuinely local (scope.set_tag + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.

So this isn't purely defensive against the impossible — add_breadcrumbs in particular is a remote call we don't want taking down a task. Mind another look?


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ab8fbb6 to ca897a8CompareJune 5, 2026 14:03
Comment on lines +303 to +306
sentry_sdk_module = sys.modules["sentry_sdk"]
sentry_sdk_module.new_scope = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)

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.

  1. This leaks fixtures outside of this test.
  2. This already exist as mock_sentry_sdk fixture and is used in the test right above this one.

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ca897a8 to ac9dd53CompareJune 11, 2026 01:28
@potiuk
potiuk marked this pull request as draft June 11, 2026 01:36
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ac9dd53 to 2fd2f54CompareJune 11, 2026 01:37
In ``ConfiguredSentry.enrich_errors``, ``prepare_to_enrich_errors``,
``add_tagging`` and ``add_breadcrumbs`` ran inside (or alongside) the
same try-block as the wrapped ``run(...)``. An exception raised by any
of them propagated through the wrapper, so a failure in non-critical
Sentry instrumentation looked indistinguishable from a real task
failure and the wrapped ``run`` never executed.
Wrap each instrumentation step in its own ``try/except`` and log a
structured warning on failure. The wrapped ``run`` still runs inside
``sentry_sdk.new_scope`` and its exceptions are captured and
re-raised — that part is unchanged.
@potiuk
potiuk marked this pull request as ready for review June 11, 2026 17:33
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 2fd2f54 to 4822f54CompareJune 11, 2026 17:33
@potiuk

Copy link
Copy Markdown
MemberAuthor

Fair question — I don't have a production occurrence. This is defensive: add_tagging/add_breadcrumbs call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped run() from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.

@potiuk

Copy link
Copy Markdown
MemberAuthor

Closing then.

@potiukpotiuk closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@potiuk@ashb
, '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

Isolate Sentry instrumentation from task run - #67505

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run
Closed

Isolate Sentry instrumentation from task run#67505
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run

Conversation

@potiuk

Copy link
Copy Markdown
Member

In ConfiguredSentry.enrich_errors, prepare_to_enrich_errors, add_tagging and add_breadcrumbs ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped run(...). An exception raised by any of them propagated through the wrapper, so:

  • A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
  • The wrapped run never executed — instrumentation could effectively prevent the task from running.

Reported as F-006 in the apache/tooling-agents L3 task-sdk sweep 0920c77.

Change

Wrap each instrumentation step (prepare_to_enrich_errors, add_tagging, add_breadcrumbs) in its own try/except and log a structured warning on failure (sentry_prepare_failed, sentry_add_tagging_failed, sentry_add_breadcrumbs_failed). The wrapped run still runs inside sentry_sdk.new_scope and its exceptions are captured via sentry_sdk.capture_exception(e) and re-raised — that path is unchanged.

Test plan

  • test_enrich_errors_isolates_instrumentation_from_task_run (parametrised across all three instrumentation steps) — asserts (a) the wrapped run() still executes when each step raises, (b) the structured warning is emitted on the task log.
  • test_enrich_errors_captures_and_reraises_task_failure — asserts the wrapped-run failure path is unchanged: sentry_sdk.capture_exception is called, exception re-raised.
  • prek run ruff clean.
  • prek run mypy-task-sdk clean.
  • Full test_sentry.py suite: 10 passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Generated-by: Claude Code (Opus 4.7) following the guidelines

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

Can those methods ever realistically fail?

Has this ever happened that we know of?

Comment on lines +157 to +162
except Exception:
log.warning("sentry_add_tagging_failed", exc_info=True)
try:
self.add_breadcrumbs(ti)
except Exception:
log.warning("sentry_add_breadcrumbs_failed", exc_info=True)

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.

A try/except for each is overkill.

Doubly so as these are all local operations. I don't think these can "ever" fail

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 7b93c6b to ab8fbb6CompareJune 5, 2026 01:10
@potiuk

Copy link
Copy Markdown
MemberAuthor

Good call on the granularity — collapsed it. prepare_to_enrich_errors stays in its own guard (it runs sentry_sdk.init()), and add_tagging + add_breadcrumbs now share a single try/except (one sentry_enrichment_failed warning) instead of one each.

On "can these ever fail / all local operations" though — two of the three aren't local:

  • add_breadcrumbs isn't local at all: it calls RuntimeTaskInstance.get_task_breadcrumbs(...), which does SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...)) — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
  • prepare_to_enrich_errors runs sentry_sdk.init(dsn=…) plus conf.getimport("sentry", "before_send"/"transport") — SDK/transport init and importing a configured callable can fail on misconfiguration.
  • add_tagging is the one that's genuinely local (scope.set_tag + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.

So this isn't purely defensive against the impossible — add_breadcrumbs in particular is a remote call we don't want taking down a task. Mind another look?


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ab8fbb6 to ca897a8CompareJune 5, 2026 14:03
Comment on lines +303 to +306
sentry_sdk_module = sys.modules["sentry_sdk"]
sentry_sdk_module.new_scope = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)

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.

  1. This leaks fixtures outside of this test.
  2. This already exist as mock_sentry_sdk fixture and is used in the test right above this one.

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ca897a8 to ac9dd53CompareJune 11, 2026 01:28
@potiuk
potiuk marked this pull request as draft June 11, 2026 01:36
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ac9dd53 to 2fd2f54CompareJune 11, 2026 01:37
In ``ConfiguredSentry.enrich_errors``, ``prepare_to_enrich_errors``,
``add_tagging`` and ``add_breadcrumbs`` ran inside (or alongside) the
same try-block as the wrapped ``run(...)``. An exception raised by any
of them propagated through the wrapper, so a failure in non-critical
Sentry instrumentation looked indistinguishable from a real task
failure and the wrapped ``run`` never executed.
Wrap each instrumentation step in its own ``try/except`` and log a
structured warning on failure. The wrapped ``run`` still runs inside
``sentry_sdk.new_scope`` and its exceptions are captured and
re-raised — that part is unchanged.
@potiuk
potiuk marked this pull request as ready for review June 11, 2026 17:33
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 2fd2f54 to 4822f54CompareJune 11, 2026 17:33
@potiuk

Copy link
Copy Markdown
MemberAuthor

Fair question — I don't have a production occurrence. This is defensive: add_tagging/add_breadcrumbs call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped run() from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.

@potiuk

Copy link
Copy Markdown
MemberAuthor

Closing then.

@potiukpotiuk closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@potiuk@ashb
, '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

Isolate Sentry instrumentation from task run - #67505

Closed
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run
Closed

Isolate Sentry instrumentation from task run#67505
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:isolate-sentry-instrumentation-from-task-run

Conversation

@potiuk

Copy link
Copy Markdown
Member

In ConfiguredSentry.enrich_errors, prepare_to_enrich_errors, add_tagging and add_breadcrumbs ran either outside any try block (line 142, pre-fix) or inside the same try-block as the wrapped run(...). An exception raised by any of them propagated through the wrapper, so:

  • A failure in non-critical Sentry instrumentation was indistinguishable from a real task failure.
  • The wrapped run never executed — instrumentation could effectively prevent the task from running.

Reported as F-006 in the apache/tooling-agents L3 task-sdk sweep 0920c77.

Change

Wrap each instrumentation step (prepare_to_enrich_errors, add_tagging, add_breadcrumbs) in its own try/except and log a structured warning on failure (sentry_prepare_failed, sentry_add_tagging_failed, sentry_add_breadcrumbs_failed). The wrapped run still runs inside sentry_sdk.new_scope and its exceptions are captured via sentry_sdk.capture_exception(e) and re-raised — that path is unchanged.

Test plan

  • test_enrich_errors_isolates_instrumentation_from_task_run (parametrised across all three instrumentation steps) — asserts (a) the wrapped run() still executes when each step raises, (b) the structured warning is emitted on the task log.
  • test_enrich_errors_captures_and_reraises_task_failure — asserts the wrapped-run failure path is unchanged: sentry_sdk.capture_exception is called, exception re-raised.
  • prek run ruff clean.
  • prek run mypy-task-sdk clean.
  • Full test_sentry.py suite: 10 passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 4.7)

Generated-by: Claude Code (Opus 4.7) following the guidelines

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

Can those methods ever realistically fail?

Has this ever happened that we know of?

Comment on lines +157 to +162
except Exception:
log.warning("sentry_add_tagging_failed", exc_info=True)
try:
self.add_breadcrumbs(ti)
except Exception:
log.warning("sentry_add_breadcrumbs_failed", exc_info=True)

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.

A try/except for each is overkill.

Doubly so as these are all local operations. I don't think these can "ever" fail

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 7b93c6b to ab8fbb6CompareJune 5, 2026 01:10
@potiuk

Copy link
Copy Markdown
MemberAuthor

Good call on the granularity — collapsed it. prepare_to_enrich_errors stays in its own guard (it runs sentry_sdk.init()), and add_tagging + add_breadcrumbs now share a single try/except (one sentry_enrichment_failed warning) instead of one each.

On "can these ever fail / all local operations" though — two of the three aren't local:

  • add_breadcrumbs isn't local at all: it calls RuntimeTaskInstance.get_task_breadcrumbs(...), which does SUPERVISOR_COMMS.send(GetTaskBreadcrumbs(...)) — a blocking Execution-API/IPC round-trip to the supervisor. That can raise (IPC/API error), and today it would surface as a task failure.
  • prepare_to_enrich_errors runs sentry_sdk.init(dsn=…) plus conf.getimport("sentry", "before_send"/"transport") — SDK/transport init and importing a configured callable can fail on misconfiguration.
  • add_tagging is the one that's genuinely local (scope.set_tag + attribute reads), so you're right there — which is partly why it's now folded into the shared guard rather than its own.

So this isn't purely defensive against the impossible — add_breadcrumbs in particular is a remote call we don't want taking down a task. Mind another look?


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ab8fbb6 to ca897a8CompareJune 5, 2026 14:03
Comment on lines +303 to +306
sentry_sdk_module = sys.modules["sentry_sdk"]
sentry_sdk_module.new_scope = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__enter__ = mock.MagicMock()
sentry_sdk_module.new_scope.return_value.__exit__ = mock.MagicMock(return_value=False)

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.

  1. This leaks fixtures outside of this test.
  2. This already exist as mock_sentry_sdk fixture and is used in the test right above this one.

@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ca897a8 to ac9dd53CompareJune 11, 2026 01:28
@potiuk
potiuk marked this pull request as draft June 11, 2026 01:36
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from ac9dd53 to 2fd2f54CompareJune 11, 2026 01:37
In ``ConfiguredSentry.enrich_errors``, ``prepare_to_enrich_errors``,
``add_tagging`` and ``add_breadcrumbs`` ran inside (or alongside) the
same try-block as the wrapped ``run(...)``. An exception raised by any
of them propagated through the wrapper, so a failure in non-critical
Sentry instrumentation looked indistinguishable from a real task
failure and the wrapped ``run`` never executed.
Wrap each instrumentation step in its own ``try/except`` and log a
structured warning on failure. The wrapped ``run`` still runs inside
``sentry_sdk.new_scope`` and its exceptions are captured and
re-raised — that part is unchanged.
@potiuk
potiuk marked this pull request as ready for review June 11, 2026 17:33
@potiuk
potiukforce-pushed the isolate-sentry-instrumentation-from-task-run branch from 2fd2f54 to 4822f54CompareJune 11, 2026 17:33
@potiuk

Copy link
Copy Markdown
MemberAuthor

Fair question — I don't have a production occurrence. This is defensive: add_tagging/add_breadcrumbs call into the Sentry SDK, which can in principle raise (SDK bugs, odd tag-value serialization), and today such a raise is indistinguishable from a real task failure and can stop the wrapped run() from executing at all. The value is decoupling non-critical instrumentation from the critical run path, not fixing an observed bug. If the bar is a demonstrated failure, I'm happy to close it — no strong attachment.

@potiuk

Copy link
Copy Markdown
MemberAuthor

Closing then.

@potiukpotiuk closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@potiuk@ashb