fix(fan-curve): keep the firmware watchdog alive at a steady temperature - #36
Merged
Conversation
The firmware watchdog is one-shot: it counts down from the last fan command and hands the fan back to thinkpad_acpi when it reaches zero. arm_fan_watchdog() was called only inside the 'level changed' branch, which reads as correct but inverts the actual risk -- a curve sitting at a steady temperature issues no fan commands at all. So roughly 30s after the temperature settled, the firmware silently reclaimed the fan. last_level still matched the target, so nothing ever rewrote it, and the UI went on emitting fan-curve-update every 2s showing the curve as active. The failure is invisible and the steady state is the common case, not an edge case. Re-arms on a timer at half the watchdog interval, which leaves a full loop tick of slack so one delayed iteration cannot lose the fan. Cleared alongside last_level wherever the curve stops steering, so re-enabling arms immediately rather than inheriting a stale deadline. Verified by mutation: forcing watchdog_due() to false reproduces the old behaviour and both tests fail, one reporting 30s without a re-arm.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
thinkutils | 5c0bb06 | Jul 20 2026, 03:08 AM |
The update event carried only fan_level, taken from last_level.unwrap_or(target_level). When every write fails -- no helper installed, /proc/acpi/ibm/fan not writable -- last_level stays None and the event reported the level the curve *wanted*, byte-identical to one it had actually set. The fan-curve-error toast fires once, guarded by permission_error_reported, and is easy to miss or dismiss. After that the panel looked correct indefinitely while the curve had never touched the fan. Adds a controlling flag alongside the level. The panel appends "(not applied)" and tints the figure when it is false. Compared with === false so an event without the field still reads as controlling.
vietanhdev
added a commit
that referenced
this pull request
Jul 20, 2026
The concurrency block intended never to cancel on main -- the comment said
so -- and did the opposite:
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
The expression renders to the STRING "false", and a non-empty string is
truthy in that position, so main was cancelled like any other ref. It
failed silently for exactly as long as nobody merged twice in quick
succession.
Four main runs were cancelled during this batch of merges (#36, #37, #39,
#32), each with ZERO jobs recorded -- so those commits have no evidence
they ever built. The runs that were supposed to be the record of what
shipped are the ones that got killed.
Encoding the rule in the concurrency GROUP is unambiguous: on main the SHA
gives every run its own group, so there is nothing to supersede; every
other ref keeps a per-ref group, so a force-push still cancels the old run.
tests/workflow_concurrency.rs guards both halves -- an expression-valued
cancel-in-progress, and a group that lost its per-SHA component (which
with cancel-in-progress: true would cancel main on every push, strictly
worse than the bug it replaced). Mutation-verified: restoring the original
two lines fails both.
118 tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
arm_fan_watchdog()was called only inside thelast_level != Some(target_level)branch — i.e. only when the fan level changes.But thinkpad_acpi's watchdog is one-shot: it counts down from the last fan command and hands the fan back to firmware control when it hits zero. The file's own doc comment already stated this contract; the code just didn't satisfy it.
A curve sitting at a steady temperature issues no fan commands at all. So:
last_levelalready 0 → no write, no re-armlast_levelis stillSome(0), so nothing ever rewrites itThe curve is now inert until the temperature crosses a curve segment. Meanwhile
fan-curve-updatekeeps emitting every 2s, so the UI shows a working curve that hasn't touched the fan in minutes.The steady state is the common case, which makes this the normal outcome rather than an edge case, and it fails invisibly.
The fix
Re-arm on a timer instead of on level change.
watchdog_due()fires at half the watchdog interval, which leaves a full 2s loop tick of slack so one delayed iteration can't lose the fan.last_armedis cleared alongsidelast_levelwherever the curve stops steering (curve disabled, temperature unreadable), so re-enabling arms immediately rather than inheriting a stale deadline.Verification
Two tests, both mutation-verified — forcing
watchdog_due()tofalserestores the old behaviour and both fail:The second walks the real 2s loop cadence across five minutes of dead-steady temperature and asserts the gap never reaches the expiry.
99 tests pass, clippy clean.
Found while auditing the backend for runtime defects.