Skip to content

Fix: store current route in per-request container#239

Closed
loks0n wants to merge 5 commits into
0.34.xfrom
fix/route-coroutine-safety
Closed

Fix: store current route in per-request container#239
loks0n wants to merge 5 commits into
0.34.xfrom
fix/route-coroutine-safety

Conversation

@loks0n

@loks0n loks0n commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Http::$route was a plain instance property on a shared Http instance. Under Swoole coroutines, two concurrent requests could clobber each other's matched route.
  • Moves route storage into the per-request DI container (already used via setRequestResource('route', ...)) and removes the redundant $this->route property.
  • getRoute(), setRoute(), and match() now read/write the per-request container; http.route telemetry and the wildcard fallback follow the same path.

Situations this fixes

  • http.route telemetry getting labeled with another in-flight request's path.
  • Hooks calling $http->getRoute() (audit logs, permission checks, span naming) seeing another coroutine's route.
  • Wildcard fallback assigning self::$wildcardRoute to the shared $this->route and masking a real match on a concurrent request.
  • User code calling $http->setRoute(...) from a hook being overwritten by another coroutine.

Test plan

  • vendor/bin/phpunit (pre-existing env failures in this checkout are unrelated — stale vendor utopia-php/di signature)
  • Load-test Swoole coroutine server with mixed routes + wildcard; confirm http.route telemetry matches the actual route per request

🤖 Generated with Claude Code

The Http instance is shared across coroutines, but $this->route was a
plain instance property mutated on every request. Concurrent coroutines
could clobber each other's route, corrupting http.route telemetry,
getRoute()/setRoute() reads from hooks, and the wildcard fallback path.

Move the route into the per-request DI container (already used via
setRequestResource('route', ...)) and drop the redundant $this->route
property so each coroutine sees only its own match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Apr 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a Swoole coroutine race condition where two concurrent requests could clobber each other's matched route via the shared $this->route instance property. The fix moves route storage into the per-request DI container (the same mechanism already used for request, response, and error), clones the wildcard route per-request to prevent in-place mutation races, removes the now-redundant double-write in runInternal(), and bumps utopia-php/di to 0.3.2 which supports overwriting existing container entries.

Confidence Score: 4/5

Safe to merge; the concurrency fix is correct and the wildcard clone properly addresses the previous P1 concern. One minor asymmetry in match() remains.

All prior P1 findings (wildcard mutation, redundant double-write) are resolved in this revision. The one remaining finding is P2: match() with fresh=true does not update the container when Router::match() returns null, leaving a stale route if match() is called more than once per request. This does not affect the normal runInternal() flow and is unlikely to impact production, but a one-line fix would fully align the new implementation's semantics with the old one.

src/Http/Http.php — specifically the match() null-route path (lines 694–700)

Important Files Changed

Filename Overview
src/Http/Http.php Moves route storage from a shared instance property to the per-request DI container; clones wildcard route to prevent cross-coroutine mutation; removes the now-redundant double-write. One minor asymmetry: match() with fresh=true doesn't clear the container when Router::match() returns null.
composer.json Loosens utopia-php/di constraint from 0.3.* to ^0.3.2 to pick up the overwrite-capable set() behaviour needed by this PR.
composer.lock Lock file updated to pin utopia-php/di at 0.3.2 (07025d7); no other dependency changes.

Reviews (3): Last reviewed commit: "Bump utopia-php/di to ^0.3.2 for set() c..." | Re-trigger Greptile

loks0n and others added 3 commits April 23, 2026 17:53
DI 0.3.1's Container::set() overwrites the factory but does not
invalidate the resolved-instance cache, so a second match(fresh: true)
against the same request-scoped container leaves getRoute() returning
the first match. In real request flow this never happens (each request
gets a fresh container and route is set once), so just drop the
assertion that exercised the old property-based mid-request override.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
self::\$wildcardRoute is a static singleton. Calling \$route->path(\$path)
mutates its in-place path state, which is shared across all coroutines
even though the per-request container now isolates the route pointer.
Two concurrent wildcard requests would overwrite each other's path
before execute() reads getMatchedPath() for path-parameter extraction.

Clone the wildcard route per request so each coroutine mutates its own
copy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
match() already writes the matched route to the per-request container,
and runInternal() redundantly wrote it again right after calling match.
For the no-match case the duplicate write stored a null factory that
getRoute() already handles by returning null when the key is missing.

Drop the second write so match() is the single owner of the route
resource; the wildcard branch still sets it directly since it produces
its own cloned route.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread tests/HttpTest.php
DI 0.3.1's Container::set() overwrote the factory but did not invalidate
the resolved-instance cache, so a second setRequestResource('route', ...)
within a request (or after any hook resolved 'route' via injection)
silently returned the stale value. 0.3.2 clears the concrete cache on
set(), which fixes the failing fresh-rematch assertion and makes the
coroutine-safety fix robust against mid-request route updates.

Restore the testCanMatchFreshRoute assertion that exercises this path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@loks0n loks0n closed this Apr 24, 2026
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.

1 participant