prevent concurrent fetches between user and status indicator checks by shiftkey · Pull Request #6141 · desktop/desktop

shiftkey · 2018-11-09T13:58:07Z

Overview

This PR does some tidy up around how our background fetching from #4770 integrates with other operations and hopefully resolves these reported issues:

Fixes #6121
Fixes #5438
Fixes #5328

Description

When we initially rolled out #4770 the goal was to be able to fetch in the background without the user knowing (or needing to care about errors). That's why we originally went through GitStore to perform the fetch - but this avoided withPushPull which checks and prevents concurrent push/pull/fetch operations.

This seemed fine for the most part, but I believe this has left us open to a race condition with concurrent fetches, where the user starts a fetch at the same time as the repository indicator updater is fetching the same repository, or vice versa.

I stumbled across microsoft/vscode#47141 earlier today where VSCode basically outlined this behaviour between itself and an extension, and this comment comes to the same conclusion about what I believe is happening to these users:

This issue occurs when git-radar is doing a fetch in background and user runs git fetch/pull. Git has index.lock to lock index during a fetch, so you cannot corrupt the index by doing concurrent fetches. Your git command fails because of this lock prevents git modifying index.

michaeldfallen/git-radar#81 (comment)

It's hard to see from the reported issues which Git operation was the one that failed, but I believe it can be tripped with just a fetch (pull also performs a fetch to update it's refs, I'm not sure about push from memory).

What can we do here? I think the simplest answer is to push these status indicator updates through our existing infrastructure - these use repository state to control whether or not an operation should be performed.

The worst case scenarios I can think of about this change:

user tries to fetch/push/pull while the status indicator update is in progress - the button will now show details about the status indicator update (it did not before)
status indicator update tries to run while user is fetch/push/pull - status indicator update is skipped

Release notes

Notes: Prevent concurrent fetches corrupting Git index by adding checking to repository indicator updates

…s that need to perform fetch

app/src/lib/stores/app-store.ts

    this.updateMenuItemLabels(repository)
    this._initializeCompare(repository)
-    this.refreshIndicatorsForRepositories([repository])
+    this.refreshIndicatorsForRepositories([repository], false)


billygriffin · 2018-11-09T15:07:56Z

This seems like a step in the right direction here.

status indicator update tries to run while user is fetch/push/pull - status indicator update is skipped

This seems like expected behavior as a user. My hierarchy of things I care about is definitely whatever is happening in Push/Pull first, much more so than the background ops of repo indicators - it's fine for them to update during the next background operation.

user tries to fetch/push/pull while the status indicator update is in progress - the button will now show details about the status indicator update (it did not before)

I'm not positive I understand what you mean by this. What will the button show differently? Does it interfere with the fetch/push/pull op?

More generally, I think you're aligned with this, but since this has been an issue since 1.3, I think it's ok to release in 1.6 or 1.5.1 from a prioritization perspective.

shiftkey · 2018-11-09T15:14:40Z

@billygriffin yeah, this isn't something I planned to pull in for 1.5.0 - I just wanted to open this PR early so I can work through the impact (race conditions are great)!

shiftkey · 2018-11-09T15:19:42Z

I'm not positive I understand what you mean by this. What will the button show differently? Does it interfere with the fetch/push/pull op?

@billygriffin our normal fetch/push/pull flow will take the progress reported by Git and display it in the toolbar button. We do this for two reasons:

it's nice to show users that progress, especially when it takes a while to complete
if a fetch/push/pull is in progress, we disable the button so the user doesn't start a second operation

It's that second point that I believe is relevant here, because of how the app can arbitrarily run a fetch in the background (with no feedback to the user). If we don't check for an in-flight fetch/push/pull operation in the background, and the fetch can take a while due to slow network or complex repositories, the likelihood of concurrent fetches increases significant.

shiftkey · 2019-01-08T17:23:37Z

Tentatively scheduling this for 1.6.1 because I'd like it to sit on beta for some time to verify the underlying issues are resolved.

shiftkey · 2019-01-18T16:56:48Z

Marking this one as time-sensitive as I'd like to get a first round of review and see how we feel about getting this into 1.6.1 because of the priority-2 issues associated with it.

outofambit

This seems like a reasonable next step to me. I'm still wrapping my head around some of the related internal logic outside of the PR, but general 👍 from me

outofambit · 2019-01-18T17:26:52Z

anyone else from @desktop/engineering want to weigh in?

tierninho · 2019-01-22T19:26:12Z

@shiftkey Any specific that needs to be tested here? I am not sure how to simultaneously trigger two or more of the outlined actions.

shiftkey · 2019-01-22T20:37:36Z

@tierninho it's kind of tricky because the background fetch doesn't really have any UI, and you'll see this message after it's completed:

desktop/app/src/lib/stores/app-store.ts

Lines 1969 to 1973 in ba999a3

    
                 log.info( 
        
                   `Background fetch for ${ 
        
                     repositories.length 
        
                   } repositories took ${timeInSeconds}sec` 
        
                 )

This also forces the background fetcher through AppStore.shouldBackgroundFetch, which means it's more likely to hit the 30min minimum interval for background fetching and not fetch.

desktop/app/src/lib/stores/app-store.ts

Lines 1286 to 1293 in ba999a3

    
           if (timeSinceFetch < BackgroundFetchMinimumInterval) { 
        
             const timeInSeconds = Math.floor(timeSinceFetch / 1000) 
        
             log.debug( 
        
               `Skipping background fetch as '${repoName}' was fetched ${timeInSeconds}s ago` 
        
             ) 
        
             return false 
        
           }

Maybe just trying to fetch regularly in the app to see if you can force the background fetcher to trip at the same time as your manual fetch?

shiftkey · 2019-01-26T21:52:02Z

Merging this in so that it's available as part of the first beta

sanjaygupta1011 · 2020-03-03T14:15:57Z

You need to update the reference using following Git command on Git bash:
$ git update-ref -d refs/remotes/origin/[locked branch name]
then pull using $git pull

[locked branch name] is the name of the branch that the error is happening because of mismatch of commit Ids.

shiftkey added 5 commits November 9, 2018 09:17

use Git terminology in here

4377ce7

rename method to indicate this should handle fetch too

0fd97be

make background fetcher use the same infrastructure as main app

be6d5a3

to prevent recursively refreshing forever, distinguish between action…

7db2721

…s that need to perform fetch

naming things is hard

6a6b731

This was referenced Nov 9, 2018

unable to update local ref error on first Pull for every repo #5328

Closed

Error: Cannot lock ref 'refs/remotes/origin/master' #6121

Closed

shiftkey commented Nov 9, 2018

View reviewed changes

app/src/lib/stores/app-store.ts

this.updateMenuItemLabels(repository)

this._initializeCompare(repository)

this.refreshIndicatorsForRepositories([repository])

this.refreshIndicatorsForRepositories([repository], false)

This comment was marked as spam.

Sign in to view

shiftkey changed the title ~~[WIP] prevent concurrent fetches between user and status indicator checks~~ prevent concurrent fetches between user and status indicator checks Nov 9, 2018

shiftkey mentioned this pull request Nov 21, 2018

Option to disable auto-fetching (background fetching) in Desktop #1128

Closed

shiftkey changed the title ~~prevent concurrent fetches between user and status indicator checks~~ [WIP] prevent concurrent fetches between user and status indicator checks Nov 23, 2018

shiftkey added 2 commits December 17, 2018 11:45

Merge branch 'master' into use-the-flag-to-detect-network-issues

dfbb3f4

make parameter consistent for APIs

3e347a6

shiftkey changed the title ~~[WIP] prevent concurrent fetches between user and status indicator checks~~ prevent concurrent fetches between user and status indicator checks Dec 17, 2018

shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 17, 2018

shiftkey mentioned this pull request Dec 20, 2018

Inconsistent behavior for uncommitted changes indicator #6462

Closed

outofambit changed the base branch from master to development December 27, 2018 21:47

shiftkey added this to the 1.6.1 milestone Jan 8, 2019

shiftkey mentioned this pull request Jan 14, 2019

a better "push needs pull" dialog to avoid race condition #6610

Merged

3 tasks

shiftkey added the time-sensitive Pull Requests where reviews need to happen in a timely manner label Jan 18, 2019

outofambit self-requested a review January 18, 2019 17:03

outofambit reviewed Jan 18, 2019

View reviewed changes

iAmWillShepherd self-assigned this Jan 25, 2019

iAmWillShepherd approved these changes Jan 25, 2019

View reviewed changes

shiftkey removed the time-sensitive Pull Requests where reviews need to happen in a timely manner label Jan 26, 2019

shiftkey merged commit 309806c into development Jan 26, 2019

shiftkey deleted the use-the-flag-to-detect-network-issues branch January 26, 2019 21:52

shiftkey mentioned this pull request Feb 5, 2019

error: cannot lock ref 'refs/remotes/origin/ #5438

Closed

shiftkey mentioned this pull request Mar 6, 2019

only trigger conflicts flow for the selected repository #7018

Merged

iAmWillShepherd mentioned this pull request Mar 18, 2019

[rebase] enable --force-with-lease after rebasing #6981

Merged

5 tasks

sanjaygupta1011 unassigned iAmWillShepherd Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent concurrent fetches between user and status indicator checks#6141

prevent concurrent fetches between user and status indicator checks#6141
shiftkey merged 7 commits intodevelopmentfrom
use-the-flag-to-detect-network-issues

shiftkey commented Nov 9, 2018 •

edited

Loading

Uh oh!

This comment was marked as spam.

Uh oh!

billygriffin commented Nov 9, 2018 •

edited

Loading

Uh oh!

shiftkey commented Nov 9, 2018

Uh oh!

shiftkey commented Nov 9, 2018

Uh oh!

shiftkey commented Jan 8, 2019

Uh oh!

shiftkey commented Jan 18, 2019

Uh oh!

outofambit left a comment

Uh oh!

outofambit commented Jan 18, 2019

Uh oh!

tierninho commented Jan 22, 2019

Uh oh!

shiftkey commented Jan 22, 2019 •

edited

Loading

Uh oh!

shiftkey commented Jan 26, 2019

Uh oh!

sanjaygupta1011 commented Mar 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

shiftkey commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Description

Release notes

Uh oh!

This comment was marked as spam.

Uh oh!

billygriffin commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiftkey commented Nov 9, 2018

Uh oh!

shiftkey commented Nov 9, 2018

Uh oh!

shiftkey commented Jan 8, 2019

Uh oh!

shiftkey commented Jan 18, 2019

Uh oh!

outofambit left a comment

Choose a reason for hiding this comment

Uh oh!

outofambit commented Jan 18, 2019

Uh oh!

tierninho commented Jan 22, 2019

Uh oh!

shiftkey commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiftkey commented Jan 26, 2019

Uh oh!

sanjaygupta1011 commented Mar 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shiftkey commented Nov 9, 2018 •

edited

Loading

billygriffin commented Nov 9, 2018 •

edited

Loading

shiftkey commented Jan 22, 2019 •

edited

Loading