Uh oh!
There was an error while loading. Please reload this page.
manual state change should not use fork-execute model on scheduler - #65677
Conversation
Signed-off-by: Maciej Obuchowski <maciej.obuchowski@datadoghq.com>
107b18f to
e0aa3e6Compare- Assigning isoformat() string to datetime-typed variable confused mypy now that the assignment is in the outer method scope (previously inside a nested closure where inference behaved differently). - ti.operator is str | None; guard against None before .lower(). Signed-off-by: Maciej Obuchowski <maciej.obuchowski@datadoghq.com>
kacpermuda
left a comment
There was a problem hiding this comment.
Thanks Maciej, nice fix !
potiuk
left a comment
There was a problem hiding this comment.
LGTM — diagnosis matches the AF3+ paths (process_executor_events → handle_failure,
Celery TI adoption after scheduler restart) and routing through the existingProcessPoolExecutor is the right fix. Pre-computing primitives in the parent
before crossing the pool boundary is correct (the comment referencing the
equivalent issue in on_dag_run_running is appreciated).
Non-blocking follow-ups for a separate PR:
Stats.gaugenow runs inside a pool worker. Existing pool callables
(adapter.dag_startedetc.) don't touchStats, and_executor_initializer
only callssettings.configure_orm(). Statsd UDP clients are stateless
per-call so this likely just works, but worth a quick sanity check that the
gauge actually lands — the previous fork-child inherited the parent's stats
client, so this is a new path for the metric.Lock in the regression with a negative assertion. Adding
mock.patch("…OpenLineageListener._fork_execute").assert_not_called()to
the three modified tests would prevent a future change from silently
reintroducingos.fork()here. Cheap insurance.if not self.executor:is effectively dead. Same pattern ason_dag_run_running/_success/_failed— theexecutorproperty
lazily instantiates aProcessPoolExecutorand a fresh one is truthy, so
the branch never logs "Executor has not started…". Either drop the guards
or change them toif self._executor is None:so they short-circuit before
instantiation. Not introduced here; just inherited.
Minor nits (take or leave):
(ti.operator or \"\").lower()will emitol.event.size.fail.(trailing dot,
empty operator) ifti.operatoris everNone. If it genuinely can't beNonehere, theor \"\"is misleading; if it can, an explicit early-return
is clearer than a malformed metric name.data_interval_start/_endsemantics narrow slightly: anything that isn't
adatetimeis nowNone, where the original preserved the value as-is.
Fine for typed ORM columns (datetime | None); flagging only in case some
caller passes a duck-typeddagrun.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Maciej Obuchowski <maciej.obuchowski@datadoghq.com>
Signed-off-by: Maciej Obuchowski <maciej.obuchowski@datadoghq.com>
The four `if not self.executor:` → `if self._executor is None:` changes in `1f2fb073e9` honored the spirit of potiuk's review nit but ignored his explicit guidance: "Don't fix in this PR." The original guard was unreachable as a log line but had a load-bearing side effect — accessing the lazy `executor` property *creates* the pool, which is what `submit_callable` then submits to. Replacing it with the side-effect-free `self._executor is None` short-circuited before pool creation, breaking `test_listener_on_dag_run_state_changes_configure_process_pool_size` in CI. Reverting; cleanup is appropriate as a separate PR. Signed-off-by: Maciej Obuchowski <maciej.obuchowski@datadoghq.com>
Uh oh!
There was an error while loading. Please reload this page.
…aints (#602) Bump apache-airflow-providers-openlineage to 2.16.0 in AF3.2.1 constraints. The default apache-airflow-providers-openlineage==2.14.0 has a known fork() bug that leads to "SSL error: decryption failed or bad record mac" due to inherited DB connection socket that's being used by two processes. Version 2.16.0 contains fix: apache/airflow#65677 * *The local development experience*: Package install succeeds: `[INFO] - Successfully installed apache-airflow-providers-openlineage-2.16.0 openlineage-integration-common-1.46.0 openlineage-python-1.46.0 openlineage-sql-1.46.0`. It's not installed by default - only if user requests it `requirements.txt`. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. Co-authored-by: Konstantin Lisitsyn <konlis@amazon.com>
When the Airflow scheduler processes externally-changed task states (orphaned Celery TIs adopted after a scheduler restart, UI/API state changes routed through
process_executor_events → handle_failure), the OpenLineage listener callsos.fork()in_fork_executeto emit the FAIL/COMPLETE event out-of-band.The forked child inherits the scheduler's SSL-wrapped Postgres connection pool and, because the AF3+ branch skipped
configure_orm(disable_connection_pool=True)(guarded byif not AIRFLOW_V_3_0_PLUS:from #47580 to avoid crashing on the worker'sairflow-db-not-allowed:///sentinel URL), the child issues DB queries over the same TLS socket as the parent, potentially desynchronizing the OpenSSL sequence counter and crashing the scheduler's very nextsession.flush()withpsycopg2.OperationalError: SSL error: decryption failed or bad record mac. This happened in our environment.This PR routes the scheduler-side "manual state change" emission through the existing
ProcessPoolExecutorthat DAG-run listeners already use (workers are initialized once via_executor_initializer, never share connections with the scheduler, and don't fork per event).Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 4.7 following the guidelines