potiuk commented on code in PR #69504:
URL: https://github.com/apache/airflow/pull/69504#discussion_r3682324102
##########
task-sdk/tests/task_sdk/execution_time/conftest.py:
##########
@@ -14,10 +14,21 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
from __future__ import annotations
import sys
+
+if sys.platform == "win32":
+ import types
+
+ fcntl = types.ModuleType("fcntl")
Review Comment:
This installs a **no-op `fcntl` into `sys.modules`** for every test under
`task_sdk/execution_time`. `flock` becomes `lambda *args, **kwargs: None`, so
any production code exercised by these tests that relies on file locking for
mutual exclusion silently stops locking — and the tests pass anyway.
That's a harness that reports success for behaviour it has disabled. If
something under here genuinely needs locking, this hides exactly the bug you'd
want a test to catch; if nothing does, the stub isn't needed.
It's also invisible to CI: the `sys.platform == "win32"` guard means Linux
runs never execute it, so a green pipeline says nothing about whether it's safe.
If Windows test support is wanted, the honest approaches are to skip the
tests that need `fcntl` on Windows, or to put a real platform abstraction in
the production code and test that — not to fake a syscall module out from under
the code under test.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
shared/observability/src/airflow_shared/observability/metrics/stats.py:
##########
@@ -48,7 +48,8 @@ def _reset_backend_after_fork() -> None:
_backend = None
-os.register_at_fork(after_in_child=_reset_backend_after_fork)
+if hasattr(os, "register_at_fork"):
Review Comment:
Reasonable guard in isolation, but this is `shared/observability` — a
library consumed by airflow-core and task-sdk — so it's a production change to
shared code arriving inside a PR titled as a Sentry fix. It belongs in the
split-out Windows PR, where a reviewer looking at fork/metrics behaviour will
actually see it.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
