Possible bug in dart:io's File.copySync on Windows

[mkustermann]

After a new checked-in SDK got pulled into the sdk in cl/150931 we're seeing issues, e.g. log:

Test configurations:
    dartkp-win-release-x64(architecture: x64, compiler: dartkp, mode: release, runtime: dart_precompiled, system: win)
Suites tested: samples, standalone_2, corelib_2, language_2, vm, benchmark_smoke, utils, lib_2, analyze_library, service, kernel, ffi_2
    dartkp-win-release-x64(architecture: x64, compiler: dartkp, mode: release, runtime: dart_precompiled, system: win)
Suites tested: observatory_ui
Unhandled exception:
FileSystemException: Cannot copy file to 'C:/b/s/w/ir/out/ReleaseX64/generated_compilations/dartkp/runtime_tests_vm_dart_appjit_bytecode_simple_test/appjit_bytecode_simple_test_body.dart', path = 'C:\b\s\w\ir\runtime\tests\vm\dart\appjit_bytecode_simple_test_body.dart' (OS Error: The operation completed successfully.

, errno = 0)
#0      _File.throwIfError (dart:io/file_impl.dart:635:7)
#1      _File.copySync (dart:io/file_impl.dart:340:5)
#2      StandardTestSuite._makeCommands (package:test_runner/src/test_suite.dart:683:39)
#3      StandardTestSuite._enqueueStandardTest (package:test_runner/src/test_suite.dart:611:22)
#4      StandardTestSuite._testCasesFromTestFile (package:test_runner/src/test_suite.dart:590:7)
#5      StandardTestSuite.findTestCases (package:test_runner/src/test_suite.dart:485:7)
#6      TestCaseEnqueuer.enqueueTestSuites (package:test_runner/src/process_queue.dart:239:13)
#7      new ProcessQueue (package:test_runner/src/process_queue.dart:177:22)
#8      testConfigurations (package:test_runner/src/test_configurations.dart:255:3)
#9      main (file:///C:/b/s/w/ir/pkg/test_runner/bin/test_runner.dart:39:5)
#10     _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:299:32)
#11     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)
INFO: Enabling coredump archiving into C:\b\s\w\ir\crashes
INFO: Core dump archiving is activated
INFO: No unexpected crashes recorded

This might have been introduced in cl/149667.

For now I reverted the SDK upgrade.

/cc @zanderso @zichangg can you confirm it is coming from this change?

[zanderso]

Probably that CL, yeah. If the call to MoveFileW() fails, GetLastError() should be cached and used to SetLastError() before returning false to get the right error message to show up in the exception.

If @zichangg can repro the issue locally, we should probably revert cl/149667, and reland after fixing the problem

[athomas]

I've made this a P1 because it means we cannot upgrade the checked-in SDK which we'll need to do eventually for #42274.

[zichangg]

Sorry for the delayed reply.

I just ran the command locally with my Windows machine.

Test configurations:
    dartkp-win-release-x64(architecture: x64, compiler: dartkp, mode: release, runtime: dart_precompiled, system: win)
Suites tested: samples, standalone_2, corelib_2, language_2, vm, benchmark_smoke, utils, lib_2, analyze_library, service, kernel, ffi_2
    dartkp-win-release-x64(architecture: x64, compiler: dartkp, mode: release, runtime: dart_precompiled, system: win)
Suites tested: observatory_ui
Total: 10327 tests
 * 1941 tests will be skipped (1099 skipped by design)
 * 5 tests are expected to be flaky but not crash
 * 2 tests are expected to flaky crash
 * 8177 tests are expected to pass
 * 0 tests are expected to fail that we won't fix
 * 11 tests are expected to fail that we should fix
 * 9 tests are expected to crash that we should fix
 * 0 tests are allowed to timeout
 * 182 could not be categorized or are in multiple categories


=== 8098 tests passed, 288 failed ===

I'm not able to reproduce. It is probably due to different versions of Windows machine.
Let me revert the change first!

[zichangg]

@athomas Is there any way to ssh into Windows bots? I failed to repro the bug with my Windows machine.
This will be very helpful. I vaguely remember there was another bug which can be only reproduced with Windows bots.

[athomas]

Did you try this with the updated checked-in SDK? The change was reverted so if you just ran from master you would not be able to reproduce the issue in the first comment.

[zichangg]

Oh, I did use the head of the tree. What should I run? Top of the tree with my cl?

[athomas]

You would have to re-apply https://dart-review.googlesource.com/c/sdk/+/150931 locally (just changes a version in the DEPS, don't forget to run gclient sync) to reproduce the issue. I'm not sure if the reproduction requires your patch to be re-applied as well (the checked-in SDK version 2.9.0-15.0.dev above has your patch built into it, and it seems this issue has more to do with that than the code being tested).

[whesse]

Did you see my comment about forward slashes being passed all the way down to the Windows api calls, which treat them as path separators? If this is the case, they need to be handled as path separators in the new copy code.

Also, the test scripts (test.py) in pkg/test_runner create generated file names that come close to the 255 byte limit, with long file names. We even have a hack in there to shorten some names to keep them under the limit.

If a path ends up too long in this change, could you allow reverting to the previous copy implementation? Alternatively, you can use Windows long paths, but they disable some normalization (like .\ and .., and forward slashes).

[zichangg]

Did you see my comment about forward slashes being passed all the way down to the Windows api calls, which treat them as path separators? If this is the case, they need to be handled as path separators in the new copy code.

That is probably the reason. I'm looking for backslashes in the destination file path.

@athomas I have created a reland cl which falls back to original schema if copying failed. The problem is how can I test it?
It seems like applying local patch is not enough to reproduce the problem. I was only able to repro with DEPS file change.