Fix write / read metadata race on local filesystem. #5698

shaunrd0 · 2025-11-21T22:12:17Z

This fixes a race condition when writing and reading metadata on separate threads that is observed only on local filesystems. As @teo-tsirpanis suggested in the story description of CORE-73, we temporarily use a .tmp extension when flushing array metadata to disk until the write is completed.

TYPE: BUG
DESC: Fix write / read metadata race on local filesystem.

shaunrd0 · 2025-11-21T22:15:07Z

format_spec/metadata.md


 ## Metadata File

+Metadata files with the `.tmp` extension are used for local filesystems only, indicating the metadata is still being flushed to disk.


Curious what others think of using a .part extension instead. Either way is fine by me so by all means we can stick with .tmp, just something that came to mind while reviewing this myself.

.tmp has an unambiguous meaning, but you can add another extension before it if you want.

Also, this is an implementation detail that should not be mentioned in the spec. If we haven't already, we should state that readers should ignore files with names that don't follow the respective format.

Done, I updated the format spec in 523230a (did not make changes to the extension)

teo-tsirpanis · 2025-11-21T22:26:00Z

tiledb/sm/array/array_directory.cc

+        uri_.join_path(constants::array_metadata_dir_name), &unfiltered_uris));
+    for (const auto& unfiltered_uri : unfiltered_uris) {
+      const auto& uri = unfiltered_uri.to_string();
+      if (uri.ends_with(constants::temp_file_suffix)) {


We should check if the file does not have a timestamped name that ends in .tdb.

Done 523230a

teo-tsirpanis · 2025-11-21T22:27:31Z

tiledb/sm/filesystem/posix.cc

+    // Do not attempt to retrieve file size for temporary metadata files that
+    // are still flushing to disk.
+    const bool temp_metadata =
+        URI(abspath)
+            .parent_path()
+            .remove_trailing_slash()
+            .to_string()
+            .ends_with(constants::array_metadata_dir_name) &&
+        abspath.ends_with(constants::temp_file_suffix);


The VFS is not an appropriate place to do such filtering.

This was removed in 523230a, it's purpose was to avoid opening the file to determine the size that was later thrown out by VFS::ls. 9b2e174 updated VFS::ls instead to avoid this by not gathering file sizes since they aren't needed.

teo-tsirpanis · 2025-11-21T22:59:32Z

tiledb/sm/metadata/metadata.cc

+  if (uri.is_file()) {
+    // For LocalFS use a .tmp extension until write is flushed.
+    URI metadata_uri(get_uri(uri).to_string() + ".tmp");
+    GenericTileIO::store_data(resources, metadata_uri, tile, encryption_key);
+    resources.vfs().move_file(metadata_uri, get_uri(uri));
+  } else {
+    // Create a metadata file name
+    GenericTileIO::store_data(resources, get_uri(uri), tile, encryption_key);
+  }


Not only metadata is susceptible to these race conditions; I can think of group members, array schema evolution, and array creation. It's OK if we want to prioritize metadata due to the increased impact.

I would have preferred to move this temporary file logic to the VFS, which would have required a pretty significant refactoring, but doing it this way is not very disruptive and OK for now.

teo-tsirpanis · 2025-11-25T19:07:43Z

tiledb/sm/filesystem/vfs.cc

  stats_->add_counter("ls_num", 1);

-  for (auto& fs : ls_with_sizes(parent)) {
+  for (auto& fs : ls_with_sizes(parent, false)) {


Why do this? It looks like a potential optimization (on POSIX only) that is unrelated to this PR.

This is to prevent us from reopening the file to determine the file size, since the writing thread will move the file when it's done flushing the metadata to disk causing a race condition when opening the array. This was the plan we discussed during sync this morning?

Oh that's what it was about. Another option is to either report zero size or skip from listing, if the second open fails. Having to change all VFSes just for POSIX is IMO too much.

I moved this to the LocalFilesystem base class so it only impacts windows / posix. We can do the try/catch pattern if you prefer though, let me know what you think.

teo-tsirpanis · 2025-12-01T19:31:29Z

tiledb/sm/filesystem/vfs.cc

+  if (parent.is_file()) {
+    parallel_sort(
+        compute_tp_,
+        entries.begin(),
+        entries.end(),
+        [](const directory_entry& l, const directory_entry& r) {
+          return l.path().native() < r.path().native();
+        });
+  }


This is pre-existing, but curious; is there code in this repo that assumes that paths are sorted? Sorting paths when listing is not a good idea in general, and precludes incrementally reading them.

The only code expecting sorted results that I'm aware of is test code. For example here's the CI run with the failures that this produced when I accidentally bypassed sorting in c2e1816

https://github.com/TileDB-Inc/TileDB/actions/runs/19717674222/job/56493579308#step:14:32027

teo-tsirpanis · 2025-12-01T19:31:45Z

tiledb/sm/filesystem/local.cc

+  // Noop if the parent ` path ` is not a directory, do not error out.
+  if (!is_dir(URI(path))) {
+    return Status::Ok();
+  }
+


Should this be in ls_with_sizes?

Yep, thanks! Fixed

This hits the retry loop sometimes, but needs to be more reliable.

VFS::ls is non-virtual and its signature varies across filesystems. This also allows us to use a single implementation instead of duplicating the logic.

This wasn't meant to make it into previous commits.

Path canonicalization on Windows strips disallowed characters from the filename, in this case `.` was stripped causing the failure. https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions

…adata-readerwriter-race-on-local-file-system-vfs

shaunrd0 force-pushed the shaunreed/core-73-metadata-readerwriter-race-on-local-file-system-vfs branch from b885040 to 2b03211 Compare November 21, 2025 22:12

shaunrd0 commented Nov 21, 2025

View reviewed changes

teo-tsirpanis reviewed Nov 21, 2025

View reviewed changes

teo-tsirpanis reviewed Nov 25, 2025

View reviewed changes

teo-tsirpanis reviewed Dec 1, 2025

View reviewed changes

shaunrd0 added 15 commits December 1, 2025 15:07

WIP fix.

29122b1

Regression test WIP.

8c0f30e

This hits the retry loop sometimes, but needs to be more reliable.

Skip partially written metadata files.

18171db

Update test.

50970d6

Add debug logging when skipping partial metadata.

0d747d0

Changes from review.

dca3901

Add flag for disabling file size on localfs.

965ec63

VFS::ls is non-virtual and its signature varies across filesystems. This also allows us to use a single implementation instead of duplicating the logic.

Use std::all_of.

ac56230

Add test for is_timestamped_name.

885d481

Fix Windows werror.

bfe511a

Revert changes from testing locally.

d8ec16b

This wasn't meant to make it into previous commits.

Debug failing windows UT.

ce1b545

Move boolean variant to LocalFilesystem base class.

c2e1816

Sort results from ls_with_sizes LocalFS helper.

21aed9c

Debug Windows failures.

e7c356b

shaunrd0 force-pushed the shaunreed/core-73-metadata-readerwriter-race-on-local-file-system-vfs branch from e56fd7e to e7c356b Compare December 1, 2025 22:15

shaunrd0 added 2 commits December 2, 2025 14:31

Fix windows failures.

c8fcade

Path canonicalization on Windows strips disallowed characters from the filename, in this case `.` was stripped causing the failure. https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions

Merge remote-tracking branch 'origin/main' into shaunreed/core-73-met…

3fd0a8e

…adata-readerwriter-race-on-local-file-system-vfs


		## Metadata File

		Metadata files with the `.tmp` extension are used for local filesystems only, indicating the metadata is still being flushed to disk.

Fix write / read metadata race on local filesystem. #5698

Are you sure you want to change the base?

Fix write / read metadata race on local filesystem. #5698

Conversation

shaunrd0 commented Nov 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants