Skip to content

Anchor service metadata patterns so foreach paths do not prefix-match - #3357

Open
winklemad wants to merge 2 commits into
Netflix:masterfrom
winklemad:fix/foreach-path-prefix-match-3341
Open

Anchor service metadata patterns so foreach paths do not prefix-match#3357
winklemad wants to merge 2 commits into
Netflix:masterfrom
winklemad:fix/foreach-path-prefix-match-3341

Conversation

@winklemad

@winklemad winklemad commented Aug 31, 2026

Copy link
Copy Markdown

PR Type

  • Bug fix

Summary

Handles the OSS service-provider side of #3341. Task.parent_tasks and
Task.child_tasks pass foreach execution-path patterns to the metadata
service, whose regex matching is unanchored. An exact path such as middle:1
could therefore also select middle:10 and middle:11.

The local-provider side is handled separately by #3342.

Root Cause

ServiceMetadataProvider.filter_tasks_by_metadata forwarded the raw pattern
to the metadata service:

query_params["pattern"] = pattern

The service applies that pattern as an unanchored regex search.

Fix

Wrap the forwarded pattern in full-string anchors:

query_params["pattern"] = "^(?:%s)$" % pattern

This makes middle:1 exact while preserving the descendant pattern
middle:1,.*. The existing .* match-all case remains short-circuited and
sends no pattern.

Tests

test/unit/test_service_metadata_provider.py covers:

  • exact path anchoring;
  • descendant pattern anchoring;
  • the match-all short circuit.

Run with:

python -m pytest test/unit/test_service_metadata_provider.py -q

Non-Goals

AI Tool Usage

  • AI tools were used.

Tool disclosed by the author: Claude Code, used to help implement the original
change and tests. The service-only rescope retains that implementation.

…r-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 Netflix#3341
@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR anchors metadata-service task-filter patterns so exact foreach execution paths no longer select tasks whose indices merely share a prefix.

  • Wraps non-match-all service patterns with full-string regex anchors.
  • Preserves the existing match-all behavior by omitting the pattern query parameter.
  • Adds unit coverage for exact, descendant, and match-all forwarding.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
metaflow/plugins/metadata_providers/service.py Anchors forwarded task-metadata regex patterns while preserving the match-all short circuit.
test/unit/test_service_metadata_provider.py Verifies exact and descendant patterns are anchored and match-all remains unfiltered.

Reviews (2): Last reviewed commit: "Scope fix to service metadata provider" | Re-trigger Greptile

@nileshpatil6

Copy link
Copy Markdown

Thanks for the credit and for laying out the overlap so clearly, that was a considerate way to handle it.

The split you proposed works well from my side: #3342 has been open since Aug 16 with the local provider fix (regex.match to regex.fullmatch) and a prefix-matching test that fails on master, and the service-side anchoring here is genuinely the piece it does not cover. So landing #3342 for the local half and scoping this one to the service provider seems like the cleanest path.

@talsperre happy to go with whatever ordering you prefer.

@winklemad

Copy link
Copy Markdown
Author

Thanks @nileshpatil6, likewise, your #3342 nailed the local-provider side. Hoping we can get both landed.

talsperre added a commit to nileshpatil6/metaflow that referenced this pull request Aug 31, 2026
@talsperre talsperre changed the title Match foreach execution paths in full so an index prefix does not over-select (#3341) Anchor service metadata patterns so foreach paths do not prefix-match Aug 31, 2026
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@ddef84a). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3357   +/-   ##
=========================================
  Coverage          ?   31.02%           
=========================================
  Files             ?      382           
  Lines             ?    52717           
  Branches          ?     9303           
=========================================
  Hits              ?    16353           
  Misses            ?    35153           
  Partials          ?     1211           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@winklemad

Copy link
Copy Markdown
Author

Thanks for the review @talsperre 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants