Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/build/adapter_filesystem.cc
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,13 @@ auto BuildAdapterFilesystem::mark(const node_type &path)
}

try {
// Keep in mind that depending on the OS, filesystem, and even standard
// library implementation, this value might not be very reliable. In fact,
// in many cases it can be outdated. Therefore, we never cache this value
return std::filesystem::last_write_time(path);
const auto value{std::filesystem::last_write_time(path)};
// Within a single run, if we didn't build this file, its mtime won't
// change. If we did build it, refreshing already set a synthetic timestamp
// that the cache lookup above would have returned instead
std::unique_lock lock{this->mutex};
this->marks.emplace(path, value);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s a potential race here: another thread could call refresh(path) after the initial cache lookup but before this emplace, causing emplace to fail while mark() still returns the older value (so the returned mark can differ from what’s cached). Consider ensuring mark() returns the value actually stored in marks under the write lock so callers see a consistent mark.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

return value;
} catch (const std::filesystem::filesystem_error &) {
return std::nullopt;
}
Expand Down