Skip to content
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.

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

Avoid reading from deleted file #3218

Merged
merged 3 commits into from
Feb 20, 2025
Merged

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Feb 20, 2025

While working on #3174 I noticed that deletion events weren't being triggered. It was failing because we try to read the file after it was deleted. It was failing silently because of the rescue Errno::ENOENT.

Copy link

graphite-app bot commented Feb 20, 2025

How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@andyw8 andyw8 added bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes labels Feb 20, 2025
@andyw8 andyw8 force-pushed the andyw8/dont-try-read-deleted-file branch from fc79a17 to 5152d12 Compare February 20, 2025 16:36
@andyw8 andyw8 changed the title Don't try read deleted file Avoid reading from deleted file Feb 20, 2025
@andyw8 andyw8 marked this pull request as ready for review February 20, 2025 16:40
@andyw8 andyw8 requested a review from a team as a code owner February 20, 2025 16:40
@andyw8 andyw8 requested a review from alexcrocha February 20, 2025 16:41
@andyw8
Copy link
Contributor Author

andyw8 commented Feb 20, 2025

You can see from the coverage report how we missed this:

Screenshot 2025-02-20 at 11 52 28 AM

@andyw8 andyw8 added the graphite-merge Ship this PR using Graphite's merge queue label Feb 20, 2025
Copy link
Contributor Author

andyw8 commented Feb 20, 2025

Merge activity

  • Feb 20, 1:50 PM EST: The merge label 'graphite-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Feb 20, 5:12 PM EST: The merge label 'graphite-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.

@andyw8 andyw8 merged commit ac8f802 into main Feb 20, 2025
43 checks passed
@andyw8 andyw8 deleted the andyw8/dont-try-read-deleted-file branch February 20, 2025 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug graphite-merge Ship this PR using Graphite's merge queue server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants