fix(status): reclaim crashed worker state#647
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Documentation previewThe documentation preview has been deployed for this pull request. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens rtp2httpd’s status subsystem against worker crashes by reclaiming per-client status slots on worker exit, switching shared status fields to lock-free 32-bit atomics, and moving status-page log writes to a supervisor-owned ring buffer fed via a non-blocking Unix datagram socket (removing process-shared mutexes that could remain locked after a crash).
Changes:
- Reap crashed/retiring worker status ownership via
waitpid()tracking, and enforce the max worker limit. - Replace process-shared mutex usage in status (client slots + logs) with lock-free atomics and supervisor-managed log ingestion.
- Add E2E coverage for worker recovery/maxclients reclamation and status log incremental/clear behavior; adjust IPv6 E2E fixture.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/worker.c | Uses atomic reads for disconnect requests on status slots. |
| src/supervisor.c | Tracks workers in a fixed-size array, reaps worker status on exit, drains worker log datagrams, and handles retire-on-reduce. |
| src/status.h | Introduces atomic-based client/log structures and new supervisor/worker status APIs. |
| src/status.c | Implements lock-free client slot lifecycle, worker reaping, and supervisor-owned log ring ingestion + SSE cursors. |
| src/connection.h | Updates SSE cursor state to epoch/sequence. |
| src/connection.c | Routes registration failures to 503 and removes pre-check capacity logic. |
| src/configuration.h | Adds CONFIG_MAX_WORKERS. |
| src/configuration.c | Validates worker count against CONFIG_MAX_WORKERS (config + CLI). |
| src/access_log.c | Adapts access-log rendering to the new client payload + owner PID model. |
| e2e/test_worker_recovery.py | Adds E2E tests for crash recovery, worker reduction reap, and log incremental/clear contract. |
| e2e/test_ipv6.py | Updates the IPv6 class-scoped fixture definition. |
| e2e/helpers/http.py | Adds helpers to poll SSE status payloads. |
| e2e/helpers/init.py | Exports new status polling helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0073d8ab04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Tie client admission and disconnect requests to recoverable slot ownership, and keep log control requests reliable across worker restarts.
Keep lock-free snapshots correctly ordered and prevent SSE buffer accounting from advancing past truncated writes.
Pass the version as an explicit format argument so static analysis can validate the bounded SSE append call accurately.
Use a recoverable lock-free admission guard so workers cannot reject all concurrent registrations at the capacity boundary, and verify every shared atomic used after fork is lock-free.
Apply the repository clang-format version so the status header passes CI lint checks.
Use the crash-recoverable shared byte shard for status totals and remove the redundant worker statistic that was cleared during worker recovery.
JSON-escape client addresses and service URLs before embedding request-derived values in status responses, preserving valid JSON and preventing markup injection.
Let status registration log the specific failure reason so expected max-client backpressure does not produce a duplicate error-level message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reclaim shared status client slots when a worker exits, before its worker index is reused. Client ownership and counters now use lock-free 32-bit atomics, and cumulative traffic is sharded per worker.
Move status-page log writes to a supervisor-owned ring fed by a non-blocking Unix datagram socket. This removes process-shared mutexes that could remain locked after a worker crash while preserving the existing SSE full/incremental/clear contract.
Keep retiring workers tracked through
waitpid()during worker-count reductions, enforce the 32-worker limit, and reject streams when status registration cannot reserve capacity.Root cause
Client slots were normally released only by worker-side connection cleanup. A worker killed by a signal could leave active slots and
total_clientsbehind indefinitely. The process-shared status mutexes also had no portable owner-death recovery path on macOS.Validation
PytestRemovedIn10Warningpromoted to an error: 10 passed