Fix nil pointer dereference in snapshotFS.GetFile for non-existent files

[dyxushuai]

Two issues here:

1. nil value with type, can't be equal to nil in interface, https://go.dev/doc/faq#nil_error
2. We need to always initialize the entry regardless of the ok value.

The fix:

• Always call entry()
• Check if the returned *diskFile is nil
• Return nil from GetFile() if file doesn't exist (not interface wrapping nil)

Fixes #1821

[Copilot]

Pull Request Overview

This PR fixes a nil pointer dereference issue in the snapshotFS.GetFile method when handling non-existent files. The problem occurred due to Go's interface nil semantics where a nil value with a type is not equal to nil when stored in an interface.

• Refactored the LoadOrStore logic to always call the entry function regardless of cache hit/miss
• Added explicit nil check for the returned file before wrapping it in an interface
• Added comprehensive test coverage for non-existent file scenarios including cache behavior

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
internal/project/snapshotfs.go	Fixed nil pointer dereference by always calling entry() and checking for nil before returning
internal/project/snapshot_test.go	Added test cases to verify GetFile and ReadFile behavior with non-existent files

[dyxushuai]

@microsoft-github-policy-service agree

[jakebailey]

If you do:

Suggested change

	type memoizedDiskFile func() FileHandle

And then change the OnceValue func to return FileHandle, that also fixes the bug without introducing other changes.

[dyxushuai]

Yes, sounds good, another issue is we still need to try to initialize newEntry during the first GetFile call?

[dyxushuai]

However, if we always need to initialize the newEntry by calling entry, then we don't need memoizedDiskFile anymore. We can just init/store it directly without any lazy mechanism.

[jakebailey]

I'm not sure what you mean... I just changed this line then fixed the compiler error it makes, and then it just worked.

[dyxushuai]

if entry, ok := s.readFiles.LoadOrStore(s.toPath(fileName), newEntry); ok {
	return entry()
}
return nil

ok == false here did not exist before, but stored a newEntry, So return nil directly.

I'm not sure what you mean... I just changed this line then fixed the compiler error it makes, and then it just worked.

Of course, it works when GetFile is called twice.

• First call: LoadOrStore stores a memoized function and returns nil
• Second call: LoadOrStore returns the existing entry (ok=true)

However, I am not sure shaw we call entry to get the file directly in the first call?

entry, _ := s.readFiles.LoadOrStore(s.toPath(fileName), newEntry)
return entry()

[jakebailey]

The latter code seems correct to me. In this atomic pattern we typically discard the ok entirely

[dyxushuai]

The latter code seems correct to me. In this atomic pattern we typically discard the ok entirely

Pls review it again

[dyxushuai]

Pull Request Overview

This PR fixes a nil pointer dereference issue in the snapshotFS.GetFile method when handling non-existent files. The problem occurred due to Go's interface nil semantics where a nil value with a type is not equal to nil when stored in an interface.

• Refactored the LoadOrStore logic to always call the entry function regardless of cache hit/miss
• Added explicit nil check for the returned file before wrapping it in an interface
• Added comprehensive test coverage for non-existent file scenarios including cache behavior

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/project/snapshotfs.go Fixed nil pointer dereference by always calling entry() and checking for nil before returning
internal/project/snapshot_test.go Added test cases to verify GetFile and ReadFile behavior with non-existent files

Always can't be nil, because we use the LoadOrStore method, there will be at least one value