Repository navigation
fix(kvstoreentry): close directory upload files and rewind before retry - #1966
Tyagiquamar wants to merge 4 commits into
Conversation
Close each file opened in ProcessDir and seek to the start before retrying insert after a connection-loss rewind error. Fixes fastly#1942 Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the contribution! This pull request doesn't follow our pull request template:
Please edit the description to use the template. If it is still missing after 3 days, this pull request will be closed automatically. |
ProcessDir tests construct CreateCommand without CLI flag defaults, leaving dirConcurrency at zero and deadlocking the upload semaphores. Use the same concurrency as the --dir-concurrency default and normalize test file LF. Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
|
Please update the description to use the PR template. Otherwise the bot will close the PR. Also: the changelog entry should link to this PR (#1966), not to the issue. |
| APIClient: &mock.API{ | ||
| InsertKVStoreKeyFn: func(_ context.Context, in *fastly.InsertKVStoreKeyInput) error { | ||
| if in.Key == "foo.txt" && capturedBody == nil { | ||
| if lr, ok := in.Body.(fastly.LengthReader); ok { |
There was a problem hiding this comment.
in.Body is already a fastly.LengthReader so you can write capturedBody = in.Body directly.
| } | ||
| } | ||
|
|
||
| func TestProcessDirRetryStillFailsReportsConnectionLoss(t *testing.T) { |
There was a problem hiding this comment.
That test doesn't test these changes. Remove it, or at least make it check that the upload is tried only twice.
| lr, rewindErr := fastly.FileLengthReader(f) | ||
| if rewindErr != nil { | ||
| mu.Lock() | ||
| processingErrors = append(processingErrors, ProcessErr{ | ||
| File: filePath, | ||
| Err: rewindErr, | ||
| }) | ||
| mu.Unlock() | ||
| return | ||
| } | ||
| opts.file = lr |
There was a problem hiding this comment.
That block can go.
Going back to the start of the file is enough. It still reads from the same file, so there's no need to call FileLengthReader() again.
jedisct1
left a comment
jedisct1
left a comment
There was a problem hiding this comment.
Thanks for these changes.
Left a few comments on things that should maybe be changed before it can be merged.
Close opened files after directory upload and rewind with Seek(0) before retrying insert after connection loss, per review feedback. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Closing because the pull request template is still missing after 3 days. Feel free to open a new pull request that uses the template. |
Problem
kv-store-entry create --diropens each file withos.OpeninProcessDirand never closes it, which can exhaust file descriptors on large directories. On a retry afternet/http: cannot rewind body after connection loss, the first attempt already consumed the request body, so the retry could upload empty or truncated content.Reproduction
See issue #1942: read from the captured request body after upload (still open, not
os.ErrClosed); simulate connection-loss error on first insert and observe empty body on retry.Root cause
Missing
Close()on opened files and no seek back to offset 0 before the retry path.Solution
defer f.Close()after each successfulos.Open. On the connection-loss retry path,Seek(0, io.SeekStart)on the same open file and reuse the existingFileLengthReader(no secondFileLengthReadercall).Tests
create_processdir_test.go:TestProcessDirClosesUploadedFilesTestProcessDirRetriesAfterConnectionLossWithRewoundBody(asserts exactly two insert attempts)Test plan
go test ./pkg/commands/kvstoreentry/... -run 'TestProcessDir' -count=1(Docker)Fixes #1942