Fix prebuild lookup for prebuilds run in reverse-chronological order

Author: filiptronicek

components/server/src/bitbucket-server/bitbucket-server-repository-provider.ts

@@ -190,7 +190,7 @@ export class BitbucketServerRepositoryProvider implements RepositoryProvider {
             owner,
             repoKind,
             repositorySlug: repo,
-            query: { shaOrRevision: ref, limit: 1000 },
+            query: { shaOrRevision: ref, limit: 1000 }, // ft: why do we limit to 1000 and not maxDepth?
         });
 
         const commits = commitsResult.values || [];

components/server/src/bitbucket/bitbucket-repository-provider.ts

@@ -143,7 +143,7 @@ export class BitbucketRepositoryProvider implements RepositoryProvider {
     ): Promise<string[]> {
         const api = await this.apiFactory.create(user);
         // TODO(janx): To get more results than Bitbucket API's max pagelen (seems to be 100), pagination should be handled.
-        // The additional property 'page' may be helfpul.
+        // The additional property 'page' may be helpful.
         const result = await api.repositories.listCommitsAt({
             workspace: owner,
             repo_slug: repo,

components/server/src/github/github-repository-provider.ts

@@ -130,7 +130,7 @@ export class GithubRepositoryProvider implements RepositoryProvider {
             }
 
             // TODO(janx): To get more results than GitHub API's max page size (seems to be 100), pagination should be handled.
-            // These additional history properties may be helfpul:
+            // These additional history properties may be helpful:
             //     totalCount,
             //     pageInfo {
             //         haxNextPage,

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

@@ -45,6 +45,7 @@ export class IncrementalWorkspaceService {
             context.revision,
             maxDepth,
         );
+
         history.commitHistory.unshift(context.revision);
         if (context.additionalRepositoryCheckoutInfo && context.additionalRepositoryCheckoutInfo.length > 0) {
             const histories = context.additionalRepositoryCheckoutInfo.map(async (info) => {
@@ -83,8 +84,21 @@ export class IncrementalWorkspaceService {
         // Note: This query returns only not-garbage-collected prebuilds in order to reduce cardinality
         // (e.g., at the time of writing, the Gitpod repository has 16K+ prebuilds, but only ~300 not-garbage-collected)
         const recentPrebuilds = await this.workspaceDB.findPrebuildsWithWorkspace(projectId);
+
+        const sortedRecentPrebuilds = recentPrebuilds
+            .filter((prebuild) => {
+                return history.commitHistory?.includes(prebuild.prebuild.commit);
+            })
+            .sort((a, b) => {
+                // instead of the DB-returned creation time we use the commit history to sort the prebuilds
+                // this way we can return the correct prebuild even if the prebuild was first created for a later commit and then another one for an earlier commit
+                const aIdx = history.commitHistory?.indexOf(a.prebuild.commit) ?? -1;
+                const bIdx = history.commitHistory?.indexOf(b.prebuild.commit) ?? -1;
+
+                return aIdx - bIdx;
+            });
         const imageSource = await imageSourcePromise;
-        for (const recentPrebuild of recentPrebuilds) {
+        for (const recentPrebuild of sortedRecentPrebuilds) {
             if (
                 this.isGoodBaseforIncrementalBuild(
                     history,
@@ -98,6 +112,7 @@ export class IncrementalWorkspaceService {
                 return recentPrebuild.prebuild;
             }
         }
+
         return undefined;
     }
 
@@ -156,10 +171,6 @@ export class IncrementalWorkspaceService {
 
         // ensure the image source hasn't changed (skips older images)
         if (JSON.stringify(imageSource) !== JSON.stringify(candidateWorkspace.imageSource)) {
-            log.debug(`Skipping parent prebuild: Outdated image`, {
-                imageSource,
-                parentImageSource: candidateWorkspace.imageSource,
-            });
             return false;
         }
 

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

@@ -130,6 +130,7 @@ class MockRepositoryProvider implements RepositoryProvider {
         }
         throw new Error(`ref ${ref} not found`);
     }
+
     async getCommitInfo(user: User, owner: string, repo: string, ref: string): Promise<CommitInfo | undefined> {
         return headu(this.branches.get(ref)?.commits);
     }
@@ -541,6 +542,92 @@ describe("ContextService", async () => {
         expect((ctx.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit).to.equal(revision1);
     });
 
+    it("should handle triggering prebuilds out of order with respect to commits", async () => {
+        const commit1 = {
+            sha: "69420",
+            author: "some-dude",
+            commitMessage: `commit 69420`,
+        };
+        const commit2 = {
+            sha: "69421",
+            author: "some-other-dude",
+            commitMessage: "commit 69421",
+        };
+        const branchName = "branch-2";
+        mockRepositoryProvider.addBranch(
+            { name: branchName, htmlUrl: `https://github.com/gitpod-io/empty/tree/${branchName}` },
+            [commit1],
+        );
+        mockRepositoryProvider.pushCommit(branchName, commit2);
+
+        // request context for both commits separately
+        const svc = container.get(ContextService);
+        let ctx1 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit1.sha}`, {
+            projectId: project.id,
+            organizationId: org.id,
+            forceDefaultConfig: false,
+        });
+        let ctx2 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit2.sha}`, {
+            projectId: project.id,
+            organizationId: org.id,
+            forceDefaultConfig: false,
+        });
+
+        // trigger and "await" prebuilds for both commits in push order
+        const prebuildManager = container.get(PrebuildManager);
+        const workspaceDb: WorkspaceDB = container.get(WorkspaceDB);
+        const prebuildForCommit2 = await prebuildManager.startPrebuild(
+            {},
+            { user: owner, project, commitInfo: commit2, context: ctx2.context as CommitContext },
+        );
+        const prebuild1 = await workspaceDb.findPrebuildByID(prebuildForCommit2.prebuildId);
+        await workspaceDb.storePrebuiltWorkspace({
+            ...prebuild1!,
+            state: "available",
+        });
+        const wsAndI1 = await workspaceDb.findWorkspaceAndInstance(prebuild1!.buildWorkspaceId);
+        await workspaceDb.updateInstancePartial(wsAndI1!.instanceId, { status: { phase: "stopped" } });
+
+        const prebuildForCommit1 = await prebuildManager.startPrebuild(
+            {},
+            { user: owner, project, commitInfo: commit1, context: ctx1.context as CommitContext, forcePrebuild: true },
+        );
+        const prebuild2 = await workspaceDb.findPrebuildByID(prebuildForCommit1.prebuildId);
+        await workspaceDb.storePrebuiltWorkspace({
+            ...prebuild2!,
+            state: "available",
+        });
+        const wsAndI2 = await workspaceDb.findWorkspaceAndInstance(prebuild2!.buildWorkspaceId);
+        await workspaceDb.updateInstancePartial(wsAndI2!.instanceId, { status: { phase: "stopped" } });
+
+        // after everything has settled, request context for both commits again
+
+        ctx1 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit1.sha}`, {
+            projectId: project.id,
+            organizationId: org.id,
+            forceDefaultConfig: false,
+        });
+        ctx2 = await svc.parseContext(owner, `https://github.com/gitpod-io/empty/commit/${commit2.sha}`, {
+            projectId: project.id,
+            organizationId: org.id,
+            forceDefaultConfig: false,
+        });
+
+        expect(ctx1.project?.id).to.equal(project.id);
+        expect(PrebuiltWorkspaceContext.is(ctx1.context)).to.equal(true);
+        expect((ctx1.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(
+            prebuildForCommit1.prebuildId,
+        );
+        expect((ctx1.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit).to.equal(commit1.sha);
+
+        expect(ctx2.project?.id).to.equal(project.id);
+        expect(PrebuiltWorkspaceContext.is(ctx2.context)).to.equal(true);
+        expect((ctx2.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.id).to.equal(
+            prebuildForCommit2.prebuildId,
+        );
+        expect((ctx2.context as any as PrebuiltWorkspaceContext).prebuiltWorkspace.commit).to.equal(commit2.sha);
+    });
+
     it("should parse snapshot context", async () => {
         const svc = container.get(ContextService);
         const ctx = await svc.parseContext(owner, `snapshot/${snapshot.id}`, {