Skip to content

improve: reduce calls to check for update, do local jwt refresh check#154

Open
PrestigePvP wants to merge 4 commits intomainfrom
tre/eng-4704-investigate-global-latency
Open

improve: reduce calls to check for update, do local jwt refresh check#154
PrestigePvP wants to merge 4 commits intomainfrom
tre/eng-4704-investigate-global-latency

Conversation

@PrestigePvP
Copy link

Description 📣

Aims to fix issues (Infisical/infisical#1001) that several users pointed out regarding high latency CLI calls, especially for geographically distributed people using the CLI.

  • Removes a couple blocking Github calls to check for updates
  • Removes an infisical auth validation check from running every command
    • Attempts to validate JWT locally rather than using the API. In the case where the JWT is invalid the call will fail regardless.

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

Two new unit tests were added.

@linear
Copy link

linear bot commented Mar 20, 2026

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 20, 2026

Greptile Summary

This PR reduces CLI latency by (1) replacing the blocking per-command GitHub update check with an async background goroutine writing to a 24-hour disk cache, and (2) replacing the blocking server-side CallIsAuthenticated API round-trip with a local JWT expiry check using ParseUnverified. Both changes meaningfully reduce network calls on the hot path, which directly addresses the reported latency issues for geographically distributed users.

Key concerns found during review:

  • os.Exit bypasses the update-check defer (packages/cmd/root.go): Go's os.Exit does not run deferred functions. On the error path in Execute(), WaitForUpdateCheck() is never called, the background goroutine is killed mid-flight, and the cache write is abandoned. The WaitGroup pattern only works on the success path. A common fix is to wrap the logic in an inner function so that defer completes before os.Exit is called from the outer function.
  • ParseUnverified skips JWT signature validation (packages/util/credentials.go): The change from api.CallIsAuthenticated (server-validated) to IsJWTExpired (local exp claim only, no signature check) is a security model change. A crafted token with any valid-looking future exp claim will pass the local check regardless of its signature. The PR's reasoning ("the API call will fail anyway") is valid, but it widens the window in which partial command logic could execute before the first rejected API call.
  • Urgent updates spawn a GitHub request on every CLI invocation (packages/util/check-for-update.go): isCacheFresh returns false unconditionally for urgent updates, so every CLI run with a pending urgent update triggers a fresh background goroutine and GitHub API call. A short TTL (e.g. 5 minutes) for urgent entries would reduce churn while preserving prompt notification behaviour.

Confidence Score: 2/5

  • This PR should not be merged as-is due to the os.Exit / deferred WaitGroup bug and the security model change introduced by ParseUnverified.
  • Two concrete correctness/security issues reduce confidence: (1) the deferred WaitForUpdateCheck is silently skipped on all error paths due to Go's os.Exit behaviour, making the cache-persistence guarantee unreliable; (2) replacing a server-validated auth check with an unverified local JWT parse changes the security model in a meaningful way. The overall goal and the new caching architecture are well-designed, and test coverage is strong — once these issues are addressed the PR would be in good shape.
  • packages/cmd/root.go (deferred WaitGroup bypassed by os.Exit) and packages/util/credentials.go (ParseUnverified security model change) need the most attention before merging.

Important Files Changed

Filename Overview
packages/cmd/root.go Adds defer util.WaitForUpdateCheck() to block until the background update goroutine finishes — but os.Exit(1) on the error path bypasses all deferred functions in Go, so the cache write is abandoned on failures.
packages/util/credentials.go Replaces the server-side CallIsAuthenticated API call with a local IsJWTExpired check using ParseUnverified, which skips signature validation entirely — a notable security model change that allows a crafted JWT with a valid exp claim to pass local auth checks.
packages/util/check-for-update.go Refactors update checking to use a 24-hour disk cache with async background refresh via a goroutine and WaitGroup; atomic write with temp file + rename is well implemented, but urgent updates bypass the cache TTL entirely, causing a background GitHub request on every CLI invocation until the user upgrades.
packages/util/check-for-update_test.go New test file with good coverage of isCacheFresh, displayCachedUpdateNotice, and atomic write/read behaviour; edge cases like urgent updates, 48h grace period, stale caches, and corrupt JSON are all covered.
packages/util/credentials_test.go New test file with thorough coverage of IsJWTExpired including the 30-second buffer, missing exp claim, malformed JWTs, empty strings, and invalid base64 payloads.
packages/util/constants.go Adds UPDATE_CHECK_CACHE_FILE_NAME constant for the update cache file; straightforward and correct.
go.mod Adds github.com/golang-jwt/jwt/v5 v5.3.1 dependency for local JWT expiry checking.

Comments Outside Diff (1)

  1. packages/cmd/root.go, line 67-71 (link)

    P1 os.Exit bypasses deferred WaitForUpdateCheck

    In Go, os.Exit terminates the process immediately and does not run deferred functions. On any error path where RootCmd.Execute() returns an error, WaitForUpdateCheck() will never execute, the background goroutine will be killed mid-flight, and the cache write will be abandoned.

    This defeats the purpose of the WaitGroup pattern: the cache is only guaranteed to persist on the success path.

    A common Go pattern to address this is to use a helper that calls os.Exit after the deferred functions have run:

    func Execute() {
        code := execute()
        os.Exit(code)
    }
    
    func execute() int {
        defer util.WaitForUpdateCheck()
        err := RootCmd.Execute()
        if err != nil {
            return 1
        }
        return 0
    }

Last reviewed commit: "improve: reduce call..."

Comment on lines +5 to +9
inputs:
is_urgent:
description: "Mark this release as urgent (bypasses 48h grace period for update notifications)"
type: boolean
default: false
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is how we use the dispatch here. This workflow is actually not made to be used with dispatch because we need the tag; we use it in the workflow to create the image names and all that.

If we dispatch with is_urgent here, we need to select a new tag, but we can't create a new tag when we run a workflow manually (only when creating a release), which means that we would need to create the tag first, but if we create the tag first, this workflow would run without the is_urgent and that's our lock here.

For this case, I think a new workflow just to mark a release as urgent would make more sense. We can follow the same release process we have now, and after it, we can run another workflow against the tag, like in the image below, to mark it as urgent (which would basically update the description of the release)

Image

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, yeah, I've never used that in github before so I thought it might just work but I see the issue.

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.

2 participants