Skip to content

chore: make AutoTokenRefresh a boolean pointer#74

Merged
varonix0 merged 3 commits into
mainfrom
daniel/boolean-pass
May 28, 2026
Merged

chore: make AutoTokenRefresh a boolean pointer#74
varonix0 merged 3 commits into
mainfrom
daniel/boolean-pass

Conversation

@varonix0

Copy link
Copy Markdown
Member

The AutoTokenRefresh=false option does nothing currently because the default value is being overwritten to true when its false. I've converted AutoTokenRefresh to a boolean pointer to fix this at the cost of changing the struct signature for folks already using the AutoTokenRefresh parameter

@infisical-review-police

Copy link
Copy Markdown

💬 Discussion in Slack: #pr-review-go-sdk-74-chore-make-autotokenrefresh-a-boolean-pointer

Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel.

@varonix0 varonix0 self-assigned this May 28, 2026
@varonix0 varonix0 requested a review from mathnogueira May 28, 2026 23:26
@greptile-apps

greptile-apps Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where setting AutoTokenRefresh: false in Config had no effect because setDefaults would unconditionally overwrite a false bool field with its struct-tag default (true). The fix converts AutoTokenRefresh to *bool, where nil means "enabled" (preserving existing behavior for callers who omit the field) and a pointer to false genuinely disables the background refresh.

  • The root-cause code — the reflect.Bool case in setDefaults — is cleanly removed along with the now-redundant struct tags on both AutoTokenRefresh and SilentMode.
  • All three call sites (NewInfisicalClient, UpdateConfiguration, and handleTokenLifeCycle) are updated to use the new autoTokenRefreshEnabled helper.
  • This is a breaking API change for any callers that currently set AutoTokenRefresh explicitly; they must switch to pointer syntax.

Confidence Score: 4/5

Safe to merge; the core fix is correct and all affected call sites are updated consistently.

The change is tightly scoped to a single file and the logic is straightforward. The two flagged points are both non-blocking: the missing Bool helper is a convenience gap, and the goroutine idle-loop after a runtime disable is a pre-existing design trait that is now simply reachable for the first time.

client.go — specifically the goroutine lifecycle in handleTokenLifeCycle and the absence of a pointer-helper export.

Important Files Changed

Filename Overview
client.go Converts AutoTokenRefresh from bool to *bool, removes the reflect.Bool default-override path, and adds autoTokenRefreshEnabled helper; fixes the stated bug cleanly with two minor usability follow-ups.

Comments Outside Diff (1)

  1. client.go, line 504-507 (link)

    P2 Goroutine keeps idling after AutoTokenRefresh is disabled at runtime

    When a client is created with AutoTokenRefresh enabled (nil or &true), the goroutine is started unconditionally. If UpdateConfiguration is later called with AutoTokenRefresh: Bool(false), the goroutine falls into the else branch and loops indefinitely on a 1-second sleep — it can only be terminated by cancelling the original context. This is now a reachable path (the old default-override bug prevented it), so callers who call UpdateConfiguration to turn off auto-refresh expecting the background worker to stop will be surprised. At minimum the comment on this branch should note that the goroutine stays alive until context cancellation.

Reviews (1): Last reviewed commit: "Update client.go" | Re-trigger Greptile

Comment thread client.go
@varonix0 varonix0 merged commit 4ed5667 into main May 28, 2026
2 checks passed
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