zed-industries
Fix race condition between auto-indent and on-type-formatting
This PR addresses to fix (#31308) a race condition where auto-indent (in buffer.cs) and on-type-formatting (in lsp_store.rs) concurrently calculate indentation using the same buffer snapshot.
Previous Solution (Abandoned): https://github.com/zed-industries/zed/pull/31340
Final Solution: Delay applying on-type-formatting until auto-indent is complete.
Issue:
If AutoindentMode finishes first, formatting works correctly. If "Formatting on typing" starts before AutoindentMode completes, it results in double indentation.
Closes #31308
Release Notes:
- N/A or Added/Fixed/Improved ...
It seems there are some infrastructure issues with the CI. Could you please retry? 👇
Would you mind adding some tests for this behavior? Especially given it's designed to fix a race conditions, It would be ideal if you added a test with a #[gpui::test(iterations = 10)] or even more than 10 if the test is fast just to make sure the race condition is truly fixed, and to make sure that we don't regress in the future :)
@probably-neb , I added a new test, but it turned out to be more challenging than finding and fixing the original issue :)
When I wrote the test, I wanted to verify that it would fail if I reverted my code changes. However, when I rolled back my changes, the test passed successfully even after 1000 iterations.
After diving deeper into the problem, I realized that the task sequence for my test was always the same. Eventually, I discovered that the sequence of foreground tasks wasn't being shuffled. After fixing the implementation in dispatcher.rs, the test started failing on the 31st iteration without my changes. That's why I ultimately set it to 40 iterations—to make it more likely to catch regressions in the future.
Ugh sorry, I should've warned you about that. In #[gpui::test] tests we run the scheduler deterministically, that's part of the reason we needed the iterations param, was to run with different seeds. I'm ok merging as is now, but what we can also do is find some specific seeds (like run 31 for you) that fail, and pass those in explicitly like #[gpui::test(iterations = 10,seeds({run_31_seed}, {another_failing_seed}] so that we make sure we hit the failing seeds every time we run the test, and also run a decent number of different versions of the previously non-failing version to catch regressions
@probably-neb, Could you please help me investigate the CI build failures? Initially, I thought the large number of failing tests was due to changes in dispatcher.rs. However, after reverting all my local changes, I noticed that some tests (e.g., editor_tests::test_range_format_during_save with seed = 7) still fail.
If you could share any advice on how to fix the CI, I’d be happy to address it right away. Once resolved, I can also add the seeds() tag with the problematic seed values for my new test
It seems that my changes in dispatchers.rs are indeed breaking a lot of tests, so I've reverted them.
What do you think—should we create a separate issue to investigate the dispatcher problem? For my current bugfix, this issue is non-trivial and would take considerable time to resolve.
I think I missed your changes to dispatcher.rs before, what was your goal there? I'm a bit lost with where you're at right now, all the tests seem to be passing including your new test. I'm not fully understanding how the dispatcher changes and the editor_tests::test_range_format_during_save failure are playing into everything right now. Are you saying that unrelated to your changes, it seems that if you run the editor_tests::test_range_format_during_save test with seed 7 it fails?
Sorry if I confused you a bit. Let me clarify my thoughts:
- I made changes that fix the issue and wrote a test that passes.
- Then I decided to verify that the test should fail if the fix is reverted. I expected the test to fail, but it remained green (even with iterations=1000).
- I started investigating why the issue is easily reproducible in the real application but not in the test environment. That's why I began modifying dispatcher.rs to make the race condition reproducible in my test. Eventually, after the changes to dispatcher.rs, I managed to reproduce the issue in the test environment with seed=31. However, these changes broke a large number of other tests.
- Regarding the test_range_format_during_save test: while investigating the CI failure, I used this test as an example. I noticed that it fails even without my changes when run with the iterations=10 tag (failing on seed 7-8).
To summarize: The original issue is fixed, the test is written, and I propose merging the fix.
Along the way, I discovered some systemic issues with the test environment that can't be resolved right now but are worth considering in the future:
- Apparently, the current test context implementation has low entropy, which prevents some race conditions (like the one in my test) from being detected even with a large number of iterations—even though the race is easily reproducible manually in the real application.
- Some tests (e.g., test_range_format_during_save) may fail with certain seeds. Perhaps such cases could be caught by occasionally running tests proactively with a high number of iterations.
Hey, yes sorry. I took some time off last week and lost this in GitHub notifications. Looks good to merge. Thanks for your work and patience!