Skip to content

fix(masking): mask at handler level instead of repointing streams#17692

Draft
kyungsoo-datahub wants to merge 6 commits into
masterfrom
fix/masking-handler-filter
Draft

fix(masking): mask at handler level instead of repointing streams#17692
kyungsoo-datahub wants to merge 6 commits into
masterfrom
fix/masking-handler-filter

Conversation

@kyungsoo-datahub
Copy link
Copy Markdown
Contributor

@kyungsoo-datahub kyungsoo-datahub commented Jun 2, 2026

Problem

In datahub-executor (celery worker mode), enabling secret masking silenced all logs from the worker process. After an ingestion with secrets, celery/root logs stopped appearing — the ingestion subprocess kept logging, which was misleading.

Root causes

  1. install_masking_filter() repointed the stdout/stderr handler streams at the wrapped sys.stderr (via _update_existing_handlers). Under celery, sys.stderr is a proxy that re-enters logging, so the handler looped until a RecursionError that the wrapper silently swallowed — every line dropped. Teardown restored sys.stderr but not the handlers, so it stayed broken.
  2. The filter sat on the root logger, which Python doesn't consult for records propagated from child loggers — so child-logger output (nearly all logs) was only ever masked via that stream-repointing.

Fix

  • Mask at the handler level: attach the filter to existing handlers (records masked in place, streams untouched).
  • Remove _update_existing_handlers() — no stream repointing, so no celery recursion.
  • Keep the sys.stdout/sys.stderr wrapper only for raw print() output.
  • Symmetric teardown: remove the filter from handlers on uninstall.
  • Skip datahub.masking.* loggers (they bypass masking by design).
  • Repeat install re-scans for handlers added since; logger/handler iteration is concurrency-safe.

Repointing handler streams at the wrapped sys.stderr deadlocked logging
under celery, whose stderr proxy re-enters the logging system, silently
dropping all output. A filter on the root logger also never saw child
logger records, so they went out unmasked.

Attach the masking filter to existing handlers (masking records in place
without touching streams), drop the stream-repointing, and remove the
filter from handlers symmetrically on teardown.
@github-actions github-actions Bot added the ingestion PR or Issue related to the ingestion of metadata label Jun 2, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

❌ Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ta-ingestion/src/datahub/masking/masking_filter.py 89.28% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

…eration

Repeat install now re-attaches the filter to handlers added since the
first install (masking is fail-open, so a missed handler leaks). Iterate
loggerDict and handler lists defensively for multi-threaded workers, and
document that masking covers handlers present at install time.

Add tests for repeat-install coverage and teardown symmetry.
…eakage

Masking is process-global (a filter on every handler + a singleton registry +
a bootstrap flag). Tests that install it (ingest CLI, initialize_secret_masking,
SecretStr config validation) don't all tear it down, so a later test's captured
logs got masked (e.g. 'test_view.' -> '***REDACTED***'). Autouse fixture calls
shutdown_secret_masking + resets the registry after each test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ingestion PR or Issue related to the ingestion of metadata

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant