@pronskiy I managed to fix eval handling so all tests are passing with this new optimization 🎉

@claude review

Claude finished @pronskiy's task in 10m 3s —— View job

Review

Big +1 on the overall direction — the optimization for "connected but idle" is a real-world common case and 80% improvement is meaningful. The mechanism (cmdloop detects no debug intent, disables statement_handler_enabled and observer_active) is the right shape. Below are issues I'd flag, ordered by severity.

🔴 Bug: pause-while-connected can be silently dropped after the optimization fires []

src/base/ctrl_socket.c:228-233 — CTRL_FUNC(pause) only sets do_break = 1 when a connection is already active:

} else {
    action = xdebug_xml_node_init("action");
    xdebug_xml_add_text(action, xdstrdup("Breakpoint Signalled"));

    XG_DBG(context).do_break = 1;
}

do_break is only observed inside xdebug_debugger_statement_call (src/debugger/debugger.c:402), and that function is gated by XG_BASE(statement_handler_enabled) in xdebug_statement_call (xdebug.c:716). With this PR, the cmdloop can leave statement_handler_enabled = false even with an active connection, so a control-socket pause will set the flag but it'll never be observed.

This is a regression vs. the previous behavior, where xdebug_init_debugger always set statement_handler_enabled = remote_connection_enabled, so pause-while-connected always worked.

The fix mirrors what PHP_FUNCTION(xdebug_break) and the no-connection branch right above already do:

XG_DBG(context).do_break = 1;
XG_BASE(statement_handler_enabled) = true;

Fix this →

🟡 Concern: stack drifts after xdebug_break in the on_demand=false + connected case []

When on_demand_debugging_enabled=false, the cmdloop disables observer_active (src/debugger/handler_dbgp.c:2507-2509). If a user then hits xdebug_break() (which is allowed because the connection is active), the new code at src/debugger/debugger.c:1235-1238 rebuilds the stack and re-enables statement_handler_enabled, but does not re-enable observer_active.

Once the IDE issues step_into, the cmdloop's new check sees do_step set and leaves things alone. That keeps observer_active = false, so calls into new functions don't push frames via xdebug_execute_user_code_begin / xdebug_execute_internal_begin, and returns don't pop them. The IDE will still get a stop on the next EXT_STMT, but the stack frame the IDE sees can be wrong (the function-name/scope reflects whatever was on top before the step).

I think the right fix is to also set XG_BASE(observer_active) = true in PHP_FUNCTION(xdebug_break) once we know we're committing to a debug session, mirroring the statement_handler_enabled = true already there. Worth adding a .phpt that covers xdebug_break followed by step_into into a callee under on_demand=0 to lock this down — I'd be surprised if any existing test covers it given the optimization is brand new.

🟡 Style / nit: tabs vs spaces [x]

src/debugger/debugger.c:1249 uses 4 spaces; the rest of the function (and file) uses tabs. Same author also added line 1269 (xdebug_connect_to_client) with the same pattern in an earlier commit, so this is consistent with prior local style — but inconsistent with the file. Worth fixing both while you're in there.

🟢 Minor: xdebug_vector_empty duplicates xdebug_vector_destroy [x]

src/lib/vector.h:128-133's body is byte-identical to the loop inside xdebug_vector_destroy (lines 109-111). Refactor xdebug_vector_destroy to call xdebug_vector_empty(v) first, then xdfree(v->data); xdfree(v);.

🟢 Minor: add_stack_frame_recursively recurses on PHP stack depth [x]

src/base/base.c:411-429 recurses once per prev_execute_data, so a deep PHP stack means a deep C stack. Real-world PHP stacks are rarely deep enough for this to matter, but converting to a two-pass iterative walk (count → fill from top) or using an explicit stack would be safer. Low priority.

🟢 Note: add_stack_frame_recursively skips a few things xdebug_execute_user_code_begin does []

It doesn't replicate the __call hack (src/base/base.c:470-472), xdebug_debugger_register_eval(fse) (line 485), or the entry-breakpoint check (line 489). Most of those would be wrong to redo on rebuild — they're "happens once when frame is first pushed" things — but worth a comment in xdebug_rebuild_stack clarifying that this rebuild deliberately does not fire entry side-effects. Future readers will wonder.

The eval-registration concern is mitigated by your other change (forcing eval through xdebug_execute_ex so it always gets registered, regardless of observer state), which I think is the right call — the PR comment captures that intent well.

🟢 Looks good

• The condition list at src/debugger/handler_dbgp.c:2496-2505 covers the right state: continuation flags, send_notifications, start_upon_error, and all three breakpoint collections. breakpoint_list is just an admin index that mirrors the typed lists, so checking the typed lists is sufficient.
• Gating the statement_handler_enabled assignment in xdebug_init_debugger on on_demand_debugging_enabled (src/debugger/com.c:664-666) is correct — for non-on_demand mode, we want the cmdloop to make the call after seeing what the IDE actually configured.
• The benchmark workflow label rename is harmless.

Summary

• Reviewed PR diff against origin/main (effectively 652f436..HEAD)
• Walked through src/base/base.c, src/base/base.h
• Walked through src/debugger/debugger.c, src/debugger/com.c, src/debugger/handler_dbgp.c
• Walked through src/lib/vector.h
• Verified statement_handler_enabled plumbing in xdebug.c:xdebug_statement_call
• Posted review

One real bug (pause-while-connected), one likely bug under a specific config (xdebug_break step-into with on_demand=false), plus minor style/refactor notes.
• Branch: feature/optimize-connected-but-not-used

@claude PR updated with your suggestions, do a new review

Claude finished @carlos-granados's task in 5m 25s —— View job

Re-review after updates

Thanks for the iteration — both critical issues from the previous round are addressed cleanly. The pause-while-connected fix in src/base/ctrl_socket.c is more thorough than I'd suggested (handles both branches, re-enables observer, and rebuilds stack on demand), and the xdebug_break path now also re-enables observer_active plus rebuilds the stack with a pop of the xdebug_break frame itself. The xdebug_vector_empty/xdebug_vector_destroy refactor and the tabs/spaces fix are in. Nice.

One concern remaining though: there's a subtle asymmetry between xdebug_break and xdebug_connect_to_client worth thinking through.

🟡 xdebug_connect_to_client does not pop its own frame after rebuild

src/debugger/debugger.c:1270-1272 rebuilds the stack but, unlike xdebug_break at lines 1235-1238, does not pop the trailing frame:

// xdebug_break (correct):
if (!XG_BASE(observer_active)) {
    xdebug_rebuild_stack();
    xdebug_vector_pop(XG_BASE(stack));   // pops xdebug_break itself
}

// xdebug_connect_to_client (asymmetric):
if (!XG_BASE(observer_active)) {
    xdebug_rebuild_stack();
    // no pop — xdebug_connect_to_client frame stays on the stack
}

When xdebug_rebuild_stack walks EG(current_execute_data) recursively up via prev_execute_data and pushes from root → leaf, the topmost frame pushed is the currently-executing function — i.e. xdebug_connect_to_client itself.

The end-handler that would normally pop this frame won't fire: xdebug_observer_init (src/base/base.c:696-703) is invoked at fcall init time with observer_active = false, so it returns {NULL, NULL} and no xdebug_execute_internal_end is registered for this call. The function returns, the frame stays.

Next statement after xdebug_connect_to_client returns will fetch fse = XDEBUG_VECTOR_TAIL(...) in xdebug_debugger_statement_call (src/debugger/debugger.c:384) and get the stale xdebug_connect_to_client frame instead of the actual caller's frame.

In practice, what would bite:

• do_next / do_finish checks via next_condition_met(fse) / finish_condition_met(fse, 0) (lines 416, 428) compare against the wrong frame.
• fse->has_line_breakpoints (line 451) reads from the wrong frame, potentially skipping line-breakpoint matching.

Suggested fix — mirror xdebug_break:

if (!XG_BASE(observer_active)) {
    xdebug_rebuild_stack();
    xdebug_vector_pop(XG_BASE(stack));
}

Fix this →

🟡 Related: pre-rebuild caller frames also dangle

The rebuild also adds frames for callers that were already running when observer_active was false (e.g. outer_function in the new test). Their begin/end handlers were never registered either, so when they return, no _end handler pops them. Most of the time this is harmless (the request soon ends, or new calls get pushed on top), but if execution returns past these frames and continues with more statements, you get the same stale-tail issue.

The new test (rebuild-stack-after-xdebug-break.phpt) doesn't expose this because it detaches immediately after step_into. A test that lets execution return past outer_function and then steps in {main} would tighten coverage. Not a blocker — flagging because the optimization is brand-new and edge cases are easy to miss.

🟢 Minor: typo in the new comment

src/base/base.c:432 — "when we the debugger is activated" → "when the debugger is activated". One-word fix.

Fix this →

🟢 Confirmed addressed from the previous round

• CTRL_FUNC(pause) re-enables statement_handler_enabled (and observer_active) plus rebuilds stack — src/base/ctrl_socket.c:234-239
• xdebug_break re-enables observer_active, rebuilds stack, pops self — src/debugger/debugger.c:1235-1250
• Tabs/spaces fixed at src/debugger/debugger.c:1249 and :1274
• xdebug_vector_destroy now reuses xdebug_vector_empty — src/lib/vector.h:107-119
• Comment added at xdebug_rebuild_stack clarifying that entry side-effects are deliberately skipped — src/base/base.c:431-435
• New .phpt test for xdebug_break + step_into under on_demand_debugging_enabled=0

Summary

Two critical bugs fixed, one asymmetry remaining (xdebug_connect_to_client should pop its own frame from the rebuilt stack, mirroring xdebug_break), plus a typo in the new comment. Nothing else from the prior review is outstanding. Once the connect-to-client pop is in, this is in good shape to merge.

• Reviewed 4187bcc against prior review feedback
• Walked through src/base/ctrl_socket.c, src/debugger/debugger.c, src/lib/vector.h, src/base/base.c
• Cross-checked xdebug_observer_init end-handler registration logic to verify the dangling-frame analysis
• Read the new .phpt test
• Posted re-review
  • Branch: feature/optimize-connected-but-not-used

@claude updated, review again

Claude finished @carlos-granados's task in 1m 10s —— View job

Re-review after bac24fe

Both items I flagged in the previous round are addressed, and the test extension is exactly what I'd hoped to see. I think this is good to merge.

Confirmed fixes

• xdebug_connect_to_client now pops its own frame — src/debugger/debugger.c:1270-1273 mirrors xdebug_break at 1235-1238. The previously-dangling xdebug_connect_to_client frame won't drift into subsequent stack_get / next_condition_met / line-breakpoint checks anymore.
• Typo fix at src/base/base.c:432 — "when we the debugger is activated" → "when the debugger is activated".
• Test extended to cover the dangling-frame-on-return scenario — tests/debugger/rebuild-stack-after-xdebug-break.phpt now does stack_get at the break, then step_into into inner_function, then step_out back to outer_function, with stack_get after each. This exercises both the rebuild itself and the case where execution returns past a rebuilt frame, which was my main concern about pre-rebuild caller frames dangling. The expected output at line 58 confirms outer_function + {main} after step_out, meaning the end-handler properly pops inner_function (which was pushed by the live observer post-break) — a good signal that re-enabling observer_active together with statement_handler_enabled keeps the frame-bookkeeping consistent going forward.

Symmetry is now correct

// xdebug_break:
if (!XG_BASE(observer_active)) {
    xdebug_rebuild_stack();
    xdebug_vector_pop(XG_BASE(stack));
}
// ...
XG_BASE(statement_handler_enabled) = true;
XG_BASE(observer_active) = true;

// xdebug_connect_to_client:
if (!XG_BASE(observer_active)) {
    xdebug_rebuild_stack();
    xdebug_vector_pop(XG_BASE(stack));
}
// ...
XG_BASE(statement_handler_enabled) = true;
XG_BASE(observer_active) = true;

// CTRL_FUNC(pause) (already from prior round):
XG_DBG(context).do_break = 1;
XG_BASE(statement_handler_enabled) = true;
if (!XG_BASE(observer_active)) {
    xdebug_rebuild_stack();
    XG_BASE(observer_active) = true;
}

pause doesn't pop because it isn't itself a PHP-visible call — there's no frame on the rebuilt stack representing the control-socket message. That's correct. xdebug_break and xdebug_connect_to_client each pop because their own frame is the stack tail after rebuild.

Nothing else outstanding

I re-walked everything from the prior round: ctrl_socket.c, debugger.c, handler_dbgp.c, base.c, vector.h, com.c, and the new test. No new concerns. The 80% performance gain in the "connected but idle" case looks well-earned and the correctness path is now solid.

LGTM. 🎉

• Pulled bac24fe and diffed against 4187bcc
• Verified xdebug_connect_to_client symmetry with xdebug_break
• Verified typo fix
• Reviewed extended test for step_out-after-step_into coverage
• Posted review
  • Branch: feature/optimize-connected-but-not-used