feast-dev / feast-dev/feast

Remote registry gRPC client sets no deadline and no keepalive, and neither is configurable: a blackholed connection hangs the caller indefinitely

Open
#6,665 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
7.3k
Forks
1.4k
Avg merge
3d 16h
Merged PRs (30d)
15

Description

## Expected Behavior

A registry RPC issued by `RemoteRegistry` should fail within a bounded time when
the connection to the registry server stops making progress, and the bound
should be configurable through `RemoteRegistryConfig`.

## Current Behavior

`RemoteRegistry` builds a channel with no keepalive and issues every RPC with no
deadline, so a connection that stops delivering data but stays `ESTABLISHED`
leaves the caller parked in `epoll_wait` forever.

Three things combine:

1. `RemoteRegistryConfig` (`sdk/python/feast/infra/registry/remote.py`) exposes
`path`, `cert`, `is_tls`, `client_cert`, `client_key` and `authority`. There
is no timeout field and no keepalive field.
2. `RemoteRegistry._create_grpc_channel` populates `options` only with
`grpc.default_authority`. No `grpc.keepalive_time_ms` or related option is set.
3. None of the RPC call sites pass a deadline. `remote.py` has 50
`self.stub.(...)` calls on `v0.64.0` and 51 on `v0.65.0`, with zero
occurrences of `timeout=` in either.

There is also no supported way to substitute an implementation.
`get_registry_config_from_type` validates `registry_type` against the hardcoded
`REGISTRY_CLASS_FOR_TYPE` map with the comment `# We do not support custom
registry's right now`, and `FeatureStore._create_registry` instantiates
`RemoteRegistry` directly, so a subclass is never reachable. This is unlike the
online store, offline store and batch engine, which all accept a dotted class
path.

One related footgun: `RegistryConfig` extends `FeastBaseModel`, whose
`model_config` is `extra="allow"`. An invented key such as `grpc_options` in the
`registry` block therefore validates cleanly and is silently ignored, because
nothing reads it. A user attempting to configure this gets no error.

**Observed impact.** In a long-running batch job that writes to the offline
store, a registry fetch on a blackholed network path never returns. The writer
thread never completes, the write queue fills, every producer blocks on it, and
the pod sits at roughly 1m CPU with 0 restarts until the node is reclaimed.
Nothing in the logs indicates a stuck RPC, because no error is ever raised.

## Steps to reproduce

1. Configure a remote registry:

```yaml
project: demo
registry:
registry_type: remote
path: registry.example:80
```

2. Make the registry endpoint blackhole traffic after the TCP connection is
established — for example drop packets to that address with a firewall rule
(`iptables -A OUTPUT -d -p tcp -j DROP`) after the first
successful call, or route through a proxy that accepts the connection and then
stops responding.

3. Call any registry method, e.g. `store.get_feature_view("some_fv")`.

4. The call does not return. `ss -tnp` shows the socket still `ESTABLISHED`;
`py-spy dump` shows the thread in `epoll_wait` under
`grpc._channel._UnaryUnaryMultiCallable.__call__`. There is no timeout and no
exception.

Adding a timeout through configuration is not possible: `registry.grpc_options`
or `registry.timeout` are accepted by pydantic and ignored, and
`registry_type: my.module.MyRegistry` is rejected by
`FeastRegistryTypeInvalidError`.

### Specifications

- Version: 0.64.0; also verified present on 0.65.0 (latest release) and `master`
- Platform: Linux, Python 3.12, Kubernetes; remote registry over gRPC
- Subsystem: registry — `feast.infra.registry.remote.RemoteRegistry`

## Possible Solution

Two parts, both small:

1. Add optional `timeout` and keepalive fields to `RemoteRegistryConfig`, and
apply them in `_create_grpc_channel`. A default deadline is the load-bearing
half: the hang always has an RPC in flight, so a deadline bounds it, requires
no server agreement, and cannot be refused by an intermediary. Keepalive is
the wider net, since it also catches a channel that rots while idle.

2. Rather than touching 50 call sites, install a
`grpc.UnaryUnaryClientInterceptor` that sets a default `timeout` on any call
issued without one. Because Feast already layers
`GrpcClientAuthHeaderInterceptor` over the channel, the two compose and every
RPC gains a deadline with no call-site changes.

On `grpc.keepalive_permit_without_calls`: it should be left unset, or at least
not defaulted on. gRPC servers enforce
`GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS` (5 minutes by default)
and answer too-frequent pings with a `GOAWAY` carrying `too_many_pings`, which
would kill working connections.

A secondary fix worth considering independently: let `registry_type` accept a
dotted class path, as the online and offline stores already do. That would give
users an escape hatch for this class of problem without a patch.

We currently monkeypatch `RemoteRegistry._create_grpc_channel` to add keepalive
options and wrap the channel in a deadline interceptor. Happy to open a PR along
the lines of (1) and (2) if the approach looks right.

Contributor guide

Open the contributing guide

Research direction

Start in sdk/python/feast/infra/registry/remote.py with RemoteRegistryConfig and RemoteRegistry._create_grpc_channel, then inspect the RPC call sites and existing GrpcClientAuthHeaderInterceptor. Verify how timeout and keepalive settings can be configured without changing every call site. Done means blackholed registry calls fail within a configurable bound and the channel applies the intended keepalive behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
grpc, python
Domain
api, backend, networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.