fix symlink handling on backups - #203
Conversation
Add Readlink and Readlinkat methods to UnixFS for symbolic link handling.
Add handling for symlink target resolution in archive.go
📝 WalkthroughWalkthroughChangesSymlink backup and restoration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FilesystemArchive
participant UnixFS
participant TarArchive
participant LocalOrS3Restore
participant RestoreCallback
participant RestoredFilesystem
FilesystemArchive->>UnixFS: Readlinkat(dirfd, name)
UnixFS-->>FilesystemArchive: return symlink target
FilesystemArchive->>TarArchive: write symlink metadata and target
LocalOrS3Restore->>RestoreCallback: pass linkTarget and nil reader
RestoreCallback->>RestoredFilesystem: create parent directory and symlink
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/filesystem/archive.go`:
- Around line 278-280: Update the symlink handling around Readlinkat in the
archive creation flow to set the generated tar header’s Name to the full
relative path after obtaining it from tar.FileInfoHeader, preserving nested
symlink paths. Add an extraction case covering a nested symlink such as
nested/unix_args.txt.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 869b92ac-1913-4544-8780-3925c161ac37
📒 Files selected for processing (9)
internal/ufs/fs_unix.gointernal/ufs/fs_unix_test.goserver/backup.goserver/backup/backup.goserver/backup/backup_local.goserver/backup/backup_s3.goserver/backup/backup_test.goserver/filesystem/archive.goserver/filesystem/archive_test.go
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-02T13:53:08.995Z
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In the server package, when quotas are enabled via config.Get().System.Quotas.Enabled, the disk space check using used >= s.DiskSpace() does not require a special guard for unlimited-disk scenarios (DiskSpace() <= 0). The filesystem handles such cases, so the existing check is sufficient. Apply this pattern to similar quota-related disk checks in the server package and ensure tests/docs reflect that unlimited-disk behavior is governed by the filesystem, not by an extra guard in code.
Applied to files:
server/filesystem/archive.goserver/backup/backup_s3.goserver/backup/backup_local.goserver/backup/backup.goserver/filesystem/archive_test.goserver/backup.goserver/backup/backup_test.go
🔇 Additional comments (9)
internal/ufs/fs_unix.go (1)
859-883: LGTM!internal/ufs/fs_unix_test.go (1)
613-723: LGTM!server/filesystem/archive_test.go (2)
4-7: LGTM!Also applies to: 91-125
176-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCopy each TAR header before storing it.
tar.Reader.Nextcan reuse metadata between calls, so the retained pointers inentriesmay share or be overwritten by later reads. Copy the header when the map stores it.Proposed fix
- entries[hdr.Name] = hdr + header := *hdr + entries[header.Name] = &header> Likely an incorrect or invalid review comment.server/backup/backup.go (1)
39-39: LGTM!server/backup/backup_local.go (1)
131-140: LGTM!server/backup/backup_s3.go (1)
111-120: LGTM!server/backup.go (1)
7-7: LGTM!Also applies to: 155-167
server/backup/backup_test.go (1)
4-176: LGTM!
| target, err = a.Filesystem.unixFS.Readlinkat(dirfd, name) | ||
| if err != nil { | ||
| // Ignore the not exist errors specifically, since there is nothing important about that. | ||
| if !os.IsNotExist(err) { | ||
| if !errors.Is(err, ufs.ErrNotExist) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What value does Go's archive/tar.FileInfoHeader use for Header.Name when given a symbolic-link FileInfo?
💡 Result:
When given a symbolic-link FileInfo, Go's archive/tar.FileInfoHeader function sets the Header.Name field to the value returned by the FileInfo's Name method [1][2][3]. The FileInfoHeader function initializes the Header by calling fi.Name on the provided fs.FileInfo object [1][3]. Unlike directories—where the function explicitly appends a trailing slash to the Name field [4][5][3]—it does not perform any additional modification to the Name field when the file mode indicates a symbolic link (fm & fs.ModeSymlink!= 0) [1][3]. Because fs.FileInfo's Name method only returns the base name of the file, the official documentation advises that it may be necessary for the caller to manually modify the resulting Header.Name to represent the correct full path within the archive [4][5][1].
Citations:
- 1: https://go.dev/src/archive/tar/common.go?m=text
- 2: https://go.googlesource.com/go/+/refs/tags/go1.24.5/src/archive/tar/common.go
- 3: https://github.com/golang/go/blob/d468ad36/src/archive/tar/common.go
- 4: https://pkg.go.dev/archive/tar
- 5: https://pkg.go.dev/archive/tar@go1.26.5
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate archive.go =="
fd -a 'archive\.go$' . || true
echo "== relevant lines =="
if [ -f server/filesystem/archive.go ]; then
nl -ba server/filesystem/archive.go | sed -n '250,310p'
fi
echo "== search addToArchive/addToArchive tests =="
rg -n "addToArchive|nested/unix_args|unix_args|Readlinkat|FileInfoHeader|ModeSymlink" server/filesystem/archive.go server/filesystem tests 2>/dev/null || true
echo "== tar FileInfoHeader behavior probe from installed Go source if available =="
go env GOROOT MOD 2>/dev/null || true
if [ -f "$(go env GOROOT)/src/archive/tar/common.go" ]; then
python3 - <<'PY'
import pathlib, re
p = pathlib.Path((__import__('os').getenv('GOROOT') or '').strip() + '/src/archive/tar/common.go')
# simple: locate FileInfoHeader and extract handling; no import of local repo
PY
rg -n "func FileInfoHeader|ModeSymlink|Name\(\)" "$(go env GOROOT)/src/archive/tar/common.go"
fiRepository: pelican-dev/wings
Length of output: 334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== available tools =="
command -v awk || true
echo "== relevant lines in server/filesystem/archive.go =="
awk '{printf "%6d %s\n", NR, $0}' server/filesystem/archive.go | sed -n '260,300p'
echo "== search addToArchive/tests/relevant symbols =="
rg -n "addToArchive|nested/unix_args|unix_args|Readlinkat|FileInfoHeader|ModeSymlink|tar.Header|WriteHeader" server/filesystem archive.go . 2>/dev/null || true
echo "== tar FileInfoHeader source if available =="
if command -v go >/dev/null 2>&1 && [ -d "$(go env GOROOT 2>/dev/null)/src/archive/tar" ]; then
GOROOT="$(go env GOROOT)"
awk '{printf "%6d %s\n", NR, $0}' "$GOROOT/src/archive/tar/common.go" | sed -n '200,280p'
else
echo "Go source not available via GOROOT"
fi
echo "== deterministic Go stdlib behavior probe via available command =="
if command -v go >/dev/null 2>&1; then
go env GOROOT GOCACHE 2>/dev/null || true
fiRepository: pelican-dev/wings
Length of output: 10498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== archive test around helpers and fixture setup =="
awk '{printf "%6d %s\n", NR, $0}' server/filesystem/archive_test.go | sed -n '1,240p'
echo "== call sites for addToArchive =="
awk '{printf "%6d %s\n", NR, $0}' server/filesystem/archive.go | sed -n '200,245p'
echo "== exact FileInfoHeader symbol in Go source =="
if [ -d /usr/local/go/src/archive/tar ]; then
rg -n "func FileInfoHeader|header\.(Name|Typeflag)|f\.fi\.Name\(\)" /usr/local/go/src/archive/tar/*.go
fiRepository: pelican-dev/wings
Length of output: 8451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Go FileInfoHeader implementation =="
awk '{printf "%6d %s\n", NR, $0}' /usr/local/go/src/archive/tar/common.go | sed -n '600,690p'
echo "== ReadDirFS extraction helper =="
awk '{printf "%6d %s\n", NR, $0}' server/filesystem/archive_test.go | sed -n '129,162p'Repository: pelican-dev/wings
Length of output: 4375
Preserve symlink paths before writing tar headers.
tar.FileInfoHeader uses fi.Name(), so nested symlinks are stored with only their base name. Set header.Name = relative after obtaining the symlink header, and add a nested-symlink extraction case such as nested/unix_args.txt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/filesystem/archive.go` around lines 278 - 280, Update the symlink
handling around Readlinkat in the archive creation flow to set the generated tar
header’s Name to the full relative path after obtaining it from
tar.FileInfoHeader, preserving nested symlink paths. Add an extraction case
covering a nested symlink such as nested/unix_args.txt.
|
@perchiban Right now we are going through a rewrite of the filesystem handlers. Mainly replacing the ufs work you have here. We can leave this open while that gets worked on. |
2 participants
Added symlink handling on backups to fix #168
Summary by CodeRabbit
New Features
Bug Fixes