Skip to content

Commit 8e8c5ca

Browse files
fix(cli-auth): retry a failed mint, port container/port fixes to Redis + k8s
Review findings from #5911: - poll now reserves the mint with an atomic NX lock instead of deleting the approval up front, so a failed mint (e.g. mothership blip) is retried by the next poll instead of forcing a fresh browser approval; the lock still prevents a double-mint and its TTL frees the slot if the caller dies - setup reuses/recreates an unhealthy managed sim-redis instead of colliding on the name (Redis has no data volume, so it removes and recreates without a prompt) - k8s failure-path hints carry --context, matching the success-path hints, so a changed ambient context can't send diagnostics to the wrong cluster - compose port-free waits for a killed port to actually release before re-checking; SIGKILL is async, so the immediate re-check re-saw the port
1 parent e00b02d commit 8e8c5ca

7 files changed

Lines changed: 128 additions & 40 deletions

File tree

apps/sim/app/api/cli/auth/poll/route.test.ts

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,24 @@
44
import { createMockRequest } from '@sim/testing'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
66

7-
const { mockPollApproval, mockGenerateCopilotApiKey, mockEnforceIpRateLimit } = vi.hoisted(() => ({
7+
const {
8+
mockPollApproval,
9+
mockCompleteApproval,
10+
mockReleaseMint,
11+
mockGenerateCopilotApiKey,
12+
mockEnforceIpRateLimit,
13+
} = vi.hoisted(() => ({
814
mockPollApproval: vi.fn(),
15+
mockCompleteApproval: vi.fn(),
16+
mockReleaseMint: vi.fn(),
917
mockGenerateCopilotApiKey: vi.fn(),
1018
mockEnforceIpRateLimit: vi.fn(),
1119
}))
1220

1321
vi.mock('@/lib/cli-auth/approval-store', () => ({
1422
pollApproval: mockPollApproval,
23+
completeApproval: mockCompleteApproval,
24+
releaseMint: mockReleaseMint,
1525
}))
1626

1727
vi.mock('@/lib/copilot/server/api-keys', () => ({
@@ -37,6 +47,8 @@ describe('POST /api/cli/auth/poll', () => {
3747
vi.clearAllMocks()
3848
mockEnforceIpRateLimit.mockResolvedValue(null)
3949
mockGenerateCopilotApiKey.mockResolvedValue({ id: 'key-1', apiKey: 'sk-test' })
50+
mockCompleteApproval.mockResolvedValue(undefined)
51+
mockReleaseMint.mockResolvedValue(undefined)
4052
})
4153

4254
it('returns pending without minting while unapproved', async () => {
@@ -47,7 +59,7 @@ describe('POST /api/cli/auth/poll', () => {
4759
expect(mockGenerateCopilotApiKey).not.toHaveBeenCalled()
4860
})
4961

50-
it('mints for the approving user once approved', async () => {
62+
it('mints, then consumes the approval, once approved', async () => {
5163
mockPollApproval.mockResolvedValue({ status: 'approved', userId: 'user-1' })
5264
const response = await POST(pollRequest({ request: REQUEST, verifier: VERIFIER }))
5365
expect(response.status).toBe(200)
@@ -56,6 +68,17 @@ describe('POST /api/cli/auth/poll', () => {
5668
key: { id: 'key-1', apiKey: 'sk-test' },
5769
})
5870
expect(mockGenerateCopilotApiKey).toHaveBeenCalledWith('user-1', expect.stringMatching(/^CLI /))
71+
expect(mockCompleteApproval).toHaveBeenCalledWith(REQUEST)
72+
expect(mockReleaseMint).not.toHaveBeenCalled()
73+
})
74+
75+
it('releases the reservation (keeps the approval) when minting fails', async () => {
76+
mockPollApproval.mockResolvedValue({ status: 'approved', userId: 'user-1' })
77+
mockGenerateCopilotApiKey.mockRejectedValue(new Error('mothership down'))
78+
const response = await POST(pollRequest({ request: REQUEST, verifier: VERIFIER }))
79+
expect(response.status).toBe(500)
80+
expect(mockReleaseMint).toHaveBeenCalledWith(REQUEST)
81+
expect(mockCompleteApproval).not.toHaveBeenCalled()
5982
})
6083

6184
it('rejects a malformed verifier before touching the store', async () => {

apps/sim/app/api/cli/auth/poll/route.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { createLogger } from '@sim/logger'
22
import { type NextRequest, NextResponse } from 'next/server'
33
import { pollCliAuthContract } from '@/lib/api/contracts'
44
import { parseRequest } from '@/lib/api/server'
5-
import { pollApproval } from '@/lib/cli-auth/approval-store'
5+
import { completeApproval, pollApproval, releaseMint } from '@/lib/cli-auth/approval-store'
66
import { CopilotApiKeyError, generateCopilotApiKey } from '@/lib/copilot/server/api-keys'
77
import { enforceIpRateLimit } from '@/lib/core/rate-limiter'
88
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -18,7 +18,8 @@ function cliKeyName(): string {
1818
* The CLI's poll endpoint. Unauthenticated by necessity — the CLI has no
1919
* session — but the request id is only a rendezvous handle and minting requires
2020
* the poll secret, which never leaves the CLI. Returns `pending` until the user
21-
* approves; on the first authorized poll it consumes the approval and mints.
21+
* approves; on an authorized poll it mints, then consumes the approval — a
22+
* failed mint releases the reservation so a later poll can retry.
2223
*/
2324
export const POST = withRouteHandler(async (request: NextRequest) => {
2425
const rateLimited = await enforceIpRateLimit('cli-auth-poll', request)
@@ -36,9 +37,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
3637

3738
try {
3839
const key = await generateCopilotApiKey(result.userId, cliKeyName())
40+
await completeApproval(requestId)
3941
logger.info('Minted CLI key on approved poll', { userId: result.userId })
4042
return NextResponse.json({ status: 'complete', key })
4143
} catch (error) {
44+
await releaseMint(requestId)
4245
const status = error instanceof CopilotApiKeyError ? error.upstreamStatus : undefined
4346
return NextResponse.json(
4447
{ error: 'Failed to generate copilot API key' },

apps/sim/lib/cli-auth/approval-store.test.ts

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,12 @@ vi.mock('@/lib/core/config/redis', () => ({
1515
getRedisClient: mockGetRedisClient,
1616
}))
1717

18-
import { createApproval, pollApproval } from '@/lib/cli-auth/approval-store'
18+
import {
19+
completeApproval,
20+
createApproval,
21+
pollApproval,
22+
releaseMint,
23+
} from '@/lib/cli-auth/approval-store'
1924

2025
const REQUEST = 'a'.repeat(43)
2126
const SECRET = 'b'.repeat(43)
@@ -48,41 +53,60 @@ describe('cli-auth approval store', () => {
4853
})
4954

5055
describe('pollApproval', () => {
56+
const storedApproval = () =>
57+
JSON.stringify({ challenge: CHALLENGE, userId: 'user-1', createdAt: Date.now() })
58+
5159
it('returns pending when no approval exists yet', async () => {
5260
mockGet.mockResolvedValue(null)
5361
await expect(pollApproval(REQUEST, SECRET)).resolves.toEqual({ status: 'pending' })
54-
expect(mockDel).not.toHaveBeenCalled()
62+
expect(mockSet).not.toHaveBeenCalled()
5563
})
5664

57-
it('returns the user for a matching secret and consumes the approval', async () => {
58-
mockGet.mockResolvedValue(
59-
JSON.stringify({ challenge: CHALLENGE, userId: 'user-1', createdAt: Date.now() })
60-
)
61-
mockDel.mockResolvedValue(1)
65+
it('reserves the mint (does not consume) for a matching secret', async () => {
66+
mockGet.mockResolvedValue(storedApproval())
67+
mockSet.mockResolvedValue('OK')
6268
await expect(pollApproval(REQUEST, SECRET)).resolves.toEqual({
6369
status: 'approved',
6470
userId: 'user-1',
6571
})
66-
expect(mockDel).toHaveBeenCalledTimes(1)
72+
// NX lock on the mint key with a TTL; the approval itself is NOT deleted here.
73+
const [key, val, px, ttl, nx] = mockSet.mock.calls[0]
74+
expect(key).toContain('cli:auth:mint:')
75+
expect([val, px, ttl, nx]).toEqual(['1', 'PX', 30_000, 'NX'])
76+
expect(mockDel).not.toHaveBeenCalled()
6777
})
6878

69-
it('does NOT delete the approval when the secret is wrong', async () => {
70-
mockGet.mockResolvedValue(
71-
JSON.stringify({ challenge: CHALLENGE, userId: 'user-1', createdAt: Date.now() })
72-
)
79+
it('does NOT touch the record when the secret is wrong', async () => {
80+
mockGet.mockResolvedValue(storedApproval())
7381
await expect(pollApproval(REQUEST, 'c'.repeat(43))).resolves.toEqual({ status: 'pending' })
74-
expect(mockDel).not.toHaveBeenCalled()
82+
expect(mockSet).not.toHaveBeenCalled()
7583
})
7684

77-
it('yields to a concurrent poll that already claimed it', async () => {
78-
mockGet.mockResolvedValue(
79-
JSON.stringify({ challenge: CHALLENGE, userId: 'user-1', createdAt: Date.now() })
80-
)
81-
mockDel.mockResolvedValue(0)
85+
it('yields to a concurrent poll already holding the mint lock', async () => {
86+
mockGet.mockResolvedValue(storedApproval())
87+
mockSet.mockResolvedValue(null) // NX lost — someone else is minting
8288
await expect(pollApproval(REQUEST, SECRET)).resolves.toEqual({ status: 'pending' })
8389
})
8490
})
8591

92+
describe('completeApproval / releaseMint', () => {
93+
it('completeApproval deletes both the approval and the mint lock', async () => {
94+
mockDel.mockResolvedValue(2)
95+
await completeApproval(REQUEST)
96+
const keys = mockDel.mock.calls[0]
97+
expect(keys.some((k: string) => k.includes('cli:auth:req:'))).toBe(true)
98+
expect(keys.some((k: string) => k.includes('cli:auth:mint:'))).toBe(true)
99+
})
100+
101+
it('releaseMint drops only the mint lock, leaving the approval for a retry', async () => {
102+
mockDel.mockResolvedValue(1)
103+
await releaseMint(REQUEST)
104+
expect(mockDel).toHaveBeenCalledTimes(1)
105+
expect(mockDel.mock.calls[0][0]).toContain('cli:auth:mint:')
106+
expect(mockDel.mock.calls[0].join(' ')).not.toContain('cli:auth:req:')
107+
})
108+
})
109+
86110
describe('without Redis', () => {
87111
beforeEach(() => mockGetRedisClient.mockReturnValue(null))
88112

apps/sim/lib/cli-auth/approval-store.ts

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { getRedisClient } from '@/lib/core/config/redis'
1717
*/
1818

1919
const APPROVAL_TTL_MS = 120_000
20+
const MINT_LOCK_TTL_MS = 30_000
2021

2122
interface ApprovalRecord {
2223
/** BASE64URL(SHA256(pollSecret)) — the CLI proves possession of the secret at poll time. */
@@ -46,6 +47,11 @@ function approvalKey(requestId: string): string {
4647
return `cli:auth:req:${sha256Hex(requestId)}`
4748
}
4849

50+
/** Guards the mint step so two concurrent valid polls can't both mint a key. */
51+
function mintLockKey(requestId: string): string {
52+
return `cli:auth:mint:${sha256Hex(requestId)}`
53+
}
54+
4955
/** Records a signed-in user's approval. Overwrites any prior approval for the same request. */
5056
export async function createApproval(
5157
userId: string,
@@ -58,30 +64,45 @@ export async function createApproval(
5864
}
5965

6066
/**
61-
* Polls for an approval, consuming it exactly once.
67+
* Polls for an approval and reserves the mint slot, without consuming the
68+
* approval itself.
6269
*
63-
* `pending` covers every non-terminal state — not yet approved, expired, or a
64-
* wrong `pollSecret` — so the endpoint is not an oracle for which requests
65-
* exist or whether a secret is close. The approving user is returned only to a
66-
* caller that proves possession of the secret.
70+
* `pending` covers every non-terminal state — not yet approved, expired, a wrong
71+
* `pollSecret`, or another poll already minting — so the endpoint is not an
72+
* oracle for which requests exist or whether a secret is close. The approving
73+
* user is returned only to a caller that proves possession of the secret.
6774
*
68-
* Verifies *before* deleting: an attacker who knows the semi-public `requestId`
69-
* but not the secret can never trigger the delete, so they cannot cancel a
70-
* pending approval. The atomic `del` (returns the count removed) then makes the
71-
* claim single-use — two concurrent valid polls can't both mint.
75+
* Verifies *before* reserving: an attacker who knows the semi-public `requestId`
76+
* but not the secret can never touch the record. The reservation is an atomic
77+
* `SET NX` lock (not a delete) so two concurrent valid polls can't both mint,
78+
* while the approval survives a *failed* mint — the caller releases the lock and
79+
* a later poll retries, instead of forcing a fresh browser approval. Its short
80+
* TTL frees the slot if the minting caller dies. Callers MUST finish with
81+
* {@link completeApproval} on success or {@link releaseMint} on failure.
7282
*/
7383
export async function pollApproval(requestId: string, pollSecret: string): Promise<PollResult> {
7484
const redis = requireRedis()
75-
const key = approvalKey(requestId)
7685

77-
const raw = await redis.get(key)
86+
const raw = await redis.get(approvalKey(requestId))
7887
if (!raw) return { status: 'pending' }
7988

8089
const record = JSON.parse(raw) as ApprovalRecord
8190
if (!safeCompare(sha256Base64Url(pollSecret), record.challenge)) return { status: 'pending' }
8291

83-
const claimed = await redis.del(key)
84-
if (claimed !== 1) return { status: 'pending' }
92+
const reserved = await redis.set(mintLockKey(requestId), '1', 'PX', MINT_LOCK_TTL_MS, 'NX')
93+
if (reserved !== 'OK') return { status: 'pending' }
8594

8695
return { status: 'approved', userId: record.userId }
8796
}
97+
98+
/** Consumes the approval after a successful mint — single-use from here on. */
99+
export async function completeApproval(requestId: string): Promise<void> {
100+
const redis = requireRedis()
101+
await redis.del(approvalKey(requestId), mintLockKey(requestId))
102+
}
103+
104+
/** Releases the mint reservation after a failed mint so a later poll can retry. */
105+
export async function releaseMint(requestId: string): Promise<void> {
106+
const redis = requireRedis()
107+
await redis.del(mintLockKey(requestId))
108+
}

scripts/setup/modes/compose.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,19 @@ async function ensurePortsFree(composeFile: string): Promise<void> {
7070
p.log.step(
7171
`Killed ${killable.map((b) => `${b.owner?.command} (pid ${b.owner?.pid})`).join(', ')}`
7272
)
73+
// SIGKILL is async — the kernel releases the listening socket a beat after
74+
// the process dies, so re-checking immediately would still see the port
75+
// held. Wait for the killed ports to actually free before looping.
76+
await waitFor(
77+
async () => {
78+
for (const b of killable) {
79+
if (await portOpen(b.port)) return false
80+
}
81+
return true
82+
},
83+
5000,
84+
250
85+
)
7386
} else if (choice === 'abort') {
7487
throw new SetupError(
7588
'ports 3000/3002 are in use',

scripts/setup/modes/k8s.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,8 +155,8 @@ export async function runK8sMode(detection: Detection): Promise<void> {
155155
} catch (error) {
156156
spin.stop(`${glyph.fail} helm install failed`)
157157
throw new SetupError(getErrorMessage(error), [
158-
`pod status: ${theme.command(`kubectl -n ${NAMESPACE} get pods`)}`,
159-
`stuck pods: ${theme.command(`kubectl -n ${NAMESPACE} describe pod <name> | tail -20`)}`,
158+
`pod status: ${theme.command(`kubectl --context ${context} -n ${NAMESPACE} get pods`)}`,
159+
`stuck pods: ${theme.command(`kubectl --context ${context} -n ${NAMESPACE} describe pod <name> | tail -20`)}`,
160160
'ImagePullBackOff on ghcr.io/simstudioai/* usually means the chart appVersion tag was never published — check Chart.yaml against ghcr',
161161
])
162162
}
@@ -171,8 +171,8 @@ export async function runK8sMode(detection: Detection): Promise<void> {
171171
if (test.status !== 0) {
172172
testSpin.stop(`${glyph.fail} helm test failed`)
173173
throw new SetupError(`helm test failed:\n${test.stdout}${test.stderr}`, [
174-
`pod status: ${theme.command(`kubectl -n ${NAMESPACE} get pods`)}`,
175-
`app logs: ${theme.command(`kubectl -n ${NAMESPACE} logs deploy/${RELEASE}-app --tail 50`)}`,
174+
`pod status: ${theme.command(`kubectl --context ${context} -n ${NAMESPACE} get pods`)}`,
175+
`app logs: ${theme.command(`kubectl --context ${context} -n ${NAMESPACE} logs deploy/${RELEASE}-app --tail 50`)}`,
176176
])
177177
}
178178
testSpin.stop('helm test passed')

scripts/setup/redis.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ async function pingWithSpinner(url: string, label: string): Promise<boolean> {
3232
async function startManagedRedis(detection: Detection): Promise<string> {
3333
const hostPort = detection.redisPortOpen ? 6380 : 6379
3434
const url = `redis://localhost:${hostPort}`
35+
// A prior sim-redis (unhealthy, or bound to a stale port) would collide on the
36+
// name. Unlike Postgres this container has no data volume, so removing it
37+
// loses nothing — no prompt needed. `rm -f` is a no-op when none exists.
38+
spawnSync('docker', ['rm', '-f', REDIS_CONTAINER], { stdio: 'ignore' })
3539
docker([
3640
'run',
3741
'-d',
@@ -113,7 +117,7 @@ export async function resolveRedis(detection: Detection, existing?: string): Pro
113117
return managedUrl
114118
}
115119
p.log.warn(
116-
`${REDIS_CONTAINER} exists but is not answering — remove it with ${theme.command(`docker rm -f ${REDIS_CONTAINER}`)} before starting a new one.`
120+
`${REDIS_CONTAINER} exists but is not answering — starting a fresh one will replace it (it holds no data).`
117121
)
118122
}
119123

0 commit comments

Comments
 (0)