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.

Sign up for GitHub

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

Jump to bottom

Race Condition in Branch Focus Rendering #4267

Open
ChrisMcD1 opened this issue Feb 15, 2025 · 4 comments · May be fixed by #4268
Open

Race Condition in Branch Focus Rendering #4267

ChrisMcD1 opened this issue Feb 15, 2025 · 4 comments · May be fixed by #4268
Labels
bug Something isn't working

Comments

@ChrisMcD1
Copy link
Contributor

Describe the bug
There is now a race condition between the selection of the focused branch, and the rendering of the view, due to the changes to refresh_helper in ff4ae4a. One way this manifests itself as a flaky integration test custom_commands/suggestions_preset

To Reproduce
Steps to reproduce the behavior:

I discovered the problem when my pre-PR checks failed on #4261

It is tough to reproduce locally, but I am reliably able to do it with this script:

#!/bin/bash
set -e
    
for (( i=0; i<100; i++ ))
do
    go1.22.0 run cmd/integration_test/main.go cli custom_commands/suggestions_preset
done

Go 1.22 is used because I ran a git bisect run on this script to find that ff4ae4a introduced the problem, which was prior to the 1.23 fix. It is unrelated to the issue at hand

  1. Run the above integration test 100 times
  2. Observe the below error on one of the runs
Unexpected selection in view 'localBranches'. Expected line 1 to be selected but got line 0.
Expected selected lines:
---
contains 'branch-four'
---

Actual selected lines:
---
  * branch-three
---

Final Lazygit state:
╭─[1]─Status───────────────────────────────────────────────────╮╭─Log────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│repo → branch-three                                           ││* commit 65d822a (HEAD -> branch-three)                                                                                     ▐
╰──────────────────────────────────────────────────────────────╯│| Author: CI <[email protected]>                                                                                               ▐
╭─[2]─Files - Worktrees - Submodules───────────────────────────╮│| Date:   0 seconds ago                                                                                                     ▐
│                                                              ││|                                                                                                                           ▐
│                                                              ││|     blah                                                                                                                  ▐
│                                                              ││|                                                                                                                           ▐
│                                                              ││* commit 55432ed (branch-two)                                                                                               ▐
│                                                              ││| Author: CI <[email protected]>                                                                                               ▐
│                                                              ││| Date:   0 seconds ago                                                                                                     ▐
│                                                              ││|                                                                                                                           ▐
│                                                              ││|     blah                                                                                                                  ▐
│                                                              ││|                                                                                                                           ▐
│                                                              ││* commit d6e4e6b (branch-one)                                                                                               ▐
│                                                              ││  Author: CI <[email protected]>                                                                                               ▐
╰───────────────────────────────────────────────────────0 of 0─╯│  Date:   0 seconds ago                                                                                                     ▐
╭─[3]─Local branches - Remotes - Tags──────────────────────────╮│                                                                                                                            ▐
│  * branch-three                                              ││      blah                                                                                                                  ▐
│0s  branch-four                                               ││                                                                                                                            ▐
│0s  branch-two                                                ││                                                                                                                            ▐
│0s  branch-one                                                ││                                                                                                                            ▐
│                                                              ││                                                                                                                            ▐
│                                                              ││                                                                                                                            │
│                                                              ││                                                                                                                            │
│                                                              ││                                                                                                                            │
│                                                              ││                                                                                                                            │
│                                                              ││                                                                                                                            │
╰───────────────────────────────────────────────────────1 of 4─╯│                                                                                                                            │
╭─[4]─Commits - Reflog─────────────────────────────────────────╮│                                                                                                                            │
│65d822a5 CI ◯ blah                                            ││                                                                                                                            │
│55432ed7 CI ◯ * blah                                          ││                                                                                                                            │
│d6e4e6bd CI ◯ * blah                                          ││                                                                                                                            │
│                                                              │╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
│                                                              │╭─Command log────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                              ││You can hide/focus this panel by pressing '@'                                                                               │
│                                                              ││                                                                                                                            │
│                                                              ││                                                                                                                            │
│                                                              ││Custom command                                                                                                              │
│                                                              ││  bash -c "git checkout branch-three"                                                                                       │
╰───────────────────────────────────────────────────────1 of 3─╯│                                                                                                                            │
╭─[5]─Stash────────────────────────────────────────────────────╮│                                                                                                                            │
│                                                              ││                                                                                                                            │
╰───────────────────────────────────────────────────────0 of 0─╯╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
Checkout: <space> | New branch: n | Delete: d | Rebase: r | Merge: M | Reset: g | Upstream: u | Keybindings: ? | Cancel: <esc>                                 Donate Ask Question unversioned

Upon failure, focused view was 'localBranches'.
Log:
Custom command
bash -c "git checkout branch-three"
panic: Test failed

goroutine 16 [running]:
github.com/jesseduffield/lazygit/pkg/gui.(*GuiDriver).Fail(0xc000591e00, {0xc000408000, 0xc6})
        /home/chrismcdonnell/linux-repos/lazygit/pkg/gui/gui_driver.go:119 +0x2f0
github.com/jesseduffield/lazygit/pkg/integration/components.(*assertionHelper).fail(...)
        /home/chrismcdonnell/linux-repos/lazygit/pkg/integration/components/assertion_helper.go:28
github.com/jesseduffield/lazygit/pkg/integration/components.(*assertionHelper).assertWithRetries(0xc0000471a0, 0x20?)
        /home/chrismcdonnell/linux-repos/lazygit/pkg/integration/components/assertion_helper.go:23 +0x32
github.com/jesseduffield/lazygit/pkg/integration/components.(*ViewDriver).assertLines(0xc0008a01a0, 0x0, {0xc00065a2f8, 0x4, 0xe6a260?})
        /home/chrismcdonnell/linux-repos/lazygit/pkg/integration/components/view_driver.go:270 +0x465
github.com/jesseduffield/lazygit/pkg/integration/components.(*ViewDriver).Lines(0xc0008a01a0, {0xc00065a2f8, 0x4, 0x4})
        /home/chrismcdonnell/linux-repos/lazygit/pkg/integration/components/view_driver.go:68 +0xb8
github.com/jesseduffield/lazygit/pkg/integration/tests/custom_commands.init.func45(_, {{{0x18d7468, 0x1}, {0xc00020f338, 0x5}, {0xc00020f360, 0x5}, {0x18d7368, 0x1}, {0xc00020f378, ...}, ...}, ...})
        /home/chrismcdonnell/linux-repos/lazygit/pkg/integration/tests/custom_commands/suggestions_preset.go:59 +0x608
github.com/jesseduffield/lazygit/pkg/integration/components.(*IntegrationTest).Run(0xc00033b550, {0x11a8b78, 0xc000591e00})
        /home/chrismcdonnell/linux-repos/lazygit/pkg/integration/components/test.go:201 +0x371
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).handleTestMode.func2()
        /home/chrismcdonnell/linux-repos/lazygit/pkg/gui/test_mode.go:41 +0x170
created by github.com/jesseduffield/lazygit/pkg/gui.(*Gui).handleTestMode in goroutine 1
        /home/chrismcdonnell/linux-repos/lazygit/pkg/gui/test_mode.go:34 +0x1de
2025/02/15 00:40:48 exit status 2
exit status 1

Expected behavior
I expect the integration test to pass.

Version info:
Building on master on 01eece3737f9
git version 2.25.1

Additional context
I added some local println debugging in, and here is what it showed on a failed run

Custom command
bash -c "git checkout branch-three"
SetFocusPoint
Finished AfterLayout
Our prev selected branch is &{branch-four    * ? ? ? ? false true false   blah abe927404c0894db9b8bf0f8894888515c081414 {{} 0}}
One branch is &{branch-four    * ? ? ? ? false true false   blah abe927404c0894db9b8bf0f8894888515c081414 {{} 0}}
One branch is &{branch-three  1s ? ? ? ? false false false   blah 6bd8e160c496dc847faaca4004b6792db1196109 {{} 0}}
One branch is &{branch-two  1s ? ? ? ? false false false   blah a2db829dee58bd8101da752ad52ad44fdb56a638 {{} 0}}
One branch is &{branch-one  1s ? ? ? ? false false false   blah 8f847dfe28ecf7c0d595c86653cd8a384afd6c5f {{} 0}}
Setting new branches
New branch is &{branch-three    * ? ? ? ? false true false   blah 6bd8e160c496dc847faaca4004b6792db1196109 {{} 0}}
New branch is &{branch-four  1s ? ? ? ? false false false   blah abe927404c0894db9b8bf0f8894888515c081414 {{} 0}}
New branch is &{branch-two  1s ? ? ? ? false false false   blah a2db829dee58bd8101da752ad52ad44fdb56a638 {{} 0}}
New branch is &{branch-one  1s ? ? ? ? false false false   blah 8f847dfe28ecf7c0d595c86653cd8a384afd6c5f {{} 0}}
Registering FocusLine
SetFocusPoint
Finished AfterLayout
Found branch with name at 1

On a successful run done with -sandbox on the integration test, we instead see:

<identical to above>
Registering FocusLine
Found branch with name at 1
SetFocusPoint
Finished AfterLayout

with the only difference being that the SetSelectedIndex was called before the SetFocusPoint in the render of the list.

https://github.com/ChrisMcD1/lazygit/blob/7d5d3c5ce5859d45af7bc731ed786b972a677aa1/pkg/gui/controllers/helpers/refresh_helper.go#L504-L513

Branch with the printlns can be found at https://github.com/ChrisMcD1/lazygit/tree/reproducing-broken. It's unclear to me the conditions that #4195 solves, which is why I am making an issue instead of a fix! It seems logical to me that we need to render the branch view after setting the selected index, so I must be missing something.

I don't think I can run --debug in an integration test to give more logs. If I can, please let me know how!
@ChrisMcD1 ChrisMcD1 added the bug Something isn't working label Feb 15, 2025
@stefanhaller stefanhaller linked a pull request Feb 15, 2025 that will close this issue
Open
@stefanhaller
Copy link
Collaborator

@ChrisMcD1 Thanks for the detailed investigation, this was very helpful in fixing the issue. Here's a PR: #4268

@stefanhaller
Copy link
Collaborator

It's unclear to me the conditions that #4195 solves

I'm a bit surprised by this; it's true that #4195 itself doesn't say much about what it fixes, but the issue it references (#4179) is quite clear about that (if you scroll down and read all the comments); also, ff4ae4a contains an integration test that describes very concise reproduction steps.

I'm just trying to learn what I should have done better to make things more obvious. I'm trying quite hard to document all my changes in ways that make it easy to understand them later.

@ChrisMcD1
Copy link
Contributor Author

the issue it references (#4179) is quite clear about that (if you scroll down and read all the comments)

I agree! My comment was actually actually less that "I don't understand what the crash was", and rather "I don't understand how #4195 doesn't fully break the focusing of branches". It's a lack of understanding around the lifecycle of when AfterLayout functions trigger.

@stefanhaller
Copy link
Collaborator

and rather "I don't understand how #4195 doesn't fully break the focusing of branches".

Ah I see. Never mind then! And to be very honest, I don't fully understand that either...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix race condition with reselecting the focused branch and rendering jesseduffield/lazygit
2 participants
@stefanhaller @ChrisMcD1