Skip to content

fix: Correctly handle symbolic link renames #3648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 7, 2025
Merged

fix: Correctly handle symbolic link renames #3648

merged 1 commit into from
Aug 7, 2025

Conversation

@vipnydav
Copy link
Member

@vipnydav vipnydav commented Aug 6, 2025

Description

A recent refactoring of the Rename operation in internal/fs/fs.go introduced an issue that prevented sym links from being renamed.

The new implementation incorrectly assumed that all inodes being renamed would implement the inode.BucketOwnedInode interface. Since SymlinkInode did not, any attempt to rename a symlink would fail with the error: Rename: input/output error, child inode (id <X>) is not owned by any bucket.

This change resolves the issue by making SymlinkInode implement the BucketOwnedInode interface. This involves:

  1. Adding a bucket field to SymlinkInode.
  2. Updating the NewSymlinkInode function and its call sites to correctly propagate bucket information.
  3. Adding logic within the Rename function to correctly handle symlinks as bucket-owned inodes.

With this fix, renaming symbolic links now works as expected.

Link to the issue in case of a bug fix.

https://b.corp.google.com/issues/436756784

Testing details

  1. Manual - Done
  2. Unit tests - Added
  3. Integration tests - Automated

Any backward incompatible change? If so, please explain.

@vipnydav vipnydav requested a review from a team as a code owner August 6, 2025 12:51
@vipnydav vipnydav requested a review from ashmeenkaur August 6, 2025 12:51
@github-actions github-actions bot added the remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR. label Aug 6, 2025
@kislaykishore kislaykishore requested review from a team and anushka567 and removed request for a team August 6, 2025 12:52
@vipnydav vipnydav changed the title fix: Handle Symlink scenario in rename fix: Correctly handle symbolic link renames Aug 6, 2025
@codecov
Copy link

codecov bot commented Aug 6, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.36%. Comparing base (7274664) to head (c2be5dd).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
internal/fs/fs.go 80.00% 4 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3648      +/-   ##
==========================================
+ Coverage   82.33%   82.36%   +0.03%     
==========================================
  Files         146      146              
  Lines       22624    22646      +22     
==========================================
+ Hits        18627    18652      +25     
+ Misses       3447     3445       -2     
+ Partials      550      549       -1
Flag Coverage Δ
unittests 82.36% <87.80%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ashmeenkaur
Copy link
Collaborator

ashmeenkaur commented Aug 6, 2025

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Aug 6, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly fixes an issue preventing symbolic links from being renamed. The approach of making SymlinkInode implement the BucketOwnedInode interface and updating the Rename logic is sound. The changes are well-structured and supported by a good set of new tests covering various scenarios. I've added a couple of minor suggestions to improve an error message for better debugging and to enhance a test assertion for better failure diagnosis. Great work!

ashmeenkaur
ashmeenkaur approved these changes Aug 7, 2025
View reviewed changes
@vipnydav vipnydav merged commit 18f780f into master Aug 7, 2025
18 checks passed
@gromgit gromgit mentioned this pull request Aug 11, 2025
Closed
ashmeenkaur pushed a commit that referenced this pull request Aug 12, 2025
@vipnydav @ashmeenkaur
Handle Symlink scenario in rename (#3648)
4f4c978
charith87 pushed a commit that referenced this pull request Aug 20, 2025
@vipnydav @charith87
Handle Symlink scenario in rename (#3648)
db59358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashmeenkaur ashmeenkaur ashmeenkaur approved these changes

@anushka567 anushka567 Awaiting requested review from anushka567 anushka567 was automatically assigned from GoogleCloudPlatform/gcsfuse-shadow-reviewers

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vipnydav @ashmeenkaur