Skip to content

Fix: atomic write for last_run metadata to prevent corruption on SIGKILL - #199

Open
CDJ-happier wants to merge 2 commits into
logstash-plugins:mainfrom
CDJ-happier:fix/atomic-write-last-run-metadata
Open

Fix: atomic write for last_run metadata to prevent corruption on SIGKILL#199
CDJ-happier wants to merge 2 commits into
logstash-plugins:mainfrom
CDJ-happier:fix/atomic-write-last-run-metadata

Conversation

@CDJ-happier

Copy link
Copy Markdown

PR 描述

## What does this PR do?

Fixes a metadata file corruption issue in the JDBC input's `last_run` tracking that can cause full-table scans on restart.

### Problem

`FileHandler#write` uses `File.write(path, content)`, which is `open(O_WRONLY | O_CREAT | O_TRUNC)` + `write` + `close`. This is a non-atomic **truncate-then-write** sequence:

1. `open(O_TRUNC)` — file is immediately truncated to 0 bytes
2. `write(new_content)` — new content written
3. `close()`

If the process receives `SIGKILL` (e.g. from the OOM killer) between step 1 and step 2, the metadata file is left empty (0 bytes). On next startup, `read_last_run_metadata` reads the empty file, `YAML.load("")` returns `false` (Psych 3.x) or `nil` (Psych 4+), and `:sql_last_value` falls back to the default value:

- `NumericValueTracker`: `0`
- `TimeValueTracker`: `Time.at(0).utc` (1970-01-01)
- `DateTimeValueTracker`: `DateTime.new(1970)` (1970-01-01)

This causes the SQL `WHERE id >= :sql_last_value` to become `WHERE id >= 0`, triggering a full-table scan that loads historical data and can cascade into downstream write failures.

### Fix

Use **write-temp-then-rename** pattern: write to a `.tmp.<pid>` file in the same directory, then `File.rename` (atomic on the same filesystem) to replace the target. If the process is killed during the temp file write, the original metadata file remains intact.

```ruby
def write(value)
  tmp = "#{@path}.tmp.#{Process.pid}"
  ::File.write(tmp, YAML.dump(value))
  ::File.rename(tmp, @path)
  @exists = true
ensure
  ::File.delete(tmp) if tmp && ::File.exist?(tmp)
end

Why rename is atomic

rename(2) is guaranteed atomic by POSIX when source and destination are on the same filesystem. The destination file is either fully replaced or untouched—there is no intermediate state where it is empty or partially written. Since the temp file is created in the same directory as the target (same filesystem), this guarantee holds.

Why PID in the temp filename

Defensive against (unlikely) concurrent writes to the same path from multiple plugin instances. Each process gets its own temp file (<path>.tmp.<pid>), avoiding collision. The ensure block cleans up any residual temp file.

Testing

Added 4 rspec tests in spec/plugin_mixins/jdbc/value_tracking_spec.rb:

  1. correct write: value written correctly via temp + rename
  2. no temp residue: no .tmp.* file left after successful write
  3. failure preservation: when temp write raises (simulated Errno::ENOSPC), original file content is untouched and no temp file remains
  4. old content intact until rename: intercepted File.rename to verify target file still has old content at the moment just before rename

Real-world incident

This fix addresses a production incident observed in a Logstash pipeline:

  1. Logstash process OOM-killed (SIGKILL) mid-File.write → metadata file truncated to 0 bytes
  2. Restart → :sql_last_value fell back to 0 → SQL WHERE id >= 0 full-table scan loaded 100k historical rows
  3. Historical data written to ES data stream with create_time from 2022 → rejected by ES time-partition validation (must not write data whose timestamp has expired) → HTTP 500
  4. Logstash retried with exponential backoff → pipeline stalled → cascading OOM

The truncate-then-write root cause was confirmed via /var/log/messages (OOM kill at 01:57:38) and logstash-plain.log (SQL changed from id >= 1283605499467534336 to id >= 0 after restart at 01:58).

FileHandler#write used File.write (truncate-then-write), which is non-atomic.
If the process is SIGKILL'd (e.g. OOM killer) between open(O_TRUNC) and write,
the metadata file is left empty. On restart, YAML.load('') returns false/nil,
:sql_last_value falls back to default (0 / 1970-01-01), and the SQL becomes
'WHERE id >= 0' triggering a full-table scan.

Fix: write to a temp file (#{path}.tmp.#{pid}) then File.rename (atomic on
same filesystem). If killed mid-write, original file stays intact.

Added 4 rspec tests covering normal write, no temp residue, failure
preservation, and old-content-intact-until-rename.
@cla-checker-service

cla-checker-service Bot commented Jul 14, 2026

Copy link
Copy Markdown

❌ Author of the following commits did not sign a Contributor Agreement:
0dc1f4c, 129a6f1

Please, read and sign the above mentioned agreement if you want to contribute to this project

FileHandler is defined at module Jdbc scope, not nested under
ValueTracking. Use FileHandler directly (visible in module Jdbc context).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses corruption of the JDBC input’s last_run metadata file by switching FileHandler#write from a truncate-then-write approach to an atomic write-temp-then-rename pattern, reducing the chance of an empty/invalid metadata file after a SIGKILL (e.g., OOM kill) and preventing unintended full-table scans on restart.

Changes:

  • Implement atomic metadata persistence in FileHandler#write using temp-file + rename.
  • Add RSpec coverage for the atomic-write behavior and failure modes.
  • Bump plugin version to 5.6.4 and document the fix in the changelog.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
version Bumps plugin version to 5.6.4.
CHANGELOG.md Adds a 5.6.4 entry describing the atomic last_run metadata persistence fix.
lib/logstash/plugin_mixins/jdbc/value_tracking.rb Updates FileHandler#write to write to a temp file and atomically rename into place; adds cleanup.
spec/plugin_mixins/jdbc/value_tracking_spec.rb Adds specs validating correctness, cleanup, and failure-preservation semantics for atomic writes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# triggering a full-table scan on next run.
# rename(2) is atomic on the same filesystem, so the target file is either
# fully updated or untouched.
tmp = "#{@path}.tmp.#{Process.pid}"

# simulate failure when writing the temp file
allow(::File).to receive(:write).and_call_original
allow(::File).to receive(:write).with(/\.tmp\.\d+$/, anything).and_raise(Errno::ENOSPC)
@robbavey

Copy link
Copy Markdown
Member

@CDJ-happier Would you mind signing the CLA? Thanks!

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