mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-09 22:31:08 +00:00
fix: improve Orchestrator task persistence and file lock handling
- Increase lock staleness timeout from 31s to 60s for complex orchestrator scenarios - Increase lock retries from 5 to 10 with higher backoff timeouts (200ms-2s) - Add retry logic (3 attempts) for critical parent task metadata persistence - Add user-visible warnings when delegation metadata persistence fails - Improve error messages with context about lock contention causes This addresses issues where Orchestrator mode tasks would disappear from the task list after creating multiple subtasks in rapid succession, caused by lock contention during concurrent file writes.
This commit is contained in:
parent
953c7773c0
commit
8a5eccb199
2 changed files with 71 additions and 23 deletions
|
|
@ -3245,23 +3245,58 @@ export class ClineProvider
|
|||
initialStatus: "active",
|
||||
})
|
||||
|
||||
// 5) Persist parent delegation metadata
|
||||
try {
|
||||
const { historyItem } = await this.getTaskWithId(parentTaskId)
|
||||
const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId]))
|
||||
const updatedHistory: typeof historyItem = {
|
||||
...historyItem,
|
||||
status: "delegated",
|
||||
delegatedToId: child.taskId,
|
||||
awaitingChildId: child.taskId,
|
||||
childIds,
|
||||
// 5) Persist parent delegation metadata with retry logic
|
||||
// This is critical - if it fails, the parent task can "disappear" from the UI
|
||||
// or appear in an inconsistent state
|
||||
const maxRetries = 3
|
||||
let lastError: Error | undefined
|
||||
|
||||
for (let attempt = 1; attempt <= maxRetries; attempt++) {
|
||||
try {
|
||||
const { historyItem } = await this.getTaskWithId(parentTaskId)
|
||||
const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId]))
|
||||
const updatedHistory: typeof historyItem = {
|
||||
...historyItem,
|
||||
status: "delegated",
|
||||
delegatedToId: child.taskId,
|
||||
awaitingChildId: child.taskId,
|
||||
childIds,
|
||||
}
|
||||
await this.updateTaskHistory(updatedHistory)
|
||||
lastError = undefined // Success - clear any previous error
|
||||
break
|
||||
} catch (err) {
|
||||
lastError = err as Error
|
||||
this.log(
|
||||
`[delegateParentAndOpenChild] Attempt ${attempt}/${maxRetries} failed to persist parent metadata for ${parentTaskId} -> ${child.taskId}: ${
|
||||
(err as Error)?.message ?? String(err)
|
||||
}`,
|
||||
)
|
||||
|
||||
// If this was the last attempt, we need to handle the failure more seriously
|
||||
if (attempt === maxRetries) {
|
||||
const errorMsg = `Failed to persist parent task delegation metadata after ${maxRetries} attempts. Parent task ${parentTaskId} may appear in an inconsistent state.`
|
||||
this.log(`[delegateParentAndOpenChild] CRITICAL: ${errorMsg}`)
|
||||
console.error(`[delegateParentAndOpenChild] CRITICAL:`, errorMsg, err)
|
||||
|
||||
// Show a user-visible warning for this critical failure
|
||||
// Note: We don't throw here because the child task was already created
|
||||
// and is now the active task. Throwing would leave both tasks in a bad state.
|
||||
vscode.window.showWarningMessage(
|
||||
`Warning: Task delegation succeeded but metadata persistence failed. The parent task may not resume correctly. Error: ${(err as Error)?.message ?? String(err)}`,
|
||||
)
|
||||
} else {
|
||||
// Wait before retry with exponential backoff
|
||||
await delay(100 * Math.pow(2, attempt - 1))
|
||||
}
|
||||
}
|
||||
await this.updateTaskHistory(updatedHistory)
|
||||
} catch (err) {
|
||||
}
|
||||
|
||||
// If we failed all retries, log it clearly but don't throw
|
||||
// (child task is already active and functional)
|
||||
if (lastError) {
|
||||
this.log(
|
||||
`[delegateParentAndOpenChild] Failed to persist parent metadata for ${parentTaskId} -> ${child.taskId}: ${
|
||||
(err as Error)?.message ?? String(err)
|
||||
}`,
|
||||
`[delegateParentAndOpenChild] WARNING: Proceeding with delegation despite metadata persistence failure. Parent=${parentTaskId}, Child=${child.taskId}`,
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -54,18 +54,18 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso
|
|||
// Acquire the lock before any file operations
|
||||
try {
|
||||
releaseLock = await lockfile.lock(absoluteFilePath, {
|
||||
stale: 31000, // Stale after 31 seconds
|
||||
stale: 60000, // Increased to 60 seconds to handle complex orchestrator scenarios with multiple rapid writes
|
||||
update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long
|
||||
realpath: false, // the file may not exist yet, which is acceptable
|
||||
retries: {
|
||||
// Configuration for retrying lock acquisition
|
||||
retries: 5, // Number of retries after the initial attempt
|
||||
retries: 10, // Increased retries to handle rapid succession writes during orchestrator delegation
|
||||
factor: 2, // Exponential backoff factor (e.g., 100ms, 200ms, 400ms, ...)
|
||||
minTimeout: 100, // Minimum time to wait before the first retry (in ms)
|
||||
maxTimeout: 1000, // Maximum time to wait for any single retry (in ms)
|
||||
minTimeout: 200, // Increased minimum timeout for better contention handling
|
||||
maxTimeout: 2000, // Increased maximum timeout for high-contention scenarios
|
||||
},
|
||||
onCompromised: (err) => {
|
||||
console.error(`Lock at ${absoluteFilePath} was compromised:`, err)
|
||||
console.error(`[safeWriteJson] Lock at ${absoluteFilePath} was compromised:`, err)
|
||||
throw err
|
||||
},
|
||||
})
|
||||
|
|
@ -73,9 +73,22 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso
|
|||
// If lock acquisition fails, we throw immediately.
|
||||
// The releaseLock remains a no-op, so the finally block in the main file operations
|
||||
// try-catch-finally won't try to release an unacquired lock if this path is taken.
|
||||
console.error(`Failed to acquire lock for ${absoluteFilePath}:`, lockError)
|
||||
// Propagate the lock acquisition error
|
||||
throw lockError
|
||||
console.error(
|
||||
`[safeWriteJson] Failed to acquire lock for ${absoluteFilePath} after retries. This may indicate:
|
||||
- High contention from rapid task delegation (e.g., Orchestrator mode creating multiple subtasks)
|
||||
- A stale lock from a crashed process
|
||||
- File system issues
|
||||
|
||||
Error:`,
|
||||
lockError,
|
||||
)
|
||||
// Propagate the lock acquisition error with enhanced context
|
||||
const enhancedError = new Error(
|
||||
`Failed to acquire file lock for ${path.basename(absoluteFilePath)}: ${lockError instanceof Error ? lockError.message : String(lockError)}`,
|
||||
)
|
||||
;(enhancedError as any).originalError = lockError
|
||||
;(enhancedError as any).filePath = absoluteFilePath
|
||||
throw enhancedError
|
||||
}
|
||||
|
||||
// Variables to hold the actual paths of temp files if they are created.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue