Skip to content

Commit 7cb64a6

Browse files
cmdr-characmdr-chara
committed
fix(suite): harden no-op sync and remote downloads
1 parent 51d7c47 commit 7cb64a6

3 files changed

Lines changed: 213 additions & 21 deletions

File tree

‎src/commands/suite.test.ts‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,60 @@ describe('suite plan', () => {
289289
expect(plan.items[0]).toMatchObject({ action: 'conflict' });
290290
expect(plan.items[0]?.reason).toContain('add testId');
291291
});
292+
293+
it('times out stalled presigned code downloads using the configured request deadline', async () => {
294+
const { manifestPath } = writeSuite([
295+
{ key: 'slow', testId: 'test_slow', name: 'Slow', codeFile: 'slow.py' },
296+
]);
297+
const fetchImpl: FetchImpl = async (input, init) => {
298+
const url = new URL(String(input));
299+
if (url.pathname.endsWith('/tests')) {
300+
return json({
301+
items: [
302+
{
303+
id: 'test_slow',
304+
projectId: 'proj_suite_1',
305+
name: 'Slow',
306+
type: 'backend',
307+
createdFrom: 'cli',
308+
status: 'ready',
309+
produces: [],
310+
consumes: [],
311+
createdAt: '2026-01-01T00:00:00.000Z',
312+
updatedAt: '2026-01-01T00:00:00.000Z',
313+
},
314+
],
315+
nextToken: null,
316+
});
317+
}
318+
if (url.pathname.endsWith('/tests/test_slow/code')) {
319+
return json({
320+
testId: 'test_slow',
321+
language: 'python',
322+
framework: 'pytest',
323+
code: 'https://storage.example.test/slow.py',
324+
codeVersion: 'v1',
325+
});
326+
}
327+
if (url.hostname === 'storage.example.test') {
328+
return new Promise<Response>((_resolve, reject) => {
329+
const signal = init?.signal;
330+
if (!signal) {
331+
reject(new Error('expected a request timeout signal'));
332+
return;
333+
}
334+
const rejectFromAbort = () => reject(signal.reason);
335+
if (signal.aborted) rejectFromAbort();
336+
else signal.addEventListener('abort', rejectFromAbort, { once: true });
337+
});
338+
}
339+
throw new Error(`unexpected request: ${url}`);
340+
};
341+
342+
await expect(
343+
runSuitePlan({ ...common(fetchImpl), manifestPath, requestTimeoutMs: 1 }, common(fetchImpl)),
344+
).rejects.toMatchObject({ name: 'RequestTimeoutError', timeoutMs: 1_000 });
345+
});
292346
});
293347

294348
describe('suite apply', () => {
@@ -400,6 +454,108 @@ describe('suite apply', () => {
400454
});
401455
});
402456

457+
it('refuses to apply a plan containing conflicts without sending mutations', async () => {
458+
const { manifestPath } = writeSuite([{ key: 'health', name: 'Health', codeFile: 'health.py' }]);
459+
let mutations = 0;
460+
const fetchImpl: FetchImpl = async (_input, init) => {
461+
if ((init?.method ?? 'GET') !== 'GET') mutations += 1;
462+
return json({
463+
items: [
464+
{
465+
id: 'test_existing',
466+
projectId: 'proj_suite_1',
467+
name: 'Health',
468+
type: 'backend',
469+
createdFrom: 'portal',
470+
status: 'ready',
471+
createdAt: '2026-01-01T00:00:00.000Z',
472+
updatedAt: '2026-01-01T00:00:00.000Z',
473+
},
474+
],
475+
nextToken: null,
476+
});
477+
};
478+
479+
await expect(
480+
runSuiteApply({ ...common(fetchImpl), manifestPath, confirm: true }, common(fetchImpl)),
481+
).rejects.toMatchObject({
482+
code: 'VALIDATION_ERROR',
483+
nextAction: expect.stringContaining('plan contains 1 conflict'),
484+
});
485+
expect(mutations).toBe(0);
486+
});
487+
488+
it('does not churn an unchanged lock entry timestamp on repeated apply', async () => {
489+
const { manifestPath, lockPath } = writeSuite([
490+
{ key: 'health', name: 'Health', codeFile: 'health.py' },
491+
]);
492+
const createFetch: FetchImpl = async (input, init) => {
493+
const url = new URL(String(input));
494+
if ((init?.method ?? 'GET') === 'GET' && url.pathname.endsWith('/tests')) {
495+
return json({ items: [], nextToken: null });
496+
}
497+
if ((init?.method ?? 'GET') === 'POST' && url.pathname.endsWith('/tests')) {
498+
return json({
499+
testId: 'test_health',
500+
type: 'backend',
501+
codeVersion: 'v1',
502+
createdAt: 'now',
503+
});
504+
}
505+
throw new Error(`unexpected request: ${init?.method ?? 'GET'} ${url.pathname}`);
506+
};
507+
await runSuiteApply(
508+
{ ...common(createFetch), manifestPath, confirm: true },
509+
common(createFetch),
510+
);
511+
const before = readFileSync(lockPath, 'utf8');
512+
513+
const noopFetch: FetchImpl = async input => {
514+
const url = new URL(String(input));
515+
if (url.pathname.endsWith('/tests')) {
516+
return json({
517+
items: [
518+
{
519+
id: 'test_health',
520+
projectId: 'proj_suite_1',
521+
name: 'Health',
522+
type: 'backend',
523+
createdFrom: 'cli',
524+
status: 'ready',
525+
produces: [],
526+
consumes: [],
527+
createdAt: '2026-01-01T00:00:00.000Z',
528+
updatedAt: '2026-01-01T00:00:00.000Z',
529+
},
530+
],
531+
nextToken: null,
532+
});
533+
}
534+
if (url.pathname.endsWith('/tests/test_health/code')) {
535+
return json({
536+
testId: 'test_health',
537+
language: 'python',
538+
framework: 'pytest',
539+
code: 'def test_health():\n assert True\n',
540+
codeVersion: 'v1',
541+
});
542+
}
543+
throw new Error(`unexpected request: ${url}`);
544+
};
545+
const later = {
546+
...common(noopFetch),
547+
now: () => new Date('2026-07-22T12:00:00.000Z'),
548+
};
549+
const result = await runSuiteApply({ ...later, manifestPath, confirm: true }, later);
550+
551+
expect('summary' in result && result.summary).toEqual({
552+
created: 0,
553+
updated: 0,
554+
unchanged: 1,
555+
});
556+
expect(readFileSync(lockPath, 'utf8')).toBe(before);
557+
});
558+
403559
it('resumes an unchanged pending create and conflicts if its definition drifted', async () => {
404560
const { manifestPath, lockPath, dir } = writeSuite([
405561
{ key: 'health', name: 'Health', codeFile: 'health.py' },

‎src/commands/suite.ts‎

Lines changed: 55 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,16 @@ import { Command } from 'commander';
1313
import {
1414
makeHttpClient,
1515
parseRequestTimeoutFlag,
16+
resolveRequestTimeoutMs,
1617
type CommonOptions,
1718
} from '../lib/client-factory.js';
18-
import { localValidationError } from '../lib/errors.js';
19-
import type { FetchImpl, HttpClient } from '../lib/http.js';
19+
import {
20+
ApiError,
21+
RequestTimeoutError,
22+
TransportError,
23+
localValidationError,
24+
} from '../lib/errors.js';
25+
import { createRequestTimeout, type FetchImpl, type HttpClient } from '../lib/http.js';
2026
import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js';
2127
import { paginate, type Page } from '../lib/pagination.js';
2228
import type {
@@ -430,12 +436,19 @@ export async function runSuiteApply(
430436
if (item.action === 'noop') {
431437
unchanged.push(item.key);
432438
if (item.testId) {
433-
lock.entries[item.key] = makeCompletedLockEntry(
434-
item.testId,
435-
item.remoteCode?.codeVersion,
436-
item.desiredHash,
437-
deps,
438-
);
439+
const existing = lock.entries[item.key];
440+
const meaningfulFieldsMatch =
441+
existing?.testId === item.testId &&
442+
existing.codeVersion === item.remoteCode?.codeVersion &&
443+
existing.desiredHash === item.desiredHash;
444+
if (!meaningfulFieldsMatch) {
445+
lock.entries[item.key] = makeCompletedLockEntry(
446+
item.testId,
447+
item.remoteCode?.codeVersion,
448+
item.desiredHash,
449+
deps,
450+
);
451+
}
439452
}
440453
continue;
441454
}
@@ -631,6 +644,7 @@ async function calculateSuitePlan(
631644

632645
const claimedRemoteIds = new Set<string>();
633646
const items: ResolvedPlanItem[] = [];
647+
const requestTimeoutMs = resolveRequestTimeoutMs(opts, deps.env ?? process.env);
634648
for (const spec of context.manifest.tests) {
635649
const desiredCode = context.codeByKey.get(spec.key)!;
636650
const desiredHashValue = context.desiredHashByKey.get(spec.key)!;
@@ -729,7 +743,11 @@ async function calculateSuitePlan(
729743
continue;
730744
}
731745
const remoteCode = await client.get<CliTestCode>(`/tests/${encodeURIComponent(testId)}/code`);
732-
const remoteCodeBody = await resolveRemoteCode(remoteCode.code, deps.fetchImpl);
746+
const remoteCodeBody = await resolveRemoteCode(
747+
remoteCode.code,
748+
deps.fetchImpl,
749+
requestTimeoutMs,
750+
);
733751
const changes = diffSuiteTest(spec, desiredCode, remote, remoteCodeBody);
734752
items.push({
735753
key: spec.key,
@@ -828,16 +846,34 @@ function createBody(
828846
};
829847
}
830848

831-
async function resolveRemoteCode(code: string, fetchImpl?: FetchImpl): Promise<string> {
849+
async function resolveRemoteCode(
850+
code: string,
851+
fetchImpl: FetchImpl | undefined,
852+
requestTimeoutMs: number,
853+
): Promise<string> {
832854
if (!code.startsWith('https://')) return code;
833-
const response = await (fetchImpl ?? globalThis.fetch)(code);
834-
if (!response.ok) {
835-
throw localValidationError(
836-
'suite',
837-
`failed to download remote test code (HTTP ${response.status})`,
838-
);
855+
const requestTimeout = createRequestTimeout(requestTimeoutMs);
856+
try {
857+
const response = await (fetchImpl ?? globalThis.fetch)(code, {
858+
signal: requestTimeout.signal,
859+
});
860+
if (!response.ok) {
861+
throw localValidationError(
862+
'suite',
863+
`failed to download remote test code (HTTP ${response.status})`,
864+
);
865+
}
866+
return await response.text();
867+
} catch (error) {
868+
if (error instanceof ApiError || error instanceof RequestTimeoutError) throw error;
869+
if (requestTimeout.signal.aborted) {
870+
throw new RequestTimeoutError(requestTimeoutMs);
871+
}
872+
const message = error instanceof Error ? error.message : String(error);
873+
throw new TransportError(`Failed to download remote test code: ${message}`);
874+
} finally {
875+
requestTimeout.clear();
839876
}
840-
return response.text();
841877
}
842878

843879
function loadSuiteLock(path: string, projectId: string): SuiteLock {
@@ -878,13 +914,13 @@ function loadSuiteLock(path: string, projectId: string): SuiteLock {
878914
throw localValidationError('lock-file', `entry ${key} is malformed`);
879915
}
880916
entries[key] = {
881-
desiredHash: value.desiredHash,
882-
updatedAt: value.updatedAt,
883917
...(typeof value.testId === 'string' ? { testId: value.testId } : {}),
884918
...(typeof value.codeVersion === 'string' || value.codeVersion === null
885919
? { codeVersion: value.codeVersion }
886920
: {}),
921+
desiredHash: value.desiredHash,
887922
...(typeof value.createKey === 'string' ? { createKey: value.createKey } : {}),
923+
updatedAt: value.updatedAt,
888924
};
889925
}
890926
return { schemaVersion: LOCK_SCHEMA_VERSION, projectId, entries };

‎src/lib/http.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -828,12 +828,12 @@ function newRequestId(): string {
828828
return `cli_${randomUUID()}`;
829829
}
830830

831-
interface RequestTimeoutHandle {
831+
export interface RequestTimeoutHandle {
832832
signal: AbortSignal;
833833
clear: () => void;
834834
}
835835

836-
function createRequestTimeout(timeoutMs: number): RequestTimeoutHandle {
836+
export function createRequestTimeout(timeoutMs: number): RequestTimeoutHandle {
837837
const controller = new AbortController();
838838
const timer = setTimeout(() => {
839839
controller.abort(makeTimeoutReason());

0 commit comments

Comments
 (0)