Problem/Motivation

Persistent app servers (like FrankenPHP in worker mode) can get stale requests. DrupalKernel::terminate() pops the main request only after the kernel.terminate subscribers and the destructable services run. If either throws, the request stays on the stack, and the next request handled by the same container gets the stale request from getMainRequest().

Also, $prepared is never reset. A page cache hit skips preHandle(), but terminate() still runs the subscribers and pops a request it never pushed.

Steps to reproduce

  1. Register a kernel.terminate subscriber that throws.
  2. Handle and terminate request A, catching the exception.
  3. Handle request B with the same kernel.
  4. \Drupal::requestStack()->getMainRequest() returns A.

Proposed resolution

Wrap the termination callbacks and destructable service cleanup in try/finally. In finally, if $prepared is TRUE, reset it to FALSE and pop the request pushed by preHandle(). Allow the exception to propagate.

This follows the existing try/finally pattern in KernelPreHandle::handle() for popping subrequests.

Issue fork drupal-3626014

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ptmkenny created an issue. See original summary.

ptmkenny opened merge request !17249

ptmkenny’s picture

ptmkenny commented
Issue summary: View changes
ptmkenny’s picture

ptmkenny commented
Status: Active » Needs review
amitgoyal’s picture

amitgoyal commented
Status: Needs review » Reviewed & tested by the community

The MR correctly wraps terminate() logic in try/finally, resetting $prepared before popping the request stack. This also correctly handles the page-cache-hit case where $prepared is FALSE.

Test coverage added for both the failed-termination and page-cache-bypass scenarios. Pipeline is green (the only failing jobs are the PHP 8.6 unit test lane, which is a non-blocking allow_failure job for the next PHP major and unrelated to this change).

Marking RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

@amitgoyal #5 looks like undisclosed LLM usage (e.g. there is no need to point out the 8.6 job is a warning, that happens on every single MR that gets committed to core), can you confirm whether your personally reviewed the MR yourself? Please see the AI disclosure policy https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2613044: Requests are pushed onto the request stack twice, popped once
andypost’s picture

andypost
he/him
Primary language Russian
commented

I thought we fixed it already but looks not, IIRC it was a compromise to do it this way #2613044: Requests are pushed onto the request stack twice, popped once

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

I had a look, although it replicates the logic in KernelPreHandle::handle() i was worried about it changing the contract, but then i noticed no catch. Oops :)

There is a behaviour change where before terminate might trigger even listeners twice. I had a quick look in contrib but couldn't really find impact.

Think this is sound, keeping stack clean in all cases is a good idea(TM), i've run into issues with this in jsonapi_extras also at some point.

RTBC.