Fix filter_tasks_by_metadata masking connection/timeout errors

[prabhaharanv]

PR Type

• Bug fix
• Core Runtime change (higher bar -- see CONTRIBUTING.md)

Summary

filter_tasks_by_metadata caught bare Exception and read e.http_code, which
only exists on ServiceException. A connection/timeout error from _request
therefore raised AttributeError, masking the real failure. Narrow the catch to
ServiceException.

Issue

Fixes #3316

Reproduction

Runtime: local (Client / metadata service call)

Commands to run:

# Mechanism (no live service needed) — reproduced in the added unit test:
python -m pytest test/unit/test_service_metadata_provider.py -v

With METAFLOW_SERVICE_URL pointed at an unreachable host, any Client call that
routes through filter_tasks_by_metadata fails with AttributeError instead of
the underlying connection error.

Where evidence shows up: the exception raised to the caller / test output

Before (real error is masked)

AttributeError: 'ConnectionError' object has no attribute 'http_code'

After (real error propagates)

requests.exceptions.ConnectionError: connection refused

Root Cause

_request re-raises transport errors (requests ConnectionError/Timeout)
unwrapped on its final retry — the bare except: ... raise at the end of its
retry loop does not convert them to ServiceException. filter_tasks_by_metadata
then caught bare Exception and accessed e.http_code, an attribute defined
only on ServiceException (set in its __init__). For any non-service error,
that attribute access raised AttributeError, discarding the original exception.
Every other handler in this file already catches ServiceException specifically.

Why This Fix Is Correct

Only ServiceException carries http_code, and only a service-level 404 should
be translated into the "upgrade your metadata service" message. Narrowing to
except ServiceException restores that invariant: service errors are handled as
before (404 → MetaflowInternalError, others re-raised), and transport errors
propagate unchanged instead of being turned into an AttributeError. The change
is minimal and brings this handler in line with the rest of the file.

Failure Modes Considered

1. A real service error stops being handled — it doesn't: ServiceException
   (including the 404 case) is still caught and handled identically; the added
   tests cover the 404 and non-404 service paths.
2. Swallowing vs. propagating transport errors — the fix intentionally lets
   ConnectionError/Timeout propagate to the caller (the prior behavior only
   ever produced an AttributeError, never a handled result), so no previously
   handled path is lost. raise e was changed to a bare raise to preserve the
   original traceback.

Tests

• Unit tests added/updated
• CI passes
• Reproduction script provided (covered by the unit tests above)

New test/unit/test_service_metadata_provider.py:

• test_filter_tasks_by_metadata_propagates_non_service_exception — fails before
  the fix (raises AttributeError), passes after (propagates ConnectionError).
• test_filter_tasks_by_metadata_missing_endpoint_raises_internal_error — 404 →
  MetaflowInternalError.
• test_filter_tasks_by_metadata_reraises_other_service_exception — non-404
  ServiceException propagates.

Non-Goals

No change to _request's retry/raise behavior or to any other handler; scope is
limited to the exception type caught in filter_tasks_by_metadata.

AI Tool Usage

• AI tools were used (describe below)

I found this bug (among several others) during independent analysis of the Client
API and metadata-service internals a few months ago, and noted that every other
handler in service.py uses except ServiceException while this one used bare
Exception. I used Claude to help draft the one-line fix, the regression tests,
and this description. I reviewed and understand the change, verified the
masking behavior and the fix myself, and can explain the root cause and the
failure modes considered.

[greptile-apps]

Greptile Summary

This PR corrects exception handling for metadata task filtering.

• Narrows the handler to ServiceException, allowing connection and timeout failures to propagate without being masked.
• Preserves the informative upgrade error for metadata-service 404 responses.
• Uses a bare re-raise for other service errors to preserve their original traceback.
• Adds regression tests covering transport errors, missing endpoints, and non-404 service errors.

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	Narrows endpoint error handling to service-level exceptions while preserving existing 404 translation and other service failures.
test/unit/test_service_metadata_provider.py	Adds focused regression coverage for transport, 404, and non-404 exception paths.

_{Reviews (2): Last reviewed commit: "Merge branch 'master' into fix/filter-ta..." | Re-trigger Greptile}

[codecov]

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #3317   +/-   ##
=========================================
  Coverage          ?   30.60%           
=========================================
  Files             ?      381           
  Lines             ?    52639           
  Branches          ?     9287           
=========================================
  Hits              ?    16110           
  Misses            ?    35330           
  Partials          ?     1199           

☔ 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.

[Shriprasad-P]

Review

PR: Fix filter_tasks_by_metadata masking connection/timeout errors

Touched: metaflow/plugins/metadata_providers/service.py, test/unit/test_service_metadata_provider.py

• Addresses a concrete correctness/reliability issue based on the title and diff.
• Includes or touches tests — good for locking the behavior.
• Size looks manageable (+57/-3).

Commenting as a drive-by reviewer after reading the diff. Happy to look again if maintainers want a deeper pass on a specific file.