fix: pass a timeout to the metadata heartbeat request

[RudraDudhat2509]

PR Type

• Bug fix
• New feature
• Core Runtime change (higher bar -- see CONTRIBUTING.md)
• Docs / tooling
• Refactoring

Summary

The metadata heartbeat now times out instead of blocking forever when the service accepts the connection but stops responding, so _ping can retry and back off like it was already written to.

Issue

Fixes #3360

Reproduction

Runtime: local
Commands to run:

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

Where evidence shows up:

Before (error / log snippet)

>       assert timeout is not None, "heartbeat post must pass a timeout to requests"
E       AssertionError: heartbeat post must pass a timeout to requests
E       assert None is not None
1 failed, 1 passed

After (evidence that fix works)

2 passed, 1 warning in 7.32s

The underlying hang, against a socket that accepts and never replies:

[1] no timeout  -> still blocked after 15s, never returns, never raises
[2] with timeout -> requests.exceptions.Timeout after 10s

Root Cause

_heartbeat posted without a timeout. requests has no default timeout, so if the metadata service accepts the connection but never replies, the post blocks indefinitely.

Two things follow from that. The except requests.exceptions.Timeout handler directly below the call is unreachable, since nothing can ever raise it. And _ping, which is the retry loop, only retries when _heartbeat raises HeartBeatException:

except HeartBeatException as e:
    retry_counter = retry_counter + 1
    time.sleep(1.5**retry_counter)

A call that neither returns nor raises never reaches that, so the retry counter and the 1.5**retry_counter backoff never run. The heartbeat thread stalls and the run stops heartbeating without surfacing anything.

Why This Fix Is Correct

The handler for this case already exists, it just could not be reached. Passing a timeout makes the existing Timeout branch reachable, which raises HeartBeatException, which is what _ping already knows how to retry. No new error paths are introduce by this fix, and there is no change to the retry logic.

(3.05, 10) is connect and read. 3.05 for connect follows the requests guidance of sitting just above a multiple of 3 so it does not line up with TCP retransmit windows.

Failure Modes Considered

1. Timeout too aggressive on a slow but healthy service. A slow response now raises instead of waiting indefinitely. This degrades to a retry rather than a failure, since _ping catches HeartBeatException and backs off before pinging again. 10s read is well above a normal heartbeat response.
2. Behaviour change for anyone relying on the current blocking. Previously a hung service silently stalled the thread. Now it raises and retries. That is the behaviour the existing Timeout handler and its error message were written for.
3. Value coupled to ping frequency. Considered tying the read timeout to default_frequency_secs, but the interval is server controlled through wait_time_in_seconds, so coupling to it would be fragile. Kept it a constant.

Tests

• Unit tests added/updated
• Reproduction script provided (required for Core Runtime)
• CI passes
• If tests are impractical: explain why below and provide manual evidence above

test_heartbeat_post_passes_a_timeout is the regression test, it fails on master. It asserts a positive timeout is passed rather than asserting a specific constant, so it does not lock in the value.

test_heartbeat_timeout_raises_heartbeat_exception passes either way. It documents that a timing out request surfaces as HeartBeatException so _ping can retry.

Full test/unit run is unchanged against master: 14 failed, 165 errors both before and after, all from optional deps missing locally, with 396 passing before and 398 after. Ran under WSL Ubuntu since metaflow imports fcntl and cannot be imported on Windows. black is clean on both files.

Non-Goals

Not touching the AWS sandbox STS call in aws_client.py, that is #2924 and has a PR open. Did not add a config knob for the timeout, and did not change the retry or backoff logic. tried to keep the change as scoped and minimal as possible

AI Tool Usage

• No AI tools were used in this contribution
• AI tools were used (describe below)

Claude Sonnet 5 was used. It wrote the change in heartbeat.py and the two tests, and found the bug while grepping for requests calls with no timeout. I reviewed all of it, ran the tests both ways, and can explain the causal chain and why the timeout value was chosen. other than that i ran all the setup and the code manually to see there was no degradation.

[greptile-apps]

Greptile Summary

The PR prevents metadata heartbeat requests from blocking indefinitely by applying finite connect and read timeouts, allowing the existing retry path to handle stalled services.

• Adds a (3.05, 10) timeout to heartbeat POST requests.
• Adds regression coverage for explicit timeout use and conversion of request timeouts into HeartBeatException.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness or security issues identified.

The heartbeat now exits stalled network reads through the existing exception and retry flow, and the added tests cover both timeout configuration and error translation.

Important Files Changed

Filename	Overview
metaflow/metadata_provider/heartbeat.py	Adds finite request timeouts while preserving the existing heartbeat exception and retry behavior; no actionable defect identified.
test/unit/test_metadata_heartbeat.py	Adds focused regression tests confirming that a positive timeout is supplied and timeout failures reach the existing heartbeat exception path.

_{Reviews (1): Last reviewed commit: "fix: pass a timeout to the metadata hear..." | Re-trigger Greptile}

[Shriprasad-P]

Reviewed the heartbeat failure path against master and #3360.

The root cause checks out: _heartbeat() calls requests.post without a timeout, so a peer that accepts the connection but never responds can block indefinitely. In that state the existing Timeout -> HeartBeatException -> _ping() retry/backoff path is never reached.

Adding a finite (connect, read) timeout makes the existing failure handling reachable without changing the retry logic, successful heartbeat path, or wait_time_in_seconds behavior. The regression test also does the right thing by asserting that a finite timeout exists without coupling the test to the exact timeout constant.

One minor observation: ConnectTimeout is still caught by the earlier ConnectionError branch, but it still becomes HeartBeatException and retries correctly, so that is pre-existing and non-blocking.

I don't see a blocking issue here. LGTM.

[Shriprasad-P]

Reviewed the heartbeat failure path against master and #3360.

The root cause checks out: _heartbeat() calls requests.post without a timeout, so a peer that accepts the connection but never responds can block indefinitely. In that state the existing Timeout -> HeartBeatException -> _ping() retry/backoff path is never reached.

Adding a finite (connect, read) timeout makes the existing failure handling reachable without changing the retry logic, successful heartbeat path, or wait_time_in_seconds behavior. The regression test also does the right thing by asserting that a finite timeout exists without coupling the test to the exact timeout constant.

One minor observation: ConnectTimeout is still caught by the earlier ConnectionError branch, but it still becomes HeartBeatException and retries correctly, so that is pre-existing and non-blocking.

I don't see a blocking issue here. LGTM.