Repository navigation
fix(file): reuse fingerprinted file handles - #26930
pznamensky wants to merge 4 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
I have read the CLA Document and I hereby sign the CLA |
![datadoghq-integration[bot]](https://avatars.githubusercontent.com/u/16866914?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The fingerprint-to-watcher handoff preserves the opened file’s identity across path rotation. Targeted execution remains unverified because the required saluki Git dependency was unavailable offline.
🤖 Bits Code Review · Commit 65cb452
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65cb452547
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| path.clone(), | ||
| file, |
There was a problem hiding this comment.
When remove_after_secs is set (especially to 0), a path can be renamed and replaced after fingerprint_or_emit opens it but before this call, so file still refers to the old inode while path now names the replacement. The watcher then reads the old handle to EOF but stores the replacement path, and the no-bytes branch later calls remove_file(&watcher.path), deleting the unread replacement before the next discovery scan; verify that the path's device/inode still matches the watcher before removing it (or otherwise avoid binding the stale path to this handle).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb05f2f247
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// # Errors | ||
| /// Returns an error if opening the path, reading metadata, or removing the file fails. | ||
| pub(super) async fn remove_file(&self) -> io::Result<bool> { | ||
| let file = File::open(&self.path).await?; |
There was a problem hiding this comment.
When a producer makes a watched log unreadable after Vector has opened it (for example, via chmod during rotation) while Vector still has permission to unlink entries from the parent directory, this read-only File::open fails even though deletion would succeed. remove_after_secs then emits an error, failed rediscovery eventually drops the watcher, and the file is left behind until its permissions change; before this change the code called remove_file directly and did not require read access. Validate the identity through a metadata-only/no-read handle so configured cleanup still works.
Useful? React with 👍 / 👎.
Summary
Kubernetes logs appeared in directories named
0006-...and0026-...instead of2026-...(for path template likepath: %Y-%m-%d), although application timestamps inside the messages were correct.A reproducer exposed a rotation race: reopening a path after fingerprinting can read a replacement file under the original fingerprint. Later, the replacement's offset can be applied to the original file, skipping, duplicating, or truncating records, including CRI timestamp prefixes.
Reuse the fingerprinted file handle when creating or updating a watcher.
Vector configuration
Affects the shared reader used by
fileandkubernetes_logs. No configuration changes or checkpoint migration are required.How did you test this PR?
137 targeted tests passed on macOS, including existing file and Kubernetes source tests.
Deterministic regression tests cover rotation during watcher creation and replacement. Additional cases verify both fingerprint strategies, checkpoint resume, start-position options, and gzip handling.
Clippy and formatting checks passed.
Does this PR include user facing changes?
changelog.d/file_fingerprint_rotation.fix.md.LLM disclosure
Most of the implementation and regression tests were generated by an LLM, which also assisted with tracing and reproducing the rotation race.