feat(BA-5504): Update file operation services to set vfolder updated_at on mutations#10821
Open
seedspirit wants to merge 2 commits intomainfrom
Open
feat(BA-5504): Update file operation services to set vfolder updated_at on mutations#10821seedspirit wants to merge 2 commits intomainfrom
updated_at on mutations#10821seedspirit wants to merge 2 commits intomainfrom
Conversation
seedspirit
added a commit
that referenced
this pull request
Apr 6, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
size:L
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces a new updated_at timestamp on vfolders and updates the vfolder file-operation service to bump that timestamp whenever file mutations occur.
Changes:
- Added nullable
updated_attoVFolderRow/VFolderDataand a repository helper to set it tonow(). - Updated
VFolderFileServicemutation methods (upload/rename/move/delete + v2 variants) to callupdate_vfolder_updated_at. - Expanded unit tests to assert
updated_atis bumped for mutations and not bumped for certain non-mutation operations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/manager/services/vfolder/test_vfolder_file_service.py | Adds assertions and new cases to verify update_vfolder_updated_at is (or isn’t) called. |
| src/ai/backend/manager/services/vfolder/services/file.py | Calls the repository method to bump updated_at after mutations (including v2). |
| src/ai/backend/manager/repositories/vfolder/repository.py | Adds update_vfolder_updated_at() and maps updated_at into VFolderData. |
| src/ai/backend/manager/models/vfolder/row.py | Adds the updated_at ORM column and includes it in to_data(). |
| src/ai/backend/manager/data/vfolder/types.py | Adds updated_at to VFolderData. |
| changes/10821.feature.md | Adds a changelog entry for the behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .where(VFolderRow.id == vfolder_id) | ||
| .values(updated_at=sa.func.now()) | ||
| ) | ||
| await session.execute(query) |
There was a problem hiding this comment.
update_vfolder_updated_at() ignores the case where vfolder_id doesn’t exist (the UPDATE affects 0 rows and silently succeeds). This can hide data integrity issues and differs from other repository methods that raise VFolderNotFound. Consider checking the execute result’s rowcount and raising VFolderNotFound() when it is 0.
Suggested change
| await session.execute(query) | |
| result = await session.execute(query) | |
| if result.rowcount == 0: | |
| raise VFolderNotFound() |
Contributor
Author
There was a problem hiding this comment.
A vfolder_id that does not exist in the call path cannot be provided. The track_content_update method is called only after the file operation service has verified the existence of the vfolder
seedspirit
changed the title
github-actions
bot
added
the
require:db-migration
seedspirit
changed the title
updated_at on mutations
jopemachine
reviewed
Apr 9, 2026
Comment on lines
+636
to
+697
| async def test_mkdir_tracks( | ||
| self, | ||
| file_service: VFolderFileService, | ||
| mock_vfolder_repository: MagicMock, | ||
| mock_vfolder_data: MagicMock, | ||
| user_uuid: uuid.UUID, | ||
| vfolder_uuid: uuid.UUID, | ||
| ) -> None: | ||
| mock_vfolder_repository.get_by_id_validated = AsyncMock(return_value=mock_vfolder_data) | ||
|
|
||
| action = MkdirAction( | ||
| user_id=user_uuid, | ||
| vfolder_uuid=vfolder_uuid, | ||
| path="newdir", | ||
| parents=False, | ||
| exist_ok=False, | ||
| ) | ||
| await file_service.mkdir(action) | ||
|
|
||
| mock_vfolder_repository.track_content_update.assert_called_once_with(vfolder_uuid) | ||
|
|
||
| async def test_list_files_does_not_track( | ||
| self, | ||
| file_service: VFolderFileService, | ||
| mock_vfolder_repository: MagicMock, | ||
| mock_vfolder_data: MagicMock, | ||
| user_uuid: uuid.UUID, | ||
| vfolder_uuid: uuid.UUID, | ||
| ) -> None: | ||
| mock_vfolder_repository.get_by_id_validated = AsyncMock(return_value=mock_vfolder_data) | ||
|
|
||
| action = ListFilesAction( | ||
| user_uuid=user_uuid, | ||
| vfolder_uuid=vfolder_uuid, | ||
| path=".", | ||
| ) | ||
| await file_service.list_files(action) | ||
|
|
||
| mock_vfolder_repository.track_content_update.assert_not_called() | ||
|
|
||
| async def test_download_does_not_track( | ||
| self, | ||
| file_service: VFolderFileService, | ||
| mock_vfolder_repository: MagicMock, | ||
| mock_vfolder_data: MagicMock, | ||
| user_uuid: uuid.UUID, | ||
| vfolder_uuid: uuid.UUID, | ||
| keypair_resource_policy: dict[str, str], | ||
| ) -> None: | ||
| mock_vfolder_repository.get_by_id_validated = AsyncMock(return_value=mock_vfolder_data) | ||
| mock_vfolder_repository.ensure_host_permission_allowed = AsyncMock() | ||
|
|
||
| action = CreateDownloadSessionAction( | ||
| keypair_resource_policy=keypair_resource_policy, | ||
| user_uuid=user_uuid, | ||
| vfolder_uuid=vfolder_uuid, | ||
| path="data/file.bin", | ||
| archive=False, | ||
| ) | ||
| await file_service.download_file(action) | ||
|
|
||
| mock_vfolder_repository.track_content_update.assert_not_called() |
Member
There was a problem hiding this comment.
These tests look like they could be consolidated into a single parametrized test, for example by extracting the actions into fixtures.
jopemachine
reviewed
Apr 9, 2026
Comment on lines
+20
to
+23
| op.add_column( | ||
| "vfolders", | ||
| sa.Column("updated_at", sa.DateTime(timezone=True), nullable=True), | ||
| ) |
Member
There was a problem hiding this comment.
Is it okay not to set last_used or a default value for updated_at?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
updated_atnullable timestamp column toVFolderRowand a repository methodupdate_vfolder_updated_at()that sets it tonow()update_vfolder_updated_atfrom allVFolderFileServicemutation methods: upload, rename, move, delete (sync/async), and v2 variantsupdated_atis set after each mutation and remains NULL on read-only operationsTest plan
update_vfolder_updated_atis called for upload, rename, move, delete (sync), delete (async), move v2, delete v2update_vfolder_updated_atpants test --changed-since=HEAD~3passesResolves BA-5504