Skip to content

[disk-buffering] Duplicate export in Windows #1907

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

Closed
jigarkb opened this issue May 23, 2025 · 5 comments · Fixed by #1917
Closed

[disk-buffering] Duplicate export in Windows #1907

jigarkb opened this issue May 23, 2025 · 5 comments · Fixed by #1917

Comments

@jigarkb
Copy link

jigarkb commented May 23, 2025

Component(s)

disk-buffering

What happened?

Description

On Windows systems, the exportStoredBatch method returns true when a file exists in the buffer directory. However, once the file is fully read, the subsequent file.delete() call in cleanUp() fails silently—returning false but not handled by the code. As a result, the same file is repeatedly re-exported on each exportStoredBatch call until a new file is written, leading to redundant exports and significant load on backend APIs.

This behavior appears linked to how Windows handles file deletion. After file.delete() is called, the file’s metadata may reflect 0KB, but the contents persist until a subsequent I/O operation (e.g., file creation) triggers actual cleanup. This suggests possible lazy deletion by the Windows OS, though further investigation at the kernel level would be needed to confirm.

Reference: https://github.com/open-telemetry/opentelemetry-java-contrib/blob/v1.39.0/disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/internal/storage/files/ReadableFile.java#L124-L127

Steps to Reproduce

Deploy your code on Windows with debugEnabled set to true, and you'll consistently see the message Now exporting batch of ... repeated.

Expected Result

The file must be deleted and should not be re-exported.

Actual Result

The file is repeatedly processed and exported.

Component version

v1.39.0

Log output

No response

Additional context

No response

@jigarkb jigarkb changed the title [disk-buffering] Duplicate export in windows [disk-buffering] Duplicate export in Windows May 23, 2025
@jigarkb
Copy link
Author

jigarkb commented May 24, 2025

Found the root cause: Within ToDiskExporter thread, the writableFile is not closed until we try to write new file. Within FromDiskExporter thread, the readableFile class tries to delete the file which is left unclosed in ToDiskExporter. In Unix/Linux this is fine but in Windows its not allowing as references are in separate threads.

For this code to work in Windows, we need to close the file within ToDiskExporter thread. I am open to suggestions on best ways to do this.

cc: @LikeTheSalad @zeitlinger

@zeitlinger
Copy link
Member

from @jigarkb

Also, after diving deep into the code, I found that the FolderManager is not shared between FromDiskExporter and ToDiskExporter so I am not sure we will ever hit this code block. Let me know if I am missing something.

@zeitlinger
Copy link
Member

zeitlinger commented May 27, 2025

FolderManager is not shared between FromDiskExporter and ToDiskExporter

yes, that looks wrong

@LikeTheSalad wdyt about #1912?

@LikeTheSalad
Copy link
Contributor

FolderManager is not shared between FromDiskExporter and ToDiskExporter

yes, that looks wrong

@LikeTheSalad wdyt about #1912?

Not having a shared storage object is wrong, so I think we must fix it. So, thank you for creating this PR, @zeitlinger.

Regarding the file not being deleted in Windows, I've been taking a look into it and I think I have an idea of what could be happening, which seems related to an issue in ReadableFile where there's a channel holding the file before deleting it. I'm planning to create a PR to sort it out, however, it shouldn't have any conflicts with yours, @zeitlinger, so I think we should merge both. I'll also try and make ReadableFile cleaner, because I struggled to understand it, it's quite messy right now, and that makes its maintenance more complicated.

@LikeTheSalad
Copy link
Contributor

LikeTheSalad commented May 28, 2025

I've just created this PR where this issue should get fixed.

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 a pull request may close this issue.

3 participants