Skip to content

fix: stamp update-check cooldown only after a successful fetch - #321

Merged
chattymin merged 3 commits into
chattymin:mainfrom
Borisserz:fix/update-check-cooldown-only-on-success
Oct 5, 2026
Merged

chattymin merged 3 commits into
chattymin:mainfrom
Borisserz:fix/update-check-cooldown-only-on-success

Conversation

@Borisserz

@Borisserz Borisserz commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #320.

  • UpdateChecker.check() stamped lastChecked before the GitHub request finished. A failed lookup (offline, timeout, bad payload) still started the 30-minute cooldown, so reopening the popover stayed silent even though no release was seen.
  • lastChecked is now recorded only after the release is validated:
    • the release URL is a trusted https://github.com URL, and
    • the tag normalizes to a plain MAJOR.MINOR.PATCH version (optional v/V prefix, surrounding whitespace and leading zeros dropped) via UpdateChecker.normalizedReleaseVersion.
  • Malformed tags (latest, v2.5, 2.5.5.1, 2.5.x, release-2.5.5, non-ASCII digits, components that would overflow) and unsupported prerelease/build tags (v2.6.0-beta.1, 2.6.0-rc1, v2.6.0+build.5) are treated as failed checks: nothing is offered, and the next attempt fetches again instead of waiting 30 minutes. Before this, isNewer read non-numeric parts as 0, so a prerelease compared equal to the release and garbage compared as 0.0.0.
  • An in-flight guard keeps overlapping popover opens from stacking concurrent requests now that the early stamp is gone.
  • The fetch is injectable (fetchLatest:), so tests don't hit the network. Rebased onto current main, which added skip/settingsNotice/defaults. The validated version goes through the existing consider(latest:url:), so skip behaviour is unchanged.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • Other:

Test plan

  • testFailedCheckDoesNotStartTheCooldown: a nil fetch does not stamp; the next check fetches.
  • testSuccessfulCheckStartsTheCooldownAndAppliesTheRelease: a valid release applies and honours minInterval; the next fetch happens after the cooldown.
  • testRejectedReleaseUrlDoesNotStartTheCooldown: an unsafe URL is a failed check.
  • testMalformedOrPrereleaseTagIsAFailedCheckAndDoesNotBlockTheNextOne: 16 malformed and prerelease tags. For each: not offered, Settings still says current, and an immediate retry fetches and applies a valid v2.5.5.
  • testNormalizedReleaseVersion: prefix, whitespace and leading zeros normalized; prerelease, short and overflowing tags rejected.
  • testOverlappingChecksShareOneFetch: a check while one is in flight does not fetch; after it finishes, the next one does.

Fault injection, each reverted afterwards:

  • Stamping lastChecked before validation fails the failed-check, malformed-tag and overlap tests.
  • Removing tag validation fails the malformed/prerelease test.
  • Removing the in-flight guard fails the overlap test.

Results:

  • swift test --filter UpdateCheckerTests: 16 passed.
  • scripts/test-gate.sh: 1390 tests ran; 1389 pass. The only local failure is SessionKeySettingsRenderingTests, a UI layout test unrelated to this change. On this machine it currently fails even alone on other branches built from main, while it passed in CI on those branches.
  • docs/reference/defect-log.md: the entry now covers tag validation.

Checklist

  • swift build and swift test pass (CI build-test green; locally all but the unrelated UI flake noted above)
  • PR title and description are written in English
  • No copyrighted assets, secrets, or private tooling references are committed (see CONTRIBUTING)
  • Tests were added or updated for this change

@chattymin chattymin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please validate and normalize the release tag before recording . A malformed or otherwise unsupported tag must be treated as a failed check so the next attempt is not blocked for 30 minutes. Please add regression coverage for malformed and unsupported prerelease tags, and rebase onto the current main branch.

@chattymin

Copy link
Copy Markdown
Owner

Clarification: the timestamp that must only be recorded after successful tag validation is lastChecked.

Record lastChecked only when the fetched release has a trusted URL and a
plain MAJOR.MINOR.PATCH tag. Network failures, unsafe URLs, malformed tags,
and prerelease tags are failed checks, so the next attempt is not blocked
for 30 minutes. Add an in-flight guard so overlapping checks share one fetch.
@Borisserz
Borisserz force-pushed the fix/update-check-cooldown-only-on-success branch from 0d7b605 to e6bd4ca Compare October 3, 2026 19:39
@Borisserz

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed in e6bd4ca (rebuilt on current main):

  • lastChecked is recorded only after tag validation. check() now requires a trusted release URL and a tag that normalizes to plain MAJOR.MINOR.PATCH (UpdateChecker.normalizedReleaseVersion: optional v/V, whitespace and leading zeros dropped). Anything else is a failed check: nothing is offered and the next attempt is not blocked for 30 minutes.
  • Regression coverage: testMalformedOrPrereleaseTagIsAFailedCheckAndDoesNotBlockTheNextOne runs 16 malformed and prerelease tags (latest, v2.5, 2.5.5.1, v2.6.0-beta.1, 2.6.0-rc1, v2.6.0+build.5, overflowing and non-ASCII digits, …). Each one is not offered, and an immediate retry applies a valid release. testNormalizedReleaseVersion covers the normalizer directly. Each guard was fault-injected to confirm its test fails.
  • Rebased onto current main. The validated version flows through the existing consider(latest:url:), so skip/settingsNotice behaviour is unchanged.

@chattymin
chattymin merged commit a2ec364 into chattymin:main Oct 5, 2026
2 checks passed
cte13 added a commit to cte13/PokeTokenBar-iOS that referenced this pull request Oct 10, 2026
donky-ey pushed a commit to donky-ey/PokeDexBar that referenced this pull request Oct 16, 2026
Upstream chattymin#321 made a check return immediately while another was running.
Here that regressed manual checks: pressing Check now in Settings while a
popover-triggered lookup was in flight returned at once and showed "up to
date" until the first lookup finished (up to 15 s on a slow network). On
main, a manual check always waited for its own lookup. Keep the in-flight
Task and have overlapping callers await it, so there is still one fetch and
every caller sees its result. testOverlappingChecksShareOneFetch now fails
on the early return; the failed-then-retry case moved to its own test.

Also remove AppLog.flush(): since the chattymin#174 port, production uses only
writeAndFlush.
donky-ey added a commit to donky-ey/PokeDexBar that referenced this pull request Oct 16, 2026
#21)

* fix: stop a second instance from appearing when the login agent is registered (upstream chattymin#163)

The login item is an SMAppService agent whose plist sets RunAtLoad, so
registering it while the app runs makes launchd exec the binary again.
LaunchServices' duplicate-launch protection does not cover a direct exec,
so turning on "Launch at login" left two menu bar icons, and both
instances wrote player-state.json.

At launch, the later instance yields (ordered by the kernel's p_starttime;
launchDate is nil for launchd-exec'd processes) before the status item and
before CrashReporter.install, so the yielding instance cannot clear the
crash marker. Missing or tied start times never yield.

Fork changes: the legacy login-item migration path does not exist here;
an extra test pins the guard's position before CrashReporter.install
(mutation-checked); e2e.sh asserts a single instance.

* fix: flush AppLog on the willTerminate exit path (upstream chattymin#174)

AppKit posts willTerminate and then exits, so markClean's async
"clean shutdown" line was usually lost. AppLog now writes through an
injectable AsyncLineLog backend, and exit paths (markClean, the duplicate
instance yield) use AppLog.writeAndFlush, which drains the queue before
returning. The bundled-app write guard stays, so tests never touch the
real log.

Fork change: AppLogTests' functionBody helper now stops at the nearest
boundary. Upstream preferred the next `func` over the next `///`, so with
the fork's member order the scanned body ran into writeIfChanged and
failed on its legitimate write(message) call. The routing guard still
fails when writeAndFlush is rewritten as write()+flush() (checked).

* fix: wait for the app's PID instead of its name in the update loop (upstream chattymin#175)

The detached upgrade script waited with 'pgrep -x PokeDexBar', which matches
any instance by name. With a duplicate instance running it always ran out
the 20 s wait and then swapped the bundle under a live app. Pass the
terminating PID as $3 and wait with 'kill -0'. The script body is now a
static constant so tests can pin it; a fork-only assertion checks that it
upgrades this app's cask.

* fix: read a trailing zero as ending in a consonant for Korean particles

Josa's digit table left out 0, so a number ending in 0 took the
no-final-consonant particle ("#20가", "v1.19.0를"). However it is read
(영, 십, 백, 천, 만), a trailing zero ends in a consonant. Found while
porting the skipped-version copy; the digit test never covered 0.

* fix: show a skipped release in Settings instead of "up to date" (upstream chattymin#311)

The banner button said "Later" but permanently skipped that version, so
Check now then claimed the installed build was the latest. The button now
reads "Skip this version"; a skipped release is kept as `skipped`, and
Settings says so, with "Show again" and an Update button that installs it.
UpdateChecker takes injected UserDefaults so tests do not touch the real
skip key. Strings are ko/en/ja only; the Korean copy picks its particle
from the version.

* fix: stamp the update-check cooldown only after a validated release (upstream chattymin#321)

check() stamped lastChecked before fetching, so one failed lookup (network,
bad response, unsafe URL) blocked the next attempt for 30 minutes. Record the
cooldown only when the release has a trusted github.com URL and a plain
MAJOR.MINOR.PATCH tag; malformed and prerelease tags count as failed checks.
An in-flight guard keeps overlapping popover opens to one fetch. The fetch is
injectable so tests never hit the network. Upstream's stray default.profraw
is not ported.

* fix: let an overlapping update check wait for the one in flight (review)

Upstream chattymin#321 made a check return immediately while another was running.
Here that regressed manual checks: pressing Check now in Settings while a
popover-triggered lookup was in flight returned at once and showed "up to
date" until the first lookup finished (up to 15 s on a slow network). On
main, a manual check always waited for its own lookup. Keep the in-flight
Task and have overlapping callers await it, so there is still one fetch and
every caller sees its result. testOverlappingChecksShareOneFetch now fails
on the early return; the failed-then-retry case moved to its own test.

Also remove AppLog.flush(): since the chattymin#174 port, production uses only
writeAndFlush.

---------

Co-authored-by: 이동기 <ldg0831@midasin.com>
Strongorange added a commit to Strongorange/PokeTokenBar-windows that referenced this pull request Oct 19, 2026
Port macOS chattymin#321: a failed fetch, unsafe URL, malformed or prerelease tag
no longer starts the 30-minute cooldown, so the next popover open
retries right away. Prerelease tags such as v2.6.0-beta.1 compare equal
to their release under IsNewer, so they are neither applied nor counted
as checked. An in-flight guard keeps overlapping checks on one fetch.
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.

Failed update check starts a 30-minute cooldown and blocks retries

2 participants