[ws-gc] WS soft deletion improvements

components/gitpod-db/src/typeorm/workspace-db-impl.ts

@@ -36,8 +36,9 @@ import {
     PrebuildWithWorkspace,
     PrebuildWithWorkspaceAndInstances,
     PrebuiltUpdatableAndWorkspace,
-    WorkspaceAndOwner,
     WorkspaceDB,
+    WorkspaceOwnerAndContentDeletedTime,
+    WorkspaceOwnerAndDeletionEligibility,
     WorkspaceOwnerAndSoftDeleted,
     WorkspacePortsAuthData,
 } from "../workspace-db";
@@ -486,42 +487,42 @@ export class TypeORMWorkspaceDBImpl extends TransactionalDBImpl<WorkspaceDB> imp
         cutOffDate: Date = new Date(),
         limit: number = 100,
         type: WorkspaceType = "regular",
-    ): Promise<WorkspaceAndOwner[]> {
+    ): Promise<WorkspaceOwnerAndDeletionEligibility[]> {
         // we do not allow to run this with a future date
         if (cutOffDate > new Date()) {
             throw new Error("cutOffDate must not be in the future, was: " + cutOffDate.toISOString());
         }
         const workspaceRepo = await this.getWorkspaceRepo();
-        const dbResults = await workspaceRepo.query(
-            `
-                SELECT ws.id AS id,
-                       ws.ownerId AS ownerId
-                    FROM d_b_workspace AS ws
-                    WHERE ws.deleted = 0
-                        AND ws.type = ?
-                        AND ws.softDeleted IS NULL
-                        AND ws.softDeletedTime = ''
-                        AND ws.pinned = 0
-                        AND ws.deletionEligibilityTime != ''
-                        AND ws.deletionEligibilityTime < ?
-                    LIMIT ?;
-            `,
-            [type, cutOffDate.toISOString(), limit],
-        );
+        const qb = workspaceRepo
+            .createQueryBuilder("ws")
+            .select(["ws.id", "ws.ownerId", "ws.deletionEligibilityTime"])
+            .where("ws.deleted = :deleted", { deleted: 0 })
+            .andWhere("ws.type = :type", { type })
+            .andWhere("ws.softDeleted IS NULL")
+            .andWhere("ws.softDeletedTime = ''")
+            .andWhere("ws.pinned = :pinned", { pinned: 0 })
+            .andWhere("ws.deletionEligibilityTime != ''")
+            .andWhere("ws.deletionEligibilityTime < :cutOffDate", { cutOffDate: cutOffDate.toISOString() })
+            .limit(limit);
 
-        return dbResults as WorkspaceAndOwner[];
+        const results: WorkspaceOwnerAndDeletionEligibility[] = await qb.getMany();
+        return results.filter(async (ws) => {
+            // we don't want to delete workspaces that have a running instance
+            const latestInstance = await this.findRunningInstance(ws.id);
+            return latestInstance === undefined;
+        });
     }
 
     public async findWorkspacesForPurging(
         minContentDeletionTimeInDays: number,
         limit: number,
         now: Date,
-    ): Promise<WorkspaceAndOwner[]> {
+    ): Promise<WorkspaceOwnerAndContentDeletedTime[]> {
         const minPurgeTime = daysBefore(now.toISOString(), minContentDeletionTimeInDays);
         const repo = await this.getWorkspaceRepo();
         const qb = repo
             .createQueryBuilder("ws")
-            .select(["ws.id", "ws.ownerId"])
+            .select(["ws.id", "ws.ownerId", "ws.contentDeletedTime"])
             .where(`ws.contentDeletedTime != ''`)
             .andWhere(`ws.contentDeletedTime < :minPurgeTime`, { minPurgeTime })
             .andWhere(`ws.deleted = 0`)

components/gitpod-db/src/workspace-db.spec.db.ts

@@ -254,7 +254,7 @@ class WorkspaceDBSpec {
     public async testPrebuildGarbageCollection() {
         const { pbws } = await this.createPrebuild(20, 15);
 
-        // mimick the behavior of the Garbage Collector
+        // mimic the behavior of the Garbage Collector
         const gcWorkspaces = await this.db.findEligibleWorkspacesForSoftDeletion(new Date(), 10, "prebuild");
         expect(gcWorkspaces.length).to.equal(1);
 
@@ -768,6 +768,7 @@ class WorkspaceDBSpec {
             {
                 id: "1",
                 ownerId,
+                contentDeletedTime: d20180131,
             },
         ]);
     }

components/gitpod-db/src/workspace-db.ts

@@ -64,6 +64,8 @@ export interface PrebuildWithWorkspaceAndInstances {
 
 export type WorkspaceAndOwner = Pick<Workspace, "id" | "ownerId">;
 export type WorkspaceOwnerAndSoftDeleted = Pick<Workspace, "id" | "ownerId" | "softDeleted">;
+export type WorkspaceOwnerAndDeletionEligibility = Pick<Workspace, "id" | "ownerId" | "deletionEligibilityTime">;
+export type WorkspaceOwnerAndContentDeletedTime = Pick<Workspace, "id" | "ownerId" | "contentDeletedTime">;
 
 export const WorkspaceDB = Symbol("WorkspaceDB");
 export interface WorkspaceDB {
@@ -102,7 +104,7 @@ export interface WorkspaceDB {
         cutOffDate?: Date,
         limit?: number,
         type?: WorkspaceType,
-    ): Promise<WorkspaceAndOwner[]>;
+    ): Promise<WorkspaceOwnerAndDeletionEligibility[]>;
     findWorkspacesForContentDeletion(
         minSoftDeletedTimeInDays: number,
         limit: number,
@@ -111,7 +113,7 @@ export interface WorkspaceDB {
         minContentDeletionTimeInDays: number,
         limit: number,
         now: Date,
-    ): Promise<WorkspaceAndOwner[]>;
+    ): Promise<WorkspaceOwnerAndContentDeletedTime[]>;
     findAllWorkspaces(
         offset: number,
         limit: number,

components/server/src/config.ts

@@ -84,10 +84,10 @@ export interface WorkspaceGarbageCollection {
     /** The minimal age of a workspace before it is marked as 'softDeleted' (= hidden for the user) */
     minAgeDays: number;
 
-    /** The minimal age of a prebuild (incl. workspace) before it's content is deleted (+ marked as 'softDeleted') */
+    /** The minimal age of a prebuild (incl. workspace) before its content is deleted (+ marked as 'softDeleted') */
     minAgePrebuildDays: number;
 
-    /** The minimal number of days a workspace has to stay in 'softDeleted' before it's content is deleted */
+    /** The minimal number of days a workspace has to stay in 'softDeleted' before its content is deleted */
     contentRetentionPeriodDays: number;
 
     /** The maximum amount of workspaces whose content is deleted in one go */

components/server/src/jobs/workspace-gc.ts

@@ -29,7 +29,7 @@ import { StorageClient } from "../storage/storage-client";
  *    - find _any_ workspace "softDeleted" for long enough -> move to "contentDeleted"
  *    - find _any_ workspace "contentDeleted" for long enough -> move to "purged"
  *  - prebuilds are special in that:
- *    - the GC has a dedicated sub-task to move workspace of type "prebuid" from "to delete" (with a different threshold) -> to "contentDeleted" directly
+ *    - the GC has a dedicated sub-task to move workspace of type "prebuild" from "to delete" (with a different threshold) -> to "contentDeleted" directly
  *    - the "purging" takes care of all Prebuild-related sub-resources, too
  */
 @injectable()
@@ -55,6 +55,11 @@ export class WorkspaceGarbageCollector implements Job {
             return;
         }
 
+        log.info("workspace-gc: job started", {
+            workspaceMinAgeDays: this.config.workspaceGarbageCollection.minAgeDays,
+            prebuildMinAgeDays: this.config.workspaceGarbageCollection.minAgePrebuildDays,
+        });
+
         // Move eligible "regular" workspace -> softDeleted
         try {
             await this.softDeleteEligibleWorkspaces();
@@ -115,6 +120,10 @@ export class WorkspaceGarbageCollector implements Job {
                         err,
                     );
                 }
+
+                log.info({ workspaceId: ws.id }, `workspace-gc: soft deleted a workspace`, {
+                    deletionEligibilityTime: ws.deletionEligibilityTime,
+                });
             }
             const afterDelete = new Date();
 
@@ -187,6 +196,9 @@ export class WorkspaceGarbageCollector implements Job {
                 } catch (err) {
                     log.error({ workspaceId: ws.id }, "workspace-gc: failed to purge workspace", err);
                 }
+                log.info({ workspaceId: ws.id }, `workspace-gc: hard deleted a workspace`, {
+                    contentDeletedTime: ws.contentDeletedTime,
+                });
             }
             const afterDelete = new Date();
 

components/server/src/workspace/workspace-service.spec.db.ts

@@ -701,7 +701,7 @@ describe("WorkspaceService", async () => {
             workspaceImage: "",
         });
 
-        await svc.updateDeletionEligabilityTime(owner.id, ws.id);
+        await svc.updateDeletionEligibilityTime(owner.id, ws.id);
 
         const workspace = await svc.getWorkspace(owner.id, ws.id);
         expect(workspace).to.not.be.undefined;
@@ -735,7 +735,7 @@ describe("WorkspaceService", async () => {
             workspaceImage: "",
         });
 
-        await svc.updateDeletionEligabilityTime(owner.id, ws.id);
+        await svc.updateDeletionEligibilityTime(owner.id, ws.id);
 
         const workspace = await svc.getWorkspace(owner.id, ws.id);
         expect(workspace).to.not.be.undefined;
@@ -771,7 +771,7 @@ describe("WorkspaceService", async () => {
             workspaceImage: "",
         });
 
-        await svc.updateDeletionEligabilityTime(owner.id, ws.id);
+        await svc.updateDeletionEligibilityTime(owner.id, ws.id);
 
         const workspace = await svc.getWorkspace(owner.id, ws.id);
         expect(workspace).to.not.be.undefined;

components/server/src/workspace/workspace-service.ts

@@ -223,8 +223,8 @@ export class WorkspaceService {
             );
             throw err;
         }
-        this.asyncUpdateDeletionEligabilityTime(user.id, workspace.id);
-        this.asyncUpdateDeletionEligabilityTimeForUsedPrebuild(user.id, workspace);
+        this.asyncUpdateDeletionEligibilityTime(user.id, workspace.id, true);
+        this.asyncUpdateDeletionEligibilityTimeForUsedPrebuild(user.id, workspace);
         if (project && workspace.type === "regular") {
             this.asyncHandleUpdatePrebuildTriggerStrategy({ ctx, project, workspace, user });
             this.asyncStartPrebuild({ ctx, project, workspace, user });
@@ -248,7 +248,7 @@ export class WorkspaceService {
         const res = await this.db.find({
             limit: 20,
             ...options,
-            userId, // gpl: We probably want to removed this limitation in the future, butkeeping the old behavior for now due to focus on FGA
+            userId, // gpl: We probably want to removed this limitation in the future, but keeping the old behavior for now due to focus on FGA
             includeHeadless: false,
         });
 
@@ -375,7 +375,7 @@ export class WorkspaceService {
             return;
         }
         await this.workspaceStarter.stopWorkspaceInstance({}, instance.id, instance.region, reason, policy);
-        this.asyncUpdateDeletionEligabilityTime(userId, workspaceId);
+        this.asyncUpdateDeletionEligibilityTime(userId, workspaceId, true);
     }
 
     public async stopRunningWorkspacesForUser(
@@ -396,21 +396,21 @@ export class WorkspaceService {
                     reason,
                     policy,
                 );
-                this.asyncUpdateDeletionEligabilityTime(userId, info.workspace.id);
+                this.asyncUpdateDeletionEligibilityTime(userId, info.workspace.id, false);
             }),
         );
         return infos.map((instance) => instance.workspace);
     }
 
-    private asyncUpdateDeletionEligabilityTimeForUsedPrebuild(userId: string, workspace: Workspace): void {
+    private asyncUpdateDeletionEligibilityTimeForUsedPrebuild(userId: string, workspace: Workspace): void {
         (async () => {
             if (WithPrebuild.is(workspace.context) && workspace.context.prebuildWorkspaceId) {
                 // mark the prebuild active
                 const prebuiltWorkspace = await this.db.findPrebuiltWorkspaceById(
                     workspace.context.prebuildWorkspaceId,
                 );
                 if (prebuiltWorkspace?.buildWorkspaceId) {
-                    await this.updateDeletionEligabilityTime(userId, prebuiltWorkspace?.buildWorkspaceId, true);
+                    await this.updateDeletionEligibilityTime(userId, prebuiltWorkspace?.buildWorkspaceId, true);
                 }
             }
         })().catch((err) =>
@@ -515,20 +515,20 @@ export class WorkspaceService {
         );
     }
 
-    private asyncUpdateDeletionEligabilityTime(userId: string, workspaceId: string): void {
-        this.updateDeletionEligabilityTime(userId, workspaceId).catch((err) =>
+    private asyncUpdateDeletionEligibilityTime(userId: string, workspaceId: string, activeNow?: boolean): void {
+        this.updateDeletionEligibilityTime(userId, workspaceId, activeNow).catch((err) =>
             log.error({ userId, workspaceId }, "Failed to update deletion eligibility time", err),
         );
     }
 
     /**
      * Sets the deletionEligibilityTime of the workspace, depending on the current state of the workspace and the configuration.
      *
-     * @param userId sets the
-     * @param workspaceId
+     * @param userId the user to act as
+     * @param workspaceId the workspace to update
      * @returns
      */
-    async updateDeletionEligabilityTime(userId: string, workspaceId: string, activeNow = false): Promise<void> {
+    async updateDeletionEligibilityTime(userId: string, workspaceId: string, activeNow = false): Promise<void> {
         try {
             let daysToLive = this.config.workspaceGarbageCollection?.minAgeDays || 14;
             const daysToLiveForPrebuilds = this.config.workspaceGarbageCollection?.minAgePrebuildDays || 7;
@@ -561,6 +561,12 @@ export class WorkspaceService {
                 daysToLive = daysToLive * 2;
             }
             deletionEligibilityTime.setDate(deletionEligibilityTime.getDate() + daysToLive);
+            if (new Date() > deletionEligibilityTime) {
+                log.warn({ userId, workspaceId }, "Prevented setting deletion eligibility time in the past", {
+                    deletionEligibilityTime: deletionEligibilityTime.toISOString(),
+                });
+                return;
+            }
             await this.db.updatePartial(workspaceId, {
                 deletionEligibilityTime: deletionEligibilityTime.toISOString(),
             });
@@ -819,7 +825,7 @@ export class WorkspaceService {
 
         // at this point we're about to actually start a new workspace
         const result = await this.workspaceStarter.startWorkspace(ctx, workspace, user, await projectPromise, options);
-        this.asyncUpdateDeletionEligabilityTime(user.id, workspaceId);
+        this.asyncUpdateDeletionEligibilityTime(user.id, workspaceId, true);
         return result;
     }
 
@@ -928,7 +934,7 @@ export class WorkspaceService {
 
         const workspace = await this.doGetWorkspace(userId, workspaceId);
         instance = await this.db.updateInstancePartial(instance.id, { gitStatus });
-        await this.updateDeletionEligabilityTime(userId, workspaceId);
+        await this.updateDeletionEligibilityTime(userId, workspaceId, true);
         await this.publisher.publishInstanceUpdate({
             instanceID: instance.id,
             ownerID: workspace.ownerId,

components/server/src/workspace/workspace-starter.ts

@@ -490,7 +490,7 @@ export class WorkspaceStarter {
             client = await this.clientProvider.get(instanceRegion);
         } catch (err) {
             log.error({ instanceId }, "cannot stop workspace instance", err);
-            // we want to stop a workspace but the region doesn't exist. So we can assume it doesn't run anyymore and there will never be updates coming to bridge.
+            // we want to stop a workspace but the region doesn't exist. So we can assume it doesn't run anymore and there will never be updates coming to bridge.
             // let's mark this workspace as stopped if it is not already stopped.
             const workspace = await this.workspaceDb.trace(ctx).findByInstanceId(instanceId);
             const instance = await this.workspaceDb.trace(ctx).findInstanceById(instanceId);