From b15457bccad6facf63fb43cb27dcdad28aaf2d7e Mon Sep 17 00:00:00 2001 From: Prem Palanisamy Date: Wed, 29 Apr 2026 05:50:37 +0100 Subject: [PATCH] style: trim verbose comments on lock factories and call sites --- app/controllers/general.php | 3 -- app/controllers/shared/api.php | 3 -- app/init/resources/request.php | 67 ++++------------------------------ 3 files changed, 8 insertions(+), 65 deletions(-) diff --git a/app/controllers/general.php b/app/controllers/general.php index aa4134614c..0fde400b89 100644 --- a/app/controllers/general.php +++ b/app/controllers/general.php @@ -136,9 +136,6 @@ function router(Http $utopia, Database $dbForPlatform, callable $getProjectDB, S if (!$project->isEmpty() && $project->getId() !== 'console') { $accessedAt = $project->getAttribute('accessedAt', 0); if (DateTime::formatTz(DateTime::addSeconds(new \DateTime(), -APP_PROJECT_ACCESS)) > $accessedAt) { - // Skip-on-contention: every concurrent router request would write - // the same throttled timestamp, so losing the race is correct — - // the winning pod's update covers ours. $distributedLock('lock:platform:projects:' . $project->getId(), function () use ($dbForPlatform, $project, $authorization) { $authorization->skip(fn () => $dbForPlatform->updateDocument('projects', $project->getId(), new Document([ 'accessedAt' => DateTime::now() diff --git a/app/controllers/shared/api.php b/app/controllers/shared/api.php index b07eec1a99..0ce1b6ca33 100644 --- a/app/controllers/shared/api.php +++ b/app/controllers/shared/api.php @@ -250,9 +250,6 @@ Http::init() } if (! $updates->isEmpty()) { - // Serialize concurrent per-request writes to this key across API nodes. - // Skip on contention is safe: the winner applies the same idempotent update; - // if this node observed a new SDK the other didn't, the next request catches up. $distributedLock('lock:platform:keys:' . $dbKey->getId(), function () use ($dbForPlatform, $dbKey, $updates, $apiKey, $project, $user, $team) { $dbForPlatform->getAuthorization()->skip(fn () => $dbForPlatform->updateDocument('keys', $dbKey->getId(), $updates)); diff --git a/app/init/resources/request.php b/app/init/resources/request.php index 6d5663cdae..71af68b4b2 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -76,25 +76,8 @@ return function (Container $container): void { return $register->get('logger'); }, ['register']); - /** - * Push lock-side errors to the configured logger (Sentry/Raygun/AppSignal/etc.) - * by mutating the per-request `Log` object — same pattern as - * Embeddings/Text/Create.php and the http.php request-end error handler. - * - * Rate limited per pod via a static bucket so a sustained backend outage - * doesn't flood the logger. At most one push per 60s per (action, target) - * combo. Across a fleet of N pods this caps at N events/min, well within - * Sentry's own dedup tolerance. - * - * Only "real" errors are reported here: - * - backend_error: Redis/Dragonfly unreachable - * - release_error: lock release failed (TTL expired or backend dropped) - * - * Skipped on purpose: contention 409s (normal user-facing concurrency), - * skip-on-contention events (idempotent fan-out by design), acquire retry - * conflicts (internal loop), destructor cleanups (have an expected - * baseline rate; aggregate via the `lock.attempts` counter instead). - */ + // Rate-limited to one push per 60s per (action, target) so a sustained + // backend outage doesn't flood Sentry across the pod fleet. $lockErrorReporter = function (Log $log, ?Logger $logger, string $action, string $key, string $target, Throwable $e): void { static $lastReportAt = []; @@ -122,8 +105,7 @@ return function (Container $container): void { : Log::ENVIRONMENT_STAGING); $log->addTag('lock.target', $target); - // Strip the trailing document ID from the key so log aggregators don't - // see unbounded cardinality; the {target} attribute already covers slicing. + // Strip trailing document ID to keep aggregator cardinality bounded. $log->addTag('lock.key_pattern', preg_replace('/:[^:]+$/', ':*', $key)); $log->addTag('code', $e->getCode()); @@ -134,33 +116,17 @@ return function (Container $container): void { try { $logger->addLog($log); } catch (Throwable) { - // best-effort; don't let logging failures break fail-open } }; - // Extract the collection segment ("keys" / "projects" / "users") from keys - // of the form "lock:platform:{target}:{id}" so metrics can slice by target. $lockTargetOf = function (string $key): string { $parts = explode(':', $key, 4); return $parts[2] ?? 'unknown'; }; /** - * Distributed-lock factory: skip-on-contention variant. - * - * For idempotent writes where losing the race is correct (e.g., per-request - * `accessedAt` updates from N pods all writing the same value). On contention, - * the callback is silently skipped — another pod is doing the same work. - * - * Behavior: - * - Non-blocking acquire (one attempt). On conflict, skip and return. - * - Fail-open: if Redis is unreachable, run the callback unlocked + warn. - * - Kill switch: `_APP_LOCKING_ENABLED=disabled` runs the callback unlocked. - * - * Returns void — the caller can't distinguish "acquired and ran" from "skipped". - * - * Metric: `lock.attempts{outcome,target}` where outcome ∈ {acquired, skipped, - * backend_error, release_error}. + * Skip-on-contention. For idempotent writes where losing the race is correct + * (e.g., the winning pod's update covers ours). Fail-open on backend error. */ $container->set('distributedLock', function (\Redis $redis, Telemetry $telemetry, Log $log, ?Logger $logger) use ($lockTargetOf, $lockErrorReporter) { $enabled = System::getEnv('_APP_LOCKING_ENABLED', 'enabled') !== 'disabled'; @@ -205,21 +171,8 @@ return function (Container $container): void { }, ['redis', 'telemetry', 'log', 'logger']); /** - * Distributed-lock factory: 409-on-contention variant. - * - * For explicit user-write endpoints where read-modify-write on shared mutable - * state must NOT silently drop a request. On contention, throws - * `Exception::GENERAL_RESOURCE_LOCKED` (HTTP 409) so the client retries. - * - * Behavior: - * - Blocking acquire with short timeout (default 3s). - * - On timeout, throws `GENERAL_RESOURCE_LOCKED`. - * - Fail-open: backend unreachable runs the callback unlocked + warning. - * - Kill switch: `_APP_LOCKING_ENABLED=disabled` runs the callback unlocked. - * - Returns the callback's return value so callers can use the result. - * - * Metric: `lock.attempts{outcome,target}` where outcome ∈ {acquired, contended, - * backend_error, release_error}. + * Block-then-409 on contention. For read-modify-write on shared mutable state + * where silently dropping a request is wrong. Fail-open on backend error. */ $container->set('distributedLockOrFail', function (\Redis $redis, Telemetry $telemetry, Log $log, ?Logger $logger) use ($lockTargetOf, $lockErrorReporter) { $enabled = System::getEnv('_APP_LOCKING_ENABLED', 'enabled') !== 'disabled'; @@ -245,11 +198,7 @@ return function (Container $container): void { if (! $acquired) { $attempts->add(1, ['outcome' => 'contended', 'target' => $target]); - // Don't pass a custom message — the catalog message in - // app/config/errors.php is reused so we don't leak the internal - // lock key (which embeds collection name and document id) into a - // user-facing 409 response. The telemetry attribute already - // carries the target collection for operator-side observability. + // No custom message — the lock key embeds collection + document id. throw new Exception(Exception::GENERAL_RESOURCE_LOCKED); }