Skip to content

Commit 02fd983

Browse files
committed
fix(byok): keep an abandoned entitlement producer from writing the cache
coalesceLocally does not cancel a producer it timed out — its docstring says so explicitly — so writing the cache from inside the producer let a late billing result overwrite a fresher answer a retry had already cached, and hold it for a full TTL. Move the write onto the value the caller actually received. A caller that timed out throws before reaching it, so an abandoned producer now resolves into nothing. The test reproduces the overwrite and fails against the previous shape. Reported by Cursor Bugbot.
1 parent f97f89a commit 02fd983

2 files changed

Lines changed: 49 additions & 17 deletions

File tree

apps/sim/lib/api-key/byok-entitlement.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,33 @@ describe('organization BYOK entitlement', () => {
128128
expect(mockResolveOrganizationPlan).toHaveBeenCalledTimes(2)
129129
})
130130

131+
/**
132+
* `coalesceLocally` does not cancel a producer it timed out, so a write from
133+
* inside the producer could land after a retry cached a fresher answer and
134+
* overwrite it for a full TTL. Keeping the write on the value the caller
135+
* received means an abandoned producer resolves into nothing.
136+
*/
137+
it('ignores a producer that settles after its caller gave up', async () => {
138+
let releaseAbandoned: (value: boolean) => void = () => {}
139+
mockResolveOrganizationPlan.mockReturnValueOnce(
140+
new Promise<boolean>((resolve) => {
141+
releaseAbandoned = resolve
142+
})
143+
)
144+
145+
const abandoned = isOrganizationBYOKEntitledCached(ORGANIZATION_ID)
146+
abandoned.catch(() => {})
147+
__resetCoalesceLocallyForTests()
148+
149+
mockResolveOrganizationPlan.mockResolvedValue(false)
150+
await expect(isOrganizationBYOKEntitledCached(ORGANIZATION_ID)).resolves.toBe(false)
151+
152+
releaseAbandoned(true)
153+
await Promise.resolve()
154+
155+
await expect(isOrganizationBYOKEntitledCached(ORGANIZATION_ID)).resolves.toBe(false)
156+
})
157+
131158
it('never consults billing off hosted, on either path', async () => {
132159
mockIsHosted.value = false
133160

apps/sim/lib/api-key/byok-entitlement.ts

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -54,29 +54,34 @@ export async function isOrganizationBYOKEntitled(organizationId: string): Promis
5454
* plan change to appear, which is true of a workflow run and false of the
5555
* settings page.
5656
*/
57-
export function isOrganizationBYOKEntitledCached(organizationId: string): Promise<boolean> {
58-
if (!isHosted) return Promise.resolve(false)
57+
export async function isOrganizationBYOKEntitledCached(organizationId: string): Promise<boolean> {
58+
if (!isHosted) return false
5959

6060
const cached = entitlementCache.get(organizationId)
61-
if (cached !== undefined) return Promise.resolve(cached)
61+
if (cached !== undefined) return cached
6262

6363
/**
64-
* `coalesceLocally` around a read-through cache is the shape
65-
* `oauth/credential-service.ts` uses. It collapses a parallel or loop block's
66-
* N simultaneous misses onto one resolution, and — unlike caching the promise
67-
* directly — bounds a *hung* billing read at its settle deadline instead of
68-
* wedging every caller for the whole TTL.
64+
* `coalesceLocally` collapses a parallel or loop block's N simultaneous
65+
* misses onto one resolution, and bounds a *hung* billing read at its settle
66+
* deadline rather than wedging every caller for the whole TTL.
6967
*
70-
* Writing the cache only on the success path is what keeps a momentary
71-
* outage from being recorded as a plan lapse; `onError: 'throw'` is what
72-
* makes that outage distinguishable, since the resolver otherwise maps a
73-
* failed read to `false` exactly like a real lapse.
68+
* The cache write stays out here, on the value this caller actually received,
69+
* rather than inside the producer. `coalesceLocally` does not cancel a
70+
* producer it timed out — it keeps running detached — so a write from inside
71+
* could land after a retry already cached a fresher answer and overwrite it
72+
* for a full TTL. A caller that timed out throws instead of reaching this
73+
* line, and the abandoned producer resolves into nothing.
74+
*
75+
* Only reaching the write on success is also what keeps a momentary outage
76+
* from being recorded as a plan lapse; `onError: 'throw'` is what makes that
77+
* outage distinguishable, since the resolver otherwise maps a failed read to
78+
* `false` exactly like a real lapse.
7479
*/
75-
return coalesceLocally(`byok-entitlement:${organizationId}`, async () => {
76-
const entitled = await resolveOrganizationPlan(organizationId, { onError: 'throw' })
77-
entitlementCache.set(organizationId, entitled)
78-
return entitled
79-
})
80+
const entitled = await coalesceLocally(`byok-entitlement:${organizationId}`, () =>
81+
resolveOrganizationPlan(organizationId, { onError: 'throw' })
82+
)
83+
entitlementCache.set(organizationId, entitled)
84+
return entitled
8085
}
8186

8287
/**

0 commit comments

Comments
 (0)