Skip to content

plumbing: object, fix Change.PatchContext hardcoded 1-hour internal timeout #1910

Open
bitpeng wants to merge 1 commit intogo-git:mainfrom
bitpeng:fix-patchcontext-with-deadline
Open

plumbing: object, fix Change.PatchContext hardcoded 1-hour internal timeout #1910
bitpeng wants to merge 1 commit intogo-git:mainfrom
bitpeng:fix-patchcontext-with-deadline

Conversation

@bitpeng
Copy link
Copy Markdown

@bitpeng bitpeng commented Mar 19, 2026

There is a hidden bug in the Change.PatchContext method: even when users call this function with a ctx that has a timeout of only a few seconds, filePatchWithContext always uses diff.Do(fromContent, toContent) — which has a one-hour execution timeout. This is extremely confusing for users.

Copy link
Copy Markdown
Contributor

@AriehSchneier AriehSchneier left a comment

Choose a reason for hiding this comment

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

The direction is right — diff.Do has a hardcoded 1-hour internal timeout that completely ignores any shorter context deadline, so this is a real bug worth fixing. A few things to address:

Also consider adding an early cancellation check at the top of filePatchWithContext, before the expensive fileContent reads:

if err := ctx.Err(); err != nil {
    return nil, ErrCanceled
}

This avoids reading file content and running the diff entirely if the context is already done on entry.

A test covering the new codepath would also be valuable — e.g. pass a context with a 1ms deadline and verify the function returns promptly rather than running for up to an hour.

Comment thread plumbing/object/patch.go Outdated
// Choose the appropriate diff method based on the remaining timeout duration.
// If there's no remaining time (or it's negative), use the regular diff.Do.
// Otherwise, use diff.DoWithTimeout with the calculated remaining time.
if remaining <= 0 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If remaining <= 0 the deadline has already passed — falling back to diff.Do here means the function will run for up to an hour on an already-expired context. Should return early instead:

if !ok {
    diffs = diff.Do(fromContent, toContent)
} else if remaining <= 0 {
    return nil, ErrCanceled
} else {
    diffs = diff.DoWithTimeout(fromContent, toContent, remaining)
}

Comment thread plumbing/object/patch.go
if remaining <= 0 {
diffs = diff.Do(fromContent, toContent)
} else {
diffs = diff.DoWithTimeout(fromContent, toContent, remaining)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth adding a comment: diff.DoWithTimeout silently returns a best-effort (potentially incomplete) diff when its internal timeout fires — it doesn't surface an error. The ctx.Done() check in the loop below won't reliably catch this. This is a limitation of the go-diff API, but worth documenting so callers aren't surprised.

@bitpeng
Copy link
Copy Markdown
Author

bitpeng commented Mar 19, 2026

Could you please review the latest commit? Also, should I squash the commits? @AriehSchneier

@bitpeng bitpeng changed the title plumbing: object, fix Change.PatchContext when calling this function … plumbing: object, fix Change.PatchContext hardcoded 1-hour internal timeout Mar 19, 2026
@AriehSchneier
Copy link
Copy Markdown
Contributor

Could you please review the latest commit? Also, should I squash the commits? @AriehSchneier

Looks good now. You will need to squash. I still suggest adding a test.

@bitpeng bitpeng force-pushed the fix-patchcontext-with-deadline branch from 2dd6356 to 86e8452 Compare March 19, 2026 10:51
@bitpeng
Copy link
Copy Markdown
Author

bitpeng commented Mar 19, 2026

The commits have been squashed. Apologies, there was no existing test function for filePatchWithContext. As I'm not very familiar with go-git's test suite, it's difficult for me to add appropriate test cases for this commit in a short time.

@AriehSchneier
Copy link
Copy Markdown
Contributor

Not sure it tests all the new paths, but maybe something like:

// modifyChange returns a Change representing a modification to a known file
// in the go-git fixture, suitable for exercising filePatchWithContext.
func (s *ChangeSuite) modifyChange() *Change {
	path := "utils/difftree/difftree.go"
	name := "difftree.go"
	mode := filemode.Regular
	fromBlob := plumbing.NewHash("05f583ace3a9a078d8150905a53a4d82567f125f")
	fromTree := plumbing.NewHash("b1f01b730b855c82431918cb338ad47ed558999b")
	toBlob := plumbing.NewHash("de927fad935d172929aacf20e71f3bf0b91dd6f9")
	toTree := plumbing.NewHash("8b0af31d2544acb5c4f3816a602f11418cbd126e")
	return &Change{
		From: ChangeEntry{
			Name:      path,
			Tree:      s.tree(fromTree),
			TreeEntry: TreeEntry{Name: name, Mode: mode, Hash: fromBlob},
		},
		To: ChangeEntry{
			Name:      path,
			Tree:      s.tree(toTree),
			TreeEntry: TreeEntry{Name: name, Mode: mode, Hash: toBlob},
		},
	}
}

// TestPatchContextDeadlineExpired verifies that filePatchWithContext returns
// ErrCanceled when the context deadline has already expired before the call.
func (s *ChangeSuite) TestPatchContextDeadlineExpired() {
	change := s.modifyChange()

	// Deadline already in the past — filePatchWithContext must detect this
	// immediately and return ErrCanceled without performing any diff work.
	ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second))
	defer cancel()

	fp, err := filePatchWithContext(ctx, change)
	s.Nil(fp)
	s.ErrorIs(err, ErrCanceled)
}

// TestPatchContextWithActiveDeadline verifies that filePatchWithContext
// succeeds and returns a non-empty patch when given a context with ample time
// remaining, exercising the diff.DoWithTimeout code path.
func (s *ChangeSuite) TestPatchContextWithActiveDeadline() {
	change := s.modifyChange()

	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	defer cancel()

	fp, err := filePatchWithContext(ctx, change)
	s.NoError(err)
	s.NotNil(fp)
	s.NotEmpty(fp.Chunks())
}

@bitpeng bitpeng force-pushed the fix-patchcontext-with-deadline branch from 86e8452 to c2fc8a1 Compare March 20, 2026 04:55
@bitpeng
Copy link
Copy Markdown
Author

bitpeng commented Mar 20, 2026

Thanks, I've added the test cases as you suggested. Could you please re-review this at your convenience?

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.

3 participants