diff --git a/services/web/app/src/Features/Institutions/InstitutionsAPI.mjs b/services/web/app/src/Features/Institutions/InstitutionsAPI.mjs index c938783154..c213e92a10 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsAPI.mjs +++ b/services/web/app/src/Features/Institutions/InstitutionsAPI.mjs @@ -9,14 +9,19 @@ import { V1ConnectionError, InvalidInstitutionalEmailError, } from '../Errors/Errors.js' -import { fetchJson, fetchNothing } from '@overleaf/fetch-utils' +import { + fetchJson, + fetchNothing, + RequestFailedError, +} from '@overleaf/fetch-utils' import Modules from '../../infrastructure/Modules.mjs' function _makeRequestOptions(options) { + const timeout = options.timeout || settings.apis.v1.timeout const requestOptions = { method: options.method, basicAuth: { user: settings.apis.v1.user, password: settings.apis.v1.pass }, - signal: AbortSignal.timeout(settings.apis.v1.timeout), + signal: AbortSignal.timeout(timeout), } if (options.body) { @@ -27,6 +32,22 @@ function _makeRequestOptions(options) { } function _responseErrorHandling(options, error) { + if (!(error instanceof RequestFailedError)) { + // The error is not an HTTP failure status from v1, so there is no response + // to inspect: e.g. a client-side timeout/abort, DNS failure or connection + // error. Log the underlying error for diagnosis before wrapping it, since + // it otherwise gets swallowed by the generic V1ConnectionError message + // below. + logger.warn( + { err: error, path: options.path }, + `${options.defaultErrorMessage}: request to v1 failed without a response` + ) + throw new V1ConnectionError({ + message: 'error getting affiliations from v1', + info: { path: options.path }, + }).withCause(error) + } + const status = error.response.status if (status >= 500) { @@ -196,6 +217,10 @@ async function getUsersNeedingReconfirmationsLapsedProcessed() { path: '/api/v2/institutions/need_reconfirmation_lapsed_processed', defaultErrorMessage: 'Could not get users that need reconfirmations lapsed processed', + // The underlying v1 query can be slow and is bounded by a 55s Postgres + // statement_timeout; give it more room than the default client timeout + // so v1 has a chance to return a clean error before we give up on it. + timeout: 60_000, }) } diff --git a/services/web/test/unit/src/Institutions/InstitutionsAPI.test.mjs b/services/web/test/unit/src/Institutions/InstitutionsAPI.test.mjs index 2b585a2ea0..a51d503194 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsAPI.test.mjs +++ b/services/web/test/unit/src/Institutions/InstitutionsAPI.test.mjs @@ -3,6 +3,7 @@ import path from 'node:path' import sinon from 'sinon' import mongodb from 'mongodb-legacy' import Errors from '../../../../app/src/Features/Errors/Errors.js' +import { RequestFailedError } from '@overleaf/fetch-utils' const modulePath = path.join( import.meta.dirname, @@ -13,6 +14,11 @@ vi.mock('../../../../app/src/Features/Errors/Errors.js', () => vi.importActual('../../../../app/src/Features/Errors/Errors.js') ) +// build the error that fetch-utils throws for a failure status code +function requestFailedError(status, body = null) { + return new RequestFailedError('v1.url', { method: 'POST' }, { status }, body) +} + describe('InstitutionsAPI', function () { beforeEach(async function (ctx) { ctx.settings = { @@ -35,6 +41,7 @@ describe('InstitutionsAPI', function () { vi.doMock('@overleaf/fetch-utils', () => ({ fetchNothing: ctx.fetchNothing, fetchJson: (ctx.fetchJson = sinon.stub()), + RequestFailedError, })) vi.doMock( @@ -288,6 +295,22 @@ describe('InstitutionsAPI', function () { ctx.InstitutionsAPI.promises.getUsersNeedingReconfirmationsLapsedProcessed() ).to.be.rejected }) + + it('handle a client-side timeout/abort with no response', async function (ctx) { + const timeoutError = new Error('The operation was aborted') + timeoutError.name = 'TimeoutError' + ctx.fetchJson.throws(timeoutError) + + let error + try { + await ctx.InstitutionsAPI.promises.getUsersNeedingReconfirmationsLapsedProcessed() + } catch (err) { + error = err + } + + expect(error).to.be.instanceOf(Errors.V1ConnectionError) + expect(error.cause).to.equal(timeoutError) + }) }) describe('addAffiliation', function () { @@ -328,7 +351,7 @@ describe('InstitutionsAPI', function () { it('handles 422 error', async function (ctx) { const messageFromApi = 'affiliation error message' const body = JSON.stringify({ errors: messageFromApi }) - ctx.fetchNothing.throws({ response: { status: 422 }, body }) + ctx.fetchNothing.throws(requestFailedError(422, body)) let error try { @@ -347,7 +370,7 @@ describe('InstitutionsAPI', function () { it('handles 500 error', async function (ctx) { const body = { errors: 'affiliation error message' } - ctx.fetchNothing.throws({ response: { status: 500 }, body }) + ctx.fetchNothing.throws(requestFailedError(500, body)) let error try { @@ -369,7 +392,7 @@ describe('InstitutionsAPI', function () { }) it('uses default error message when no error body in response', async function (ctx) { - ctx.fetchNothing.throws({ response: { status: 429 } }) + ctx.fetchNothing.throws(requestFailedError(429)) let error try { @@ -406,7 +429,7 @@ describe('InstitutionsAPI', function () { describe('removeAffiliation', function () { beforeEach(function (ctx) { - ctx.fetchNothing.throws({ response: { status: 404 } }) + ctx.fetchNothing.throws(requestFailedError(404)) }) it('remove affiliation', async function (ctx) { @@ -423,7 +446,7 @@ describe('InstitutionsAPI', function () { }) it('handle error', async function (ctx) { - ctx.fetchNothing.throws({ response: { status: 500 } }) + ctx.fetchNothing.throws(requestFailedError(500)) let error try {