Skip to content

Commit a0ca8cb

Browse files
committed
fix(self-host): address review findings on instance org and org delete
Drop the per-process instance-org id cache. It went stale once the organization was deleted through the Admin API, and clearing it from the delete handler would only heal the replica that served that request. The lookup runs on the signup path against a single-row table, so re-reading costs nothing and keeps every replica self-correcting. Scope the org-delete subscription conflict to entitled statuses. Matching any row regardless of status let a canceled subscription — which bills nobody — permanently block deletion.
1 parent 49981f5 commit a0ca8cb

4 files changed

Lines changed: 50 additions & 23 deletions

File tree

apps/sim/app/api/v1/admin/organizations/[id]/route.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,23 @@ describe('admin organization DELETE', () => {
9595
expect(mockDetachOrganizationWorkspaces).not.toHaveBeenCalled()
9696
})
9797

98+
it('is not blocked by a canceled subscription', async () => {
99+
queueOrganization()
100+
/**
101+
* The status filter runs in SQL, so a canceled row never comes back — an
102+
* empty result here is what an organization whose subscription already
103+
* ended looks like. Without that filter the row would block deletion
104+
* forever even though it bills nobody.
105+
*/
106+
queueTableRows(schemaMock.subscription, [])
107+
queueTableRows(schemaMock.member, [{ value: 1 }])
108+
109+
const response = await DELETE(deleteRequest('acme-inc'), routeContext)
110+
111+
expect(response.status).toBe(200)
112+
expect(mockDetachOrganizationWorkspaces).toHaveBeenCalledWith(ORG_ID)
113+
})
114+
98115
it('detaches workspaces before deleting the organization', async () => {
99116
queueOrganization()
100117
queueTableRows(schemaMock.subscription, [])

apps/sim/app/api/v1/admin/organizations/[id]/route.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,11 +242,20 @@ export const DELETE = withRouteHandler(
242242
* cascades. Deleting an organization out from under a live subscription
243243
* would strand it, and its Stripe billing, against an id that no longer
244244
* resolves. Refuse instead of guessing whether to cancel.
245+
*
246+
* Scoped to entitled statuses. A canceled or ended row bills nobody, so
247+
* treating it as a blocker would make an organization that once had a
248+
* subscription permanently undeletable.
245249
*/
246250
const [existingSubscription] = await db
247251
.select({ id: subscription.id, plan: subscription.plan })
248252
.from(subscription)
249-
.where(eq(subscription.referenceId, organizationId))
253+
.where(
254+
and(
255+
eq(subscription.referenceId, organizationId),
256+
inArray(subscription.status, ENTITLED_SUBSCRIPTION_STATUSES)
257+
)
258+
)
250259
.limit(1)
251260

252261
if (existingSubscription) {

apps/sim/lib/organizations/instance-org.test.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ import {
5656
getInstanceOrganizationConfig,
5757
isInstanceOrganizationMode,
5858
joinInstanceOrganization,
59-
resetInstanceOrganizationCache,
6059
} from '@/lib/organizations/instance-org'
6160

6261
const ORIGINAL_ENV = { ...process.env }
@@ -72,7 +71,6 @@ describe('instance organization', () => {
7271
beforeEach(() => {
7372
vi.clearAllMocks()
7473
queuedRows.length = 0
75-
resetInstanceOrganizationCache()
7674
setEnvFlags({ isBillingEnabled: false })
7775
mockSelect.mockImplementation(buildSelectChain)
7876
mockExecute.mockResolvedValue(undefined)
@@ -176,14 +174,25 @@ describe('instance organization', () => {
176174
expect(mockCreateOrganizationWithOwnerTx).not.toHaveBeenCalled()
177175
})
178176

179-
it('caches the id so repeat calls do not re-query', async () => {
177+
it('re-reads the organization on every call instead of caching the id', async () => {
178+
/**
179+
* A per-process cache goes stale when the organization is deleted through
180+
* the Admin API, and clearing it from the delete handler would only heal
181+
* the replica that served that request. Re-reading keeps every replica
182+
* self-correcting.
183+
*/
180184
queueRows([{ id: 'org_existing' }])
181185
await ensureInstanceOrganization('user-1')
182186
const callsAfterFirst = mockSelect.mock.calls.length
183187

184-
await ensureInstanceOrganization('user-2')
188+
queueRows([])
189+
queueRows([])
190+
queueRows([])
191+
mockCreateOrganizationWithOwnerTx.mockResolvedValue({ organizationId: 'org_recreated' })
192+
const recreated = await ensureInstanceOrganization('user-2')
185193

186-
expect(mockSelect.mock.calls.length).toBe(callsAfterFirst)
194+
expect(mockSelect.mock.calls.length).toBeGreaterThan(callsAfterFirst)
195+
expect(recreated).toBe('org_recreated')
187196
})
188197
})
189198

apps/sim/lib/organizations/instance-org.ts

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,6 @@ const logger = createLogger('InstanceOrganization')
3232
/** Bounds the wait for a concurrent provisioning attempt on another replica. */
3333
const INSTANCE_ORG_LOCK_TIMEOUT_MS = 10_000
3434

35-
/**
36-
* Once provisioned, the instance organization's id never changes, so it is
37-
* cached for the life of the process. `null` is deliberately not cached — a
38-
* miss means provisioning has not happened yet, and the next caller should
39-
* retry rather than be told "no org" forever.
40-
*/
41-
let cachedInstanceOrganizationId: string | null = null
42-
4335
/** Derives a slug the same way the admin organization API does. */
4436
function slugifyOrganizationName(name: string): string {
4537
return name
@@ -95,10 +87,17 @@ export function isInstanceOrganizationMode(): boolean {
9587
/**
9688
* Returns the instance organization's id without creating it, or `null` when
9789
* the mode is off or provisioning has not run yet.
90+
*
91+
* Deliberately uncached. Caching the id per process looks free — it never
92+
* changes while the organization exists — but it goes stale the moment the
93+
* organization is deleted (the Admin API allows this), and every later signup
94+
* then tries to join an id that no longer resolves. Clearing the cache from the
95+
* delete handler would only fix the replica that served the request, leaving
96+
* every other replica broken until restart. The read is one lookup on a table
97+
* that holds a single row in this mode, and it only runs on the signup path, so
98+
* there is nothing worth caching against that failure mode.
9899
*/
99100
export async function getInstanceOrganizationId(): Promise<string | null> {
100-
if (cachedInstanceOrganizationId) return cachedInstanceOrganizationId
101-
102101
const config = getInstanceOrganizationConfig()
103102
if (!config) return null
104103

@@ -108,7 +107,6 @@ export async function getInstanceOrganizationId(): Promise<string | null> {
108107
.where(eq(organization.slug, config.slug))
109108
.limit(1)
110109

111-
if (row) cachedInstanceOrganizationId = row.id
112110
return row?.id ?? null
113111
}
114112

@@ -221,7 +219,6 @@ export async function ensureInstanceOrganization(
221219
return created.organizationId
222220
})
223221

224-
if (organizationId) cachedInstanceOrganizationId = organizationId
225222
return organizationId
226223
} catch (error) {
227224
/**
@@ -288,8 +285,3 @@ export async function joinInstanceOrganization(userId: string): Promise<void> {
288285
})
289286
}
290287
}
291-
292-
/** Clears the cached id. Exported for tests. */
293-
export function resetInstanceOrganizationCache(): void {
294-
cachedInstanceOrganizationId = null
295-
}

0 commit comments

Comments
 (0)