fix(localfile): harden wildcard Read (per-file fd release + final grep line) - #634
Open
sfc-gh-ikryvanos wants to merge 2 commits into
Open
fix(localfile): harden wildcard Read (per-file fd release + final grep line)#634sfc-gh-ikryvanos wants to merge 2 commits into
sfc-gh-ikryvanos wants to merge 2 commits into
Conversation
The wildcard Read worker opened each matched file and deferred both the file close and the dataPrep cleanup inside the consumer loop, so every descriptor stayed open until the whole request finished. A read over a large directory could therefore exhaust the process's file-descriptor limit (on Linux each file also allocates inotify/epoll fds). The producer also blocked forever on a full channel if consumers exited early, leaking that goroutine. Move the per-file body verbatim into readSingleFile so its defers run per file, make the producer send honor context cancellation, and always close the channel once. The extracted body is unchanged, so reviewing with whitespace hidden shows only the real changes. Adds a regression test that reads a wildcard directory under a constrained RLIMIT_NOFILE. Co-authored-by: Cursor <cursoragent@cursor.com>
…g newline Read's grep path buffers each line in trailingLine until it sees a '\n'. When a file's last line had no trailing newline, that line stayed in the buffer and was silently dropped on EOF, so the end of the file was missing from grep results. Flush any remaining trailingLine through the matcher at EOF (honoring invert-match) before finishing the file. The existing max-line-length guard still bounds buffer growth, so this doesn't reintroduce unbounded memory use. Add TestReadDifferentLengthFiles covering files of varying lengths around the streaming chunk boundary: byte-exact plain reads, grep of unterminated final lines (including one spanning a chunk), invert-match, and a wildcard total-bytes check. Co-authored-by: Cursor <cursoragent@cursor.com>
| return status.Error(codes.InvalidArgument, "wildcard reads are not supported in declared compatible API version.") | ||
| } | ||
| errs.Go(func() error { | ||
| if err := s.listFor(path, ctx, func(item *pb.StatReply) error { |
Collaborator
There was a problem hiding this comment.
Could you please add PR description?
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
2 participants
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
Two related availability/correctness fixes in the
LocalFileReadpath:Per-file descriptor release in wildcard reads. Wildcard reads processed every matched file inside one goroutine loop, so the open file and
dataPrepcloser were held by loop-scopeddefers that only ran when the whole request finished. A large directory could therefore exhaust the process's file descriptors. Each file is now handled in its ownreadSingleFilecall so its descriptors are released as soon as that file is done. The producer also respects context cancellation so it can't block forever (and leak the goroutine) if the consumers have already exited on error or client disconnect.Return the final line when grepping files without a trailing newline. The grep path buffers each line in
trailingLineuntil it sees a\n. A file whose last line had no trailing newline left that line in the buffer, and it was silently dropped on EOF — so the end of the file was missing from grep results. It's now flushed through the matcher at EOF (honoring invert-match) before finishing the file. The existing max-line-length guard still bounds buffer growth, so this doesn't reintroduce unbounded memory use.Test plan
TestReadWildcardManyFilesNoFDLeak: reads a directory with many files under a constrainedRLIMIT_NOFILE; fails with the old loop-scoped defers, passes now.TestReadDifferentLengthFiles: files of varying lengths around the streaming chunk boundary — byte-exact plain reads; grep of unterminated final lines (including one spanning a chunk boundary); invert-match; and a wildcard total-bytes check.go test ./services/localfile/server/passes;go vetclean.