From b53499cf4037e1aa78212b31be84faa69aad5dba Mon Sep 17 00:00:00 2001 From: Madan Kumar Date: Mon, 31 Aug 2026 06:21:19 +0530 Subject: [PATCH 1/2] Match foreach execution paths in full so an index prefix does not over-select Client.parent_tasks/child_tasks build a filter pattern from a task's foreach-execution-path and query filter_tasks_by_metadata. The local provider matched it with regex.match, which only anchors the start, so an exact path like "middle:1" also matched "middle:10" and "middle:11" once a foreach had 10+ splits, and parent_tasks/child_tasks returned the wrong tasks. Use regex.fullmatch in the local provider, and anchor the pattern the service provider forwards to the metadata service as ^(?:pattern)$ so it matches in full there too (the existing ".*" and "{path},.*" patterns already require the service to treat this as a regex). The ".*" match-all short-circuit and the "{path},.*" descendant form are unchanged. Fixes #3341 --- metaflow/plugins/metadata_providers/local.py | 4 +- .../plugins/metadata_providers/service.py | 8 +++- test/unit/test_local_metadata_provider.py | 38 +++++++++++++++++++ test/unit/test_service_metadata_provider.py | 38 +++++++++++++++++++ 4 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 test/unit/test_service_metadata_provider.py diff --git a/metaflow/plugins/metadata_providers/local.py b/metaflow/plugins/metadata_providers/local.py index 424812c810f..7767faafab9 100644 --- a/metaflow/plugins/metadata_providers/local.py +++ b/metaflow/plugins/metadata_providers/local.py @@ -261,7 +261,9 @@ def filter_tasks_by_metadata( if any( meta.get("field_name") == field_name - and regex.match(meta.get("value", "")) + # fullmatch, not match: an exact foreach path like "middle:1" + # must not also match "middle:10"/"middle:11" (prefix match) + and regex.fullmatch(meta.get("value", "")) for meta in metadata ): matching_task_pathspecs.append( diff --git a/metaflow/plugins/metadata_providers/service.py b/metaflow/plugins/metadata_providers/service.py index c9db2a31822..10f7060a6ca 100644 --- a/metaflow/plugins/metadata_providers/service.py +++ b/metaflow/plugins/metadata_providers/service.py @@ -363,7 +363,13 @@ def filter_tasks_by_metadata( if field_name: query_params["metadata_field_name"] = field_name if pattern: - query_params["pattern"] = pattern + # Anchor so the service matches the value in full. Without this an + # exact foreach path like "middle:1" also matches "middle:10" on the + # service backend, the same prefix bug fixed locally with fullmatch. + # "^(?:...)$" is an exact match whether the service applies + # re.match/search/fullmatch; the existing ".*" and "{path},.*" + # patterns already require the service to treat this as a regex. + query_params["pattern"] = "^(?:%s)$" % pattern url = ServiceMetadataProvider._obj_path(flow_name, run_id, step_name) url = f"{url}/filtered_tasks?{urlencode(query_params)}" diff --git a/test/unit/test_local_metadata_provider.py b/test/unit/test_local_metadata_provider.py index be080c5f115..6f62a006e06 100644 --- a/test/unit/test_local_metadata_provider.py +++ b/test/unit/test_local_metadata_provider.py @@ -29,3 +29,41 @@ def test_deduce_run_id_from_meta_dir(): case["meta_path"], case["sub_type"] ) assert case["expected_run_id"] == actual_run_id + + +def test_filter_tasks_by_metadata_matches_exact_foreach_path(monkeypatch): + # A foreach with 10+ splits produces execution paths like "middle:1" and + # "middle:10". regex.match only anchors the start, so querying "middle:1" + # also pulled in "middle:10"/"middle:11", giving Task.parent_tasks and + # child_tasks the wrong tasks (issue #3341). + paths = { + "t1": "middle:1", + "t2": "middle:10", + "t3": "middle:11", + "t4": "middle:1,inner:0", + } + + def fake_get_object(cls, obj_type, sub_type, filters, attempt, *args): + if sub_type == "task": + return [{"task_id": task_id} for task_id in paths] + task_id = args[-1] + return [{"field_name": "foreach-execution-path", "value": paths[task_id]}] + + monkeypatch.setattr( + LocalMetadataProvider, "get_object", classmethod(fake_get_object) + ) + + def matches(pattern): + return LocalMetadataProvider.filter_tasks_by_metadata( + "Flow", "run", "middle", "foreach-execution-path", pattern + ) + + # an exact path must not pull in the longer indices that start with it + assert matches("middle:1") == ["Flow/run/middle/t1"] + assert matches("middle:10") == ["Flow/run/middle/t2"] + # a nested foreach still resolves its children through the "{path},.*" form + assert matches("middle:1,.*") == ["Flow/run/middle/t4"] + # the match-all pattern keeps returning every task + assert sorted(matches(".*")) == sorted( + f"Flow/run/middle/{task_id}" for task_id in paths + ) diff --git a/test/unit/test_service_metadata_provider.py b/test/unit/test_service_metadata_provider.py new file mode 100644 index 00000000000..4bcb57249e8 --- /dev/null +++ b/test/unit/test_service_metadata_provider.py @@ -0,0 +1,38 @@ +from urllib.parse import parse_qs, urlparse + +from metaflow.plugins.metadata_providers.service import ServiceMetadataProvider + + +def _forwarded_pattern(monkeypatch, pattern): + # The service provider builds the request URL and lets the metadata service + # do the matching, so we capture what pattern it forwards. + captured = {} + + def fake_request(cls, callback, url, method, *args, **kwargs): + captured["url"] = url + return [], None + + monkeypatch.setattr(ServiceMetadataProvider, "_request", classmethod(fake_request)) + ServiceMetadataProvider.filter_tasks_by_metadata( + "Flow", "run", "middle", "foreach-execution-path", pattern + ) + query = parse_qs(urlparse(captured["url"]).query) + return query.get("pattern", [None])[0] + + +def test_filter_tasks_by_metadata_anchors_exact_path(monkeypatch): + # An exact foreach path must be anchored so "middle:1" cannot prefix-match + # "middle:10"/"middle:11" on the service backend (issue #3341), matching the + # local provider's fullmatch behavior. + assert _forwarded_pattern(monkeypatch, "middle:1") == "^(?:middle:1)$" + + +def test_filter_tasks_by_metadata_anchors_descendant_path(monkeypatch): + # The "{path},.*" child form is still anchored; the trailing .* keeps + # matching a task's descendants. + assert _forwarded_pattern(monkeypatch, "middle:1,.*") == "^(?:middle:1,.*)$" + + +def test_filter_tasks_by_metadata_match_all_sends_no_pattern(monkeypatch): + # ".*" is short-circuited to "match every task", so no pattern is forwarded. + assert _forwarded_pattern(monkeypatch, ".*") is None From 85e071e0a0c824edce2c6da12a972f5416ffbb2d Mon Sep 17 00:00:00 2001 From: Shashank Srikanth Date: Mon, 31 Aug 2026 19:18:03 +0000 Subject: [PATCH 2/2] Scope fix to service metadata provider --- metaflow/plugins/metadata_providers/local.py | 4 +- .../plugins/metadata_providers/service.py | 8 +--- test/unit/test_local_metadata_provider.py | 38 ------------------- test/unit/test_service_metadata_provider.py | 34 +++++------------ 4 files changed, 13 insertions(+), 71 deletions(-) diff --git a/metaflow/plugins/metadata_providers/local.py b/metaflow/plugins/metadata_providers/local.py index 7767faafab9..424812c810f 100644 --- a/metaflow/plugins/metadata_providers/local.py +++ b/metaflow/plugins/metadata_providers/local.py @@ -261,9 +261,7 @@ def filter_tasks_by_metadata( if any( meta.get("field_name") == field_name - # fullmatch, not match: an exact foreach path like "middle:1" - # must not also match "middle:10"/"middle:11" (prefix match) - and regex.fullmatch(meta.get("value", "")) + and regex.match(meta.get("value", "")) for meta in metadata ): matching_task_pathspecs.append( diff --git a/metaflow/plugins/metadata_providers/service.py b/metaflow/plugins/metadata_providers/service.py index 10f7060a6ca..1616d143d76 100644 --- a/metaflow/plugins/metadata_providers/service.py +++ b/metaflow/plugins/metadata_providers/service.py @@ -363,12 +363,8 @@ def filter_tasks_by_metadata( if field_name: query_params["metadata_field_name"] = field_name if pattern: - # Anchor so the service matches the value in full. Without this an - # exact foreach path like "middle:1" also matches "middle:10" on the - # service backend, the same prefix bug fixed locally with fullmatch. - # "^(?:...)$" is an exact match whether the service applies - # re.match/search/fullmatch; the existing ".*" and "{path},.*" - # patterns already require the service to treat this as a regex. + # The service performs an unanchored regex search, so anchor the + # pattern to match the local provider's fullmatch behavior. query_params["pattern"] = "^(?:%s)$" % pattern url = ServiceMetadataProvider._obj_path(flow_name, run_id, step_name) diff --git a/test/unit/test_local_metadata_provider.py b/test/unit/test_local_metadata_provider.py index 6f62a006e06..be080c5f115 100644 --- a/test/unit/test_local_metadata_provider.py +++ b/test/unit/test_local_metadata_provider.py @@ -29,41 +29,3 @@ def test_deduce_run_id_from_meta_dir(): case["meta_path"], case["sub_type"] ) assert case["expected_run_id"] == actual_run_id - - -def test_filter_tasks_by_metadata_matches_exact_foreach_path(monkeypatch): - # A foreach with 10+ splits produces execution paths like "middle:1" and - # "middle:10". regex.match only anchors the start, so querying "middle:1" - # also pulled in "middle:10"/"middle:11", giving Task.parent_tasks and - # child_tasks the wrong tasks (issue #3341). - paths = { - "t1": "middle:1", - "t2": "middle:10", - "t3": "middle:11", - "t4": "middle:1,inner:0", - } - - def fake_get_object(cls, obj_type, sub_type, filters, attempt, *args): - if sub_type == "task": - return [{"task_id": task_id} for task_id in paths] - task_id = args[-1] - return [{"field_name": "foreach-execution-path", "value": paths[task_id]}] - - monkeypatch.setattr( - LocalMetadataProvider, "get_object", classmethod(fake_get_object) - ) - - def matches(pattern): - return LocalMetadataProvider.filter_tasks_by_metadata( - "Flow", "run", "middle", "foreach-execution-path", pattern - ) - - # an exact path must not pull in the longer indices that start with it - assert matches("middle:1") == ["Flow/run/middle/t1"] - assert matches("middle:10") == ["Flow/run/middle/t2"] - # a nested foreach still resolves its children through the "{path},.*" form - assert matches("middle:1,.*") == ["Flow/run/middle/t4"] - # the match-all pattern keeps returning every task - assert sorted(matches(".*")) == sorted( - f"Flow/run/middle/{task_id}" for task_id in paths - ) diff --git a/test/unit/test_service_metadata_provider.py b/test/unit/test_service_metadata_provider.py index 4bcb57249e8..568f4cc3516 100644 --- a/test/unit/test_service_metadata_provider.py +++ b/test/unit/test_service_metadata_provider.py @@ -3,9 +3,7 @@ from metaflow.plugins.metadata_providers.service import ServiceMetadataProvider -def _forwarded_pattern(monkeypatch, pattern): - # The service provider builds the request URL and lets the metadata service - # do the matching, so we capture what pattern it forwards. +def test_filter_tasks_by_metadata_anchors_patterns(monkeypatch): captured = {} def fake_request(cls, callback, url, method, *args, **kwargs): @@ -13,26 +11,14 @@ def fake_request(cls, callback, url, method, *args, **kwargs): return [], None monkeypatch.setattr(ServiceMetadataProvider, "_request", classmethod(fake_request)) - ServiceMetadataProvider.filter_tasks_by_metadata( - "Flow", "run", "middle", "foreach-execution-path", pattern - ) - query = parse_qs(urlparse(captured["url"]).query) - return query.get("pattern", [None])[0] + def forwarded_pattern(pattern): + ServiceMetadataProvider.filter_tasks_by_metadata( + "Flow", "run", "middle", "foreach-execution-path", pattern + ) + query = parse_qs(urlparse(captured["url"]).query) + return query.get("pattern", [None])[0] -def test_filter_tasks_by_metadata_anchors_exact_path(monkeypatch): - # An exact foreach path must be anchored so "middle:1" cannot prefix-match - # "middle:10"/"middle:11" on the service backend (issue #3341), matching the - # local provider's fullmatch behavior. - assert _forwarded_pattern(monkeypatch, "middle:1") == "^(?:middle:1)$" - - -def test_filter_tasks_by_metadata_anchors_descendant_path(monkeypatch): - # The "{path},.*" child form is still anchored; the trailing .* keeps - # matching a task's descendants. - assert _forwarded_pattern(monkeypatch, "middle:1,.*") == "^(?:middle:1,.*)$" - - -def test_filter_tasks_by_metadata_match_all_sends_no_pattern(monkeypatch): - # ".*" is short-circuited to "match every task", so no pattern is forwarded. - assert _forwarded_pattern(monkeypatch, ".*") is None + assert forwarded_pattern("middle:1") == "^(?:middle:1)$" + assert forwarded_pattern("middle:1,.*") == "^(?:middle:1,.*)$" + assert forwarded_pattern(".*") is None