v0.1.29 - fix auth routing: allow public GET /health operation bypass
This commit is contained in:
2
.gitignore
vendored
2
.gitignore
vendored
@@ -4,5 +4,5 @@ nostr_core_lib/
|
|||||||
blobs/
|
blobs/
|
||||||
c-relay/
|
c-relay/
|
||||||
text_graph/
|
text_graph/
|
||||||
.test_keys/
|
.test_keys
|
||||||
ginxsom-local.service
|
ginxsom-local.service
|
||||||
Binary file not shown.
BIN
build/main.o
BIN
build/main.o
Binary file not shown.
Binary file not shown.
@@ -199,5 +199,5 @@ echo " Reload: sudo systemctl reload nginx"
|
|||||||
echo " Logs: sudo tail -f /var/log/nginx/error.log"
|
echo " Logs: sudo tail -f /var/log/nginx/error.log"
|
||||||
echo ""
|
echo ""
|
||||||
echo "Test the server:"
|
echo "Test the server:"
|
||||||
echo " curl http://localhost:$NGINX_PORT/health"
|
echo " curl -k https://localhost:$NGINX_PORT/health"
|
||||||
echo " curl http://localhost:$NGINX_PORT/"
|
echo " curl -k https://localhost:$NGINX_PORT/"
|
||||||
|
|||||||
149
plans/fix-ginxsom-cpu-spin.md
Normal file
149
plans/fix-ginxsom-cpu-spin.md
Normal file
@@ -0,0 +1,149 @@
|
|||||||
|
# Fix: Ginxsom 100% CPU Busy-Wait Bug
|
||||||
|
|
||||||
|
## Root Cause Analysis
|
||||||
|
|
||||||
|
The ginxsom-fcgi process consumes 100% CPU due to a **busy-wait spin loop** in the relay management thread.
|
||||||
|
|
||||||
|
### The Bug Chain
|
||||||
|
|
||||||
|
1. **`relay_management_thread()`** in `src/relay_client.c:328` runs a `while` loop calling `nostr_relay_pool_poll()` with a 1000ms timeout
|
||||||
|
2. **`nostr_relay_pool_poll()`** in `nostr_core_lib/nostr_core/core_relay_pool.c:1699` iterates over all relays, but **skips disconnected relays** at line 1725-1728 — it only calls `nostr_ws_receive()` (which actually blocks) for connected relays
|
||||||
|
3. **When no relays are connected** (or all are in error/disconnected state), the entire poll function returns 0 instantly with zero blocking
|
||||||
|
4. **The caller has no sleep** for the `events_processed == 0` case — it immediately loops back and calls poll again
|
||||||
|
5. This creates a **tight spin loop** consuming 100% of a CPU core
|
||||||
|
|
||||||
|
### Additional Issues Found
|
||||||
|
|
||||||
|
- **No signal handling**: `main.c` has zero `signal()`, `SIGTERM`, or `SIGINT` handling. When systemd sends SIGTERM to stop the service, the process ignores it and must be SIGKILL'd after timeout (confirmed in journal: `State 'stop-sigterm' timed out. Killing.`)
|
||||||
|
- **`relay_client_stop()` never called**: The cleanup function exists but is never invoked — there's no shutdown path
|
||||||
|
- **`g_relay_state.running` not volatile**: The `running` flag is read by the management thread but set by the main thread (or signal handler). Without `volatile`, the compiler may optimize away the check
|
||||||
|
|
||||||
|
```mermaid
|
||||||
|
graph TD
|
||||||
|
A["relay_management_thread()"] -->|"while running"| B["nostr_relay_pool_poll(pool, 1000)"]
|
||||||
|
B --> C{"Any relays connected?"}
|
||||||
|
C -->|"Yes"| D["nostr_ws_receive() blocks up to timeout_ms"]
|
||||||
|
C -->|"No - all disconnected"| E["Returns 0 immediately"]
|
||||||
|
D --> F{"events_processed > 0?"}
|
||||||
|
E --> F
|
||||||
|
F -->|"> 0"| A
|
||||||
|
F -->|"== 0"| G["No sleep! Loops immediately"]
|
||||||
|
F -->|"< 0"| H["sleep(1) then loop"]
|
||||||
|
G -->|"BUSY WAIT"| A
|
||||||
|
```
|
||||||
|
|
||||||
|
## Fixes Required
|
||||||
|
|
||||||
|
### Fix 1: Add idle sleep to poll loop (`src/relay_client.c`)
|
||||||
|
|
||||||
|
**File:** `src/relay_client.c`, lines 327-337
|
||||||
|
|
||||||
|
**Before:**
|
||||||
|
```c
|
||||||
|
while (g_relay_state.running) {
|
||||||
|
int events_processed = nostr_relay_pool_poll(g_relay_state.pool, 1000);
|
||||||
|
if (events_processed < 0) {
|
||||||
|
app_log(LOG_ERROR, "Error polling relay pool");
|
||||||
|
sleep(1);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**After:**
|
||||||
|
```c
|
||||||
|
while (g_relay_state.running) {
|
||||||
|
int events_processed = nostr_relay_pool_poll(g_relay_state.pool, 1000);
|
||||||
|
if (events_processed < 0) {
|
||||||
|
app_log(LOG_ERROR, "Error polling relay pool");
|
||||||
|
sleep(1);
|
||||||
|
} else if (events_processed == 0) {
|
||||||
|
// Prevent busy-wait when no relays are connected or no events pending.
|
||||||
|
// nostr_relay_pool_poll() returns immediately if all relays are disconnected
|
||||||
|
// because it skips the blocking nostr_ws_receive() call for non-connected relays.
|
||||||
|
usleep(100000); // 100ms idle sleep
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### Fix 2: Make `running` flag volatile (`src/relay_client.c`)
|
||||||
|
|
||||||
|
**File:** `src/relay_client.c`, line 39
|
||||||
|
|
||||||
|
**Before:**
|
||||||
|
```c
|
||||||
|
static struct {
|
||||||
|
int enabled;
|
||||||
|
int initialized;
|
||||||
|
int running;
|
||||||
|
...
|
||||||
|
```
|
||||||
|
|
||||||
|
**After:**
|
||||||
|
```c
|
||||||
|
static struct {
|
||||||
|
int enabled;
|
||||||
|
int initialized;
|
||||||
|
volatile int running;
|
||||||
|
...
|
||||||
|
```
|
||||||
|
|
||||||
|
### Fix 3: Add signal handler for graceful shutdown (`src/main.c`)
|
||||||
|
|
||||||
|
**File:** `src/main.c`
|
||||||
|
|
||||||
|
Add near the top (after includes):
|
||||||
|
```c
|
||||||
|
#include <signal.h>
|
||||||
|
|
||||||
|
static volatile sig_atomic_t g_shutdown_requested = 0;
|
||||||
|
|
||||||
|
static void shutdown_handler(int signum) {
|
||||||
|
(void)signum;
|
||||||
|
g_shutdown_requested = 1;
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
Register the handler before the FCGI loop (before line 2664):
|
||||||
|
```c
|
||||||
|
// Register signal handlers for graceful shutdown
|
||||||
|
signal(SIGTERM, shutdown_handler);
|
||||||
|
signal(SIGINT, shutdown_handler);
|
||||||
|
```
|
||||||
|
|
||||||
|
### Fix 4: Call `relay_client_stop()` on shutdown (`src/main.c`)
|
||||||
|
|
||||||
|
**File:** `src/main.c`
|
||||||
|
|
||||||
|
Modify the FCGI loop condition (line 2667) to also check the shutdown flag:
|
||||||
|
```c
|
||||||
|
while (FCGI_Accept() >= 0 && !g_shutdown_requested) {
|
||||||
|
```
|
||||||
|
|
||||||
|
Add cleanup after the FCGI loop exits (before `return 0` at line 3068):
|
||||||
|
```c
|
||||||
|
// Graceful shutdown
|
||||||
|
app_log(LOG_INFO, "Shutting down ginxsom...");
|
||||||
|
relay_client_stop();
|
||||||
|
app_log(LOG_INFO, "Ginxsom shutdown complete");
|
||||||
|
```
|
||||||
|
|
||||||
|
Also add the `relay_client.h` include at the top of `main.c`.
|
||||||
|
|
||||||
|
## Summary of Changes
|
||||||
|
|
||||||
|
| File | Change | Purpose |
|
||||||
|
|------|--------|---------|
|
||||||
|
| `src/relay_client.c:328-336` | Add `usleep(100000)` when poll returns 0 | Fix 100% CPU busy-wait |
|
||||||
|
| `src/relay_client.c:39` | Make `running` field `volatile` | Thread-safe flag checking |
|
||||||
|
| `src/main.c` (top) | Add signal handler function | Respond to SIGTERM/SIGINT |
|
||||||
|
| `src/main.c` (before FCGI loop) | Register signal handlers | Enable graceful shutdown |
|
||||||
|
| `src/main.c:2667` | Add `&& !g_shutdown_requested` to loop | Exit FCGI loop on signal |
|
||||||
|
| `src/main.c` (after FCGI loop) | Call `relay_client_stop()` | Clean up relay thread |
|
||||||
|
|
||||||
|
## Testing
|
||||||
|
|
||||||
|
1. Build locally: `make clean && make`
|
||||||
|
2. Run locally and verify CPU usage stays near 0% when idle
|
||||||
|
3. Send SIGTERM and verify clean shutdown (no SIGKILL needed)
|
||||||
|
4. Deploy to remote server via `deploy_lt.sh`
|
||||||
|
5. Monitor with `top` to confirm CPU usage is normal
|
||||||
@@ -10,8 +10,8 @@
|
|||||||
// Version information (auto-updated by build system)
|
// Version information (auto-updated by build system)
|
||||||
#define VERSION_MAJOR 0
|
#define VERSION_MAJOR 0
|
||||||
#define VERSION_MINOR 1
|
#define VERSION_MINOR 1
|
||||||
#define VERSION_PATCH 28
|
#define VERSION_PATCH 29
|
||||||
#define VERSION "v0.1.28"
|
#define VERSION "v0.1.29"
|
||||||
|
|
||||||
#include <stddef.h>
|
#include <stddef.h>
|
||||||
#include <stdint.h>
|
#include <stdint.h>
|
||||||
|
|||||||
26
src/main.c
26
src/main.c
@@ -23,6 +23,15 @@
|
|||||||
#include <unistd.h>
|
#include <unistd.h>
|
||||||
#include <dirent.h>
|
#include <dirent.h>
|
||||||
#include <sched.h>
|
#include <sched.h>
|
||||||
|
#include <signal.h>
|
||||||
|
|
||||||
|
// Shutdown flag set by signal handler - volatile sig_atomic_t is async-signal-safe
|
||||||
|
static volatile sig_atomic_t g_shutdown_requested = 0;
|
||||||
|
|
||||||
|
static void shutdown_handler(int signum) {
|
||||||
|
(void)signum;
|
||||||
|
g_shutdown_requested = 1;
|
||||||
|
}
|
||||||
|
|
||||||
// Centralized logging system (declaration in ginxsom.h)
|
// Centralized logging system (declaration in ginxsom.h)
|
||||||
void app_log(log_level_t level, const char *format, ...) {
|
void app_log(log_level_t level, const char *format, ...) {
|
||||||
@@ -2662,9 +2671,13 @@ if (!config_loaded /* && !initialize_server_config() */) {
|
|||||||
// THIS IS WHERE THE REQUESTS ENTER THE FastCGI
|
// THIS IS WHERE THE REQUESTS ENTER THE FastCGI
|
||||||
/////////////////////////////////////////////////////////////////////
|
/////////////////////////////////////////////////////////////////////
|
||||||
app_log(LOG_INFO, "FastCGI request loop starting - ready to accept requests");
|
app_log(LOG_INFO, "FastCGI request loop starting - ready to accept requests");
|
||||||
|
|
||||||
|
// Register signal handlers for graceful shutdown
|
||||||
|
signal(SIGTERM, shutdown_handler);
|
||||||
|
signal(SIGINT, shutdown_handler);
|
||||||
|
|
||||||
int first_request = 1;
|
int first_request = 1;
|
||||||
while (FCGI_Accept() >= 0) {
|
while (FCGI_Accept() >= 0 && !g_shutdown_requested) {
|
||||||
// Test stderr capture on first request
|
// Test stderr capture on first request
|
||||||
if (first_request) {
|
if (first_request) {
|
||||||
fprintf(stderr, "FCGI: First request received - testing nginx stderr capture\n");
|
fprintf(stderr, "FCGI: First request received - testing nginx stderr capture\n");
|
||||||
@@ -2794,6 +2807,8 @@ if (!config_loaded /* && !initialize_server_config() */) {
|
|||||||
operation = "list";
|
operation = "list";
|
||||||
} else if (strcmp(request_method, "GET") == 0 && strcmp(request_uri, "/auth") == 0) {
|
} else if (strcmp(request_method, "GET") == 0 && strcmp(request_uri, "/auth") == 0) {
|
||||||
operation = "challenge";
|
operation = "challenge";
|
||||||
|
} else if (strcmp(request_method, "GET") == 0 && strcmp(request_uri, "/health") == 0) {
|
||||||
|
operation = "health";
|
||||||
} else if (strcmp(request_method, "DELETE") == 0) {
|
} else if (strcmp(request_method, "DELETE") == 0) {
|
||||||
operation = "delete";
|
operation = "delete";
|
||||||
resource_hash = extract_sha256_from_uri(request_uri);
|
resource_hash = extract_sha256_from_uri(request_uri);
|
||||||
@@ -2831,6 +2846,8 @@ if (!config_loaded /* && !initialize_server_config() */) {
|
|||||||
// HEAD requests might not require auth depending on config - let handler decide
|
// HEAD requests might not require auth depending on config - let handler decide
|
||||||
} else if (strcmp(operation, "list") == 0) {
|
} else if (strcmp(operation, "list") == 0) {
|
||||||
// List operation might be optional auth - let handler decide
|
// List operation might be optional auth - let handler decide
|
||||||
|
} else if (strcmp(operation, "health") == 0) {
|
||||||
|
// Health endpoint is public and doesn't require authentication - let handler decide
|
||||||
} else if (strcmp(operation, "admin") == 0 && strcmp(request_uri, "/api/health") == 0) {
|
} else if (strcmp(operation, "admin") == 0 && strcmp(request_uri, "/api/health") == 0) {
|
||||||
// Health endpoint is public and doesn't require authentication - let handler decide
|
// Health endpoint is public and doesn't require authentication - let handler decide
|
||||||
} else if (strcmp(operation, "admin_event") == 0) {
|
} else if (strcmp(operation, "admin_event") == 0) {
|
||||||
@@ -3065,6 +3082,11 @@ if (!config_loaded /* && !initialize_server_config() */) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Graceful shutdown - stop relay client thread cleanly
|
||||||
|
app_log(LOG_INFO, "Shutting down ginxsom (shutdown_requested=%d)...", (int)g_shutdown_requested);
|
||||||
|
relay_client_stop();
|
||||||
|
app_log(LOG_INFO, "Ginxsom shutdown complete");
|
||||||
|
|
||||||
return 0;
|
return 0;
|
||||||
}
|
}
|
||||||
// ===== END UNUSED CODE =====
|
// ===== END UNUSED CODE =====
|
||||||
|
|||||||
@@ -36,7 +36,7 @@ void app_log(log_level_t level, const char* format, ...);
|
|||||||
static struct {
|
static struct {
|
||||||
int enabled;
|
int enabled;
|
||||||
int initialized;
|
int initialized;
|
||||||
int running;
|
volatile int running;
|
||||||
char db_path[512];
|
char db_path[512];
|
||||||
nostr_relay_pool_t* pool;
|
nostr_relay_pool_t* pool;
|
||||||
char** relay_urls;
|
char** relay_urls;
|
||||||
@@ -332,6 +332,11 @@ static void *relay_management_thread(void *arg) {
|
|||||||
if (events_processed < 0) {
|
if (events_processed < 0) {
|
||||||
app_log(LOG_ERROR, "Error polling relay pool");
|
app_log(LOG_ERROR, "Error polling relay pool");
|
||||||
sleep(1);
|
sleep(1);
|
||||||
|
} else if (events_processed == 0) {
|
||||||
|
// Prevent busy-wait: nostr_relay_pool_poll() returns immediately when all
|
||||||
|
// relays are disconnected because it skips the blocking nostr_ws_receive()
|
||||||
|
// call for non-connected relays. Without this sleep, the loop spins at 100% CPU.
|
||||||
|
usleep(100000); // 100ms idle sleep
|
||||||
}
|
}
|
||||||
// Pool handles all connection management, reconnection, and message processing
|
// Pool handles all connection management, reconnection, and message processing
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user