fix(BA-3250): Download directory not removed after moving files to archive directory

[jopemachine]

resolves #7114 (BA-3250)

Checklist: (if applicable)

• Mention to the original issue

[Copilot]

Pull request overview

This PR resolves BA-3250 by changing the artifact import process from copying files to moving them, ensuring download directories are properly removed after files are transferred to the archive directory.

Key changes:

• Refactored cleanup logic by introducing a default cleanup_stage implementation in the base ImportStep class that relies on abstract methods registry_type and stage_storage
• Modified storage transfer operations from copy to move for VFS-to-VFS transfers, with automatic cleanup of empty parent directories
• Optimized archive transfer to move entire model directories at once instead of transferring files individually

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/ai/backend/storage/services/artifacts/types.py	Added abstract methods registry_type and stage_storage, provided default cleanup_stage implementation
src/ai/backend/storage/services/artifacts/storage_transfer.py	Changed copy operations to move operations, added _move_vfs_directory and _cleanup_empty_parents methods for VFS transfers
src/ai/backend/storage/services/artifacts/reservoir.py	Implemented new abstract methods, removed custom cleanup implementation
src/ai/backend/storage/services/artifacts/huggingface.py	Implemented new abstract methods, removed custom cleanup implementation
src/ai/backend/storage/services/artifacts/common.py	Implemented stage_storage for verify/archive steps, removed custom cleanup, changed to transfer entire directories, fixed typo in variable name
changes/7115.fix.md	Added changelog entry for the fix

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment states "Default cleanup implementation" but the method is no longer abstract, making it concrete. This implementation relies on abstract methods registry_type and stage_storage that must be implemented by subclasses. Consider clarifying the comment to indicate this is a concrete implementation that can be overridden if needed, rather than calling it "default" which might imply it does nothing.

Suggested change

	"""Default cleanup implementation that removes files"""
	"""
	Concrete cleanup implementation that removes files for this step.
	This method relies on the abstract methods `registry_type` and `stage_storage`,
	which must be implemented by subclasses. Subclasses may override this method
	to customize cleanup behavior if needed.
	"""

[Copilot]

The _cleanup_empty_parents method uses synchronous rmdir() and file system operations without running them in an executor. While this might be acceptable for lightweight operations like checking if a directory is empty and removing it, it could block the event loop if the filesystem is slow (e.g., network-mounted storage). Consider wrapping the filesystem operations in asyncio.get_event_loop().run_in_executor(None, ...) for consistency with other filesystem operations in this file.

Suggested change

	self._cleanup_empty_parents(source_path.parent, source_storage.base_path)
	def _cleanup_empty_parents(self, path: Path, base_path: Path) -> None:
	"""Remove empty parent directories up to base_path."""
	current = path
	while current != base_path and current.exists():
	try:
	current.rmdir() # Only removes if empty
	await self._cleanup_empty_parents(source_path.parent, source_storage.base_path)
	async def _cleanup_empty_parents(self, path: Path, base_path: Path) -> None:
	"""Remove empty parent directories up to base_path."""
	loop = asyncio.get_event_loop()
	current = path
	while current != base_path and await loop.run_in_executor(None, current.exists):
	try:
	await loop.run_in_executor(None, current.rmdir) # Only removes if empty

[Copilot]

[nitpick] In the directory cleanup loop, if current == base_path initially (unlikely but possible if path and base_path are the same), the while condition prevents execution but doesn't provide any logging or indication. While this is likely a safe guard, consider adding a check or log statement if this edge case is encountered to aid debugging.

[Copilot]

The changelog filename is 7115.fix.md but the PR description mentions issue #7114 (BA-3250). There's a mismatch between the changelog filename (7115) and the referenced issue number (7114). Ensure the changelog filename matches the correct issue number.

[Copilot]

The _move_vfs_to_vfs method (used for individual file transfers) does not include cleanup of empty parent directories, while _move_vfs_directory does (line 180). This inconsistency means that when moving individual files between VFS storages, empty parent directories may be left behind. Consider adding the same cleanup logic here for consistency, or document why it's not needed for individual file moves.

Suggested change

	)
	)
	# Cleanup empty artifact directories after moving a file
	self._cleanup_empty_parents(source_full_path.parent, source_storage.base_path)

[Copilot]

Path comparison using current != base_path (line 185) may not work as expected if the paths are not normalized or if they differ in representation (e.g., relative vs absolute, symlinks). Consider using current.resolve() != base_path.resolve() or not current.is_relative_to(base_path) for more robust path comparison, or check if current is a parent of or equal to base_path using path methods.

Suggested change

	while current != base_path and current.exists():
	while current.resolve() != base_path.resolve() and current.exists():

[HyeockJinKim]

Overall, it looks like a repeating pattern, so it would be more natural to just return which step it is and structure it from the top.