Skip to content

Fix the history list still claiming to update after it was cleaned up - #686

Merged
hannesa2 merged 2 commits into
masterfrom
settle-the-history-list-on-cleanup
Sep 21, 2026
Merged

hannesa2 merged 2 commits into
masterfrom
settle-the-history-list-on-cleanup

Conversation

@goneng

@goneng goneng commented Sep 19, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #669: PBGitWindowWiringTests timing 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.

PBGitWindowWiringTests has 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 three
different test methods. The count is a floor: a run that someone re-runs
to green no longer reports the failure at all.

-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. tearDown cleans up
and 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.

  • Report the list as settled once cleanup has stopped the walk and the
    graph queue, since that is exactly what the flag means
  • Say whether the walk was still parsing, and how many graph operations
    were left, when a wait gives up, so the next failure names the cause

This does not claim to explain the other half. -finishedGraphing clears
the flag only when the walk has stopped parsing and the queue has
drained, and PBGitHistoryGrapher.graphCommits: returns early without
calling 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:

  • The new test fails on master ("a list that was cleaned up still claims
    to be updating") and passes with the fix
  • PBGitHistoryListCleanupTests and PBGitWindowWiringTests run green
    together, 7 tests, on arm64

@goneng
goneng marked this pull request as ready for review September 19, 2026 11:15
@goneng goneng added the WIP Work in progress label Sep 19, 2026
@goneng
goneng marked this pull request as draft September 19, 2026 11:29
@goneng
goneng force-pushed the settle-the-history-list-on-cleanup branch from bf49489 to 765e2f9 Compare September 19, 2026 20:24
@goneng
goneng force-pushed the settle-the-history-list-on-cleanup branch from 765e2f9 to bb4bdff Compare September 21, 2026 05:26
@goneng
goneng marked this pull request as ready for review September 21, 2026 05:57
@goneng goneng added pipeline CI pipeline test Test related and removed WIP Work in progress test Test related labels Sep 21, 2026
-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
goneng force-pushed the settle-the-history-list-on-cleanup branch from bb4bdff to 5aaa612 Compare September 21, 2026 06:22
@hannesa2

Copy link
Copy Markdown
Contributor

Screenshot differs: emulatorApi=arm64 with 1 screenshot(s)

setpoint (old) | diff | actual screenshot (new)

staging-view.pngscreenshot

@goneng goneng added test Test related and removed pipeline CI pipeline labels Sep 21, 2026
@goneng
goneng requested a review from hannesa2 September 21, 2026 07:27
@hannesa2
hannesa2 merged commit d32c773 into master Sep 21, 2026
3 checks passed
@hannesa2
hannesa2 deleted the settle-the-history-list-on-cleanup branch September 21, 2026 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PBGitWindowWiringTests times out waiting for the revision list

2 participants