Merge pull request #36700 from overleaf/tm-institutions-api-timeout-abort-handling

Handle client-side v1 timeouts gracefully in InstitutionsAPI, bump reconfirmation-lapsed timeout to 60s

GitOrigin-RevId: ecb2b61e257fdd41cadb353da3af98fec0cdf617

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,
   })
 }
 

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 {