Fix GH-18956: PHP-FPM Process Count Inconsistencies

[bukka]

This fixes extra increments on keep alive that happen for follow up request in headers reading stage. It is because the accepting stage is only done on the first request. It adds a new flag to the on_read fastcgi callback that sets whether the kept alive request is being done and then skips the further active increments.

It also fixes issue with incorrect decrement of the active number of processes. This was done in accepting stage even on the first accept which might have been be problematic if there were already some running processes as it decremented their number first and resulted in incorrect total (lower than the actual number). The fix is to skip this decrement of active processes on the first accept.

[bukka]

This needs more testing - I will go only for WST test as PHP test is too tricky for this and would not most likely work in the pipeline anyway.

[bukka]

So I have been testing and debugging this. At the end the best way to see what's going on was adding some extra logs which I have in this debug branch: master...bukka:php-src:fpm_gh18956_request_active_decrement_debug .

If it's done without keepalive, a single request call looks like following:

NOTICE: scoreboard active++
NOTICE: prev active: 0
NOTICE: new active: 1
NOTICE: fcgi reading 1 (len=8, type=1)
NOTICE: fcgi reading 2 (len=8, type=1, id=1)
NOTICE: req accepted: 0x5695f037fbc0
NOTICE: finished
NOTICE: req shutdown: 0x5695f037fbc0
NOTICE: scoreboard active--
NOTICE: prev active: 1
NOTICE: new active: 0
NOTICE: prev active: 0
NOTICE: new active: 0

But with keepalive (fastcgi_keep_conn on;), the situation is a bit different

NOTICE: scoreboard active++
NOTICE: prev active: 0
NOTICE: new active: 1
NOTICE: fcgi reading 1 (len=8, type=1)
NOTICE: fcgi reading 2 (len=8, type=1, id=1)
NOTICE: req accepted: 0x5695f037fd50
NOTICE: req shutdown: 0x5695f037fd50
NOTICE: scoreboard active--
NOTICE: prev active: 1
NOTICE: new active: 0
NOTICE: scoreboard active++
NOTICE: prev active: 0
NOTICE: new active: 1
NOTICE: fcgi reading 1 (len=0, type=5)
NOTICE: fcgi reading end 1
NOTICE: finished
NOTICE: prev active: 1
NOTICE: new active: 1

The problem is that there are two on_read calls because keepalive means that FPM (or more fastcgi.c) does second loop but if nginx gets single non keepalive http request, the second read is just 0 so FPM closes connection.

In this case it might seem logical to reduce active in fpm_request_finished (on_close hook) except it wouldn't work for non keepalive. Unfortunately there is currently no way to differentiate it in fpm_request_finished. Also on_close can be called in couple of other cases so it would need additional checking.

I will need to think about it more and see if there is some clean solution. Alternatively there might be some hacky solution to do that but it will require more checking as well. I will get this sorted - it will just require a bit of time to get right and properly test it.

[Appla]

Would it be possible to explicitly mark keepalive requests and reflect them in the statistics? For keepalive connections, subsequent reuse should only increment the reuse count without affecting the active/idle process state.

Based on this idea, I created a demonstration that minimizes changes to the existing API. ref

[bukka]

@Appla I was checking your changes and not sure if it would work unless I'm misunderstanding it. The thing is that used does not distinguish between keep alive and non keep alive but this is what is needed to correctly handle active increments.

I just got an idea to pass that info from fastcgi (extra parameter for on_read and on_accept) and skip the increment for follow up requests and decrement for the first accept. That should hopefully balance it better but will need to properly test it.

[Appla]

@Appla I was checking your changes and not sure if it would work unless I'm misunderstanding it. The thing is that used does not distinguish between keep alive and non keep alive but this is what is needed to correctly handle active increments.

I just got an idea to pass that info from fastcgi (extra parameter for on_read and on_accept) and skip the increment for follow up requests and decrement for the first accept. That should hopefully balance it better but will need to properly test it.

The used was used to store the connection usage count. As soon as it’s greater than 1, we treat it as a reused (keepalive) connection. This way we can distinguish the first accept from subsequent requests without introducing extra parameters.

[Appla]

I just got an idea to pass that info from fastcgi (extra parameter for on_read and on_accept) and skip the increment for follow up requests and decrement for the first accept. That should hopefully balance it better but will need to properly test it.

Distinguishing keep‑alive connections is important, as it also relates to the request_terminate_timeout issue. The current implementation has problems with timing for keep‑alive connections. Once we make this distinction, in principle it could also resolve that problem.

I tried to fix the timing issue with minimal changes (ref), but an overall reconsideration would likely be better.