Skip to content

RUM-6171: Use FileObserver#3168

Draft
kikoveiga wants to merge 2 commits intodevelopfrom
kikoveiga/RUM-6171/file-observer-alt
Draft

RUM-6171: Use FileObserver#3168
kikoveiga wants to merge 2 commits intodevelopfrom
kikoveiga/RUM-6171/file-observer-alt

Conversation

@kikoveiga
Copy link
Contributor

@kikoveiga kikoveiga commented Feb 4, 2026

What does this PR do?

This is the follow-up on #2720, which was rolled back in #3124.
The fixes can be found in this commit, which include:

  • Add existsSafe call inside getReadableFile(), so that we don't return a stale file that no longer exists on disk.
  • Improve thread safety, by using synchronized and grouping the state into currentBatchState: AtomicReference<CurrentBatchState>.
  • Instead of using direct equality between event and FILE_OBSERVER_MASK, use bitmask logic to guarantee matches when some additional bits might have been set.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

This is the FileObserver-based approach from the previously closed PR.
Created as an alternative branch for comparison.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@kikoveiga kikoveiga force-pushed the kikoveiga/RUM-6171/file-observer-alt branch 3 times, most recently from df19652 to 4b68488 Compare February 4, 2026 18:29
@kikoveiga kikoveiga force-pushed the kikoveiga/RUM-6171/file-observer-alt branch from 4b68488 to 9f7d1cb Compare February 4, 2026 18:51
@datadog-official

This comment has been minimized.

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2026

Codecov Report

❌ Patch coverage is 96.38554% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.02%. Comparing base (39d8b4a) to head (9f7d1cb).
⚠️ Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
...al/persistence/file/batch/BatchFileOrchestrator.kt 96.39% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3168      +/-   ##
===========================================
+ Coverage    70.96%   71.02%   +0.06%     
===========================================
  Files          912      912              
  Lines        33548    33577      +29     
  Branches      5640     5648       +8     
===========================================
+ Hits         23804    23845      +41     
+ Misses        8173     8162      -11     
+ Partials      1571     1570       -1     
Files with missing lines Coverage Δ
...al/persistence/file/batch/BatchFileOrchestrator.kt 94.35% <96.39%> (+1.45%) ⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

currentBatchState.set(CurrentBatchState(newFile, 1L, now))
pendingFiles.incrementAndGet()
return newFile
synchronized(knownFiles) {
Copy link
Contributor

@aleksandr-gringauz aleksandr-gringauz Feb 6, 2026

Choose a reason for hiding this comment

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

Between currentBatchState.set() and knownFiles.add(), there's a brief window where:

  1. currentBatchState points to the new file
  2. knownFiles doesn't contain it yet

If listBatchFiles() is called during this window, it won't include the new file, causing getReusableWritableFile() to potentially return null when it shouldn't.

@kikoveiga kikoveiga self-assigned this Feb 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants