server: health: fix race condition on slots data using tasks queue

[phymbert]

Issue

Changes on /health and /slots endpoints in PRs #5550 and #5548 incorrectly access the slots internal data. It can create a race condition under some circonstances identified in #5566.

Changes

Use the tasks_queue to access slots data in health checks. As of now access to health are synchronized, it will be useful to also include slots detail in the /health response body with the query parameter include_slots if except if --slots-endpoint-disable is set.

[Artefact2]

I think include_slots should be disabled if --slots-endpoint-disable is set, considering it's a trivial bypass otherwise.

[phymbert]

--slots-endpoint-disable

Thanks 👍 done!

[fcecagno]

Hi guys, I believe this might have introduced an issue.
I've tried c71d608 from a few weeks ago and then tried the latest build yesterday. When I use the API, the /health answer hangs, which is quite problematic since it's used to check the health of the application. If it hangs, it means the application might not be responding and could be killed/restarted. Have you tested this scenario? Is there any specific thing I can provide to help this issue to be fixed? This issue isn't present on c71d608.

[ggerganov]

It works on my end:

./server -m models/llama-7b-v2/ggml-model-q4_0.gguf

$ ▶ curl http://localhost:8080/health -H "Content-Type: application/json" -H "Authorization: Bearer no-key"
{"slots_idle":1,"slots_processing":0,"status":"ok"}

$ ▶ curl http://localhost:8080/health -H "Content-Type: application/json" -H "Authorization: Bearer no-key"
{"slots_idle":1,"slots_processing":0,"status":"ok"}

$ ▶ curl http://localhost:8080/health -H "Content-Type: application/json" -H "Authorization: Bearer no-key"
{"slots_idle":1,"slots_processing":0,"status":"ok"}

Can you provide repro using curl commands?

[fcecagno]

I tried this:

while [ true ]; do date; curl --max-time 1 localhost:8080/health; echo "  -> $?"; sleep 1; done

Depending on the prompt, it fails for 10s, if it's a bigger prompt, it will fail for many many cycles.
If it works on your end, I'll try to provide a full step-to-step on how to reproduce it.

[ggerganov]

Aha, got it. If the prompt processing lasts more than 1s, the query will timeout because the server is busy processing:

{"slots_idle":1,"slots_processing":0,"status":"ok"}  -> 0
Wed Feb 28 16:01:33 EET 2024
{"slots_idle":1,"slots_processing":0,"status":"ok"}  -> 0
Wed Feb 28 16:01:34 EET 2024
curl: (28) Operation timed out after 1006 milliseconds with 0 bytes received
  -> 28
Wed Feb 28 16:01:36 EET 2024
{"slots_idle":0,"slots_processing":1,"status":"no slot available"}  -> 0
Wed Feb 28 16:01:37 EET 2024
{"slots_idle":0,"slots_processing":1,"status":"no slot available"}  -> 0
Wed Feb 28 16:01:38 EET 2024
{"slots_idle":0,"slots_processing":1,"status":"no slot available"}  -> 0
Wed Feb 28 16:01:39 EET 2024
{"slots_idle":0,"slots_processing":1,"status":"no slot available"}  -> 0
Wed Feb 28 16:01:40 EET 2024
{"slots_idle":0,"slots_processing":1,"status":"no slot available"}  -> 0
Wed Feb 28 16:01:41 EET 2024
{"slots_idle":1,"slots_processing":0,"status":"ok"}  -> 0
Wed Feb 28 16:01:42 EET 2024

We probably have to rethink the task queueing logic

[fcecagno]

Yes, the amount of time /health hangs depend on the number of tokens of the query, it seems.

[ngxson]

I can confirm the bug: When the server is busy doing something, the queue is blocked by llama_decode. During this time, incoming requests will be queued instead of processing right away.

Before this PR, we don't have this behavior because /health does not depends on queue.

What I'm thinking is to delegate access to slot data to mutex inside llama_server_queue (i.e. adding a new function in llama_server_queue to get/set slot data). This way, we no longer need to push requests to /health into the queue.

[fcecagno]

Hi guys, following up the issue of blocking /health, is there an issue already created for it?

[ggerganov]

#5882 should improve the situation. Very large prompts will now be chunked in n_batch tokens and after each chunk the server should respond to /health requests. Please give it a try and see if it helps, but keep in mind there could be some bugs in that branch - not tested thoroughly yet