From bb366b370bf01f9ce4dc0bb02f6074668608a9f0 Mon Sep 17 00:00:00 2001 From: Thomas Durieux <5577568+tdurieux@users.noreply.github.com> Date: Mon, 2 Nov 2026 22:16:16 +0000 Subject: [PATCH] fix: handle settings saves before repository cleanup --- src/core/Repository.ts | 33 ++++++++++++------- test/large-repository-tree.test.js | 51 +++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/src/core/Repository.ts b/src/core/Repository.ts index b05877c..50cd56f 100644 --- a/src/core/Repository.ts +++ b/src/core/Repository.ts @@ -38,6 +38,7 @@ import AnonymizedRepositoryModel from "./model/anonymizedRepositories/anonymized import { createLogger, serializeError } from "./logger"; const logger = createLogger("repository"); +const snapshotValue = (value: T | undefined) => value === undefined ? { $exists: false } : value; /** Capture and retire the stored key while clearing it, including concurrent index publication. */ async function updateRetiringPathIndex( filter: () => FilterQuery, @@ -1027,6 +1028,19 @@ export default class Repository { } } + /** Fields that identify the cached source and its current lifecycle owner. */ + private fileSnapshotFilter() { + return { + treeGeneration: snapshotValue(this.model.treeGeneration), + status: snapshotValue(this.model.status), statusDate: snapshotValue(this.model.statusDate), + anonymizeDate: snapshotValue(this.model.anonymizeDate), + fileMetadataRevision: snapshotValue(this.model.fileMetadataRevision), + "source.type": snapshotValue(this.model.source.type), "source.repositoryName": snapshotValue(this.model.source.repositoryName), + "source.commit": snapshotValue(this.model.source.commit), "source.branch": snapshotValue(this.model.source.branch), + "githubAccess.revision": snapshotValue(this.model.githubAccess?.revision), + }; + } + /** * Remove the repository */ @@ -1036,8 +1050,9 @@ export default class Repository { this.assertNotArchived(); const statusDate = new Date(); const result = await AnonymizedRepositoryModel.updateOne({ _id: this.model._id, - status: this.model.status, - "githubAccess.revision": expected.accessRevision || { $exists: false }, + ...this.fileSnapshotFilter(), ...this.refreshFilter(), + settingsSavedAt: snapshotValue(this.model.settingsSavedAt), + "githubAccess.revision": snapshotValue(expected.accessRevision), }, { $set: { status: RepositoryStatus.REMOVING, statusDate } }); if (!result.matchedCount) throw new AnonymousError("connection_changed", { httpStatus: 409 }); this.model.status = RepositoryStatus.REMOVING; @@ -1058,19 +1073,13 @@ export default class Repository { const defer = isConnected && (status || this.status) === RepositoryStatus.PREPARING; const previousGeneration = this.model.treeGeneration; const generation = defer ? randomUUID() : previousGeneration; - const expected = (value: T | undefined) => value === undefined ? { $exists: false } : value; const refreshToken = this.refreshToken; - const snapshot = { - status: expected(this.model.status), statusDate: expected(this.model.statusDate), - anonymizeDate: expected(this.model.anonymizeDate), settingsSavedAt: expected(this.model.settingsSavedAt), - fileMetadataRevision: expected(this.model.fileMetadataRevision), - "source.type": expected(this.model.source.type), "source.repositoryName": expected(this.model.source.repositoryName), - "source.commit": expected(this.model.source.commit), "source.branch": expected(this.model.source.branch), - "githubAccess.revision": expected(this.model.githubAccess?.revision), - }; + // Options-only saves apply at read time and leave this source snapshot valid. + // They must not cancel a queued worker without scheduling replacement work. + const snapshot = this.fileSnapshotFilter(); if (isConnected) { const result = await updateRetiringPathIndex(() => ({ _id: this.model._id, - treeGeneration: expected(previousGeneration), ...snapshot, + ...snapshot, ...(refreshToken ? { refreshToken, refreshUntil: { $gt: new Date() } } : {}), }), { $set: { fileMetadataRevision: revision, ...(defer ? { treeGeneration: generation } : {}) }, ...(defer ? { $addToSet: { retiredTreeGenerations: previousGeneration || "", diff --git a/test/large-repository-tree.test.js b/test/large-repository-tree.test.js index 481e818..8cacb2c 100644 --- a/test/large-repository-tree.test.js +++ b/test/large-repository-tree.test.js @@ -227,7 +227,6 @@ const uri = process.env.PERFORMANCE_MONGO_URI; ["source branch", { "source.branch": "replacement" }], ["source type", { "source.type": "Zip" }], ["anonymization snapshot", { anonymizeDate: new Date(1) }], - ["settings revision", { settingsSavedAt: new Date(1) }], ["metadata revision", { fileMetadataRevision: "replacement" }], ["access revision", { "githubAccess.revision": "replacement" }], ]; @@ -334,6 +333,56 @@ const uri = process.env.PERFORMANCE_MONGO_URI; expect(jobs).to.have.length(1); expect(jobs[0][1]).to.deep.equal({ repoId: row.repoId }); }); + for (const [label, change] of [...changedSnapshots, ["settings revision", { settingsSavedAt: new Date(1) }]] ) { + it(`rejects removal before claiming the lifecycle after a changed ${label}`, async () => { + const row = await repository({ status: "ready", treeGeneration: "old", pathIndexKey: "old-key" }), stale = new Repository(row); + await rows(1); await Repo.updateOne({ _id: row._id }, { $set: change }); + const before = await Repo.findById(row._id).lean(); + stale.removeCache = () => { throw Error("stale removal touched storage"); }; + const error = await stale.remove({}).then(() => null, error => error); + expect(error.message).to.equal("connection_changed"); + expect(await Repo.findById(row._id).lean()).to.deep.equal(before); + expect(await File.countDocuments({ repoId: row.repoId })).to.equal(1); + }); + } + + it("finishes preparation after an accepted options-only save with the same source", async () => { + const row = await repository({ status: "preparing", treeGeneration: "old", anonymizeDate: new Date(1), + source: { type: "GitHubStream", repositoryName: "fixture/repo", branch: "main", commit: "a".repeat(40) }, + options: { terms: ["before"], expirationMode: "never" } }); + const worker = new Repository(row), editor = new Repository(await Repo.findById(row._id)); + await rows(1); + Object.defineProperty(worker, "source", { get: () => ({ async *iterateFiles() { + yield { name: "replacement.txt", path: "", size: 2, sha: "b".repeat(40) }; + } }) }); + const utils = require("../src/server/routes/route-utils"), queue = require("../src/queue"); + const router = require("../src/server/routes/repository-private").default; + const handler = router.stack.find(layer => layer.route?.path === "/:repoId/" && layer.route.methods.post).route.stack[0].handle; + const restores = [], jobs = []; let savedSettingsAt; + const stub = (object, key, value) => { const original = object[key]; restores.push(() => { object[key] = original; }); object[key] = value; }; + stub(utils, "getRepo", async () => editor); + stub(utils, "getUser", async () => ({ id: editor.owner.id, model: { id: editor.owner.id } })); + stub(utils, "handleError", error => { throw error; }); + stub(queue, "downloadQueue", { add: async (...args) => { jobs.push(args); } }); + stub(db, "connect", async () => {}); + stub(db, "getRepository", async () => { + // The worker snapshot was loaded before this accepted owner edit. + await handler({ body: { repoId: row.repoId, fullName: "fixture/repo", terms: ["after"], + source: { branch: "main", commit: "a".repeat(40) }, options: { expirationMode: "never" } } }, { json() {} }); + savedSettingsAt = (await Repo.findById(row._id)).settingsSavedAt; + return worker; + }); + stub(github, "getToken", async () => "fixture"); stub(github, "getRedisGateResetAt", async () => 0); + try { + await require("../src/queue/processes/downloadRepository").default({ data: { repoId: row.repoId }, updateProgress: async () => {} }); + } finally { while (restores.length) restores.pop()(); } + const saved = await Repo.findById(row._id); + expect(saved.status).to.equal("ready"); expect(saved.options.terms).to.deep.equal(["after"]); + expect(saved.settingsSavedAt).to.deep.equal(savedSettingsAt); expect(savedSettingsAt).to.be.instanceOf(Date); + expect(jobs).to.have.length(0); + expect((await File.find({ repoId: row.repoId })).map(file => file.name)).to.deep.equal(["replacement.txt"]); + }); + it("retains retirement markers when foreground cleanup reaches its batch budget", async () => { const row = await repository({ treeGeneration: "active", retiredTreeGenerations: ["old"] }), repo = new Repository(row); await rows(501);