Skip to content

fix(auth): stop infinite updateAuthPlan loop on self-hosted #1901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
richiemcilroy merged 2 commits into from
Jun 8, 2026
+2 −5
Merged

fix(auth): stop infinite updateAuthPlan loop on self-hosted #1901

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9df1ddf
fix(auth): stop infinite updateAuthPlan loop when org fetch always fa…
ManthanNimodiya Jun 8, 2026
c19e55c
fix(auth): add comment explaining org refresh bootstrap guard
ManthanNimodiya Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions apps/desktop/src/utils/queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,12 +351,9 @@ export function createCustomDomainQuery() {
export function createOrganizationsQuery() {
const auth = authStore.createQuery();

// Refresh organizations if they're missing
// Bootstrap only: auth.rs stamps organizations_updated_at even on org-fetch failure, stopping the loop on self-hosted where the endpoint is absent.
createEffect(() => {
if (
auth.data?.user_id &&
(!auth.data?.organizations || auth.data.organizations.length === 0)
) {
if (auth.data?.user_id && !auth.data?.organizations_updated_at) {
commands.updateAuthPlan().catch(console.error);
}
Comment on lines 355 to 358

@greptile-apps greptile-apps Bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Behavioral gap: no org refresh on app restart when organizations_updated_at is persisted

organizations_updated_at is written to the on-disk store by the Rust backend and therefore survives app restarts. When a user closes and reopens the app while still signed in, the effect sees organizations_updated_at as already set and skips the fetch entirely — organizations are never refreshed until the user signs out. The old organizations.length === 0 guard had the same property for non-empty org lists, but for a self-hosted user whose org list was empty (the loop case), it would at least attempt a refresh on every restart. With this fix, a single failed attempt per sign-in cycle is all you get.

This is acceptable for the loop-prevention goal, but it's worth a comment explaining the intentional trade-off so a future reader doesn't accidentally "fix" it back.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/desktop/src/utils/queries.ts
Line: 354-357

Comment:
**Behavioral gap: no org refresh on app restart when `organizations_updated_at` is persisted**

`organizations_updated_at` is written to the on-disk store by the Rust backend and therefore survives app restarts. When a user closes and reopens the app while still signed in, the effect sees `organizations_updated_at` as already set and skips the fetch entirely — organizations are never refreshed until the user signs out. The old `organizations.length === 0` guard had the same property for non-empty org lists, but for a self-hosted user whose org list was empty (the loop case), it would at least attempt a refresh on every restart. With this fix, a single failed attempt per sign-in cycle is all you get.

This is acceptable for the loop-prevention goal, but it's worth a comment explaining the intentional trade-off so a future reader doesn't accidentally "fix" it back.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 355 to 358

@greptile-apps greptile-apps Bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Implicit dependency on Rust error-path always stamping the timestamp

The fix works because auth.rs unconditionally stamps organizations_updated_at when auth.organizations.is_empty() on error (lines 105-107). If that guard is ever relaxed to allow retries (e.g., "only stamp after N attempts"), the JS effect would re-enter the loop immediately. A short inline comment linking this condition to the Rust error path would protect against that silent regression.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/desktop/src/utils/queries.ts
Line: 354-357

Comment:
**Implicit dependency on Rust error-path always stamping the timestamp**

The fix works because `auth.rs` unconditionally stamps `organizations_updated_at` when `auth.organizations.is_empty()` on error (lines 105-107). If that guard is ever relaxed to allow retries (e.g., "only stamp after N attempts"), the JS effect would re-enter the loop immediately. A short inline comment linking this condition to the Rust error path would protect against that silent regression.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines 355 to 358

@greptile-apps greptile-apps Bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Migration loop for existing cloud users with non-empty org lists

The fix relies on auth.rs stamping organizations_updated_at after any updateAuthPlan call. This works for self-hosted users (always empty orgs) because the error path in auth.rs (lines 105–107) stamps the timestamp only when auth.organizations.is_empty(). Users upgrading from before PR #1898 may have a non-empty organizations list with organizations_updated_at = null — the field simply didn't exist. For these users the new trigger fires, updateAuthPlan runs, and if the org fetch happens to fail (transient network error, server hiccup), organizations_updated_at is not stamped because !is_empty(), the store is still saved unconditionally (line 111 of auth.rs), the reactive effect re-fires, and the loop restarts indefinitely.

The simplest fix is to stamp organizations_updated_at unconditionally in the error branch of auth.rs, removing the is_empty() guard — the JS logic no longer needs the org-count distinction to prevent the loop.

Also, the inline comment says "stamps organizations_updated_at even on org-fetch failure" which implies unconditional behaviour; tightening it to mention the is_empty() precondition would prevent a future reader from relying on an assumption that doesn't hold.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/desktop/src/utils/queries.ts
Line: 355-358

Comment:
**Migration loop for existing cloud users with non-empty org lists**

The fix relies on `auth.rs` stamping `organizations_updated_at` after any `updateAuthPlan` call. This works for self-hosted users (always empty orgs) because the error path in `auth.rs` (lines 105–107) stamps the timestamp only when `auth.organizations.is_empty()`. Users upgrading from before PR #1898 may have a non-empty `organizations` list with `organizations_updated_at = null` — the field simply didn't exist. For these users the new trigger fires, `updateAuthPlan` runs, and if the org fetch happens to fail (transient network error, server hiccup), `organizations_updated_at` is **not** stamped because `!is_empty()`, the store is still saved unconditionally (line 111 of `auth.rs`), the reactive effect re-fires, and the loop restarts indefinitely.

The simplest fix is to stamp `organizations_updated_at` unconditionally in the error branch of `auth.rs`, removing the `is_empty()` guard — the JS logic no longer needs the org-count distinction to prevent the loop.

Also, the inline comment says "stamps `organizations_updated_at` **even** on org-fetch failure" which implies unconditional behaviour; tightening it to mention the `is_empty()` precondition would prevent a future reader from relying on an assumption that doesn't hold.

How can I resolve this? If you propose a fix, please make it concise.

});
Expand Down
Loading