Skip to content

Commit 9e8b999

Browse files
committed
fix(agiloft): resolve the instance once so pre-send failures stay pre-flight
Review finding. The create route resolves the instance URL itself to keep a rejected host on the pre-flight side of the line, but executeEwRequest then resolved it a second time before sending. A DNS failure on that second lookup never sent the create, yet every throw out of the executor was reported as an unconfirmed write that must not be retried - telling the caller a record might exist when nothing had been transmitted. The resolved IP is now handed down, so there is exactly one resolution and everything after it is genuinely post-transmit. It also drops the duplicate DNS lookup the two-resolve arrangement was paying for.
1 parent 21c6ad5 commit 9e8b999

3 files changed

Lines changed: 43 additions & 4 deletions

File tree

apps/sim/app/api/tools/agiloft/create_record/route.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,36 @@ describe('EWCreate', () => {
269269
expect(data.output.id).toBeNull()
270270
})
271271

272+
/**
273+
* A rejected instance URL is decided before anything is sent, so it must not
274+
* come back wearing the do-not-retry warning. The route resolves the instance
275+
* itself and hands the result down, so there is exactly one resolution and
276+
* everything after it is genuinely post-transmit.
277+
*/
278+
it('reports a rejected instance URL as a pre-flight failure, not an unconfirmed write', async () => {
279+
inputValidationMockFns.mockValidateUrlWithDNS.mockResolvedValueOnce({
280+
isValid: false,
281+
error: 'URL resolves to a private IP address',
282+
})
283+
284+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
285+
const data = (await response.json()) as { success: boolean; error?: string }
286+
287+
expect(response.status).toBe(400)
288+
expect(data.success).toBe(false)
289+
expect(data.error).toContain('private IP')
290+
expect(data.error).not.toContain('may exist')
291+
expect(inputValidationMockFns.mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
292+
})
293+
294+
it('resolves the instance once, so a DNS failure cannot land on the wrong side of the line', async () => {
295+
arrangeCreate(res({ text: "EWREST_id='353';" }))
296+
297+
await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
298+
299+
expect(inputValidationMockFns.mockValidateUrlWithDNS).toHaveBeenCalledTimes(1)
300+
})
301+
272302
/**
273303
* The request was already on the wire, so the write may have committed. A 500
274304
* here is what would have the caller retry and duplicate the record.

apps/sim/app/api/tools/agiloft/create_record/route.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,9 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
103103
* URL stays a pre-flight failure. Everything thrown after this point has to
104104
* be treated as "the request may have been transmitted".
105105
*/
106+
let resolvedIP: string
106107
try {
107-
await resolveAgiloftInstance(params.instanceUrl)
108+
resolvedIP = await resolveAgiloftInstance(params.instanceUrl)
108109
} catch (error) {
109110
logger.warn(`[${requestId}] Rejected Agiloft instance URL`, { error })
110111
return NextResponse.json(
@@ -207,7 +208,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
207208
}
208209

209210
return { success: true, output: { id, fields } }
210-
}
211+
},
212+
resolvedIP
211213
)
212214
} catch (error) {
213215
/**

apps/sim/tools/agiloft/utils.server.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,9 +331,16 @@ export async function executeAlrestRequest<R extends ToolResponse>(
331331
export async function executeEwRequest<R extends ToolResponse>(
332332
params: AgiloftCredentials,
333333
buildRequest: (base: string) => AgiloftRequestConfig,
334-
transformResponse: (response: SecureFetchResponse) => Promise<R>
334+
transformResponse: (response: SecureFetchResponse) => Promise<R>,
335+
preResolvedIP?: string
335336
): Promise<R> {
336-
const resolvedIP = await resolveAgiloftInstance(params.instanceUrl)
337+
/**
338+
* A write caller resolves the instance itself so it can tell a rejected host
339+
* (nothing sent) from a failure after the request went out (the write may
340+
* have landed). Resolving a second time here would put a DNS failure on the
341+
* wrong side of that line, so the caller's already-validated IP is reused.
342+
*/
343+
const resolvedIP = preResolvedIP ?? (await resolveAgiloftInstance(params.instanceUrl))
337344
const req = buildRequest(params.instanceUrl.replace(/\/$/, ''))
338345
const response = await secureFetchWithPinnedIP(req.url, resolvedIP, {
339346
method: req.method,

0 commit comments

Comments
 (0)