I think there's just one last thing: the os import needs to be removed.

# github.com/prometheus/prometheus/promql [github.com/prometheus/prometheus/promql.test]
promql/engine_test.go:19:2: imported and not used: "os"

I think there's just one last thing: the os import needs to be removed.

# github.com/prometheus/prometheus/promql [github.com/prometheus/prometheus/promql.test]
promql/engine_test.go:19:2: imported and not used: "os"

fix

OK, so I think I've figured out why test_windows keeps failing with the following error: testing.go:967: TempDir RemoveAll cleanup: remove C:\Users\circleci\AppData\Local\Temp\TestQueryConcurrency3635900781\001\queries.active: The process cannot access the file because it is being used by another process. This error happens because the query logger is still using the active query file when the temporary directory is removed. That file should be closed before the temporary directory is removed, and we do stop the query engine (which should close the file), just using a defer statement. That worked before this change when the temporary directory was also removed using a defer statement. But now, t.TempDir removes the directory using a cleanup function, which is called before the deferred functions are. Therefore, the directory is attempted to be removed before the query engine is closed, causing the error.

In order to fix this, these two deferred functions should be registered as cleanup functions instead:

https://github.com/ianwoolf/prometheus/blob/4ae36a0575b4a4bc243862231a0cf4e613d7970c/promql/engine_test.go#L53

https://github.com/ianwoolf/prometheus/blob/4ae36a0575b4a4bc243862231a0cf4e613d7970c/promql/engine_test.go#L58
(I don't think this one's actually necessary but we might as well)

That should fix it.

OK, so I think I've figured out why test_windows keeps failing with the following error: testing.go:967: TempDir RemoveAll cleanup: remove C:\Users\circleci\AppData\Local\Temp\TestQueryConcurrency3635900781\001\queries.active: The process cannot access the file because it is being used by another process. This error happens because the query logger is still using the active query file when the temporary directory is removed. That file should be closed before the temporary directory is removed, and we do stop the query engine (which should close the file), just using a defer statement. That worked before this change when the temporary directory was also removed using a defer statement. But now, t.TempDir removes the directory using a cleanup function, which is called before the deferred functions are. Therefore, the directory is attempted to be removed before the query engine is closed, causing the error.

In order to fix this, these two deferred functions should be registered as cleanup functions instead:

https://github.com/ianwoolf/prometheus/blob/4ae36a0575b4a4bc243862231a0cf4e613d7970c/promql/engine_test.go#L53

https://github.com/ianwoolf/prometheus/blob/4ae36a0575b4a4bc243862231a0cf4e613d7970c/promql/engine_test.go#L58 (I don't think this one's actually necessary but we might as well)

That should fix it.

only registering the refer astesting.ClearUp doesn't work. Looks like has to wait for the query logger to be closed.

I add Close for ActiveQueryTracker to close the os.File and call it in test. now i get an error as follow

=== RUN   TestQueryConcurrency
    testing.go:967: TempDir RemoveAll cleanup: remove C:\Users\circleci\AppData\Local\Temp\TestQueryConcurrency191334965\001\queries.active: Access is denied.
--- FAIL: TestQueryConcurrency (0.04s)

and i pass the test on mac and linux, it still fails on windows.

The error log is not is being used by another process any more, so closing the query log is worked. But i don't understant why Access is denied.
I open an issue and pr to discuss it, turn this issue to WIP

cc @ianwoolf can you please rebase this pull request? Thanks!

cc @ianwoolf can you please rebase this pull request? Thanks!

done :)

Discussed in the bug scrub; apologies we dropped the ball in July. This still looks worthwhile, however at the last CI run Windows tests failed and something needs a DCO signed-off line.

@ianwoolf could you add the signed-off, rebase again and we can try to figure out the Windows test?

I think this PR can now be safely closed, since after rebasing on main and fixing conflicts, there are no changes left.

Thanks!