Merge pull request #841 from tdurieux/fix/repository-refresh-feedback

fix: report repository refresh failures and reject busy updates

public/asset-manifest.json

@@ -1,11 +1,11 @@
 {
-  "core.min.js": "core.c5bd53363a.min.js",
-  "vendor.min.js": "vendor.7f9da8be8e.min.js",
+  "core.min.js": "core.3b07189932.min.js",
+  "vendor.min.js": "vendor.3036803e29.min.js",
   "mermaid.min.js": "mermaid.f848a72d16.min.js",
   "all.min.css": "all.5fbafcda4e.min.css",
-  "markdown.min.js": "markdown.ad7b1d71c3.min.js",
-  "pdf.min.js": "pdf.eaa7573247.min.js",
-  "notebook.min.js": "notebook.8844e2735f.min.js",
-  "org.min.js": "org.f4e2a3f59f.min.js",
-  "editor.min.js": "editor.e243722d87.min.js"
+  "markdown.min.js": "markdown.44f2e93820.min.js",
+  "pdf.min.js": "pdf.0175d53d2f.min.js",
+  "notebook.min.js": "notebook.c3144c18ae.min.js",
+  "org.min.js": "org.f61f262915.min.js",
+  "editor.min.js": "editor.4f142d3adc.min.js"
 }

public/script/app.js

@@ -649,11 +649,12 @@ export const unifiedDashboardController = function (state, http, location, promi
         });
       };
 
-      function waitRepoToBeReady(repoId, callback) {
+      function waitRepoToBeReady(repoId, callback, onError) {
         http.get("/api/repo/" + repoId).then((res) => {
           for (const item of state.items) {
             if (item._type === "repo" && item.repoId == repoId) {
               item.status = res.data.status;
+              item.statusMessage = res.data.statusMessage;
               break;
             }
           }
@@ -666,8 +667,8 @@ export const unifiedDashboardController = function (state, http, location, promi
             callback(res.data);
             return;
           }
-          timers.timeout(() => waitRepoToBeReady(repoId, callback), 2500);
-        });
+          timers.timeout(() => waitRepoToBeReady(repoId, callback, onError), 2500);
+        }, onError);
       }
 
       const labelOf = (t) =>
@@ -716,26 +717,31 @@ export const unifiedDashboardController = function (state, http, location, promi
           body: `The ${label} ${item._id} is going to be refreshed.`,
         });
         state.addToast(toast);
+        const onError = (error) => {
+          toast.title = `Error during the refresh of ${item._id}.`;
+          toast.body = error.data?.error || error.body || "The refresh could not be completed. Please try again.";
+          loadAll();
+        };
         const endpoint = `${apiBaseOf(item._type)}/${item._id}/refresh`;
         http.post(endpoint).then(
           () => {
             if (item._type === "repo") {
-              waitRepoToBeReady(item._id, () => {
+              waitRepoToBeReady(item._id, (repo) => {
+                if (repo.status !== "ready") {
+                  onError({ body: repo.statusMessage || `The repository is ${repo.status}.` });
+                  return;
+                }
                 toast.title = `${item._id} is refreshed.`;
                 toast.body = `The ${label} ${item._id} is refreshed.`;
 
-              });
+              }, onError);
             } else {
               toast.title = `${item._id} is refreshed.`;
               toast.body = `The ${label} ${item._id} is refreshed.`;
               loadAll();
             }
           },
-          (error) => {
-            toast.title = `Error during the refresh of ${item._id}.`;
-            toast.body = error.body;
-            loadAll();
-          }
+          onError
         );
       };
 

src/server/routes/repository-private.ts

@@ -126,15 +126,19 @@ router.post(
       });
       if (!repo) return;
 
-      if (
-        repo.status == "preparing" ||
-        repo.status == "removing" ||
-        repo.status == "expiring"
-      )
-        return;
-
       const user = await getUser(req);
       isOwnerCoauthorOrAdmin(repo, user);
+
+      if (
+        repo.status == RepositoryStatus.PREPARING ||
+        repo.status == RepositoryStatus.QUEUE ||
+        repo.status == RepositoryStatus.DOWNLOAD ||
+        repo.status == RepositoryStatus.REMOVING ||
+        repo.status == RepositoryStatus.EXPIRING
+      ) {
+        throw new AnonymousError("invalid_status", { httpStatus: 409 });
+      }
+
       await repo.updateIfNeeded({ force: true });
       res.json({ status: repo.status });
     } catch (error) {

test/dashboard-refresh.test.js

@@ -0,0 +1,64 @@
+const { expect } = require("chai");
+const fs = require("fs");
+const vm = require("vm");
+const { setImmediate } = require("timers");
+const source = fs.readFileSync(require("path").join(__dirname, "../public/script/app.js"), "utf8");
+const actions = source.slice(source.indexOf("      function waitRepoToBeReady("), source.indexOf("      state.itemFilter ="));
+
+describe("dashboard refresh feedback", () => {
+  async function run({ statuses = [], postError, getError } = {}) {
+    let toast;
+    const item = { _type: "repo", _id: "restore-me", repoId: "restore-me", status: "removed" };
+    const state = { items: [item], addToast: value => { toast = value; } };
+    const pending = [];
+    let reloads = 0;
+    vm.runInNewContext(actions, {
+      state, reactive: value => value,
+      http: {
+        post: async () => { if (postError) throw postError; return {}; },
+        get: async () => { if (getError) throw getError; return { data: statuses.shift() }; },
+      },
+      timers: { timeout: callback => pending.push(callback) },
+      loadAll: () => { reloads++; },
+    });
+    state.refreshItem(item);
+    await new Promise(resolve => setImmediate(resolve));
+    while (pending.length) {
+      pending.shift()();
+      await new Promise(resolve => setImmediate(resolve));
+    }
+    return { toast, item, reloads };
+  }
+
+  it("reports success after preparation reaches ready", async () => {
+    const result = await run({ statuses: [{ status: "preparing" }, { status: "ready" }] });
+    expect(result.toast.title).to.equal("restore-me is refreshed.");
+    expect(result.item.status).to.equal("ready");
+    expect(result.reloads).to.equal(0);
+  });
+  for (const status of ["error", "removed", "expired"]) {
+    it(`reports ${status} as a failed refresh`, async () => {
+      const result = await run({ statuses: [{ status, statusMessage: "token_expired" }] });
+      expect(result.toast.title).to.equal("Error during the refresh of restore-me.");
+      expect(result.toast.body).to.equal("token_expired");
+      expect(result.item.statusMessage).to.equal("token_expired");
+      expect(result.reloads).to.equal(1);
+    });
+  }
+  it("explains a terminal status without a status message", async () => {
+    const result = await run({ statuses: [{ status: "expired" }] });
+    expect(result.toast.body).to.equal("The repository is expired.");
+  });
+  for (const stage of ["postError", "getError"]) {
+    it(`shows the API error from ${stage}`, async () => {
+      const result = await run({ [stage]: { data: { error: "not_connected" } } });
+      expect(result.toast.title).to.equal("Error during the refresh of restore-me.");
+      expect(result.toast.body).to.equal("not_connected");
+    });
+  }
+  it("reports a polling network failure", async () => {
+    const result = await run({ getError: new Error("Network failure") });
+    expect(result.toast.title).to.equal("Error during the refresh of restore-me.");
+    expect(result.toast.body).to.include("Please try again");
+  });
+});

test/repository-refresh.test.js

@@ -0,0 +1,85 @@
+const { expect } = require("chai");
+require("ts-node/register/transpile-only");
+const Repository = require("../src/core/Repository").default;
+const Model = require("../src/core/model/anonymizedRepositories/anonymizedRepositories.model").default;
+const utils = require("../src/server/routes/route-utils");
+const github = require("../src/core/source/GitHubRepository");
+const queue = require("../src/queue");
+const db = require("../src/server/database");
+const router = require("../src/server/routes/repository-private").default;
+const refresh = router.stack.find(layer => layer.route?.path === "/:repoId/refresh").route.stack[0].handle;
+
+describe("repository refresh and restoration", () => {
+  const restores = [];
+  function stub(object, key, value) {
+    const original = object[key];
+    restores.push(() => { object[key] = original; });
+    object[key] = value;
+  }
+  afterEach(() => { while (restores.length) restores.pop()(); });
+  function repository(status) {
+    const repo = new Repository(new Model({ repoId: "restore-me", status,
+      owner: "507f1f77bcf86cd799439011",
+      source: { repositoryName: "owner/repo", branch: "main", commit: "saved-sha" },
+      options: { expirationMode: "never" },
+    }));
+    stub(utils, "getRepo", async () => repo);
+    stub(utils, "getUser", async () => ({ isAdmin: true }));
+    stub(utils, "handleError", error => { throw error; });
+    return repo;
+  }
+
+  for (const status of ["preparing", "queue", "download", "removing", "expiring"]) {
+    it(`responds with a conflict during ${status} without starting another update`, async () => {
+      const repo = repository(status);
+      repo.updateIfNeeded = async () => { throw new Error("must not update"); };
+      let failure;
+      try { await refresh({}, {}); } catch (error) { failure = error; }
+      expect(failure?.message).to.equal("invalid_status");
+      expect(failure.httpStatus).to.equal(409);
+    });
+  }
+
+  it("checks ownership before returning an operation status", async () => {
+    repository("preparing");
+    stub(utils, "getUser", async () => ({ model: { id: "other" } }));
+    let failure;
+    try { await refresh({}, {}); } catch (error) { failure = error; }
+    expect(failure?.message).to.equal("not_authorized");
+    expect(failure.httpStatus).to.equal(403);
+  });
+
+  for (const commit of ["saved-sha", "new-sha"]) {
+    it(`rebuilds a removed repository at ${commit} while preserving its ID`, async () => {
+      const repo = repository("removed");
+      stub(db, "isConnected", false);
+      repo.getToken = async () => "test-token";
+      stub(github, "getRepositoryFromGitHub", async () => ({
+        fullName: "owner/repo", model: { defaultBranch: "main" },
+        branches: async () => [{ name: "main", commit }],
+        getCommitInfo: async () => ({ commit: {} }),
+      }));
+      repo.resetSate = async status => { repo.model.status = status; };
+      let added;
+      stub(queue, "downloadQueue", { add: async (...args) => { added = args; } });
+      let response;
+      await refresh({}, { json: body => { response = body; } });
+      expect(response.status).to.equal("preparing");
+      expect(repo.model.source.commit).to.equal(commit);
+      expect(repo.repoId).to.equal("restore-me");
+      expect(added[1]).to.deep.equal({ repoId: "restore-me" });
+    });
+  }
+
+  it("preserves a removed repository and reports a GitHub access failure", async () => {
+    const repo = repository("removed");
+    const failure = new Error("token_expired");
+    repo.getToken = async () => { throw failure; };
+    stub(queue, "downloadQueue", { add: async () => { throw new Error("must not enqueue"); } });
+    let caught;
+    try { await refresh({}, {}); } catch (error) { caught = error; }
+    expect(caught).to.equal(failure);
+    expect(repo.status).to.equal("removed");
+    expect(repo.model.source.commit).to.equal("saved-sha");
+  });
+});