Fixed new survey import concurrency issue

server/job/jobCreator.js

@@ -22,6 +22,7 @@ import RecordsValidationJob from '@server/modules/record/service/recordsValidati
 import SelectedRecordsExportJob from '@server/modules/record/service/selectedRecordsExportJob'
 import SurveyCloneJob from '@server/modules/survey/service/clone/surveyCloneJob'
 import SurveyActivityLogClearJob from '@server/modules/survey/service/surveyActivityLogClearJob'
+import SurveyCreateJob from '@server/modules/survey/service/surveyCreateJob'
 import SurveyExportJob from '@server/modules/survey/service/surveyExport/surveyExportJob'
 import SurveyLabelsImportJob from '@server/modules/survey/service/surveyLabelsImportJob'
 import SurveyPublishJob from '@server/modules/survey/service/publish/surveyPublishJob'
@@ -55,6 +56,7 @@ const jobClasses = [
   SelectedRecordsExportJob,
   SurveyCloneJob,
   SurveyActivityLogClearJob,
+  SurveyCreateJob,
   SurveyExportJob,
   SurveyLabelsImportJob,
   SurveyPublishJob,

server/modules/survey/api/surveyApi.js

@@ -56,9 +56,8 @@ export const init = (app) => {
           res.json({ job })
           return
         }
-        const survey = await SurveyService.insertSurvey({ user, surveyInfo: surveyInfoTarget })
-
-        res.json({ survey })
+        const job = SurveyService.startCreateSurveyJob({ user, surveyInfo: surveyInfoTarget })
+        res.json({ job })
       } else {
         res.json({ validation })
       }

server/modules/survey/manager/surveyManager.js

@@ -112,84 +112,109 @@ export const insertSurvey = async (params, client = db) => {
     temporary = false,
   } = params
 
-  return client.tx(async (t) => {
-    // Insert survey into db
-    const surveyProps = { ...Survey.getProps(surveyInfoParam) }
-    if (temporary) {
-      surveyProps.temporary = true
-    }
-    const surveyInfo = await SurveyRepository.insertSurvey({ survey: surveyInfoParam, propsDraft: surveyProps }, t)
-    const survey = assocSurveyInfo(surveyInfo)
-    const surveyId = Survey.getIdSurveyInfo(surveyInfo)
+  // Insert survey row on its own (not wrapped in a held-open transaction): DBMigrator.migrateSurveySchema
+  // below opens its own separate db connections (CREATE SCHEMA + db-migrate), so if it ran inside an open
+  // transaction here, that transaction's connection would sit idle while a second connection is acquired
+  // from the same pool. Under concurrent survey creation this starves the pool (no connectionTimeoutMillis
+  // is configured) and can hang the whole server, since every other request also needs a pool connection.
+  const surveyProps = { ...Survey.getProps(surveyInfoParam) }
+  if (temporary) {
+    surveyProps.temporary = true
+  }
+  const surveyInfo = await SurveyRepository.insertSurvey({ survey: surveyInfoParam, propsDraft: surveyProps }, client)
+  const survey = assocSurveyInfo(surveyInfo)
+  const surveyId = Survey.getIdSurveyInfo(surveyInfo)
 
-    // Create survey data schema
+  try {
+    // Create survey data schema (runs outside of any transaction held by this function; see comment above)
     await DBMigrator.migrateSurveySchema(surveyId)
 
-    // Log survey create activity
-    await ActivityLogRepository.insert(user, surveyId, ActivityLog.type.surveyCreate, surveyInfo, system, t)
-
-    if (createRootEntityDef) {
-      // Insert root entity def
-      const rootEntityDef = NodeDef.newNodeDef(
-        null,
-        NodeDef.nodeDefType.entity,
-        [Survey.cycleOneKey], // Use first (and only) cycle
-        {
-          [NodeDef.propKeys.name]: 'root_entity',
-          [NodeDef.propKeys.multiple]: false,
-          [NodeDefLayout.keys.layout]: NodeDefLayout.newLayout(
-            Survey.cycleOneKey,
-            NodeDefLayout.renderType.form,
-            uuidv4()
-          ),
-        }
-      )
-      await NodeDefManager.insertNodeDef({ user, survey, nodeDef: rootEntityDef, system: true }, t)
-    }
+    return await client.tx(async (t) => {
+      // Log survey create activity
+      await ActivityLogRepository.insert(user, surveyId, ActivityLog.type.surveyCreate, surveyInfo, system, t)
+
+      if (createRootEntityDef) {
+        // Insert root entity def
+        const rootEntityDef = NodeDef.newNodeDef(
+          null,
+          NodeDef.nodeDefType.entity,
+          [Survey.cycleOneKey], // Use first (and only) cycle
+          {
+            [NodeDef.propKeys.name]: 'root_entity',
+            [NodeDef.propKeys.multiple]: false,
+            [NodeDefLayout.keys.layout]: NodeDefLayout.newLayout(
+              Survey.cycleOneKey,
+              NodeDefLayout.renderType.form,
+              uuidv4()
+            ),
+          }
+        )
+        await NodeDefManager.insertNodeDef({ user, survey, nodeDef: rootEntityDef, system: true }, t)
+      }
 
-    if (updateUserPrefs) {
-      const userUpdated = User.assocPrefSurveyCurrentAndCycle(surveyId, Survey.cycleOneKey)(user)
-      await UserRepository.updateUserPrefs(userUpdated, t)
-    }
+      if (updateUserPrefs) {
+        const userUpdated = User.assocPrefSurveyCurrentAndCycle(surveyId, Survey.cycleOneKey)(user)
+        await UserRepository.updateUserPrefs(userUpdated, t)
+      }
 
-    // Create default groups for this survey
-    surveyInfo.authGroups = await AuthGroupRepository.createSurveyGroups(surveyId, Survey.getDefaultAuthGroups(), t)
+      // Create default groups for this survey
+      surveyInfo.authGroups = await AuthGroupRepository.createSurveyGroups(surveyId, Survey.getDefaultAuthGroups(), t)
 
-    await _addUserToSurveyAdmins({ user, surveyInfo }, t)
+      await _addUserToSurveyAdmins({ user, surveyInfo }, t)
 
-    return assocSurveyInfo(surveyInfo)
-  })
+      return assocSurveyInfo(surveyInfo)
+    })
+  } catch (error) {
+    // Survey row (and possibly the schema) were already created outside of this failed step;
+    // clean them up so a failed creation doesn't leave an orphaned survey/schema behind.
+    Logger.error(`error creating survey ${surveyId}, cleaning up: ${error.stack || error}`)
+    await deleteSurvey(surveyId, { deleteUserPrefs: true }, client).catch((cleanupError) => {
+      Logger.error(`error cleaning up survey ${surveyId} after failed creation: ${cleanupError.stack || cleanupError}`)
+    })
+    throw error
+  }
 }
 
 export const importSurvey = async (params, client = db) => {
   const { user, surveyInfo: surveyInfoParam, authGroups = Survey.getDefaultAuthGroups(), backup } = params
 
-  return client.tx(async (t) => {
-    // Insert survey into db
-    let surveyInfo = await SurveyRepository.insertSurvey(
-      {
-        survey: surveyInfoParam,
-        props: backup ? Survey.getProps(surveyInfoParam) : {},
-        propsDraft: backup ? Survey.getPropsDraft(surveyInfoParam) : Survey.getProps(surveyInfoParam),
-      },
-      t
-    )
-    const surveyId = Survey.getIdSurveyInfo(surveyInfo)
+  // See insertSurvey above: migrateSurveySchema opens its own separate db connections, so it must not
+  // run inside a transaction held open by this function (same connection-pool starvation risk).
+  const surveyInfo = await SurveyRepository.insertSurvey(
+    {
+      survey: surveyInfoParam,
+      props: backup ? Survey.getProps(surveyInfoParam) : {},
+      propsDraft: backup ? Survey.getPropsDraft(surveyInfoParam) : Survey.getProps(surveyInfoParam),
+    },
+    client
+  )
+  const surveyId = Survey.getIdSurveyInfo(surveyInfo)
 
-    // Create survey data schema
+  try {
+    // Create survey data schema (runs outside of any transaction held by this function; see insertSurvey above)
     await DBMigrator.migrateSurveySchema(surveyId)
 
-    // Create default groups for this survey
-    surveyInfo = Survey.assocAuthGroups(await AuthGroupRepository.createSurveyGroups(surveyId, authGroups, t))(
-      surveyInfo
-    )
+    return await client.tx(async (t) => {
+      // Create default groups for this survey
+      let surveyInfoUpdated = Survey.assocAuthGroups(
+        await AuthGroupRepository.createSurveyGroups(surveyId, authGroups, t)
+      )(surveyInfo)
 
-    surveyInfo = await _fetchAndAssocAdditionalInfo({ surveyInfo }, t)
+      surveyInfoUpdated = await _fetchAndAssocAdditionalInfo({ surveyInfo: surveyInfoUpdated }, t)
 
-    await _addUserToSurveyAdmins({ user, surveyInfo }, t)
+      await _addUserToSurveyAdmins({ user, surveyInfo: surveyInfoUpdated }, t)
 
-    return assocSurveyInfo(surveyInfo)
-  })
+      return assocSurveyInfo(surveyInfoUpdated)
+    })
+  } catch (error) {
+    // Survey row (and possibly the schema) were already created outside of this failed step;
+    // clean them up so a failed import doesn't leave an orphaned survey/schema behind.
+    Logger.error(`error importing survey ${surveyId}, cleaning up: ${error.stack || error}`)
+    await deleteSurvey(surveyId, { deleteUserPrefs: true }, client).catch((cleanupError) => {
+      Logger.error(`error cleaning up survey ${surveyId} after failed import: ${cleanupError.stack || cleanupError}`)
+    })
+    throw error
+  }
 }
 
 // ====== READ

server/modules/survey/service/surveyCreateJob.js

@@ -0,0 +1,36 @@
+import Job from '@server/job/job'
+
+import * as Survey from '@core/survey/survey'
+
+import * as SurveyManager from '../manager/surveyManager'
+
+export default class SurveyCreateJob extends Job {
+  constructor(params) {
+    super(SurveyCreateJob.type, params)
+  }
+
+  async execute() {
+    const { user, surveyInfo, createRootEntityDef, updateUserPrefs, temporary } = this.context
+
+    // Insert survey out of this job's own transaction (this.tx): SurveyManager.insertSurvey creates the
+    // survey data schema, which uses its own separate db connections and must not run inside an open one.
+    const survey = await SurveyManager.insertSurvey({
+      user,
+      surveyInfo,
+      createRootEntityDef,
+      updateUserPrefs,
+      temporary,
+    })
+
+    const surveyId = Survey.getId(survey)
+
+    this.setContext({ survey, surveyId })
+  }
+
+  async beforeSuccess() {
+    const { surveyId } = this.context
+    this.setResult({ surveyId })
+  }
+}
+
+SurveyCreateJob.type = 'SurveyCreateJob'

server/modules/survey/service/surveyService.js

@@ -19,6 +19,7 @@ import * as FileUtils from '@server/utils/file/fileUtils'
 import * as SurveyManager from '../manager/surveyManager'
 import * as SurveyFileManager from '../manager/surveyFileManager'
 import SurveyCloneJob from './clone/surveyCloneJob'
+import SurveyCreateJob from './surveyCreateJob'
 import SurveyPublishJob from './publish/surveyPublishJob'
 import { SchemaSummaryExportJob } from './schemaSummary'
 import SurveyActivityLogClearJob from './surveyActivityLogClearJob'
@@ -113,6 +114,12 @@ export const cloneSurvey = ({ user, surveyId, surveyInfoTarget, cycle = null })
   return JobUtils.jobToJSON(job)
 }
 
+export const startCreateSurveyJob = ({ user, surveyInfo }) => {
+  const job = new SurveyCreateJob({ user, surveyInfo })
+  JobManager.enqueueJob(job)
+  return JobUtils.jobToJSON(job)
+}
+
 /**
  * Starts the node definitions AI translation job.
  * @param {object} params - Params.

test/e2e/tests/_surveyCreate/index.js

@@ -19,26 +19,20 @@ export const createSurvey = (surveyToAdd) => {
         testId: TestId.surveyCreate.surveyCloneFrom,
         label: `${cloneFrom} - ${cloneFromLabel}`,
       })
-
-      // press "Create survey" and wait for the job to complete
-      await page.click(getSelector(TestId.surveyCreate.submitBtn, 'button'))
-      await page.waitForSelector(getSelector(TestId.modal.modal))
-
-      // close the job dialog and wait fot the navigation to the survey dashboard
-      await Promise.all([
-        page.waitForNavigation(/* { url: `{BASE_URL}/app/home/landing/` } */),
-        page.click(TestId.modal.close),
-      ])
     } else {
       await FormUtils.fillInput(TestId.surveyCreate.surveyLabel, label)
-
-      // press "Create survey" and wait for the navigation to the survey dashboard
-      await Promise.all([
-        page.waitForNavigation(/* { url: `{BASE_URL}/app/home/landing/` } */),
-        page.click(getSelector(TestId.surveyCreate.submitBtn, 'button')),
-      ])
     }
 
+    // press "Create survey" and wait for the job to complete (survey creation runs as a job too, to bound concurrency)
+    await page.click(getSelector(TestId.surveyCreate.submitBtn, 'button'))
+    await page.waitForSelector(getSelector(TestId.modal.modal))
+
+    // close the job dialog and wait fot the navigation to the survey dashboard
+    await Promise.all([
+      page.waitForNavigation(/* { url: `{BASE_URL}/app/home/landing/` } */),
+      page.click(TestId.modal.close),
+    ])
+
     const surveyTitleSelector = getSelector(TestId.header.surveyTitle)
     await expect(await page.innerText(surveyTitleSelector)).toBe(`${label} [${name}]`)
   })

test/integration/tests/001surveyIntegrationtest.js

@@ -7,6 +7,10 @@ describe('Survey Test', () => {
 
   test('Create Survey', async () => SurveyIntegrationTest.createSurveyTest())
 
+  test('Create Surveys Concurrently', async () => SurveyIntegrationTest.createSurveysConcurrentlyTest())
+
+  test('Import Surveys Concurrently', async () => SurveyIntegrationTest.importSurveysConcurrentlyTest())
+
   // ==== CATEGORY
 
   test('Create Category', async () => CategoryTest.createCategoryTest())

test/integration/tests/_survey/surveyTest.js

@@ -24,3 +24,46 @@ export const createSurveyTest = async () => {
   expect(Survey.getDefaultLanguage(surveyInfo)).toEqual(expectedDefaultLanguage)
   expect(Survey.getDefaultLabel(surveyInfo)).toEqual(Survey.getDefaultLabel(surveyInfoTest))
 }
+
+// Regression test: SurveyManager.insertSurvey used to hold a db transaction open while
+// DBMigrator.migrateSurveySchema acquired another connection from the same pool; concurrent survey
+// creations could then exhaust the pool and hang the whole server (no connectionTimeoutMillis is set).
+export const createSurveysConcurrentlyTest = async () => {
+  const user = getContextUser()
+
+  const newSurveyInfo = () =>
+    Survey.newSurvey({
+      ownerUuid: User.getUuid(user),
+      name: `do_not_use__test_survey_concurrent_${uuidv4()}`,
+      label: 'DO NOT USE! Test Survey (concurrent)',
+      languages: ['en'],
+    })
+
+  const [surveyA, surveyB] = await Promise.all([
+    SurveyManager.insertSurvey({ user, surveyInfo: newSurveyInfo() }),
+    SurveyManager.insertSurvey({ user, surveyInfo: newSurveyInfo() }),
+  ])
+
+  expect(Survey.getId(surveyA)).not.toEqual(Survey.getId(surveyB))
+}
+
+// Regression test: SurveyManager.importSurvey (used when restoring an Arena backup file, and when
+// cloning a survey) had the same transaction-held-open-during-migration bug as insertSurvey above.
+export const importSurveysConcurrentlyTest = async () => {
+  const user = getContextUser()
+
+  const newSurveyInfo = () =>
+    Survey.newSurvey({
+      ownerUuid: User.getUuid(user),
+      name: `do_not_use__test_survey_import_concurrent_${uuidv4()}`,
+      label: 'DO NOT USE! Test Survey (import concurrent)',
+      languages: ['en'],
+    })
+
+  const [surveyA, surveyB] = await Promise.all([
+    SurveyManager.importSurvey({ user, surveyInfo: newSurveyInfo(), backup: true }),
+    SurveyManager.importSurvey({ user, surveyInfo: newSurveyInfo(), backup: true }),
+  ])
+
+  expect(Survey.getId(surveyA)).not.toEqual(Survey.getId(surveyB))
+}