LSP recheck can panic after an export change propagates through a deep dependency chain

[teerthsharma]

LSP recheck can panic after an export change propagates through a deep dependency chain

Pyrefly commit: c195e9fbb5c47394987dfd9d1221b1ff27a205ec (1.2.0-dev.2), release build, Windows, four worker threads.

No matching open issue or PR was found for the panic text, epoch warning, or incremental dependency propagation failure.

Observed behavior

A saved type change propagated through a generated chain of modules. With 192 two-module SCC layers, the update completes. At 200 layers, the first updated diagnostic arrives, but the following save panics. At 208 or more layers, the first save panics.

The terminal warning and panic are:

Exceeded maximum epochs (100) without stabilizing. ... Forcing invalidation.
Transaction has uncommitted changes

The panic is at State::commit_transaction in pyrefly/lib/state/state.rs.

Runtime trace

With 32 layers, verbose logging shows 35 epochs and one changed export per epoch:

leaf -> a_0 -> a_1 -> ... -> a_31 -> main -> stabilization

With 192 layers, the first run reaches epoch 100 after a_98, invokes the fallback, and a second run needs 94 epochs to reach main and stabilize.

The coarse fallback has already transitively marked a_99 through main dirty, but its single run_step changes only a_99. The remaining closure was processed before its dependencies became current and cannot be re-dirtied in that same epoch.

The same 100 + 94 pattern occurs with one worker, ruling out parallel races as a necessary cause and making a sequential dependency-first fallback a viable minimum-code experiment.

Source-level cause

When demand observes changed exports, affected direct rdeps are added to TransactionData.dirty. run_step transfers that set into TaskHeap only at the beginning of the next epoch, so a long path advances roughly one edge per epoch.

After MAX_EPOCHS, run_internal calls invalidate_rdeps and performs one final run_step. That final step can record more changed exports, but the fallback returns without draining them. The later commit assertion then rejects the transaction.

The cap and fallback were introduced by c6ca82ee2 as defense-in-depth for fine-grained cycle detection. Existing tests cover overlapping cycles, independent export changes, and short re-export chains. They do not cover a finite acyclic chain that exceeds the cap.

Expected behavior

Deep but finite acyclic propagation should converge or fail with a controlled error; it should not leave a committable transaction in an internally inconsistent state.

Design question before a PR

Would maintainers prefer this scoped as:

1. a regression test plus the smallest correctness fix that guarantees the fallback drains changed before commit;
2. repeating the current unordered coarse fallback until stable; or
3. traversing the already-invalidated closure sequentially by dependency-first SCC condensation, benchmarked against option 2 and an importer-first same-cost null?

I have a deterministic long-lived LSP harness and successful/failing traces, but have intentionally not prepared a source patch pending agreement on scope.

Sandbox / IDE information

This reproduces through a direct stdio LSP client rather than the VS Code extension. A Pyrefly Sandbox link cannot model the required sequence of opening two files, changing the leaf in memory, writing it, and sending didSave across hundreds of generated modules. The reproducer is deterministic and can be translated into an LspInteraction regression test once scope is agreed.

[kinto0]

thanks for the detailed writeup! the best thing would be a PR that can reproduce this issue on its own. if we can [expect(panic)] in it, even better and we can merge that. then a fix as a secondary PR. that way, we know without changing the PR whether the PR fixed it.

your diagnosis is very low-level. what type of code can reproduce this that is actually used? I wonder if there is another bug at play here that causes us to exceed the maximum epochs.

[teerthsharma]

Thanks — I’ll keep the reproducer and fix as separate PRs.

I tried to ground the low-level diagnosis in real package shapes before writing the test:

• Pyrefly’s Django fixture has condensation height 12 and a largest SCC of 97 modules.
• A full PyTorch 2.5.1 check graph has a very large SCC, but a production-style public Tensor export edit demanded only 66 dependent modules and converged normally in the watcher. So I do not currently have an actually-used package edit that exceeds 100 epochs, and I don’t want to overstate prevalence.
• A finite synthetic Python graph does reproduce the mechanism deterministically: a 97-module cycle carrying one exported value followed by a 32-module importer tail reaches epoch 100, invokes coarse invalidation, then needs 31 more epochs. A 366-wide shallow fanout settles in four epochs, so depth/order rather than width is the trigger. The longer 208-level finite chain reaches the existing uncommitted-transaction panic.

Would the clearly-labeled synthetic finite case still be useful as the test-only [expect(panic)] PR you suggested, or would you prefer that I wait until I can reduce an actually-used package pattern? If useful, I’ll submit only the regression first—no scheduler change—and leave the SCC-ordering experiment for a separately approved fix scope.

[teerthsharma]

#claim

[github-actions]

@teerthsharma you've claimed this issue — it's now assigned to you. Thanks for picking it up! 🎉

[teerthsharma]

Test-only expected-panic reproducer PR: #4180

[kinto0]

sorry about not responding before the PR. it looks great, let me get it merged. feel free to start working on the fix while it's still merging off of that branch if you want.

[kinto0]

since this one is pretty complicated feel free to shoot us a message on discord if you want to discuss it