Skip to content

Commit 29c4690

Browse files
ovaistariqclaude
andcommitted
fix: Resolve critical concurrency issues in directory sealing
This commit addresses multiple race conditions and lock ordering issues: 1. Fixed generation capture timing: Now captures generation at the correct point relative to lock acquisition/release to prevent races. 2. Moved markGapLoaded after sealing: Ensures gaps are only marked as loaded after successful sealing, preventing inconsistent state. 3. For inode == parent with lock=true: Eliminated unnecessary unlock/relock pattern. Now seals directly while holding parent.mu and marks gap afterward. 4. For inode != parent with lock=true: Properly manages lock ordering by capturing parent generation before unlock, sealing inode with its own lock, then reacquiring parent lock to mark gap only if parent hasn't changed. 5. For lock=false: Caller manages locks, so we seal and mark gap while respecting caller's lock ownership. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 4b37034 commit 29c4690

File tree

1 file changed

+18
-27
lines changed

1 file changed

+18
-27
lines changed

core/dir.go

Lines changed: 18 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -342,53 +342,42 @@ func (parent *Inode) listObjectsSlurp(inode *Inode, startAfter string, sealEnd b
342342
}
343343

344344
if seal {
345-
// Calculate nextStartAfter first before marking gap
345+
// Calculate nextStartAfter first
346346
var calculatedNextStartAfter string
347347
if obj != nil {
348348
calculatedNextStartAfter = *obj.Key
349349
}
350350

351-
// Remember this range as already loaded before we release locks
352-
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
353-
354-
var inodeGen uint64
355-
356-
// Capture generation right before unlocking to minimize staleness window
357-
if inode != parent {
358-
inodeGen = atomic.LoadUint64(&inode.dir.generation)
359-
} else {
360-
inodeGen = atomic.LoadUint64(&parent.dir.generation)
361-
}
362-
363351
// When lock=true, we need to release parent lock before sealing inode
364352
// to avoid lock ordering issues. When lock=false, caller already holds
365353
// the lock and we must NOT release it (they'll handle lock ordering).
366354
if lock {
367-
parent.mu.Unlock()
368-
369355
if inode != parent {
356+
// Capture generation right before unlocking parent
357+
inodeGen := atomic.LoadUint64(&parent.dir.generation)
358+
parent.mu.Unlock()
359+
370360
inode.mu.Lock()
371-
// Check if generation changed while we didn't hold the lock
372-
if atomic.LoadUint64(&inode.dir.generation) == inodeGen {
373-
inode.sealDir()
374-
}
361+
inode.sealDir()
375362
inode.mu.Unlock()
376-
} else {
377-
// inode == parent, we need to reacquire the lock after unlocking
378-
parent.mu.Lock()
379363

380-
// Check generation after reacquiring lock
364+
// Reacquire parent lock and verify its generation
365+
parent.mu.Lock()
381366
if atomic.LoadUint64(&parent.dir.generation) == inodeGen {
382-
parent.sealDir()
367+
// Parent hasn't changed, mark gap as loaded
368+
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
383369
}
384-
385-
// Ensure parent lock is released before return in seal path
370+
parent.mu.Unlock()
371+
} else {
372+
// inode == parent, we already hold the lock so seal directly
373+
parent.sealDir()
374+
// Mark gap as loaded after sealing
375+
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
386376
parent.mu.Unlock()
387377
}
388378
} else {
389379
// lock=false: Caller holds parent.mu, so we can't do unlock/relock.
390380
// This is used by Rename which already handles lock ordering properly.
391-
// Just seal the inode directly.
392381
if inode != parent {
393382
inode.mu.Lock()
394383
inode.sealDir()
@@ -397,6 +386,8 @@ func (parent *Inode) listObjectsSlurp(inode *Inode, startAfter string, sealEnd b
397386
// inode == parent, caller holds parent.mu so we can seal directly
398387
inode.sealDir()
399388
}
389+
// Mark gap as loaded after sealing
390+
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
400391
}
401392

402393
nextStartAfter = ""

0 commit comments

Comments
 (0)