Skip to content

fix: retry upload on stashfilestorage/backend-fail-internal storage errors #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaxServer merged 1 commit into from
Jul 5, 2026
Merged

fix: retry upload on stashfilestorage/backend-fail-internal storage errors #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions backend/src/__tests__/uploadClient.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,99 @@ describe('MediaWikiClient.uploadFile error paths', () => {
await expect(callUploadFile(client, redis)).rejects.toBeInstanceOf(StorageError)
})

it('throws StorageError when chunk upload returns stashfilestorage (storage backend failure)', async () => {
const { client, redis } = makeChunkUploadClient()
// biome-ignore lint/suspicious/noExplicitAny: overriding private methods for testing
;(client as any).apiUploadChunk = mock(async () => ({
error: {
code: 'stashfilestorage',
info: 'An unknown error occurred in storage backend "local-swift-codfw".',
},
}))
await expect(callUploadFile(client, redis)).rejects.toBeInstanceOf(StorageError)
})

it('throws StorageError when final commit returns stashfilestorage (storage backend failure)', async () => {
const { client, redis } = makeChunkUploadClient()
let call = 0
// biome-ignore lint/suspicious/noExplicitAny: overriding private methods for testing
;(client as any).apiUploadChunk = mock(async () => {
if (++call === 1) return { upload: { filekey: 'stash-key', result: 'Continue' } }
return {
error: {
code: 'stashfilestorage',
info: 'An unknown error occurred in storage backend "local-swift-codfw".',
},
}
})
await expect(callUploadFile(client, redis)).rejects.toBeInstanceOf(StorageError)
})

it('throws StorageError when chunk upload returns backend-fail-internal (publish-time storage backend failure)', async () => {
const { client, redis } = makeChunkUploadClient()
// biome-ignore lint/suspicious/noExplicitAny: overriding private methods for testing
;(client as any).apiUploadChunk = mock(async () => ({
error: {
code: 'backend-fail-internal',
info: 'An unknown error occurred in storage backend "local-swift-codfw".',
},
}))
await expect(callUploadFile(client, redis)).rejects.toBeInstanceOf(StorageError)
})

it('throws StorageError when final commit returns backend-fail-internal (publish-time storage backend failure)', async () => {
const { client, redis } = makeChunkUploadClient()
let call = 0
// biome-ignore lint/suspicious/noExplicitAny: overriding private methods for testing
;(client as any).apiUploadChunk = mock(async () => {
if (++call === 1) return { upload: { filekey: 'stash-key', result: 'Continue' } }
return {
error: {
code: 'backend-fail-internal',
info: 'An unknown error occurred in storage backend "local-swift-codfw".',
},
}
})
await expect(callUploadFile(client, redis)).rejects.toBeInstanceOf(StorageError)
})

it('preserves the MediaWiki error code on the thrown StorageError (chunk phase)', async () => {
const { client, redis } = makeChunkUploadClient()
// biome-ignore lint/suspicious/noExplicitAny: overriding private methods for testing
;(client as any).apiUploadChunk = mock(async () => ({
error: {
code: 'stashfilestorage',
info: 'An unknown error occurred in storage backend "local-swift-codfw".',
},
}))
const caught = await callUploadFile(client, redis).catch((e) => e)
expect(caught.code).toBe('stashfilestorage')
})

it('preserves the MediaWiki error code on the thrown error for an unmapped chunk error code', async () => {
const { client, redis } = makeChunkUploadClient()
// biome-ignore lint/suspicious/noExplicitAny: overriding private methods for testing
;(client as any).apiUploadChunk = mock(async () => ({
error: { code: 'some-unmapped-error', info: 'Something else went wrong.' },
}))
const caught = await callUploadFile(client, redis).catch((e) => e)
expect(caught).not.toBeInstanceOf(StorageError)
expect(caught.code).toBe('some-unmapped-error')
})

it('preserves the MediaWiki error code on the thrown error for an unmapped commit error code', async () => {
const { client, redis } = makeChunkUploadClient()
let call = 0
// biome-ignore lint/suspicious/noExplicitAny: overriding private methods for testing
;(client as any).apiUploadChunk = mock(async () => {
if (++call === 1) return { upload: { filekey: 'stash-key', result: 'Continue' } }
return { error: { code: 'some-unmapped-error', info: 'Something else went wrong.' } }
})
const caught = await callUploadFile(client, redis).catch((e) => e)
expect(caught).not.toBeInstanceOf(StorageError)
expect(caught.code).toBe('some-unmapped-error')
})

it('throws MediaWikiServerError when the chunk upload endpoint returns 5xx', async () => {
const client = new MediaWikiClient(['key', 'secret'])
// biome-ignore lint/suspicious/noExplicitAny: overriding private methods for testing
Expand Down
24 changes: 18 additions & 6 deletions backend/src/mediawiki/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,10 +286,16 @@ export class MediaWikiClient {
if (errorObj) {
if (
errorObj.code === 'uploadstash-exception' ||
errorObj.code === UPLOAD_CHUNK_FILE_EXCEPTION
errorObj.code === UPLOAD_CHUNK_FILE_EXCEPTION ||
errorObj.code === 'stashfilestorage' ||
errorObj.code === 'backend-fail-internal'
)
throw new StorageError(errorObj.info ?? 'Stash exception')
throw new Error(errorObj.info ?? 'Upload chunk failed')
throw Object.assign(new StorageError(errorObj.info ?? 'Stash exception'), {
code: errorObj.code,
})
throw Object.assign(new Error(errorObj.info ?? 'Upload chunk failed'), {
code: errorObj.code,
})
}
const upload = result.upload as Record<string, unknown>
stashKey = upload.filekey as string
Expand Down Expand Up @@ -330,10 +336,16 @@ export class MediaWikiClient {
}
if (
errorObj.code === 'uploadstash-exception' ||
errorObj.code === UPLOAD_CHUNK_FILE_EXCEPTION
errorObj.code === UPLOAD_CHUNK_FILE_EXCEPTION ||
errorObj.code === 'stashfilestorage' ||

@greptile-apps greptile-apps Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Commit Failure Becomes Requeue

When backend-fail-internal is returned during the final commit, this branch now sends the job through the full StorageError requeue path. Commit errors are ambiguous because the file page may already have been created; in that state the retry starts a fresh upload instead of treating the commit result as uncertain, which can waste work and may create an extra revision if duplicate detection does not catch the already-published file first.

@DaxServer DaxServer Jul 5, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uploadFile() re-checks for an existing file by SHA1 content hash (findDuplicates(), near the top of the method) on every invocation, before any stash/commit activity. The worker's StorageError requeue path (upload.worker.ts:288, enqueueUploadFn) creates a fresh job that re-runs uploadFile() from scratch, including that duplicate check.

So if the commit actually succeeded server-side despite the client seeing backend-fail-internal, the retry hits DuplicateUploadError and updates SDC on the existing file instead of creating a new revision/duplicate — same safety net that already covers every other retryable path in this file (e.g. MediaWikiServerError for chunk-phase 5xx from #112). Not treating this as commit-phase-specific ambiguity since the duplicate check already handles it.

— Claude Sonnet 5

@greptile-apps greptile-apps Bot Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a solid safety net — the SHA1 duplicate check running unconditionally at the top of uploadFile() before any stash/commit work means the retry path is already idempotent. If the commit landed server-side, the retry detects it and routes to SDC update rather than re-uploading. That's the same guarantee you get for every other retried error in this file, so treating backend-fail-internal consistently with the rest makes sense.

My concern doesn't apply here. The existing duplicate check covers it.

errorObj.code === 'backend-fail-internal'
)
throw new StorageError(errorObj.info ?? 'Stash exception')
throw new Error(errorObj.info ?? 'Upload commit failed')
throw Object.assign(new StorageError(errorObj.info ?? 'Stash exception'), {
code: errorObj.code,
})
throw Object.assign(new Error(errorObj.info ?? 'Upload commit failed'), {
code: errorObj.code,
})
}
const upload = result.upload as Record<string, unknown>
if (upload.result === 'Success') {
Expand Down