Skip to content

Commit d61960c

Browse files
ovaistariqclaude
andcommitted
fix: Simplify generation validation to fix logical inconsistencies
This commit addresses critical flaws in the generation validation logic identified in code review (confidence score 1/5): **Problems Fixed:** 1. Race condition: Was capturing inode.dir.generation without holding inode.mu lock (line 358) 2. Flawed logic: Checking for inodeGen+1 after sealing couldn't distinguish between "we sealed it" vs "someone else sealed it" 3. Over-validation: Pre-sealing generation checks (line 363) combined with post-sealing checks (line 371) created logical inconsistencies 4. Same issues in inode==parent case (lines 378-381) **Solution:** Simplified approach that only validates what matters: - For inode != parent: Seal inode unconditionally, only validate parent generation hasn't changed before marking gap (we care about parent consistency for gap marking, not inode state) - For inode == parent: Seal and mark gap while holding lock (no checks needed as we hold lock throughout) This eliminates race conditions and logical flaws while maintaining correct concurrency semantics. The test failure (I/O errors and timeout) was likely caused by these logical inconsistencies leading to incorrect gap marking and filesystem state corruption. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 828545e commit d61960c

File tree

1 file changed

+9
-16
lines changed

1 file changed

+9
-16
lines changed

core/dir.go

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -353,34 +353,27 @@ func (parent *Inode) listObjectsSlurp(inode *Inode, startAfter string, sealEnd b
353353
// the lock and we must NOT release it (they'll handle lock ordering).
354354
if lock {
355355
if inode != parent {
356-
// Capture both generations before unlocking to validate later
356+
// 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

360+
// Seal the inode with its own lock
361361
inode.mu.Lock()
362-
// Check if inode generation is still the same before sealing
363-
if atomic.LoadUint64(&inode.dir.generation) == inodeGen {
364-
inode.sealDir()
365-
}
362+
inode.sealDir()
366363
inode.mu.Unlock()
367364

368-
// Reacquire parent lock and verify both generations
365+
// Reacquire parent lock and verify parent hasn't changed
369366
parent.mu.Lock()
370-
if atomic.LoadUint64(&parent.dir.generation) == parentGen &&
371-
atomic.LoadUint64(&inode.dir.generation) == inodeGen+1 {
372-
// Parent hasn't changed and inode was sealed by us, mark gap as loaded
367+
if atomic.LoadUint64(&parent.dir.generation) == parentGen {
368+
// Parent hasn't changed, safe to mark gap as loaded
373369
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
374370
}
375371
parent.mu.Unlock()
376372
} else {
377-
// inode == parent, capture generation before sealing
378-
gen := atomic.LoadUint64(&parent.dir.generation)
373+
// inode == parent, we already hold the lock so seal directly
379374
parent.sealDir()
380-
// Only mark gap if generation changed as expected (incremented by 1)
381-
if atomic.LoadUint64(&parent.dir.generation) == gen+1 {
382-
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
383-
}
375+
// Mark gap as loaded after sealing while still holding lock
376+
parent.dir.markGapLoaded(NilStr(startWith), calculatedNextStartAfter)
384377
parent.mu.Unlock()
385378
}
386379
} else {

0 commit comments

Comments
 (0)