Schedule the next ti request after finishing the handling of request and updating program typing files

[sheetalkamat]

This also fixes the flaky test (i think)

[Copilot]

Pull Request Overview

This PR updates the scheduling logic for the typings installer so that the next request is scheduled after completing both the handling of the current request and the program typing file updates.

• Moved the pending run request scheduling block from an earlier section to after the file update and logging.
• Ensures that state updates complete before processing the next request.

[Copilot]

Consider using a defer statement immediately after acquiring the lock (e.g. 'ti.pendingRunRequestsMu.Lock(); defer ti.pendingRunRequestsMu.Unlock()') to improve code safety and clarity.

Suggested change

	ti.pendingRunRequestsMu.Lock()
	ti.pendingRunRequestsMu.Lock()
	defer ti.pendingRunRequestsMu.Unlock()

[jakebailey]

Before:

gotip test -count=100 -run='TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit' ./internal/project
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc0010d0000 Success} (status project.TypingsInstallerStatus) != {1 0xc001bb4000 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc001bb4a80 Success} (status project.TypingsInstallerStatus) != {1 0xc002a28300 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.04s)
        ata_test.go:267: assertion failed: {2 0xc0010d1380 Success} (status project.TypingsInstallerStatus) != {1 0xc002a28900 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc001bb5680 Success} (status project.TypingsInstallerStatus) != {1 0xc001bb4c00 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc0010d0d80 Success} (status project.TypingsInstallerStatus) != {1 0xc006998180 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc001bb5800 Success} (status project.TypingsInstallerStatus) != {1 0xc0010d1980 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc006998000 Success} (status project.TypingsInstallerStatus) != {1 0xc00a6b5080 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc002a29380 Success} (status project.TypingsInstallerStatus) != {1 0xc002a28000 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc01195c780 Success} (status project.TypingsInstallerStatus) != {1 0xc00961d380 Success} (project.TypingsInstallerStatus)
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {2 0xc00961cd80 Success} (status project.TypingsInstallerStatus) != {1 0xc00a6b4a80 Success} (project.TypingsInstallerStatus)
FAIL
FAIL    github.com/microsoft/typescript-go/internal/project     3.012s
FAIL

After:

gotip test -count=100 -run='TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit' ./internal/project
--- FAIL: TestAta (0.00s)
    --- FAIL: TestAta/Throttle_-_scheduled_run_install_requests_reaching_limit (0.03s)
        ata_test.go:267: assertion failed: {1 0xc0000ea300 Success} (status project.TypingsInstallerStatus) != {1 0xc0000ea480 Success} (project.TypingsInstallerStatus)
FAIL
FAIL    github.com/microsoft/typescript-go/internal/project     2.995s
FAIL

So, the race is far less prevalent, but still not impossible.

[jakebailey]

Approving just since this is a net improvement and I want CI to be cleaner, but I think that there are still some concurrency bugs in the ATA code that might be fixed by a little restructuring / a different kind of request queue.

[sheetalkamat]

I dont think there is concurrency issue with project as such as it doesnt access anythin on project thats not handled by mutex.. I am also surprised on the failure you are getting as its specifically testing that installation doesnt start before finishing the first one when the throttle limit is reached. i have run that test lot of times, just it, sometimes all project and sometimes all tests and never ran into it. i will try on a linux vm to see if i can repro it and see whats going on.

[jakebailey]

It's definitely a concurrency issue. It's not a data race, but it is a logical race, otherwise the test wouldn't fail. The failure is that there's a different project that succeeds first, which is an ordering problem.

All of the use of atomics and a manual work queue is what I think is causing this to happen; I think that if this were instead converted to not have ATA call back into anything in the Project, instead make Projects call a function on ATA that returns info (so, keep the concurrency out of the contract, manage concurrency within ATA for deduping/throttling, and within Project for safely setting things). That sort of model is a lot easier to reason about than passing a Project into ATA, which then does work, and then calls back onto Project with info.