fix(server): sync files to disk

[uhthomas]

Description

Ensure that all files are flushed after they've been written.

At current, files are not explicitly flushed to disk, which can cause data corruption. In extreme circumstances, it's possible that uploaded files may not ever be persisted at all.

The consequence of this is that it may reduce throughput, but Immich should prioritise data integrity over performance.

Refs: #26720, more?

[mmomjian]

The consequence of this is that it may reduce throughput

is this something that should be benchmarked before we make this a non configurable change? Not all storage backends benefit from or need this syscall.

[uhthomas]

IMO, no. This is absolutely necessary. Every storage medium needs and benefits from this.

[mmomjian]

Perhaps there is backend discussion I’m not aware of. We’ve come a long way with minimal (?) issues from the current setup. Doing this without batching could cause massive overhead on simple operations like bulk asset tagging and XMP writes.

[uhthomas]

I would be more concerned if every single write was subject to sync, but syncing a whole file is fine. It does not make any sense to write dozens or hundreds of files and not have actually persisted them, especially when the current assumption is that they have been.

We have seen plenty of consistency issues, especially with unstable storage like SMB, NFS or USB. They are the most likely to benefit from this.

This is incredibly important. Immich shouldn't just yolo anything - people rely on Immich for backing up photos and videos and expect the backed up data the be durable.

[alextran1502]

This PR should be benchmarked on the impact when ingesting large library so we can clearly see the pros and cons

[uhthomas]

Sure.

The impact should be minimal, but for uploads this is really non negotiable. It does not make any sense for Immich to persist asset data to the DB a tell clients an upload was successful without syncing the file to the filesystem.

[uhthomas]

I ran a quick bench on a 990 Pro and:

Size	No sync	flush: true	Separate fdatasync
1KB	0.068ms	0.082ms (+19%)	0.119ms (+73%)
100KB	0.096ms	0.114ms (+19%)	0.142ms (+48%)
1MB	0.330ms	0.319ms (~0%)	0.420ms (+27%)
10MB	3.29ms	3.23ms (~0%)	3.15ms (~0%)

So, no impact for large files on reasonable storage. I imagine there will be more of an impact on slower storage, but what other option is there? Lose data?

[uhthomas]

On really bad storage, like a cheap USB stick that I bought 10 years ago, it's got quite a big impact. Not really surprising though.

Size	No sync	flush: true	Separate fdatasync
1KB	0.65ms	7.42ms (+1042%)	8.17ms (+1158%)
100KB	0.91ms	9.38ms (+930%)	9.10ms (+900%)
1MB	1.76ms	246.28ms (+13893%)	71.60ms (+3968%)
10MB	14.64ms	563.50ms (+3749%)	820.62ms (+5505%)

[alextran1502]

I want to take a step back and consider whether enforcing this at the application level is the right approach, given the performance impact.

The failure mode in #26720 could have other root causes, NFS mounted with async, unstable USB storage, etc. In those cases, fdatasync per write is compensating for a broken storage configuration rather than fixing the actual problem.

Data integrity is important, but I think we should:

• Scope the sync to the upload path only, that's where data loss is unrecoverable.
• Thumbnails, transcodes, and XMP sidecars can all be regenerated.
• Benchmark this on a bulk ingest over NFS, USB HDD, and local SSD before making it a non-configurable default.
• Consider whether better documentation on recommended mount options (e.g., NFS sync, journaling filesystems for USB) is the more appropriate fix.

Can we get numbers on the benchmarking first?

[uhthomas]

@alextran1502 I considered allowing non sync writes for stuff like thumbnails and metadata, but we aren't doing any sort of integrity checking there at all. It is impossible to deterministically find and fix / regenerate thumbnails, etc which are corrupt because of this.

Some filesystems are slow, and will hold data in a cache for 30 seconds or minutes. As a user who has terabytes of photos and videos, not syncing to the filesystem properly makes me incredibly nervous and I hope that we can agree that data integrity and durability should be prioritised above everything else. If the performance is worse because we're actually handling data safely, then so be it. That's how it's supposed to be, and that's the philosophy that other systems (like Ceph) take.

We have to enforce this within Immich. It doesn't matter how great your storage medium is if the writes aren't being flushed to disk. Any number of things could prevent the write from actually being persisted and Immich would have no idea.

[uhthomas]

If you want to try it on your own storage, use:

BENCH_DIR=/home/thomas/code/github.com/immich-app/immich/mnt/immich-datasync-bench npx vitest bench --config test/vitest.config.mjs src/benchmarks/datasync.bench.ts

server/src/benchmarks/datasync.bench.ts

❯ cat server/src/benchmarks/datasync.bench.ts
import fs from 'node:fs/promises';
import os from 'node:os';
import path from 'node:path';
import { bench, describe } from 'vitest';

const sizes = [
  { name: '1KB', bytes: 1024 },
  { name: '100KB', bytes: 100 * 1024 },
  { name: '1MB', bytes: 1024 * 1024 },
  { name: '10MB', bytes: 10 * 1024 * 1024 },
];

const tmpDir = process.env.BENCH_DIR || path.join(os.tmpdir(), 'immich-datasync-bench');

let counter = 0;
const nextPath = () => path.join(tmpDir, `bench-${counter++}`);

// pre-allocate buffers
const buffers = new Map<number, Buffer>();
for (const { bytes } of sizes) {
  buffers.set(bytes, Buffer.alloc(bytes, 0x42));
}

describe('writeFile', async () => {
  await fs.mkdir(tmpDir, { recursive: true });

  for (const { name, bytes } of sizes) {
    const buf = buffers.get(bytes)!;

    bench(`${name}`, async () => {
      const p = nextPath();
      await fs.writeFile(p, buf);
    });

    bench(`${name} + flush: true`, async () => {
      const p = nextPath();
      await fs.writeFile(p, buf, { flush: true });
    });

    bench(`${name} + fdatasync`, async () => {
      const p = nextPath();
      await fs.writeFile(p, buf);
      const handle = await fs.open(p, 'r');
      try {
        await handle.datasync();
      } finally {
        await handle.close();
      }
    });
  }
});

[uhthomas]

To add some context to the benchmark numbers, also consider that the majority of the time and work for uploads will be the network. A few milliseconds, or a few hundred at worst is negligible.

[alextran1502]

Integrity check is in the works #24205, I will run some benchmarks on my own. Can you help by enforcing it only on the upload path?

[danieldietzler]

FWIW I think we all agree on that we want to have certainty about not losing files. Proper integrity checks are already in the works though, which should also help with giving confidence. I think for Immich particularly the reality is that many people are running it on slower storage (especially not NVMes), in which case performance impacts are very significant. The truth is also that most people don't run CEPH in their homelab, and instead pick an FS with less performance impact at the expense of worse persistence guarantees.

As Alex mentioned, it's quite likely that most of the issues we have seen come from either bad network mounts, or FSs lying about persistence status. There's only so much an fsync will do if the FS doesn't play along, too.

So, regardless we would want to have higher integrity around files/file persistence and surface lost/failed writes to the user

[uhthomas]

I don't think integrity checks are a substitute for syncing files to disk correctly.

I strongly believe that we should be syncing all writes, so I'm keen to hear the results from your findings. I/O will be the most expensive part of these operations and syncing should be minimal overhead.

[uhthomas]

If filesytems are already lying about whether things have been persisted, then explicitly syncing won't make any difference in those cases (or maybe it will! and it will prevent corruption).

I really cannot see how non-durable data is desirable at all. Much like how Postgres gets really fast when writing without fsync, it's kind of pointless and dangerous. Postgres cares about data integrity for a reason.

[uhthomas]

There's also no reason Immich shouldn't work with network mounts or precarious setups if the only thing preventing it from doing so is syncing properly. If we try to sync the data and it fails, then users will get proper errors rather than silent corruption.

[alextran1502]

I/O will be the most expensive part of these operations and syncing should be minimal overhead.

@uhthomas

Yeah. I will report back later this week. I am also very hopeful that the sync overhead is minimal in the grand scheme of things, but we will see. I have a very common setup for most use cases.

Can you help by just limiting the scope of sync to the upload path?

[uhthomas]

I tried this on an old WD Red 3TB, and the overhead for large writes is small, but the overhead for small writes is large. This is unsurprising given how few IOPS HDDs have.

Size	No sync	flush: true	Separate fdatasync
1KB	0.068ms	48.66ms (+71,459%)	56.69ms (+83,268%)
100KB	1.38ms	61.22ms (+4,336%)	59.17ms (+4,187%)
1MB	10.17ms	71.21ms (+600%)	72.32ms (+611%)
10MB	75.77ms	162.45ms (+114%)	154.55ms (+104%)
100MB	824.20ms	879.15ms (+7%)	883.46ms (+7%)

[uhthomas]

I agree that uploads are the most important, though let's wait until you've tried it before we remove the other syncs. I really believe they are important too.

[uhthomas]

It's still not merged yet @platima - your input would be greatly appreciated.

[platima]

It's still not merged yet @platima - your input would be greatly appreciated.

Noted, will let it finish then!

Sorry, saw a dozen emails and what looked like an approval 😅

[uhthomas]

Tested the latest changes on the preview instance and seems to work as expected.

[platima]

TrueNAS Scale 24.04 with 3x 20TB WD Ultrastar DC HC560's in RAIDZ1
Left the SSD enabled as it's L2ARC, so read cache not write - will be useful for testing #26939 which I intend to do within 24 hours too

Fresh Immich 2.5.6 stack deployed directly on the NAS as the Docker host (Xeon E5-2620 v3 CPU - 6C/12T, 32GB ECC RAM)
Machine Learning disabled same as done above
2,500x JPGs ranging in size from 1.5MB to 13MB ea, total 7.6GB, over 1GbE
Upload source was my RAID1 WD Black SSDs so no latency on the source

I compared the oldest and newest 'birth' date of JPG files on the disk in library/uploads: the delta was 13m 55s average across three runs. I then wiped files and database from disk, re-deployed with #26881. The birth delta average was 17m 59s.

So that difference is +29%.

However

Most of that testing was yesterday. Today I have done two more tests with the more recent commits, as I'm testing via the PR tag not the commit sha256sum, and the deltas were 19m 36s and 19m23s, making it a 40% performance penalty it seems.

Realistically though, we're talking about 18-20 minutes instead of ~14 minutes. That is also only approximately 0.1338 seconds per file. For guaranteed data integrity, I am more than happy to forgo those extra few minutes.

-P

[uhthomas]

Thank you for testing, it's hugely appreciated. Would you mind just clarifying which commit you used for those tests? I know you were doing it by tag, but I want to be clear on whether those results are from just syncing uploaded files or everything.

[platima]

No worries.

I cannot be 100% sorry, as I was testing using the PR tag not the commit tag, however, the most recent ~19 minute upload was less than 1 hour ago. The ~17 minute uploads were both about 16 and 4 hours ago.

I'm in and out of the office all day but can re-test with specific commits if you prefer.

Cheers

[uhthomas]

Thanks - that does surprise me. { flush: true } on the write stream should be better than opening a new file descriptor and flushing it.

Is your docker configured to always pull the image, even if it's unchanged? Were you restarting the server and getting new changes between tests?

[platima]

Yep the process was:

1. Deploy stack
2. Turn off ML
3. Upload images
4. Shutdown stack
5. Delete 'library' and 'database' folders
6. Update stack env from v2.5.6 to pr-26881
7. Tick 'always pull the image' as deploying
8. Turn off ML
9. Upload images again

Want to give me two specific commits and I'll re-run with just those?

[uhthomas]

You've already done a lot, you don't have to.

If you are interested, these two would be good to compare.

• fdatasync: 0ab057f
• flush: b108f04

[platima]

All good. I want to use Immich as a live platform for both myself, and my partner, instead of Google Drive or OneDrive, and we have 20+ years of photos and videos, so data integrity is hugely important.

Starting those tests now. Hoping to get them done within 3-4 hours, but that depends on phone calls and meetings.

Cheers

[mertalev]

Those are very interesting results. I have to say a 30-40% penalty is unfortunate considering that a mobile client has limited time to upload, but I'm not sure if it can be improved.

[platima]

@mertalev I agree to a limited amount.

Mobile has limited time-frame, but I think mobile is more likely bound by upload capacity (in Australia it's common to only have ~10-40Mbps upload on home internet), and I doubt that the average user would be uploading 2,500 images at once.

The total impact average was only ~0.1338 seconds per file across a range of file sizes, so as an indented end-user, I would consider that worthwhile.

Re-testing two specific commits nearly finished.

[platima]

Same files and setup re-tested between 9:53am and 11:30am. The numbers are very much the same.

v2.5.6
13m 49s

flush commit b108f04
19m 17s
+5m 28s / 39.6%

fdatasync commit 0ab057f:
19m 45s
+5m 56s / 42.9% from v2.5.6
+28s / +2.4% from flush commit b108f04

[uhthomas]

Thank you so much @platima!!

[platima]

Thank you so much @platima!!

Any time @uhthomas - thank YOU for adding code that hardens up the data integrity. Fast ain't always better than slow you know.

Oh and thanks and your assist on the other one (bed time shortly here, so I'll get back to that one tomorrow).

[ReanimationXP]

I have to say, reading this thread and the back and forth is very concerning to me and is continuing to shake my already-shaken confidence in using Immich as a backup platform. The existence of the "clear up space" feature alone should completely nullify arguments that Immich has client-side originals to check against. Assuming such a thing is insane. I want to give a massive thank you to @uhthomas for continuing to reply and insisting that this be done correctly. I absolutely do not care if it took 300% the amount of backup time, data integrity is absolutely, positively, required full stop, or I will not be using the product. I can't believe there was even pushback on this over performance of all things. If anything, you should be focusing on integrity first and worrying about optimizations later. Users are trusting you with what is arguably their most important data - their precious memories, and when backing up a huge library we should NOT be having to spot check that the app did what it claims to do. For most people like me with massive libraries it's impossible or impractical to check that the app is doing backups correctly unless it reports a failure to me. All I have to say here is wow @ the project leaders, and a massive thank you to @uhthomas. You may not have intended for your arguments to come across as refusal or pushback, but I don't see how weighing performance against data integrity at all can be interpreted any other way.

[platima]

Well said @ReanimationXP - seconded, and kudos.