-
Notifications
You must be signed in to change notification settings - Fork 8
C API: wait on async path writer when parsing store path #325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughEmbeds an AsyncPathWriter into created Store objects, passes it into EvalState where applicable, and adds synchronization by waiting for a parsed store path via asyncPathWriter->waitForPath(...) before returning the StorePath. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant StoreModule as StoreModule
participant Store as nix::Store
participant AsyncWriter as AsyncPathWriter
participant Eval as EvalState
Caller->>StoreModule: openStore(uri, params)
StoreModule->>Store: nix::openStore(...)
StoreModule->>AsyncWriter: AsyncPathWriter::make(store)
StoreModule-->>Caller: return Store{store, asyncPathWriter}
Caller->>StoreModule: nix_store_parse_path(path_str)
StoreModule->>StoreModule: parse path -> s
StoreModule->>AsyncWriter: waitForPath(s)
AsyncWriter-->>StoreModule: ready
StoreModule-->>Caller: return StorePath(s)
Caller->>Eval: construct EvalState(..., buildStore, asyncPathWriter)
Eval->>AsyncWriter: use provided asyncPathWriter (or make(store) if null)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/libstore-c/nix_api_store.cc`:
- Around line 130-132: nix_store_parse_path currently creates an AsyncPathWriter
via AsyncPathWriter::make and calls waitForPath on a path returned from
parseStorePath even though that path was never added with addPath, which can
lead to calling .get() on an uninitialized future and indefinite blocking; fix
by changing nix_store_parse_path to avoid calling AsyncPathWriter::waitForPath
for parsed-only paths (skip the async writer path), or ensure the path is first
registered with AsyncPathWriter::addPath before waiting, add a bounded timeout
when calling future.get() inside waitForPath, and update AsyncPathWriter::make
to return an optional/nullable writer (or a no-op implementation) so caller code
gracefully handles stores that do not support async path writing (adjust callers
of AsyncPathWriter::make and waitForPath accordingly).
src/libstore-c/nix_api_store.cc
Outdated
| auto asyncPathWriter = nix::AsyncPathWriter::make(store->ptr); | ||
| nix::StorePath s = store->ptr->parseStorePath(path); | ||
| asyncPathWriter->waitForPath(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
find . -type f \( -name "*.hh" -o -name "*.h" \) | xargs grep -l "AsyncPathWriter" | head -5Repository: DeterminateSystems/nix-src
Length of output: 210
🏁 Script executed:
find . -type f \( -name "*.cc" -o -name "*.cpp" \) | xargs grep -l "AsyncPathWriter" | head -5Repository: DeterminateSystems/nix-src
Length of output: 191
🏁 Script executed:
rg -n "class AsyncPathWriter" --type cc --type hRepository: DeterminateSystems/nix-src
Length of output: 96
🏁 Script executed:
rg -n -A10 "waitForPath" --type cc --type hRepository: DeterminateSystems/nix-src
Length of output: 96
🏁 Script executed:
rg -n -A5 "AsyncPathWriter::make"Repository: DeterminateSystems/nix-src
Length of output: 1375
🏁 Script executed:
cat -n src/libstore/include/nix/store/async-path-writer.hhRepository: DeterminateSystems/nix-src
Length of output: 624
🏁 Script executed:
cat -n src/libstore/async-path-writer.ccRepository: DeterminateSystems/nix-src
Length of output: 6152
🏁 Script executed:
cat -n src/libstore-c/nix_api_store.cc | sed -n '120,140p'Repository: DeterminateSystems/nix-src
Length of output: 811
🏁 Script executed:
rg -B15 "AsyncPathWriter::make" src/libstore-c/nix_api_store.cc | head -25Repository: DeterminateSystems/nix-src
Length of output: 647
The code calls waitForPath() on a path that was never added to the async writer, creating undefined behavior.
The nix_store_parse_path() function creates an AsyncPathWriter and then calls waitForPath() on a path obtained from parseStorePath(). However, waitForPath() only tracks paths added via addPath()—not paths created by parseStorePath(). When the path is not found in the futures map (line 109), the lambda returns early with a void return (line 110), but the calling code then tries to call .get() on an uninitialized future variable, resulting in undefined behavior.
Additionally:
waitForPath()callsfuture.get()without any timeout (line 113), risking indefinite blocking if an async write stalls.AsyncPathWriter::make()unconditionally creates anAsyncPathWriterImplregardless of store type, with no graceful degradation for stores that may not support async path writing.
The likely intent was either to skip the async writer call for simple parse operations, or to use a different synchronization mechanism for parsed paths.
🤖 Prompt for AI Agents
In `@src/libstore-c/nix_api_store.cc` around lines 130 - 132, nix_store_parse_path
currently creates an AsyncPathWriter via AsyncPathWriter::make and calls
waitForPath on a path returned from parseStorePath even though that path was
never added with addPath, which can lead to calling .get() on an uninitialized
future and indefinite blocking; fix by changing nix_store_parse_path to avoid
calling AsyncPathWriter::waitForPath for parsed-only paths (skip the async
writer path), or ensure the path is first registered with
AsyncPathWriter::addPath before waiting, add a bounded timeout when calling
future.get() inside waitForPath, and update AsyncPathWriter::make to return an
optional/nullable writer (or a no-op implementation) so caller code gracefully
handles stores that do not support async path writing (adjust callers of
AsyncPathWriter::make and waitForPath accordingly).
3eda24e to
7bff820
Compare
|
I don't think this is correct. If the code isn't currently using You probably want to do this in Also, |
|
Hmm, I wasn't sure what the fix would be since I saw this failure when calling |
Motivation
Prevent
nix_store_parse_pathfrom failing to parse a valid store path.Context
Mutlithreaded evaluation & instantiation currently will have issues because the async path writer isn't synced up.
Summary by CodeRabbit