fix: Admin server dies silently by mkleczek · Pull Request #5064 · PostgREST/postgrest

mkleczek · 2026-07-02T07:39:17Z

This is a workaround for Warp crashing with "thread blocked indefinitely in an STM transaction" when receiving eMFILE from accept.

The fix is to provide Warp with a custom accept function that handles eMFILE by logging it and keeping accepting connections.

Fixes #5012.

wolfgangwalther

Thought a bit about how to test EMFILE stuff. I think it should be possible to test this with our current IO-test infrastructure. Will need to change how PostgREST is run - something like:

subprocess.Popen('ulimit ...; postgrest', shell=True)

This way we can lower the ulimit to the bare minimum, so that possibly even the first admin request fails, when no other concurrent requests are handled.

mkleczek · 2026-07-03T06:27:21Z

Thought a bit about how to test EMFILE stuff. I think it should be possible to test this with our current IO-test infrastructure.

Added reproducer in a separate commit.

taimoorzaeem · 2026-07-03T15:45:37Z

+
+        with contextlib.ExitStack() as stack:
+            opened_sockets = 0
+            for _ in range(nofile_limit * 3):


Why multiple it by 3? I am assuming this is to ensure that socket exhaustion is reached, right? Maybe we can add a comment?

I think we could indeed remove the * 3. That's because PostgREST will always use some FDs for other things than http connections: At least stdin, stdout, stderr, but also PG connections and other stuff. So we are guaranteed to hit EMFILE before rlimit_nofile - if not, something is seriously broken.

taimoorzaeem · 2026-07-03T15:49:22Z

+        accept sock `catchEMFile`
+          do
+            logEMFILE
+


I noticed that this function could be moved to where like this. You may ignore this suggestion as this isn't strictly needed, just a matter of personal preference I think.

address <- resolveSocketToAddress adminSocket debouncedEMFILELogger <- makeDebouncer $ observer AdminServerAcceptEMFILEFailure *> threadDelay 10_000_000 void . forkIO $ handle (onError adminSocket) $ Warp.runSettingsSocket (adminServerSettings conf address (safeAccept debouncedEMFILELogger)) adminSocket adminApp where ... isEMFILE err = Just eMFILE == fmap Errno (ioe_errno err) catchEMFile action handler = handleJust (\e -> [ e | isEMFILE e]) (const handler) action adminServerSettings config addr safeAccept' = do -- log EMFILE at most once every 10s settings & Warp.setAccept safeAccept' & Warp.setBeforeMainLoop (observer $ AdminStartObs addr) & maybe identity Warp.setPort (configAdminServerPort config) ... -- Keep accepting on eMFILE safeAccept logEMFILEObs sock = accept sock `catchEMFile` do void logEMFILEObs connCount <- Warp.currentOpenConnections serverState join $ atomically $ do if connCount > 0 then do -- There are open connections -- wait for closing some and restart accepting nextConnCount <- Warp.currentOpenConnectionsSTM serverState check (nextConnCount < connCount) pure $ safeAccept logEMFILEObs sock else pure $ do -- Backoff for 1ms so that we don't enter busy accept loop -- this should let the system recover fd's once the pressure is gone -- then restart accepting threadDelay 1_000 safeAccept logEMFILEObs sock

@mkleczek Please address this feedback. Trying to merge this now.

taimoorzaeem · 2026-07-03T15:52:59Z

Overall looks good! I think this should be tested the same way as done in #5012 (comment) to make sure this works under heavy load.

steve-chavez · 2026-07-03T16:22:28Z

Q: Shouldn't we better try to discuss/fix this upstream given we already got feedback?

So far looks like the fix here is resulting in perf loss (ref) and the logic looks a bit fragile.

mkleczek · 2026-07-03T16:37:13Z

Q: Shouldn't we better try to discuss/fix this upstream given we already got feedback?

Not sure when upstream is going to release the fix, created this PR to have it fixed in PostgREST ASAP (for warp this case is an edge case, for PostgREST it seems to be critical)

So far looks like the fix here is resulting in perf loss (ref).

As stated in one of the previous threads, our loadtests are not trustworthy. The changes in this PR have nothing to do with main server (they are really only affecting an exceptional path in Admin). There is no way they can affect application request handling performance.

and the logic looks a bit fragile

Why? That's really what upstream is going to do. Right now they are doing the same except they don't check for 0 active connections, hence the bug.

There is also a question about observation logging in upstream fix.

wolfgangwalther · 2026-07-05T18:58:23Z

So far looks like the fix here is resulting in perf loss (ref).

As stated in one of the previous threads, our loadtests are not trustworthy. The changes in this PR have nothing to do with main server (they are really only affecting an exceptional path in Admin). There is no way they can affect application request handling performance.

I wonder whether we can accidentally test changes that are on main, but not in the PR, with the loadtests. Here's how:

You branch off of commit A on main and add commit B in your PR.
Before you push, commit C is added on main.
You open the PR, the loadtests run. We just checkout "latest main" for the comparison, but that has C, while the PR doesn't.

We should really checkout the merge-base between the PR and main instead.

wolfgangwalther

Only looked at the test, this looks much better than my shell-based proposal.

(I would have tried that and opened a PR, but didn't find the time earlier)

wolfgangwalther · 2026-07-05T19:02:06Z

+
+        with contextlib.ExitStack() as stack:
+            opened_sockets = 0
+            for _ in range(nofile_limit * 3):


I think we could indeed remove the * 3. That's because PostgREST will always use some FDs for other things than http connections: At least stdin, stdout, stderr, but also PG connections and other stuff. So we are guaranteed to hit EMFILE before rlimit_nofile - if not, something is seriously broken.

wolfgangwalther · 2026-07-05T19:03:07Z

+    with run(env=env, wait_for=None, rlimit_nofile=nofile_limit) as postgrest:
+        wait_until_status_code(postgrest.session.baseurl + "/", 5, 200)


Suggested change

with run(env=env, wait_for=None, rlimit_nofile=nofile_limit) as postgrest:

wait_until_status_code(postgrest.session.baseurl + "/", 5, 200)

with run(env=env, rlimit_nofile=nofile_limit) as postgrest:

It does not seem necessary to do that in my tests. It's fine to not have the very first connection to the admin server further below. Fails and passes as expected.

(the comment further down needs adjustment as well then)

wolfgangwalther · 2026-07-05T19:03:45Z

+            opened_sockets = 0
+            for _ in range(nofile_limit * 3):
+                sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+                sock.settimeout(0.2)


In my tests it seemed like this was not needed:

Suggested change

sock.settimeout(0.2)

wolfgangwalther · 2026-07-05T19:05:00Z

+            # sockets are connected to the main server, so the admin server has
+            # no active accepted connections when accept hits EMFILE.
+            with pytest.raises(requests.RequestException):
+                postgrest.admin.get("/live", timeout=0.2)


Maybe have a comment explaining this timeout: If we don't have it, the connection will be stuck forever.

I believe this would be fixed by #5077, so maybe make it a TODO to remove the timeout after that PR is in.

Although I can't reproduce it, so maybe it was not even this sleep at all...

wolfgangwalther · 2026-07-05T19:07:46Z

+                opened_sockets += 1
+
+            assert opened_sockets >= nofile_limit
+            time.sleep(0.2)


I seem to need that sleep, but I don't know exactly why - and I don't like that fact. This seems to just open the door for another strange timing related failure in IO tests.

When I don't have it, then I sometimes get a Failed: DID NOT RAISE <class 'requests.exceptions.RequestException'>, right below...

I think that sleep is needed to give the server some time to hit the FD limit and sit in that EMFILE error state. Not sure exactly why either. Without this the below exception isn't raised which is why you see the Failed: DID NOT RAISE ... error.

This seems to just open the door for another strange timing related failure in IO tests.

Agreed. Another option could be to test that the exception occurs by making the request in a while loop with 0.05 sec sleep on each iteration.

steve-chavez · 2026-07-06T20:26:29Z

Tested this with postgrest-benchmark and the behavior is:

# while under load
curl localhost:3001/ready -i
# this hangs for a while.. then replies:

HTTP/1.1 500 Internal Server Error
Transfer-Encoding: chunked
Date: Mon, 06 Jul 2026 20:21:55 GMT
Server: postgrest/15 (pre-release)
Content-Type: text/plain; charset=utf-8

Something went wrong

Maybe we can improve the error message above?

Then once the load stops the Admin server recovers:

curl localhost:3001/ready -i
HTTP/1.1 200 OK
Transfer-Encoding: chunked
Date: Mon, 06 Jul 2026 20:22:01 GMT
Server: postgrest/15 (pre-release)

The logs show:

$ journalctl -u postgrest
...
06/Jul/2026:20:21:56 +0000: Admin server failed to accept connection due to file descriptor exhaustion (EMFILE)

Network.Socket.socket: resource exhausted (No file descriptors available)

steve-chavez · 2026-07-06T20:28:24Z

So far it's looking like an acceptable fix. Although I'm seeing a fix upstream already open yesodweb/wai#1084, would be good to compare the solutions and give feeback there too.

steve-chavez · 2026-07-07T02:41:57Z

Something went wrong

Any idea why the above response happens?

steve-chavez · 2026-07-07T02:46:30Z

+def test_admin_server_does_not_crash_when_file_limit_is_reached(defaultenv):
+    "Admin server should keep running when accept reaches the open file limit."


I was thinking this test is unnecessary because it's testing Warp behavior.

But on second thought, it's worth to have it since EMFILE will surface even with the upstream patch and we have an uncommon setup with two servers.

So this test can be merged independently of this PR (marked with xfail). That would also allow reusing it on #5078.

mkleczek · 2026-07-07T13:30:46Z

Something went wrong

Any idea why the above response happens?

Looks like the message is produced by warp - analyzing the exact code path.

mkleczek · 2026-07-07T13:43:39Z

Something went wrong

Any idea why the above response happens?

Looks like the message is produced by warp - analyzing the exact code path.

warp produces this message by default when the application throws exception. It is most probably caused by /ready trying to connect to main server (ie. perform socket probing) and throws IOError or another exception.

Our code allows escaping exceptions - it does not wrap the whole piece in try:

reachMainApp appSock = do
  sockAddr <- NS.getSocketName appSock
  sock <- NS.socket (addrFamily sockAddr) NS.Stream NS.defaultProtocol
  try $ do
    NS.connect sock sockAddr
    NS.withSocketsDo $ bracket (pure sock) NS.close sendEmpty

I suppose NS.socket may throw (for example on EMFILE).

Looks like the code should be changed to:

reachMainApp appSock = try $ do
  sockAddr <- NS.getSocketName appSock
  sock <- NS.socket (addrFamily sockAddr) NS.Stream NS.defaultProtocol
  NS.connect sock sockAddr
  NS.withSocketsDo $ bracket (pure sock) NS.close sendEmpty

It is also a strong hint to merge #5077.

steve-chavez · 2026-07-07T22:49:50Z

Looks like the code should be changed to:

Thanks for clarifying, I think we should do that.

It is also a strong hint to merge #5077.

There are some doubts if that's the right move, but even then this fix is good because the admin server survives.

I'll also add that I've tried postgrest-benchmark both here and #5078 and the performance is mostly the same.

steve-chavez · 2026-07-07T23:39:06Z

Looks like the code should be changed to:

Can confirm the "Something went wrong" body goes away with the above, added it on 1e4dc38

steve-chavez · 2026-07-07T23:41:07Z

@mkleczek @wolfgangwalther @taimoorzaeem Any thoughts on whether we should prefer this PR or #5078?

taimoorzaeem · 2026-07-08T07:20:02Z

Any thoughts on whether we should prefer this PR or #5078?

Although we have tested the upstream fix, I think we can't completely trust it until it is merged and released.

Besides, the fix in this PR is also logging an observation on failing to accept the connection which #5078 doesn't have. If the behavior and performance is almost the same, I think we should go with this PR for now (we do need to improve this PR though).

steve-chavez · 2026-07-08T17:16:31Z

Ok cool, so then we should merge #5077 before to not have the Something went wrong body response.

Added test_admin_server_does_not_crash_when_file_limit_is_reached marked with xfail.

This is a workaround for Warp crashing with "thread blocked indefinitely in an STM transaction" when receiving eMFILE from accept. The fix is to provide Warp with a custom accept function that handles eMFILE by logging it and keeping accepting connections.

steve-chavez · 2026-07-10T18:10:52Z

Besides, the fix in this PR is also logging an observation on failing to accept the connection which #5078 doesn't have.

Looking at that log line I don't think it helps operators that much. We already log:

Network.Socket.accept: resource exhausted (No file descriptors available)

Which gives more or less the same info plus /ready will respond with 500.

Although we have tested the upstream fix, I think we can't completely trust it until it is merged and released.

Upstream already merged the PR yesodweb/wai#1084. I think it's clear that relying on upstream patches when we can is much better for us since we have less code/tests + less to review. Also not that much difference in patch speed.

So IMO we should go with #5078.

taimoorzaeem · 2026-07-10T18:29:48Z

I think it's clear that relying on upstream patches when we can is much better for us since we have less code/tests + less to review. Also not that much difference in patch speed.

So IMO we should go with #5078.

Agreed. 👍

mkleczek marked this pull request as draft July 2, 2026 07:40

mkleczek force-pushed the push-mxkvrmlnlwyy branch from d5b6b67 to 7f2a8bf Compare July 2, 2026 07:46

mkleczek requested review from steve-chavez and taimoorzaeem July 2, 2026 08:14

wolfgangwalther reviewed Jul 2, 2026

View reviewed changes

Comment thread src/PostgREST/Admin.hs Outdated

mkleczek force-pushed the push-mxkvrmlnlwyy branch 2 times, most recently from 155ff1d to fb666ef Compare July 2, 2026 11:16

mkleczek mentioned this pull request Jul 3, 2026

fix: Warp crashes with "thread blocked indefinitely in an STM transaction" when receiving eMFILE from accept yesodweb/wai#1083

Closed

mkleczek force-pushed the push-mxkvrmlnlwyy branch 7 times, most recently from 3b0a203 to 5d23def Compare July 3, 2026 06:25

mkleczek marked this pull request as ready for review July 3, 2026 06:26

mkleczek requested a review from wolfgangwalther July 3, 2026 06:27

mkleczek force-pushed the push-mxkvrmlnlwyy branch from 5d23def to 77226f7 Compare July 3, 2026 09:25

taimoorzaeem reviewed Jul 3, 2026

View reviewed changes

Comment thread src/PostgREST/Admin.hs Outdated

mkleczek force-pushed the push-mxkvrmlnlwyy branch 4 times, most recently from 3195a6d to b104780 Compare July 3, 2026 14:22

mkleczek requested a review from taimoorzaeem July 3, 2026 14:59

taimoorzaeem reviewed Jul 3, 2026

View reviewed changes

mkleczek force-pushed the push-mxkvrmlnlwyy branch from b104780 to 554fe47 Compare July 3, 2026 17:20

wolfgangwalther reviewed Jul 5, 2026

View reviewed changes

mkleczek force-pushed the push-mxkvrmlnlwyy branch 5 times, most recently from d3e7298 to 6ea1a07 Compare July 6, 2026 07:17

steve-chavez mentioned this pull request Jul 6, 2026

fix: admin server dies silently by upgrading warp #5078

Merged

steve-chavez reviewed Jul 7, 2026

View reviewed changes

mkleczek added 2 commits July 9, 2026 06:30

test: Admin server dies silently reproducer

a98b622

Added test_admin_server_does_not_crash_when_file_limit_is_reached marked with xfail.

mkleczek force-pushed the push-mxkvrmlnlwyy branch from 6ea1a07 to 975f4ec Compare July 9, 2026 04:33

		with run(env=env, wait_for=None, rlimit_nofile=nofile_limit) as postgrest:
		wait_until_status_code(postgrest.session.baseurl + "/", 5, 200)

	with run(env=env, wait_for=None, rlimit_nofile=nofile_limit) as postgrest:
	wait_until_status_code(postgrest.session.baseurl + "/", 5, 200)
	with run(env=env, rlimit_nofile=nofile_limit) as postgrest:

		def test_admin_server_does_not_crash_when_file_limit_is_reached(defaultenv):
		"Admin server should keep running when accept reaches the open file limit."

Uh oh!

Uh oh!

Conversation

mkleczek commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkleczek commented Jul 3, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taimoorzaeem commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steve-chavez commented Jul 3, 2026

Uh oh!

mkleczek commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Jul 5, 2026

Uh oh!

wolfgangwalther left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-chavez commented Jul 6, 2026

Uh oh!

steve-chavez commented Jul 6, 2026

Uh oh!

steve-chavez commented Jul 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkleczek commented Jul 7, 2026

Uh oh!

mkleczek commented Jul 7, 2026

Uh oh!

steve-chavez commented Jul 7, 2026

Uh oh!

steve-chavez commented Jul 7, 2026

Uh oh!

steve-chavez commented Jul 7, 2026

Uh oh!

taimoorzaeem commented Jul 8, 2026

Uh oh!

steve-chavez commented Jul 8, 2026

Uh oh!

steve-chavez commented Jul 10, 2026

Uh oh!

taimoorzaeem commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

mkleczek commented Jul 2, 2026 •

edited

Loading

taimoorzaeem commented Jul 3, 2026 •

edited

Loading

mkleczek commented Jul 3, 2026 •

edited

Loading