)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":10,"context_line":"as the quarantine target. This interface works well for the case where a"},{"line_number":11,"context_line":"(meta)data file is corrupt and the intention is to quarantine the"},{"line_number":12,"context_line":"enclosing hash directory. For use cases where the exact path of the"},{"line_number":13,"context_line":"directory to be quarantined is know , the caller has to supply a path"},{"line_number":14,"context_line":"ending with a dummy filename for the quarantine target to be selected"},{"line_number":15,"context_line":"correctly."},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"5e81e9c1_938d1c9c","line":13,"updated":"2026-05-22 18:59:41.000000000","message":"s/know ,/known,/\n\nN.B. I would just fix the commit message and merge if I liked the new tests/names better ;)","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":19,"context_line":""},{"line_number":20,"context_line":"The patch also adds test coverage for scenarios where hash directory"},{"line_number":21,"context_line":"quarantining failures means that we have to quarantine the suffix"},{"line_number":22,"context_line":"directory."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"Change-Id: Ie261697f72635d8d0f8ece23d8bae0e48ea8d0af"},{"line_number":25,"context_line":"Signed-off-by: Wael Halbawi \u003cwhalbawi@nvidia.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"bca62e86_a20ea0e8","line":22,"updated":"2026-05-22 18:59:41.000000000","message":"this is a nice `Drive-By: add test coverage ...`","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"2792e80d6b6c5c4913214bc31266f23dd3b6dc87","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"96ec5a39_ff8e8b4a","updated":"2026-05-21 15:58:36.000000000","message":"I think this is a nice improvement. Did not fully test yet, will do that later. One small comment inline","commit_id":"6d1376537d5e5d919855ddde16a6686980d6d72f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"a967bf65_fbb18d4b","updated":"2026-05-22 18:59:41.000000000","message":"I think this new interface is a helpful ergonomic addition (new way to spell an existing capability, worth maintaining), the drive-by test coverage addition for the untested branch is the largest material (non-ascetic) improvement.\n\nIn all but one case the diff clearly reads as replacing an ugly `made-up-filename` with a more obvious and direct spelling.  I think we should prefer \n\n`quarantine_exactly_dir(dir_path)`\nover\n`quarantine_rename(os.path.join(dir_path, \u0027made-up-filename\u0027))`\n\nThe one outlier was:\n\n```\ntry:\n   quarantine_rename(os.path.join(dir_path, \u0027made-up-filename\u0027))\nexcept:\n   # try to quarantine the parent\n   quarantine_renamer(dir_path)\n```\n\nstrangely that hunk ended up doing a `dirname(dir_path)` on the next line anyway for logging so in retrospect I came to agree with the diff hunk as written.  It almost seems like `qurantine_exactly_dir(dir_path)` is the common case and `quarantine_file_parent_dir(file_path)` is the rarer syntactic suger?  (maybe the diff just makes it look that way b/c it\u0027s targeting the cases we fail to build df instances b/c of corruption at or above the hashdir)\n\nHolding off +1 to fix typo in commit message; possible test/nit improvements; feedback from author, and lastly maybe a rename:\n\n```\n- def quarantine_renamer\n+ def quarantine_file_parent_dir\n- def quarantine_dir_renamer\n+ def quarantine_exactly_dir\n+ # deprecated/legacy backwards compat alias\n+ quarantine_renamer \u003d quarantine_file_parent_dir\n```\n\nIn retrospect if we could have only had one quarantine method, `quarantine_exactly_dir(dirname(filepath))` might have been a better primitive.  To that end, I think we should discourage `quarantine_renamer(dir_path)` *in general* as an awkward spelling of `quarantine_exactly_dir(dirname(dir_path))` and reserve `quarantine[_file]_parent_dir(file_path)` for ONLY cases when we pass in a file.  This \"discourage\" may be limited to docstrings, cargo cult and surrounding patterns, but could also be pushed further by way of an audit. I think it would be a larger change with more risk to have `quarantine_file_parent_dir` raise `ValueError` if you pass it a dir, and b/c of fs corruption it may be difficult to tell.  A deprecation cycle may allow us to make the new `quarantine_file_parent_dir` more strict and eventually not have to think about ambiguous usage of the old `quarantine_renamer` - but I\u0027d be quite happy with just better names, docstrings and consistent usage in-tree, i.e. even if it did you want, who would write: `quarantine_file_parent_dir(dir_path)` instead of `quarantine_exactly_dir(dirname(dir_path))`","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":38496,"name":"Andressa Cabistani","display_name":"Andressa","email":"acabistani@gmail.com","username":"andressadotpy"},"change_message_id":"9f4d995f913a3086d5848d73b2001b467bf8b77d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"6592d9c8_e3b31c99","updated":"2026-05-25 13:00:03.000000000","message":"Thank you very much for your contribution! Here is a summary of my review:\n\n1) Great test coverage:\n    - Hash directory quarantine (test_quarantine_dir_renamer_hash)\n    - Suffix directory quarantine (test_quarantine_dir_renamer_suffix)\n    - Fallback scenarios when hash quarantine fails (test_get_diskfile_from_hash_fs_corrupt_quarantined_suffix, test_hash_suffix_cleanup_ondisk_files_fs_corrupt_quarantined)\n    - Edge case: non-existent directory raises OSError with ENOENT\n    \n2) All .unittests are still passing\n\n3) Code best practices: clean refactoring and keeps backward compatibility\n\n4) I manually validated this in SAIO:\n    - Simulated ENOTDIR scenario: replaced hash directory with file\n    - Ran `swift-init object-auditor once`\n    - Read the logs/Confirm quarantines:\n\nvagrant@saio:~/swift$ sudo ls -la /srv/node4/sdb4/objects/957/383/\ntotal 0\ndrwxr-xr-x 2 vagrant vagrant 46 May 25 12:45 .\ndrwxr-xr-x 3 vagrant vagrant 52 May 25 12:41 ..\n-rw-r--r-- 1 root    root     0 May 25 12:45 ef5b56a33b3071310cbdbc4fbba5d383\nvagrant@saio:~/swift$ sudo swift-init object-auditor once\nRunning object-auditor once...(/etc/swift/object-server/1.conf.d)\nRunning object-auditor once...(/etc/swift/object-server/2.conf.d)\nRunning object-auditor once...(/etc/swift/object-server/3.conf.d)\nRunning object-auditor once...(/etc/swift/object-server/4.conf.d)\nvagrant@saio:~/swift$ sudo grep \"ef5b56a33b3071310cbdbc4fbba5d383\" /var/log/syslog | grep -i quarantine\n2026-05-25T12:45:39.965985+00:00 saio object-auditor-6040: Quarantined object /srv/node4/sdb4/objects/957/383/ef5b56a33b3071310cbdbc4fbba5d383: Expected directory, found file at /srv/node4/sdb4/objects/957/383/ef5b56a33b3071310cbdbc4fbba5d383\n2026-05-25T12:45:39.966173+00:00 saio object-auditor-6040: ERROR Object /srv/node4/sdb4/objects/957/383/ef5b56a33b3071310cbdbc4fbba5d383 failed audit and was quarantined: Expected directory, found file at /srv/node4/sdb4/objects/957/383/ef5b56a33b3071310cbdbc4fbba5d383\nvagrant@saio:~/swift$ sudo ls -la /srv/node4/sdb4/quarantined/objects/\ntotal 0\ndrwxr-xr-x 3 vagrant vagrant 86 May 25 12:45 .\ndrwxr-xr-x 3 vagrant vagrant 21 May 19 09:30 ..\ndrwxr-xr-x 2 vagrant vagrant 35 May 19 09:30 8a902763e16a8667823444614855f9f2\n-rw-r--r-- 1 root    root     0 May 25 12:45 ef5b56a33b3071310cbdbc4fbba5d383\nvagrant@saio:~/swift$ sudo cat /srv/node4/sdb4/objects/957/hashes.invalid\n383\n383\nvagrant@saio:~/swift$ sudo rm -rf /srv/node4/sdb4/quarantined/objects/ef5b56a33b3071310cbdbc4fbba5d383\nvagrant@saio:~/swift$ rm /tmp/test-object\nvagrant@saio:~/swift$ swift delete test-container88 test-object\ntest-object\nvagrant@saio:~/swift$ swift delete test-container88\n\nThank you very much and it looks very good to me!","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"}],"swift/obj/diskfile.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":1593,"context_line":"                except (OSError, IOError):"},{"line_number":1594,"context_line":"                    # We\u0027ve *also* seen the bad sectors lead to us needing to"},{"line_number":1595,"context_line":"                    # quarantine the whole suffix, not just the hash dir"},{"line_number":1596,"context_line":"                    quar_path \u003d self.quarantine_renamer(dev_path, object_path)"},{"line_number":1597,"context_line":"                    orig_path \u003d os.path.dirname(object_path)"},{"line_number":1598,"context_line":"                logging.exception("},{"line_number":1599,"context_line":"                    \u0027Quarantined %(orig_path)s to %(quar_path)s because \u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"907f71e2_bee5697f","side":"PARENT","line":1596,"updated":"2026-05-22 18:59:41.000000000","message":"so here we\u0027re explicitly leverging the \"parent dir\" behavior to quarantine the whole suffix","commit_id":"3392fa3ce5df938c8392ac04ff6b69e36b17eadd"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":1594,"context_line":"                    # We\u0027ve *also* seen the bad sectors lead to us needing to"},{"line_number":1595,"context_line":"                    # quarantine the whole suffix, not just the hash dir"},{"line_number":1596,"context_line":"                    quar_path \u003d self.quarantine_renamer(dev_path, object_path)"},{"line_number":1597,"context_line":"                    orig_path \u003d os.path.dirname(object_path)"},{"line_number":1598,"context_line":"                logging.exception("},{"line_number":1599,"context_line":"                    \u0027Quarantined %(orig_path)s to %(quar_path)s because \u0027"},{"line_number":1600,"context_line":"                    \u0027it could not be listed\u0027, {\u0027orig_path\u0027: orig_path,"}],"source_content_type":"text/x-python","patch_set":2,"id":"929124dd_643ff9ff","side":"PARENT","line":1597,"updated":"2026-05-22 18:59:41.000000000","message":"oh heh, THIS is why `quarantine_parent(object_path)` and `quarantine_exactly(orig_path)` are equivalent.","commit_id":"3392fa3ce5df938c8392ac04ff6b69e36b17eadd"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":2703,"context_line":"        :param msg: reason for quarantining to be included in the exception"},{"line_number":2704,"context_line":"        :returns: DiskFileQuarantined exception object"},{"line_number":2705,"context_line":"        \"\"\""},{"line_number":2706,"context_line":"        self._quarantined_dir \u003d self.manager.quarantine_renamer("},{"line_number":2707,"context_line":"            self._device_path, data_file)"},{"line_number":2708,"context_line":"        self._logger.warning(\"Quarantined object %s: %s\" % ("},{"line_number":2709,"context_line":"            data_file, msg))"}],"source_content_type":"text/x-python","patch_set":2,"id":"da47831a_c1675289","side":"PARENT","line":2706,"updated":"2026-05-22 18:59:41.000000000","message":"is this side-effect part of the interface only for testing?\n\nN.B. this turned out to be the `df` instance; not the `df_mgr` - so you can imagine that you only quarantine a df once and in doing so set this sentinal attribute.","commit_id":"3392fa3ce5df938c8392ac04ff6b69e36b17eadd"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":369,"context_line":""},{"line_number":370,"context_line":"    :params device_path: The path to the device the corrupted file is on"},{"line_number":371,"context_line":"    :params corrupted_file_path: The path to the file whose parent directory"},{"line_number":372,"context_line":"                                 you want quarantined"},{"line_number":373,"context_line":""},{"line_number":374,"context_line":"    :returns: path (str) of directory the file was moved to"},{"line_number":375,"context_line":"    :raises OSError: re-raises non errno.EEXIST / errno.ENOTEMPTY"}],"source_content_type":"text/x-python","patch_set":2,"id":"b6a25ba6_0c715f59","line":372,"updated":"2026-05-22 18:59:41.000000000","message":"I think the doc string change to emphasize \"parent directory\" is good; I think the diff might look better if define `quarantine_dir_renamer` *after* `quarantine_renamer`","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":38496,"name":"Andressa Cabistani","display_name":"Andressa","email":"acabistani@gmail.com","username":"andressadotpy"},"change_message_id":"9f4d995f913a3086d5848d73b2001b467bf8b77d","unresolved":false,"context_lines":[{"line_number":375,"context_line":"    :raises OSError: re-raises non errno.EEXIST / errno.ENOTEMPTY"},{"line_number":376,"context_line":"                     exceptions from rename"},{"line_number":377,"context_line":"    \"\"\""},{"line_number":378,"context_line":"    return quarantine_dir_renamer(device_path, dirname(corrupted_file_path))"},{"line_number":379,"context_line":""},{"line_number":380,"context_line":""},{"line_number":381,"context_line":"def valid_suffix(value):"}],"source_content_type":"text/x-python","patch_set":2,"id":"4ae35361_eabbe2cc","line":378,"updated":"2026-05-25 13:00:03.000000000","message":"Nicely done! I like the wrapper to ensure backward compatibility!","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":743,"context_line":""},{"line_number":744,"context_line":"    invalidate_hash \u003d staticmethod(invalidate_hash)"},{"line_number":745,"context_line":"    consolidate_hashes \u003d staticmethod(consolidate_hashes)"},{"line_number":746,"context_line":"    quarantine_renamer \u003d staticmethod(quarantine_renamer)"},{"line_number":747,"context_line":"    quarantine_dir_renamer \u003d staticmethod(quarantine_dir_renamer)"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"    def __init__(self, conf, logger):"}],"source_content_type":"text/x-python","patch_set":2,"id":"a9ef4d11_4ed5c94f","line":746,"updated":"2026-05-22 18:59:41.000000000","message":"is this method ... deprecated in favor of `quarantine_exactly_dir(dirname(file_path))`\n\n... or is it still a reasonable way to spell `quarantine_parent_dir(file_path)`","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":1599,"context_line":"                logging.exception("},{"line_number":1600,"context_line":"                    \u0027Quarantined %(orig_path)s to %(quar_path)s because \u0027"},{"line_number":1601,"context_line":"                    \u0027it could not be listed\u0027, {\u0027orig_path\u0027: orig_path,"},{"line_number":1602,"context_line":"                                               \u0027quar_path\u0027: quar_path})"},{"line_number":1603,"context_line":"                raise DiskFileNotExist()"},{"line_number":1604,"context_line":"            if err.errno !\u003d errno.ENOENT:"},{"line_number":1605,"context_line":"                raise"}],"source_content_type":"text/x-python","patch_set":2,"id":"f74c17ef_3c568331","line":1602,"updated":"2026-05-22 18:59:41.000000000","message":"as best I can tell this this block was untested - unfortunate - KUDOS","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":2380,"context_line":""},{"line_number":2381,"context_line":"    def _quarantine(self, msg):"},{"line_number":2382,"context_line":"        self._quarantined_dir \u003d self.manager.quarantine_renamer("},{"line_number":2383,"context_line":"            self._device_path, self._data_file)"},{"line_number":2384,"context_line":"        self._logger.warning(\"Quarantined object %s: %s\" % ("},{"line_number":2385,"context_line":"            self._data_file, msg))"},{"line_number":2386,"context_line":"        self._logger.increment(\u0027quarantines\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"97995d2e_a18bc755","line":2383,"updated":"2026-05-22 18:59:41.000000000","message":"strange that a `DiskFileReader` is not an instance of a `BaseDiskFile`","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":2636,"context_line":"                    # We\u0027ve *also* seen the bad sectors lead to us needing to"},{"line_number":2637,"context_line":"                    # quarantine the whole suffix, not just the hash dir"},{"line_number":2638,"context_line":"                    raise self._quarantine_dir("},{"line_number":2639,"context_line":"                        os.path.dirname(self._datadir),"},{"line_number":2640,"context_line":"                        \"Failed to list directory at %s\" % self._datadir)"},{"line_number":2641,"context_line":"            elif err.errno !\u003d errno.ENOENT:"},{"line_number":2642,"context_line":"                raise DiskFileError("}],"source_content_type":"text/x-python","patch_set":2,"id":"b8dcd208_5e9c2b63","line":2639,"updated":"2026-05-22 18:59:41.000000000","message":"this seems more obviously correct - we change from `quarantine_parent(datadir)` to `quarantine_exactly(dirname(datadir))`\n\nWHY did it change tho?","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":2695,"context_line":"        Quarantine a directory; responsible for incrementing the associated"},{"line_number":2696,"context_line":"        logger\u0027s count of quarantines."},{"line_number":2697,"context_line":""},{"line_number":2698,"context_line":"        :param dir_path: full path of the directory to quarantine"},{"line_number":2699,"context_line":"        :param msg: reason for quarantining to be included in the exception"},{"line_number":2700,"context_line":"        :param source_path: Use this path as the quarantine source for logging"},{"line_number":2701,"context_line":"                            purposes"}],"source_content_type":"text/x-python","patch_set":2,"id":"cbb8ca45_f9b37ace","line":2698,"updated":"2026-05-22 18:59:41.000000000","message":"ok, so this method extraction is used in many methods before it\u0027s defined","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":2698,"context_line":"        :param dir_path: full path of the directory to quarantine"},{"line_number":2699,"context_line":"        :param msg: reason for quarantining to be included in the exception"},{"line_number":2700,"context_line":"        :param source_path: Use this path as the quarantine source for logging"},{"line_number":2701,"context_line":"                            purposes"},{"line_number":2702,"context_line":"        :returns: DiskFileQuarantined exception object"},{"line_number":2703,"context_line":"        \"\"\""},{"line_number":2704,"context_line":"        self._quarantined_dir \u003d self.manager.quarantine_dir_renamer("}],"source_content_type":"text/x-python","patch_set":2,"id":"1b3e8e72_c7dc1d61","line":2701,"updated":"2026-05-22 18:59:41.000000000","message":"maybe:\n\n```\ndef _log_quarantine(self, method, path, msg)\n   result \u003d method(self._device_path, path)\n   log(\u0027moved {path} to {results}: {msg}\u0027)\n   increment(\u0027quarantines\u0027)\n   raise DiskFileQuarantine(msg)\n   \ndef _quarantine_parent_dir(self, file_path, msg):\n    return _log_quarantine(manager.quarantine_renamer, file_path, msg)\n    \ndef _quarantine_exactly_dir(self, dir_path, msg):\n    return _log_quarantine(manager.qurantine_dir_renamer, dir_path, msg)\n```","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":2719,"context_line":"        \"\"\""},{"line_number":2720,"context_line":"        return self._quarantine_dir(dirname(data_file),"},{"line_number":2721,"context_line":"                                    msg,"},{"line_number":2722,"context_line":"                                    source_path\u003ddata_file)"},{"line_number":2723,"context_line":""},{"line_number":2724,"context_line":"    def _get_ondisk_files(self, files, policy\u003dNone):"},{"line_number":2725,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"7012709f_7eceec0c","line":2722,"updated":"2026-05-22 18:59:41.000000000","message":"I think we may have removed the only caller of `manager.quarantine_renamer` in `BaseDiskFile`\n\n```\n(vagrant-swift-all-in-one) cgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ ag quarantine_renamer swift/obj/\nswift/obj/diskfile.py\n365:def quarantine_renamer(device_path, corrupted_file_path):\n746:    quarantine_renamer \u003d staticmethod(quarantine_renamer)\n2382:        self._quarantined_dir \u003d self.manager.quarantine_renamer(\n```","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"}],"test/unit/obj/test_diskfile.py":[{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"2792e80d6b6c5c4913214bc31266f23dd3b6dc87","unresolved":true,"context_lines":[{"line_number":542,"context_line":"            self.assertEqual(to_dir, exp_dir)"},{"line_number":543,"context_line":"            self.assertTrue(os.path.isdir(to_dir))"},{"line_number":544,"context_line":"            self.assertTrue(os.path.join(to_dir,"},{"line_number":545,"context_line":"                                         os.path.basename(df._datadir)))"},{"line_number":546,"context_line":"            self.assertFalse(os.path.exists(suffix_dir))"},{"line_number":547,"context_line":""},{"line_number":548,"context_line":"            with self.assertRaises(OSError) as cm:"}],"source_content_type":"text/x-python","patch_set":1,"id":"c3a24aac_368da326","line":545,"updated":"2026-05-21 15:58:36.000000000","message":"Is there maybe missing a isdir here? os.path.join is likely always true?\n\nLike:\n\n    self.assertTrue(os.path.isdir(os.path.join(to_dir, os.path.basename(df._datadir))))","commit_id":"6d1376537d5e5d919855ddde16a6686980d6d72f"},{"author":{"_account_id":38767,"name":"Wael Halbawi","display_name":"Wael Halbawi","email":"whalbawi@nvidia.com","username":"whalbawi"},"change_message_id":"e256fec6ce15fce9c8a76c067398d96e51d1aa7b","unresolved":false,"context_lines":[{"line_number":542,"context_line":"            self.assertEqual(to_dir, exp_dir)"},{"line_number":543,"context_line":"            self.assertTrue(os.path.isdir(to_dir))"},{"line_number":544,"context_line":"            self.assertTrue(os.path.join(to_dir,"},{"line_number":545,"context_line":"                                         os.path.basename(df._datadir)))"},{"line_number":546,"context_line":"            self.assertFalse(os.path.exists(suffix_dir))"},{"line_number":547,"context_line":""},{"line_number":548,"context_line":"            with self.assertRaises(OSError) as cm:"}],"source_content_type":"text/x-python","patch_set":1,"id":"b941b5f8_930dc9a4","line":545,"in_reply_to":"c3a24aac_368da326","updated":"2026-05-21 17:23:50.000000000","message":"Fixed. Thanks!","commit_id":"6d1376537d5e5d919855ddde16a6686980d6d72f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":1795,"context_line":"                    \u0027cleanup_ondisk_files\u0027)) as cleanup, \\"},{"line_number":1796,"context_line":"                mock.patch(\u0027swift.obj.diskfile.read_metadata\u0027) as readmeta, \\"},{"line_number":1797,"context_line":"                mock.patch(self._manager_mock("},{"line_number":1798,"context_line":"                    \u0027quarantine_renamer\u0027)) as quarantine_renamer:"},{"line_number":1799,"context_line":"            osexc \u003d OSError()"},{"line_number":1800,"context_line":"            osexc.errno \u003d errno.ENOTDIR"},{"line_number":1801,"context_line":"            cleanup.side_effect \u003d osexc"}],"source_content_type":"text/x-python","patch_set":2,"id":"f4f637da_04f22308","side":"PARENT","line":1798,"updated":"2026-05-22 18:59:41.000000000","message":"it\u0027s actually pretty annoying these tests are written this way - i\u0027d be better if we let the shutil code move stuff around in our `/tmp/fake_devices_root/` path and assert on the behavior rather than the method signatures.","commit_id":"3392fa3ce5df938c8392ac04ff6b69e36b17eadd"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":1915,"context_line":"                mock.patch(self._manager_mock("},{"line_number":1916,"context_line":"                    \u0027cleanup_ondisk_files\u0027)) as cleanup_mock, \\"},{"line_number":1917,"context_line":"                mock.patch(self._manager_mock("},{"line_number":1918,"context_line":"                    \u0027quarantine_dir_renamer\u0027)) as qr_mock:"},{"line_number":1919,"context_line":"                osexc \u003d OSError()"},{"line_number":1920,"context_line":"                osexc.errno \u003d err"},{"line_number":1921,"context_line":"                cleanup_mock.side_effect \u003d osexc"}],"source_content_type":"text/x-python","patch_set":2,"id":"1bccfb9c_d67bae92","line":1918,"updated":"2026-05-22 18:59:41.000000000","message":"i\u0027m not sure `qr_mock` is the greatest name for the `q_dir_mock`","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"142eb105856bc2dc7ae2737a3e0697073109db44","unresolved":true,"context_lines":[{"line_number":1933,"context_line":"                                                       hsh,"},{"line_number":1934,"context_line":"                                                       policy)"},{"line_number":1935,"context_line":"                self.assertEqual("},{"line_number":1936,"context_line":"                    [mock.call(dev_path, call[0]) for call in qr_mock_calls],"},{"line_number":1937,"context_line":"                    qr_mock.call_args_list)"},{"line_number":1938,"context_line":""},{"line_number":1939,"context_line":"    def test_get_diskfile_from_hash_no_dir(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"d32b9f5b_5385cf3a","line":1936,"updated":"2026-05-22 18:59:41.000000000","message":"OMG this took me *forever* to read - we\u0027re asserting that we `qurantine_dir_renamer` gets called with first with `hash_dir` argument (which raises OSError) and then again with `suffix_dir`\n\nI think there\u0027s a good bit more one could to to try and make this better; I got this far:\n\n```\ndiff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py\nindex cce08bf678..56848a11fe 100644\n--- a/test/unit/obj/test_diskfile.py\n+++ b/test/unit/obj/test_diskfile.py\n@@ -1902,39 +1902,47 @@ class DiskFileManagerMixin(BaseDiskFileTestMixin):\n                 \u0027/srv/dev/\u0027,\n                 \u0027/srv/dev/objects/9/900/9a7175077c01a23ade5956b8a2bba900\u0027)\n \n-    def test_get_diskfile_from_hash_fs_corrupt_nested_error(self):\n+    def _check_quarantine_suffix(self, err):\n         dev_path \u003d \u0027/srv/dev/\u0027\n         part \u003d \u00279\u0027\n         hsh \u003d \u00279a7175077c01a23ade5956b8a2bba900\u0027\n         suffix \u003d hsh[-3:]\n         policy \u003d 0\n+        suffix_dir \u003d os.path.join(dev_path, \u0027objects\u0027, part, suffix)\n+        hash_dir \u003d os.path.join(suffix_dir, hsh)\n+        qr_dir \u003d os.path.join(dev_path, \u0027quarantined\u0027, part, suffix)\n+\n+        cleanup_err \u003d OSError(err, os.strerror(err))\n+        self.assertEqual(cleanup_err.errno, err)  # sanity\n \n         self.df_mgr.get_dev_path \u003d mock.MagicMock(return_value\u003ddev_path)\n-        for err in (errno.ENODATA, EUCLEAN):\n-            with mock.patch(self._manager_mock(\u0027diskfile_cls\u0027)), \\\n+        with mock.patch(self._manager_mock(\u0027diskfile_cls\u0027)), \\\n                 mock.patch(self._manager_mock(\n                     \u0027cleanup_ondisk_files\u0027)) as cleanup_mock, \\\n                 mock.patch(self._manager_mock(\n-                    \u0027quarantine_dir_renamer\u0027)) as qr_mock:\n-                osexc \u003d OSError()\n-                osexc.errno \u003d err\n-                cleanup_mock.side_effect \u003d osexc\n-\n-                suffix_dir \u003d os.path.join(dev_path, \u0027objects\u0027, part, suffix)\n-                hash_dir \u003d os.path.join(suffix_dir, hsh)\n-                qr_dir \u003d os.path.join(dev_path, \u0027quarantined\u0027, part, suffix)\n-\n-                # (input, side_effect) pairs\n-                qr_mock_calls \u003d ((hash_dir, OSError()), (suffix_dir, qr_dir))\n-                qr_mock.side_effect \u003d [call[1] for call in qr_mock_calls]\n-                with self.assertRaises(DiskFileNotExist):\n-                    self.df_mgr.get_diskfile_from_hash(dev_path,\n-                                                       part,\n-                                                       hsh,\n-                                                       policy)\n-                self.assertEqual(\n-                    [mock.call(dev_path, call[0]) for call in qr_mock_calls],\n-                    qr_mock.call_args_list)\n+                    \u0027quarantine_dir_renamer\u0027)) as qd_mock:\n+            # when cleanup raises an error we try to quarantine\n+            cleanup_mock.side_effect \u003d cleanup_err\n+            # first quarantine_dir call will raise error, second will return\n+            # quarantined path\n+            qd_mock.side_effect \u003d [OSError, qr_dir]\n+\n+            with self.assertRaises(DiskFileNotExist):\n+                self.df_mgr.get_diskfile_from_hash(dev_path,\n+                                                   part,\n+                                                   hsh,\n+                                                   policy)\n+        # qd_mock called twice, second time with suffix_dir\n+        self.assertEqual([\n+            mock.call(dev_path, hash_dir),\n+            mock.call(dev_path, suffix_dir),\n+        ], qd_mock.call_args_list)\n+\n+    def test_get_diskfile_from_hash_fs_corrupt_nested_enodata(self):\n+        self._check_quarantine_suffix(errno.ENODATA)\n+\n+    def test_get_diskfile_from_hash_fs_corrupt_nested_euclean(self):\n+        self._check_quarantine_suffix(errno.EUCLEAN)\n \n     def test_get_diskfile_from_hash_no_dir(self):\n         self.df_mgr.get_dev_path \u003d mock.MagicMock(return_value\u003d\u0027/srv/dev/\u0027)\n```","commit_id":"77cfd2d99cbabc738759af039c9251884bde6fa8"}]}