Skip to content

Guard StackdriverRemoteLogIO's transport.send() against errors - #71337

Closed
roshanprabu wants to merge 1 commit into
apache:mainfrom
roshanprabu:fix-stackdriver-transport-send-guard
Closed

roshanprabu wants to merge 1 commit into
apache:mainfrom
roshanprabu:fix-stackdriver-transport-send-guard

Conversation

@roshanprabu

Copy link
Copy Markdown
Contributor

Summary

Fixes one of three bugs reported in #68240 (the one I could verify is still present and fix with high confidence -- see scope note below).

StackdriverRemoteLogIO.processors' proc closure calls _transport.send(...) with no error handling:

_transport.send(record, str(msg.get("event", "")), resource=self.resource, labels=labels)
return event

This processor is installed for every supervised component (scheduler, dag-processor, triggerer, workers) whenever REMOTE_TASK_LOG routes through Stackdriver. Any IAM permission error, gRPC connectivity failure, or quota exception from Cloud Logging propagates straight out of the structlog processor and takes down the whole process -- reported as a dag-processor pod stuck in CrashLoopBackOff on every log emit when its Kubernetes Service Account lacked the logging.logEntries.create IAM binding.

The read path already has exactly this guard, a few lines down in the same file, with a comment explaining the rationale:

try:
    messages, end_of_log, next_page_token = self.io.read_logs(log_filter, next_page_token, all_pages)
except Exception:
    # Cloud Logging unavailable / IAM glitch / gRPC error. Without a guard, the
    # exception used to propagate up as HTTP 500 from the log viewer. ...
    _logger.exception(...)

The write path was simply missing the equivalent. This PR adds it, reusing the same _logger (already defined in this file specifically for handler-internal failures) and logging a warning instead of raising, since log delivery here is best-effort.

Scope note: the linked issue reports three bugs. I traced the other two (empty labels when record.task_instance is unset in supervisor context, and read() filtering on logical_date) against current main and found the code has moved on since the report -- there's now a fallback in proc() that reads labels from the event dict when no task_instance is present (see test_processors_fallback_to_event_labels), and read()'s ti: RuntimeTI already exposes logical_date locally without a DB round-trip. I wasn't confident those two are still bugs as described, so this PR only covers the third (transport.send() unguarded), which I could verify precisely.

Test plan

  • Added test_processors_survives_transport_send_failure: mocks transport.send() to raise, asserts the processor doesn't propagate the exception.
  • Verified the test actually catches the bug: reverted the source fix and confirmed the new test fails with Exception: IAM permission denied propagating out of proc(); re-applied the fix and confirmed it passes.
  • Ran the full file: pytest tests/unit/google/cloud/log/test_stackdriver_task_handler.py -- 33 passed.
  • ruff check and ruff format --check pass on both changed files.

The log processor's transport.send() call ran unguarded. This
processor is installed for every supervised component (scheduler,
dag-processor, triggerer, workers) whenever REMOTE_TASK_LOG routes
through Stackdriver, so any IAM permission error, gRPC connectivity
failure, or quota exception from Cloud Logging propagated straight out
of the structlog processor and crashed the entire process -- observed
as a dag-processor stuck in CrashLoopBackOff from a missing
logging.logEntries.create IAM binding.

The read path already has this exact guard one function down (see the
try/except around read_logs() a few lines below, with the same
rationale in its comment); the write path was simply missing the
equivalent. Log delivery is best-effort, so this logs a warning via
the handler's own logger instead of raising.
@roshanprabu
roshanprabu requested a review from shahar1 as a code owner August 8, 2026 20:16
@boring-cyborg boring-cyborg Bot added area:logging area:providers provider:google Google (including GCP) related issues labels Aug 8, 2026
@olayiwola-gbenga

Copy link
Copy Markdown
Contributor

Hi @roshanprabu Please look into failing check

@shahar1 shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your contribution! Two requests:

  1. Resolve conflicts and make the appropriate adjustments as classes have been renamed.
  2. Add AI attribution checkboxes to the PR's body.

@shahar1

shahar1 commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Thanks for working on this. With the default BackgroundThreadTransport, which is what the stackdriver:// wiring, from_config and the deprecated alias all use, send() only puts the entry on an in-memory queue. The Cloud Logging API call happens in the library's background worker, which already catches and logs commit errors (_safely_commit_batch), so the new except branch can't run in supported configurations. It would only fire if a custom Python logging config passed SyncTransport explicitly; remote_task_handler_kwargs can't select one. The reporter's stack trace was never provided, and an earlier PR for the same fix (#68250) was closed for the same reason, so I'm closing this one. If you can reproduce the crash with the default transport, please open an issue with the traceback and we'll look at the real failure path.


Drafted-by: Claude Code (Opus 5.5); reviewed by @shahar1 before posting

@shahar1 shahar1 closed this Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:logging area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants