NSFS | Fix Bug | Race between list object and delete object during op…

…eration

1. Replace the function stat_ignore_eacces with stat_if_exists and add ignore of ENOENT and ENOTDIR (in addition to EACCES with a flag of config.EACCES_IGNORE_ENTRY).
2. Call stat on the entry_path one time and save is instead of calling it in concurrency so that we can be sure that the value was not deleted from the result array. Move the stat call before results.push(r) and results.splice(pos, 0, r) as they both add a result to the array.
3. Add a comment before the check_access, see also in PR description of Added check_access() call to list objects #6576.

This change is continuing PR #8751 by adding a case to ignore a stat failure entry.

Signed-off-by: shirady <[email protected]>

config.js

@@ -913,6 +913,9 @@ config.ANONYMOUS_ACCOUNT_NAME = 'anonymous';
 
 config.NFSF_UPLOAD_STREAM_MEM_THRESHOLD = 8 * 1024 * 1024;
 
+// we want to change our handling related to EACCESS error
+config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES = true;
+
 ////////////////////////////
 // NSFS NON CONTAINERIZED //
 ////////////////////////////

src/native/fs/fs_napi.cpp

@@ -752,6 +752,11 @@ struct FSWrapWorker : public FSWorker
 
 /**
  * Stat is an fs op
+ * 
+ * Note: this stat operation contains the system call of open.
+ *       Currently, we use it in list objects, but might want to create a different stat call
+ *       (or add changes inside this) to avoid permission check during list objects
+ *       while we stat each file (to avoid EACCES error)
  */
 struct Stat : public FSWorker
 {

src/sdk/namespace_fs.js

@@ -718,11 +718,17 @@ class NamespaceFS {
                     if (!delimiter && r.common_prefix) {
                         await process_dir(r.key);
                     } else {
-                        if (pos < results.length) {
-                            results.splice(pos, 0, r);
-                        } else {
-                            const stat = await native_fs_utils.stat_ignore_eacces(this.bucket_path, r, fs_context);
-                            if (stat) {
+                        const entry_path = path.join(this.bucket_path, r.key);
+                        // If entry is outside of bucket, returns stat of symbolic link
+                        const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path));
+                        const stat = await native_fs_utils.stat_if_exists(fs_context, entry_path,
+                            use_lstat, config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES);
+                        if (stat) {
+                            r.stat = stat;
+                            // add the result only if we have the stat information
+                            if (pos < results.length) {
+                                results.splice(pos, 0, r);
+                            } else {
                                 results.push(r);
                             }
                         }
@@ -764,6 +770,11 @@ class NamespaceFS {
                     await insert_entry_to_results_arr(r);
                 };
 
+                // our current mechanism - list the files and skipping inaccessible directory (invisible in the list).
+                // We use this check_access in case the directory is not accessible inside a bucket.
+                // In a directory if we don’t have access to the directory, we want to skip the directory and its sub directories from the list.
+                // We did it outside to avoid undefined values in the cache.
+                // Note: It is not the same case as a file without permission.
                 if (!(await this.check_access(fs_context, dir_path))) return;
                 try {
                     if (list_versions) {
@@ -885,13 +896,6 @@ class NamespaceFS {
 
             const prefix_dir_key = prefix.slice(0, prefix.lastIndexOf('/') + 1);
             await process_dir(prefix_dir_key);
-            await Promise.all(results.map(async r => {
-                if (r.common_prefix) return;
-                const entry_path = path.join(this.bucket_path, r.key);
-                //If entry is outside of bucket, returns stat of symbolic link
-                const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path));
-                r.stat = await nb_native().fs.stat(fs_context, entry_path, { use_lstat });
-            }));
             const res = {
                 objects: [],
                 common_prefixes: [],

src/test/unit_tests/jest_tests/test_nsfs_concurrency.test.js

@@ -6,7 +6,7 @@ const P = require('../../../util/promise');
 const fs_utils = require('../../../util/fs_utils');
 const NamespaceFS = require('../../../sdk/namespace_fs');
 const buffer_utils = require('../../../util/buffer_utils');
-const { TMP_PATH } = require('../../system_tests/test_utils');
+const { TMP_PATH, TEST_TIMEOUT } = require('../../system_tests/test_utils');
 const { crypto_random_string } = require('../../../util/string_utils');
 const endpoint_stats_collector = require('../../../sdk/endpoint_stats_collector');
 
@@ -27,18 +27,20 @@ function make_dummy_object_sdk(nsfs_config, uid, gid) {
 }
 
 const DUMMY_OBJECT_SDK = make_dummy_object_sdk(true);
+
+const tmp_fs_path = path.join(TMP_PATH, 'test_nsfs_concurrency');
+
+const nsfs = new NamespaceFS({
+    bucket_path: tmp_fs_path,
+    bucket_id: '1',
+    namespace_resource_id: undefined,
+    access_mode: undefined,
+    versioning: 'DISABLED',
+    force_md5_etag: false,
+    stats: endpoint_stats_collector.instance(),
+});
+
 describe('test nsfs concurrency', () => {
-    const tmp_fs_path = path.join(TMP_PATH, 'test_nsfs_concurrency');
-
-    const nsfs = new NamespaceFS({
-        bucket_path: tmp_fs_path,
-        bucket_id: '1',
-        namespace_resource_id: undefined,
-        access_mode: undefined,
-        versioning: 'DISABLED',
-        force_md5_etag: false,
-        stats: endpoint_stats_collector.instance(),
-    });
 
     beforeEach(async () => {
         await fs_utils.create_fresh_path(tmp_fs_path);
@@ -68,5 +70,101 @@ describe('test nsfs concurrency', () => {
         }
         await P.delay(5000);
         expect(res_etags).toHaveLength(15);
-    }, 6000);
+    }, TEST_TIMEOUT);
+
+    test_list_and_delete({
+        test_name: 'list objects and delete an object during it - random deletion',
+        bucket_name: 'bucket1',
+        num_of_objects_to_upload: 5,
+        expected_num_of_object_in_list: 4,
+        key_to_delete: `my-key-${random_integer(1, 5)}`,
+        iterations: 5,
+    });
+
+    test_list_and_delete({
+        test_name: 'list objects and delete an object during it - delete the last object',
+        bucket_name: 'bucket2',
+        num_of_objects_to_upload: 1000,
+        expected_num_of_object_in_list: 999,
+        key_to_delete: `my-key-1000`,
+        iterations: 1
+    });
 });
+
+    /**
+     * @param {{
+    *      test_name: string,
+    *      bucket_name: string,
+    *      num_of_objects_to_upload: number,
+    *      expected_num_of_object_in_list: number,
+    *      key_to_delete?: string,
+    *      iterations?: number,
+    * }} params
+    */
+   function test_list_and_delete({
+       test_name,
+       bucket_name,
+       num_of_objects_to_upload,
+       expected_num_of_object_in_list,
+       key_to_delete,
+       iterations = 1
+   }) {
+
+    it(test_name, async () => {
+        await _upload_objects(bucket_name, num_of_objects_to_upload);
+        if (key_to_delete === undefined) {
+            key_to_delete = `my-key-${random_integer(1, expected_num_of_object_in_list)}`;
+            console.log('test_list_and_delete: key_to_delete', key_to_delete);
+        }
+
+        console.log(`test_list_and_delete: ${test_name} ${bucket_name} num_of_objects_to_upload: ${num_of_objects_to_upload},
+            expected_num_of_object_in_list ${expected_num_of_object_in_list} key_to_delete ${key_to_delete}`);
+
+        for (let i = 0; i < iterations; ++i) {
+                nsfs.list_objects({ bucket: bucket_name }, DUMMY_OBJECT_SDK)
+                .catch(err => {
+                    console.log('error during list_objects', err);
+                    throw err;
+                }).then(res => {
+                    console.log('list was successful');
+                });
+            nsfs.delete_object({ bucket: bucket_name, key: key_to_delete }, DUMMY_OBJECT_SDK)
+                .catch(err => {
+                    console.log('delete_object got an error', err);
+                    throw err;
+                }).then(res => {
+                    console.log('delete_object during list objects was successful');
+                });
+            await P.delay(5000);
+            // up to this point if it was successful, the race between the delete object and list object went fine.
+        }
+    }, TEST_TIMEOUT);
+}
+
+/**
+ * _upload_objects uploads number_of_versions of objects in bucket
+ * note: this function is not concurrent, it's a helper function for preparing a bucket with a couple of objects
+ * @param {string} bucket
+ * @param {number} number_of_objects
+ */
+async function _upload_objects(bucket, number_of_objects) {
+    const keys_names = [];
+    for (let i = 0; i < number_of_objects; i++) {
+        const key_name = `my-key-${i + 1}`;
+        const random_data = Buffer.from(String(crypto_random_string(7)));
+        const body = buffer_utils.buffer_to_read_stream(random_data);
+        await nsfs.upload_object({ bucket: bucket, key: key_name, source_stream: body }, DUMMY_OBJECT_SDK);
+        keys_names.push(key_name);
+    }
+    return keys_names;
+}
+
+/**
+ * randomInteger between min (included) and max (included)
+ * // copied from: https://stackoverflow.com/questions/4959975/generate-random-number-between-two-numbers-in-javascript
+ * @param {number} min
+ * @param {number} max
+ */
+function random_integer(min, max) {
+    return Math.floor(Math.random() * (max - min + 1)) + min;
+  }

src/util/native_fs_utils.js

@@ -303,6 +303,30 @@ async function stat_ignore_enoent(fs_context, file_path) {
     }
 }
 
+/**
+ * stat_if_exists execute stat on entry_path and ignores on certain error codes.
+ * @param {nb.NativeFSContext} fs_context
+ * @param {string} entry_path
+ * @param {boolean} use_lstat
+ * @param {boolean} should_ignore_eacces
+ * @returns {Promise<nb.NativeFSStats>}
+ */
+async function stat_if_exists(fs_context, entry_path, use_lstat, should_ignore_eacces) {
+    try {
+        return await nb_native().fs.stat(fs_context, entry_path, { use_lstat });
+    } catch (err) {
+        // we might want to expand the error list due to permission/structure
+        // change (for example: ELOOP, ENAMETOOLONG) or other reason (EPERM) - need to be decided
+        if ((err.code === 'EACCES' && should_ignore_eacces) ||
+             err.code === 'ENOENT' || err.code === 'ENOTDIR') {
+            dbg.log0('stat_if_exists: Could not access file entry_path',
+                entry_path, 'error code', err.code, ', skipping...');
+        } else {
+            throw err;
+        }
+    }
+}
+
 ////////////////////////
 /// NON CONTAINERIZED //
 ////////////////////////
@@ -688,16 +712,6 @@ function translate_error_codes(err, entity) {
     return err;
 }
 
-async function stat_ignore_eacces(bucket_path, result, fs_context) {
-    const entry_path = path.join(bucket_path, result.key);
-    try {
-        return await nb_native().fs.stat(fs_context, entry_path);
-    } catch (err) {
-        if (err.code !== 'EACCES') throw err;
-        dbg.log0('NamespaceFS: check_stats_for_object : couldnt access file entry_path', entry_path, ", skipping...");
-    }
-}
-
 exports.get_umasked_mode = get_umasked_mode;
 exports._make_path_dirs = _make_path_dirs;
 exports._create_path = _create_path;
@@ -708,6 +722,7 @@ exports.finally_close_files = finally_close_files;
 exports.get_user_by_distinguished_name = get_user_by_distinguished_name;
 exports.get_config_files_tmpdir = get_config_files_tmpdir;
 exports.stat_ignore_enoent = stat_ignore_enoent;
+exports.stat_if_exists = stat_if_exists;
 
 exports._is_gpfs = _is_gpfs;
 exports.safe_move = safe_move;
@@ -739,4 +754,3 @@ exports.get_bucket_tmpdir_full_path = get_bucket_tmpdir_full_path;
 exports.get_bucket_tmpdir_name = get_bucket_tmpdir_name;
 exports.entity_enum = entity_enum;
 exports.translate_error_codes = translate_error_codes;
-exports.stat_ignore_eacces = stat_ignore_eacces;