GeezerVison

@claude review

Claude finished @carlos-granados's task in 6m 46s —— View job

Claude Code Review

• Read the changed files
• Analyze the fix against the described root causes
• Check for correctness / edge cases
• Post review

Reviewed the PR against origin/main. The three coupled root causes are correctly identified, and the fix is coherent and well-commented. Nothing here blocks merging — a few notes and small suggestions below.

Correctness — looks good

Mid-call activation guard (src/base/base.c:445–523, 655–667). The new xdebug_frame_belongs_to_call correctly uses fse->execute_data == execute_data->prev_execute_data to detect whether the tail frame really belongs to this call. Both begin handlers stamp fse->execute_data = execute_data->prev_execute_data right after xdebug_add_stack_frame (base.c:502 and base.c:641), so the invariant holds. I walked through the interesting cases (top-level begin no-ops, then observer flips on before nested calls; nested begin no-ops with a legitimate outer frame on the stack) and the check discriminates them correctly. Empty-stack safety is handled by the XDEBUG_VECTOR_COUNT(...) == 0 || !fse early return.

Observer install decision (src/base/base.c:723–738). Returning real handlers on FrankenPHP regardless of observer_active is the right fix for cache poisoning across worker requests. The fast-path in xdebug_execute_begin/xdebug_execute_end (base.c:686, 706) keeps the no-debug overhead to a single load + branch per fcall.

FrankenPHP sapi_activate re-arming (src/debugger/frankenphp.c:121–126). Setting observer_active per request based on trigger detection matches the RINIT-only startup constraint you documented.

ZEND_COMPILE_EXTENDED_STMT at MINIT (src/debugger/frankenphp.c:182). Forcing this process-wide is the only way to make user code compiled by later trigger requests carry EXT_STMT opcodes — since xdebug_base_rinit_if_enabled (which normally sets the flag, base.c:1095) doesn't run per-request in the worker flow.

Suggestions

1. observer_can_activate_mid_call and on_demand_debugging_enabled (src/base/base.c:1017–1035). The comment argues that on-demand paths can't actually cause mid-call activation on their own ("both only happen when a client was already attached at request start — in which case the observer was active for the whole request anyway"). Yet the flag is set true for that case anyway. Either drop on_demand_debugging_enabled from the OR (simpler; the cost is one branch on every fcall return for those users), or expand the comment to say explicitly this is a defensive belt-and-suspenders — right now the code disagrees with its own justification.

2. xdebug_disable_opcache_optimizer — ZEND_INI_STAGE_STARTUP from a per-request path (src/lib/lib.c:141–150). ZEND_INI_STAGE_STARTUP is normally reserved for MINIT and bypasses ZEND_INI_SYSTEM modifiability restrictions. Using it from sapi_activate is unusual and load-bearing (needed because opcache.optimization_level is SYSTEM). Worth a one-line comment on zend_alter_ini_entry explaining why the STARTUP stage is required outside MINIT — a future reader will otherwise want to "correct" it to ZEND_INI_STAGE_RUNTIME and silently break the fix. Fix this →

3. Minor perf: cache the persistent zend_strings. xdebug_disable_opcache_optimizer allocates two persistent zend_strings on every request. In a FrankenPHP worker under load this is a lot of small allocations for a constant pair. Consider MINIT-time init + MSHUTDOWN release, or ZEND_STRL static literal wrappers. Not blocking.

4. No automated regression coverage. tests/frankenphp/dbgp_listener.py + the Dockerfile is a great manual repro, but the test suite (run-xdebug-tests.php) has no FrankenPHP path. Given how easy it is for a future refactor of the observer/statement plumbing to re-poison the cache silently, a follow-up CI job (or at least a documented "how to verify #63 manually" pointer in tests/frankenphp/README.md) would be worthwhile. Not blocking this PR.

5. Document the FrankenPHP overhead trade-off. Because EXT_STMT is now forced for the whole process, FrankenPHP requests without any debug trigger still pay per-statement dispatch through xdebug_statement_call (xdebug.c:695) — it bails cheaply on !xdebug_is_debug_connection_active(), but it's no longer "zero overhead" the way CLI/FPM is. The PR body acknowledges this; consider surfacing it in user docs too so FrankenPHP users know the trade-off applies as soon as the extension is loaded.

6. Dead-code nit (src/debugger/frankenphp.c:150–183). The header block comment about sapi_module.activate may still be NULL is helpful context, but the code doesn't actually protect against it — sapi_module.activate is dereferenced via original_sapi_activate inside xdebug_frankenphp_sapi_activate where the guard if (original_sapi_activate) already handles NULL. Minor: the comment might read as if there's something special being done here when the guard is elsewhere. Optional cleanup.

Nit

• tests/frankenphp/dbgp_listener.py has a shebang but is unlikely to be marked executable in git (chmod +x before committing, or drop the shebang).

Nice work — the commentary in base.c:445, base.c:1017 and frankenphp.c:108 in particular makes the invariants easy to follow for future maintainers.
• Branch: fix/frankenphp-observer-cache

@claude I decided not to do #3, the gains are minuscule. Please review again the rest

Claude finished @carlos-granados's task in 4m 19s —— View job

Claude Code Re-Review (round 2)

• Fetch latest changes and diff against previous review
• Verify item Bump actions/checkout from 4 to 6 #1 (observer_can_activate_mid_call comment/logic)
• Verify item Bump actions/download-artifact from 4 to 8 #2 (ZEND_INI_STAGE_STARTUP comment)
• Verify item Phase 2: RINIT fast-path & deferred hooks — 630% → 10% overhead #4 (regression coverage / manual test doc)
• Verify item Phase 3: Runtime Inspection API (JSON transport + stubs) #5 (FrankenPHP overhead trade-off docs)
• Verify item Phase 2.5: Deferred activation — connect before enabling EXT_STMT #6 (dead-code nit)
• Verify nit (shebang / executable bit)
• Post updated review

Reviewed commit 9c5d112 against origin/main. All the follow-ups landed cleanly. Nothing blocking — one minor cosmetic nit.

Follow-ups verified

#1 — observer_can_activate_mid_call (src/base/base.c, src/base/base_globals.h, xdebug.c)
You went further than I suggested and dropped the flag entirely, always running xdebug_frame_belongs_to_call inside the two end handlers. That's the cleaner call. The added cost is a couple of loads + a compare, and only on the return path of calls the engine has actually installed our handlers on (debug session active, or FrankenPHP) — never in the no-debug CLI/FPM path. The fast-path if (EXPECTED(!XG_BASE(observer_active))) at xdebug_execute_end (base.c:709) still short-circuits before reaching the frame check on FrankenPHP-without-trigger requests, so the "single load + branch per fcall" property holds. The rewritten comment at base.c:445 correctly enumerates the mid-call activation triggers and no longer contradicts itself.

#2 — opcache optimizer INI stage (src/lib/lib.c:141)
Nice — switching to ZEND_INI_STAGE_RUNTIME with the explanation of why the stage is inert here is more correct than the "comment on STARTUP" I suggested. The claim in the comment holds: zend_alter_ini_entry_ex() gates on modify_type & ini_entry->modifiable, and opcache.optimization_level's OnUpdateLong ignores stage. The one behavioural difference — RUNTIME saves orig_value for restoration at zend_ini_deactivate — is benign in FrankenPHP because RSHUTDOWN doesn't run per request in the worker loop (the value is re-set on every sapi_activate).

#4 — automated regression coverage (.github/workflows/frankenphp.yml, tests/frankenphp/run-worker-tests.sh, dbgp_listener.py)
Much better than a docs pointer. The script starts a fresh worker + fresh listener per scenario so state can't leak, the port-preflight prevents the classic "IDE on 9003 masks the failure" confusion, and the added STACK OK assertion (dbgp_listener.py:78) covers the observer path independently of the statement handler — good separation. stacks_ok will catch a regression where the breakpoint fires but the stack is empty because xdebug_frame_belongs_to_call starts returning false spuriously, so it doubles as a guard for the item-#1 simplification.

#5 — FrankenPHP overhead trade-off (README.md:139)
Clear and honest. The "Setting the mode off … runs at full speed" pointer is the right escape hatch to surface.

#6 — MINIT header comment (src/debugger/frankenphp.c:150)
Rewritten to explain why both hooks are wrapped and why only activate may be NULL. Reads correctly now.

Nit — executable bit
dbgp_listener.py is now 0755. Good.

One minor nit on this round

• src/base/base.c:1019–1020 has a double blank line where the removed observer_can_activate_mid_call initialisation used to live. Purely cosmetic — collapse to a single blank line if you touch the file again. Fix this →

LGTM to merge as-is.
• Branch: fix/frankenphp-observer-cache