Repository navigation
Fix the history list still claiming to update after it was cleaned up - #686
Merged
Merged
Conversation
goneng
marked this pull request as ready for review
September 19, 2026 11:15
goneng
marked this pull request as draft
September 19, 2026 11:29
goneng
force-pushed
the
settle-the-history-list-on-cleanup
branch
from
September 19, 2026 20:24
bf49489 to
765e2f9
Compare
goneng
force-pushed
the
settle-the-history-list-on-cleanup
branch
from
September 21, 2026 05:26
765e2f9 to
bb4bdff
Compare
goneng
marked this pull request as ready for review
September 21, 2026 05:57
-cleanup cancels the walk and the graph queue, but cancelled operations never run the completion blocks that call -finishedGraphing, so nothing clears isUpdating and the flag stays set for good. Whoever waits on it afterwards waits for a callback that is never coming, which is what the wiring tests do in tearDown before deleting their fixture. - Report the list as settled once cleanup has stopped the walk and the graph queue, since that is exactly what the flag means - Cover it with a test that cleans up a list and then asks it
-finishedGraphing clears isUpdating only once the walk has stopped parsing and the graph queue has drained, and the message named neither, so each intermittent failure in CI said no more than the last one did. - Report whether the walk was still parsing, and how many graph operations were left, when the wait gave up
goneng
force-pushed
the
settle-the-history-list-on-cleanup
branch
from
September 21, 2026 06:22
bb4bdff to
5aaa612
Compare
Contributor
hannesa2
approved these changes
Sep 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Fixes #669:
PBGitWindowWiringTeststiming out waiting for the revision list to settle.This was originally filed as half of #643, which has since been closed - its other half turned out to be #703, a different failure in the same test class, fixed by #702.
PBGitWindowWiringTestshas timed out four times in the last 87 CI runs(17 Sep master, 18 Sep #668, 19 Sep #683 and 19 Sep master), always on
arm64, always on
((revisionList.isUpdating) is false), across threedifferent test methods. The count is a floor: a run that someone re-runs
to green no longer reports the failure at all.
-cleanupcancels the walk and the graph queue, but cancelled operationsnever run the completion blocks that call
-finishedGraphing, so nothingclears
isUpdatingand the flag stays set for good.tearDowncleans upand then waits on that flag, so that wait cannot succeed at any timeout:
in the master failure the two assertions are exactly 30 seconds apart,
one per wait. Locally each of these tests settles in under a second, so
30 seconds of genuine walking was never the explanation.
graph queue, since that is exactly what the flag means
were left, when a wait gives up, so the next failure names the cause
This does not claim to explain the other half.
-finishedGraphingclearsthe flag only when the walk has stopped parsing and the queue has
drained, and
PBGitHistoryGrapher.graphCommits:returns early withoutcalling it at all when its commit list is empty or its thread was
cancelled - both are ways for the light to be left on. The new message is
there to say which one, rather than guessing now.
Test plan:
to be updating") and passes with the fix
PBGitHistoryListCleanupTestsandPBGitWindowWiringTestsrun greentogether, 7 tests, on arm64