Merge pull request #1500 from ahoppen/fix-index-cancellation

Send `SIGKILL` to `swift-frontend` indexing processes

Author: ahoppen

Sources/SKSupport/Process+Run.swift

@@ -26,12 +26,30 @@ import WinSDK
 
 extension Process {
   /// Wait for the process to exit. If the task gets cancelled, during this time, send a `SIGINT` to the process.
+  /// Should the process not terminate on SIGINT after 2 seconds, it is killed using `SIGKILL`.
   @discardableResult
-  public func waitUntilExitSendingSigIntOnTaskCancellation() async throws -> ProcessResult {
+  public func waitUntilExitStoppingProcessOnTaskCancellation() async throws -> ProcessResult {
+    let hasExited = AtomicBool(initialValue: false)
     return try await withTaskCancellationHandler {
-      try await waitUntilExit()
+      defer {
+        hasExited.value = true
+      }
+      return try await waitUntilExit()
     } onCancel: {
       signal(SIGINT)
+      Task {
+        // Give the process 2 seconds to react to a SIGINT. If that doesn't work, kill the process.
+        try await Task.sleep(for: .seconds(2))
+        if !hasExited.value {
+          #if os(Windows)
+          // Windows does not define SIGKILL. Process.signal sends a `terminate` to the underlying Foundation process
+          // for any signal that is not SIGINT. Use `SIGABRT` to terminate the process.
+          signal(SIGABRT)
+          #else
+          signal(SIGKILL)
+          #endif
+        }
+      }
     }
   }
 
@@ -138,7 +156,7 @@ extension Process {
     )
     return try await withTaskPriorityChangedHandler(initialPriority: Task.currentPriority) { @Sendable in
       setProcessPriority(pid: process.processID, newPriority: Task.currentPriority)
-      return try await process.waitUntilExitSendingSigIntOnTaskCancellation()
+      return try await process.waitUntilExitStoppingProcessOnTaskCancellation()
     } taskPriorityChanged: {
       setProcessPriority(pid: process.processID, newPriority: Task.currentPriority)
     }

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -133,6 +133,11 @@ public final actor SemanticIndexManager {
   /// The build system manager that is used to get compiler arguments for a file.
   private let buildSystemManager: BuildSystemManager
 
+  /// How long to wait until we cancel an update indexstore task. This timeout should be long enough that all
+  /// `swift-frontend` tasks finish within it. It prevents us from blocking the index if the type checker gets stuck on
+  /// an expression for a long time.
+  public var updateIndexStoreTimeout: Duration
+
   private let testHooks: IndexTestHooks
 
   /// The task to generate the build graph (resolving package dependencies, generating the build description,
@@ -209,6 +214,7 @@ public final actor SemanticIndexManager {
   public init(
     index: UncheckedIndex,
     buildSystemManager: BuildSystemManager,
+    updateIndexStoreTimeout: Duration,
     testHooks: IndexTestHooks,
     indexTaskScheduler: TaskScheduler<AnyIndexTaskDescription>,
     logMessageToIndexLog: @escaping @Sendable (_ taskID: IndexTaskID, _ message: String) -> Void,
@@ -217,6 +223,7 @@ public final actor SemanticIndexManager {
   ) {
     self.index = index
     self.buildSystemManager = buildSystemManager
+    self.updateIndexStoreTimeout = updateIndexStoreTimeout
     self.testHooks = testHooks
     self.indexTaskScheduler = indexTaskScheduler
     self.logMessageToIndexLog = logMessageToIndexLog
@@ -522,6 +529,7 @@ public final actor SemanticIndexManager {
         indexStoreUpToDateTracker: indexStoreUpToDateTracker,
         indexFilesWithUpToDateUnit: indexFilesWithUpToDateUnit,
         logMessageToIndexLog: logMessageToIndexLog,
+        timeout: updateIndexStoreTimeout,
         testHooks: testHooks
       )
     )

Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

@@ -19,6 +19,7 @@ import SwiftExtensions
 
 import struct TSCBasic.AbsolutePath
 import class TSCBasic.Process
+import struct TSCBasic.ProcessResult
 
 private let updateIndexStoreIDForLogging = AtomicUInt32(initialValue: 1)
 
@@ -120,6 +121,11 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription {
   /// See `SemanticIndexManager.logMessageToIndexLog`.
   private let logMessageToIndexLog: @Sendable (_ taskID: IndexTaskID, _ message: String) -> Void
 
+  /// How long to wait until we cancel an update indexstore task. This timeout should be long enough that all
+  /// `swift-frontend` tasks finish within it. It prevents us from blocking the index if the type checker gets stuck on
+  /// an expression for a long time.
+  private let timeout: Duration
+
   /// Test hooks that should be called when the index task finishes.
   private let testHooks: IndexTestHooks
 
@@ -147,6 +153,7 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription {
     indexStoreUpToDateTracker: UpToDateTracker<DocumentURI>,
     indexFilesWithUpToDateUnit: Bool,
     logMessageToIndexLog: @escaping @Sendable (_ taskID: IndexTaskID, _ message: String) -> Void,
+    timeout: Duration,
     testHooks: IndexTestHooks
   ) {
     self.filesToIndex = filesToIndex
@@ -155,6 +162,7 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription {
     self.indexStoreUpToDateTracker = indexStoreUpToDateTracker
     self.indexFilesWithUpToDateUnit = indexFilesWithUpToDateUnit
     self.logMessageToIndexLog = logMessageToIndexLog
+    self.timeout = timeout
     self.testHooks = testHooks
   }
 
@@ -366,19 +374,21 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription {
     let stdoutHandler = PipeAsStringHandler { logMessageToIndexLog(logID, $0) }
     let stderrHandler = PipeAsStringHandler { logMessageToIndexLog(logID, $0) }
 
-    // Time out updating of the index store after 2 minutes. We don't expect any single file compilation to take longer
-    // than 2 minutes in practice, so this indicates that the compiler has entered a loop and we probably won't make any
-    // progress here. We will try indexing the file again when it is edited or when the project is re-opened.
-    // 2 minutes have been chosen arbitrarily.
-    let result = try await withTimeout(.seconds(120)) {
-      try await Process.run(
-        arguments: processArguments,
-        workingDirectory: workingDirectory,
-        outputRedirection: .stream(
-          stdout: { stdoutHandler.handleDataFromPipe(Data($0)) },
-          stderr: { stderrHandler.handleDataFromPipe(Data($0)) }
+    let result: ProcessResult
+    do {
+      result = try await withTimeout(timeout) {
+        try await Process.run(
+          arguments: processArguments,
+          workingDirectory: workingDirectory,
+          outputRedirection: .stream(
+            stdout: { stdoutHandler.handleDataFromPipe(Data($0)) },
+            stderr: { stderrHandler.handleDataFromPipe(Data($0)) }
+          )
         )
-      )
+      }
+    } catch {
+      logMessageToIndexLog(logID, "Finished error in \(start.duration(to: .now)): \(error)")
+      throw error
     }
     let exitStatus = result.exitStatus.exhaustivelySwitchable
     logMessageToIndexLog(logID, "Finished with \(exitStatus.description) in \(start.duration(to: .now))")

Sources/SourceKitLSP/Swift/DocumentFormatting.swift

@@ -144,7 +144,7 @@ extension SwiftLanguageService {
     writeStream.send(snapshot.text)
     try writeStream.close()
 
-    let result = try await process.waitUntilExitSendingSigIntOnTaskCancellation()
+    let result = try await process.waitUntilExitStoppingProcessOnTaskCancellation()
     guard result.exitStatus == .terminated(code: 0) else {
       let swiftFormatErrorMessage: String
       switch result.stderrOutput {

Sources/SourceKitLSP/Workspace.swift

@@ -116,6 +116,7 @@ public final class Workspace: Sendable {
       self.semanticIndexManager = SemanticIndexManager(
         index: uncheckedIndex,
         buildSystemManager: buildSystemManager,
+        updateIndexStoreTimeout: options.indexOptions.updateIndexStoreTimeout,
         testHooks: options.indexTestHooks,
         indexTaskScheduler: indexTaskScheduler,
         logMessageToIndexLog: logMessageToIndexLog,
@@ -275,17 +276,24 @@ public struct IndexOptions: Sendable {
   /// Setting this to a value < 1 ensures that background indexing doesn't use all CPU resources.
   public var maxCoresPercentageToUseForBackgroundIndexing: Double
 
+  /// How long to wait until we cancel an update indexstore task. This timeout should be long enough that all
+  /// `swift-frontend` tasks finish within it. It prevents us from blocking the index if the type checker gets stuck on
+  /// an expression for a long time.
+  public var updateIndexStoreTimeout: Duration
+
   public init(
     indexStorePath: AbsolutePath? = nil,
     indexDatabasePath: AbsolutePath? = nil,
     indexPrefixMappings: [PathPrefixMapping]? = nil,
     listenToUnitEvents: Bool = true,
-    maxCoresPercentageToUseForBackgroundIndexing: Double = 1
+    maxCoresPercentageToUseForBackgroundIndexing: Double = 1,
+    updateIndexStoreTimeout: Duration = .seconds(120)
   ) {
     self.indexStorePath = indexStorePath
     self.indexDatabasePath = indexDatabasePath
     self.indexPrefixMappings = indexPrefixMappings
     self.listenToUnitEvents = listenToUnitEvents
     self.maxCoresPercentageToUseForBackgroundIndexing = maxCoresPercentageToUseForBackgroundIndexing
+    self.updateIndexStoreTimeout = updateIndexStoreTimeout
   }
 }

Tests/SourceKitLSPTests/BackgroundIndexingTests.swift

@@ -1344,6 +1344,35 @@ final class BackgroundIndexingTests: XCTestCase {
     )
     XCTAssertEqual(callHierarchyAfterReindex, [expectedCallHierarchyItem])
   }
+
+  func testCancelIndexing() async throws {
+    try await SkipUnless.swiftPMSupportsExperimentalPrepareForIndexing()
+    try SkipUnless.longTestsEnabled()
+
+    var serverOptions = SourceKitLSPServer.Options.testDefault
+    serverOptions.experimentalFeatures.insert(.swiftpmPrepareForIndexing)
+    serverOptions.indexOptions.updateIndexStoreTimeout = .seconds(1)
+
+    let dateStarted = Date()
+    _ = try await SwiftPMTestProject(
+      files: [
+        "Test.swift": """
+        func slow(x: Invalid1, y: Invalid2) {
+          x / y / x / y / x / y / x / y
+        }
+        """
+      ],
+      serverOptions: serverOptions,
+      enableBackgroundIndexing: true
+    )
+    // Creating the `SwiftPMTestProject` implicitly waits for background indexing to finish.
+    // Preparation of `Test.swift` should finish instantly because it doesn't type check the function body.
+    // Type-checking the body relies on rdar://80582770, which makes the line hard to type check. We should hit the
+    // timeout of 1s. Adding another 2s to escalate a SIGINT (to which swift-frontend doesn't respond) to a SIGKILL mean
+    // that the initial indexing should be done in ~3s. 30s should be enough to always finish within this time while
+    // also testing that we don't wait for type checking of Test.swift to finish.
+    XCTAssert(Date().timeIntervalSince(dateStarted) < 30)
+  }
 }
 
 extension HoverResponseContents {