Address shadowing divergence in reftest, update semantics doc (#1201)

This commit addresses a case where MP model and property tests diverge (#1066). The issue was caused by the reference not correctly implementing the shadowing order defined in [#4f8cf0b](4f8cf0b). This commit fixes the reference model, and clarifies the semantics arising from concurrent MPUs. This is not a breaking change, as it only impacts the reference tests. This does not need a Changelog entry, as the change does not impact Mountpoint's behaviour. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the [Developer Certificate of Origin (DCO)](https://developercertificate.org/). --------- Signed-off-by: Christian Hagemeier <chagem@amazon.com>
awslabs · Dec 17, 2024 · 631e6e0 · 631e6e0 · github-actions · Dec 17, 2024
1 parent d5b36e8
commit 631e6e0
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 3 deletions.
diff --git a/doc/SEMANTICS.md b/doc/SEMANTICS.md
@@ -148,8 +148,13 @@ S3 places fewer restrictions on [valid object keys](https://docs.aws.amazon.com/
 
   * `blue`
   * `blue/image.jpg`
+
+  then mounting your bucket would give a file system with a `blue` directory, containing the file `image.jpg`. 
+The `blue` object will not be accessible. Deleting the key `blue/image.jpg` will remove the `blue` directory, and cause the `blue` file to become visible.
 
-  then mounting your bucket would give a file system with a `blue` directory, containing the file `image.jpg`. The `blue` object will not be accessible. Deleting the key `blue/image.jpg` will remove the `blue` directory, and cause the `blue` file to become visible.
+Additionally, remote directories will always shadow local directories or files. 
+Thus Mountpoint shadows directory entries in the following order, where the first takes precedence: remote directories, any local state, remote files.
+For example, if you create a directory i.e. `blue/` and a conflicting object with key `blue` appears in the bucket, the local directory will still be accessible.  
 
 We test Mountpoint against these restrictions using a [reference model](https://github.com/awslabs/mountpoint-s3/blob/main/mountpoint-s3/tests/reftests/reference.rs) that programmatically encodes the expected mapping between S3 objects and file system structure.
 
@@ -270,4 +275,3 @@ Mountpoint provides strong read-after-write consistency for new object creation
 * A process deletes an existing object from your S3 bucket using another client, and then tries to open the object with Mountpoint and read from it. The open operation will fail.
 * A process deletes an existing object from your S3 bucket, using either Mountpoint or another client, and then lists the directory the object was previously in with Mountpoint. The object will not appear in the list.
 * A process deletes an existing object from your S3 bucket using another client, and then queries the object’s metadata with Mountpoint using the `stat`` system call. The returned metadata could reflect the old object for up to 1 second after the DeleteObject request.
-
diff --git a/mountpoint-s3/tests/reftests/harness.rs b/mountpoint-s3/tests/reftests/harness.rs
@@ -1290,4 +1290,27 @@ mod mutations {
             0,
         )
     }
+
+    /*
+     Ensure that local files are shadowed by the remote directories.
+    */
+    #[test]
+    fn regression_put_shadowing_new_local_file() {
+        run_test(
+            TreeNode::Directory(BTreeMap::from([])),
+            vec![
+                Op::CreateFile(
+                    ValidName("a".into()),
+                    DirectoryIndex(0),
+                    FileContent(0, FileSize::Small(0)),
+                ),
+                Op::PutObject(
+                    DirectoryIndex(0),
+                    Name("a/b".into()),
+                    FileContent(0, FileSize::Small(0)),
+                ),
+            ],
+            0,
+        )
+    }
 }
diff --git a/mountpoint-s3/tests/reftests/reference.rs b/mountpoint-s3/tests/reftests/reference.rs
@@ -81,7 +81,8 @@ impl MaterializedReference {
     /// Try to add a new node to the tree. Returns whether the node was added.
     ///
     /// If the path already exists it will be overwritten, unless both the existing and new nodes
-    /// are directories. If any intermediate directory is not present, the node won't be added and
+    /// are directories or the new node is a file and were to override a directory.
+    /// If any intermediate directory is not present, the node won't be added and
     /// this function returns false.
     ///
     /// TODO: today our semantics makes local files/directories invisible if any of their ancestors
@@ -111,6 +112,12 @@ impl MaterializedReference {
                         break;
                     }
                 }
+                // If a directory of this name exists, ignore any local file
+                if let Some(node) = children.get(dir) {
+                    if node.node_type() == NodeType::Directory {
+                        return false;
+                    }
+                }
                 let new_node = match typ {
                     NodeType::Directory => Node::Directory {
                         children: BTreeMap::new(),