From e707a0fd84044a7ef7c3be9948a9cbfbe0676c9f Mon Sep 17 00:00:00 2001
From: Christian Dupuis <cd@atomist.com>
Date: Wed, 22 Aug 2018 18:04:40 +0200
Subject: [PATCH] Cleanup clones in CachingProjectLoader

fixes #482
---
 .../project/CachingProjectLoader.ts           | 36 ++++++++++++++-----
 src/api-helper/project/support/LruCache.ts    |  8 +++--
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/src/api-helper/project/CachingProjectLoader.ts b/src/api-helper/project/CachingProjectLoader.ts
index e376da177..ebd3ad118 100644
--- a/src/api-helper/project/CachingProjectLoader.ts
+++ b/src/api-helper/project/CachingProjectLoader.ts
@@ -16,9 +16,13 @@
 
 import { logger } from "@atomist/automation-client";
 import { GitProject } from "@atomist/automation-client/project/git/GitProject";
-import * as fs from "fs";
+import * as fs from "fs-extra";
 import { promisify } from "util";
-import { ProjectLoader, ProjectLoadingParameters, WithLoadedProject } from "../../spi/project/ProjectLoader";
+import {
+    ProjectLoader,
+    ProjectLoadingParameters,
+    WithLoadedProject,
+} from "../../spi/project/ProjectLoader";
 import { CloningProjectLoader } from "./cloningProjectLoader";
 import { cacheKeyForSha } from "./support/cacheKey";
 import { LruCache } from "./support/LruCache";
@@ -33,12 +37,16 @@ export class CachingProjectLoader implements ProjectLoader {
 
     public async doWithProject<T>(params: ProjectLoadingParameters, action: WithLoadedProject<T>): Promise<T> {
         if (!params.readOnly) {
-            logger.info("CachingProjectLoader: Forcing fresh clone for non readonly use of %j", params.id);
+            logger.info("Forcing fresh clone for non readonly use of '%j'", params.id);
             const p = await save(this.delegate, params);
-            return action(p);
+            return action(p)
+                .then(result => {
+                    cleanUp(p);
+                    return result;
+                });
         }
 
-        logger.debug("CachingProjectLoader: Hoping to reuse clone for readonly use of %j", params.id);
+        logger.debug("Attempting to reuse clone for readonly use of '%j'", params.id);
         const key = cacheKeyForSha(params.id);
         let project = this.cache.get(key);
         if (!!project) {
@@ -47,27 +55,37 @@ export class CachingProjectLoader implements ProjectLoader {
                 await promisify(fs.access)(project.baseDir);
             } catch {
                 this.cache.evict(key);
-                logger.warn("CachingProjectLoader: Invalid cache entry %s", key);
+                logger.warn("Invalid cache entry '%s'", key);
                 project = undefined;
             }
         }
 
         if (!project) {
             project = await save(this.delegate, params);
-            logger.info("Caching project %j", project.id);
+            logger.info("Caching project '%j'", project.id);
             this.cache.put(key, project);
         }
 
-        logger.debug("CachingProjectLoader: About to invoke action. Cache stats: %j", this.cache.stats);
+        logger.debug("About to invoke action. Cache stats: %j", this.cache.stats);
         return action(project);
     }
 
     constructor(
         private readonly delegate: ProjectLoader = CloningProjectLoader,
         maxEntries: number = 20) {
-        this.cache = new LruCache<GitProject>(maxEntries);
+        this.cache = new LruCache<GitProject>(maxEntries, cleanUp);
     }
+}
 
+function cleanUp(p: GitProject): void {
+    logger.debug(`Evicting project '%j'`, p.id);
+    if (p.baseDir && fs.accessSync(p.baseDir)) {
+        try {
+            fs.removeSync(p.baseDir);
+        } catch (err) {
+            logger.warn(err);
+        }
+    }
 }
 
 export function save(pl: ProjectLoader, params: ProjectLoadingParameters): Promise<GitProject> {
diff --git a/src/api-helper/project/support/LruCache.ts b/src/api-helper/project/support/LruCache.ts
index 3aa0e419c..d5262865f 100644
--- a/src/api-helper/project/support/LruCache.ts
+++ b/src/api-helper/project/support/LruCache.ts
@@ -27,7 +27,8 @@ export class LruCache<T> implements SimpleCache<T> {
 
     private readonly values: Map<string, T> = new Map<string, T>();
 
-    constructor(private readonly maxEntries: number = 200) {
+    constructor(private readonly maxEntries: number = 200,
+                private readonly evictCallback: (t: T) => void = () => { /** intentionally left empty */}) {
     }
 
     get stats() {
@@ -42,7 +43,7 @@ export class LruCache<T> implements SimpleCache<T> {
             ++this.hits;
             // Peek the entry, re-insert for LRU strategy
             entry = this.values.get(key);
-            this.values.delete(key);
+            this.evict(key);
             this.values.set(key, entry);
         }
         return entry;
@@ -52,12 +53,13 @@ export class LruCache<T> implements SimpleCache<T> {
         if (this.values.size >= this.maxEntries) {
             // least-recently used cache eviction strategy
             const keyToDelete = this.values.keys().next().value;
-            this.values.delete(keyToDelete);
+            this.evict(keyToDelete);
         }
         this.values.set(key, value);
     }
 
     public evict(key: string) {
+        this.evictCallback(this.values.get(key));
         return this.values.delete(key);
     }