Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 12, 2025

  • Fix race condition in LsaNode causing ConcurrentModificationException
  • Add comprehensive concurrency test for LsaNode
  • Add GraphQL-specific test that reproduces original race condition
  • Resolve Detekt lint issues by adding exception logging in test
  • Fix Checkstyle violations in LsaNodeConcurrencyTest (star imports, trailing spaces, magic numbers, illegal catch, empty line separators)
  • Move test constants to class-level static final fields
  • Verify all tests pass and lint checks succeed

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@Copilot Copilot AI changed the title [WIP] Non deterministic crash caused by race conditions when accessing contents in GraphQL Query/Subscription Fix race condition in LsaNode causing ConcurrentModificationException in GraphQL subscriptions Sep 12, 2025
@Copilot Copilot AI requested a review from DanySK September 12, 2025 21:19
Copilot finished work on behalf of DanySK September 12, 2025 21:19
@DanySK DanySK marked this pull request as ready for review September 13, 2025 11:57
@DanySK DanySK force-pushed the copilot/fix-4464 branch 5 times, most recently from 743a663 to 778d22f Compare September 13, 2025 14:18
@DanySK
Copy link
Member

DanySK commented Sep 13, 2025

@copilot can you add a test in the graphql module that triggers the original problem, verify that it does indeed trigger the problem, and make sure that your patch solves?

Because I think the problem here could be more general: access to the environment should be performed using the simulation thread, or the risk of race conditions is always there.

Copy link
Contributor Author

Copilot AI commented Sep 13, 2025

@copilot can you add a test in the graphql module that triggers the original problem, verify that it does indeed trigger the problem, and make sure that your patch solves?

Because I think the probl...

I've added a comprehensive test in the GraphQL module that reproduces the original race condition and verifies the fix works:

Test created: LsaNodeGraphQLConcurrencyTest in alchemist-graphql/src/jvmTest/kotlin

What the test does:

  1. Loads a SAPERE simulation environment with LsaNodes
  2. Creates 20 reader threads that simulate GraphQL subscription access via NodeSurrogate.contents()
  3. Creates 20 writer threads that simulate simulation reactions modifying node state
  4. Each thread performs 500 operations concurrently
  5. Asserts that no ConcurrentModificationException occurs

Verification of fix:

  • ✅ Test fails when run against the original (unfixed) LsaNode code
  • ✅ Test passes when run against the synchronized LsaNode implementation

The test specifically targets the GraphQL access pattern that was causing the race condition: NodeSurrogate.contents()LsaNode.getContents() → iteration over unsynchronized instances collection.

Commit: 793deb8

Copilot finished work on behalf of DanySK September 13, 2025 15:06
@DanySK DanySK force-pushed the copilot/fix-4464 branch 8 times, most recently from 66fed3b to 2fced34 Compare September 14, 2025 08:52
@DanySK
Copy link
Member

DanySK commented Sep 14, 2025

@copilot build fails:

:alchemist-graphql:detektJvmTest FAILED	
/home/runner/work/Alchemist/Alchemist/alchemist-graphql/src/jvmTest/kotlin/it/unibo/alchemist/boundary/graphql/schema/model/LsaNodeGraphQLConcurrencyTest.kt:70:30: The caught exception is swallowed. The original exception could be lost. [SwallowedException]	
/home/runner/work/Alchemist/Alchemist/alchemist-graphql/src/jvmTest/kotlin/it/unibo/alchemist/boundary/graphql/schema/model/LsaNodeGraphQLConcurrencyTest.kt:92:38: The caught exception is swallowed. The original exception could be lost. [SwallowedException]	
/home/runner/work/Alchemist/Alchemist/alchemist-graphql/src/jvmTest/kotlin/it/unibo/alchemist/boundary/graphql/schema/model/LsaNodeGraphQLConcurrencyTest.kt:98:30: The caught exception is swallowed. The original exception could be lost. [SwallowedException]	

Copy link
Contributor Author

Copilot AI commented Sep 14, 2025

@copilot build fails:

...

Fixed the Detekt SwallowedException lint issues by adding proper logging to the exception handlers in the GraphQL concurrency test. The test continues to work correctly, successfully completing 20,000 concurrent operations (10,000 reads + 10,000 writes) with 0 ConcurrentModificationExceptions, verifying that the race condition fix is working as expected.

Commit: 4bbab22

Copilot finished work on behalf of DanySK September 14, 2025 20:37
Copy link

Please retry analysis of this Pull-Request directly on SonarQube Cloud

@DanySK DanySK enabled auto-merge (squash) October 11, 2025 17:42
@DanySK DanySK force-pushed the copilot/fix-4464 branch 13 times, most recently from b4df144 to 43e57de Compare October 13, 2025 21:26
Copy link

@DanySK DanySK merged commit 1899c20 into master Oct 13, 2025
26 checks passed
@DanySK DanySK deleted the copilot/fix-4464 branch October 13, 2025 22:29
DanySK pushed a commit that referenced this pull request Oct 14, 2025
## [42.3.10](42.3.9...42.3.10) (2025-10-14)

### Dependency updates

* **deps:** update danysk/makepkg docker tag to v2.0.8 ([#4864](#4864)) ([f518523](f518523))
* **deps:** update dependency scalafmt to v3.10.0 ([#4868](#4868)) ([b506b60](b506b60))
* **deps:** update plugin com.gradle.develocity to v4.2.2 ([#4867](#4867)) ([077a866](077a866))
* **deps:** update react to v2025.10.6-19.2.0 (patch) ([#4865](#4865)) ([bf44cee](bf44cee))
* **deps:** update react to v2025.10.7-19.2.0 (patch) ([#4866](#4866)) ([400d9e7](400d9e7))

### Bug Fixes

* **incarnation-sapere:** fix race condition in LsaNode causing ConcurrentModificationException in GraphQL subscriptions ([#4789](#4789)) ([1899c20](1899c20))

### General maintenance

* **build:** update the javadoc.io cache ([c778be2](c778be2))
* **build:** update the javadoc.io cache ([1ee4714](1ee4714))
@DanySK
Copy link
Member

DanySK commented Oct 14, 2025

🎉 This PR is included in version 42.3.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants