fix: use stackless singleton error for test permission - #1250
Conversation
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. Small, well-contained performance refactor that replaces a per-call new TestPermissionRollbackError() (stack allocation + prototype walk) with a frozen, stackless module-level singleton compared by reference (===) instead of instanceof.
What was reviewed: traced the sentinel error through withTransaction/runQuery's catch paths — mapPgError only transforms DatabaseError instances, so the singleton's identity survives unchanged through nested/savepoint rollback paths. Checked for other instanceof TestPermissionRollbackError" or mutation of the frozen instance elsewhere in the codebase — none found. New tests cover single, throwing, concurrent, and nested-savepoint testPermission` flows, including a concurrency test confirming the shared singleton is safe when multiple in-flight calls throw the same object.
Extended reasoning...
Overview
This PR touches only src/storage/database/pg.ts and its test file. It replaces the per-call allocation of TestPermissionRollbackError in StoragePgDB.testPermission with a single frozen, stack-less instance created once at module load, and switches the catch-side check from instanceof to reference equality (===). This is a hot-path micro-optimization for the RLS permission-check flow, which throws-and-immediately-catches a sentinel error to force a transaction rollback while keeping the callback's result.
Security risks
None identified. The change does not affect authorization logic, SQL construction, or transaction semantics — it only changes how the internal control-flow sentinel is represented and detected. The actual RLS/permission check behavior (running the callback inside a transaction, then always rolling back) is unchanged.
Level of scrutiny
This warrants light-to-moderate scrutiny: it's a pure internal refactor of an error-signaling mechanism confined to one class, with no behavior change from the caller's perspective (uploader.ts, storage.ts, object.ts all just call testPermission and get back the callback result or a rethrown error). I verified the singleton's identity is preserved as it passes through mapPgError/mapPgErrorWithQueryName (which only rewrite DatabaseError instances) in both the plain-transaction and nested-savepoint rollback paths, and confirmed there are no other instanceof checks against TestPermissionRollbackError elsewhere in the codebase that the switch to reference equality could break.
Other factors
The PR adds solid test coverage for this exact change: a basic success case, an error-rethrow case, a concurrency case explicitly designed to catch bugs from sharing one frozen error object across in-flight `testPermission" calls, and two nested-savepoint cases. This gives good confidence the shared-singleton approach is safe under concurrent use, which was the main risk I'd otherwise worry about.
Coverage Report for CI Build 29817741293Coverage increased (+0.03%) to 79.579%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
What kind of change does this PR introduce?
refactor for performance
What is the current behavior?
On hot path (RLS check), an error is thrown and caught immediately but it does stack allocation and walk the prototype chain.
What is the new behavior?
It doesn't carry information but a signal so a module singleton without stack and identity check is enough.
Additional context
It's 4-5x faster but latency win (0.1%) is small compared to other work so don't expect much there. However, it's hot path allocation, and it becomes zero now.