feat(monitor): appendfsync=always main-thread blocking advisory (valkey#3515)

[jamby77]

Summary

Adds the AOF fsync-policy advisory from valkey-io/valkey#3515 to the config-hazard subsystem (the sibling of #337's valkey#3983 hazard): with appendfsync always, every write fsyncs synchronously on the main thread, so disk latency becomes command latency. The broader upstream AOF-modernization work (WAL headers, io_uring, direct I/O) is unshipped and not pollable — the misconfiguration and its symptom are observable today.

Detection (per the issue's guardrails)

• appendonly=no → no-op (managed/ephemeral instances with AOF intentionally off never fire).
• appendfsync=always with no symptoms → low-severity advisory (severity: info, status: advisory): the config is a latency risk, consider everysec unless per-write durability is a hard requirement.
• Escalates to a warning hazard when symptoms confirm blocking, and the message names the specific counters: aof_delayed_fsync rising across probes, aof-fsync-always/aof-write LATENCY events, or aof_last_write_status != ok.
• appendfsync=everysec stays quiet unless aof_delayed_fsync climbs on two consecutive probes (the once-per-second background fsync itself backing up).
• Rate limiting comes from the subsystem's design: findings are TTL-cached polled state (60s) surfaced via health → dashboard banner, not repeated events.

Changes

• evaluateAppendfsyncHazard pure evaluator in config-hazard.ts; ConfigHazardFinding widened (new ids, info severity, advisory status).
• ConfigHazardService probes appendfsync, INFO persistence, and LATENCY LATEST on the existing TTL-cached path, tracking a per-connection rising streak for aof_delayed_fsync. Symptom probe failures degrade to config-only evaluation instead of suppressing the advisory.
• ConfigHazardBanner gains an advisory presentation (info icon, muted border) alongside hazard/unverified.

Test plan

• 9 pure evaluator tests (acceptance matrix: AOF off, always clean/rising/latency-event/write-status, everysec healthy/single-rise/steady-climb, unknown policies).
• 7 service tests (probe wiring, cross-probe streak escalation, everysec two-probe gate, LATENCY failure tolerance, coexistence with the valkey#3983 finding); cache tests updated to count probes rather than raw CONFIG GET calls.
• 2 new banner tests (advisory rendering, escalated hazard rendering).
• monitor suite 346/346, health + MCP health 7/7, banner 6/6; tsc --noEmit clean for api and web.

Closes #368

---

Note

Medium Risk
Adds read-only Redis probes on the health-polling path with nuanced escalation logic; failures degrade safely but incorrect LATENCY freshness or streak logic could cause false positives/negatives.

Overview
Extends the config-hazard monitor with appendfsync detection (valkey#3515): when AOF is on, appendfsync=always surfaces as a low-severity advisory unless fresh aof-fsync-always/aof-write LATENCY spikes or bad aof_last_write_status escalate to a hazard; everysec stays silent until aof_delayed_fsync rises on two consecutive TTL probes.

ConfigHazardService now aggregates ACL and appendfsync findings, probes CONFIG GET appendfsync, INFO persistence, LATENCY LATEST (300s freshness using server TIME to avoid clock skew), and tracks per-connection delayed-fsync streaks. Symptom probe failures fall back to config-only advisory evaluation.

Shared ConfigHazardFinding types gain new ids, info severity, and advisory status. The dashboard ConfigHazardBanner shows all active findings with status-specific presentation (including the new advisory style) instead of hiding advisories.

^{Reviewed by Cursor Bugbot for commit 69d941d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[jamby77]

@claude review

[jamby77]

@BugBot review

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 8f96986. Configure here.}

[KIvanow]

one issue plus a correctness nit.

1. Dead escalation path for appendfsync=always. aof_delayed_fsync is only incremented in the everysec branch of flushAppendOnlyFile() - in always mode the fsync is inline and the counter never moves. So the delayedFsyncRisingStreak >= 1 escalation on the always path (config-hazard.ts:486) can't fire in the field, and the tests exercising it (escalates when aof_delayed_fsync is rising, and the service delayed:5→9 case) validate a scenario that only exists with synthetic input - which reads as coverage we don't actually have. Please drop the delayed-fsync signal from the always branch (keep it for everysec, where it's the correct signal); real always escalation already comes from the aof-fsync-always LATENCY event and aof_last_write_status, which stay.

2. While you're in there - LATENCY freshness compares two clocks. Date.now() is the monitor host; spikeAtSeconds is the monitored server's time(NULL). Skew makes the 300s window wrong both ways (stale spikes read fresh, fresh ones suppressed), undercutting the freshness fix from the second commit. Anchor "now" to the server (TIME / INFO uptime) or track deltas across probes.

[jamby77]

Both fixed.

1. Dead escalation path — you're right, aof_delayed_fsync++ sits inside the AOF_FSYNC_EVERYSEC guard in flushAppendOnlyFile(), so under always the counter never moves. Dropped it from the always-branch symptoms; always now escalates only on the aof-* LATENCY events and aof_last_write_status, and everysec keeps the signal unchanged. The two tests that exercised the dead path are now negative assertions pinning the engine contract, so the reasoning survives in the suite instead of reading as coverage we didn't have.

2. Clock skew — readServerTimeSeconds anchors "now" to the monitored server via TIME, falling back to the local clock only when TIME is unavailable (no worse than what it replaces). Added coverage in both directions, each of which fails under the old comparison:

• server clock an hour behind → a spike 10s old on the server was being suppressed as an hour stale
• server clock an hour ahead → an hour-old spike was reading as current evidence

TIME costs one extra command, but only on the TTL-cached probe path, not per poll.