potiuk commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4700343874
Thanks @ashb — good catch on the leak risk especially. Addressed all three and rebased onto main: 1. **The leak:** you're right. A `MaskSecret` build failure can raise an error whose text reprs the value, and on this path the local `mask_logs` processor is already off — so `exc_info=True` could have guaranteed the very leak the warning reports on. It now records only `error_type=type(e).__name__` — no `exc_info`, no exception message, no value. The new test pins this down: the unserializable value's `repr` embeds a canary string and the test asserts that canary never appears in the emitted warning. 2. **"context" nit:** reworded to "No supervisor to notify (not running under a supervisor process)" so it doesn't clash with the SDK's `context`. 3. **On whether `MaskSecret` can realistically fail to serialize** — you may well be right that it shouldn't in normal use (callers pass `JsonValue`). I've kept a forced-failure test only as defense-in-depth: if a future caller ever does pass a bad value, the warning must still not leak it. The tests now exist mainly to prove that no-leak property, with the broken-socket case as the other defensible path. Since the warning is now provably leak-safe and only fires on a genuine registration failure, hopefully this addresses the concern — happy to drop it entirely if you'd still rather not carry it. -- 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]
