Skip to content

Fix agent startup race across PHP runtimes - #447

Open
teta2k wants to merge 9 commits into
mainfrom
agent/fix-agent-startup-race
Open

Fix agent startup race across PHP runtimes#447
teta2k wants to merge 9 commits into
mainfrom
agent/fix-agent-startup-race

Conversation

@teta2k

@teta2k teta2k commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fix a long-standing race in Aikido Agent startup.

The PHP extension now starts a short-lived launcher with posix_spawn(). A worker acquires a kernel-enforced exclusive lock before initialization and owns the Unix socket and gRPC server for its lifetime. The launcher waits until that shared socket is ready and retries if its worker fails during startup.

The issue was discovered while testing 1.5.18, but the affected startup design predates that release.

Problem

Previously, every PHP runtime initialized the Agent using several separate operations:

  1. Inspect the socket, PID file, and /proc.
  2. Decide whether an Agent is running.
  3. Kill processes and remove the socket if the state appears inconsistent.
  4. Start another Agent using fork() and daemon().

These operations were not performed as one indivisible decision: another process could start or stop an Agent between the check and the following action. Concurrent PHP initialization could therefore allow several processes to make conflicting startup and cleanup decisions.

There was an additional problem in FrankenPHP: it runs PHP in a multithreaded process. Calling fork() or daemon() there creates a child containing only the calling thread while retaining state inherited from the other threads. The result depends on timing, which explains why the missing socket occurred intermittently. FrankenPHP internals

Request-local attack detection does not depend on this socket, so SQL injection blocking could continue working while metadata RPC calls repeatedly failed.

New architecture

PHP extension
    |
    | posix_spawn() and wait
    v
short-lived launcher
    |
    | probe runtime-directory lock
    | start worker candidate
    | wait for socket; retry failed startup
    v
Agent worker
    |
    | acquire and retain runtime-directory lock
    | own socket and gRPC server

All SAPIs use this same path:

SAPI Relevant process model
PHP-FPM A master normally initializes extensions before forking workers, so worker count alone does not cause concurrent Agent startup. Overlapping masters or instances sharing the runtime directory may try to start the same Agent at the same time.
Apache mod_php PHP initializes during server startup, so worker count alone does not cause concurrent Agent startup. Overlapping server generations or instances may try to start the same Agent at the same time.
FrankenPHP classic PHP requests run in a multithreaded host, so the extension must not detach through fork()/daemon().
FrankenPHP worker Long-lived PHP workers use the same multithreaded host and the same Agent startup path.

FrankenPHP is the most direct production case because even one server is multithreaded, making the previous fork()/daemon() path unsafe. PHP-FPM and Apache still need correct singleton behavior when independent server startups overlap during deployments, reloads, or supervisor retries. The ten-process stress test deliberately forces that worst-case overlap to verify the singleton guarantee; it does not model normal worker startup or represent a production failure rate.

The launcher probes the runtime-directory lock before starting a candidate. The worker then opens and locks that directory itself. If candidates start at the same time, only the worker that acquires the lock initializes; the others exit and their launchers continue waiting for the shared socket.

The lock is held on the existing /run/aikido-<version> directory, so no additional singleton file or fixed file-descriptor handoff is needed. A worker that fails before creating the socket releases the lock automatically. Its launcher reaps the failed process and starts a replacement.

PHP waits for and reaps only the short-lived launcher. The long-lived worker is detached and adopted by the container init process or subreaper.

What changed

  • Move singleton selection and Agent lifecycle ownership into the Go executable.
  • Replace the non-atomic PID, socket, and /proc checks with one atomic directory lock.
  • Remove process killing and socket cleanup from the PHP extension.
  • Remove fork() and daemon() from the PHP host process.
  • Remove the PID file, which was only used by the old C++ process inspection.
  • Remove the standalone Agent executable's obsolete direct cgo dependency.
  • Use one startup path for PHP-FPM, Apache, and both FrankenPHP modes.
  • Keep the launcher and worker in the same executable to avoid packaging another versioned binary.

Stress-test method

The same concurrent-start test was run separately with PHP 7.2 NTS, PHP 8.2 ZTS, PHP 8.5 NTS, and PHP 8.5 ZTS.

The purpose was to verify the singleton guarantee: all PHP processes using the same Aikido version in one container must share exactly one Agent worker and one Unix socket. Multiple workers would compete for the same socket and reproduce the startup race this PR fixes.

In each trial the test:

  1. stopped any Agent left by the previous trial;
  2. removed the stale aikido-agent.sock;
  3. prepared ten PHP processes behind a start barrier and released them together;
  4. required exactly one live --agent-worker process and a Unix socket;
  5. stopped the PHP processes and repeated the trial 100 times.

The PHP built-in server (php -S) was used only as a small test driver that loads the extension in ten independent PHP processes and keeps them alive. Each launcher stopped itself immediately before executing PHP; one SIGCONT then released all ten launchers together:

php_pids=""
for port in $(seq 15001 15010); do
    sh -c 'kill -STOP $$; exec "$@"' sh \
        php -n -d "extension=$PHP_EXTENSION" \
        -S "127.0.0.1:$port" -t /tmp &
    php_pids="$php_pids $!"
done

# After all ten launcher PIDs report the stopped (`T`) state:
kill -CONT $php_pids

The barrier makes all ten extension initializations begin at the same time instead of depending on shell and scheduler timing. This isolates the cross-process Agent startup race; it is not presented as a production SAPI test. Actual FrankenPHP classic and worker processes were tested separately. Apache and PHP-FPM were exercised by the normal integration-test matrix, not by this 100-trial stress harness.

Local stress results

Each value is successful trials out of 100. A trial passed only when all ten synchronized php -S processes converged on exactly one live Agent worker and the Unix socket existed. A duplicate or missing Agent, or a missing socket, counted as a failure. These deliberately synchronized stress results are not production failure rates.

PHP profile Previous implementation This PR
PHP 7.2 NTS 77/100 100/100
PHP 8.2 ZTS 68/100 100/100
PHP 8.5 NTS 16/100 100/100
PHP 8.5 ZTS 11/100 100/100

Actual FrankenPHP classic and worker servers were also tested separately with PHP 8.2 and PHP 8.5. All four patched profiles completed 100/100 startup trials.

A separate test killed the selected worker before it created the socket. The waiting launchers reaped the failed process and recovered with one replacement worker in 100/100 trials.

Validation

  • Go unit tests and go vet pass with CGO_ENABLED=0.
  • Production Agent build passes.
  • Extension builds pass for PHP 7.2 NTS, PHP 8.2 ZTS, PHP 8.5 NTS, and PHP 8.5 ZTS.
  • Concurrent startup passes 100/100 for all four PHP profiles.
  • FrankenPHP classic and worker startup passes 100/100 with PHP 8.2 and PHP 8.5.
  • The full integration matrix passes for Apache mod_php, Apache/PHP-FPM, Nginx/PHP-FPM, PHP built-in, and FrankenPHP classic and worker.
  • git diff --check passes.

@teta2k teta2k changed the title Make agent startup atomic across PHP runtimes Fix agent startup race across PHP runtimes Aug 13, 2026
@teta2k
teta2k marked this pull request as ready for review August 14, 2026 07:48

@tomaisthorpe tomaisthorpe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's potential issue here if a worker fails during startup, something like the following:

  1. N processes start
  2. One process wins the lock, the other processes continue as another process has the lock, they report success
  3. During startup, the winning worker fails.
  4. Lock is released, but it's not noticed, and isn't reattempted.

Comment thread lib/agent/main.go Outdated
@teta2k

teta2k commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

I think there's potential issue here if a worker fails during startup, something like the following:

  1. N processes start
  2. One process wins the lock, the other processes continue as another process has the lock, they report success
  3. During startup, the winning worker fails.
  4. Lock is released, but it's not noticed, and isn't reattempted.

Thanks. I updated the launcher to wait for the shared socket instead of treating a held lock as success. If the winning worker dies before the socket is ready, its launcher reaps it and retries while the other launchers keep waiting. I tested this by killing the winning worker before socket creation, and it recovered with exactly one worker in 100/100 runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants