Skip to content

fix(update-notifier): only mark sentinel checked when notice is shown - #10104

Draft
nkolev92 wants to merge 2 commits into
latestfrom
nkolev92/update-notifier-sentinel-on-shown
Draft

nkolev92 wants to merge 2 commits into
latestfrom
nkolev92/update-notifier-sentinel-on-shown

Conversation

@nkolev92

@nkolev92 nkolev92 commented Oct 9, 2026 •

Copy link
Copy Markdown

Summary

The update-notifier sentinel was written as soon as a check started, not when a notice was actually shown. Fast commands (e.g. npm version) wrote the sentinel and exited before the notice could ever print, so the throttle window was wasted and the user never saw the update message.

This changes the sentinel's mtime to mean "last time a notice was shown", and retries the check on every eligible run until a notice is actually displayed.

Changes

  • updateCheck() returns { message, onShown } instead of a bare string. The sentinel is only written when onShown() runs, after the notice is logged.
  • onShown() uses writeFileSync since it's called from the process.on('exit', ...) handler, where async work can't be awaited.
  • Removed the weekly/daily split between GA and prerelease — now a single daily check for both.
  • Bundled npm.updateNotification into { message, onShown }, removing the separate updateNotificationShown field.
  • Silent runs don't show or record the notice, so a later non-silent run gets another chance.
  • Node.js engine compatibility is still evaluated fresh every run, never cached.

Tradeoffs

If a notice is never shown, every later eligible run retries the check. We preferred this over a more complex sentinel that caches a pending version/check state, since that would need to be environment-aware (Node version can change between runs) — not worth the added complexity.

nkolev92 and others added 2 commits October 9, 2026 16:16
Previously the `_update-notifier-last-checked` sentinel was written as
soon as a check *started*, regardless of whether a notice was ever
shown to the user. This meant fast commands (e.g. `npm version`) would
silently consume the check window without the user ever seeing the
update notice, since the sentinel's mtime only ever meant "a check was
attempted", not "a notice was shown".

This redefines the sentinel's mtime to mean "last time a notice was
actually displayed":

- `updateCheck()` now returns `{ message, onShown }` instead of a bare
  message string. The sentinel is written lazily via `onShown()`,
  invoked from the exit handler synchronously (via `writeFileSync`,
  required because `process.on('exit', ...)` can't await async work)
  only after the notice has actually been logged.
- The weekly/daily split between GA and prerelease checks is removed
  in favor of a single daily check for both, since the throttle is now
  tied to actual visibility rather than check attempts.
- `npm.updateNotification` is now a single bundled object
  (`{ message, onShown }`) instead of a bare string, removing the need
  for a separate `updateNotificationShown` field and the duplication
  that came with keeping two npm instance fields in sync.
- When silenced (`npm.silent`), the notice is not shown and the
  sentinel is not written, so a later non-silent run will still get a
  chance to show (and record) the notice.
- Node.js engine compatibility continues to be evaluated fresh on
  every run (never cached), since the active Node version can change
  between invocations (e.g. via nvm).

Ref: github/npm#15901 (comment)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r path

Full-suite coverage caught two branches the initial tests missed:
- entry.js: the branch that actually assigns npm.updateNotification
  when the notifier resolves a real notification (previously only
  exercised the null/no-op path since update-notifier.js isn't mocked
  in entry.js's own tests).
- exit-handler.js: the try/catch around onShown() when it throws,
  which only logs a verbose message and must not crash the exit path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

1 participant