fix(relay): isUserPro fail-closed on entitlement-endpoint error (layer 3)#3485
Merged
Conversation
…r 3) Companion to PR #3483 (server-side mutation gate, layer 2). The 2026-04-28 audit found 7 of 28 enabled alertRules belonged to free-tier users — the relay's PRO filter has been silently masking a write-side gap. With PR #3483 closing the write path, this PR tightens the delivery path so an entitlement-endpoint outage no longer exposes paywalled notifications to the legacy free-user rows that still exist (until the upcoming cleanup script disables them). Change: both error paths in `isUserPro()` flip from `return true` (fail- open) to `return false` (fail-closed). Logged with stable `[relay][entitlement-fail-closed]` prefix so an uptime/alerting rule can fire on volume. Tradeoff: a real entitlement-endpoint outage that exceeds the 15-min Upstash cache TTL drops notifications for legitimate PRO users until recovered. The cache covers all warm users, so the blast radius is limited to (a) cold-start users in the first 15 min of the next event arriving for them, and (b) any user whose cache entry expired during the outage. Acceptable bias: better to drop legit-user delivery than expose the feature to free users. No code or test changes outside this single function. Behavior unchanged for the cache-hit path and for the success path.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
Greptile SummaryThis PR flips
Confidence Score: 3/5Safe to merge directionally, but the missing negative-result cache creates a relay-stability issue under sustained outages that should be addressed before or alongside deployment. One P1 finding (no caching of fail-closed result causes repeated 5-second timeout stalls per event during outages) plus a P2 (drainBatchOnWake bypass) pull the score below the P1 ceiling of 4. scripts/notification-relay.cjs — specifically the error return paths in isUserPro() (lines 128–133, 138–142) and the drainBatchOnWake function (lines 254–270). Important Files Changed
Sequence DiagramsequenceDiagram
participant Relay as notification-relay
participant Cache as Upstash Cache
participant Ent as Entitlement Endpoint
Relay->>Cache: GET relay:entitlement:{userId}
alt Cache hit
Cache-->>Relay: tier value → return tier>=1
else Cache miss
Cache-->>Relay: null
Relay->>Ent: POST /relay/entitlement (5s timeout)
alt res.ok (success)
Ent-->>Relay: { tier }
Relay->>Cache: SET cacheKey tier EX 900
Relay-->>Relay: return tier>=1
else !res.ok — NEW: fail-CLOSED
Ent-->>Relay: HTTP 4xx/5xx
Note over Relay: console.error [relay][entitlement-fail-closed]
Note over Relay,Cache: No cache write — next event re-fetches
Relay-->>Relay: return false (drop delivery)
else network/timeout error — NEW: fail-CLOSED
Ent-->>Relay: error / timeout
Note over Relay: console.error [relay][entitlement-fail-closed]
Note over Relay,Cache: No cache write — next event re-fetches
Relay-->>Relay: return false (drop delivery)
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * Layer-3 PRO gate. Returns true iff the user has tier>=1 entitlement. | ||
| * | ||
| * Fail-CLOSED policy (changed from fail-open in PR #348X following the | ||
| * 2026-04-28 audit that found 7 of 28 enabled alertRules belonged to free- |
Contributor
There was a problem hiding this comment.
The comment references PR #348X which appears to be an unfilled placeholder. The actual PR number is #3485.
Suggested change
| * 2026-04-28 audit that found 7 of 28 enabled alertRules belonged to free- | |
| * Fail-CLOSED policy (changed from fail-open in PR #3485 following the |
Comment on lines
+128
to
+133
| if (!res.ok) { | ||
| // fail-CLOSED: drop delivery rather than risk exposing a paywalled | ||
| // feature to a free-tier user during an entitlement-endpoint outage. | ||
| // Logged with a stable prefix so an alert can fire on volume. | ||
| console.error(`[relay][entitlement-fail-closed] HTTP ${res.status} for ${userId}; dropping delivery`); | ||
| return false; |
Contributor
There was a problem hiding this comment.
When the entitlement endpoint returns a non-OK HTTP status (or throws, line 141), the function returns false without writing anything to the Upstash cache. Once the 15-min TTL has passed during a sustained outage, every subsequent event triggers a fresh 5-second AbortSignal.timeout wait for every unique user — parallelized via Promise.all (line 944), but repeated on each polling cycle. A high-frequency event stream during an outage turns into a continuous 5-second stall per poll iteration.
Caching a zero/false value with a short TTL (e.g., 60–120 s) on the error paths would let the relay process events at normal speed while still recovering quickly when the endpoint comes back up. The catch branch (line 138) would benefit from the same treatment.
5 tasks
…tile P1+P2) Two findings on PR #3485: P1 - Fail-closed result not cached: Without a cache write on the fail-closed branches, every event during a sustained entitlement-endpoint outage triggered a fresh 5-second AbortSignal.timeout per unique user — turning a high-frequency event stream into a continuous 5-sec stall per relay poll iteration. Fix: cache "0" (free) with 60s TTL on both fail-closed branches (HTTP non-OK, transport/timeout). The cache absorbs subsequent events during the same outage at normal speed; the success path naturally refreshes with the real tier on the next attempt after TTL. 60s is the sweet spot: long enough to absorb a poll burst, short enough to recover quickly when the endpoint comes back. Tradeoff documented in the function-level JSDoc. P2 - Placeholder PR number in JSDoc: Replaced "PR #348X" with the actual "PR #3485". Cache writes are best-effort (try/catch around upstashRest SET) — if Upstash itself is down, we still fail-closed but skip the cache, which just means the next event re-attempts the 5-sec fetch. Acceptable secondary fail-closed under double-outage.
koala73
added a commit
that referenced
this pull request
Apr 28, 2026
…ertRules rows (#3486) * chore(notifications): cleanup script for 7 grandfathered free-tier alertRules rows Companion to PR #3483 (server-side mutation gate, layer 2) and PR #3485 (relay fail-closed, layer 3). Disables notifications for the 7 free-tier users that have grandfathered alertRules rows from before the layer-2 gate existed. Mechanism: calls internal.alertRules.setAlertRulesForUser (the INTENTIONALLY-ungated operator path — see PR #3483's contract test) with enabled=false. Preserves sensitivity/channels/eventTypes; only the on/off toggle flips. Affected userIds (verified via 2026-04-28 dry-run): user_3BwtmadXVR2XLnGfIUndMvCNn3S variant=full daily/high planKey=free user_3BybzNDxo0OIm1Q0wseyVPyZZI6 variant=full daily/high planKey=free user_3Bzq0tV0AqS1NGkGfAwkGiloBUW variant=full daily/all planKey=free user_3C0GSJz7gpTcqWdujingRCxxmMs variant=full daily/high planKey=free user_3C2rpobAryOk10hR2BKopVwaupC variant=full daily/all planKey=free user_3C4Y8NV7cVZlysodXrfOQufFBRE variant=full daily/all planKey=free user_3C6K7V9lEzPIH81TdSgthOmf2ZQ variant=full daily/all planKey=free DO NOT MERGE / RUN UNTIL #3483 + #3485 ARE DEPLOYED. Otherwise the same users could re-enable through the still-open mutation surface tomorrow. The user explicitly stated: "close it first, before doing anything to free users." Tier breakdown at time of script authoring: tier 0 (free): 7 users (25%) tier 1 (pro): 18 users tier 2 (pro+): 3 users Dry-run by default. --apply required to mutate. Per-row failures logged + counted; exit 1 on any failure. Idempotent: re-running after apply finds 0 free-tier rows in the enabled set. Audit trail kept committed (matches PR #3463 / #3468 / #3481 pattern). * fix(disable-free-notif): preserve eventTypes+channels, fail-closed on lookup errors (Greptile P1+P1) Two P1 findings on PR #3486 round 1 — both real, both fixed. P1 #1 (line 142): apply mode wiped eventTypes + channels. setAlertRulesForUser PATCHES (does NOT preserve) eventTypes and channels when they're supplied in args. The previous version sent `eventTypes: [], channels: []` — which would have wiped any user's saved subscriptions/channels, even though the script claimed to "only flip enabled". A user later upgrading to PRO would have to reconfigure from scratch. Concrete impact in the current population: user_3Bzq0tV0AqS1NGkGfAwkGiloBUW has channels=[1] (one configured channel). Old version would have wiped it. Fix: pass through `row.eventTypes ?? []` and `row.channels ?? []` from the existing row (which we already have in scope from getByEnabled). P1 #2 (line 87): silent failure on entitlement lookup. Previous version did `tier: tierMatch ? parseInt(tierMatch[1]) : -1` and then `filter(r => r.tier === 0)` — so a failed `npx convex run`, timeout, or output-format change would silently mark every user as tier=-1, get filtered out as "not free", and exit 0 with "no free users to disable." A regression in the lookup path could mask actual free users behind a green-status script run. Fix: fail-closed on three signals: - result.status !== 0 → FATAL exit code 4 - tier regex no match → FATAL exit code 4 - planKey regex no match → FATAL exit code 4 Includes the last few lines of stdout/stderr in the error so the operator can see the actual failure without re-running. Latent bug also fixed: the previous dedupe-by-userId-during-iteration would have caused multi-variant free users to only have ONE variant disabled. New version: dedupe entitlement LOOKUPS by userId (per-user data, no need to look up twice), but iterate ALL rows for the cleanup target list. Multi-variant users get every variant disabled in one pass. Dry-run on prod still shows the expected 7 rows. New diagnostic surface prints `eventTypes=[N] channels=[N]` per row so the operator sees what's preserved.
fuleinist
pushed a commit
to fuleinist/worldmonitor
that referenced
this pull request
May 9, 2026
…r 3) (koala73#3485) * fix(relay): isUserPro fail-closed on entitlement-endpoint error (layer 3) Companion to PR koala73#3483 (server-side mutation gate, layer 2). The 2026-04-28 audit found 7 of 28 enabled alertRules belonged to free-tier users — the relay's PRO filter has been silently masking a write-side gap. With PR koala73#3483 closing the write path, this PR tightens the delivery path so an entitlement-endpoint outage no longer exposes paywalled notifications to the legacy free-user rows that still exist (until the upcoming cleanup script disables them). Change: both error paths in `isUserPro()` flip from `return true` (fail- open) to `return false` (fail-closed). Logged with stable `[relay][entitlement-fail-closed]` prefix so an uptime/alerting rule can fire on volume. Tradeoff: a real entitlement-endpoint outage that exceeds the 15-min Upstash cache TTL drops notifications for legitimate PRO users until recovered. The cache covers all warm users, so the blast radius is limited to (a) cold-start users in the first 15 min of the next event arriving for them, and (b) any user whose cache entry expired during the outage. Acceptable bias: better to drop legit-user delivery than expose the feature to free users. No code or test changes outside this single function. Behavior unchanged for the cache-hit path and for the success path. * fix(relay): cache fail-closed entitlement result with short TTL (Greptile P1+P2) Two findings on PR koala73#3485: P1 - Fail-closed result not cached: Without a cache write on the fail-closed branches, every event during a sustained entitlement-endpoint outage triggered a fresh 5-second AbortSignal.timeout per unique user — turning a high-frequency event stream into a continuous 5-sec stall per relay poll iteration. Fix: cache "0" (free) with 60s TTL on both fail-closed branches (HTTP non-OK, transport/timeout). The cache absorbs subsequent events during the same outage at normal speed; the success path naturally refreshes with the real tier on the next attempt after TTL. 60s is the sweet spot: long enough to absorb a poll burst, short enough to recover quickly when the endpoint comes back. Tradeoff documented in the function-level JSDoc. P2 - Placeholder PR number in JSDoc: Replaced "PR #348X" with the actual "PR koala73#3485". Cache writes are best-effort (try/catch around upstashRest SET) — if Upstash itself is down, we still fail-closed but skip the cache, which just means the next event re-attempts the 5-sec fetch. Acceptable secondary fail-closed under double-outage.
fuleinist
pushed a commit
to fuleinist/worldmonitor
that referenced
this pull request
May 9, 2026
…ertRules rows (koala73#3486) * chore(notifications): cleanup script for 7 grandfathered free-tier alertRules rows Companion to PR koala73#3483 (server-side mutation gate, layer 2) and PR koala73#3485 (relay fail-closed, layer 3). Disables notifications for the 7 free-tier users that have grandfathered alertRules rows from before the layer-2 gate existed. Mechanism: calls internal.alertRules.setAlertRulesForUser (the INTENTIONALLY-ungated operator path — see PR koala73#3483's contract test) with enabled=false. Preserves sensitivity/channels/eventTypes; only the on/off toggle flips. Affected userIds (verified via 2026-04-28 dry-run): user_3BwtmadXVR2XLnGfIUndMvCNn3S variant=full daily/high planKey=free user_3BybzNDxo0OIm1Q0wseyVPyZZI6 variant=full daily/high planKey=free user_3Bzq0tV0AqS1NGkGfAwkGiloBUW variant=full daily/all planKey=free user_3C0GSJz7gpTcqWdujingRCxxmMs variant=full daily/high planKey=free user_3C2rpobAryOk10hR2BKopVwaupC variant=full daily/all planKey=free user_3C4Y8NV7cVZlysodXrfOQufFBRE variant=full daily/all planKey=free user_3C6K7V9lEzPIH81TdSgthOmf2ZQ variant=full daily/all planKey=free DO NOT MERGE / RUN UNTIL koala73#3483 + koala73#3485 ARE DEPLOYED. Otherwise the same users could re-enable through the still-open mutation surface tomorrow. The user explicitly stated: "close it first, before doing anything to free users." Tier breakdown at time of script authoring: tier 0 (free): 7 users (25%) tier 1 (pro): 18 users tier 2 (pro+): 3 users Dry-run by default. --apply required to mutate. Per-row failures logged + counted; exit 1 on any failure. Idempotent: re-running after apply finds 0 free-tier rows in the enabled set. Audit trail kept committed (matches PR koala73#3463 / koala73#3468 / koala73#3481 pattern). * fix(disable-free-notif): preserve eventTypes+channels, fail-closed on lookup errors (Greptile P1+P1) Two P1 findings on PR koala73#3486 round 1 — both real, both fixed. P1 #1 (line 142): apply mode wiped eventTypes + channels. setAlertRulesForUser PATCHES (does NOT preserve) eventTypes and channels when they're supplied in args. The previous version sent `eventTypes: [], channels: []` — which would have wiped any user's saved subscriptions/channels, even though the script claimed to "only flip enabled". A user later upgrading to PRO would have to reconfigure from scratch. Concrete impact in the current population: user_3Bzq0tV0AqS1NGkGfAwkGiloBUW has channels=[1] (one configured channel). Old version would have wiped it. Fix: pass through `row.eventTypes ?? []` and `row.channels ?? []` from the existing row (which we already have in scope from getByEnabled). P1 #2 (line 87): silent failure on entitlement lookup. Previous version did `tier: tierMatch ? parseInt(tierMatch[1]) : -1` and then `filter(r => r.tier === 0)` — so a failed `npx convex run`, timeout, or output-format change would silently mark every user as tier=-1, get filtered out as "not free", and exit 0 with "no free users to disable." A regression in the lookup path could mask actual free users behind a green-status script run. Fix: fail-closed on three signals: - result.status !== 0 → FATAL exit code 4 - tier regex no match → FATAL exit code 4 - planKey regex no match → FATAL exit code 4 Includes the last few lines of stdout/stderr in the error so the operator can see the actual failure without re-running. Latent bug also fixed: the previous dedupe-by-userId-during-iteration would have caused multi-variant free users to only have ONE variant disabled. New version: dedupe entitlement LOOKUPS by userId (per-user data, no need to look up twice), but iterate ALL rows for the cleanup target list. Multi-variant users get every variant disabled in one pass. Dry-run on prod still shows the expected 7 rows. New diagnostic surface prints `eventTypes=[N] channels=[N]` per row so the operator sees what's preserved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Companion to #3483 (server-side mutation gate, layer 2). The 2026-04-28 audit found 7 of 28 enabled
alertRulesrows belonged to free-tier users — the relay's PRO filter has been silently masking a write-side gap.With #3483 closing the write path (layer 2), this PR tightens the delivery path (layer 3) so an entitlement-endpoint outage no longer exposes paywalled notifications to the legacy free-user rows that still exist (until the upcoming cleanup script disables them).
Change
scripts/notification-relay.cjs:isUserPro()— both error branches flip fromreturn true(fail-open) toreturn false(fail-closed):Logged with stable
[relay][entitlement-fail-closed]prefix so an uptime/alerting rule can fire on volume.Tradeoff
A real entitlement-endpoint outage that exceeds the 15-min Upstash cache TTL drops notifications for legitimate PRO users until recovered. The cache covers all warm users, so the blast radius is limited to:
Acceptable bias: better to drop legit-user delivery than expose the feature to free users.
Test plan
node -c scripts/notification-relay.cjs— syntax cleannpm run typecheck— cleanFollow-up monitoring
Once merged + deployed, set an alert on the Railway logs for the
[relay][entitlement-fail-closed]prefix. A spike indicates entitlement-endpoint health degradation; a sustained presence indicates ongoing outage and needs paging.Sequence (3-layer entitlement gate)