Repository navigation
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7065 +/- ##
==========================================
+ Coverage 51.28% 51.31% +0.03%
==========================================
Files 458 458
Lines 57409 57418 +9
Branches 8415 8417 +2
==========================================
+ Hits 29443 29465 +22
+ Misses 25957 25945 -12
+ Partials 2009 2008 -1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
26042ff to
b891917
Compare
Gunicorn workers inherited the parent's pooled SQL registry connections, so several workers shared one socket and the server crashed on startup. A post_fork hook, set for every run, now disposes the pools with close=False, so the parent's connections are left open. Fixes feast-dev#5784 Signed-off-by: Carlos Sánchez <[email protected]>
Signed-off-by: Carlos Sánchez <[email protected]>
3bcc3de to
8557e38
Compare
tair800
left a comment
tair800
left a comment
There was a problem hiding this comment.
Thanks for the focused fix and the detailed analysis in #5784. The approach looks right to me: post_fork runs in the worker right after os.fork() and before worker.init_process() (gunicorn's Arbiter.spawn_worker), and dispose(close=False) is the variant SQLAlchemy recommends after a fork.
An AI-assisted reproduction with standalone scripts (SQLAlchemy 2.0.54 on Linux, with a stand-in driver whose close() sends a quit message like PyMySQL's COM_QUIT; Feast itself and its test suite were not run) showed the difference: without a reset, the child received the parent's connection and both processes read each other's replies; with dispose(close=False), each process had its own session; with a plain dispose(), the child's quit ended the parent's session.
One dependency-compatibility point inline; it does not change the approach.
Disclosure: this review was prepared with an AI assistant, which also ran the standalone reproductions described here.
| # close=False because the parent still owns those connections; closing | ||
| # them would send a disconnect on a socket the parent shares. See | ||
| # https://docs.sqlalchemy.org/en/20/core/pooling.html#using-connection-pools-with-multiprocessing-or-os-fork | ||
| self.write_engine.dispose(close=False) |
There was a problem hiding this comment.
Engine.dispose(close=...) was added in SQLAlchemy 1.4.33. Feast's core dependency is SQLAlchemy[mypy]>1 (pyproject.toml line 31; line 59 has SQLAlchemy>=1.4.19), and the comment at line 107 of this file says 1.4.x is still supported. On 1.4.0–1.4.32 this call raises; an AI-assisted check against the 1.4.32 and 1.4.33 sources gives:
sqlalchemy 1.4.32: engine.dispose(close=False) -> TypeError: Engine.dispose() got an unexpected keyword argument 'close'
sqlalchemy 1.4.33: engine.dispose(close=False) -> OK
Because this runs in post_fork, an exception there should make the worker exit with a boot error and gunicorn stop the server. That last step comes from reading gunicorn's arbiter, not from running feast serve.
close= was introduced in 1.4.33, but 1.4.33 and 1.4.34 were both yanked from PyPI (an ORM insert regression in 1.4.33, and a caching issue in 1.4.34 that 1.4.35 resolves), which makes 1.4.35 the appropriate minimum supported version. Would raising the floor to SQLAlchemy[mypy]>=1.4.35 work for you?
What this PR does / why we need it:
feast serve --workers Nbuilds the FastAPI app, and with it theFeatureStoreand its registry, in the Gunicorn master before it forks the workers.get_app()createsVectorStoreRegistry(store), whose firstrefresh()lists feature views, which creates theSqlRegistryand runs queries in the master. SQLAlchemy'sQueuePoolkeeps that connection idle in the master, and every forked worker inherits the pool and checks out the same DBAPI connection, so two processes talk over one socket.With MySQL over TLS this fails with
(2013, 'Lost connection to MySQL server during query ([SSL] record layer failure ...)'), either as asqlalchemy.pool"Exception during reset or similar", or, when it hitsasync_refresh()in the lifespan, as "Application startup failed", after which Gunicorn halts the whole server ("Worker failed to boot."). Without TLS the same sharing gives other errors (see #5784 for a PostgresResourceClosedError). Workers re-forked through--max-requestsinherit the master's pool again. Details and a deterministic reproduction are in #5784.This applies SQLAlchemy's documented recipe ("Using Connection Pools with Multiprocessing or os.fork()"):
engine.dispose(close=False)in the child.close=Falsematters: a plaindispose()would close the inherited connections in the child and send a disconnect on a socket the master still shares (sqlalchemy/sqlalchemy#7815).BaseRegistrygains a no-opreset_after_fork(), so file, remote and other registries are unaffected.SqlRegistry.reset_after_fork()disposeswrite_engine, andread_enginewhen it is a separate engine, withclose=False.FeastServeApplication.load_configalways sets a Gunicornpost_forkhook,_gunicorn_post_fork, which calls it. Unlike the metricspost_worker_init/child_exithooks, it is set for every run, because the bug does not depend on metrics.post_forkruns in the child right after the fork, before the worker loads the app or runs the lifespan, and on every re-fork. The hook does nothing when no registry was built, and never builds one.#5795 targets the same issue; this PR keeps the change to the registry engines and uses
close=False.Which issue(s) this PR fixes:
Fixes #5784
Misc
Tests:
test_sql_registry.py:test_forked_child_does_not_reuse_parent_pooled_connectionuses the file-backed SQLiteSqlRegistryfixture (a realQueuePool) andos.fork(). Without the fix, the child checks out the parent's DBAPI connection object. With it, the child gets its own. Two more tests check thatreset_after_fork()disposes the write engine, and the read engine whenread_pathis set, withclose=False.test_feature_server.py: thepost_forkhook is registered with and without metrics, it callsreset_after_fork(), and it does not build a registry when none exists.feast serve --workers 2behind a 10 ms latency proxy, 10 runs on each side. Before (master): "Exception during reset or similar" / "Lost connection to MySQL server during query" in 2 of 10 runs, one of them with[SSL] record layer failure. After (this PR): 0 of 10. Both workers report "Application startup complete" in every run on both sides.ruff check,ruff format --checkandmypy feastpass. The touched unit test files pass (206 tests).