Warn when supervisor-side mask_secret IPC send fails

[potiuk]

mask_secret() mirrors the mask to the supervisor process so any forwarded logs are masked there too. The send at log.py:281 was wrapped in a bare suppress(Exception), so a transient IPC failure was silently swallowed.

When sending_to_supervisor=True the local task drops its own mask_logs processor (it relies on the supervisor to do the masking — see log.py lines 72-73 and 115-118) — so a silent send failure could leave the secret unmasked in supervisor-level logs. Operators had no signal that this had happened.

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

Change

Replace the bare suppress with a targeted try/except that emits a warning to a dedicated airflow.logging.mask_secret structlog logger when the supervisor is available but the send fails. The "no supervisor context" case (mask_secret called outside task execution, no SUPERVISOR_COMMS) stays silent — there is no supervisor to notify.

Test plan

• test_warns_when_supervisor_send_fails — patches task_runner.SUPERVISOR_COMMS to a Mock whose send raises; captures structlog output and asserts the supervisor_mask_secret_failed event lands with the secret name and exc_info.
• test_silent_when_supervisor_send_succeeds — success path emits no warning, comms.send called once.
• test_silent_when_no_supervisor_context — outside a task-execution context the function is a no-op.
• prek run ruff clean.
• prek run mypy-task-sdk clean.
• Full test_log.py suite: 8 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

[potiuk]

Gentle nudge — this and a few related task-SDK / execution-API hardening PRs are green and have been waiting on review (9 days): #67503, #67504, #67505, #67506, #67628. They're small, self-contained fixes. @ashb @kaxil @amoghrajesh — would appreciate a look when you have a moment.

---

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

[ashb]

Fairly sure this is a false positive.

[potiuk]

Fair point that in a full supervisor crash the warning can't be delivered — stdout/stderr are the supervisor's sockets, as you say. But comms.send() raising isn't only the crash case: the MaskSecret message can fail to build/serialize before the socket is touched. MaskSecret.value is JsonValue, and send() runs _make_frame(msg).as_bytes() (pydantic model_dump + msgspec encode, plus OTel trace injection) before socket.sendall. A non-serializable value (or an encode hiccup) raises while the supervisor is alive and reachable — so the warning is both deliverable and the only signal that registration was lost.

And that's the case that actually bites: under sending_to_supervisor=True the task drops its own mask_logs processor (log.py), so a silently-swallowed failure leaves the secret unmasked in supervisor-level logs. The old with suppress(Exception) hid exactly that.

You're right that the original test (a generic RuntimeError mock) read like the unrealistic crash case — I've reworked it: it now feeds a non-serializable value so the MaskSecret build genuinely fails and asserts comms.send is never reached (a real, supervisor-alive failure), plus a separate BrokenPipeError case for the socket branch. Mind taking another look?

---

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

[ashb]

the MaskSecret message can fail to build/serialize before the socket is touched

No, I don't think it can.

[potiuk]

To lay out where this stands and a way to make it stronger.

Current approach. The PR replaces the old with suppress(Exception) around the supervisor IPC with a try/except that logs a dedicated supervisor_mask_secret_failed warning. The motivation is that this is a security path, and the failure is silent today: add_mask is deliberately built to mask non-str/dict iterables element-wise (its elif isinstance(secret, Iterable) branch — set/tuple/frozenset/generators), but MaskSecret.value: JsonValue rejects exactly those at construction. So mask_secret(<set of secrets>) masks locally yet never registers with the supervisor, and under sending_to_supervisor=True (where the task drops its own mask_logs processor) the secret surfaces unmasked in supervisor-side logs. A small test pins this down: same input fails on main (warning swallowed) and passes here (warning fires), with local masking and comms.send-not-reached both holding — i.e. a real, supervisor-alive silent leak.

On the "the types already prevent this" point. You're right that JsonValue + mypy reject a literal set/tuple, and I agree a statically-JsonValue value can't fail this. But I'm not comfortable basing a security control on mypy at all: there's no guarantee or expectation that our users run mypy — it isn't part of the contract for Dag/provider authors, and until the mypy plugin we're currently preparing actually ships, running mypy against Airflow code is practically discouraged. On top of that, mask_secret is public API and the values that actually reach it are routinely Any (anything pulled out of a bare dict, a secrets backend, etc.), which mypy waves straight through even if the author does run it. For redaction, "the annotation says it can't happen" isn't the same as "it can't happen", and the cost of being wrong is a leaked secret — so I'd rather have a runtime signal than rely on the annotation.

Proposal to make it stronger. Rather than just reporting the gap, we can mostly close it by coercing the non-JSON iterables to a list before building the message, so they actually propagate and get masked supervisor-side:

ipc_value = list(secret) if isinstance(secret, (set, frozenset, tuple)) else secret
comms.send(MaskSecret(value=ipc_value, name=name))

That makes the two paths agree for the common case (a set/tuple/frozenset of secrets) instead of papering over the difference. The warning then drops to a genuine last-resort guard for the residue coercion can't save — an exhausted generator (already consumed by the earlier add_mask) or a truly non-serializable object — which is a much more defensible role for it. I'd also document the JsonValue constraint on MaskSecret so the boundary isn't implicit.

Happy to push the coercion + a test asserting the supervisor now receives the masked list, if you're on board — or keep it warning-only if you'd prefer the smaller change. Which do you want?

---

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

[potiuk]

@ashb — gentle nudge on this one. It's now rebased on main (the test_log.py conflict from the #66571 revert is resolved) and green.

The open question from my last comment is still the decision point: mask_secret is public API and routinely receives Any values, and add_mask deliberately masks non-str/dict iterables (set/tuple/frozenset/generators) element-wise — but MaskSecret.value: JsonValue rejects exactly those at construction, so today they're masked locally yet silently never registered with the supervisor. I'd rather not gate a redaction control on mypy, since users aren't required to run it.

Two ways forward, your call:

1. Warning-only (this PR as-is) — surface the silent failure.
2. Coerce + warn — list() the set/tuple/frozenset before building the message so they actually propagate and get masked supervisor-side, leaving the warning as a genuine last-resort guard. I think this is the stronger fix and I'm happy to push it.

Which would you prefer?

---

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

[ashb]

Doesn't hurt to play it safe.

[potiuk]

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.

[potiuk]

Possibly still worth merging it now @ashb ?