Skip to content

Conversation

oskardudycz
Copy link
Contributor

@oskardudycz oskardudycz commented May 22, 2025

In the CI where the thread number is limited, 500ms may be too short for high threading tests, increased the max waiting time in SecondaryIndexCheckpointTrackerTests.

In the CI where thread number is limited 500ms may be too short for high threading tests, increaded the max waiting time in SecondaryIndexCheckpointTrackerTests.

Moved also reseting tracker loop waiter to happen before resetting the increment counter to 0.
Copy link
Contributor

github-actions bot commented May 22, 2025

Qodana for .NET

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@oskardudycz oskardudycz marked this pull request as ready for review May 22, 2025 11:57
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR aims to fix a race condition in the Secondary Index Checkpoint Tracker by adjusting the order of operations in the tracker loop and increasing the timeout in tests to accommodate high-threading scenarios.

  • Moved the _signal.Reset() call to occur before the counter exchange to prevent race conditions.
  • Increased timeout values in tests from 500ms to 5 seconds to ensure stability under high threading conditions.

Reviewed Changes

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

File Description
src/KurrentDB.SecondaryIndexing/Subscriptions/SecondaryIndexCheckpointTracker.cs Adjusts signal reset ordering to mitigate race conditions.
src/KurrentDB.SecondaryIndexing.Tests/Subscriptions/SecondaryIndexCheckpointTrackerTests.cs Increases timeout durations in tests to prevent premature failure.

}

private static readonly TimeSpan HalfOfASecond = TimeSpan.FromMilliseconds(500);
private static readonly TimeSpan FiveSeconds = TimeSpan.FromSeconds(5);
Copy link
Contributor

@sakno sakno May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It's better not to reflect the value as well as the type of the variable in its name. Just choose CommitSignalTimeout or similar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sakno, fair, updated in 1f4672d :)

@oskardudycz oskardudycz merged commit 3268b32 into master May 23, 2025
11 of 14 checks passed
@oskardudycz oskardudycz deleted the indexing/checkpoint_tracker_stability branch May 23, 2025 07:55
@timothycoleman timothycoleman changed the title Fixed race condition in Secondary Index Checkpoint Tracker Increased timeout in Secondary Index Checkpoint Tracker tests May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants