Add logging and test coverage for document close race condition handling #3725

Copilot · 2026-01-02T08:42:24Z

Addressed review feedback on the document close race condition fix by improving observability and test coverage.

Changes

Extracted timeout constant: Added AWAIT_FORCE_TERMINATION constant (1 second) for the post-shutdown termination wait, replacing hardcoded values in multiple locations
Added diagnostic logging: Log warnings when worker threads fail to terminate within timeout after shutdownNow(), including:
- Normal shutdown path timeout
- Interrupt handler timeout
- Re-interruption during wait
Added test coverage for race condition scenarios:
- Document close with pending changes queued
- Document close during active change processing
- Document close with executor termination delays

All tests verify document closure using ServerContext.isDocumentOpened() to confirm proper cleanup.

Example

if (!docExecutor.awaitTermination(AWAIT_FORCE_TERMINATION, TimeUnit.SECONDS)) {
  LOGGER.warn(
    "Document executor for URI {} did not terminate within the additional timeout after shutdownNow()",
    uri
  );
}

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

coderabbitai · 2026-01-02T08:42:29Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Co-authored-by: nixel2007 <[email protected]>

Copilot

Pull request overview

This PR enhances the document close race condition handling by adding diagnostic logging, extracting a timeout constant for consistency, and introducing test coverage for various document close scenarios. The changes improve observability when worker threads fail to terminate gracefully after shutdown.

Key Changes:

Extracted hardcoded 1-second timeout into AWAIT_FORCE_TERMINATION constant for reuse across multiple code paths
Added warning logs when document executors fail to terminate within timeout after shutdownNow() (normal path, interrupt path, and re-interruption scenarios)
Added three new test cases covering document close with pending changes, active changes, and termination delays

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java`	Added `AWAIT_FORCE_TERMINATION` constant and warning logs for executor termination timeouts in the `didClose()` method
`src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java`	Added three new test methods to verify document closure behavior with pending/active changes and executor termination

Copilot · 2026-01-02T10:46:08Z

src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java

+  @Test
+  void didCloseAwaitTerminationCompletes() throws IOException {
+    // given - open a document
+    var textDocumentItem = getTextDocumentItem();
+    var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem);
+    textDocumentService.didOpen(didOpenParams);
+
+    var uri = textDocumentItem.getUri();
+    var documentContext = serverContext.getDocumentUnsafe(uri);
+    assertThat(documentContext).isNotNull();
+    assertThat(serverContext.isDocumentOpened(documentContext)).isTrue();
+
+    // when - close the document (which should wait for executor to terminate)
+    var closeParams = new DidCloseTextDocumentParams();
+    closeParams.setTextDocument(new TextDocumentIdentifier(uri));
+    textDocumentService.didClose(closeParams);
+
+    // then - verify the document is properly closed
+    // The close should complete successfully even if executor needs time to terminate
+    assertThat(serverContext.isDocumentOpened(documentContext)).isFalse();
+  }


This test appears to be redundant with the existing didClose() test at line 183. Both tests simply open a document and immediately close it without any intervening changes. The test doesn't actually verify "executor termination delays" as mentioned in the PR description because there are no pending operations to delay termination.

Consider either removing this test or modifying it to actually test a scenario with delays (e.g., by mocking the DocumentChangeExecutor to simulate slow termination).

Copilot · 2026-01-02T10:46:08Z

src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java

+  @Test
+  void didCloseWithPendingChanges() throws IOException {
+    // given - open a document and make changes
+    var textDocumentItem = getTextDocumentItem();
+    var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem);
+    textDocumentService.didOpen(didOpenParams);
+
+    var documentContext = serverContext.getDocumentUnsafe(textDocumentItem.getUri());
+    assertThat(documentContext).isNotNull();
+    assertThat(serverContext.isDocumentOpened(documentContext)).isTrue();
+
+    // when - submit multiple changes rapidly and then close immediately
+    var params = new DidChangeTextDocumentParams();
+    var uri = textDocumentItem.getUri();
+
+    for (int i = 0; i < 5; i++) {
+      params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2 + i));
+      var range = Ranges.create(0, 0, 0, 0);
+      var changeEvent = new TextDocumentContentChangeEvent(range, "// Change " + i + "\n");
+      List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>();
+      contentChanges.add(changeEvent);
+      params.setContentChanges(contentChanges);
+      textDocumentService.didChange(params);
+    }
+
+    // then - close should wait for pending changes to complete
+    var closeParams = new DidCloseTextDocumentParams();
+    closeParams.setTextDocument(new TextDocumentIdentifier(uri));
+    textDocumentService.didClose(closeParams);
+
+    // verify the document is closed
+    assertThat(serverContext.isDocumentOpened(documentContext)).isFalse();
+  }


The tests didCloseWithPendingChanges and didCloseDuringActiveChange may have race conditions. After submitting document changes with didChange(), the changes are processed asynchronously in the DocumentChangeExecutor's worker thread. The didClose() method should wait for these changes to complete, but the tests should verify that this waiting actually works correctly.

Consider using await() with atMost() (as done in other tests like didChangeIncremental() at line 116) to verify that the document changes were actually applied before closing. This would make the tests more robust and actually test the race condition handling rather than just testing that close completes.

Copilot · 2026-01-02T10:46:08Z

src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java

+  @Test
+  void didCloseDuringActiveChange() throws IOException {
+    // given - open a document
+    var textDocumentItem = getTextDocumentItem();
+    var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem);
+    textDocumentService.didOpen(didOpenParams);
+
+    var documentContext = serverContext.getDocumentUnsafe(textDocumentItem.getUri());
+    assertThat(documentContext).isNotNull();
+    assertThat(serverContext.isDocumentOpened(documentContext)).isTrue();
+
+    // when - submit a change
+    var params = new DidChangeTextDocumentParams();
+    var uri = textDocumentItem.getUri();
+    params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2));
+    var range = Ranges.create(0, 0, 0, 0);
+    var changeEvent = new TextDocumentContentChangeEvent(range, "// New content\n");
+    List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>();
+    contentChanges.add(changeEvent);
+    params.setContentChanges(contentChanges);
+    textDocumentService.didChange(params);
+
+    // then - close immediately while change may still be processing
+    var closeParams = new DidCloseTextDocumentParams();
+    closeParams.setTextDocument(new TextDocumentIdentifier(uri));
+    textDocumentService.didClose(closeParams);
+
+    // verify the document is closed
+    assertThat(serverContext.isDocumentOpened(documentContext)).isFalse();
+  }


Similar to didCloseWithPendingChanges, this test submits a change and immediately closes without verifying that the change processing would have completed. While the test verifies that didClose() completes successfully, it doesn't actually verify the race condition scenario where changes are actively processing.

Consider adding verification that the changes were actually applied, or add a small delay before closing to force the race condition scenario. This would more thoroughly test the synchronization logic in didClose().

Initial plan

6b92861

Copilot AI assigned Copilot and nixel2007 Jan 2, 2026

Copilot AI mentioned this pull request Jan 2, 2026

Исправлена потенциальная гонка при закрытии документа в BSLTextDocumentService. #3724

Merged

5 tasks

Copilot started work on behalf of nixel2007 January 2, 2026 08:49 View session

Copilot AI and others added 3 commits January 2, 2026 08:57

Add logging and extract timeout constant for document close termination

e2c80ca

Co-authored-by: nixel2007 <[email protected]>

Add test coverage for document close race conditions

fb345fb

Co-authored-by: nixel2007 <[email protected]>

Fix grammar in log message: 'second' -> 'seconds'

ff642f4

Co-authored-by: nixel2007 <[email protected]>

Copilot AI changed the title ~~[WIP] Fix potential race condition when closing document in BSLTextDocumentService~~ Add logging and test coverage for document close race condition handling Jan 2, 2026

Copilot AI requested a review from nixel2007 January 2, 2026 09:06

Copilot finished work on behalf of nixel2007 January 2, 2026 09:06

nixel2007 marked this pull request as ready for review January 2, 2026 10:41

Copilot AI review requested due to automatic review settings January 2, 2026 10:41

Copilot started reviewing on behalf of nixel2007 January 2, 2026 10:41 View session

Copilot AI reviewed Jan 2, 2026

View reviewed changes

nixel2007 merged commit 9f00ce2 into fix/possible-race-on-did-close Jan 2, 2026
7 checks passed

nixel2007 deleted the copilot/sub-pr-3724 branch January 2, 2026 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add logging and test coverage for document close race condition handling #3725

Add logging and test coverage for document close race condition handling #3725

Copilot AI commented Jan 2, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Jan 2, 2026

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Copilot AI Jan 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add logging and test coverage for document close race condition handling #3725

Add logging and test coverage for document close race condition handling #3725

Conversation

Copilot AI commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Example

Uh oh!

coderabbitai bot commented Jan 2, 2026

Review skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Jan 2, 2026 •

edited

Loading