Skip to content

Changed the members CSV importer to only write a file for queued imports#29453

Open
rob-ghost wants to merge 1 commit into
mainfrom
fix/ber-3811-remove-importer-disk-round-trip
Open

Changed the members CSV importer to only write a file for queued imports#29453
rob-ghost wants to merge 1 commit into
mainfrom
fix/ber-3811-remove-importer-disk-round-trip

Conversation

@rob-ghost

@rob-ghost rob-ghost commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Problem

Every member import left behind a file that nothing ever deleted.

The importer parsed the uploaded CSV, wrote it back out as a second CSV, then read that file and parsed it again. Nobody cleaned up the second file.

On Ghost(Pro) those files land on a shared, persistent volume with:

  • no cleanup, in any repo
  • no per-site quota
  • disk alerts that only fire at 95% across the whole volume, with no way to tell which site filled it

For imports small enough to run inside the request, the round trip achieved nothing. Same process, same code, a moment apart.

It broke things too:

  • Re-serialising to CSV drops any column the member model doesn't know about, so custom field values can't be imported at all
  • A CSV with headers but no rows returned a 500
  • A label containing a comma got split into two labels
  • Values were saved with the spreadsheet formula-escape character still attached, so a member named -5 was stored as '-5

Solution

Parse once. Write a file only where one is actually needed.

Small imports run inside the request and never touch the disk.

Queued imports outlive the request, and the uploaded CSV is deleted the moment the response finishes. So their rows are spooled to a temp file, and the job that reads them is responsible for deleting it.

Three deliberate choices about that spool:

Choice Why
One row per line Neither writing nor reading builds one giant string. That matters on the path built for large imports.
JSON, not CSV CSV drops unknown columns. JSON keeps them, which is what unblocks custom field values.
Deleted whether it succeeds or fails Keeping the rows on failure would mean holding member data indefinitely against a manual replay that has no tooling behind it. The publisher still has the file they uploaded.

Fixed along the way: the empty-CSV 500, the leaked file, the split label.

Deliberate changes to stored data: the formula-escape character is no longer saved, and mapped columns the member model doesn't recognise now reach the import instead of being dropped.

One ordering change worth knowing about: if a mapping names two columns for the same field (subscribed and subscribed_to_emails are one field), the first now wins. Previously the column mapped to the literal subscribed won regardless of order, and it was last-wins for anything else. Only reachable by hand-built API calls — the import UI can't produce it — but it decides a member's subscription state, so it's a deliberate rule now rather than an accident of column order.

How this fits #29170

The queued import is shaped for a queue that stores its jobs.

The job body is one method with two arguments:

runImportJob(data, deps)
  • data is the spool path, recipient, and label. Plain and serialisable. A test asserts it survives JSON.parse(JSON.stringify(...)).
  • deps is the label model and verification trigger. Exactly the things that can't be serialised, and the ones Added JobQueue and made Ghost's background jobs serialisable  #29170 resolves from injected getters at run time.

It's dispatched through the job manager's own data parameter rather than captured in a closure.

So the change on your side is swapping the dispatch for an enqueue. The job body doesn't move.

Two things worth deciding together rather than separately:

Where the spool lives. It's in the temp directory because its durability only has to match the queue's, and today's queue is in-process. A restart loses the job anyway. Once a durable adapter lands, the spool has to become as durable as the queue. It shouldn't go back into the site's content directory, which is how this went unnoticed for five years.

Why a spool rather than the uploaded CSV. Carrying the upload and re-parsing it in the job is just as serialisable, and avoids a second on-disk format. But the upload is deleted when the response finishes, and the opt-out for that (req.disableUploadClear) is checked in four places and set in none.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Refactored members CSV importing to parse uploads into in-memory rows and metadata, removing prepared-file creation and related storage and timezone configuration. Updated inline and queued processing to consume parsed rows, with queued jobs using temporary NDJSON spooling. Expanded unit, end-to-end, and legacy integration coverage for mappings, labels, subscriptions, header-only uploads, and queued imports.

Suggested labels: preview

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately captures the main change: only queued member imports now write a file.
Description check ✅ Passed The description is clearly related to the CSV importer changes and matches the implemented behavior.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ber-3811-remove-importer-disk-round-trip

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nx-cloud

nx-cloud Bot commented Jul 20, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 4e107d7

Command Status Duration Result
nx run ghost:test:ci:integration ✅ Succeeded 3m 5s View ↗
nx run ghost:test:integration ✅ Succeeded 2m 18s View ↗
nx run ghost:test:legacy ✅ Succeeded 3m 21s View ↗
nx run ghost:test:e2e ✅ Succeeded 2m 4s View ↗
nx run-many -t test:unit -p ghost ✅ Succeeded 30s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded 19s View ↗
nx run-many -t lint -p ghost,ghost-monorepo ✅ Succeeded 19s View ↗
nx run-many --target=build --projects=tag:publi... ✅ Succeeded 1s View ↗
nx run @tryghost/admin:build ✅ Succeeded 3s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-07-20 22:11:54 UTC

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.72917% with 85 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.48%. Comparing base (b825daf) to head (4e107d7).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
.../services/members/importer/members-csv-importer.js 55.72% 84 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #29453      +/-   ##
==========================================
- Coverage   74.53%   74.48%   -0.06%     
==========================================
  Files        1602     1602              
  Lines      140399   140512     +113     
  Branches    17066    17066              
==========================================
+ Hits       104652   104657       +5     
- Misses      34693    34836     +143     
+ Partials     1054     1019      -35     
Flag Coverage Δ
e2e-tests 76.55% <55.72%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rob-ghost
rob-ghost marked this pull request as ready for review July 20, 2026 15:11
@rob-ghost
rob-ghost force-pushed the fix/ber-3811-remove-importer-disk-round-trip branch from a89dfc1 to f477791 Compare July 20, 2026 16:29
@rob-ghost
rob-ghost marked this pull request as draft July 20, 2026 17:58
@rob-ghost
rob-ghost force-pushed the fix/ber-3811-remove-importer-disk-round-trip branch 2 times, most recently from d29e6db to b194b52 Compare July 20, 2026 19:46
@rob-ghost rob-ghost changed the title Removed the disk round-trip from the members CSV importer Changed the members CSV importer to only write a file for queued imports Jul 20, 2026
@rob-ghost
rob-ghost marked this pull request as ready for review July 20, 2026 20:00

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@ghost/core/core/server/services/members/importer/members-csv-importer.js`:
- Around line 533-546: Update the _addJob call in the members import flow to set
offloaded to true so runImportJob executes off-thread and the request returns
promptly. Preserve spool ownership and cleanup by ensuring spoolPath is removed
from the job execution path after the job completes or fails, rather than
relying only on the pre-enqueue catch block.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 99a87024-3645-4d95-a5c5-7881dad83393

📥 Commits

Reviewing files that changed from the base of the PR and between f477791 and b194b52.

⛔ Files ignored due to path filters (4)
  • ghost/core/test/unit/server/services/members/importer/fixtures/duplicate-subscribed-target.csv is excluded by !**/*.csv
  • ghost/core/test/unit/server/services/members/importer/fixtures/unmapped-column.csv is excluded by !**/*.csv
  • ghost/core/test/utils/fixtures/csv/members-headers-only.csv is excluded by !**/*.csv
  • ghost/core/test/utils/fixtures/csv/members-mapped-subscribed.csv is excluded by !**/*.csv
📒 Files selected for processing (4)
  • ghost/core/core/server/services/members/importer/members-csv-importer.js
  • ghost/core/core/server/services/members/service.js
  • ghost/core/test/e2e-api/admin/members-importer.test.js
  • ghost/core/test/unit/server/services/members/importer/members-csv-importer.test.js
💤 Files with no reviewable changes (1)
  • ghost/core/core/server/services/members/service.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • ghost/core/test/e2e-api/admin/members-importer.test.js

@rob-ghost
rob-ghost force-pushed the fix/ber-3811-remove-importer-disk-round-trip branch 4 times, most recently from 45120b0 to 09b8a11 Compare July 20, 2026 21:48
ref BER-3811

prepare() re-serialised the parsed CSV, wrote it to content/data, and perform()
read it straight back off disk. Nothing ever deleted that file, so every import
left one behind — on Ghost(Pro) that lands on a shared, persistent volume with
no cleanup and no per-site quota. prepare() now returns the parsed rows and
perform() takes them, and a file is written only where one is genuinely needed:
a queued import outlives the request that started it, and the uploaded CSV is
deleted the moment the response finishes, so its rows are spooled to the system
temp directory and the job that reads them deletes them, whether it succeeded
or failed. The spool is JSON rather than re-serialised CSV because CSV drops
any column outside the member model's own set and flattens every value to a
string, so reading it back would need the same coercion the parse already did.

The caller's header mapping is composed with the model's field mapping before
parsing rather than after, because the parser types a value by the name it
lands on, so a blank subscribed cell still reads as subscribed. Where two
columns claim one model field the first now wins, which is a chosen rule rather
than the previous behaviour.

Three latent bugs go with it: a CSV with headers but no data rows returned a
500, a prepared file leaked on every import, and a global label containing a
comma was split into two labels by the serialise and re-parse. Two changes to
stored data are deliberate: values were previously persisted with the CSV
formula-escape apostrophe still attached, so a member named -5 was stored as
'-5, and columns the caller maps but the member model does not know now reach
the import instead of being erased.
@rob-ghost
rob-ghost force-pushed the fix/ber-3811-remove-importer-disk-round-trip branch from 09b8a11 to 4e107d7 Compare July 20, 2026 21:58
@rob-ghost
rob-ghost requested a review from allouis July 21, 2026 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants