fix: improve handling of repository clones in the .remote folder

src/proxy/actions/Action.ts

@@ -94,7 +94,8 @@ class Action {
   }
 
   /**
-   * Set the commit range for the action.
+   * Set the commit range for the action. Changes the action.id to be based on
+   * the commit details.
    * @param {string} commitFrom the starting commit
    * @param {string} commitTo the ending commit
    */

src/proxy/chain.ts

@@ -10,15 +10,13 @@ const pushActionChain: ((req: any, action: Action) => Promise<Action>)[] = [
   proc.push.checkCommitMessages,
   proc.push.checkAuthorEmails,
   proc.push.checkUserPushPermission,
-  proc.push.pullRemote,
+  proc.push.pullRemote, // cleanup is handled after chain execution if successful
   proc.push.writePack,
   proc.push.checkHiddenCommits,
   proc.push.checkIfWaitingAuth,
   proc.push.preReceive,
   proc.push.getDiff,
-  // run before clear remote
   proc.push.gitleaks,
-  proc.push.clearBareClone,
   proc.push.scanDiff,
   proc.push.blockForAuth,
 ];
@@ -35,6 +33,7 @@ let pluginsInserted = false;
 
 export const executeChain = async (req: any, res: any): Promise<Action> => {
   let action: Action = {} as Action;
+  let checkoutCleanUpRequired = false;
 
   try {
     action = await proc.pre.parseAction(req);
@@ -44,18 +43,28 @@ export const executeChain = async (req: any, res: any): Promise<Action> => {
       action = await fn(req, action);
       if (!action.continue() || action.allowPush) {
         break;
+      } else if (fn === proc.push.pullRemote) {
+        //if the pull was successful then record the fact we need to clean it up again
+        // pullRemote should cleanup unsuccessful clones itself
+        checkoutCleanUpRequired = true;
       }
     }
   } catch (e) {
     action.error = true;
     action.errorMessage = `An error occurred when executing the chain: ${e}`;
     console.error(action.errorMessage);
   } finally {
-    await proc.push.audit(req, action);
+    //clean up the clone created
+    if (checkoutCleanUpRequired) {
+      action = await proc.post.clearBareClone(req, action);
+    }
+
+    action = await proc.post.audit(req, action);
+
     if (action.autoApproved) {
-      attemptAutoApproval(action);
+      await attemptAutoApproval(action);
     } else if (action.autoRejected) {
-      attemptAutoRejection(action);
+      await attemptAutoRejection(action);
     }
   }
 

src/proxy/index.ts

@@ -43,6 +43,16 @@ export class Proxy {
   constructor() {}
 
   private async proxyPreparations() {
+    // Clean-up the .remote folder in case anything was in-progress when we shut down
+    const remoteDir = './.remote';
+    if (fs.existsSync(remoteDir)) {
+      console.log('Cleaning up the existing .remote dir...');
+      // Recursively remove the contents of ./.remote and ignore exceptions
+      fs.rmSync(remoteDir, { recursive: true, force: true, retryDelay: 100 });
+    }
+    console.log('Creating the .remote dir...');
+    fs.mkdirSync(remoteDir);
+
     const plugins = getPlugins();
     const pluginLoader = new PluginLoader(plugins);
     await pluginLoader.load();

src/proxy/processors/index.ts

@@ -1,4 +1,5 @@
 import * as pre from './pre-processor';
 import * as push from './push-action';
+import * as post from './post-processor';
 
-export { pre, push };
+export { pre, push, post };

src/proxy/processors/post-processor/clearBareClone.ts

@@ -0,0 +1,23 @@
+import { Action, Step } from '../../actions';
+import fs from 'fs';
+
+const exec = async (req: any, action: Action): Promise<Action> => {
+  const step = new Step('clearBareClone');
+
+  // Recursively remove the contents of ./.remote and ignore exceptions
+  if (action.proxyGitPath) {
+    fs.rmSync(action.proxyGitPath, { recursive: true, force: true });
+    step.log(`.remote is deleted!`);
+  } else {
+    // This action should not be called unless a clone was made successfully as pullRemote cleans up after itself on failures
+    // Log an error as we couldn't delete the clone
+    step.setError(`action.proxyGitPath was not set and cannot be removed`);
+  }
+  action.addStep(step);
+
+  return action;
+};
+
+exec.displayName = 'clearBareClone.exec';
+
+export { exec };

src/proxy/processors/post-processor/index.ts

@@ -0,0 +1,4 @@
+import { exec as audit } from '../post-processor/audit';
+import { exec as clearBareClone } from '../post-processor/clearBareClone';
+
+export { audit, clearBareClone };

src/proxy/processors/push-action/index.ts

@@ -1,7 +1,7 @@
 import { exec as parsePush } from './parsePush';
 import { exec as preReceive } from './preReceive';
 import { exec as checkRepoInAuthorisedList } from './checkRepoInAuthorisedList';
-import { exec as audit } from './audit';
+
 import { exec as pullRemote } from './pullRemote';
 import { exec as writePack } from './writePack';
 import { exec as getDiff } from './getDiff';
@@ -13,14 +13,13 @@ import { exec as checkIfWaitingAuth } from './checkIfWaitingAuth';
 import { exec as checkCommitMessages } from './checkCommitMessages';
 import { exec as checkAuthorEmails } from './checkAuthorEmails';
 import { exec as checkUserPushPermission } from './checkUserPushPermission';
-import { exec as clearBareClone } from './clearBareClone';
+
 import { exec as checkEmptyBranch } from './checkEmptyBranch';
 
 export {
   parsePush,
   preReceive,
   checkRepoInAuthorisedList,
-  audit,
   pullRemote,
   writePack,
   getDiff,
@@ -32,6 +31,5 @@ export {
   checkCommitMessages,
   checkAuthorEmails,
   checkUserPushPermission,
-  clearBareClone,
   checkEmptyBranch,
 };

src/proxy/processors/push-action/parsePush.ts

@@ -63,6 +63,8 @@ async function exec(req: any, action: Action): Promise<Action> {
     // Strip everything after NUL, which is cap-list from
     // https://git-scm.com/docs/http-protocol#_smart_server_response
     action.branch = ref.replace(/\0.*/, '').trim();
+
+    // Note this will change the action.id to be based on the commits
     action.setCommit(oldCommit, newCommit);
 
     // Check if the offset is valid and if there's data after it

src/proxy/processors/push-action/pullRemote.ts

@@ -7,45 +7,54 @@ const dir = './.remote';
 
 const exec = async (req: any, action: Action): Promise<Action> => {
   const step = new Step('pullRemote');
+  action.proxyGitPath = `${dir}/${action.id}`;
 
-  try {
-    action.proxyGitPath = `${dir}/${action.id}`;
-
-    if (!fs.existsSync(dir)) {
-      fs.mkdirSync(dir);
-    }
-
-    if (!fs.existsSync(action.proxyGitPath)) {
+  //the specific checkout folder should not exist
+  // - fail out if it does to avoid concurrent processing of conflicting requests
+  if (fs.existsSync(action.proxyGitPath)) {
+    const errMsg =
+      'The checkout folder already exists - we may be processing a concurrent request for this push. If this issue persists the proxy may need to be restarted.';
+    // do not delete the folder so that the other request can complete if its going to
+    step.setError(errMsg);
+    action.addStep(step);
+    throw new Error(errMsg);
+  } else {
+    try {
       step.log(`Creating folder ${action.proxyGitPath}`);
       fs.mkdirSync(action.proxyGitPath, 0o755);
-    }
 
-    const cmd = `git clone ${action.url}`;
-    step.log(`Executing ${cmd}`);
-
-    const authHeader = req.headers?.authorization;
-    const [username, password] = Buffer.from(authHeader.split(' ')[1], 'base64')
-      .toString()
-      .split(':');
-
-    // Note: setting singleBranch to true will cause issues when pushing to
-    // a non-default branch as commits from those branches won't be fetched
-    await git.clone({
-      fs,
-      http: gitHttpClient,
-      url: action.url,
-      dir: `${action.proxyGitPath}/${action.repoName}`,
-      onAuth: () => ({ username, password }),
-      depth: 1,
-    });
-
-    step.log(`Completed ${cmd}`);
-    step.setContent(`Completed ${cmd}`);
-  } catch (e: any) {
-    step.setError(e.toString('utf-8'));
-    throw e;
-  } finally {
-    action.addStep(step);
+      const cmd = `git clone ${action.url}`;
+      step.log(`Executing ${cmd}`);
+
+      const authHeader = req.headers?.authorization;
+      const [username, password] = Buffer.from(authHeader.split(' ')[1], 'base64')
+        .toString()
+        .split(':');
+
+      // Note: setting singleBranch to true will cause issues when pushing to
+      // a non-default branch as commits from those branches won't be fetched
+      await git.clone({
+        fs,
+        http: gitHttpClient,
+        url: action.url,
+        dir: `${action.proxyGitPath}/${action.repoName}`,
+        onAuth: () => ({ username, password }),
+        depth: 1,
+      });
+
+      step.log(`Completed ${cmd}`);
+      step.setContent(`Completed ${cmd}`);
+    } catch (e: any) {
+      step.setError(e.toString('utf-8'));
+
+      //clean-up the check out folder so it doesn't block subsequent attempts
+      fs.rmSync(action.proxyGitPath, { recursive: true, force: true });
+      step.log(`.remote is deleted!`);
+
+      throw e;
+    } finally {
+      action.addStep(step);
+    }
   }
   return action;
 };