Skip to content

Commit e81f3db

Browse files
ovaistariqclaude
andcommitted
fix: Eliminate critical race conditions in generation validation
This commit fixes critical race conditions that cause data corruption and inconsistent filesystem state (confidence score: 0/5). **Critical Issues Fixed:** 1. **Data race at line 358**: Was capturing inode.dir.generation without holding inode.mu lock. Atomic operations alone don't prevent races - we need to hold the lock to read protected state. 2. **Broken validation logic at line 372**: Checking if inode.generation == inodeGen after potential seal was logically flawed. It couldn't distinguish "we sealed it" from "someone else sealed it", leading to incorrect gap marking. 3. **Always-failing check at line 382**: Checking if generation == gen after sealDir() would ALWAYS fail because sealDir() increments generation by 1. This meant gaps were NEVER marked in the inode==parent case, causing filesystem inconsistency. 4. **Race window in lines 365-372**: Complex pre-seal checks created multiple race windows where concurrent modifications could result in marking gaps even though we didn't seal the directory. **Solution:** Reverted to the simple, correct approach: - For inode != parent: Seal unconditionally, only validate parent generation for gap marking consistency - For inode == parent: Seal and mark gap while holding lock (no validation needed) - No pre-seal generation checks that create race windows - No post-seal validation of inode state (we own the lock during seal) This eliminates all race conditions and logical errors while maintaining correct concurrency semantics. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent f32ac70 commit e81f3db

File tree

1 file changed

+4
-12
lines changed

1 file changed

+4
-12
lines changed

core/dir.go

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -355,33 +355,25 @@ func (parent *Inode) listObjectsSlurp(inode *Inode, startAfter string, sealEnd b
355355
if inode != parent {
356356
// Capture parent generation before unlocking to validate later
357357
parentGen := atomic.LoadUint64(&parent.dir.generation)
358-
inodeGen := atomic.LoadUint64(&inode.dir.generation)
359358
parent.mu.Unlock()
360359

361360
// Seal the inode with its own lock
362361
inode.mu.Lock()
363-
364-
// Check if generation changed while we didn't hold the lock
365-
if atomic.LoadUint64(&inode.dir.generation) == inodeGen {
366-
inode.sealDir()
367-
}
362+
inode.sealDir()
368363
inode.mu.Unlock()
369364

370365
// Reacquire parent lock and verify parent hasn't changed
371366
parent.mu.Lock()
372-
if atomic.LoadUint64(&parent.dir.generation) == parentGen && atomic.LoadUint64(&inode.dir.generation) == inodeGen {
367+
if atomic.LoadUint64(&parent.dir.generation) == parentGen {
373368
// Parent hasn't changed, safe to mark gap as loaded
374369
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
375370
}
376371
parent.mu.Unlock()
377372
} else {
378373
// inode == parent, we already hold the lock so seal directly
379-
gen := atomic.LoadUint64(&parent.dir.generation)
380374
parent.sealDir()
381-
// Only mark gap if generation didn't change during sealing
382-
if atomic.LoadUint64(&parent.dir.generation) == gen {
383-
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
384-
}
375+
// Mark gap as loaded after sealing while still holding lock
376+
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
385377
parent.mu.Unlock()
386378
}
387379
} else {

0 commit comments

Comments
 (0)