fix: improve Windows compatibility for worker host configuration in tests - #671
Draft
Edwardvaneechoud wants to merge 21 commits into
Draft
fix: improve Windows compatibility for worker host configuration in tests#671Edwardvaneechoud wants to merge 21 commits into
Edwardvaneechoud wants to merge 21 commits into
Conversation
…error handling in tests
✅ Deploy Preview for flowfile-wasm canceled.
|
…ion param drop_all leaves alembic_version behind, so a failed test-DB unlink (WinError 32) stranded a stamped-but-empty DB and poisoned the next run with "no such table: users". Drop it and dispose the engine first. The execution_location fixture skipped "remote" only when no worker was listening; with FLOWFILE_OFFLOAD_TO_WORKER=0 a worker still listens while get_prio_execution_location silently downgrades remote to local, running the same path twice. Gate on the global settings as well. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Escape filesystem paths in generated Python via _py_path (json.dumps): backslash paths raised unicodeescape SyntaxErrors or were silently corrupted (\a \f \t) in the emitted literals. Output for POSIX paths is byte-identical. - Write the PyInstaller .spec and the designer's custom-node files with an explicit encoding/newline so the artifacts are byte-stable across platforms (cp1252 .spec bytes; CRLF-unstable node round-trip and hashes). - Reconfigure stdout/stderr to UTF-8 in exported projects' main.py: printing Polars frames (box-drawing chars) crashed on non-UTF-8 console codepages, and the exporter tests now decode the child as UTF-8. - Make 8 tests platform-correct without weakening assertions (SIGKILL split, read-only .git rmtree, native-path assertions, catalog URI join). test_path_security.py::test_traversal_in_external_path_is_rejected stays red deliberately: the validator is correct (Windows %TEMP% resolves inside an allowed data root); the test-side fix is pending review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The traversal test asserted an escape that only exists where the temp dir sits outside the allowed data roots. storage.user_data_directory falls back to Path.home(), and on Windows %TEMP% lives inside %USERPROFILE% — so the "escape" genuinely resolved into an allowed root and the validator correctly accepted it. No bypass: the realpath+commonpath containment check survived adversarial probing (junctions, symlinks, 8.3 names, UNC, \\?\ prefixes, mixed separators) on Windows and has no string-parsing seam on POSIX. Pinning the roots under tmp_path makes the traversal a genuine escape on every platform (including HOME=/root container runs) and adds a guard that keeps the rejection tests from passing vacuously under over-restricted roots. A gutted containment check still turns the rejection tests red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test_auth_e2e.py's local is_docker_available() only pinged the daemon. GitHub windows-latest runners carry Docker in Windows-container mode, and the daemon is up on some runner instances and down on others — when it is up, the module-scope fixture tries to build the linux/amd64 core image and all 11 tests error at setup with "no matching manifest for windows(...)/amd64" instead of skipping. Checking the daemon's OSType makes the skip deterministic on Windows-container daemons while leaving Linux/macOS CI unchanged and keeping the suite runnable for Windows developers on Docker Desktop/WSL2 (which reports OSType linux). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Temporary measurement commit for the Windows-vs-Linux CI speed investigation: --durations=300 on the core/worker pytest steps of the matrix and Windows backend jobs produces a per-test duration table on every platform, giving a Windows/Linux ratio per test. A concentrated ratio points at specific slow modules; a flat ~4x points at environment-wide overhead (AV scanning, process spawn, disk). The coverage and kernel jobs are deliberately untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mote CI forensics (run 31744430520) measured a ~11.4-13.6s worker round-trip tax per remote node execution on Windows runners (fresh spawned interpreter per task) — and 132 ordinary graph tests were paying it only because FlowGraphConfig.execution_location's default_factory resolves to "remote" whenever a worker is listening. Repointing that one factory to "local" in conftest (at import time, before collection) makes incidental flows run in-process while leaving get_global_execution_location itself untouched, so explicit execution_location="remote" and the 129 execution_location-fixture [remote] params keep running genuinely remote. Verified locally: test_basic_filter.py 176s -> 19s, test_setting_updators 61s -> 19s; [remote] params still run and still show the remote tax; skip counts unchanged. test_yaml_io's number_of_records assertion only passed on remote runs (local keeps the frame lazy and reports the -1 sentinel); it now asserts the same property via count(), with the remote ride-along contract still pinned by test_flowfile.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fallout from defaulting test flows to local (ddff8cf), caught by CI run 31754475336 on all platforms: test_instant_function_result_after_run asserts the after-run instant preview, which is served only from results.example_data_path — written exclusively by the worker-backed sampler/fetcher paths; test_add_database_input asserts needs_run clears, which on the default remote branch requires the worker to hold a completed result for the node hash. Both tests exist to cover those remote pathways, so their flows now request remote explicitly (via the FlowGraph.execution_location setter, which resets nodes). No assertion changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Install Dependencies costs ~4m26s on windows-latest because setup-python's pip cache does not cover poetry-installed packages. The venv now lives in-project (workspace drive) and is cached under an exact-match key (runner.os + resolved interpreter patch version + poetry.lock hash, no restore-keys — poetry install does not prune, so a stale-lock restore would silently diverge). poetry install still runs unconditionally to self-heal drift. The 11 mock-service steps are removed from this job only: every test_utils fixture hardcodes Docker as unavailable on Windows CI, so each step was a guaranteed no-op that still paid a poetry+interpreter spin-up. The matrix and coverage jobs keep them unchanged. Expected: ~4-5 min off every warm Windows run; a cache miss costs exactly today's cold install. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every mp spawn child re-executes the parent's __main__. The Poetry console script (a plain file, __spec__ None) made spawn re-run it via init_main_from_path, so each child re-imported the entire FastAPI app (fastapi, uvicorn, openpyxl, faker, httpx) before doing any work: 1,249 modules and ~2,390ms per warm round trip on Windows. Three coordinated changes take spawn's documented free path (_fixup_main_from_name returns immediately for a ".__main__" name): a thin __main__.py; a parent-only __spec__ stamp in __init__.py (only when __spec__ is None — a repo-wide grep found no reader); and the console script now points at cli.py, whose imports are trivial. The Dockerfile CMD moves to `python -m flowfile_worker` for the same reason. Measured after: 513 modules, ~878ms (-63%). The unified pip path (include_worker_routes) inherits the stamp — verified, no second seam needed. test_import_purity.py now reproduces the real console-script child shape and fails if the launcher re-import ever comes back (verified by reverting the stamp). Existing dev environments need `poetry install --only-root` to regenerate the console script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Worker children POST every node log line to core's /raw_logs (flow_logger.py, connect timeout 2s). The core suite runs core in-process via TestClient, so nothing listens on 63578 — and on Windows a refused loopback connect burns the full ~2s timeout instead of failing instantly, costing ~7 lines x 2s on every worker-backed test. Measured: each [remote] contract test call drops 14.4s -> 1.9s; a 112-test worker-backed sweep drops 6m17s -> 1m45s. A stdlib ThreadingHTTPServer on 127.0.0.1:63578 (autouse session fixture, started before the worker) keeps the real delivery path executing end to end at loopback speed. Guards: the bind itself is the already-in-use check (real listeners win); SO_REUSEADDR stays off on Windows, where it can steal an actively-listening port; sessions that legitimately claim the core port (kernel_manager_with_core, the auth E2E Docker suite) suppress the sink at collection time; GETs 404 so a health probe never mistakes it for core. FLOWFILE_TEST_LOG_SINK=0 disables it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keeps the Windows job's mock-step removal and venv cache; adopts main's new Windows 'Run pytest for shared' step (#670) in its intended position. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Defer the sql_utils/delta_utils re-exports in shared/__init__ and the models import in flowfile_worker/__init__ behind module __getattr__. Boot-path win, measured on Win11/py3.11: `import shared` 311ms -> 24ms (241 -> 95 modules), bare `import flowfile_worker` 700ms -> 27ms (498 -> 102), scheduler boot -187ms; flowfile/web's `from flowfile_worker import CACHE_DIR, mp_context` rides the same cut. The public surface is preserved exactly: attribute access (including shared.sql_utils / shared.delta_utils / shared.db_dialects and the five worker names the old eager chain bound), dir(), star-import (flowfile_worker gains an explicit __all__ pinned to the previous star surface), and get_type_hints(flowfile_worker) all behave as before. New fresh-interpreter tests (test_lazy_export_surface.py) pin that surface; the extended purity tests pin the laziness itself. The spawned worker child is unchanged (513 modules before and after): funcs.py reaches models/pydantic/sql_utils/delta_utils through its own module-top imports, so the Phase-2 design's per-spawn target (~878ms -> ~420ms) is not reachable at the package-init layer. That work is deferred to Phase 2b (split RawLogInput out of models.py). The child gates ratchet regardless (module ceiling 700 -> 550, +sqlalchemy in the forbidden list). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The sink was inert on every full CI run: session_claims_core_port suppressed it whenever test_auth_e2e.py items were merely collected, and on Windows CI those items skip only at fixture runtime (no Docker), so the whole session kept paying ~2s of refused-connect stall per shipped worker log line (~11.4s per worker-backed test instead of ~1.9s; an estimated 25-35 min of the Windows core step). The claim now consults the claimer's own skip predicate — read off the collected item's module for test_auth_e2e.py, imported lazily for the kernel_manager_with_core fixture — and fails closed: a missing, unimportable, or raising predicate counts as "will run" and suppresses the sink, preserving today's behavior everywhere the claimers really run (ubuntu keeps suppressing; Windows/macOS CI now get the sink). The conftest hook is trylast so the claim sees the post-deselection item set: pytest's own -m/-k filtering runs after a plain conftest hook, and 15 deselected kernel items were reaching the old check. Measured on the executor sweep (107 tests, same tree): 132s with the sink vs 372s with it disabled. The sink itself had no tests; it now has both-direction coverage, fail-closed proofs, a predicate drift pin and a hook-order pin (test_core_log_sink.py, 11 tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
--durations=300 was saturated: the 300th-slowest test in run 31775139898 still took 3.03s, hiding everything below it (~19% of the step's time). Shard rebalancing needs the full table; --durations-min 0.05 keeps the output to a few thousand lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The child's entry surface is funcs.py, which pulled the entire graph
through its own module-top imports regardless of the task: models (->
pydantic + the external-source models), flow_logger -> models (solely
for RawLogInput), sql_source models -> shared.sql_utils, and
pl_fuzzy_frame_match. Split RawLogInput into a pydantic-free
log_models.py (models re-exports it; the wire payload to core's
/raw_logs is byte-identical) and import the heavy names at use inside
the tasks that need them; polars stays at module top. Children are
single-task processes, so the cost moves off every spawn and onto only
the tasks that use those deps.
Measured (Win11/py3.11, console-script parent shape, median of 35
interleaved runs per side): warm funcs.store round trip 763.8ms ->
456.7ms (-40%), child module set 513 -> 345. An independent verifier
harness reproduced the ratio (-42%) with non-overlapping ranges and
proved import-at-use completeness at the bytecode level. The purity
gates now assert the child surface for real: pydantic and
pl_fuzzy_frame_match root-forbidden in the child, flowfile_worker.models
and shared.{sql_utils,db_dialects,delta_utils} exact-name-forbidden,
module ceiling 550 -> 380.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pytest-cov's .pth hook bootstraps the tracer into every spawned child (+166 modules on the CI coverage job, +210 locally -- the tax is platform-dependent, so a constant allowance would drift), which tripped CHILD_MODULE_CEILING on the coverage job while every uninstrumented leg passes. Gate the ceiling on the child's own module list not containing coverage; the forbidden-root and exact-name purity checks stay unconditional and were proven to still bite under instrumentation (re-eagerizing models fails the gate in both shapes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Split backend-tests-windows into three shard jobs - core-a (flowfile_core/tests/flowfile + project + test_migration.py, 817s measured), core-b (the rest of flowfile_core/tests via --ignore, 841s), and pkg-other (the shared/frame/worker/CLI suites, 368s) - balanced from run 31878578384's full durations table (2.8% imbalance). The node-ID sets partition the monolith exactly: 3849 + 3015 = 6864, disjoint, and both shards are order-preserving subsequences of the monolith's collection order. core-b is defined by --ignore so a newly added test file always lands in a shard instead of silently vanishing. The required branch-protection context backend-tests-windows survives as a fan-in job that checks every needs result explicitly: red unless detect-changes succeeded and either every shard succeeded (when the path filters selected the group) or every shard was skipped (when they excluded it). A skipped-but-required shard is red, never silently green. A TEMPORARY workflow_dispatch canary input (fail-shard-a / skip-shard-a) exists to prove both red directions on real dispatches; it is removed in a follow-up commit once the canary runs are recorded. Projected Windows critical path: 37.3 min -> ~17.4 min. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both red directions are proven on real dispatches: run 31882370276 (canary=fail-shard-a) turned the backend-tests-windows fan-in red on a failed shard, and run 31883873983 (canary=skip-shard-a) turned it red on a skipped-but-required shard - the case a plain needs: fan-in reports green. The normal PR run 31882372500 was green with exact test conservation across the shards (3849 + 3015 = 6864 selected, 6682 passed, 177 skipped, zero order-dependence failures) and a 16.1-minute Windows critical path. core-a's if: is restored byte-identical to core-b's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request improves the reliability of test execution across platforms, especially for Windows CI, by adjusting how the worker host is determined and how worker startup failures are handled. The changes ensure that tests fail clearly if the worker cannot start, preventing false-positive results on Windows.
Platform-specific worker host configuration:
WORKER_HOSTin bothflowfile_core/tests/conftest.pyandflowfile_core/tests/flowfile/external_sources/test_rest_api_flow_graph.pyto use"127.0.0.1"instead of"0.0.0.0"on Windows, addressing loopback alias differences between platforms. [1] [2]import platformwhere necessary to support platform checks.Test failure handling and messaging:
managed_workerfixture to callpytest.exit(which fails the test suite) instead ofpytest.skipwhen the worker fails to start, ensuring that a broken worker causes a CI failure rather than a false pass. The error message now explicitly mentions the host/port and the option to setSKIP_WORKER_TESTS=1to bypass the worker.