Repository navigation
chore(feature-flags): Keep the runtime clients safe across fork() - #767
Conversation
[Medium impact] The PR appears safe to merge; no new actionable issues were found. SummaryThis PR rebuilds runtime clients after a fork while keeping their last flag configuration available.
Both supplied, unnumbered previous findings are addressed. No new actionable issues were found. This assessment is based on source and test inspection; tests were not run.
|
|
@greptileai review |
|
About the custom-backend finding in the review summary: it's fixed in 625309a. The record of HTTP clients that sent requests is now a |
|
@greptileai review |
gjtorikian
left a comment
There was a problem hiding this comment.
Thorough work on a nasty problem: locks rebuilt in the child, the inherited client abandoned rather than closed, request tracking keyed on the HTTP client, and a hook that survives one client failing. One decision to make before merge, on the macOS behavior. Details inline.
| # the system trust store) when it opens a network connection. Every | ||
| # WorkOSClient has an HTTP client, so a runtime client created before the fork | ||
| # never polls in the child there. | ||
| FORK_SAFE_PLATFORM = sys.platform != "darwin" |
There was a problem hiding this comment.
On macOS this makes start() and refresh() raise in every forked child of a process that created a client, with no opt-out. That is stricter than the rest of the SDK: the inherited WorkOSClient still serves normal calls in the child with the same theoretical crash risk. A developer running gunicorn --preload on a Mac gets a RuntimeError at worker boot that does not happen on Linux, and FORK_SAFE_PLATFORM is private, so there is no supported way around it.
Could we warn and skip polling on macOS, as the was-polling path already does, instead of raising from start()? Alternatively, honor OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES as the opt-out, since that is the variable people already set for this crash. Either way, the gunicorn sentence in the README should call out the platform difference.
There was a problem hiding this comment.
Went with warn and skip in 84a302f. On macOS in a forked child, start() now logs one warning (pointing to gunicorn's post_fork hook) and returns instead of raising. refresh() still raises since actually fetching would crash, and wait_until_ready() raises right away if there's no config, otherwise start() + wait_until_ready() would just hang forever. Also updated the README's gunicorn sentence to call out macOS.
I skipped the OBJC_DISABLE_INITIALIZE_FORK_SAFETY opt-out because it doesn't actually prevent the crash. I tested it locally (macOS 27, Python 3.14, httpx2 2.13) and the child still segfaults inside SecTrustEvaluate on its first new TLS connection, with or without the variable. It's the system trust store crashing, not the ObjC check the variable turns off, so honoring it would just turn the warning into a crashing worker.
FWIW the inherited WorkOSClient isn't really safe either. It only works while it's reusing a connection the parent opened, and if the parent never made a call, the child's first call segfaults. That's outside this PR but figured I'd flag it.
|
@greptileai review |
73ce16b to
097d9bf
Compare
84a302f to
9757715
Compare
|
@greptileai review |
097d9bf to
28a35f6
Compare
9757715 to
364e412
Compare
A runtime client created before a fork (gunicorn --preload) can be used in each worker. With the SDK's default HTTP client, the sync client polls through an HTTP client of its own, so a child replaces it and resumes polling on new connections the first time it is used. Where the child would share the parent's connections or event loop, or on macOS, start() and refresh() raise and the client keeps serving its last configuration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…kOS clients Two WorkOS clients can share one HTTP client you supplied. A request sent through one marked only that WorkOS client, so an unused runtime client on the other still polled in a forked child over the parent's connections, or on the parent's event loop for async. Requests are now recorded against the HTTP client itself, and the fork check reads that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ckends The record of HTTP clients that sent requests was a WeakSet, which hashes its members, so a caller's own backend that can't be hashed (any dataclass with eq=True) raised TypeError in the fork hook and stopped it from rebuilding the remaining runtime clients. The record is now keyed by id, and a client that fails to rebuild is logged without stopping the others. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On macOS, start() raised in every forked child of a process that had created a client, so gunicorn --preload on a Mac failed at worker boot with no opt-out. Every client created before the fork is blocked there, however the parent used it, so start() now logs the fork warning once and returns without polling. The client serves its bootstrap or last configuration. refresh() still raises, since it would open a connection, and wait_until_ready() raises at once for a blocked child without a configuration instead of waiting for a poll that never comes. The Linux blockers, where the parent used a caller-supplied HTTP client or the AsyncWorkOSClient, still raise from start(). The warning and README now point to gunicorn's post_fork hook. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
364e412 to
d60a620
Compare
Stack. This is #762, split into four PRs for review. Merge them in order; after each merge I'll rebase the next one onto
main.Description
A runtime client created before a fork, as in an app gunicorn loads with
--preload, can now be used in each worker.Default HTTP client. With the SDK's default HTTP client, the sync client polls through an httpx client of its own instead of the
WorkOSClient's. A forked child drops the inherited client without closing it, because its sockets belong to the parent. The child resumes polling on new connections the first time the runtime client is used there.When the child doesn't poll. Some things a child can't safely replace:
AsyncWorkOSClientused in the parent, whose connections belong to the parent's event loop.In those cases,
start()andrefresh()raise,wait_until_ready()raises right away if there's no configuration, and the client keeps serving its last configuration. A client that was polling logs a warning the first time it's used.macOS. On macOS a child never polls. Once a process has created an HTTP client, a child it forks segfaults in the system trust store when it opens a new TLS connection. So a preloaded app still boots on a Mac,
start()there logs one warning, naming gunicorn'spost_forkhook as the fix, and returns instead of raising.refresh()still raises, andwait_until_ready()raises right away if there's no configuration. The client serves its bootstrap or last configuration, otherwise the caller's defaults.Rebuilding in the child. The child rebuilds locks, events and the poller thread through
os.register_at_fork. A client that fails to rebuild is logged and doesn't stop the others.Where to look:
_sync.py:_reinitialize_after_forkand_before_use._transport.py:fork_blockerand_httpx_client._async.py:_reinitialize_after_fork.How I checked it:
test_fork.pyforks a real child in a subprocess for each case. It polls a local HTTP server that records each request's TCP port, and checks that the child polls on its own connections or not at all. The cases are:start()warns once instead of raising, with and without a bootstrap.It also checks that an async client's
close()returns in the child. The tests pass on macOS here, and CI runs them on Linux.I reproduced each Greptile finding on feat(feature-flags): Evaluate feature flags locally with a polling runtime client #762 and on this PR before fixing it:
close()in a child hung;Event loop is closed;On this Mac, I forked a real child after creating a
WorkOSClient. Its first new TLS connection segfaulted insideSecTrustEvaluate, with or withoutOBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES. With this change, a child that callsstart()andwait_until_ready()logs one warning, serves its bootstrap, makes no network attempt and exits cleanly; without a bootstrap,wait_until_ready()raises at once instead of hanging.The full feature flags runtime lab passed against this branch before the macOS
start()change: 87 of 87 checks, and the bridge suite 5 of 5.nox -s cipasses locally: 3,743 tests.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
The docs PR noted on #765 should cover forked workers.
🤖 Generated with Claude Code