Address comments

Author: forsyth2

docs/source/usage.rst

@@ -56,8 +56,8 @@ Additional optional arguments:
   than MAXSIZE except when individual input files exceed MAXSIZE (as 
   individual files are never split up between different tar files).
 * ``--non-blocking`` Zstash will submit a Globus transfer and immediately create a subsequent tarball. That is, Zstash will not wait until the transfer completes to start creating a subsequent tarball. On machines where it takes more time to create a tarball than transfer it, each Globus transfer will have one file. On machines where it takes less time to create a tarball than transfer it, the first transfer will have one file, but the number of tarballs in subsequent transfers will grow finding dynamically the most optimal number of tarballs per transfer. NOTE: zstash is currently always non-blocking.
-* ``--error-on-duplicate-tar`` Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash's behavior will depend on whether or not the --overwrite-duplicate-tar flag is set.
-* ``--overwrite-duplicate-tars`` If a duplicate tar is encountered, overwrite the existing tar file with the new one (i.e., it will assume the latest tar is the correct one). If this flag is not set, zstash will permit multiple entries for the same tar in its database.
+* ``--error-on-duplicate-tar`` FOR ADVANCED USERS ONLY: Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash's behavior will depend on whether or not the --overwrite-duplicate-tar flag is set.
+* ``--overwrite-duplicate-tars`` FOR ADVANCED USERS ONLY: If a duplicate tar is encountered, overwrite the existing tar file with the new one (i.e., it will assume the latest tar is the correct one). If this flag is not set, zstash will permit multiple entries for the same tar in its database.
 * ``-v`` increases output verbosity.
 
 Local tar files as well as the sqlite3 index database (index.db) will be stored
@@ -155,7 +155,7 @@ where
   an incomplete tar file, then the archive you're checking
   must have been created using ``zstash >= v1.1.0``.
 * ``--tars`` to specify specific tars to check. See below for example usage.
-* ``--error-on-duplicate-tar`` Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash will check if the sizes and md5sums match *at least one* of the tars.
+* ``--error-on-duplicate-tar`` FOR ADVANCED USERS ONLY: Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash will check if the size matches the *most recent* entry.
 * ``-v`` increases output verbosity.
 * ``[files]`` is a list of files to check (standard wildcards supported).
 
@@ -243,8 +243,8 @@ where
   they have been extracted from the archive. Normally, they are deleted after
   successful transfer.
 * ``--non-blocking`` Zstash will submit a Globus transfer and immediately create a subsequent tarball. That is, Zstash will not wait until the transfer completes to start creating a subsequent tarball. On machines where it takes more time to create a tarball than transfer it, each Globus transfer will have one file. On machines where it takes less time to create a tarball than transfer it, the first transfer will have one file, but the number of tarballs in subsequent transfers will grow finding dynamically the most optimal number of tarballs per transfer. NOTE: zstash is currently always non-blocking.
-* ``--error-on-duplicate-tar`` Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash's behavior will depend on whether or not the --overwrite-duplicate-tar flag is set.
-* ``--overwrite-duplicate-tars`` If a duplicate tar is encountered, overwrite the existing tar file with the new one (i.e., it will assume the latest tar is the correct one). If this flag is not set, zstash will permit multiple entries for the same tar in its database.
+* ``--error-on-duplicate-tar`` FOR ADVANCED USERS ONLY: Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash's behavior will depend on whether or not the --overwrite-duplicate-tar flag is set.
+* ``--overwrite-duplicate-tars`` FOR ADVANCED USERS ONLY: If a duplicate tar is encountered, overwrite the existing tar file with the new one (i.e., it will assume the latest tar is the correct one). If this flag is not set, zstash will permit multiple entries for the same tar in its database.
 * ``-v`` increases output verbosity.
 
 Note: in the event that an update includes revisions to files previously archived, ``zstash update``
@@ -324,7 +324,7 @@ where
   an incomplete tar file, then the archive you're extracting from
   must have been created using ``zstash >= v1.1.0``.
 * ``--tars`` to	specify	specific tars to extract. See "Check" above for example usage.
-* ``--error-on-duplicate-tar`` Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash will check if the sizes and md5sums match *at least one* of the tars.
+* ``--error-on-duplicate-tar`` FOR ADVANCED USERS ONLY: Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash will check if the size matches the *most recent* entry.
 * ``-v`` increases output verbosity.
 * ``[files]`` is a list of files to be extracted (standard wildcards supported).
 

tests/scripts/database_corruption.bash

@@ -60,6 +60,7 @@ run_test_cases()
     # 5. `zstash create` with `--for-developers-force-database-corruption="simulate_row_existing"`. We simply add a duplicate tar, but `zstash check` with `--error-on-duplicate-tar` errors out because it finds two entries for the same tar.
     # 6. `zstash create` with `--for-developers-force-database-corruption="simulate_no_correct_size"` to construct a very bad database: two entries for the same tar, both with incorrect sizes. `zstash check` confirms that no entries match the actual file size.
     # 7. `zstash create` with `--for-developers-force-database-corruption="simulate_row_existing_bad_size"`. We add a duplicate tar, but with the wrong size. `zstash check` confirms that the other entry matches the actual file size, so it succeeds.
+    # 8. `zstash create` with `--for-developers-force-database-corruption="simulate_bad_size_for_most_recent"` to construct two entries for the same tar, the most recent of which has an incorrect size. `zstash check` fails because the most recent size does not match, but it does log that one of the entries matches the actual file size.
 
     # Standard cases ##########################################################
 
@@ -201,7 +202,7 @@ run_test_cases()
     fi
     cd ${src_prefix}_check
     zstash check --hpss=${DST_DIR}/${case_name} --error-on-duplicate-tar 2>&1 | tee check.log
-    grep "ERROR: Database corruption detected! Found 2 database entries for 000000.tar with sizes" check.log
+    grep "ERROR: Database corruption detected! Found 2 database entries for 000000.tar, with sizes" check.log
     if [ $? != 0 ]; then
         ((fail_count++))
         review_str+="${case_name}_check/check.log,"
@@ -225,17 +226,23 @@ run_test_cases()
     fi
     cd ${src_prefix}_check
     zstash check --hpss=${DST_DIR}/${case_name} 2>&1 | tee check.log
-    grep "INFO: 000000.tar: No database entries match the actual file size:" check.log
+    grep "WARNING: Database corruption detected! Found 2 database entries for 000000.tar, with sizes" check.log
     if [ $? != 0 ]; then
         ((fail_count++))
         review_str+="${case_name}_check/check.log,"
     else
         ((success_count++))
     fi
+    grep "INFO: 000000.tar: No database entry matches the actual file size:" check.log
+    if [ $? != 0 ]; then
+        ((fail_count++))
+    else
+        ((success_count++))
+    fi
 
 
-    # Case 7: Duplicates detected! Allow them. Pass check because at least one of the sizes match.
-    case_name="check_finds_a_matching_size"
+    # Case 7: Duplicates detected! Allow them. Pass check because the most recent size matches.
+    case_name="check_finds_most_recent_size_matches"
     src_prefix=${SRC_DIR}/${case_name}
     setup ${case_name} ${src_prefix}
     cd ${src_prefix}_create
@@ -261,13 +268,48 @@ run_test_cases()
     fi
     cd ${src_prefix}_check
     zstash check --hpss=${DST_DIR}/${case_name} 2>&1 | tee check.log
-    grep "INFO: 000000.tar: Found a database entry with the same size as the actual file size:" check.log
+    grep "WARNING: Database corruption detected! Found 2 database entries for 000000.tar, with sizes" check.log
     if [ $? != 0 ]; then
         ((fail_count++))
         review_str+="${case_name}_check/check.log,"
     else
         ((success_count++))
     fi
+    grep "INFO: 000000.tar: The most recent database entry has the same size as the actual file size:" check.log
+    if [ $? != 0 ]; then
+        ((fail_count++))
+    else
+        ((success_count++))
+    fi
+
+    # Case 8: Duplicates detected! Allow them. Error out on check because the most recent size doesn't match.
+    case_name="check_finds_most_recent_size_does_not_match"
+    src_prefix=${SRC_DIR}/${case_name}
+    setup ${case_name} ${src_prefix}
+    cd ${src_prefix}_create
+    zstash create --hpss=${DST_DIR}/${case_name} --for-developers-force-database-corruption="simulate_bad_size_for_most_recent" zstash_demo 2>&1 | tee create.log
+    grep "INFO: TESTING/DEBUGGING ONLY: Simulating bad size for most recent entry for 000000.tar." create.log
+    if [ $? != 0 ]; then
+        ((fail_count++))
+        review_str+="${case_name}_create/create.log,"
+    else
+        ((success_count++))
+    fi
+    cd ${src_prefix}_check
+    zstash check --hpss=${DST_DIR}/${case_name} 2>&1 | tee check.log
+    grep "WARNING: Database corruption detected! Found 2 database entries for 000000.tar, with sizes" check.log
+    if [ $? != 0 ]; then
+        ((fail_count++))
+        review_str+="${case_name}_check/check.log,"
+    else
+        ((success_count++))
+    fi
+    grep "INFO: 000000.tar: A database entry matches the actual file size," check.log
+    if [ $? != 0 ]; then
+        ((fail_count++))
+    else
+        ((success_count++))
+    fi
 
 
     # Summary
@@ -278,6 +320,6 @@ run_test_cases()
 
 run_test_cases
 
-# Success count: 20
+# Success count: 25
 # Fail count: 0
-# Review:
+# Review: 

zstash/create.py

@@ -169,17 +169,17 @@ def setup_create() -> Tuple[str, argparse.Namespace]:
     optional.add_argument(
         "--error-on-duplicate-tar",
         action="store_true",
-        help="Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash's behavior will depend on whether or not the --overwrite-duplicate-tar flag is set.",
+        help="FOR ADVANCED USERS ONLY: Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash's behavior will depend on whether or not the --overwrite-duplicate-tar flag is set.",
     )
     optional.add_argument(
         "--overwrite-duplicate-tars",
         action="store_true",
-        help="If a duplicate tar is encountered, overwrite the existing tar file with the new one (i.e., it will assume the latest tar is the correct one). If this flag is not set, zstash will permit multiple entries for the same tar in its database.",
+        help="FOR ADVANCED USERS ONLY: If a duplicate tar is encountered, overwrite the existing tar file with the new one (i.e., it will assume the latest tar is the correct one). If this flag is not set, zstash will permit multiple entries for the same tar in its database.",
     )
     optional.add_argument(
         "--for-developers-force-database-corruption",
         type=str,
-        help="For developers only! Forces database corruption by inserting a duplicate tar entry into the tars table. This is useful for testing. 3 options: simulate_row_existing -- add the tar before the main logic. simulate_row_existing_bad_size -- add the tar before the main logic, but with the wrong size. simulate_no_correct_size -- add the tar twice, both times with incorrect size. If this option is not set, no corruption will be forced.",
+        help="FOR DEVELOPERS ONLY! Forces database corruption by inserting a duplicate tar entry into the tars table. This is useful for testing. 3 options: simulate_row_existing -- add the tar before the main logic. simulate_row_existing_bad_size -- add the tar before the main logic, but with the wrong size. simulate_no_correct_size -- add the tar twice, both times with incorrect size. If this option is not set, no corruption will be forced. simulate_bad_size_for_most_recent -- add the tar twice, first with good size, then with bad size.",
         default="",
     )
     # Now that we're inside a subcommand, ignore the first two argvs

zstash/extract.py

@@ -104,7 +104,7 @@ def setup_extract() -> Tuple[argparse.Namespace, str]:
     optional.add_argument(
         "--error-on-duplicate-tar",
         action="store_true",
-        help="Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash will check if the sizes and md5sums match *at least one* of the tars.",
+        help="FOR ADVANCED USERS ONLY: Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash will check if the size matches the *most recent* entry.",
     )
     parser.add_argument("files", nargs="*", default=["*"])
     args: argparse.Namespace = parser.parse_args(sys.argv[2:])
@@ -401,7 +401,9 @@ def check_sizes_match(cur, tfname, error_on_duplicate_tar):
         name_only: str = os.path.split(tfname)[1]
 
         # Get ALL entries for this tar name
-        cur.execute("SELECT size FROM tars WHERE name = ?", (name_only,))
+        cur.execute(
+            "SELECT size FROM tars WHERE name = ? ORDER by id DESC", (name_only,)
+        )
         results = cur.fetchall()
 
         if not results:
@@ -414,32 +416,38 @@ def check_sizes_match(cur, tfname, error_on_duplicate_tar):
             # Extract just the size values
             sizes: List[int] = [row[0] for row in results]
             error_str: str = (
-                f"Database corruption detected! Found {len(results)} database entries for {name_only} with sizes {sizes}"
+                f"Database corruption detected! Found {len(results)} database entries for {name_only}, with sizes {sizes}"
             )
 
             if error_on_duplicate_tar:
                 # Tested by database_corruption.bash Case 5
                 logger.error(error_str)
                 raise RuntimeError(error_str)
-
             logger.warning(error_str)
-            unique_sizes: Set[int] = set(sizes)
-            if actual_size in unique_sizes:
+
+            # We ordered the results by id DESC,
+            # so the first entry is the most recent.
+            most_recent_size: int = sizes[0]
+            if actual_size == most_recent_size:
                 # Tested by database_corruption.bash Case 7
-                # If the actual size matches at least one of the sizes in the database,
+                # If the actual size matches the most recent size,
                 # then we can assume that the tar is valid.
                 logger.info(
-                    f"{name_only}: Found a database entry with the same size as the actual file size: {actual_size}."
+                    f"{name_only}: The most recent database entry has the same size as the actual file size: {actual_size}."
                 )
                 return True
+            unique_sizes: Set[int] = set(sizes)
+            if actual_size in unique_sizes:
+                # Tested by database_corruption.bash Case 8
+                logger.info(
+                    f"{name_only}: A database entry matches the actual file size, {actual_size}, but it is not the most recent entry."
+                )
             else:
                 # Tested by database_corruption.bash Case 6
-                # If the actual size does not match any of the sizes in the database,
-                # then we cannot assume that the tar is valid.
                 logger.info(
-                    f"{name_only}: No database entries match the actual file size: {actual_size}."
+                    f"{name_only}: No database entry matches the actual file size: {actual_size}."
                 )
-                return False
+            return False
         else:
             # Tested by database_corruption.bash Cases 1,2,4
             # Single entry - normal case

zstash/hpss_utils.py

@@ -234,6 +234,21 @@ def add_files(
                         "INSERT INTO tars VALUES (NULL,?,?,?)",
                         (tfname, tarsize + 2000, tar_md5),
                     )
+                elif force_database_corruption == "simulate_bad_size_for_most_recent":
+                    # Tested by database_corruption.bash Case 8
+                    # For developers only! For debugging purposes only!
+                    # Add this tar twice, second time with bad size.
+                    logger.info(
+                        f"TESTING/DEBUGGING ONLY: Simulating bad size for most recent entry for {tfname}."
+                    )
+                    cur.execute(
+                        "INSERT INTO tars VALUES (NULL,?,?,?)",
+                        (tfname, tarsize, tar_md5),
+                    )
+                    cur.execute(
+                        "INSERT INTO tars VALUES (NULL,?,?,?)",
+                        (tfname, tarsize + 2000, tar_md5),
+                    )
                 else:
                     # Tested by database_corruption.bash Cases 1,2
                     # Typical case

zstash/update.py

@@ -108,12 +108,12 @@ def setup_update() -> Tuple[argparse.Namespace, str]:
     optional.add_argument(
         "--error-on-duplicate-tar",
         action="store_true",
-        help="Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash's behavior will depend on whether or not the --overwrite-duplicate-tar flag is set.",
+        help="FOR ADVANCED USERS ONLY: Raise an error if a tar file with the same name already exists in the database. If this flag is set, zstash will exit if it sees a duplicate tar. If it is not set, zstash's behavior will depend on whether or not the --overwrite-duplicate-tar flag is set.",
     )
     optional.add_argument(
         "--overwrite-duplicate-tars",
         action="store_true",
-        help="If a duplicate tar is encountered, overwrite the existing tar file with the new one (i.e., it will assume the latest tar is the correct one). If this flag is not set, zstash will permit multiple entries for the same tar in its database.",
+        help="FOR ADVANCED USERS ONLY: If a duplicate tar is encountered, overwrite the existing tar file with the new one (i.e., it will assume the latest tar is the correct one). If this flag is not set, zstash will permit multiple entries for the same tar in its database.",
     )
     optional.add_argument(
         "-v", "--verbose", action="store_true", help="increase output verbosity"