Skip to content

Commit fbb8def

Browse files
Iris Alexandrescuclaude
andcommitted
fix(serenity): retry transient upstream/network errors during a workspace status poll (LLMO-7418 external-review Finding 14)
Previously any error from transport.getWorkspaceStatus() — including a transient Semrush 503 or a network blip — landed in the generic outer catch and permanently failed the whole provisioning attempt with zero retries, even though the worker already has a bounded self-requeue mechanism built for exactly this "try again shortly" case. Classify by error.status: SerenityTransportError with 429/500/502/503/504, or any error with no status at all (a raw network-level failure), is now treated as transient — the poll result falls through as if the workspace were merely "not ready", routing through the EXISTING self-requeue ladder (same backoff/depth cap, no new mechanism). Everything else (a permanent 4xx like an expired IMS token) keeps today's fail-fast behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent a7cc69a commit fbb8def

2 files changed

Lines changed: 133 additions & 1 deletion

File tree

src/support/serenity/handlers/provision-workspace-job.js

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,23 @@ import {
3838
*/
3939
export const PROVISION_WORKSPACE_JOB_TYPE = 'serenity-provision-workspace';
4040

41+
// LLMO-7418 external-review Finding 14: retry classification for a status-poll failure.
42+
// `SerenityTransportError` always carries a numeric `.status` (the upstream HTTP status);
43+
// a raw network-level failure (fetch itself throwing — DNS, connection reset, timeout) has
44+
// none. Only these are treated as transient and routed through the existing bounded
45+
// self-requeue ladder below; everything else (a permanent 4xx like an expired/invalid IMS
46+
// token, or an unexpected non-transport error) keeps today's fail-fast behavior via the
47+
// outer catch.
48+
const RETRYABLE_TRANSPORT_STATUSES = new Set([429, 500, 502, 503, 504]);
49+
function isRetryableWorkspaceStatusError(error) {
50+
const { status } = error ?? {};
51+
if (typeof status !== 'number') {
52+
// No upstream status at all — a network-level failure, not an application error.
53+
return true;
54+
}
55+
return RETRYABLE_TRANSPORT_STATUSES.has(status);
56+
}
57+
4158
/**
4259
* Hard cap on self-requeue depth. Live-verified settle time for a SUCCESSFUL create is
4360
* seconds, not minutes (LLMO-7352 incident data: ~10s); this ladder exists for the
@@ -382,7 +399,25 @@ export async function provisionWorkspaceHandler(context, job, accessToken) {
382399
}
383400
}
384401

385-
const statusResult = await transport.getWorkspaceStatus(candidate.workspaceId);
402+
let statusResult;
403+
try {
404+
statusResult = await transport.getWorkspaceStatus(candidate.workspaceId);
405+
} catch (error) {
406+
if (!isRetryableWorkspaceStatusError(error)) {
407+
throw error;
408+
}
409+
// Transient upstream/network failure — leave `statusResult` undefined so `status`
410+
// below is `undefined`, which is neither ready nor terminal-failure, and this hop
411+
// falls straight into the existing "still settling" self-requeue branch below (same
412+
// bounded backoff/depth cap already used for an actual `not ready` poll result).
413+
log?.warn?.('provision-workspace-job: transient error polling workspace status; treating as not-ready and self-requeuing', {
414+
brandId,
415+
attemptId,
416+
semrushWorkspaceId: candidate.workspaceId,
417+
error: error?.message,
418+
status: error?.status,
419+
});
420+
}
386421
const status = statusResult?.status;
387422

388423
if (isWorkspaceReady(status)) {

test/support/serenity/handlers/provision-workspace-job.test.js

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,103 @@ describe('handlers/provision-workspace-job.js (LLMO-7352 / LLMO-7418)', () => {
757757
});
758758
});
759759

760+
describe('poll result: transient error retry classification (LLMO-7418 external-review Finding 14)', () => {
761+
it('treats a 503 SerenityTransportError as transient and self-requeues instead of failing the attempt', async () => {
762+
const err = new Error('Semrush GET .../status failed: 503');
763+
err.status = 503;
764+
transport.getWorkspaceStatus.rejects(err);
765+
const { provisionWorkspaceHandler } = await loadHandler();
766+
const job = makeJob(makeMetadata({ requeueDepth: 0 }));
767+
768+
const result = await provisionWorkspaceHandler(context, job, 'token');
769+
770+
expect(createAndEnqueueJobStub).to.have.been.calledOnce;
771+
expect(promoteProvisioningFailedStub).to.not.have.been.called;
772+
expect(result).to.deep.equal({ requeuedJobId: 'job-followup' });
773+
});
774+
775+
it('retries 429/500/502/504 the same way', async () => {
776+
for (const status of [429, 500, 502, 504]) {
777+
const err = new Error(`upstream ${status}`);
778+
err.status = status;
779+
transport.getWorkspaceStatus.reset();
780+
transport.getWorkspaceStatus.rejects(err);
781+
createAndEnqueueJobStub.resetHistory();
782+
promoteProvisioningFailedStub.resetHistory();
783+
// eslint-disable-next-line no-await-in-loop
784+
const { provisionWorkspaceHandler } = await loadHandler();
785+
const job = makeJob(makeMetadata({ requeueDepth: 0 }));
786+
787+
// eslint-disable-next-line no-await-in-loop
788+
const result = await provisionWorkspaceHandler(context, job, 'token');
789+
790+
expect(createAndEnqueueJobStub, `status ${status}`).to.have.been.calledOnce;
791+
expect(promoteProvisioningFailedStub, `status ${status}`).to.not.have.been.called;
792+
expect(result, `status ${status}`).to.deep.equal({ requeuedJobId: 'job-followup' });
793+
}
794+
});
795+
796+
it('treats a raw network failure (no .status) as transient and self-requeues', async () => {
797+
transport.getWorkspaceStatus.rejects(new TypeError('fetch failed'));
798+
const { provisionWorkspaceHandler } = await loadHandler();
799+
const job = makeJob(makeMetadata({ requeueDepth: 0 }));
800+
801+
const result = await provisionWorkspaceHandler(context, job, 'token');
802+
803+
expect(createAndEnqueueJobStub).to.have.been.calledOnce;
804+
expect(result).to.deep.equal({ requeuedJobId: 'job-followup' });
805+
});
806+
807+
it('still fails fast on a permanent 401 (expired/invalid IMS token), never self-requeuing it', async () => {
808+
const err = new Error('Semrush GET .../status failed: 401');
809+
err.status = 401;
810+
transport.getWorkspaceStatus.rejects(err);
811+
const { provisionWorkspaceHandler } = await loadHandler();
812+
const job = makeJob(makeMetadata());
813+
814+
await expect(provisionWorkspaceHandler(context, job, 'token')).to.be.rejectedWith('401');
815+
816+
expect(createAndEnqueueJobStub).to.not.have.been.called;
817+
expect(promoteProvisioningFailedStub).to.have.been.calledOnceWith({
818+
brandId: BRAND_ID,
819+
attemptId: ATTEMPT_ID,
820+
error: UNEXPECTED_ERROR_MESSAGE,
821+
postgrestClient,
822+
});
823+
});
824+
825+
it('still fails fast on a permanent 400, never self-requeuing it', async () => {
826+
const err = new Error('Semrush GET .../status failed: 400');
827+
err.status = 400;
828+
transport.getWorkspaceStatus.rejects(err);
829+
const { provisionWorkspaceHandler } = await loadHandler();
830+
const job = makeJob(makeMetadata());
831+
832+
await expect(provisionWorkspaceHandler(context, job, 'token')).to.be.rejectedWith('400');
833+
834+
expect(createAndEnqueueJobStub).to.not.have.been.called;
835+
});
836+
837+
it('still respects the requeue depth cap for a transient error at the cap (fails, does not loop forever)', async () => {
838+
const err = new Error('upstream 503');
839+
err.status = 503;
840+
transport.getWorkspaceStatus.rejects(err);
841+
const { provisionWorkspaceHandler } = await loadHandler();
842+
const job = makeJob(makeMetadata({ requeueDepth: MAX_PROVISION_REQUEUE_DEPTH }));
843+
844+
const result = await provisionWorkspaceHandler(context, job, 'token');
845+
846+
expect(createAndEnqueueJobStub).to.not.have.been.called;
847+
expect(promoteProvisioningFailedStub).to.have.been.calledOnceWith({
848+
brandId: BRAND_ID,
849+
attemptId: ATTEMPT_ID,
850+
error: REQUEUE_EXHAUSTED_MESSAGE,
851+
postgrestClient,
852+
});
853+
expect(result).to.deep.equal({ provisioningStatus: 'failed' });
854+
});
855+
});
856+
760857
describe('unexpected errors reaching the outer catch clean up an owned candidate (LLMO-7418 external-review Finding 16)', () => {
761858
it('cleans up a freshly-created candidate when an unexpected error is thrown BEFORE any self-requeue is enqueued', async () => {
762859
getBrandProvisioningStateStub.rejects(new Error('db read blip'));

0 commit comments

Comments
 (0)