From 7ada28f9e33e448157d5c6b9ef9f6ff87a85500a Mon Sep 17 00:00:00 2001 From: Thomas Durieux <5577568+tdurieux@users.noreply.github.com> Date: Sun, 27 Sep 2026 06:09:19 +0000 Subject: [PATCH] fix: report repository refresh failures and reject busy updates --- public/script/app.js | 26 +++++--- src/server/routes/repository-private.ts | 18 ++++-- test/dashboard-refresh.test.js | 64 +++++++++++++++++++ test/repository-refresh.test.js | 85 +++++++++++++++++++++++++ 4 files changed, 176 insertions(+), 17 deletions(-) create mode 100644 test/dashboard-refresh.test.js create mode 100644 test/repository-refresh.test.js diff --git a/public/script/app.js b/public/script/app.js index 427c854..ce86ad8 100644 --- a/public/script/app.js +++ b/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 ); }; diff --git a/src/server/routes/repository-private.ts b/src/server/routes/repository-private.ts index 4f03b35..b248a45 100644 --- a/src/server/routes/repository-private.ts +++ b/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) { diff --git a/test/dashboard-refresh.test.js b/test/dashboard-refresh.test.js new file mode 100644 index 0000000..de629d2 --- /dev/null +++ b/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"); + }); +}); diff --git a/test/repository-refresh.test.js b/test/repository-refresh.test.js new file mode 100644 index 0000000..c6d1900 --- /dev/null +++ b/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"); + }); +});