Repository navigation
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7065 +/- ##
==========================================
+ Coverage 50.97% 51.02% +0.04%
==========================================
Files 455 455
Lines 56994 57003 +9
Branches 8321 8323 +2
==========================================
+ Hits 29055 29083 +28
+ Misses 25954 25934 -20
- Partials 1985 1986 +1
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
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 <carlos.sancheza@cabify.com>
casaar97
force-pushed
the
casaar97/sql-registry-postfork-dispose
branch
from
October 27, 2026 11:04
26042ff to
b891917
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).