From 00c532e1d9242a874d2d1ff94b7424472ff420dc Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Wed, 6 Dec 2023 19:02:59 +0800 Subject: [PATCH] fix(rollback handler): convert to ts for safety (#1044) * fix(rollback handler): convert to ts for safety * Untitled commit * Untitled commit * Untitled commit * Untitled commit * fix(retry): add max 5 retries) * feat(loggin): add logging for possibility of alarm * feat(style): use resultAsync for clarity + refactor * fix(privatisation): should be write handler --- src/middleware/routeHandler.js | 202 ------------- src/middleware/routeHandler.ts | 284 ++++++++++++++++++ src/routes/v2/authenticatedSites/contactUs.js | 4 +- .../v2/authenticatedSites/navigation.js | 4 +- src/routes/v2/authenticatedSites/settings.js | 2 +- src/services/db/GitFileSystemService.ts | 27 +- .../db/__tests__/GitFileSystemService.spec.ts | 3 + src/utils/neverthrow.ts | 15 + 8 files changed, 332 insertions(+), 209 deletions(-) delete mode 100644 src/middleware/routeHandler.js create mode 100644 src/middleware/routeHandler.ts create mode 100644 src/utils/neverthrow.ts diff --git a/src/middleware/routeHandler.js b/src/middleware/routeHandler.js deleted file mode 100644 index 147c75534..000000000 --- a/src/middleware/routeHandler.js +++ /dev/null @@ -1,202 +0,0 @@ -const { backOff } = require("exponential-backoff") -const SimpleGit = require("simple-git") - -const { config } = require("@config/config") - -const logger = require("@logger/logger").default - -const { default: GithubSessionData } = require("@classes/GithubSessionData") - -const { lock, unlock } = require("@utils/mutex-utils") -const { getCommitAndTreeSha, revertCommit } = require("@utils/utils.js") - -const { MAX_CONCURRENT_GIT_PROCESSES } = require("@constants/constants") - -const { FEATURE_FLAGS } = require("@root/constants/featureFlags") -const GitFileSystemError = require("@root/errors/GitFileSystemError").default -const LockedError = require("@root/errors/LockedError").default -const { - default: GitFileSystemService, -} = require("@services/db/GitFileSystemService") - -const BRANCH_REF = config.get("github.branchRef") - -const gitFileSystemService = new GitFileSystemService( - new SimpleGit({ maxConcurrentProcesses: MAX_CONCURRENT_GIT_PROCESSES }) -) - -const handleGitFileLock = async (repoName, next) => { - const result = await gitFileSystemService.hasGitFileLock(repoName) - if (result.isErr()) { - next(result.err) - return false - } - const isGitLocked = result.value - if (isGitLocked) { - logger.error(`Failed to lock repo ${repoName}: git file system in use.`) - next( - new LockedError( - `Someone else is currently modifying repo ${repoName}. Please try again later.` - ) - ) - return false - } - return true -} - -// Used when there are no write API calls to the repo on GitHub -const attachReadRouteHandlerWrapper = (routeHandler) => async ( - req, - res, - next -) => { - routeHandler(req, res).catch((err) => { - next(err) - }) -} - -// Used when there are write API calls to the repo on GitHub -const attachWriteRouteHandlerWrapper = (routeHandler) => async ( - req, - res, - next -) => { - const { siteName } = req.params - const { growthbook } = req - - let isGitAvailable = true - - // only check git file lock if the repo is ggs enabled - if (growthbook?.getFeatureValue(FEATURE_FLAGS.IS_GGS_ENABLED, false)) { - isGitAvailable = await handleGitFileLock(siteName, next) - } - - if (!isGitAvailable) return - - try { - await lock(siteName) - } catch (err) { - next(err) - return - } - - await routeHandler(req, res, next).catch(async (err) => { - await unlock(siteName) - next(err) - }) - - try { - await unlock(siteName) - } catch (err) { - next(err) - } -} - -const attachRollbackRouteHandlerWrapper = (routeHandler) => async ( - req, - res, - next -) => { - const { userSessionData } = res.locals - const { siteName } = req.params - - const { accessToken } = userSessionData - const { growthbook } = req - - const shouldUseGitFileSystem = !!growthbook?.getFeatureValue( - FEATURE_FLAGS.IS_GGS_ENABLED, - false - ) - - const isGitAvailable = await handleGitFileLock(siteName, next) - if (!isGitAvailable) return - try { - await lock(siteName) - } catch (err) { - next(err) - return - } - - let originalCommitSha - if (shouldUseGitFileSystem) { - const result = await gitFileSystemService.getLatestCommitOfBranch( - siteName, - BRANCH_REF - ) - if (result.isErr()) { - await unlock(siteName) - next(result.err) - return - } - originalCommitSha = result.value.sha - if (!originalCommitSha) { - await unlock(siteName) - next(result.err) - return - } - // Unused for git file system, but to maintain existing structure - res.locals.githubSessionData = new GithubSessionData({ - currentCommitSha: "", - treeSha: "", - }) - } else { - try { - const { currentCommitSha, treeSha } = await getCommitAndTreeSha( - siteName, - accessToken - ) - - const githubSessionData = new GithubSessionData({ - currentCommitSha, - treeSha, - }) - res.locals.githubSessionData = githubSessionData - - originalCommitSha = currentCommitSha - } catch (err) { - await unlock(siteName) - next(err) - return - } - } - await routeHandler(req, res, next).catch(async (err) => { - try { - if (shouldUseGitFileSystem) { - await backOff(() => { - const rollbackRes = gitFileSystemService - .rollback(siteName, originalCommitSha) - .unwrapOr(false) - if (!rollbackRes) throw new GitFileSystemError("Rollback failure") - }) - await backOff(() => { - const pushRes = gitFileSystemService - .push(siteName, true) - .unwrapOr(false) - if (!pushRes) throw new GitFileSystemError("Push failure") - }) - } else { - await backOff(() => { - revertCommit(originalCommitSha, siteName, accessToken) - }) - } - } catch (retryErr) { - await unlock(siteName) - next(retryErr) - return - } - await unlock(siteName) - next(err) - }) - - try { - await unlock(siteName) - } catch (err) { - next(err) - } -} - -module.exports = { - attachReadRouteHandlerWrapper, - attachWriteRouteHandlerWrapper, - attachRollbackRouteHandlerWrapper, -} diff --git a/src/middleware/routeHandler.ts b/src/middleware/routeHandler.ts new file mode 100644 index 000000000..46d9352a0 --- /dev/null +++ b/src/middleware/routeHandler.ts @@ -0,0 +1,284 @@ +import { GrowthBook } from "@growthbook/growthbook" +import { BackoffOptions, backOff } from "exponential-backoff" +import { ResultAsync } from "neverthrow" +import simpleGit from "simple-git" + +import { config } from "@config/config" + +import GithubSessionData from "@classes/GithubSessionData" + +import { lock, unlock } from "@utils/mutex-utils" +import { getCommitAndTreeSha, revertCommit } from "@utils/utils.js" + +import { + MAX_CONCURRENT_GIT_PROCESSES, + STAGING_BRANCH, + STAGING_LITE_BRANCH, +} from "@constants/constants" + +import { FEATURE_FLAGS } from "@root/constants/featureFlags" +import LockedError from "@root/errors/LockedError" +import logger from "@root/logger/logger" +import { FeatureFlags } from "@root/types/featureFlags" +import convertNeverThrowToPromise from "@root/utils/neverthrow" +import GitFileSystemService from "@services/db/GitFileSystemService" + +const BRANCH_REF = config.get("github.branchRef") + +const backoffOptions: BackoffOptions = { + numOfAttempts: 5, +} +const simpleGitInstance = simpleGit({ + maxConcurrentProcesses: MAX_CONCURRENT_GIT_PROCESSES, +}) +const gitFileSystemService = new GitFileSystemService(simpleGitInstance) + +const handleGitFileLock = async ( + repoName: string, + next: (arg0: any) => void +) => { + const result = await gitFileSystemService.hasGitFileLock(repoName, true) + if (result.isErr()) { + next(result.error) + return false + } + const isGitLocked = result.value + if (isGitLocked) { + logger.error(`Failed to lock repo ${repoName}: git file system in use.`) + next( + new LockedError( + `Someone else is currently modifying repo ${repoName}. Please try again later.` + ) + ) + return false + } + return true +} + +const isGitFileAndIsGitAvail = async ( + growthbook: GrowthBook, + siteName: string, + next: any +) => { + let isGitAvailable = true + + // only check git file lock if the repo is ggs enabled + if (growthbook?.getFeatureValue(FEATURE_FLAGS.IS_GGS_ENABLED, false)) { + isGitAvailable = await handleGitFileLock(siteName, next) + } + return isGitAvailable +} + +// Used when there are no write API calls to the repo on GitHub +export const attachReadRouteHandlerWrapper = (routeHandler: any) => async ( + req: any, + res: any, + next: any +) => { + routeHandler(req, res).catch((err: any) => { + next(err) + }) +} + +// Used when there are write API calls to the repo on GitHub +export const attachWriteRouteHandlerWrapper = (routeHandler: any) => async ( + req: any, + res: any, + next: any +) => { + const { siteName } = req.params + const { growthbook } = req + + let isGitAvailable = true + + // only check git file lock if the repo is ggs enabled + if (growthbook?.getFeatureValue(FEATURE_FLAGS.IS_GGS_ENABLED, false)) { + isGitAvailable = await handleGitFileLock(siteName, next) + } + + if (!isGitAvailable) return + + try { + await lock(siteName) + } catch (err) { + next(err) + return + } + + await routeHandler(req, res, next).catch(async (err: any) => { + await unlock(siteName) + next(err) + }) + + try { + await unlock(siteName) + } catch (err) { + next(err) + } +} + +export const attachRollbackRouteHandlerWrapper = (routeHandler: any) => async ( + req: any, + res: any, + next: any +) => { + const { userSessionData } = res.locals + const { siteName } = req.params + + const { accessToken } = userSessionData + const { growthbook } = req + + const isGitAvailable = await isGitFileAndIsGitAvail( + growthbook, + siteName, + next + ) + + if (!isGitAvailable) return + try { + await lock(siteName) + } catch (err) { + next(err) + return + } + + let originalStagingCommitSha: string + let originalStagingLiteCommitSha: string + + const shouldUseGitFileSystem = growthbook?.getFeatureValue( + FEATURE_FLAGS.IS_GGS_ENABLED, + false + ) + if (shouldUseGitFileSystem) { + const results = await ResultAsync.combine([ + gitFileSystemService.getLatestCommitOfBranch(siteName, STAGING_BRANCH), + gitFileSystemService.getLatestCommitOfBranch( + siteName, + STAGING_LITE_BRANCH + ), + ]) + + if (results.isErr()) { + await unlock(siteName) + next(results.error) + return + } + const [stagingResult, stagingLiteResult] = results.value + if (!stagingResult.sha || !stagingLiteResult.sha) { + await unlock(siteName) + return + } + + originalStagingCommitSha = stagingResult.sha + originalStagingLiteCommitSha = stagingLiteResult.sha + // Unused for git file system, but to maintain existing structure + res.locals.githubSessionData = new GithubSessionData({ + currentCommitSha: "", + treeSha: "", + }) + } else { + try { + const { + currentCommitSha: currentStgCommitSha, + treeSha: stgTreeSha, + } = await getCommitAndTreeSha(siteName, accessToken, STAGING_BRANCH) + + const { + currentCommitSha: currentStgLiteCommitSha, + } = await getCommitAndTreeSha(siteName, accessToken, STAGING_LITE_BRANCH) + + const githubSessionData = new GithubSessionData({ + currentCommitSha: currentStgCommitSha, + treeSha: stgTreeSha, + }) + res.locals.githubSessionData = githubSessionData + + originalStagingCommitSha = currentStgCommitSha + originalStagingLiteCommitSha = currentStgLiteCommitSha + } catch (err) { + await unlock(siteName) + logger.error(`Failed to rollback repo ${siteName}: ${err}`) + next(err) + return + } + } + + await routeHandler(req, res, next).catch(async (err: any) => { + try { + if (shouldUseGitFileSystem) { + await backOff( + () => + convertNeverThrowToPromise( + gitFileSystemService.rollback( + siteName, + originalStagingCommitSha, + STAGING_BRANCH + ) + ), + backoffOptions + ) + + await backOff( + () => + convertNeverThrowToPromise( + gitFileSystemService.rollback( + siteName, + originalStagingLiteCommitSha, + STAGING_LITE_BRANCH + ) + ), + backoffOptions + ) + + await backOff(() => { + let pushRes = gitFileSystemService.push( + siteName, + STAGING_BRANCH, + true + ) + if (originalStagingLiteCommitSha) { + pushRes = pushRes.andThen(() => + gitFileSystemService.push(siteName, STAGING_LITE_BRANCH, true) + ) + } + + return convertNeverThrowToPromise(pushRes) + }, backoffOptions) + } else { + await backOff( + () => + revertCommit( + originalStagingCommitSha, + siteName, + accessToken, + STAGING_BRANCH + ), + backoffOptions + ) + await backOff( + () => + revertCommit( + originalStagingLiteCommitSha, + siteName, + accessToken, + STAGING_LITE_BRANCH + ), + backoffOptions + ) + } + } catch (retryErr) { + await unlock(siteName) + logger.error(`Failed to rollback repo ${siteName}: ${retryErr}`) + next(retryErr) + return + } + await unlock(siteName) + next(err) + }) + + try { + await unlock(siteName) + } catch (err) { + next(err) + } +} diff --git a/src/routes/v2/authenticatedSites/contactUs.js b/src/routes/v2/authenticatedSites/contactUs.js index b146aa1f9..2da7c57dc 100644 --- a/src/routes/v2/authenticatedSites/contactUs.js +++ b/src/routes/v2/authenticatedSites/contactUs.js @@ -6,7 +6,7 @@ const { BadRequestError } = require("@errors/BadRequestError") const { attachReadRouteHandlerWrapper, - attachWriteRouteHandlerWrapper, + attachRollbackRouteHandlerWrapper, } = require("@middleware/routeHandler") const { UpdateContactUsSchema } = require("@validators/RequestSchema") @@ -53,7 +53,7 @@ class ContactUsRouter { const router = express.Router({ mergeParams: true }) router.get("/", attachReadRouteHandlerWrapper(this.readContactUs)) - router.post("/", attachWriteRouteHandlerWrapper(this.updateContactUs)) + router.post("/", attachRollbackRouteHandlerWrapper(this.updateContactUs)) return router } diff --git a/src/routes/v2/authenticatedSites/navigation.js b/src/routes/v2/authenticatedSites/navigation.js index 291942a04..0092fa1b8 100644 --- a/src/routes/v2/authenticatedSites/navigation.js +++ b/src/routes/v2/authenticatedSites/navigation.js @@ -6,7 +6,7 @@ const { BadRequestError } = require("@errors/BadRequestError") const { attachReadRouteHandlerWrapper, - attachWriteRouteHandlerWrapper, + attachRollbackRouteHandlerWrapper, } = require("@middleware/routeHandler") const { UpdateNavigationRequestSchema } = require("@validators/RequestSchema") @@ -52,7 +52,7 @@ class NavigationRouter { const router = express.Router({ mergeParams: true }) router.get("/", attachReadRouteHandlerWrapper(this.readNavigation)) - router.post("/", attachWriteRouteHandlerWrapper(this.updateNavigation)) + router.post("/", attachRollbackRouteHandlerWrapper(this.updateNavigation)) return router } diff --git a/src/routes/v2/authenticatedSites/settings.js b/src/routes/v2/authenticatedSites/settings.js index 3e9f2a1f1..4005a3977 100644 --- a/src/routes/v2/authenticatedSites/settings.js +++ b/src/routes/v2/authenticatedSites/settings.js @@ -6,8 +6,8 @@ const { BadRequestError } = require("@errors/BadRequestError") // Import middleware const { - attachReadRouteHandlerWrapper, attachWriteRouteHandlerWrapper, + attachReadRouteHandlerWrapper, attachRollbackRouteHandlerWrapper, } = require("@middleware/routeHandler") diff --git a/src/services/db/GitFileSystemService.ts b/src/services/db/GitFileSystemService.ts index 5f4cbe402..348dd1ad2 100644 --- a/src/services/db/GitFileSystemService.ts +++ b/src/services/db/GitFileSystemService.ts @@ -542,10 +542,33 @@ export default class GitFileSystemService { isForce ? this.git .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) - .push(["--force"]) + .push([...gitOptions, "--force"]) + : this.git + .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) + .push(gitOptions), + (error) => { + logger.error(`Error when pushing ${repoName}: ${error}`) + + if (error instanceof GitError) { + return new GitFileSystemError( + "Unable to push latest changes of repo" + ) + } + + return new GitFileSystemError("An unknown error occurred") + } + ) + ) + .orElse(() => + // Retry push twice + ResultAsync.fromPromise( + isForce + ? this.git + .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) + .push([...gitOptions, "--force"]) : this.git .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) - .push(), + .push(gitOptions), (error) => { logger.error(`Error when pushing ${repoName}: ${error}`) diff --git a/src/services/db/__tests__/GitFileSystemService.spec.ts b/src/services/db/__tests__/GitFileSystemService.spec.ts index c38cbabe1..f7fa089bf 100644 --- a/src/services/db/__tests__/GitFileSystemService.spec.ts +++ b/src/services/db/__tests__/GitFileSystemService.spec.ts @@ -901,6 +901,9 @@ describe("GitFileSystemService", () => { MockSimpleGit.cwd.mockReturnValueOnce({ push: jest.fn().mockRejectedValueOnce(new GitError()), }) + MockSimpleGit.cwd.mockReturnValueOnce({ + push: jest.fn().mockRejectedValueOnce(new GitError()), + }) const result = await GitFileSystemService.push("fake-repo", BRANCH_REF) diff --git a/src/utils/neverthrow.ts b/src/utils/neverthrow.ts new file mode 100644 index 000000000..5f29b15c5 --- /dev/null +++ b/src/utils/neverthrow.ts @@ -0,0 +1,15 @@ +import { ResultAsync } from "neverthrow" + +/** + * This function is only used when integrating with third party libraries that + * expect a .catch() method on the returned promise. This should not be used in most + * control flows as it removes the benefits that neverthrow provides. + */ +const convertNeverThrowToPromise = async ( + x: ResultAsync +): Promise => { + const res = await x + return res._unsafeUnwrap() +} + +export default convertNeverThrowToPromise