You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Scope reduced and two spec errors fixed by the unblocker; two more fixed by the
resolver (dependency route, panic-policy boundary) — see the two boxed sections
below. This issue failed three attempts. It never once failed the gate: bash scripts/gate.sh default passed on four of the five runs, and the one
failure was the abort-stub crash described below. It died to fourteen blocking
review findings across four rounds, none of them repeating, while thread.rs
grew from 824 to 2169 lines (+117%, measured in #102). That is the signature of
a diff too large to converge — the same one that killed #11 before it was split
into #79/#80/#75.
drain_chan was unowned, and this issue is why. See "The drain signal".
Port vendor/frankenphp/phpmainthread.go (316 lines), phpthread.go (296) and threadinactive.go (67).
Files you own:crates/frankenrust-core/src/thread.rs, src/thread_inactive.rs, src/callbacks/mainthread.rs, src/callbacks/thread.rs. Plus crates/frankenrust-core/src/lib.rs, for the
two pub mod declarations (see #49 — an undeclared .rs is silently never
compiled and still passes every gate step), additive edits to crates/frankenrust-sys/build.rs, and — added by the resolver — crates/frankenrust-core/Cargo.toml and the workspace Cargo.lock. Do not
touch any other callback module — #11 and #12 are editing those in parallel.
build.rs's bindgen builder uses an explicit allowlist_function list, so it
emits only the named items. frankenphp_init_thread_metrics and frankenphp_destroy_thread_metrics (frankenphp.h:216-217), frankenphp_force_kill_thread and frankenphp_release_thread_for_kill
(:228-229), and frankenphp_init_persistent_string (:209, for go_init_os_env) are all invisible today. Add them additively, the same rule #11 states for the same file; do not hand-write an extern "C" block in frankenrust-core instead. #11 also adds frankenphp_init_persistent_string;
that duplicate resolves trivially.
Dependencies: what to declare and what not to
frankenrust-core has exactly one dependency today (frankenrust-sys) and no
external crates at all. This issue needs two capabilities that are not there.
Both routes are decided; do not improvise a third.
libc malloc/free for go_get_custom_php_ini: get them from bindgen, not
from a new crate.crates/frankenrust-sys/wrapper.h includes <php.h>,
which includes <stdlib.h> unconditionally (main/php.h:210), so bindgen
already parses both declarations and only the explicit allowlist is hiding
them. Add .allowlist_function("malloc") and .allowlist_function("free")
to the same additive block as the five frankenphp_* symbols above, and call
them through frankenrust_sys. This keeps the crate free of a libc
dependency. If — and only if — bindgen does not emit them, add libc = "0.2" to crates/frankenrust-core/Cargo.toml and say so in your
final message. Do not hand-write extern "C" { fn malloc(..) } in thread.rs, and do not fall back to CString::into_raw; that is the UB
the allocator section below names and that docs: PORTING-NOTES.md:120 tells agents to hand CString::into_raw memory to C's free() #18 already had to fix in the docs
once.
crossbeam-channel for the drain signal must be declared. It is not in Cargo.lock at all. Add crossbeam-channel = "0.5" to crates/frankenrust-core/Cargo.toml; Cargo.lock updates as a side effect of
the build and must be committed with it. The gate does not pass --locked and
the dev container has network plus a shared cargo registry volume, so the
fetch is fine. docs/PORTING-NOTES.md:123-124 already prescribes this crate
for exactly this construct. Note server: regular-thread handler, hyper HTTP/1.1 front end, and the async/pthread bridge #13 will also add a dependency to a Cargo.toml; a lockfile conflict there resolves trivially.
This is the section that decides whether this attempt converges. #102 measured
the previous three: thread.rs reached 2169 lines against a 679-line oracle,
and the reviewer who found the cause named it — "the panic-recovery machinery
this diff invents, which has no upstream counterpart." Round 4 then blocked on
a CAS to ShuttingDown that upstream does not have, introduced to support that
same machinery.
The shared unwind guard at the extern "C" boundary belongs to #78, in crates/frankenrust-core/src/callbacks/mod.rs, which you do not own. Your
in-lane obligation is narrower and entirely negative: do not panic. No unwrap/expect/slice-indexing on anything a caller controls; a registry
lookup for an out-of-range or unbooted thread_index returns rather than
panics; a poisoned lock is handled, not unwrapped. Do not invent
per-callback catch_unwind wrappers, thread-health tracking, or new state-machine
edges to support them. If a reviewer asks for panic recovery here, that is a
request for machinery with no oracle — point at this paragraph and at #78, fix
the panic source instead, and say so in your final message. The stderr rule
below still applies to any diagnostic you do write.
Initialisation order — none of this is optional
From phpmainthread.go:44-85, which comments each constraint:
A registry slot for index 0 must exist beforefrankenphp_new_main_thread().
Extensions touch environment variables during module startup, and the main
thread's thread_index defaults to 0, so callbacks fired during startup route
to slot 0 (phpmainthread.go:53-58).
frankenphp_new_main_thread(num_threads), then block until state Ready.
The argument is the TSRM pre-allocation hint.
frankenphp_init_thread_metrics(max_threads) must run after the main
thread is Ready — max_threads is only final inside go_frankenphp_main_thread_is_ready — and before any frankenphp_new_php_thread, because php_thread writes thread_metrics[thread_index] unguarded at frankenphp.c:1541 and would
otherwise null-deref.
Shutdown: every PHP thread must reach Donebefore the main thread is
released, because php_main calls tsrm_shutdown() as soon as it unparks.
Then wait for Reserved, then frankenphp_destroy_thread_metrics().
The two blocking callbacks
go_frankenphp_main_thread_is_readymust block until shutdown
(phpmainthread.go:248-257: set Ready, then wait for Done or Rebooting).
It is called at frankenphp.c:1710 and the whole remaining lifetime of the C
main pthread is spent inside it.
go_frankenphp_before_script_execution must block until work arrives and return
the script path as a NUL-terminated pointer that stays valid until at least after_script_execution. C never frees it. Returning NULL terminates the C loop
and the thread.
Note it deliberately reads the thread's handler without taking the handler
lock (phpthread.go:236-237). Taking it deadlocks against setHandler, which
holds the write lock across the whole rendezvous while waiting for a state this
very callback must publish. Safety comes from the state handshake alone —
reproduce that, and write down why in a comment.
The drain signal — drainChan is live, the drain() method is not
The previous version of this issue said only "do not port the dead drain()
hook", and said nothing about drainChan. That is half right and it cost five
of the fourteen review findings.
The drain()method on threadHandler (phpthread.go:44-49) is dead:
its only caller, drainWorkerThreads, does not exist anywhere in the tree.
Do not port it.
drainChan (phpthread.go:23) is load-bearing. It is closed in exactly
the two functions you are porting — shutdown() (phpthread.go:124) and setHandler() (:162) — each immediately after the state change the parked
handler must observe, and it is recreated at :70, :145 and :166. It is
selected on by threadregular.go:111 and threadworker.go:242.
Without it, a handler parked on a request channel never observes ShuttingDown,
force-kill cannot interrupt a Rust channel receive, and the deliberately
unbounded Done wait below hangs forever. #13's body already specifies
"generation-tagged drain ... closed at phpthread.go:124 and :162" as
something it will select on, but #13 owns neither thread.rs nor the callbacks. It is yours. Go's select composes a channel receive with the request
channel and std has no select, so expose it as something a handler can compose
with (crossbeam_channel is the prescribed primitive; see "Dependencies"
above). Closing a Go channel twice panics; upstream's close-then-replace is
exactly once per generation, so a generation tag is the Rust-side obligation.
The inactive handler parks on ThreadState itself, which the state change
already wakes, so it does not need to select on anything.
Its only two receivers (threadregular.go:111, threadworker.go:242) are out
of scope, so nothing else in the gate can catch a wrong generation tag — it gets
its own acceptance criterion below.
The handler contract C guarantees
Reviewers found violations of this four separate times. State it in code and
uphold it:
C calls go_frankenphp_after_script_executiononly for a non-NULL script
(frankenphp.c:1506-1562). So a callback must never let a handler return a
script path and then discard it: by then a real handler has already taken a
request off its channel, and that request's completion signal is never sent.
Upstream has exactly one NULL path — scriptName == "" (phpthread.go:241-243)
— returned before the dequeue. Any health check, retirement decision or
path validation you add must happen on the same side of the handler call.
A thread that retires itself must not strand a controller. Upstream leaves a
stable state only by publishing one (threadregular.go:49-53 sets Ready, threadinactive.go:28-29 sets Inactive), and ShuttingDown is published
only by shutdown() (phpthread.go:118). If you add a self-retire edge, the
slot must end reachable — a Done slot that no controller CASes back to Reserved is a permanently lost registry slot, and a controller parked in RequestSafeStateChange never wakes for it.
No eprintln!/println! anywhere inside a callback, or inside any panic
recovery arm. They panic on a write error, Rust sets SIGPIPE to SIG_IGN,
and a panic escaping an extern "C" frame aborts the process — which is
exactly what the containment exists to prevent. Use let _ = writeln!(io::stderr().lock(), ...). Same for the Rust-side
lifecycle functions, where an escaping panic skips release_main_thread and
parks php_main forever.
Allocator rules
go_get_custom_php_ini (phpmainthread.go:288-315) returns key=value\n
concatenated (map order, no escaping, no quoting — :304-315), allocated with
libc malloc (routed through frankenrust_sys — see "Dependencies"), and C free()s it at frankenphp.c:1723. CString::into_raw uses Rust's global
allocator; handing that pointer to free() is undefined behaviour. Check the malloc result for NULL. The same rule applies to go_read_cookies in #12.
Do not skip the disableTimeouts branch at :294-300 (max_execution_time=0, max_input_time=-1). The dev container defines ZEND_MAX_EXECUTION_TIMERS, so
C always passes false (frankenphp.c:1681) and the gate can never catch its
omission — cover it with a unit test instead.
Force-kill slots
frankenphp_register_thread_for_kill (frankenphp.c:283-300) runs on the PHP
thread itself, right after ts_resource(0), and hands you a force_kill_slot by value holding &EG(vm_interrupt), &EG(timed_out) and pthread_self().
Store it verbatim; hand it back verbatim to frankenphp_force_kill_thread.
Call frankenphp_release_thread_for_kill on the previous slot before
overwriting — a thread can reboot and re-register (phpthread.go:263-268) —
and again on clear.
go_frankenphp_clear_force_kill_slot is called at frankenphp.c:1598
immediately before ts_free_thread() at :1602, because that call frees the
TSRM storage those pointers point into. The write lock taken in clear must
exclude any concurrent kill, so a killer either completed before the lock or
sees a zeroed slot.
The struct holds raw pointers and a pthread_t; it is not Send/Sync. Wrap
it explicitly with a // SAFETY: comment naming the invariant.
Note the lock direction is inverted from intuition: the writers (store, clear)
run on the PHP thread, the reader (send-kill) runs from the async side.
Reproduce the invisible restart
On a zend_catch bailout that leaves the thread unhealthy, C calls frankenphp_new_php_thread(thread_index)from inside the dying thread
(frankenphp.c:1611-1616) and skips go_frankenphp_on_thread_shutdown
entirely. The registry must tolerate a fresh thread appearing at an index with
no prior shutdown notification, in whatever state the old one left behind.
Do not bound the shutdown and reboot waits
phpmainthread.go:179-196 is a 15-line comment explaining that bounding them is
a use-after-free confirmed by CI (php/frankenphp#2573): php_main calls tsrm_shutdown() as soon as the main state changes, and that requires every PHP
thread to have exited. A thread parked in a blocking call may never yield to force_kill. The pattern is: grace period, then arm force-kill, then wait unbounded. Keep it.
Scaling exclusion
drainPHPThreads opens with scalingMu.Lock() and the comment "disallow any
scaling or restarting threads while draining" (phpmainthread.go:88-90); getInactivePHPThread (:231-246) is package-private and every caller holds
that same mutex. Port the exclusion, not just the functions: a getInactivePHPThread that boots a Reserved slot the drain has already passed
runs ts_resource(0) while php_main is inside tsrm_shutdown() — the exact
use-after-free cited above. If the Rust API surface is pub, the guard has to
be in the type, not in a convention.
Inactive handler
threadinactive.go: parks in Inactive, never returns a script name, and its afterScriptExecution panics ("inactive threads should not execute scripts").
Its beforeScriptExecution is written as tail recursion (threadinactive.go:36, :41) — write it as a loop, for the same fixed-stack reason as #8. Its afterScriptExecution is unreachable by the handler contract above; do not
port the panic literally — return, and note in a comment that C cannot call it
for a NULL script.
Acceptance
The one-shot boot/drain test, in a subprocess. Boot the main thread plus
N PHP threads in the inactive state and assert every thread reaches Inactive, then drain them and assert every PHP thread reaches Reserved and the main thread's blocking callback returns. PHP's own php_module_shutdown() then aborts in core: the output and input SAPI callbacks #12's go_write_headers abort-stub,
before go_frankenphp_shutdown_main_thread can publish the main thread's Reserved — so run this in a subprocess and assert exactly that: the
lifecycle markers above, then death in the named stub, mirroring the
existing pattern in tests/abort_stub.rs. Write it so it fails loudly, not
silently passes, once the abort stops happening. core: the end-to-end main-thread lifecycle test — php_module_shutdown() calls go_write_headers, so it cannot run until #12 lands #97 owns the positive tsrm_shutdown assertion and depends on core: the output and input SAPI callbacks #12.
The three-cycle test, with no main thread and no C thread. Drive
per-slot boot() -> Inactive -> shutdown() -> Reserved three times in
one test, over the Rust state machine alone, without deadlocking. This
criterion is deliberately not drainPHPThreads: that function drives the
main thread Ready -> Done -> Reserved once and ends with phpThreads = nil (phpmainthread.go:104-107), so it is not a repeatable
cycle in upstream either, and repeating it would need a main-thread stand-in
this issue does not want you to invent. What is repeatable is the per-slot
one: shutdown() ends at Reserved (phpthread.go:147) precisely so getInactivePHPThread can CAS Reserved -> BootRequested and boot it
again (:238-242). Use a Rust stand-in for the PHP pthread's side of the
handshake — publishing TransitionInProgress, parking, exiting — in place of frankenphp_new_php_thread; the seam is whatever your boot() calls to
start the C thread, and it must be substitutable in tests without a #[cfg]
fork of the production path. It must also cover a set_handler transition: TransitionRequested -> InProgress -> Complete with a second, test-only
handler, since server: regular-thread handler, hyper HTTP/1.1 front end, and the async/pthread bridge #13/core: worker mode -- the frankenphp_handle_request park/resume loop #14 are not here to provide one.
A drain-signal unit test. Nothing else in the gate can catch a wrong
generation tag, because both receivers are out of scope. Assert: a receiver
taken before shutdown() observes the close (a disconnected/ready receive);
a receiver taken after a completed set_handler does not see the previous
generation's close; and closing twice within one generation is impossible by
construction (the API gives no way to express it — say in your final message
which construction you used).
go_get_custom_php_ini: a test that it renders a two-key ini map as key=value\n pairs and that the returned pointer is freeable by free(),
and one covering the disableTimeouts branch.
bash scripts/gate.sh default passes.
Note that cargo test runs a binary's tests in parallel threads: anything that
boots a main thread must be one #[test], or serialized behind a mutex. Note
also that an empty go_init_os_env satisfies every criterion above — say so
explicitly in your final message if you leave it thin.
zend_first_try/zend_catch is setjmp/longjmp, not unwinding. A zend_bailout() — OOM, exit(), fatal error, timeout — longjmps straight past
every intervening frame to frankenphp.c:1565, running no destructors. Never
hold a MutexGuard, Box or Vec in a Rust frame across a call into PHP that
can bail; the mutex stays locked forever.
Every one of these callbacks runs on a C-created pthread that has already done ts_resource(0). Touching PHP state from any other thread has no TSRM context
and will null-deref.
frankenphp_sapi_module.log_message is frankenphp_log_message
(frankenphp.c:1385-1387) -> go_log, which is core: the output and input SAPI callbacks #12's abort-stub, and PHP's log_errors defaults to on with error_log unset — so any warning during php_module_startup aborts your test process. display_startup_errors routes
to go_ub_write the same way. Not yours to fix; the in-lane lever is the
test's own ini map, which go_get_custom_php_ini renders.
Every unsafe block needs a // SAFETY: comment naming the invariant and
where it is established. The recurring one here is "called only from php_thread() on the thread that owns thread_index."
Do not modify anything under vendor/frankenphp/.
Out of scope
The regular handler (#13) and the worker handler (#14). Request plumbing (#11, #12). max_threads=auto and everything it reads (#103) — leave the seam.
The tsrm_shutdown-survival and three-cycle C-level lifecycle test (#97).
The extern "C" unwind guard (#78). Autoscaling, metrics beyond the
init/destroy calls, max_requests, opcache reset and reboot-all-threads
(rebootAllThreads), the debugstate.go surface, and the dead drain() method on the handler interface — but not drainChan, which is yours; see
above.
phpThread.reboot() / forceReboot() (phpthread.go:82-114) are also out of
scope: their only callers are max_requests and rebootAllThreads, both
excluded. Say which way you went in your final message.
Go's os.Environ semantics in go_init_os_env — duplicate keys and entries
with no = — are a known divergence from std::env::vars_os, already filed as #98. Do not expand this diff to fix it.
Scope reduced and two spec errors fixed by the unblocker; two more fixed by the
resolver (dependency route, panic-policy boundary) — see the two boxed sections
below. This issue failed three attempts. It never once failed the gate:
bash scripts/gate.sh defaultpassed on four of the five runs, and the onefailure was the abort-stub crash described below. It died to fourteen blocking
review findings across four rounds, none of them repeating, while
thread.rsgrew from 824 to 2169 lines (+117%, measured in #102). That is the signature of
a diff too large to converge — the same one that killed #11 before it was split
into #79/#80/#75.
Three things changed:
max_threads=autois gone, to core: resolve max_threads=auto from the PHP memory limit and total system memory #103.max_threadshere is a plain countsupplied by the caller. Leave a named seam where the resolution belongs
(
phpmainthread.go:250, beforeSet(Ready)); core: resolve max_threads=auto from the PHP memory limit and total system memory #103 fills it in.tsrm_shutdown, three times" test isgone, to core: the end-to-end main-thread lifecycle test — php_module_shutdown() calls go_write_headers, so it cannot run until #12 lands #97. It was unsatisfiable in this lane and it killed attempt 1
outright:
php_module_shutdown()callssapi_flush()unconditionally, whichreaches
go_write_headersandgo_sapi_flush— both still frankenrust-sys: hand-write _cgo_export.h, compile and link upstream's C shim #7 abort-stubs incallbacks/output.rs, which is core: the output and input SAPI callbacks #12's file and which you must not touch.Confirmed by gdb backtrace on attempt 1 and independently re-confirmed by a
reviewer in two later rounds. See the replacement criteria below; they are
not weaker, they are the part that is observable from here.
drain_chanwas unowned, and this issue is why. See "The drain signal".Port
vendor/frankenphp/phpmainthread.go(316 lines),phpthread.go(296) andthreadinactive.go(67).Files you own:
crates/frankenrust-core/src/thread.rs,src/thread_inactive.rs,src/callbacks/mainthread.rs,src/callbacks/thread.rs. Pluscrates/frankenrust-core/src/lib.rs, for thetwo
pub moddeclarations (see #49 — an undeclared.rsis silently nevercompiled and still passes every gate step), additive edits to
crates/frankenrust-sys/build.rs, and — added by the resolver —crates/frankenrust-core/Cargo.tomland the workspaceCargo.lock. Do nottouch any other callback module — #11 and #12 are editing those in parallel.
build.rs's bindgen builder uses an explicitallowlist_functionlist, so itemits only the named items.
frankenphp_init_thread_metricsandfrankenphp_destroy_thread_metrics(frankenphp.h:216-217),frankenphp_force_kill_threadandfrankenphp_release_thread_for_kill(
:228-229), andfrankenphp_init_persistent_string(:209, forgo_init_os_env) are all invisible today. Add them additively, the same rule#11 states for the same file; do not hand-write an
extern "C"block infrankenrust-coreinstead. #11 also addsfrankenphp_init_persistent_string;that duplicate resolves trivially.
Dependencies: what to declare and what not to
frankenrust-corehas exactly one dependency today (frankenrust-sys) and noexternal crates at all. This issue needs two capabilities that are not there.
Both routes are decided; do not improvise a third.
malloc/freeforgo_get_custom_php_ini: get them from bindgen, notfrom a new crate.
crates/frankenrust-sys/wrapper.hincludes<php.h>,which includes
<stdlib.h>unconditionally (main/php.h:210), so bindgenalready parses both declarations and only the explicit allowlist is hiding
them. Add
.allowlist_function("malloc")and.allowlist_function("free")to the same additive block as the five
frankenphp_*symbols above, and callthem through
frankenrust_sys. This keeps the crate free of alibcdependency. If — and only if — bindgen does not emit them, add
libc = "0.2"tocrates/frankenrust-core/Cargo.tomland say so in yourfinal message. Do not hand-write
extern "C" { fn malloc(..) }inthread.rs, and do not fall back toCString::into_raw; that is the UBthe allocator section below names and that docs: PORTING-NOTES.md:120 tells agents to hand
CString::into_rawmemory to C'sfree()#18 already had to fix in the docsonce.
crossbeam-channelfor the drain signal must be declared. It is not inCargo.lockat all. Addcrossbeam-channel = "0.5"tocrates/frankenrust-core/Cargo.toml;Cargo.lockupdates as a side effect ofthe build and must be committed with it. The gate does not pass
--lockedandthe dev container has network plus a shared cargo registry volume, so the
fetch is fine.
docs/PORTING-NOTES.md:123-124already prescribes this cratefor exactly this construct. Note server: regular-thread handler, hyper HTTP/1.1 front end, and the async/pthread bridge #13 will also add a dependency to a
Cargo.toml; a lockfile conflict there resolves trivially.Callbacks implemented here (replacing #7's abort-stubs):
go_frankenphp_main_thread_is_ready,go_frankenphp_shutdown_main_thread,go_get_custom_php_ini,go_init_os_env,go_frankenphp_before_script_execution,go_frankenphp_after_script_execution,go_frankenphp_on_thread_shutdown,go_frankenphp_store_force_kill_slot,go_frankenphp_clear_force_kill_slot.Panic policy — the boundary guard is not yours
This is the section that decides whether this attempt converges. #102 measured
the previous three:
thread.rsreached 2169 lines against a 679-line oracle,and the reviewer who found the cause named it — "the panic-recovery machinery
this diff invents, which has no upstream counterpart." Round 4 then blocked on
a CAS to
ShuttingDownthat upstream does not have, introduced to support thatsame machinery.
The shared unwind guard at the
extern "C"boundary belongs to #78, incrates/frankenrust-core/src/callbacks/mod.rs, which you do not own. Yourin-lane obligation is narrower and entirely negative: do not panic. No
unwrap/expect/slice-indexing on anything a caller controls; a registrylookup for an out-of-range or unbooted
thread_indexreturns rather thanpanics; a poisoned lock is handled, not unwrapped. Do not invent
per-callback
catch_unwindwrappers, thread-health tracking, or new state-machineedges to support them. If a reviewer asks for panic recovery here, that is a
request for machinery with no oracle — point at this paragraph and at #78, fix
the panic source instead, and say so in your final message. The stderr rule
below still applies to any diagnostic you do write.
Initialisation order — none of this is optional
From
phpmainthread.go:44-85, which comments each constraint:frankenphp_new_main_thread().Extensions touch environment variables during module startup, and the main
thread's
thread_indexdefaults to 0, so callbacks fired during startup routeto slot 0 (
phpmainthread.go:53-58).frankenphp_new_main_thread(num_threads), then block until stateReady.The argument is the TSRM pre-allocation hint.
frankenphp_init_thread_metrics(max_threads)must run after the mainthread is Ready —
max_threadsis only final insidego_frankenphp_main_thread_is_ready— and before anyfrankenphp_new_php_thread, becausephp_threadwritesthread_metrics[thread_index]unguarded atfrankenphp.c:1541and wouldotherwise null-deref.
Donebefore the main thread isreleased, because
php_maincallstsrm_shutdown()as soon as it unparks.Then wait for
Reserved, thenfrankenphp_destroy_thread_metrics().The two blocking callbacks
go_frankenphp_main_thread_is_readymust block until shutdown(
phpmainthread.go:248-257: setReady, then wait forDoneorRebooting).It is called at
frankenphp.c:1710and the whole remaining lifetime of the Cmain pthread is spent inside it.
go_frankenphp_before_script_executionmust block until work arrives and returnthe script path as a NUL-terminated pointer that stays valid until at least
after_script_execution. C never frees it. Returning NULL terminates the C loopand the thread.
Note it deliberately reads the thread's handler without taking the handler
lock (
phpthread.go:236-237). Taking it deadlocks againstsetHandler, whichholds the write lock across the whole rendezvous while waiting for a state this
very callback must publish. Safety comes from the state handshake alone —
reproduce that, and write down why in a comment.
The drain signal —
drainChanis live, thedrain()method is notThe previous version of this issue said only "do not port the dead
drain()hook", and said nothing about
drainChan. That is half right and it cost fiveof the fourteen review findings.
drain()method onthreadHandler(phpthread.go:44-49) is dead:its only caller,
drainWorkerThreads, does not exist anywhere in the tree.Do not port it.
drainChan(phpthread.go:23) is load-bearing. It is closed in exactlythe two functions you are porting —
shutdown()(phpthread.go:124) andsetHandler()(:162) — each immediately after the state change the parkedhandler must observe, and it is recreated at
:70,:145and:166. It isselected on by
threadregular.go:111andthreadworker.go:242.Without it, a handler parked on a request channel never observes
ShuttingDown,force-kill cannot interrupt a Rust channel receive, and the deliberately
unbounded
Donewait below hangs forever. #13's body already specifies"generation-tagged drain ... closed at
phpthread.go:124and:162" assomething it will select on, but #13 owns neither
thread.rsnor the callbacks.It is yours. Go's
selectcomposes a channel receive with the requestchannel and std has no
select, so expose it as something a handler can composewith (
crossbeam_channelis the prescribed primitive; see "Dependencies"above). Closing a Go channel twice panics; upstream's close-then-replace is
exactly once per generation, so a generation tag is the Rust-side obligation.
The inactive handler parks on
ThreadStateitself, which the state changealready wakes, so it does not need to select on anything.
Its only two receivers (
threadregular.go:111,threadworker.go:242) are outof scope, so nothing else in the gate can catch a wrong generation tag — it gets
its own acceptance criterion below.
The handler contract C guarantees
Reviewers found violations of this four separate times. State it in code and
uphold it:
go_frankenphp_after_script_executiononly for a non-NULL script(
frankenphp.c:1506-1562). So a callback must never let a handler return ascript path and then discard it: by then a real handler has already taken a
request off its channel, and that request's completion signal is never sent.
Upstream has exactly one NULL path —
scriptName == ""(phpthread.go:241-243)— returned before the dequeue. Any health check, retirement decision or
path validation you add must happen on the same side of the handler call.
stable state only by publishing one (
threadregular.go:49-53setsReady,threadinactive.go:28-29setsInactive), andShuttingDownis publishedonly by
shutdown()(phpthread.go:118). If you add a self-retire edge, theslot must end reachable — a
Doneslot that no controller CASes back toReservedis a permanently lost registry slot, and a controller parked inRequestSafeStateChangenever wakes for it.eprintln!/println!anywhere inside a callback, or inside any panicrecovery arm. They panic on a write error, Rust sets
SIGPIPEtoSIG_IGN,and a panic escaping an
extern "C"frame aborts the process — which isexactly what the containment exists to prevent. Use
let _ = writeln!(io::stderr().lock(), ...). Same for the Rust-sidelifecycle functions, where an escaping panic skips
release_main_threadandparks
php_mainforever.Allocator rules
go_get_custom_php_ini(phpmainthread.go:288-315) returnskey=value\nconcatenated (map order, no escaping, no quoting —
:304-315), allocated withlibc
malloc(routed throughfrankenrust_sys— see "Dependencies"), and Cfree()s it atfrankenphp.c:1723.CString::into_rawuses Rust's globalallocator; handing that pointer to
free()is undefined behaviour. Check themallocresult for NULL. The same rule applies togo_read_cookiesin #12.Do not skip the
disableTimeoutsbranch at:294-300(max_execution_time=0,max_input_time=-1). The dev container definesZEND_MAX_EXECUTION_TIMERS, soC always passes
false(frankenphp.c:1681) and the gate can never catch itsomission — cover it with a unit test instead.
Force-kill slots
frankenphp_register_thread_for_kill(frankenphp.c:283-300) runs on the PHPthread itself, right after
ts_resource(0), and hands you aforce_kill_slotby value holding
&EG(vm_interrupt),&EG(timed_out)andpthread_self().frankenphp_force_kill_thread.frankenphp_release_thread_for_killon the previous slot beforeoverwriting — a thread can reboot and re-register (
phpthread.go:263-268) —and again on clear.
go_frankenphp_clear_force_kill_slotis called atfrankenphp.c:1598immediately before
ts_free_thread()at:1602, because that call frees theTSRM storage those pointers point into. The write lock taken in
clearmustexclude any concurrent kill, so a killer either completed before the lock or
sees a zeroed slot.
pthread_t; it is notSend/Sync. Wrapit explicitly with a
// SAFETY:comment naming the invariant.Note the lock direction is inverted from intuition: the writers (store, clear)
run on the PHP thread, the reader (send-kill) runs from the async side.
Reproduce the invisible restart
On a
zend_catchbailout that leaves the thread unhealthy, C callsfrankenphp_new_php_thread(thread_index)from inside the dying thread(
frankenphp.c:1611-1616) and skipsgo_frankenphp_on_thread_shutdownentirely. The registry must tolerate a fresh thread appearing at an index with
no prior shutdown notification, in whatever state the old one left behind.
Do not bound the shutdown and reboot waits
phpmainthread.go:179-196is a 15-line comment explaining that bounding them isa use-after-free confirmed by CI (php/frankenphp#2573):
php_maincallstsrm_shutdown()as soon as the main state changes, and that requires every PHPthread to have exited. A thread parked in a blocking call may never yield to
force_kill. The pattern is: grace period, then arm force-kill, then waitunbounded. Keep it.
Scaling exclusion
drainPHPThreadsopens withscalingMu.Lock()and the comment "disallow anyscaling or restarting threads while draining" (
phpmainthread.go:88-90);getInactivePHPThread(:231-246) is package-private and every caller holdsthat same mutex. Port the exclusion, not just the functions: a
getInactivePHPThreadthat boots aReservedslot the drain has already passedruns
ts_resource(0)whilephp_mainis insidetsrm_shutdown()— the exactuse-after-free cited above. If the Rust API surface is
pub, the guard has tobe in the type, not in a convention.
Inactive handler
threadinactive.go: parks inInactive, never returns a script name, and itsafterScriptExecutionpanics ("inactive threads should not execute scripts").Its
beforeScriptExecutionis written as tail recursion (threadinactive.go:36,:41) — write it as a loop, for the same fixed-stack reason as #8. ItsafterScriptExecutionis unreachable by the handler contract above; do notport the panic literally — return, and note in a comment that C cannot call it
for a NULL script.
Acceptance
The one-shot boot/drain test, in a subprocess. Boot the main thread plus
N PHP threads in the inactive state and assert every thread reaches
Inactive, then drain them and assert every PHP thread reachesReservedand the main thread's blocking callback returns. PHP's ownphp_module_shutdown()then aborts in core: the output and input SAPI callbacks #12'sgo_write_headersabort-stub,before
go_frankenphp_shutdown_main_threadcan publish the main thread'sReserved— so run this in a subprocess and assert exactly that: thelifecycle markers above, then death in the named stub, mirroring the
existing pattern in
tests/abort_stub.rs. Write it so it fails loudly, notsilently passes, once the abort stops happening. core: the end-to-end main-thread lifecycle test — php_module_shutdown() calls go_write_headers, so it cannot run until #12 lands #97 owns the positive
tsrm_shutdownassertion and depends on core: the output and input SAPI callbacks #12.The three-cycle test, with no main thread and no C thread. Drive
per-slot
boot()->Inactive->shutdown()->Reservedthree times inone test, over the Rust state machine alone, without deadlocking. This
criterion is deliberately not
drainPHPThreads: that function drives themain thread
Ready->Done->Reservedonce and ends withphpThreads = nil(phpmainthread.go:104-107), so it is not a repeatablecycle in upstream either, and repeating it would need a main-thread stand-in
this issue does not want you to invent. What is repeatable is the per-slot
one:
shutdown()ends atReserved(phpthread.go:147) precisely sogetInactivePHPThreadcan CASReserved->BootRequestedand boot itagain (
:238-242). Use a Rust stand-in for the PHP pthread's side of thehandshake — publishing
TransitionInProgress, parking, exiting — in place offrankenphp_new_php_thread; the seam is whatever yourboot()calls tostart the C thread, and it must be substitutable in tests without a
#[cfg]fork of the production path. It must also cover a
set_handlertransition:TransitionRequested->InProgress->Completewith a second, test-onlyhandler, since server: regular-thread handler, hyper HTTP/1.1 front end, and the async/pthread bridge #13/core: worker mode -- the frankenphp_handle_request park/resume loop #14 are not here to provide one.
A drain-signal unit test. Nothing else in the gate can catch a wrong
generation tag, because both receivers are out of scope. Assert: a receiver
taken before
shutdown()observes the close (a disconnected/ready receive);a receiver taken after a completed
set_handlerdoes not see the previousgeneration's close; and closing twice within one generation is impossible by
construction (the API gives no way to express it — say in your final message
which construction you used).
go_get_custom_php_ini: a test that it renders a two-key ini map askey=value\npairs and that the returned pointer is freeable byfree(),and one covering the
disableTimeoutsbranch.bash scripts/gate.sh defaultpasses.Note that
cargo testruns a binary's tests in parallel threads: anything thatboots a main thread must be one
#[test], or serialized behind a mutex. Notealso that an empty
go_init_os_envsatisfies every criterion above — say soexplicitly in your final message if you leave it thin.
Hazards
extern "C"callback aborts the process. Theanswer is not to panic; the guard is core: no unwind guard on any go_* callback, so one panic aborts the whole server #78's — see "Panic policy" above.
zend_first_try/zend_catchis setjmp/longjmp, not unwinding. Azend_bailout()— OOM,exit(), fatal error, timeout — longjmps straight pastevery intervening frame to
frankenphp.c:1565, running no destructors. Neverhold a
MutexGuard,BoxorVecin a Rust frame across a call into PHP thatcan bail; the mutex stays locked forever.
ts_resource(0). Touching PHP state from any other thread has no TSRM contextand will null-deref.
frankenphp_sapi_module.log_messageisfrankenphp_log_message(
frankenphp.c:1385-1387) ->go_log, which is core: the output and input SAPI callbacks #12's abort-stub, and PHP'slog_errorsdefaults to on witherror_logunset — so any warning duringphp_module_startupaborts your test process.display_startup_errorsroutesto
go_ub_writethe same way. Not yours to fix; the in-lane lever is thetest's own ini map, which
go_get_custom_php_inirenders.unsafeblock needs a// SAFETY:comment naming the invariant andwhere it is established. The recurring one here is "called only from
php_thread()on the thread that ownsthread_index."vendor/frankenphp/.Out of scope
The regular handler (#13) and the worker handler (#14). Request plumbing (#11,
#12).
max_threads=autoand everything it reads (#103) — leave the seam.The
tsrm_shutdown-survival and three-cycle C-level lifecycle test (#97).The
extern "C"unwind guard (#78). Autoscaling, metrics beyond theinit/destroy calls,
max_requests, opcache reset and reboot-all-threads(
rebootAllThreads), thedebugstate.gosurface, and the deaddrain()method on the handler interface — but not
drainChan, which is yours; seeabove.
phpThread.reboot()/forceReboot()(phpthread.go:82-114) are also out ofscope: their only callers are
max_requestsandrebootAllThreads, bothexcluded. Say which way you went in your final message.
Go's
os.Environsemantics ingo_init_os_env— duplicate keys and entrieswith no
=— are a known divergence fromstd::env::vars_os, already filed as#98. Do not expand this diff to fix it.
Gate: default
Agent: duel
Depends on: #7, #8
Recoveries: 1
Revisions: 1