Skip to content

Unlink broken symlinks in the assets folder#6140

Open
masenf wants to merge 1 commit intomainfrom
masenf/unlink-bad-symlinks
Open

Unlink broken symlinks in the assets folder#6140
masenf wants to merge 1 commit intomainfrom
masenf/unlink-bad-symlinks

Conversation

@masenf
Copy link
Collaborator

@masenf masenf commented Feb 19, 2026

Certain build environments, such as docker bind mounts, can create symlinks that exist, but cannot be overwritten for whatever reason. So we unlink those for Simon 🎁

Certain build environments, such as docker bind mounts, can create symlinks
that exist, but cannot be overwritten for whatever reason. So we unlink those
for Simon 🎁
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 19, 2026

Merging this PR will not alter performance

✅ 8 untouched benchmarks


Comparing masenf/unlink-bad-symlinks (644a2bb) with main (88155a7)

Open in CodSpeed

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 19, 2026

Greptile Summary

Added error handling for FileExistsError when creating symlinks in the assets folder, specifically to handle broken symlinks in Docker bind mount environments.

  • Wrapped dst_file.symlink_to() in try-catch block
  • On FileExistsError, unlinks the existing path and recreates the symlink
  • Solves issue where broken symlinks cannot be overwritten in certain build environments

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk
  • The change is a focused bug fix for a specific Docker environment issue. The try-catch error handling is appropriate, though there's a minor edge case around race conditions that could be made more robust by verifying it's a symlink before unlinking
  • No files require special attention

Important Files Changed

Filename Overview
reflex/assets.py Added try-catch to handle FileExistsError when creating symlinks in Docker environments; potential edge case with race conditions

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Check if dst_file exists and symlink is valid] --> B{Needs new symlink?}
    B -->|No| C[Return asset path]
    B -->|Yes| D[Try to create symlink]
    D --> E{FileExistsError?}
    E -->|No| C
    E -->|Yes| F[Unlink existing path]
    F --> G[Create symlink]
    G --> C
Loading

Last reviewed commit: 644a2bb

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +97 to +100
except FileExistsError:
# This happens when Simon builds the app on a bind mount in a docker container.
dst_file.unlink()
dst_file.symlink_to(src_file_shared)
Copy link
Contributor

Choose a reason for hiding this comment

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

Catching FileExistsError and blindly unlinking could be risky if a race condition creates a legitimate file between the check on line 92 and the symlink creation. Consider checking dst_file.is_symlink() before unlinking to ensure you're only removing symlinks:

Suggested change
except FileExistsError:
# This happens when Simon builds the app on a bind mount in a docker container.
dst_file.unlink()
dst_file.symlink_to(src_file_shared)
except FileExistsError:
# This happens when Simon builds the app on a bind mount in a docker container.
if dst_file.is_symlink():
dst_file.unlink()
dst_file.symlink_to(src_file_shared)
else:
raise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments