diff --git a/docs/ISSUE_reload_persistent_coroutine_segv.md b/docs/ISSUE_reload_persistent_coroutine_segv.md new file mode 100644 index 0000000..5c7f9d0 --- /dev/null +++ b/docs/ISSUE_reload_persistent_coroutine_segv.md @@ -0,0 +1,154 @@ +# Known issue — SIGSEGV during pool reload when the bootloader spawns a long‑lived coroutine (#93) + +Status: reload **deadlock** is fixed (see "Fix that shipped"). The **intermittent +crash** it exposes is now **root-caused and filed as an ext/async bug**: +[true-async/php-async#176](https://github.com/true-async/php-async/issues/176). + +**Root cause (confirmed — code + ASAN + gdb):** `async_thread_create_closure` +(`thread.c`) does `memcpy(&func, copy->func, sizeof(zend_op_array))` — a *shallow* +copy of only the **top-level** op_array. Its `dynamic_func_defs` (nested closures +declared inside the transferred closure, e.g. the `spawn(fn: while(true) delay())` +inside the bootloader) are copied as a pointer → **all workers share the same +nested op_array** from the persistent snapshot (proven: same `func` address +`0x531…` on every worker; top-level closures are per-worker `0x7fff…`). Then +`zend_create_closure` writes a per-thread arena `run_time_cache` **direct pointer** +into that shared op_array (`zend_closures.c:817`), which is read from another +thread / after that worker's arena is freed → dangling read → SEGV. Fix belongs +in ext/async: deep-copy `dynamic_func_defs` per worker (like the top-level). +Everything below is the raw investigation trail. + +--- + +## 1. Background: the original deadlock (fixed) + +`HttpServer::reload()` under the built‑in worker pool (`mode=pool`, sync mode) +rotates the cohort: it bumps the epoch, each worker retires (drain #74 → stop → +`start()` returns), the ThreadPool `reload()` waits for every old worker to post +its exit token, then submits the fresh cohort. + +A worker only posts its exit token after its scheduler has fully drained +(`ZEND_ASYNC_RUN_SCHEDULER_AFTER_MAIN` in `thread_pool_worker_handler`). That +drain loops while `has_handles` is true. If the **bootloader spawned a +never‑ending coroutine into the worker's main scope** — e.g. laravel‑spawn's +async DB pool arms a recurring `PDO::ATTR_POOL_HEALTHCHECK_INTERVAL` timer, or +any `Async\spawn(fn: while(true) …)` — that coroutine's live reactor handle keeps +`has_handles` true forever. Nothing cancels it on retire (drain #74 only touches +`server_scope`, not the main scope), so the drain never completes, the exit token +is never sent, and `ThreadPool::reload()` blocks forever. The user sees: + +``` +reload.start workers=8 +[true-async-server] worker thread exited cleanly ×8 (old cohort — normal) + +``` + +Reproduced on Linux with the real laravel‑spawn app (the 8× "exited cleanly" is +the normal old cohort retiring; the bug is the missing `reload.done`). + +## 2. Fix that shipped + +- **ext/async `thread_pool.c`** — at the worker's `done:` exit, for sync‑mode + pools only (`!pool->coroutine_mode`), call `start_graceful_shutdown()` before + `RUN_SCHEDULER_AFTER_MAIN`. It cancels the main‑scope stragglers and arms the + drain's escape valve, so the drain completes and the exit token is posted. + Gated to sync mode so coroutine‑mode pools keep draining their in‑flight task + coroutines (test `thread_pool/077`). +- **server `http_server_class.c`** — lifecycle logs so the journal reads + `worker shutting down (reason=reload) → worker exited` instead of the + confusing bare `worker thread exited cleanly ×N`. + +Result: deadlock gone. **Real laravel‑spawn server: 10/10 reloads clean.** +Server hot‑reload phpt 5/5, ThreadPool suite 78/79 (077 green via the gate), +zero compiler warnings. + +## 3. The remaining problem: intermittent SIGSEGV under stress + +Under an aggressive synthetic harness (below), **~50 % of reloads SIGSEGV.** The +real server did not hit it in 10 reloads, but it is a real latent crash. + +### Proven facts (gdb, release `-O2 -g` build) + +Backtrace is stable: + +``` +#0 ZEND_INIT_FCALL_SPEC_CONST_HANDLER Zend/zend_vm_execute.h:4186 + fbc = CACHED_PTR(opline->result.num); // result.num = 0 +#1 execute_ex +#2 zend_call_function Zend/zend_execute_API.c:1155 +#3 async_coroutine_execute ext/async/coroutine.c:532 // first start (SET_STARTED just above) +#4 fiber_entry ext/async/scheduler.c:1822 +``` + +Inspecting the crashing coroutine: + +- It is the **bootloader‑spawned persistent coroutine itself** — its closure is + `{closure:{closure:…repro:25}:30}` (the `while(true){ delay() }`). +- The closure and its op_array are **VALID**: `fci_cache.function_handler` is a + live `ZEND_USER_FUNCTION` (type 2), the closure object `refcount = 2`. + **It is NOT freed** (an earlier "op_array freed" guess was wrong). +- Coroutine flags: `STARTED = 1` (set at coroutine.c:524 immediately before the + crash — i.e. this is its **first** execution), `CANCELLED = 0`, `MAIN = 0`. +- The fault: `CACHED_PTR(0)` dereferences a **NULL run‑time cache**, even though + `op_array.cache_size = 8`. So the op_array expects a run‑time cache but the + `ZEND_MAP_PTR` resolves to NULL in this execution. + +### What was ruled out + +- **"Coroutine never ticked before reload"** — ruled out. A first‑run probe + shows both workers' persistent coroutines run *during serving* (before the + reload trigger). The crash happens later, in the rotation phase (a 3rd + first‑run — the replacement worker re‑booting the bootloader — is observed + after `reload.start`, then the crash). +- **Cancelled‑coroutine‑executed** — ruled out; the crashing coroutine has + `CANCELLED = 0`. + +### Failed fix attempt (do not repeat blindly) + +Cancelling the main scope earlier, server‑side, in `start()`'s post‑wakeup path +(right after the #74 `server_scope` drain, on the still‑valid `start()` +coroutine, with the root coroutine `SET_PROTECTED`) made the crash **worse** +(~75 %). So the crash is **not** simply "the context is torn down by the time we +reach the pool `done:`": executing/cancelling this coroutine is unsafe at both +points. Reverted. + +### Open questions for next session + +1. Why does a **cloned/transferred closure** (created from the bootloader + snapshot in the worker) have a **NULL run‑time cache** on the execution that + crashes, while the *same* coroutine executed fine during serving? +2. Who **fresh‑starts** this coroutine during the rotation drain, and why is it a + fresh start (not a resume) when it already ran during serving? Is there a + second instance (replacement worker) vs the old instance being re‑entered? +3. Is the fix better placed in ext/async (make the drain skip/deallocate + never‑resumable coroutines, or ensure the run‑time cache is bound before + execution) rather than in the server? + +## 4. Reproduction + +Build the release stack (php‑src `true-async-stable` + ext/async `main` → +`/home/edmond/php-release`, then the server extension against it). Run the +synthetic harness — a trivial bootloader that leaves one persistent coroutine, +which is all it takes: + +```php +// repro_persistent.php (2 workers, watcher hot‑reload) +$config = (new HttpServerConfig()) + ->addListener('127.0.0.1', (int)$argv[1]) + ->setWorkers(2) + ->setLogSeverity(TrueAsync\LogSeverity::INFO)->setLogStream(STDERR) + ->setBootloader(static function () use ($code) { + define('APP_V', (string) include $code); + Async\spawn(static function () { // <-- the trigger: a never‑ending + while (true) { Async\delay(1000); } // main‑scope coroutine + }); + }) + ->enableHotReload([$dir], ['php'], 150, 1000); +// a spawned driver curls until v1, rewrites the watched file to v2, curls for v2. +``` + +Loop it on fresh ports; ~half the runs SIGSEGV (rc 139), the rest complete with +`reload.done` + `after=v2`. The crash needs the parent reactor busy (the driver's +blocking `shell_exec(curl)` on the parent supplies the timing); the real server, +whose requests come from external processes, has not reproduced it. + +Full harness + gdb logs from the investigation live under the session scratchpad. diff --git a/docs/repro_reload_persistent_coroutine_segv.php b/docs/repro_reload_persistent_coroutine_segv.php new file mode 100644 index 0000000..e980b28 --- /dev/null +++ b/docs/repro_reload_persistent_coroutine_segv.php @@ -0,0 +1,58 @@ +addListener('127.0.0.1', $port) + ->setReadTimeout(5)->setWriteTimeout(5) + ->setWorkers(2) + ->setLogSeverity(LogSeverity::INFO)->setLogStream(STDERR) + ->setBootloader(static function () use ($code, $mode): void { + define('APP_V', (string) include $code); + if ($mode === 'persistent') { + // Persistent background coroutine — arms a recurring timer on the + // worker reactor that outlives any per-request scope. + spawn(static function () { + while (true) { + \Async\delay(1000); + } + }); + } + }) + ->enableHotReload([$dir], ['php'], 150, 1000); + +$server = new HttpServer($config); +$server->addHttpHandler(function ($req, $res) { + $res->setStatusCode(200)->setBody(APP_V); +}); + +spawn(function () use ($port, $code, $dir) { + $curl = static fn (): string => (string) shell_exec(sprintf( + 'curl -s --max-time 2 http://127.0.0.1:%d/ 2>/dev/null', $port)); + $v1 = ''; + for ($i = 0; $i < 50 && $v1 !== 'v1'; $i++) { usleep(200000); $v1 = $curl(); } + fwrite(STDOUT, "PROOF before=$v1\n"); + file_put_contents($code, "start(); diff --git a/php_true_async_server.h b/php_true_async_server.h index d45cad6..7484d2d 100644 --- a/php_true_async_server.h +++ b/php_true_async_server.h @@ -24,8 +24,8 @@ * * It is kept flat at the extension root (not under include/) on purpose: * php-src's static-build codegen (build/genif.sh + build/print_include.awk) - * discovers each bundled extension's module_entry by globbing - * ext//*.h non-recursively and grepping the matched files for a + * discovers each bundled extension's module_entry by globbing the + * ext/ directory for *.h files non-recursively and grepping them for a * "phpext_" line — it does not follow nested #include paths. Keeping the * module-entry declaration here, at ext/server/php_true_async_server.h, * lets static (non-shared) builds such as static-php-cli find diff --git a/src/http_server_class.c b/src/http_server_class.c index 276a82c..20ad9d6 100644 --- a/src/http_server_class.c +++ b/src/http_server_class.c @@ -609,6 +609,9 @@ static void http_server_reload_stop_entry(void) http_server_worker_inbox_retire(server); server->stopping = true; + fprintf(stderr, "[true-async-server] worker shutting down (reason=reload, grace=%us)\n", + server->shutdown_timeout_s); + fflush(stderr); http_server_do_stop(server, "reload"); } @@ -1927,11 +1930,12 @@ static void pool_worker_handler(zend_async_event_t *event, void *vctx) fflush(stderr); zend_clear_exception(); } else { - /* Even no-exception exit while server should still be running - * is suspicious — e.g. a bailout that left EG(exception) NULL - * (OOM during cleanup). Log a lighter notice. */ + /* Clean exit — the normal path when a worker retires on reload + * (paired with the "worker shutting down" line above) or on a + * graceful stop. A clean exit while the server should still be + * running would instead point at a swallowed bailout. */ fprintf(stderr, - "[true-async-server] worker thread exited cleanly\n"); + "[true-async-server] worker exited\n"); fflush(stderr); } } else {