Skip to content

Construct the error Log per request instead of sharing a singleton#6

Merged
lohanidamodar merged 1 commit into
mainfrom
fix/per-request-log
Jun 25, 2026
Merged

Construct the error Log per request instead of sharing a singleton#6
lohanidamodar merged 1 commit into
mainfrom
fix/per-request-log

Conversation

@lohanidamodar

Copy link
Copy Markdown
Member

What

Follow-up to #5 addressing a Greptile review comment.

The log resource was registered on the root utopia-php/di Container. Container::get() caches the factory result, so the per-request context (a child container) falls through to the root and every request receives the same Log instance. With enable_coroutine enabled, concurrent requests share that instance, so error context set in the error handler (setNamespace, setMessage, addTag, ...) can bleed between requests.

geodb and logger are correctly long-lived singletons (read-only DB reader / log provider). Only log is request-scoped.

Fix

The error action is the only consumer of log, so it now builds a fresh Log locally — the same pattern Appwrite's worker/cli/realtime error handlers use — and the shared log resource registration is removed.

Testing

  • composer lint (pint) and composer check (phpstan level 8) pass.
  • Built the image and exercised the error path in a running container:
    • GET /v1/health{"status":"ok"}
    • GET /v1/ips/8.8.8.8 without key → 401 (Init throws → error action)
    • GET /v1/nope404 (no route → error action)
    • GET /v1/ips/1.1.1.1 with key → correct payload
    • No warnings/errors in the container logs.

The `log` resource was registered on the root di Container, which caches the
factory result. With enable_coroutine on, that single Log instance was shared
across concurrent requests, so error context (namespace, message, tags) from
one request could bleed into another.

The error action is the only consumer of `log`, so build a fresh Log inside it
(matching how Appwrite's worker/cli error handlers construct their own Log) and
drop the shared resource registration.
@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a request-context bleed bug where a singleton Log instance registered in the root DI container was shared across concurrent coroutine requests, allowing error metadata (setNamespace, setMessage, addTag, etc.) set during one request's error handler to be visible to another.

  • Server.php: Removes the log resource registration entirely and its associated use import, since Log is request-scoped rather than a long-lived singleton.
  • Error.php: Drops the ->inject('log') call and the $log method parameter; instantiates new Log() directly inside the logError() call, matching the pattern used in Appwrite's worker, CLI, and realtime error handlers.

Confidence Score: 5/5

The change is safe to merge; it narrows scope from a shared singleton to a local allocation with no other behavioral changes.

Both files have minimal, targeted changes. The root cause (singleton Log shared across coroutine requests) is correctly addressed by instantiating new Log() locally in the error action, and the now-dead import and resource registration are cleanly removed. The use Utopia\Logger\Log import in Error.php is still present and correctly used for the inline new Log() construction. No other callers of the removed resource exist in the changed files.

No files require special attention.

Important Files Changed

Filename Overview
src/Geo/Modules/Core/Http/Error.php Drops the injected $log parameter; now constructs new Log() inline on each call to logError(), ensuring per-request isolation under coroutine concurrency.
src/Geo/Server/Server.php Removes the singleton log resource registration and the now-unused use Utopia\Logger\Log import; no other changes.

Reviews (1): Last reviewed commit: "Construct the error Log per request inst..." | Re-trigger Greptile

@lohanidamodar lohanidamodar merged commit 122f86e into main Jun 25, 2026
4 checks passed
@lohanidamodar lohanidamodar deleted the fix/per-request-log branch June 25, 2026 06:38
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