Skip to content

Commit 5a9b225

Browse files
jpicklykclaude
andcommitted
feat: implement atomic locking with mutex-based synchronization
- Add tryAcquireLock() method to SimpleLockingService for atomic lock acquisition - Replace TOCTOU-vulnerable two-step locking with single atomic operation - Use Mutex-based synchronization to prevent race conditions in lock acquisition - Update SimpleLockAwareToolDefinition to use atomic locking pattern - Ensure thread safety for concurrent sub-agent operations - Maintain backward compatibility with existing tests (773/773 passing) This resolves race conditions where multiple agents could simultaneously pass canProceed() checks before any called recordOperationStart(), leading to concurrent access to the same resources. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent d109e20 commit 5a9b225

File tree

8 files changed

+808
-666
lines changed

8 files changed

+808
-666
lines changed

src/main/kotlin/io/github/jpicklyk/mcptask/application/service/SimpleLockingService.kt

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package io.github.jpicklyk.mcptask.application.service
22

33
import io.github.jpicklyk.mcptask.domain.model.*
4+
import kotlinx.coroutines.sync.Mutex
5+
import kotlinx.coroutines.sync.withLock
46
import org.slf4j.LoggerFactory
57
import java.time.Instant
68
import java.util.*
@@ -25,6 +27,28 @@ interface SimpleLockingService {
2527
* Records the completion of an operation.
2628
*/
2729
suspend fun recordOperationComplete(operationId: String)
30+
31+
/**
32+
* Atomically attempts to acquire a lock for the given operation.
33+
* This combines conflict checking and operation recording in a single atomic operation
34+
* to prevent TOCTOU race conditions.
35+
*/
36+
suspend fun tryAcquireLock(operation: LockOperation): LockAcquisitionResult
37+
}
38+
39+
/**
40+
* Result of attempting to acquire a lock atomically.
41+
*/
42+
sealed class LockAcquisitionResult {
43+
/**
44+
* Lock was successfully acquired.
45+
*/
46+
data class Success(val operationId: String) : LockAcquisitionResult()
47+
48+
/**
49+
* Lock acquisition failed due to conflicts.
50+
*/
51+
data class Conflict(val conflictingOperations: List<LockOperation>) : LockAcquisitionResult()
2852
}
2953

3054
/**
@@ -66,6 +90,9 @@ class DefaultSimpleLockingService(
6690
private val activeOperations = ConcurrentHashMap<String, LockOperation>()
6791
private val operationStartTimes = ConcurrentHashMap<String, Instant>()
6892

93+
// Mutex to ensure atomic lock acquisition and prevent TOCTOU race conditions
94+
private val lockMutex = Mutex()
95+
6996
override suspend fun canProceed(operation: LockOperation): Boolean {
7097
logger.debug("Checking if operation '${operation.description}' can proceed")
7198

@@ -116,6 +143,49 @@ class DefaultSimpleLockingService(
116143
logger.debug("Completed operation '$operationId'")
117144
}
118145

146+
override suspend fun tryAcquireLock(operation: LockOperation): LockAcquisitionResult {
147+
return lockMutex.withLock {
148+
logger.debug("Attempting to acquire lock for operation '${operation.description}'")
149+
150+
// Clean up expired operations first (lazy cleanup)
151+
cleanupExpiredOperations()
152+
153+
// Check for conflicts using the same logic as canProceed
154+
val conflictingOperations = activeOperations.values.filter { activeOp ->
155+
val entityOverlap = activeOp.entityIds.intersect(operation.entityIds).isNotEmpty()
156+
157+
when {
158+
// DELETE operations are blocked by any operation on same entities
159+
operation.operationType == OperationType.DELETE && entityOverlap -> true
160+
// Other operations are blocked by DELETE operations on same entities
161+
activeOp.operationType == OperationType.DELETE && entityOverlap -> true
162+
// WRITE operations are blocked by other WRITE operations on same entities
163+
operation.operationType == OperationType.WRITE && activeOp.operationType == OperationType.WRITE && entityOverlap -> true
164+
// CREATE operations are blocked by other CREATE operations on same entities (prevents duplicate creation)
165+
operation.operationType == OperationType.CREATE && activeOp.operationType == OperationType.CREATE && entityOverlap -> true
166+
// STRUCTURE_CHANGE operations are blocked by any non-READ operation on same entities
167+
operation.operationType == OperationType.STRUCTURE_CHANGE && activeOp.operationType != OperationType.READ && entityOverlap -> true
168+
activeOp.operationType == OperationType.STRUCTURE_CHANGE && operation.operationType != OperationType.READ && entityOverlap -> true
169+
// No conflicts for other operation combinations (READ operations never conflict)
170+
else -> false
171+
}
172+
}
173+
174+
if (conflictingOperations.isNotEmpty()) {
175+
logger.debug("Operation '${operation.description}' blocked due to ${conflictingOperations.size} conflicting operation(s)")
176+
return@withLock LockAcquisitionResult.Conflict(conflictingOperations)
177+
}
178+
179+
// No conflicts found - acquire the lock atomically
180+
val operationId = "op-${UUID.randomUUID()}"
181+
activeOperations[operationId] = operation
182+
operationStartTimes[operationId] = Instant.now()
183+
184+
logger.debug("Successfully acquired lock for operation '${operation.description}' with ID '$operationId'")
185+
LockAcquisitionResult.Success(operationId)
186+
}
187+
}
188+
119189
/**
120190
* Gets statistics about active operations.
121191
*/

src/main/kotlin/io/github/jpicklyk/mcptask/application/tools/base/SimpleLockAwareToolDefinition.kt

Lines changed: 61 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -164,57 +164,78 @@ abstract class SimpleLockAwareToolDefinition(
164164
}
165165

166166
/**
167-
* Checks if the operation can proceed and handles any lock conflicts gracefully.
167+
* Executes an operation with proper locking lifecycle management.
168+
* This ensures operations are recorded, conflicts are detected, and cleanup happens.
168169
*/
169-
protected suspend fun checkOperationPermissions(
170+
protected suspend fun executeWithLocking(
170171
operationName: String,
171172
entityType: io.github.jpicklyk.mcptask.domain.model.EntityType,
172-
entityId: UUID
173-
): JsonElement? {
174-
// For Phase 2, this just logs the operation intent
175-
// In Phase 3, this will perform actual lock checking
173+
entityId: UUID,
174+
operation: suspend () -> JsonElement
175+
): JsonElement {
176+
// If locking is disabled, execute directly
177+
if (!shouldUseLocking() || lockingService == null || sessionManager == null) {
178+
return operation()
179+
}
176180

177-
lockingService?.let { service ->
178-
sessionManager?.let { session ->
179-
val operation = io.github.jpicklyk.mcptask.application.service.LockOperation(
180-
operationType = mapOperationToLockType(operationName),
181-
toolName = name,
182-
description = "Tool operation: $operationName on ${entityType.name}",
183-
entityIds = setOf(entityId)
184-
)
185-
186-
val canProceed = service.canProceed(operation)
187-
if (!canProceed) {
188-
// Create a simulated conflict error for demonstration
189-
val context = createLockErrorContext(operationName, entityType, entityId, session.getCurrentSession())
190-
val conflictError = LockError.LockConflict(
191-
message = "Operation cannot proceed due to conflicting locks",
192-
conflictingLocks = emptyList(), // Would be populated in real implementation
193-
requestedLock = io.github.jpicklyk.mcptask.domain.model.LockRequest(
181+
val lockOperation = io.github.jpicklyk.mcptask.application.service.LockOperation(
182+
operationType = mapOperationToLockType(operationName),
183+
toolName = name,
184+
description = "Tool operation: $operationName on ${entityType.name}",
185+
entityIds = setOf(entityId)
186+
)
187+
188+
// Atomically attempt to acquire the lock
189+
val lockResult = lockingService.tryAcquireLock(lockOperation)
190+
191+
when (lockResult) {
192+
is io.github.jpicklyk.mcptask.application.service.LockAcquisitionResult.Conflict -> {
193+
val context = createLockErrorContext(operationName, entityType, entityId, sessionManager.getCurrentSession())
194+
val conflictError = LockError.LockConflict(
195+
message = "Operation cannot proceed due to conflicting locks",
196+
conflictingLocks = emptyList(),
197+
requestedLock = io.github.jpicklyk.mcptask.domain.model.LockRequest(
198+
entityId = entityId,
199+
scope = io.github.jpicklyk.mcptask.domain.model.LockScope.TASK,
200+
lockType = io.github.jpicklyk.mcptask.domain.model.LockType.SHARED_WRITE,
201+
sessionId = sessionManager.getCurrentSession(),
202+
operationName = operationName,
203+
expectedDuration = 120L // 2 minutes
204+
),
205+
suggestions = errorHandler.suggestAlternatives(emptyList(),
206+
io.github.jpicklyk.mcptask.domain.model.LockRequest(
194207
entityId = entityId,
195208
scope = io.github.jpicklyk.mcptask.domain.model.LockScope.TASK,
196209
lockType = io.github.jpicklyk.mcptask.domain.model.LockType.SHARED_WRITE,
197-
sessionId = session.getCurrentSession(),
210+
sessionId = sessionManager.getCurrentSession(),
198211
operationName = operationName,
199-
expectedDuration = 1800L // 30 minutes
200-
),
201-
suggestions = errorHandler.suggestAlternatives(emptyList(),
202-
io.github.jpicklyk.mcptask.domain.model.LockRequest(
203-
entityId = entityId,
204-
scope = io.github.jpicklyk.mcptask.domain.model.LockScope.TASK,
205-
lockType = io.github.jpicklyk.mcptask.domain.model.LockType.SHARED_WRITE,
206-
sessionId = session.getCurrentSession(),
207-
operationName = operationName,
208-
expectedDuration = 1800L
209-
), context),
210-
context = context
211-
)
212-
return handleLockError(conflictError)
212+
expectedDuration = 120L
213+
), context),
214+
context = context
215+
)
216+
return handleLockError(conflictError)
217+
}
218+
is io.github.jpicklyk.mcptask.application.service.LockAcquisitionResult.Success -> {
219+
// Lock acquired successfully, continue with operation
220+
val operationId = lockResult.operationId
221+
222+
try {
223+
// Execute the actual operation
224+
return operation()
225+
} finally {
226+
// Always clean up the lock, even if operation fails
227+
lockingService.recordOperationComplete(operationId)
213228
}
214229
}
215230
}
216-
217-
return null // Operation can proceed
231+
}
232+
233+
/**
234+
* Helper method to extract entity ID from different parameter structures.
235+
*/
236+
protected fun extractEntityId(params: JsonElement, paramName: String = "id"): UUID {
237+
return extractUuidFromParameters(params, paramName)
238+
?: throw ToolValidationException("Missing or invalid $paramName parameter")
218239
}
219240

220241
private fun mapOperationToLockType(operationName: String): io.github.jpicklyk.mcptask.application.service.OperationType {

0 commit comments

Comments
 (0)