Skip to content

Commit f3c669a

Browse files
icecrasher321claude
andcommitted
fix(deploy): stop the redeploy check checking out a second connection
`checkNeedsRedeployment` opens a REPEATABLE READ transaction and then called `materializeDeploymentState` without a workspaceId, which resolves one through `getActiveWorkflowContext` — on the global pool. A transaction holding one connection while awaiting a second checkout starves the pool under any concurrency, and this endpoint is polled and refetches on window focus. The nested read then failed and surfaced as a 500 on `/api/workflows/[id]/deploy`, with the failing statement being the authz context lookup rather than anything the caller wrote. Introduced by the operand fix earlier on this branch. `materializeDeploymentState` now REQUIRES a workspaceId, so it cannot check out a connection at all and is safe inside any transaction by construction; the two non-transactional entry points resolve theirs through a named helper that says so. The type change surfaced both remaining callers rather than leaving the hazard to be avoided by convention. The UI half: an absent answer was rendered as a positive one, three times over. `isDeployed` is `deploymentInfo?.isDeployed ?? false`, so a pending OR FAILED request was indistinguishable from a genuinely undeployed workflow — the modal told the user to deploy a workflow whose version list showed v4 live. The status now reports `unknown` until deployment info actually answers, the chip is disabled there (a click is interpreted against that same flag: deployed opens the modal, undeployed deploys), and the General tab renders a skeleton rather than claiming undeployed whenever the live workflow cannot yet be shown. No retry or fallback: the earlier draft of this commit polled the snapshot query while it held no data, which papered over the pool starvation instead of fixing it. That is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent d54b8d3 commit f3c669a

8 files changed

Lines changed: 148 additions & 39 deletions

File tree

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,13 @@ export function Deploy({ activeWorkflowId, userPermissions, disabled = false }:
5454
isDeploying ||
5555
!canDeploy ||
5656
isEmpty ||
57+
/*
58+
* A click is interpreted against `isDeployed`: deployed opens the modal,
59+
* undeployed deploys. While that is unknown the click has no defined
60+
* meaning, and guessing "undeployed" would turn a failed info read into an
61+
* unintended new version.
62+
*/
63+
buttonStatus === 'unknown' ||
5764
(!isDeployed && deployReadiness.isBlocked && !deployReadiness.isSyncing)
5865

5966
const onDeployClick = async () => {

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.test.ts

Lines changed: 55 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ type Input = Parameters<typeof resolveDeployButtonStatus>[0]
1111

1212
const base: Input = {
1313
workflowId: 'wf-1',
14+
isDeploymentInfoResolved: false,
1415
isDeployed: false,
1516
isAwaitingFirstDeployedState: false,
1617
clientChangeDetected: false,
@@ -39,28 +40,44 @@ describe('resolveDeployButtonStatus', () => {
3940
// 1. Nothing loaded.
4041
{},
4142
// 2. deploymentInfo lands — isDeployed and needsRedeployment arrive together.
42-
{ isDeployed: true, serverNeedsRedeployment: true, isAwaitingFirstDeployedState: true },
43+
{
44+
isDeploymentInfoResolved: true,
45+
isDeployed: true,
46+
serverNeedsRedeployment: true,
47+
isAwaitingFirstDeployedState: true,
48+
},
4349
// 3. The deployed snapshot lands; the client diff agrees.
4450
{
51+
isDeploymentInfoResolved: true,
4552
isDeployed: true,
4653
serverNeedsRedeployment: true,
4754
hasDeployedState: true,
4855
clientChangeDetected: true,
4956
},
5057
])
5158

52-
expect(statuses).toEqual(['undeployed', 'changed'])
59+
expect(statuses).toEqual(['unknown', 'changed'])
5360
expect(statuses).not.toContain('live')
5461
})
5562

5663
it('settles straight to live for a deployed workflow with no changes', () => {
5764
const statuses = committed([
5865
{},
59-
{ isDeployed: true, serverNeedsRedeployment: false, isAwaitingFirstDeployedState: true },
60-
{ isDeployed: true, serverNeedsRedeployment: false, hasDeployedState: true },
66+
{
67+
isDeploymentInfoResolved: true,
68+
isDeployed: true,
69+
serverNeedsRedeployment: false,
70+
isAwaitingFirstDeployedState: true,
71+
},
72+
{
73+
isDeploymentInfoResolved: true,
74+
isDeployed: true,
75+
serverNeedsRedeployment: false,
76+
hasDeployedState: true,
77+
},
6178
])
6279

63-
expect(statuses).toEqual(['undeployed', 'live'])
80+
expect(statuses).toEqual(['unknown', 'live'])
6481
expect(statuses).not.toContain('changed')
6582
})
6683

@@ -70,6 +87,7 @@ describe('resolveDeployButtonStatus', () => {
7087
*/
7188
it('holds its answer across a background refetch', () => {
7289
const settled: Partial<Input> = {
90+
isDeploymentInfoResolved: true,
7391
isDeployed: true,
7492
serverNeedsRedeployment: true,
7593
hasDeployedState: true,
@@ -90,6 +108,7 @@ describe('resolveDeployButtonStatus', () => {
90108
// Unsaved edits: the server still describes the persisted draft.
91109
const status = resolveDeployButtonStatus({
92110
...base,
111+
isDeploymentInfoResolved: true,
93112
isDeployed: true,
94113
serverNeedsRedeployment: false,
95114
hasDeployedState: true,
@@ -103,9 +122,40 @@ describe('resolveDeployButtonStatus', () => {
103122
expect(resolveDeployButtonStatus({ ...base, workflowId: null })).toBe('undeployed')
104123
})
105124

125+
/**
126+
* `GET /api/workflows/[id]/deploy` returning 500 made `isDeployed` default to
127+
* false, which rendered a live workflow as "Deploy" beside a version list
128+
* showing v4 live — an absence of information presented as a fact.
129+
*
130+
* The click is also interpreted against the same flag (deployed opens the
131+
* modal, undeployed deploys), so guessing here decides an action, not just a
132+
* label. Both reasons say the same thing: do not answer until asked.
133+
*/
134+
it('does not claim undeployed when deployment info has not answered', () => {
135+
const status = resolveDeployButtonStatus({
136+
...base,
137+
isDeploymentInfoResolved: false,
138+
isDeployed: false,
139+
})
140+
141+
expect(status).toBe('unknown')
142+
expect(status).not.toBe('undeployed')
143+
})
144+
145+
it('reports undeployed only once info has actually said so', () => {
146+
const status = resolveDeployButtonStatus({
147+
...base,
148+
isDeploymentInfoResolved: true,
149+
isDeployed: false,
150+
})
151+
152+
expect(status).toBe('undeployed')
153+
})
154+
106155
it('falls back to unknown only when deployed with no verdict from either side', () => {
107156
const status = resolveDeployButtonStatus({
108157
...base,
158+
isDeploymentInfoResolved: true,
109159
isDeployed: true,
110160
isAwaitingFirstDeployedState: true,
111161
serverNeedsRedeployment: undefined,

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deploy-button-status.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,12 @@ export type DeployButtonStatus = 'unknown' | 'undeployed' | 'live' | 'changed'
1010

1111
interface ResolveDeployButtonStatusInput {
1212
workflowId: string | null
13+
/**
14+
* Whether deployment info has actually answered. `isDeployed` defaults to
15+
* `false` while the request is pending OR failed, so without this the two are
16+
* indistinguishable and a 500 reads as "not deployed".
17+
*/
18+
isDeploymentInfoResolved: boolean
1319
isDeployed: boolean
1420
/** True only before the FIRST deployed snapshot lands — never on a refetch. */
1521
isAwaitingFirstDeployedState: boolean
@@ -49,13 +55,27 @@ interface ResolveDeployButtonStatusInput {
4955
*/
5056
export function resolveDeployButtonStatus({
5157
workflowId,
58+
isDeploymentInfoResolved,
5259
isDeployed,
5360
isAwaitingFirstDeployedState,
5461
clientChangeDetected,
5562
hasDeployedState,
5663
serverNeedsRedeployment,
5764
}: ResolveDeployButtonStatusInput): DeployButtonStatus {
58-
if (!workflowId || !isDeployed) return 'undeployed'
65+
if (!workflowId) return 'undeployed'
66+
67+
/*
68+
* Not knowing is its own answer. `isDeployed` is `deploymentInfo?.isDeployed
69+
* ?? false`, so a pending or FAILED info request is indistinguishable from a
70+
* genuinely undeployed workflow — and a transient 500 on that endpoint
71+
* rendered a live workflow as "Deploy", next to a version list showing v4
72+
* live. Worse, the chip acts on that: with `isDeployed` false a click runs a
73+
* fresh deploy instead of opening the modal, so a failed read could be
74+
* converted into an unintended new version.
75+
*/
76+
if (!isDeploymentInfoResolved) return 'unknown'
77+
78+
if (!isDeployed) return 'undeployed'
5979

6080
if (hasDeployedState && !isAwaitingFirstDeployedState) {
6181
return clientChangeDetected ? 'changed' : 'live'

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/hooks/use-deployment-view-state.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ export function useDeploymentViewState({
5252
deployReadiness,
5353
}: UseDeploymentViewStateProps): DeploymentViewState {
5454
const { data: deploymentInfo } = useDeploymentInfo(workflowId, { enabled })
55+
/* Undefined covers both "still loading" and "the request failed". */
56+
const isDeploymentInfoResolved = deploymentInfo !== undefined
5557
const isDeployed = deploymentInfo?.isDeployed ?? false
5658

5759
const snapshotEnabled = Boolean(workflowId) && isDeployed && enabled
@@ -77,6 +79,7 @@ export function useDeploymentViewState({
7779

7880
const status = resolveDeployButtonStatus({
7981
workflowId,
82+
isDeploymentInfoResolved,
8083
isDeployed,
8184
isAwaitingFirstDeployedState: isLoadingDeployedState,
8285
clientChangeDetected: changeDetected,
@@ -99,7 +102,12 @@ export function useDeploymentViewState({
99102
status,
100103
isDeployed,
101104
deployedState,
102-
isAwaitingSnapshot: snapshotEnabled && deployedState === null,
105+
/*
106+
* "We cannot show you the live workflow yet" covers both a snapshot in
107+
* flight and not knowing whether one exists. Neither is evidence the
108+
* workflow is undeployed, so neither may render as that claim.
109+
*/
110+
isAwaitingSnapshot: status === 'unknown' || (snapshotEnabled && deployedState === null),
103111
isSettling,
104112
changeDetected,
105113
changedFields,

apps/sim/hooks/queries/deployments.ts

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,6 @@ export type { ChatDetail, DeploymentVersionsResponse }
3434
export const DEPLOYMENT_INFO_STALE_TIME = 30 * 1000
3535
export const DEPLOYMENT_STATUS_REFETCH_INTERVAL = 5 * 1000
3636
export const DEPLOYED_WORKFLOW_STATE_STALE_TIME = 30 * 1000
37-
/** Retry cadence while the deployed snapshot is expected but not yet readable. */
38-
export const DEPLOYED_STATE_RECOVERY_INTERVAL = 3 * 1000
3937
export const DEPLOYMENT_VERSIONS_STALE_TIME = 30 * 1000
4038
export const CHAT_DEPLOYMENT_STATUS_STALE_TIME = 30 * 1000
4139
export const CHAT_DETAIL_STALE_TIME = 30 * 1000
@@ -160,19 +158,6 @@ export function useDeployedWorkflowState(
160158
queryFn: ({ signal }) => fetchDeployedWorkflowState(workflowId!, signal),
161159
enabled: Boolean(workflowId) && (options?.enabled ?? true),
162160
staleTime: DEPLOYED_WORKFLOW_STATE_STALE_TIME,
163-
/*
164-
* Callers enable this only once deployment info reports the workflow as
165-
* deployed, so a null snapshot is a contradiction rather than an answer:
166-
* the version exists but is not readable yet.
167-
*
168-
* Nothing re-invalidates this key when activation cuts over —
169-
* `refetchDeploymentBoundary` fires while the query is still disabled, and a
170-
* disabled query cannot be refetched — so the null was cached for the whole
171-
* stale window. That is what put an empty preview next to a live version
172-
* row. Retrying resolves the contradiction and stops the instant it does.
173-
*/
174-
refetchInterval: (query) =>
175-
query.state.data == null ? DEPLOYED_STATE_RECOVERY_INTERVAL : false,
176161
})
177162
}
178163

apps/sim/lib/workflows/deployment-status.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { db, workflowDeploymentVersion } from '@sim/db'
2+
import { workflow as workflowTable } from '@sim/db/schema'
23
import { and, desc, eq, sql } from 'drizzle-orm'
34
import { hasWorkflowChanged } from '@/lib/workflows/comparison'
45
import {
@@ -22,12 +23,23 @@ import type { WorkflowState } from '@/stores/workflows/workflow/types'
2223
export async function checkNeedsRedeployment(workflowId: string): Promise<boolean> {
2324
return db.transaction(async (tx) => {
2425
await tx.execute(sql`SET TRANSACTION ISOLATION LEVEL REPEATABLE READ`)
26+
/*
27+
* `workspaceId` is selected here, in this transaction, rather than left for
28+
* `materializeDeploymentState` to look up. It resolves an absent one through
29+
* `getActiveWorkflowContext`, which runs on the global pool — a second
30+
* connection checkout while this transaction already holds one. Under any
31+
* concurrency (this endpoint is polled, and refetches on window focus) that
32+
* starves the pool and fails the nested read, surfacing as a 500 on
33+
* `/api/workflows/[id]/deploy`. A transaction must not await a checkout.
34+
*/
2535
const [active] = await tx
2636
.select({
2737
id: workflowDeploymentVersion.id,
2838
state: workflowDeploymentVersion.state,
39+
workspaceId: workflowTable.workspaceId,
2940
})
3041
.from(workflowDeploymentVersion)
42+
.innerJoin(workflowTable, eq(workflowTable.id, workflowDeploymentVersion.workflowId))
3143
.where(
3244
and(
3345
eq(workflowDeploymentVersion.workflowId, workflowId),
@@ -37,7 +49,8 @@ export async function checkNeedsRedeployment(workflowId: string): Promise<boolea
3749
.orderBy(desc(workflowDeploymentVersion.createdAt))
3850
.limit(1)
3951

40-
if (!active?.state) return false
52+
/* The inner join guarantees a workspace row; a null id means unusable data. */
53+
if (!active?.state || !active.workspaceId) return false
4154

4255
/*
4356
* Sequential, not `Promise.all`: both reads share the transaction's single
@@ -46,7 +59,12 @@ export async function checkNeedsRedeployment(workflowId: string): Promise<boolea
4659
const currentState = await loadWorkflowDeploymentSnapshot(workflowId, tx)
4760
if (!currentState) return false
4861

49-
const deployedState = await materializeDeploymentState(workflowId, active, undefined, tx)
62+
const deployedState = await materializeDeploymentState(
63+
workflowId,
64+
active,
65+
active.workspaceId,
66+
tx
67+
)
5068

5169
return hasWorkflowChanged(currentState, deployedState as WorkflowState)
5270
})

apps/sim/lib/workflows/persistence/utils.ts

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,20 @@ export function invalidateDeployedStateCache(deploymentVersionId?: string): void
145145
deployedStateCache.clear()
146146
}
147147

148-
export interface DeploymentStateRow {
148+
export /**
149+
* Only for entry points that are NOT inside a transaction — it checks out a
150+
* connection of its own.
151+
*/
152+
async function resolveWorkspaceId(workflowId: string, provided?: string): Promise<string> {
153+
if (provided) return provided
154+
const workflowContext = await getActiveWorkflowContext(workflowId)
155+
if (!workflowContext?.workspaceId) {
156+
throw new Error(`Workflow ${workflowId} has no workspace`)
157+
}
158+
return workflowContext.workspaceId
159+
}
160+
161+
interface DeploymentStateRow {
149162
id: string
150163
state: unknown
151164
}
@@ -160,10 +173,18 @@ export interface DeploymentStateRow {
160173
* the handle canonicalization and the `errorEnabled` backfill below, the two
161174
* surfaces answered the same question differently for the same workflow.
162175
*/
176+
/**
177+
* `workspaceId` is required rather than resolved here on purpose. Resolving it
178+
* means `getActiveWorkflowContext`, which runs on the global pool — and this
179+
* function is called from inside a REPEATABLE READ transaction by
180+
* `checkNeedsRedeployment`, where a second connection checkout while holding one
181+
* starves the pool under concurrency and fails the nested read. Taking it as an
182+
* argument makes that impossible instead of merely avoided.
183+
*/
163184
export async function materializeDeploymentState(
164185
workflowId: string,
165186
version: DeploymentStateRow,
166-
providedWorkspaceId?: string,
187+
workspaceId: string,
167188
executor?: DbOrTx
168189
): Promise<DeployedWorkflowData> {
169190
const cached = deployedStateCache.get(version.id)
@@ -172,19 +193,10 @@ export async function materializeDeploymentState(
172193
}
173194

174195
const state = version.state as WorkflowState & { variables?: Record<string, unknown> }
175-
let resolvedWorkspaceId = providedWorkspaceId
176-
if (!resolvedWorkspaceId) {
177-
const workflowContext = await getActiveWorkflowContext(workflowId)
178-
resolvedWorkspaceId = workflowContext?.workspaceId
179-
}
180-
181-
if (!resolvedWorkspaceId) {
182-
throw new Error(`Workflow ${workflowId} has no workspace`)
183-
}
184196

185197
const { blocks: migratedBlocks } = await applyBlockMigrations(
186198
state.blocks || {},
187-
resolvedWorkspaceId,
199+
workspaceId,
188200
executor
189201
)
190202
/*
@@ -255,7 +267,11 @@ export async function loadDeployedWorkflowState(
255267
throw new NoActiveDeploymentError(workflowId)
256268
}
257269

258-
return materializeDeploymentState(workflowId, active, providedWorkspaceId)
270+
return materializeDeploymentState(
271+
workflowId,
272+
active,
273+
await resolveWorkspaceId(workflowId, providedWorkspaceId)
274+
)
259275
} catch (error) {
260276
logger.error(`Error loading deployed workflow state ${workflowId}:`, error)
261277
throw error
@@ -288,7 +304,11 @@ export async function loadWorkflowDeploymentVersionState(
288304
throw new Error(`Deployment ${deploymentVersionId} was not found for workflow ${workflowId}`)
289305
}
290306

291-
return materializeDeploymentState(workflowId, version, providedWorkspaceId)
307+
return materializeDeploymentState(
308+
workflowId,
309+
version,
310+
await resolveWorkspaceId(workflowId, providedWorkspaceId)
311+
)
292312
}
293313

294314
interface MigrationContext {

apps/sim/scripts/dump-change-detection-states.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,13 +186,14 @@ async function main(): Promise<void> {
186186
const webhooks = webhooksByWorkflow.get(row.workflowId)
187187
if (webhooksOnly && !webhooks) continue
188188

189+
if (!row.workspaceId) continue
189190
const current = await loadWorkflowDeploymentSnapshot(row.workflowId)
190191
if (!current) continue
191192

192193
const deployed = await materializeDeploymentState(
193194
row.workflowId,
194195
{ id: row.versionId, state: row.state },
195-
row.workspaceId ?? undefined
196+
row.workspaceId
196197
)
197198

198199
const currentBlocks =

0 commit comments

Comments
 (0)