1fanwang commented on code in PR #66633:
URL: https://github.com/apache/airflow/pull/66633#discussion_r3227419819
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -2189,11 +2190,36 @@ def ensure_secrets_backend_loaded() ->
list[BaseSecretsBackend]:
return ensure_secrets_loaded(default_backends=fallback_backends)
-def _configure_logging(log_path: str, client: Client) ->
tuple[FilteringBoundLogger, BinaryIO | TextIO]:
+def _close_remote_log_handler(handler: RemoteLogIO) -> None:
+ """
+ Close the remote log handler explicitly after all task log messages have
been drained, before process exit triggers logging.shutdown().
+
+ This prevents WatchtowerWarning: "Received message after logging system
shutdown" which causes CloudWatch log streams to be silently truncated.
+ """
+ import logging as _logging
+
+ # If the handler is also a stdlib logging.Handler, deregister it from
+ # the logging hierarchy so logging.shutdown() at process exit cannot
+ # double-close it.
+ if isinstance(handler, _logging.Handler):
+ root = _logging.getLogger()
Review Comment:
Is this `isinstance(handler, _logging.Handler)` guard expected to ever
evaluate truthy in practice? Looking at the three concrete `RemoteLogIO`
implementations:
- `CloudWatchRemoteLogIO(LoggingMixin)` in
`providers/amazon/.../cloudwatch_task_handler.py:85` — `LoggingMixin` just
provides a `.log` property, not a `logging.Handler` base
- `ElasticsearchRemoteLogIO` and `OpenSearchRemoteLogIO` are the same shape
(`LoggingMixin`, not `Handler`)
So the dereg block (`root.removeHandler` / per-logger `removeHandler`) is
effectively dead, and the only thing this function does at runtime is
`handler.close()` — which for CloudWatch lands on the existing `close()` that
just calls `self.handler.flush()` without touching the inner watchtower
handler's stdlib registration. If the goal is to prevent `logging.shutdown()`
from later double-closing the *inner* watchtower handler, you probably want one
level deeper — something like `getattr(handler, 'handler', None)` or letting
each `RemoteLogIO.close()` impl handle its own dereg (and document that
contract on the protocol).
Happy to be wrong if there's a planned `RemoteLogIO` subclass that actually
inherits `logging.Handler` — wasn't sure from the diff.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
