Skip to content

Conversation

realFlowControl
Copy link
Member

Description

Move resetting the REQUEST_LOCALS.interrupt_count to 0 from rinit to rshutdown or better: to the interrupt manager when we remove an interrupt. This fixes a tiny race condition:

  • Our interrupt manager triggers an interrupt
    pub(super) fn trigger_interrupts(&self) {
    let vm_interrupts = self.vm_interrupts.lock().unwrap();
    vm_interrupts.iter().for_each(|obj| unsafe {
    (*obj.interrupt_count_ptr).fetch_add(1, Ordering::SeqCst);
    (*obj.engine_ptr).store(true, Ordering::SeqCst);
    });
    }
  • while the PHP engine finished processing the request and enters RSHUTDOWN
  • the raised interrupt, as well as our incremented counter leak into the next request
  • PHP does not handle a new request, but instead shuts down
  • MSHUTDOWN gets called and PHP cleans up internal classes' static properties which could trigger a userland constructor (bug in PHP 8.0, fixed in >8.1) and we collect a sample and crash in the first execute_ex because of the leaked interrupt

So far we mitigated sampling bias (just collecting at the first opportunity in the new request because of the leaked interrupt) by initialising the interrupt_count with 0 in RINIT:

locals.interrupt_count.store(0, Ordering::SeqCst);

Moving this to RSHUTDOWN makes sure it does not leak to whatever comes after RSHUTDOWN, also does not introduce any sampling bias and seems a cleaner approach.

I can't think of any situation right now that could result in REQUEST_LOCALS.interrupt_count being not 0 in RINIT.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@realFlowControl realFlowControl requested a review from a team as a code owner October 18, 2025 10:31
@github-actions github-actions bot added profiling Relates to the Continuous Profiler tracing labels Oct 18, 2025
Copy link
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

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

Approved. Interesting sequence of events!

@morrisonlevi morrisonlevi changed the title fix(prof) reset interrupt count when removing interrupt fix(prof): reset interrupt count when removing interrupt Oct 20, 2025
@realFlowControl realFlowControl merged commit 1fa51af into master Oct 21, 2025
1871 of 1874 checks passed
@realFlowControl realFlowControl deleted the florian/fix-interrupt-leak branch October 21, 2025 09:35
@github-actions github-actions bot added this to the 1.14.0 milestone Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crashtracker-identified profiling Relates to the Continuous Profiler tracing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants