fix(state-writer): skip atomic rename when content unchanged (#94)

d4rken · web-flow · commit c300ef06406f · 2026-05-18T14:01:20.000+02:00
* fix(state-writer): skip atomic rename when content unchanged

Wrappers under Restart=always with a sleep+exit-0 disabled branch (dump978-fa's no_hardware loop, airplanes-mlat's mlat_enabled_false loop) re-classify on every cycle and call airplanes_write_state with the same decision but a fresh decided_at timestamp. The atomic rename was firing every ~1200 cycles/day on a typical no-978-dongle feeder, waking any inotify consumer (e.g. systemd .path units watching the file).

Compare the proposed content against the current target ignoring decided_at= lines and skip the rename when they match. Existing on-disk decided_at stays, so consumers see the time of the last material decision rather than the time of the last wrapper restart. AIRPLANES_WRITE_STATE_FORCE=1 keeps the previous always-rename behavior for callers that need it.

* fix(state-writer): include trailing line in dedupe canonicaliser

A target left behind without a final newline (interrupted previous write, external tampering) would lose its last line in the bash while-read loop, so a proposed write that removed a tail field could falsely dedupe against the truncated remainder. The `|| [[ -n line ]]` tail picks up the unterminated last line. Regression coverage includes the no-trailing-newline case and a backslashes-plus-trailing-spaces round-trip.
diff --git a/scripts/lib/state-writer.sh b/scripts/lib/state-writer.sh
@@ -20,6 +20,17 @@
 # connected) is observable via systemd liveness, journal output, and
 # external probes.
 #
+# Dedupe: when an activation re-runs the wrapper and produces the same
+# decision (the common case for wrappers that sleep + exit 0 under
+# Restart=always — dump978-fa's no_hardware loop, airplanes-mlat's
+# disabled loop), the writer skips the atomic-rename if the proposed
+# content equals the current file content, ignoring decided_at= (which
+# is by convention a per-activation timestamp every caller refreshes).
+# Net effects: mtime stays at the time of the last *material* change,
+# inotify consumers (e.g. systemd .path units) don't fire on no-op
+# re-runs. AIRPLANES_WRITE_STATE_FORCE=1 disables this and always
+# atomically replaces the target.
+#
 # Readers MUST NOT `source` this file — values are unquoted; arbitrary
 # string content (e.g. user-supplied MLAT_USER) could contain shell
 # metacharacters. Parse line-by-line with KEY=VALUE split on first `=`.
@@ -34,7 +45,27 @@
 #       mlat_user=
 #
 # Returns 0 on success, 1 on validation failure or write error
-# (target file is left unchanged on either).
+# (target file is left unchanged on either). Returns 0 with target
+# unchanged when the dedupe path skips the rename.
+
+# Print $1's content with decided_at= lines filtered out. Used by the
+# dedupe path so a fresh timestamp on an otherwise-identical decision
+# doesn't force a rename. The `|| [[ -n "$line" ]]` tail catches an
+# existing target that's missing its final newline — without it the
+# loop would silently drop the last line and a proposed write that
+# removed a tail field could falsely dedupe against the truncated
+# remainder. (The writer's own output always ends in \n, so this only
+# matters for files left behind by an interrupted write or external
+# tampering.)
+_airplanes_write_state_canonical() {
+    local line
+    while IFS= read -r line || [[ -n "$line" ]]; do
+        case "$line" in
+            decided_at=*) continue ;;
+        esac
+        printf '%s\n' "$line"
+    done < "$1"
+}
 
 airplanes_write_state() {
     local target="$1"; shift
@@ -81,6 +112,17 @@ airplanes_write_state() {
         return 1
     fi
 
+    # Dedupe path: same decision as already on disk → skip the rename.
+    # The mode-preserving chmod is also skipped — the existing file's
+    # mode reflects an earlier successful write that already set 0644.
+    if [[ "${AIRPLANES_WRITE_STATE_FORCE:-}" != "1" ]] \
+        && [[ -f "$target" && -r "$target" ]] \
+        && [[ "$(_airplanes_write_state_canonical "$tmp")" \
+            == "$(_airplanes_write_state_canonical "$target")" ]]; then
+        rm -f "$tmp"
+        return 0
+    fi
+
     chmod 0644 "$tmp" || { rm -f "$tmp"; return 1; }
     mv -f "$tmp" "$target" || { rm -f "$tmp"; return 1; }
     return 0
diff --git a/test/test_state_writer.bats b/test/test_state_writer.bats
@@ -143,3 +143,209 @@ bar"
     grep -qx 'service=second' "$TARGET"
     ! grep -qx 'service=first' "$TARGET"
 }
+
+# --- Dedupe path ---
+# The writer skips the atomic rename when the proposed content matches
+# the current target ignoring decided_at= lines. This kills inotify
+# wakeups on no-op wrapper re-runs (Restart=always + sleep + exit 0)
+# without changing the on-disk content readers see.
+
+@test "dedupe: identical content with same decided_at → no rename, mtime preserved" {
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=enabled \
+        reason=ok \
+        decided_at=2026-05-18T10:00:00Z
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1  # widen mtime resolution window
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=enabled \
+        reason=ok \
+        decided_at=2026-05-18T10:00:00Z
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -eq "$mtime_before" ]
+}
+
+@test "dedupe: identical content with fresh decided_at → no rename" {
+    # Every wrapper restart cycle re-computes decided_at. Without
+    # ignoring it the file would be atomically replaced ~1200 times/day
+    # on a feeder where the daemon's classify produces the same result
+    # cycle after cycle.
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=disabled \
+        reason=mlat_enabled_false \
+        decided_at=2026-05-18T10:00:00Z
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=disabled \
+        reason=mlat_enabled_false \
+        decided_at=2026-05-18T10:00:30Z
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -eq "$mtime_before" ]
+    # Old decided_at stays — readers see the time of the last material
+    # decision, not the time of the last wrapper restart.
+    grep -qx 'decided_at=2026-05-18T10:00:00Z' "$TARGET"
+}
+
+@test "dedupe: changed non-timestamp field → full rename happens" {
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=enabled \
+        reason=ok \
+        decided_at=2026-05-18T10:00:00Z
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=disabled \
+        reason=mlat_enabled_false \
+        decided_at=2026-05-18T10:00:30Z
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -gt "$mtime_before" ]
+    grep -qx 'state=disabled' "$TARGET"
+    grep -qx 'reason=mlat_enabled_false' "$TARGET"
+    grep -qx 'decided_at=2026-05-18T10:00:30Z' "$TARGET"
+}
+
+@test "dedupe: added field → full rename happens" {
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=disabled \
+        decided_at=2026-05-18T10:00:00Z
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=disabled \
+        decided_at=2026-05-18T10:00:30Z \
+        new_field=v1
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -gt "$mtime_before" ]
+    grep -qx 'new_field=v1' "$TARGET"
+}
+
+@test "dedupe: removed field → full rename happens" {
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=enabled \
+        reason=ok \
+        decided_at=2026-05-18T10:00:00Z \
+        extra=v
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1
+    airplanes_write_state "$TARGET" \
+        service=airplanes-mlat \
+        state=enabled \
+        reason=ok \
+        decided_at=2026-05-18T10:00:30Z
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -gt "$mtime_before" ]
+    ! grep -qx 'extra=v' "$TARGET"
+}
+
+@test "dedupe: reordered fields → full rename happens (order is semantic)" {
+    # KEY=VALUE order is part of the writer's contract (caller-provided
+    # order). A reorder is a content change.
+    airplanes_write_state "$TARGET" \
+        service=foo \
+        state=enabled \
+        decided_at=2026-05-18T10:00:00Z
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1
+    airplanes_write_state "$TARGET" \
+        state=enabled \
+        service=foo \
+        decided_at=2026-05-18T10:00:30Z
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -gt "$mtime_before" ]
+}
+
+@test "dedupe: AIRPLANES_WRITE_STATE_FORCE=1 always renames" {
+    airplanes_write_state "$TARGET" \
+        service=foo \
+        state=enabled \
+        decided_at=2026-05-18T10:00:00Z
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1
+    AIRPLANES_WRITE_STATE_FORCE=1 airplanes_write_state "$TARGET" \
+        service=foo \
+        state=enabled \
+        decided_at=2026-05-18T10:00:30Z
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -gt "$mtime_before" ]
+    grep -qx 'decided_at=2026-05-18T10:00:30Z' "$TARGET"
+}
+
+@test "dedupe: target missing → write proceeds (no dedupe path engaged)" {
+    [ ! -e "$TARGET" ]
+    airplanes_write_state "$TARGET" service=foo state=enabled
+    [ -f "$TARGET" ]
+    grep -qx 'service=foo' "$TARGET"
+}
+
+@test "dedupe: validation failure short-circuits before dedupe check" {
+    airplanes_write_state "$TARGET" service=before
+    run airplanes_write_state "$TARGET" service=after 'bad-key=value'
+    [ "$status" -eq 1 ]
+    grep -qx 'service=before' "$TARGET"
+}
+
+@test "dedupe: existing target without trailing newline → tail field still compared" {
+    # An external writer or an interrupted previous run could leave a
+    # target with no final newline. Without the `|| [[ -n line ]]` tail
+    # in the canonicaliser the last line silently disappears from the
+    # comparison, and a proposed write that removed the trailing field
+    # would falsely dedupe against the truncated remainder, preserving
+    # stale content. This case asserts the canonicaliser includes the
+    # final line so the writer detects the difference and renames.
+    printf 'schema_version=1\nservice=mlat\nstate=disabled\nextra=v' \
+        > "$TARGET"
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1
+    airplanes_write_state "$TARGET" \
+        service=mlat \
+        state=disabled
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -gt "$mtime_before" ]
+    ! grep -qx 'extra=v' "$TARGET"
+}
+
+@test "dedupe: values with backslashes + trailing spaces preserved verbatim" {
+    # `IFS= read -r` + `printf '%s\n'` round-trips backslashes and
+    # trailing whitespace; canonicalisation must not mangle them, or
+    # else two semantically-identical writes would compare unequal and
+    # break dedupe.
+    airplanes_write_state "$TARGET" \
+        "feed_bin=/usr/bin/x\\trailing " \
+        "decided_at=2026-05-18T10:00:00Z"
+    local mtime_before
+    mtime_before="$(stat -c %Y "$TARGET")"
+    sleep 1
+    airplanes_write_state "$TARGET" \
+        "feed_bin=/usr/bin/x\\trailing " \
+        "decided_at=2026-05-18T10:00:30Z"
+    local mtime_after
+    mtime_after="$(stat -c %Y "$TARGET")"
+    [ "$mtime_after" -eq "$mtime_before" ]
+    grep -qFx 'feed_bin=/usr/bin/x\trailing ' "$TARGET"
+}
