Skip to content

fix: Lazily instantiate the KV-IR writer when using disk buffering to prevent uploading files that only contain IR preamble.#18

Merged
davemarco merged 3 commits intoy-scope:mainfrom
davemarco:recoverfix
Feb 25, 2026
Merged

fix: Lazily instantiate the KV-IR writer when using disk buffering to prevent uploading files that only contain IR preamble.#18
davemarco merged 3 commits intoy-scope:mainfrom
davemarco:recoverfix

Conversation

@davemarco
Copy link
Contributor

@davemarco davemarco commented Jan 31, 2026

Description

I noticed a subtle behavioral change introduced by the KV-IR PR related to when the IR preamble gets written to the tag’s IR file.

Consider the expected behavior from the old implementation.

Suppose logs are written for a specific tag and the plugin crashes. During recovery, we upload that tag’s IR file to S3 and then truncate the file so it becomes empty. If no additional logs arrive for that tag and the plugin crashes again, the file remains empty and is deleted on startup. Nothing further is uploaded.

This worked because of how the old writer used an intermediate buffer. Even though the IR file itself already existed on disk, the writer did not write the preamble directly into the file. Instead, the preamble and subsequent bytes were written into a buffer, and that buffer was only flushed into the IR file when the first log record was actually written. If no logs arrived after recovery, nothing ever triggered a flush, so the truncated file stayed truly empty.

With the KV-IR changes, this behavior is different. The writer now writes the IR preamble directly into the IR file as soon as the writer is instantiated. As a result, after recovery and truncation, the new IR file is no longer truly empty — it immediately contains the preamble. If no logs are written for that tag and the plugin crashes again, this preamble-only file is treated as non-empty and gets uploaded to S3.

To address this, I changed the code to lazily instantiate the writer so that the IR preamble is only written once there is at least one log record to write, restoring the previous behavior.

Note a buffered writer wrapper could also solve this problem, but for now decided just to use the exported IR interface directly.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Sent some logs to plugin with disk buffer. Then closed and restarted plugin multiple times. Only the first restart actually sent logs. Later restarts sent nothing. Tmp folders were empty as well.

@coderabbitai
Copy link

coderabbitai bot commented Jan 31, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@davemarco davemarco marked this pull request as ready for review February 20, 2026 03:35
@davemarco davemarco requested a review from a team as a code owner February 20, 2026 03:35
@davemarco davemarco requested a review from davidlion February 20, 2026 03:35
@davemarco davemarco changed the title fix: Lazily instantiate the KV-IR writer to prevent uploading files that only contain IR preamble. fix: Lazily instantiate the KV-IR writer when using disk buffering to prevent uploading files that only contain IR preamble. Feb 20, 2026
@davemarco davemarco merged commit 46cc3e2 into y-scope:main Feb 25, 2026
3 checks passed
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.

2 participants