Clear, drain and bound the pending-exception discard - #364

What was wrong

Seven places in the extension discarded a pending exception like this:

if (EG(exception)) {
    OBJ_RELEASE(EG(exception));
    EG(exception) = NULL;
}

The release happens while EG(exception) still names the object being released. The engine forbids exactly that: zend_objects_destroy_object() opens with if (EG(exception) == object) zend_error_noreturn(E_CORE_ERROR, "Attempt to destruct pending exception"). Its own discard, zend_clear_exception(), does the opposite and says why in a comment: /* exception may have destructor */.

The price is not theoretical, and it is paid at the most reachable of the seven — the uncaught exception of a worker-mode handler. For an exception class declaring __destruct (one handing back a lock, removing a temp file, closing a handle on the way out):

• the destructor never ran, and never ran later either, because the store flags the object as destructed before asking for it;
• the request answered 500 but was filed as a worker that came apart rather than an application that threw. Measured against a throwing endpoint: 31 such requests retired 8 workers, against the documented rule that an uncaught exception is answered and stays neutral to recycling;
• the fatal marks every object alive on the worker as already destructed on its way past, so nothing on that worker — including anything the application holds across requests — ever ran a destructor again;
• the object leaked: the store takes its own reference before the refusal, so the catch arm's discard brought the refcount to one and free_obj was never called. One exception plus its stack trace per request that threw, for the life of the worker.

What changed

• Slot before object, through zend_clear_exception(), collected into one function rather than repeated at each site.
• The discard drains until the slot is empty. The destructor now really runs, which makes it user code that can throw, and zend_call_function() returns SUCCESS without calling anything while an exception is pending — a single throwing destructor would silently cancel every register_shutdown_function() of the request. In the reset a worker runs between requests it is worse: nothing clears the slot before the next request, which is then blamed for an exception it never raised.
• The drain has a ceiling. It does not end by itself: two classes whose destructors throw each other free one object and allocate one per turn, so no memory limit arrives, and the new throw does not even chain onto the old one because the slot is already empty when it is raised. The execution deadline does not stand in for it where it matters — the between-request reset runs after the ini rollback, which disarms the timer and deliberately does not arm it again, and a streaming script is told to call set_time_limit(0). Past the ceiling the last object is dropped with IS_OBJ_DESTRUCTOR_CALLED set and a line written to the log.
• The discards in zend_catch arms are guarded with a zend_try of their own and with fiber switching blocked. A destructor that fatals there would leave through the bailout target the arm has already handed back; one that suspends would park the fiber while CG(unclean_shutdown) and gc_protect are still raised and hand the thread to another request wearing those flags. These arms are not unreachable with a live destructor: the cancellation check on the output path raises a bare zend_bailout() with no error reported, so nothing is flagged on the way.
• CG(unclean_shutdown) is lowered on both sides of the discard in the fiber release guard — before, because a generator the destructor reaches skips its finally blocks and closes the short way while the flag is up; after, because the bailout this guard swallows raises it again and no recovery follows this one, so the rest of the scheduler's teardown walk would run in that state.
• The exception read behind a decorator or a trace keeps the same order, spelled out rather than through zend_clear_exception() (which would also discard the caller's saved exception and move its instruction pointer), and gains the zend_catch its trace call already had: the function has taken the caller's exception out of the slot by then, so a fatal would lose it with its only reference and leak both out-parameter buffers.

Evidence

Three pairs of worker-mode probes, each talking through a file so they hold at any pool size. Every pair asserts the mechanism, not the status — 500 comes back on a broken build too.

The third pair lifts its own deadline with set_time_limit(0) and asserts how long the request took rather than that it finished, because an unbounded drain finishes too — measured on the mutant, the spin ends at max_execution_time with a 504 when the deadline is in force, and only when the client stops waiting when it is not. The ceiling is observable rather than inferred: on this branch the same request answers 500 in 0.000s and logs one line saying the last exception was dropped without running its destructor.

After the fix, the same throwing endpoint leaves oxphp_worker_recycles_total 0 (no reason="error" series at all), no core errors in the log, and the destructor marker in place.

Verification

cargo fmt -- --check
cargo clippy --no-default-features -- -D warnings
cargo test --no-default-features          # 1170 passed, 0 failed
./tests/run_all.sh --profile=worker       # 64 passed, 0 failed

The extension sources are compiled by the Dockerfile rather than by cargo, so the worker-profile runs above are what covers this change; every image was built with DOCKER_BUILDKIT=0 docker build --no-cache and checked for freshness by a string the fix introduces.

Notes for the reviewer

• The ceiling is 16 turns. It is sized so that the cost of exceeding it — one destructor refused, one log line — stays cheap rather than from a survey of legitimate chain depths; a stricter rule ("a destructor threw twice in a row, stop asking") would behave the same in practice.
• Two discards stay outside a zend_try: the tail after zend_exception_error() and the first step of the between-request reset. The hole they leave got strictly smaller with this change — before it, any exception with a __destruct escaped there; now only one whose destructor itself fatals — and closing them is a separate piece of work.