Skip to content

Skip action if response already sent by init hook#234

Merged
loks0n merged 2 commits into0.34.xfrom
feat/dont-run-if-sent
Apr 12, 2026
Merged

Skip action if response already sent by init hook#234
loks0n merged 2 commits into0.34.xfrom
feat/dont-run-if-sent

Conversation

@loks0n
Copy link
Copy Markdown
Contributor

@loks0n loks0n commented Apr 12, 2026

Summary

  • Init hooks (e.g. a cache layer) can call $response->send() to deliver a cached response directly to the client
  • Without this guard, the framework still runs the route action immediately after — doing the full work pipeline before discovering the response was already sent and silently no-op'ing the second send()
  • This adds a single isSent() check before dispatching the action; shutdown hooks still run so metrics, billing, and teardown logic are unaffected

Test plan

  • Verify that a route whose init hook calls $response->send() no longer executes the action
  • Verify that shutdown hooks still execute after a sent-in-init response (metrics, billing, etc.)
  • Verify normal request flow (no init hook sends) is unchanged

🤖 Generated with Claude Code

If an init hook (e.g. cache) calls $response->send(), the action should
not run — the response has already been delivered to the client. Without
this guard the action executes in full, wasting worker time on work
whose output is immediately discarded.

Shutdown hooks still run so metrics, billing, and other teardown logic
are unaffected.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@loks0n loks0n changed the base branch from master to 0.34.x April 12, 2026 14:05
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 12, 2026

Greptile Summary

This PR adds a guard in execute() that skips the route action when an init hook has already called $response->send() (e.g., a cache layer), while keeping shutdown hooks running. A follow-up fix commit restores the Response parameter to execute(Route, Request, Response) — which had been dropped in 0.34.x compared to 0.33.x — so the $response->isSent() check is properly in scope. The DI-registered response and the parameter passed to execute() are the same object instance (set in runInternal()), so the isSent() flag propagates correctly between hook and guard.

Confidence Score: 5/5

Safe to merge — the critical undefined-variable bug from the first commit is fully resolved, and the guard logic is correct.

The only open concern (missing test for the isSent() guard) was already flagged in a prior review thread and is P2. No new P0/P1 issues remain: the $response parameter is properly in scope, the same object instance is shared between the DI container and the guard, and shutdown hooks are correctly unaffected.

No files require special attention.

Important Files Changed

Filename Overview
src/Http/Http.php Adds Response $response as a third parameter to execute() (restoring 0.33.x API) and guards the route action dispatch with if (!$response->isSent()); shutdown hooks are unaffected and correctly still run.
tests/HttpTest.php All execute() call sites updated to pass new Response() as the third argument; no test added for the new isSent() guard behaviour (init-hook-sends-then-action-skipped scenario is untested).

Reviews (2): Last reviewed commit: "Fix: pass Response to execute() so isSen..." | Re-trigger Greptile

Comment on lines +723 to +726
if (!$response->isSent()) {
$arguments = $this->getArguments($route, $pathValues, $request->getParams());
\call_user_func_array($route->getAction(), $arguments);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 No unit test for the new guard

The PR description lists three test-plan items (all unchecked). There is no new test case in tests/HttpTest.php or the e2e suite that verifies an init hook calling $response->send() actually skips the route action while still executing shutdown hooks. Without a test, a future refactor could silently regress this behavior.

The 0.34.x execute() method had dropped the Response parameter compared
to 0.33.x, meaning $response->isSent() in the action guard would be an
undefined variable. Restore the parameter and update all call sites in
the test suite.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@loks0n loks0n merged commit d6b360d into 0.34.x Apr 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants