diff --git a/.github/actions/load-docker-image/action.yml b/.github/actions/load-docker-image/action.yml index ad1197cfe4e..bc0573974f6 100644 --- a/.github/actions/load-docker-image/action.yml +++ b/.github/actions/load-docker-image/action.yml @@ -8,7 +8,7 @@ inputs: description: 'Docker image tags (multi-line string)' required: true artifact-name: - description: 'Name of the artifact to download (fork PRs only)' + description: 'Name of the image artifact to download' required: false default: 'docker-image' @@ -20,15 +20,17 @@ runs: uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: name: ${{ inputs.artifact-name }} + path: ${{ runner.temp }}/${{ inputs.artifact-name }} - name: Load image from artifact (artifact) if: inputs.use-artifact == 'true' shell: bash env: - ARTIFACT_NAME: ${{ inputs.artifact-name }} + ARTIFACT_PATH: ${{ runner.temp }}/${{ inputs.artifact-name }}/${{ inputs.artifact-name }}.tar.gz run: | + trap 'rm -f -- "$ARTIFACT_PATH"' EXIT echo "Loading Docker image from artifact..." - gunzip -c "${ARTIFACT_NAME}.tar.gz" | docker load + gunzip -c "$ARTIFACT_PATH" | docker load echo "Available images after load:" docker images diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ae3e4c7c5b6..831d6393257 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2060,28 +2060,33 @@ jobs: - name: Setup Docker Registry Mirrors uses: ./.github/actions/setup-docker-registry-mirrors - - name: Set up Docker Buildx - uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4 + - name: Pull Tinybird CLI image + id: pull_tb_cli + if: matrix.analytics == 'true' && needs.job_setup.outputs.changed_tb_cli != 'true' + run: | + COMPOSE_IMAGE="${COMPOSE_PROJECT_NAME:-ghost-dev}-tb-cli" + if docker pull ghcr.io/tryghost/tb-cli:latest; then + docker tag ghcr.io/tryghost/tb-cli:latest "$COMPOSE_IMAGE" + echo "Pulled tb-cli from GHCR" + echo "build_required=false" >> "$GITHUB_OUTPUT" + else + echo "GHCR image not available, building from source" + echo "build_required=true" >> "$GITHUB_OUTPUT" + fi - - name: Pull or build Tinybird CLI Image - if: matrix.analytics == 'true' + - name: Build Tinybird CLI image + if: matrix.analytics == 'true' && (needs.job_setup.outputs.changed_tb_cli == 'true' || steps.pull_tb_cli.outputs.build_required == 'true') env: CHANGED_TB_CLI: ${{ needs.job_setup.outputs.changed_tb_cli }} run: | COMPOSE_IMAGE="${COMPOSE_PROJECT_NAME:-ghost-dev}-tb-cli" - # GHCR's :latest is only republished from main, so it can't reflect a - # Dockerfile change under review — build from source when this PR - # touches it, otherwise take the prebuilt fast path. if [[ "$CHANGED_TB_CLI" == 'true' ]]; then echo "docker/tb-cli changed, building from source" - docker buildx build --load -t "$COMPOSE_IMAGE" -f docker/tb-cli/Dockerfile . - elif docker pull ghcr.io/tryghost/tb-cli:latest 2>/dev/null; then - echo "Pulled tb-cli from GHCR" - docker tag ghcr.io/tryghost/tb-cli:latest "$COMPOSE_IMAGE" else echo "GHCR image not available, building from source" - docker buildx build --load -t "$COMPOSE_IMAGE" -f docker/tb-cli/Dockerfile . fi + docker build --tag "$COMPOSE_IMAGE" --file docker/tb-cli/Dockerfile . + docker builder prune --all --force - name: Load Image uses: ./.github/actions/load-docker-image @@ -2103,6 +2108,12 @@ jobs: # (admin, apps, ghost/core). Scope the install to cut shard setup time. run: pnpm install --frozen-lockfile --filter @tryghost/e2e... + - name: Report Analytics runner disk usage + if: matrix.analytics == 'true' + run: | + df -h / + docker system df + - name: Prepare E2E CI job env: GHOST_E2E_IMAGE: ${{ steps.load.outputs.image-tag }} diff --git a/ghost/core/core/server/api/endpoints/posts.js b/ghost/core/core/server/api/endpoints/posts.js index add78359041..ce378911f08 100644 --- a/ghost/core/core/server/api/endpoints/posts.js +++ b/ghost/core/core/server/api/endpoints/posts.js @@ -129,9 +129,8 @@ const controller = { method: 'importContent', }, async query(frame) { - // The CSV must be parsed before the response goes out: the uploaded temp file is - // deleted as soon as it is sent. The posts are written by a background job behind - // the 202. + // The content-import service stages and validates the upload before this response; + // Multer deletes the request temp file as soon as the 202 is sent. const { importId, total } = await contentImportService.importCSV({ filePath: frame.file.path, fileName: frame.file.name, diff --git a/ghost/core/core/server/services/content-import/import/importer.ts b/ghost/core/core/server/services/content-import/import/importer.ts index c36b6fe3f33..836395f86ee 100644 --- a/ghost/core/core/server/services/content-import/import/importer.ts +++ b/ghost/core/core/server/services/content-import/import/importer.ts @@ -13,6 +13,8 @@ import type { Clock, ImportRun, ImportRunStore, RowOutcome } from './store'; import type { PreparedImportSource } from './source'; import type { PreparedPostRow, PreparedPostRows } from './reader'; import { MediaInliningFailure, type PostMediaInlining } from './media'; +import type { ImportFileStager, StagedImportFile } from './staged-file'; +import ContentCSVImportJob from '../jobs/content-csv-import-job'; export type { ImportRequest } from './schema'; @@ -20,9 +22,9 @@ const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); const tpl = require('@tryghost/tpl'); -// The CSV is parsed inside the request (the uploaded temp file is deleted when the -// response is sent); the parsed rows are handed to an in-process background job -// that writes one post per row. +// The upload is staged because the request temp file is deleted with the response. +// Request-time parsing preserves synchronous validation and the current row count; +// the class-based job reparses the staged file so its payload stays serializable. // The id is what a completion report will be looked up by. export interface ImportAccepted { @@ -54,7 +56,7 @@ const messages = { function logLifecycle(message: string): void { try { - logging.info(`[Background Job] content-import ${message}`); + logging.info(`[Background Job] ${ContentCSVImportJob.type} ${message}`); } catch { // Observability must not change whether an import is queued or resolves. } @@ -72,7 +74,8 @@ interface ImporterDeps { getCleanHTML: () => CleanHTML; createMediaInliner: () => PostMediaInlining; email: EmailNotifications; - addJob: (job: { job: () => Promise; offloaded: boolean; name: string }) => void; + dispatchJob: (job: ContentCSVImportJob) => Promise; + fileStager: ImportFileStager; report: FailureReporter; store: ImportRunStore; urlForPost: (post: WrittenPost) => string; @@ -97,7 +100,8 @@ class ContentCSVImporter { private _getCleanHTML: () => CleanHTML; private _createMediaInliner: () => PostMediaInlining; private _email: EmailNotifications; - private _addJob: ImporterDeps['addJob']; + private _dispatchJob: ImporterDeps['dispatchJob']; + private _fileStager: ImportFileStager; private _report: FailureReporter; private _store: ImportRunStore; private _urlForPost: (post: WrittenPost) => string; @@ -114,7 +118,8 @@ class ContentCSVImporter { getCleanHTML, createMediaInliner, email, - addJob, + dispatchJob, + fileStager, report, store, urlForPost, @@ -130,7 +135,8 @@ class ContentCSVImporter { this._getCleanHTML = getCleanHTML; this._createMediaInliner = createMediaInliner; this._email = email; - this._addJob = addJob; + this._dispatchJob = dispatchJob; + this._fileStager = fileStager; this._report = report; this._store = store; this._urlForPost = urlForPost; @@ -141,66 +147,103 @@ class ContentCSVImporter { async importCSV(request: ImportRequest): Promise { const emailRecipient = request.requestUserEmail ?? (await this._email.getDefaultRecipient()); - const source = await this._prepareSource(request); - let preparedRows: PreparedPostRows; + let stagedFile: StagedImportFile; try { - const result = await this._readRows(source.filePath, request.mapping); - preparedRows = Array.isArray(result) - ? { columns: [], rows: result.map((data, index) => ({ data, line: index + 2 })) } - : result; + stagedFile = await this._fileStager.stage(request); } catch (error) { - await this.cleanupSource(source.cleanup); throw new errors.ValidationError({ message: tpl(messages.unreadableFile), err: error, }); } + let handedOff = false; - // Temporary while import state is held in memory: the durable job - // system milestone removes the cap. - if (preparedRows.rows.length > MAX_POSTS) { + try { + const stagedRequest = { + filePath: stagedFile.path, + fileName: stagedFile.name, + mapping: request.mapping, + requestUserEmail: request.requestUserEmail, + }; + const { source, preparedRows } = await this.readPreparedRows(stagedRequest); await this.cleanupSource(source.cleanup); - throw new errors.ValidationError({ - message: tpl(messages.tooManyPosts, { max: MAX_POSTS }), + + this.assertWithinPostLimit(preparedRows.rows); + + const runId = this._newRunId(); + const importTagNames = buildImportTagNames(runId, this._getTimezone(), this._now()); + this._store.create(runId, preparedRows.rows.length, preparedRows.columns); + const job = new ContentCSVImportJob({ + importId: runId, + file: stagedFile, + mapping: request.mapping, + importTagNames, + emailRecipient, }); + + logLifecycle('queued'); + try { + await this._dispatchJob(job); + } catch (error) { + this._store.fail(runId, messageOf(error)); + this._store.release(runId); + throw error; + } + + handedOff = true; + return { importId: runId, total: preparedRows.rows.length }; + } finally { + if (!handedOff) { + await this.cleanupStagedFile(stagedFile); + } } + } - const runId = this._newRunId(); - const importTagNames = buildImportTagNames(runId, this._getTimezone(), this._now()); - this._store.create(runId, preparedRows.rows.length, preparedRows.columns); + async handle(job: ContentCSVImportJob): Promise { + let source: PreparedImportSource | undefined; - logLifecycle('queued'); try { - this._addJob({ - job: () => - this.runImportJob(runId, importTagNames, preparedRows.rows, source, emailRecipient), - offloaded: false, - name: 'content-import', + const prepared = await this.readPreparedRows({ + filePath: job.file.path, + fileName: job.file.name, + mapping: job.mapping, }); + source = prepared.source; + this.assertWithinPostLimit(prepared.preparedRows.rows); + await this.processRows( + job.importId, + job.importTagNames, + prepared.preparedRows.rows, + prepared.source, + ); } catch (error) { - this._store.fail(runId, messageOf(error)); - await this.cleanupSource(source.cleanup); - this._store.release(runId); + this._store.fail(job.importId, messageOf(error)); throw error; + } finally { + if (source) { + await this.cleanupSource(source.cleanup); + } + const run = this._store.get(job.importId); + if (run) { + await this.settle(() => this._email.send(run, job.emailRecipient)); + } + this._store.release(job.importId); + await this.cleanupStagedFile(job.file); } + } - return { importId: runId, total: preparedRows.rows.length }; + allSettled(): Promise { + return this._store.allSettled(); } - // Must resolve in every case: the job manager reads a rejected inline job as a - // defect in the job itself, and there is no retry behind it. - private async runImportJob( + private async processRows( runId: string, importTagNames: string[], rows: PreparedPostRow[], source: PreparedImportSource, - emailRecipient: string, ): Promise { - const startedAt = Date.now(); - logLifecycle('started'); let urlFailureCount = 0; let firstUrlFailure: unknown; - let failed = false; try { if (source.assets) { @@ -347,19 +390,36 @@ class ContentCSVImporter { this.reportUrlFailures(urlFailureCount, firstUrlFailure); this._store.finish(runId); } catch (error) { - failed = true; this.reportUrlFailures(urlFailureCount, firstUrlFailure); - this._report(error); - this._store.fail(runId, messageOf(error)); - } finally { + throw error; + } + } + + private async readPreparedRows( + request: ImportRequest, + ): Promise<{ source: PreparedImportSource; preparedRows: PreparedPostRows }> { + const source = await this._prepareSource(request); + try { + const result = await this._readRows(source.filePath, request.mapping); + const preparedRows = Array.isArray(result) + ? { columns: [], rows: result.map((data, index) => ({ data, line: index + 2 })) } + : result; + return { source, preparedRows }; + } catch (error) { await this.cleanupSource(source.cleanup); - const run = this._store.get(runId); - if (run) { - await this.settle(() => this._email.send(run, emailRecipient)); - } - this._store.release(runId); - const outcome = failed ? 'failed after' : 'completed in'; - logLifecycle(`${outcome} ${Date.now() - startedAt}ms`); + throw new errors.ValidationError({ + message: tpl(messages.unreadableFile), + err: error, + }); + } + } + + private assertWithinPostLimit(rows: PreparedPostRow[]): void { + // Temporary while import state is held in memory: M8 removes the cap. + if (rows.length > MAX_POSTS) { + throw new errors.ValidationError({ + message: tpl(messages.tooManyPosts, { max: MAX_POSTS }), + }); } } @@ -393,6 +453,14 @@ class ContentCSVImporter { } } + private async cleanupStagedFile(file: StagedImportFile): Promise { + try { + await this._fileStager.remove(file); + } catch (error) { + this._report(error); + } + } + private async settle(operation: () => Promise): Promise { try { await operation(); diff --git a/ghost/core/core/server/services/content-import/import/staged-file.ts b/ghost/core/core/server/services/content-import/import/staged-file.ts new file mode 100644 index 00000000000..3ded349c1fd --- /dev/null +++ b/ghost/core/core/server/services/content-import/import/staged-file.ts @@ -0,0 +1,36 @@ +import crypto from 'node:crypto'; +import os from 'node:os'; +import path from 'node:path'; +import fs from 'fs-extra'; + +export interface StagedImportFile { + path: string; + name: string; +} + +export interface ImportFileStager { + stage(file: { filePath: string; fileName: string }): Promise; + remove(file: StagedImportFile): Promise; +} + +export function createImportFileStager(): ImportFileStager { + return { + async stage({ filePath, fileName }) { + const stagedPath = path.join(os.tmpdir(), `content-csv-import-${crypto.randomUUID()}`); + + try { + await fs.copyFile(filePath, stagedPath, fs.constants.COPYFILE_EXCL); + await fs.chmod(stagedPath, 0o600); + } catch (error) { + await fs.remove(stagedPath).catch(() => {}); + throw error; + } + + return { path: stagedPath, name: fileName }; + }, + + async remove(file) { + await fs.remove(file.path); + }, + }; +} diff --git a/ghost/core/core/server/services/content-import/import/store.ts b/ghost/core/core/server/services/content-import/import/store.ts index e25d42445f2..e82f3d2d569 100644 --- a/ghost/core/core/server/services/content-import/import/store.ts +++ b/ghost/core/core/server/services/content-import/import/store.ts @@ -47,6 +47,7 @@ const MAX_RUN_AGE_MS = 60 * 60 * 1000; export class ImportRunStore { // Insertion-ordered, so count-eviction drops the oldest run first. private _runs = new Map(); + private _settledWaiters = new Set<() => void>(); private _now: Clock; constructor({ now = () => new Date() }: { now?: Clock } = {}) { @@ -95,12 +96,34 @@ export class ImportRunStore { release(id: string): void { this._runs.delete(id); + this.resolveSettledWaiters(); + } + + allSettled(): Promise { + if (this._runs.size === 0) { + return Promise.resolve(); + } + + return new Promise((resolve) => { + this._settledWaiters.add(resolve); + }); + } + + private resolveSettledWaiters(): void { + if (this._runs.size > 0) { + return; + } + + for (const resolve of this._settledWaiters) { + resolve(); + } + this._settledWaiters.clear(); } // A running run is never evicted, whatever its age: the job holds only the runId, // so evicting mid-import would silently turn its record()/finish() calls into // no-ops and lose the report. The count cap can briefly overshoot while several - // imports run at once; the inline job queue bounds how many that can be. + // imports run at once; the jobs backend bounds how many that can be. private evict(): void { const cutoff = this._now().getTime() - MAX_RUN_AGE_MS; for (const [id, run] of this._runs) { diff --git a/ghost/core/core/server/services/content-import/index.ts b/ghost/core/core/server/services/content-import/index.ts index 5efde634691..43167a719ee 100644 --- a/ghost/core/core/server/services/content-import/index.ts +++ b/ghost/core/core/server/services/content-import/index.ts @@ -13,6 +13,9 @@ import { prepareImportSource } from './import/source'; import { PostMediaInliner } from './import/media'; import { isLocalMediaUrl } from './import/local-media-url'; import { urlForImportedPost } from './import/post-link'; +import { createImportFileStager } from './import/staged-file'; +import ContentCSVImportJob from './jobs/content-csv-import-job'; +import { getInstance as getJobsService } from '../jobs-service'; // The request is built from HTTP upload metadata, so it is validated at the // service boundary rather than trusted. @@ -30,7 +33,6 @@ function makeImporter(): ContentCSVImporter { // are guaranteed loaded. const models = require('../../models'); const lexicalLib = require('../../lib/lexical'); - const jobsService = require('../jobs'); const settingsCache = require('../../../shared/settings-cache'); const urlService = require('../url'); const urlUtils = require('../../../shared/url-utils').default; @@ -40,8 +42,8 @@ function makeImporter(): ContentCSVImporter { const { GhostMailer } = require('../mail'); const ghostMailer = new GhostMailer(); - // Inline jobs never reach the job manager's Sentry handler, which is wired to the - // offloaded worker path only, so a throw here would be seen by nobody. + // Row aggregates and best-effort cleanup are intentionally reported without + // failing a run. Fatal handler errors are reported by the class-based jobs service. const report: FailureReporter = (error) => { try { logging.error( @@ -82,7 +84,8 @@ function makeImporter(): ContentCSVImporter { }), }), email, - addJob: jobsService.addJob.bind(jobsService), + dispatchJob: (job) => getJobsService().dispatch(job), + fileStager: createImportFileStager(), report, store: new ImportRunStore(), urlForPost: (post) => @@ -122,3 +125,21 @@ export function importCSV(request: ImportRequest): Promise { return importer.importCSV(parsedRequest.data); } + +export function handleJob(job: ContentCSVImportJob): Promise { + if (!importer) { + throw new errors.InternalServerError({ message: 'Content import service used before init' }); + } + + return importer.handle(job); +} + +// Test-facing parity with the legacy inline queue while the import run store +// remains in memory. M8 removes this together with that store. +export function allSettled(): Promise { + if (!importer) { + return Promise.resolve(); + } + + return importer.allSettled(); +} diff --git a/ghost/core/core/server/services/content-import/jobs/content-csv-import-job.ts b/ghost/core/core/server/services/content-import/jobs/content-csv-import-job.ts new file mode 100644 index 00000000000..118fed22ca1 --- /dev/null +++ b/ghost/core/core/server/services/content-import/jobs/content-csv-import-job.ts @@ -0,0 +1,30 @@ +import { Job } from '../../jobs-service/job'; + +export interface ContentCSVImportJobData { + importId: string; + file: { + path: string; + name: string; + }; + mapping?: Record; + importTagNames: string[]; + emailRecipient: string; +} + +export default class ContentCSVImportJob extends Job { + static type = 'content-csv-import'; + readonly importId: string; + readonly file: ContentCSVImportJobData['file']; + readonly mapping?: Record; + readonly importTagNames: string[]; + readonly emailRecipient: string; + + constructor(data: ContentCSVImportJobData) { + super(); + this.importId = data.importId; + this.file = data.file; + this.mapping = data.mapping; + this.importTagNames = data.importTagNames; + this.emailRecipient = data.emailRecipient; + } +} diff --git a/ghost/core/core/server/services/jobs-service/register-job-handlers.ts b/ghost/core/core/server/services/jobs-service/register-job-handlers.ts index 9e1451df159..166ca2bce9a 100644 --- a/ghost/core/core/server/services/jobs-service/register-job-handlers.ts +++ b/ghost/core/core/server/services/jobs-service/register-job-handlers.ts @@ -8,6 +8,8 @@ import * as gifts from '../gifts'; import CleanGiftsJob from '../gifts/jobs/clean-gifts-job'; import ExternalMediaInliner from '../media-inliner/external-media-inliner'; import ExternalMediaInlinerJob from '../media-inliner/external-media-inliner-job'; +import ContentCSVImportJob from '../content-import/jobs/content-csv-import-job'; +import * as contentImport from '../content-import'; interface RegisterJobHandlersDependencies { jobsService: JobsService; @@ -57,4 +59,8 @@ export default function registerJobHandlers({ jobsService.handle(ExternalMediaInlinerJob, async (job) => { await mediaInliner.inline(job.domains); }); + + jobsService.handle(ContentCSVImportJob, async (job) => { + await contentImport.handleJob(job); + }); } diff --git a/ghost/core/test/e2e-api/admin/posts-importer.test.js b/ghost/core/test/e2e-api/admin/posts-importer.test.js index 76fc0bc5a6e..11910d42fa2 100644 --- a/ghost/core/test/e2e-api/admin/posts-importer.test.js +++ b/ghost/core/test/e2e-api/admin/posts-importer.test.js @@ -14,7 +14,7 @@ const path = require('path'); const nock = require('nock'); const papaparse = require('papaparse'); const models = require('../../../core/server/models'); -const jobsService = require('../../../core/server/services/jobs'); +const contentImportService = require('../../../core/server/services/content-import'); const mediaInlinerService = require('../../../core/server/services/media-inliner'); const { PostMediaInliner, @@ -111,7 +111,7 @@ describe('Posts Importer API', function () { afterEach(async function () { // Every accepted upload schedules a background import — drain it so a job // doesn't run on into another test (or another file on this fork's DB) - await jobsService.allSettled(); + await contentImportService.allSettled(); await cleanupRemoteImportedMedia(); await Promise.all(getImportedAssetPaths().map((filePath) => fs.rm(filePath, { force: true }))); mockManager.restore(); @@ -142,7 +142,7 @@ describe('Posts Importer API', function () { .post('posts/upload/') .attach('postsfile', completionCsvPath) .expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const email = mockManager.assert.sentEmail({ subject: 'Your content import is complete', @@ -180,7 +180,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', preExistingPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); mockManager.assert.sentEmail({ subject: 'Your content import is complete' }); const reportPath = await csvFile( @@ -200,7 +200,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', reportPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const email = mockManager.assert.sentEmail({ subject: 'Your content import is complete' }); assert.match(email.html, /processed 6 rows/); @@ -275,7 +275,7 @@ describe('Posts Importer API', function () { }); await agent.post('posts/upload/').body(form).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const email = mockManager.assert.sentEmail({ subject: 'Your content import was unsuccessful', @@ -309,23 +309,27 @@ describe('Posts Importer API', function () { contentType: 'text/csv', }); await agent.post('posts/upload/').body(retryForm).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); mockManager.assert.sentEmail({ subject: 'Your content import was unsuccessful' }); }); it('Keeps content import initialization idempotent and rejects invalid service requests', async function () { - const contentImportService = + const isolatedContentImportService = await import('../../../core/server/services/content-import/index.ts?coverage-lifecycle'); assert.throws( - () => contentImportService.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }), + () => + isolatedContentImportService.importCSV({ + filePath: '/tmp/posts.csv', + fileName: 'posts.csv', + }), /Content import service used before init/, ); - contentImportService.init(); - contentImportService.init(); + isolatedContentImportService.init(); + isolatedContentImportService.init(); assert.throws( - () => contentImportService.importCSV({ filePath: '', fileName: '' }), + () => isolatedContentImportService.importCSV({ filePath: '', fileName: '' }), (error) => { assert.equal(error.errorType, 'ValidationError'); assert.match(error.message, /Too small/); @@ -333,7 +337,7 @@ describe('Posts Importer API', function () { }, ); await assert.rejects( - contentImportService.importCSV({ + isolatedContentImportService.importCSV({ filePath: path.join(tmpDir, 'missing.csv'), fileName: 'missing.csv', }), @@ -382,7 +386,7 @@ describe('Posts Importer API', function () { const filePath = await csvFile('remote-media.csv', csv); await agent.post('posts/upload/').attach('postsfile', filePath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); for (const request of requests) { assert.equal(request.isDone(), true, request.pendingMocks().join(', ')); @@ -436,7 +440,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', filePath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); assert.equal(request.isDone(), true, request.pendingMocks().join(', ')); const post = await models.Post.findOne({ title: 'Unsupported remote media', status: 'all' }); @@ -479,7 +483,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', filePath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); sinon.assert.notCalled(importUrl); const post = await models.Post.findOne({ title: 'Local media', status: 'all' }); @@ -606,7 +610,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', filePath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); sinon.assert.calledOnceWithExactly(importUrl, sourceUrl); const post = await models.Post.findOne({ title: 'Unexpected media failure', status: 'all' }); @@ -631,7 +635,7 @@ describe('Posts Importer API', function () { const { body } = await agent.post('posts/upload/').body(form).expectStatus(202); assert.equal(body.meta.total, 1); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne({ title: 'ZIP mapping post', status: 'all' }); assert.ok(post); assert.match(post.get('html'), /Mapped from ZIP/); @@ -658,7 +662,7 @@ describe('Posts Importer API', function () { .attach('postsfile', zipPath) .expectStatus(202); assert.equal(body.meta.total, 3); - await jobsService.allSettled(); + await contentImportService.allSettled(); sinon.assert.notCalled(importUrl); @@ -719,7 +723,7 @@ describe('Posts Importer API', function () { .attach('postsfile', zipPath) .expectStatus(202); assert.equal(body.meta.total, 1); - await jobsService.allSettled(); + await contentImportService.allSettled(); for (const filePath of getImportedAssetPaths().slice(4, 7)) { assert.equal(await fs.stat(filePath).then(() => true), true, `${filePath} was stored`); @@ -760,7 +764,7 @@ describe('Posts Importer API', function () { }); await agent.post('posts/upload/').attach('postsfile', zipPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne({ title: 'ZIP failed assets', status: 'all' }); assert.equal(post, null); @@ -785,7 +789,7 @@ describe('Posts Importer API', function () { }); await agent.post('posts/upload/').attach('postsfile', zipPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne({ title: 'ZIP partial file failure', status: 'all' }); assert.equal(post, null); @@ -806,7 +810,7 @@ describe('Posts Importer API', function () { }); await agent.post('posts/upload/').attach('postsfile', zipPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne({ title: 'ZIP cross-group failure', status: 'all' }); assert.equal(post, null); @@ -835,7 +839,7 @@ describe('Posts Importer API', function () { }); await agent.post('posts/upload/').attach('postsfile', zipPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne({ title: 'ZIP incomplete rollback', status: 'all' }); assert.equal(post, null); @@ -1058,7 +1062,7 @@ describe('Posts Importer API', function () { assert.match(body.meta.import_id, /^[0-9a-f]{24}$/); assert.equal(body.meta.total, 2); - await jobsService.allSettled(); + await contentImportService.allSettled(); const { data: posts } = await models.Post.findPage({ filter: `title:~'Content check post'`, @@ -1133,9 +1137,9 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', duplicateCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); await agent.post('posts/upload/').attach('postsfile', duplicateCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const { data: posts } = await models.Post.findPage({ filter: "slug:'csv-deduplication-check'", @@ -1157,7 +1161,7 @@ describe('Posts Importer API', function () { 'CSV source ID original,csv-source-id-original,m5-source-id-primary\n', ); await agent.post('posts/upload/').attach('postsfile', originalCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const comparisonCsvPath = await csvFile( 'posts-import-source-id-comparisons.csv', @@ -1176,7 +1180,7 @@ describe('Posts Importer API', function () { contentType: 'text/csv', }); await agent.post('posts/upload/').body(form).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const original = await models.Post.findOne({ slug: 'csv-source-id-original', status: 'all' }); const sourceDuplicate = await models.Post.findOne({ @@ -1208,7 +1212,7 @@ describe('Posts Importer API', function () { 'CSV update slug original,csv-update-by-slug,,2025-01-01T00:00:00.000Z\n', ); await agent.post('posts/upload/').attach('postsfile', originalCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const updatesCsvPath = await csvFile( 'posts-import-update-comparisons.csv', @@ -1221,7 +1225,7 @@ describe('Posts Importer API', function () { 'CSV update after invalid,csv-update-after-invalid,,2025-04-01T00:00:00.000Z\n', ); await agent.post('posts/upload/').attach('postsfile', updatesCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const newer = await models.Post.findOne({ comment_id: 'm5-update-source', @@ -1258,7 +1262,7 @@ describe('Posts Importer API', function () { await agent.post('posts/upload/').attach('postsfile', paidSiteCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne({ title: 'Visibility check post', status: 'all' }); // left to the model, visibility would have followed default_content_visibility @@ -1298,7 +1302,7 @@ describe('Posts Importer API', function () { }); await agent.post('posts/upload/').body(form).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne( { title: 'Mapped field post', status: 'all' }, @@ -1361,7 +1365,7 @@ describe('Posts Importer API', function () { }); await agent.post('posts/upload/').body(form).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne( { title: 'CSV existing relations', status: 'all' }, @@ -1396,7 +1400,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', authorsCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const contributor = await models.User.findOne( { email: 'new-csv-contributor@example.com', status: 'all' }, @@ -1447,7 +1451,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', authorsCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); assert.equal( await models.User.findOne({ email: 'csv-rollback-contributor@example.com', status: 'all' }), @@ -1469,14 +1473,14 @@ describe('Posts Importer API', function () { 'CSV created tags two,"#CSV Internal Tag,New CSV Tag"\n', ); await agent.post('posts/upload/').attach('postsfile', firstCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const secondCsvPath = await csvFile( 'posts-import-reused-tags.csv', 'title,tags\nCSV reused tags,New CSV Tag\n', ); await agent.post('posts/upload/').attach('postsfile', secondCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const publicTags = await models.Tag.findAll({ filter: "name:'New CSV Tag'" }); const internalTags = await models.Tag.findAll({ filter: "name:'#CSV Internal Tag'" }); @@ -1534,7 +1538,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', tagsCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); assert.equal(await models.Tag.findOne({ name: 'CSV Rollback Tag' }), null); assert.equal(await models.Post.findOne({ title: 'CSV tag rollback', status: 'all' }), null); @@ -1556,7 +1560,7 @@ describe('Posts Importer API', function () { }); await agent.post('posts/upload/').body(form).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne({ title: 'Markdown field post', status: 'all' }); assert.ok(post); @@ -1573,7 +1577,7 @@ describe('Posts Importer API', function () { ); await agent.post('posts/upload/').attach('postsfile', cleanupCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const post = await models.Post.findOne({ title: 'Clean HTML post', status: 'all' }); assert.ok(post); @@ -1601,7 +1605,7 @@ describe('Posts Importer API', function () { await agent.post('posts/upload/').attach('postsfile', badRowsCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const { data: posts } = await models.Post.findPage({ filter: `title:~'Bad rows check'`, @@ -1632,7 +1636,7 @@ describe('Posts Importer API', function () { await agent.post('posts/upload/').attach('postsfile', garbageCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const { meta: { @@ -1661,7 +1665,7 @@ describe('Posts Importer API', function () { assert.match(body.errors[0].message, /more than 100 posts/); - await jobsService.allSettled(); + await contentImportService.allSettled(); const { data: posts } = await models.Post.findPage({ filter: `title:~'Over cap post'`, diff --git a/ghost/core/test/e2e-webhooks/posts-importer.test.ts b/ghost/core/test/e2e-webhooks/posts-importer.test.ts index e709ad138db..c9eb406a42b 100644 --- a/ghost/core/test/e2e-webhooks/posts-importer.test.ts +++ b/ghost/core/test/e2e-webhooks/posts-importer.test.ts @@ -6,7 +6,7 @@ import assert from 'node:assert/strict'; const DomainEvents = require('@tryghost/domain-events'); const { agentProvider, mockManager, fixtureManager, dbUtils } = require('../utils/e2e-framework'); const models = require('../../core/server/models'); -const jobsService = require('../../core/server/services/jobs'); +const contentImportService = require('../../core/server/services/content-import'); // The importer's key safety guarantee: a bulk import sends zero newsletter emails // and fires zero per-post webhooks. Every consumer of post events checks @@ -36,7 +36,7 @@ describe('CSV content import side-effects', function () { }); afterEach(async function () { - await jobsService.allSettled(); + await contentImportService.allSettled(); mockManager.restore(); }); @@ -99,7 +99,7 @@ describe('CSV content import side-effects', function () { ); await adminAPIAgent.post('posts/upload/').attach('postsfile', csvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); const updateCsvPath = path.join(tmpDir, 'posts-import-update-side-effects.csv'); await fs.writeFile( @@ -109,7 +109,7 @@ describe('CSV content import side-effects', function () { ); await adminAPIAgent.post('posts/upload/').attach('postsfile', updateCsvPath).expectStatus(202); - await jobsService.allSettled(); + await contentImportService.allSettled(); await DomainEvents.allSettled(); // The negative needs a settle window: a wrongly-fired webhook reaches nock a // beat after the model event, and asserting too early would pass vacuously diff --git a/ghost/core/test/unit/server/services/content-import/import/importer.test.ts b/ghost/core/test/unit/server/services/content-import/import/importer.test.ts index cbafbd96a70..1893f085381 100644 --- a/ghost/core/test/unit/server/services/content-import/import/importer.test.ts +++ b/ghost/core/test/unit/server/services/content-import/import/importer.test.ts @@ -8,6 +8,7 @@ import type { PostImportRow } from '../../../../../../core/server/services/conte import type { PostData } from '../../../../../../core/server/services/content-import/import/post-data'; import type { PostWriteMetadata } from '../../../../../../core/server/services/content-import/import/post-repository'; import type { ImportRun } from '../../../../../../core/server/services/content-import/import/store'; +import ContentCSVImportJob from '../../../../../../core/server/services/content-import/jobs/content-csv-import-job'; const row = (title: string, html = `

${title}

`): PostImportRow => ({ title, @@ -23,7 +24,9 @@ function harness( ) { const created: Array<{ data: PostData; options: object; metadata?: PostWriteMetadata }> = []; const reported: unknown[] = []; - const jobs: Array<{ name: string; offloaded: boolean; job: () => Promise }> = []; + const jobs: ContentCSVImportJob[] = []; + const stagedFiles: Array<{ path: string; name: string }> = []; + const removedFiles: Array<{ path: string; name: string }> = []; const createFailures = new Map(); const duplicateSlugs = new Set(); const updatedTitles = new Set(); @@ -99,9 +102,19 @@ function harness( send: sendEmail, getDefaultRecipient, }, - addJob: (job: { name: string; offloaded: boolean; job: () => Promise }) => { + dispatchJob: async (job: ContentCSVImportJob) => { jobs.push(job); }, + fileStager: { + stage: async ({ fileName }: { fileName: string }) => { + const file = { path: `/tmp/staged-${stagedFiles.length + 1}`, name: fileName }; + stagedFiles.push(file); + return file; + }, + remove: async (file: { path: string; name: string }) => { + removedFiles.push(file); + }, + }, report: (error: unknown) => { reported.push(error); }, @@ -120,14 +133,14 @@ function harness( const importer = new ContentCSVImporter(deps); - // The scheduled job is invoked directly rather than through the job manager. + // The dispatched job is invoked directly rather than through the jobs backend. const run = async () => { const accepted = await importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv', }); for (const job of jobs) { - await job.job(); + await importer.handle(job); } return accepted; }; @@ -151,6 +164,8 @@ function harness( created, reported, jobs, + stagedFiles, + removedFiles, createFailures, duplicateSlugs, updatedTitles, @@ -184,7 +199,7 @@ describe('ContentCSVImporter', function () { sinon.restore(); }); - it('accepts the upload with the row count and defers the writes to one inline job', async function () { + it('accepts the upload with the row count and dispatches one serializable job', async function () { const h = harness(); const accepted = await h.importer.importCSV({ @@ -194,8 +209,15 @@ describe('ContentCSVImporter', function () { assert.deepEqual(accepted, { importId: 'run_test', total: 2 }); assert.equal(h.jobs.length, 1); - assert.equal(h.jobs[0].name, 'content-import'); - assert.equal(h.jobs[0].offloaded, false); + assert.equal(h.jobs[0].constructor, ContentCSVImportJob); + const serialized = JSON.parse(JSON.stringify(h.jobs[0])); + assert.deepEqual(serialized, { + importId: 'run_test', + file: { path: '/tmp/staged-1', name: 'posts.csv' }, + importTagNames: ['#Import 2026-01-01 11:30', '#Import Run run_test'], + emailRecipient: 'owner@example.com', + }); + assert.deepEqual(JSON.parse(JSON.stringify(new ContentCSVImportJob(serialized))), serialized); assert.equal(h.created.length, 0, 'nothing is written until the job runs'); assert.equal( h.store.get('run_test')?.status, @@ -204,14 +226,30 @@ describe('ContentCSVImporter', function () { ); }); - it('logs the searchable lifecycle of the inline job', async function () { + it('logs when the class-based job is queued', async function () { const h = harness(); await h.run(); - sinon.assert.calledWithExactly(infoLog, '[Background Job] content-import queued'); - sinon.assert.calledWithExactly(infoLog, '[Background Job] content-import started'); - sinon.assert.calledWithMatch(infoLog, /^\[Background Job\] content-import completed in \d+ms$/); + sinon.assert.calledWithExactly(infoLog, '[Background Job] content-csv-import queued'); + assert.deepEqual(h.removedFiles, [{ path: '/tmp/staged-1', name: 'posts.csv' }]); + }); + + it('settles only after the dispatched import has finished reporting and cleanup', async function () { + const h = harness(); + await h.importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }); + let settled = false; + const waiting = h.importer.allSettled().then(() => { + settled = true; + }); + + await Promise.resolve(); + assert.equal(settled, false); + + h.releaseRun.restore(); + await h.importer.handle(h.jobs[0]); + await waiting; + assert.equal(settled, true); }); it('carries original source columns and cells into the completed run', async function () { @@ -262,7 +300,7 @@ describe('ContentCSVImporter', function () { fileName: 'posts.csv', requestUserEmail: 'requester@example.com', }); - await h.jobs[0].job(); + await h.importer.handle(h.jobs[0]); sinon.assert.notCalled(h.getDefaultRecipient); sinon.assert.calledOnce(h.sendEmail); @@ -315,7 +353,7 @@ describe('ContentCSVImporter', function () { throw failure; }); - await h.run(); + await assert.rejects(h.run(), failure); sinon.assert.calledOnce(h.sendEmail); assert.equal(h.sentRuns[0].status, 'failed'); @@ -329,7 +367,7 @@ describe('ContentCSVImporter', function () { await h.importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }); h.store.release('run_test'); - await h.jobs[0].job(); + await h.importer.handle(h.jobs[0]); sinon.assert.notCalled(h.sendEmail); assert.equal(h.created.length, 2); @@ -341,7 +379,7 @@ describe('ContentCSVImporter', function () { const importer = new ContentCSVImporter(deps); await importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }); - await h.jobs[0].job(); + await importer.handle(h.jobs[0]); assert.ok(h.store.get('run_test')?.startedAt instanceof Date); assert.ok(h.store.get('run_test')?.finishedAt instanceof Date); @@ -365,11 +403,11 @@ describe('ContentCSVImporter', function () { it('passes a caller-supplied mapping to the CSV reader', async function () { const h = harness(); - let receivedMapping: Record | undefined; + const receivedMappings: Array | undefined> = []; const importer = new ContentCSVImporter({ ...h.deps, readRows: async (_path, mapping) => { - receivedMapping = mapping; + receivedMappings.push(mapping); return [row('Mapped')]; }, }); @@ -379,8 +417,10 @@ describe('ContentCSVImporter', function () { fileName: 'posts.csv', mapping: { Headline: 'title' }, }); + await importer.handle(h.jobs[0]); - assert.deepEqual(receivedMapping, { Headline: 'title' }); + assert.deepEqual(receivedMappings, [{ Headline: 'title' }, { Headline: 'title' }]); + assert.deepEqual(h.jobs[0].mapping, { Headline: 'title' }); }); it('reads a prepared archive source and cleans it after the job', async function () { @@ -399,9 +439,9 @@ describe('ContentCSVImporter', function () { await importer.importCSV({ filePath: '/tmp/upload', fileName: 'posts.zip' }); assert.equal(receivedPath, '/tmp/extracted/posts.csv'); - sinon.assert.notCalled(cleanup); - await h.jobs[0].job(); sinon.assert.calledOnce(cleanup); + await importer.handle(h.jobs[0]); + sinon.assert.calledTwice(cleanup); }); it('stores and rewrites every asset before resolving content converters', async function () { @@ -436,11 +476,11 @@ describe('ContentCSVImporter', function () { }); await importer.importCSV({ filePath: '/tmp/posts.zip', fileName: 'posts.zip' }); - await h.jobs[0].job(); + await importer.handle(h.jobs[0]); assert.deepEqual(events.slice(0, 4), ['store', 'rewrite', 'convert', 'inline']); assert.match(h.created[0].data.lexical ?? '', /unique\.jpg/); - sinon.assert.calledOnce(cleanup); + sinon.assert.calledTwice(cleanup); }); it('fails the run without converting or creating posts when asset storage fails', async function () { @@ -464,15 +504,15 @@ describe('ContentCSVImporter', function () { }); await importer.importCSV({ filePath: '/tmp/posts.zip', fileName: 'posts.zip' }); - await h.jobs[0].job(); + await assert.rejects(importer.handle(h.jobs[0]), failure); assert.equal(h.converterResolutions(), 0); assert.equal(h.created.length, 0); assert.equal(h.store.get('run_test')?.status, 'failed'); assert.equal(h.store.get('run_test')?.failureReason, 'storage unavailable'); - assert.equal(h.reported.at(-1), failure); + assert.deepEqual(h.reported, []); sinon.assert.notCalled(rewriteRows); - sinon.assert.calledOnce(cleanup); + sinon.assert.calledTwice(cleanup); }); it('writes one post per row, in order, under the importing options', async function () { @@ -520,7 +560,7 @@ describe('ContentCSVImporter', function () { }); await importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }); - await h.jobs[0].job(); + await importer.handle(h.jobs[0]); assert.deepEqual(events, ['convert', 'inline', 'write']); }); @@ -529,9 +569,9 @@ describe('ContentCSVImporter', function () { const h = harness([row('Cached media')]); await h.importer.importCSV({ filePath: '/tmp/first.csv', fileName: 'first.csv' }); - await h.jobs[0].job(); + await h.importer.handle(h.jobs[0]); await h.importer.importCSV({ filePath: '/tmp/second.csv', fileName: 'second.csv' }); - await h.jobs[1].job(); + await h.importer.handle(h.jobs[1]); sinon.assert.calledTwice(h.createMediaInliner); assert.notEqual( @@ -601,11 +641,11 @@ describe('ContentCSVImporter', function () { const failure = new Error('media importer defect'); h.inlineMedia.rejects(failure); - await h.run(); + await assert.rejects(h.run(), failure); assert.equal(h.created.length, 0); assert.equal(h.inlineMedia.callCount, 1); - assert.deepEqual(h.reported, [failure]); + assert.deepEqual(h.reported, []); assert.equal(h.store.get('run_test')?.status, 'failed'); assert.equal(h.store.get('run_test')?.failureReason, failure.message); }); @@ -681,6 +721,28 @@ describe('ContentCSVImporter', function () { assert.equal(h.jobs.length, 0, 'no job was scheduled'); sinon.assert.calledOnce(cleanup); assert.equal(h.reported.at(-1), cleanupError); + assert.deepEqual(h.removedFiles, [{ path: '/tmp/staged-1', name: 'posts.csv' }]); + }); + + it('reports an upload that cannot be staged as an unreadable file', async function () { + const h = harness(); + const importer = new ContentCSVImporter({ + ...h.deps, + fileStager: { + ...h.deps.fileStager, + stage: async () => { + throw new Error('copy failed'); + }, + }, + }); + + await assert.rejects( + importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }), + /The file could not be parsed as a CSV file/, + ); + + assert.equal(h.jobs.length, 0); + assert.equal(h.store.get('run_test'), undefined); }); it('resolves the html converter once per run, not per row', async function () { @@ -738,15 +800,65 @@ describe('ContentCSVImporter', function () { assert.equal(h.store.get('run_test'), undefined, 'no run was registered'); sinon.assert.notCalled(storeAssets); sinon.assert.calledOnce(cleanup); + assert.deepEqual(h.removedFiles, [{ path: '/tmp/staged-1', name: 'posts.zip' }]); }); - it('cleans a prepared source if scheduling throws', async function () { + it('rechecks the temporary cap inside the job before writing', async function () { + const h = harness(); + let readCount = 0; + const importer = new ContentCSVImporter({ + ...h.deps, + readRows: async () => { + readCount += 1; + return readCount === 1 + ? [row('Preflight')] + : Array.from({ length: 101 }, (_, i) => row(`Post ${i + 1}`)); + }, + }); + + await importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }); + await assert.rejects(importer.handle(h.jobs[0]), /more than 100 posts/); + + assert.equal(h.created.length, 0); + assert.equal(h.store.get('run_test')?.status, 'failed'); + assert.match(h.store.get('run_test')?.failureReason ?? '', /more than 100 posts/); + assert.deepEqual(h.removedFiles, [{ path: '/tmp/staged-1', name: 'posts.csv' }]); + }); + + it('fails and emails the run if the staged file cannot be reparsed by the job', async function () { + const h = harness(); + let readCount = 0; + const importer = new ContentCSVImporter({ + ...h.deps, + readRows: async () => { + readCount += 1; + if (readCount === 2) { + throw new Error('staged file unavailable'); + } + return [row('Preflight')]; + }, + }); + + await importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }); + await assert.rejects(importer.handle(h.jobs[0]), /could not be parsed as a CSV/); + + assert.equal(h.store.get('run_test')?.status, 'failed'); + assert.match(h.store.get('run_test')?.failureReason ?? '', /could not be parsed as a CSV/); + sinon.assert.calledOnceWithExactly( + h.sendEmail, + sinon.match({ status: 'failed' }), + 'owner@example.com', + ); + assert.deepEqual(h.removedFiles, [{ path: '/tmp/staged-1', name: 'posts.csv' }]); + }); + + it('cleans a prepared source if dispatch fails', async function () { const h = harness(); const cleanup = sinon.stub().resolves(); const importer = new ContentCSVImporter({ ...h.deps, prepareSource: async () => ({ filePath: '/tmp/extracted/posts.csv', cleanup }), - addJob: () => { + dispatchJob: async () => { throw new Error('queue unavailable'); }, }); @@ -760,6 +872,7 @@ describe('ContentCSVImporter', function () { sinon.assert.calledOnce(cleanup); sinon.assert.notCalled(h.sendEmail); sinon.assert.calledOnceWithExactly(h.releaseRun, 'run_test'); + assert.deepEqual(h.removedFiles, [{ path: '/tmp/staged-1', name: 'posts.zip' }]); }); it('reports cleanup failures without rejecting the completed job', async function () { @@ -776,10 +889,30 @@ describe('ContentCSVImporter', function () { }); await importer.importCSV({ filePath: '/tmp/posts.zip', fileName: 'posts.zip' }); - await h.jobs[0].job(); + await importer.handle(h.jobs[0]); assert.equal(h.store.get('run_test')?.status, 'complete'); - assert.equal(h.reported.at(-1), cleanupError); + assert.deepEqual(h.reported, [cleanupError, cleanupError]); + }); + + it('reports a staged-file cleanup failure without rejecting the completed job', async function () { + const h = harness(); + const cleanupError = new Error('staged cleanup failed'); + const importer = new ContentCSVImporter({ + ...h.deps, + fileStager: { + ...h.deps.fileStager, + remove: async () => { + throw cleanupError; + }, + }, + }); + + await importer.importCSV({ filePath: '/tmp/posts.csv', fileName: 'posts.csv' }); + await importer.handle(h.jobs[0]); + + assert.equal(h.store.get('run_test')?.status, 'complete'); + assert.deepEqual(h.reported, [cleanupError]); }); it('accepts a file exactly at the cap', async function () { @@ -1038,20 +1171,19 @@ describe('ContentCSVImporter', function () { ); }); - it("still reports a run-level failure that is nobody's row", async function () { + it("rejects a run-level failure that is nobody's row for the jobs service to report", async function () { const h = harness(); const failure = new Error('converter unavailable'); h.setHtmlToLexicalFactory(() => { throw failure; }); - await h.run(); + await assert.rejects(h.run(), failure); - assert.deepEqual(h.reported, [failure]); + assert.deepEqual(h.reported, []); assert.equal(h.store.get('run_test')?.status, 'failed'); assert.equal(h.store.get('run_test')?.failureReason, 'converter unavailable'); assert.ok(h.store.get('run_test')?.finishedAt instanceof Date); - sinon.assert.calledWithMatch(infoLog, /^\[Background Job\] content-import failed after \d+ms$/); }); it('stops the run for an unexpected row-processing failure', async function () { @@ -1064,9 +1196,9 @@ describe('ContentCSVImporter', function () { }); const h = harness([badRow, row('Never reached')]); - await h.run(); + await assert.rejects(h.run(), failure); - assert.deepEqual(h.reported, [failure]); + assert.deepEqual(h.reported, []); assert.equal(h.created.length, 0); assert.equal(h.store.get('run_test')?.status, 'failed'); assert.equal(h.store.get('run_test')?.failureReason, 'unexpected row failure'); @@ -1078,9 +1210,9 @@ describe('ContentCSVImporter', function () { throw {}; }); - await h.run(); + await assert.rejects(h.run(), () => true); - assert.deepEqual(h.reported, [{}]); + assert.deepEqual(h.reported, []); assert.equal(h.store.get('run_test')?.status, 'failed'); assert.equal(h.store.get('run_test')?.failureReason, 'Unknown error'); }); diff --git a/ghost/core/test/unit/server/services/content-import/import/staged-file.test.ts b/ghost/core/test/unit/server/services/content-import/import/staged-file.test.ts new file mode 100644 index 00000000000..26a1b011ae0 --- /dev/null +++ b/ghost/core/test/unit/server/services/content-import/import/staged-file.test.ts @@ -0,0 +1,56 @@ +import assert from 'node:assert/strict'; +import os from 'node:os'; +import path from 'node:path'; +import fs from 'fs-extra'; +import sinon from 'sinon'; +import { afterEach, beforeEach, describe, it } from 'vitest'; +import { createImportFileStager } from '../../../../../../core/server/services/content-import/import/staged-file'; + +describe('content import staged file', function () { + let sourceDirectory: string; + let sourcePath: string; + const stagedPaths: string[] = []; + + beforeEach(async function () { + sourceDirectory = await fs.mkdtemp(path.join(os.tmpdir(), 'content-import-stager-test-')); + sourcePath = path.join(sourceDirectory, 'upload'); + await fs.writeFile(sourcePath, 'title\nA staged post\n'); + }); + + afterEach(async function () { + sinon.restore(); + await Promise.all(stagedPaths.map((stagedPath) => fs.remove(stagedPath))); + stagedPaths.length = 0; + await fs.remove(sourceDirectory); + }); + + it('copies uploads to unique private files and removes them idempotently', async function () { + const stager = createImportFileStager(); + const first = await stager.stage({ filePath: sourcePath, fileName: 'posts.csv' }); + const second = await stager.stage({ filePath: sourcePath, fileName: 'posts.csv' }); + stagedPaths.push(first.path, second.path); + + assert.notEqual(first.path, second.path); + assert.equal(first.name, 'posts.csv'); + assert.equal(await fs.readFile(first.path, 'utf8'), 'title\nA staged post\n'); + assert.equal((await fs.stat(first.path)).mode & 0o777, 0o600); + + await stager.remove(first); + await stager.remove(first); + assert.equal(await fs.pathExists(first.path), false); + }); + + it('removes a partial staged file when securing it fails', async function () { + const stager = createImportFileStager(); + const remove = sinon.spy(fs, 'remove'); + sinon.stub(fs, 'chmod').rejects(new Error('chmod failed')); + + await assert.rejects( + stager.stage({ filePath: sourcePath, fileName: 'posts.csv' }), + /chmod failed/, + ); + + sinon.assert.calledOnce(remove); + assert.equal(await fs.pathExists(remove.firstCall.args[0]), false); + }); +}); diff --git a/ghost/core/test/unit/server/services/content-import/import/store.test.ts b/ghost/core/test/unit/server/services/content-import/import/store.test.ts index 4c9c1fcf19c..79c73f8989d 100644 --- a/ghost/core/test/unit/server/services/content-import/import/store.test.ts +++ b/ghost/core/test/unit/server/services/content-import/import/store.test.ts @@ -93,6 +93,35 @@ describe('ImportRunStore', function () { assert.equal(failed?.finishedAt, finishedAt); }); + it('settles immediately when no imports are running', async function () { + const store = new ImportRunStore(); + + await store.allSettled(); + }); + + it('settles waiters only after every import has finished its reporting and is released', async function () { + const store = new ImportRunStore(); + store.create('run_1', 1); + store.create('run_2', 1); + let settled = false; + const waiting = store.allSettled().then(() => { + settled = true; + }); + + store.finish('run_1'); + store.release('run_1'); + await Promise.resolve(); + assert.equal(settled, false); + + store.fail('run_2', 'failed'); + await Promise.resolve(); + assert.equal(settled, false); + + store.release('run_2'); + await waiting; + assert.equal(settled, true); + }); + it('keeps only the most recent finished runs', function () { const store = new ImportRunStore(); diff --git a/ghost/core/test/unit/server/services/content-import/index.test.ts b/ghost/core/test/unit/server/services/content-import/index.test.ts new file mode 100644 index 00000000000..52d16055c34 --- /dev/null +++ b/ghost/core/test/unit/server/services/content-import/index.test.ts @@ -0,0 +1,19 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'vitest'; +import ContentCSVImportJob from '../../../../../core/server/services/content-import/jobs/content-csv-import-job'; + +const contentImport = require('../../../../../core/server/services/content-import'); + +describe('content import service', function () { + it('fails loudly when a job is delivered before the service is initialised', async function () { + const job = new ContentCSVImportJob({ + importId: 'run_test', + file: { path: '/tmp/staged-import', name: 'posts.csv' }, + importTagNames: ['#Import 2026-01-01 10:30', '#Import Run run_test'], + emailRecipient: 'owner@example.com', + }); + + assert.throws(() => contentImport.handleJob(job), /Content import service used before init/); + await contentImport.allSettled(); + }); +}); diff --git a/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts b/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts index 8717b7427ed..48c976f3206 100644 --- a/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts +++ b/ghost/core/test/unit/server/services/jobs-service/register-job-handlers.test.ts @@ -5,6 +5,7 @@ import logging from '@tryghost/logging'; import { JobsService } from '../../../../../core/server/services/jobs-service/jobs-service'; import ExternalMediaInliner from '../../../../../core/server/services/media-inliner/external-media-inliner'; import ExternalMediaInlinerJob from '../../../../../core/server/services/media-inliner/external-media-inliner-job'; +import ContentCSVImportJob from '../../../../../core/server/services/content-import/jobs/content-csv-import-job'; const registerJobHandlers = require('../../../../../core/server/services/jobs-service/register-job-handlers').default; @@ -116,4 +117,20 @@ describe('register-job-handlers', function () { await externalMediaInlinerHandler(job); }, error); }); + + it('routes content CSV import jobs to the content import service', async function () { + const job = new ContentCSVImportJob({ + importId: 'run_test', + file: { path: '/tmp/staged-import', name: 'posts.zip' }, + mapping: { Headline: 'title' }, + importTagNames: ['#Import 2026-01-01 10:30', '#Import Run run_test'], + emailRecipient: 'owner@example.com', + }); + const contentImportHandler = handlerFor('content-csv-import'); + + await assert.rejects( + () => contentImportHandler(job), + /Content import service used before init/, + ); + }); });