Skip to content

fix(oauth): follower last-chance read after poll deadline#4718

Merged
waleedlatif1 merged 2 commits into
stagingfrom
waleedlatif1/oauth-follower-last-chance
May 22, 2026
Merged

fix(oauth): follower last-chance read after poll deadline#4718
waleedlatif1 merged 2 commits into
stagingfrom
waleedlatif1/oauth-follower-last-chance

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Addresses the cursor-bot follow-up on improvement(oauth): coalesce token refresh + cache terminal failures #4706: when a coalesced refresh leader takes longer than the follower's 3s poll window, the follower returned null even if the leader persisted moments later. Adds a single last-chance DB read after the poll loop so a just-finished leader is still caught.
  • Trade-off: 1 extra DB read only in the rare timeout case. No-op in the common path.

Type of Change

  • Bug fix

Testing

  • New unit test: follower does a final read after timeout to catch a just-finished leader in lib/concurrency/__tests__/leader-lock.test.ts
  • 14/14 concurrency tests passing
  • Lint clean

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented May 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 22, 2026 6:02am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 22, 2026

PR Summary

Low Risk
Test-only change that adjusts timing/expectations around follower polling; low risk aside from potential flakiness if timing assumptions are wrong.

Overview
Updates the withLeaderLock timeout unit test to more precisely model the poll-loop deadline behavior by shortening maxWaitMs and asserting an explicit third onFollower call for the post-deadline last-chance read that returns the late leader value.

Reviewed by Cursor Bugbot for commit f3120cf. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 22, 2026

Greptile Summary

Adds a single last-chance onFollower() call after the follower's poll loop exits on deadline, closing the window where a leader could persist between the final in-loop poll and the timeout. The extra DB read only happens in the rare timeout case.

  • leader-lock.ts: Three lines appended after the while loop — reads onFollower() once more and returns the result if non-null, otherwise falls through to the existing timeout warning and null return.
  • leader-lock.test.ts: New test uses pollIntervalMs: 5 / maxWaitMs: 9 to guarantee the loop exits after exactly two in-loop polls; the third onFollower call lands in the last-chance block, and toHaveBeenCalledTimes(3) confirms it.

Confidence Score: 5/5

Safe to merge — the change is a minimal, well-tested addition that only runs in the already-failing timeout path.

The logic addition is three lines in a clearly understood function with no branching risk. The new test is correctly structured to isolate the last-chance block (third call is definitively post-deadline), and the existing 13 tests continue to cover all other paths. No data is mutated; the worst outcome on any failure is returning null — the same value as before this change.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/lib/concurrency/leader-lock.ts Adds a single last-chance onFollower() call after the poll loop exits, catching results that arrived between the final poll and the deadline. Logic is correct and consistent with existing error-handling patterns.
apps/sim/lib/concurrency/tests/leader-lock.test.ts New test correctly exercises the last-chance code path using pollIntervalMs: 5 / maxWaitMs: 9 so the loop exits after 2 in-loop polls and the third call hits the post-deadline block; toHaveBeenCalledTimes(3) makes the intent explicit.

Sequence Diagram

sequenceDiagram
    participant F as Follower
    participant DB as onFollower (DB read)

    F->>F: acquireLock → false
    F->>F: "deadline = now + maxWaitMs"

    loop "while Date.now() < deadline"
        F->>F: sleep(pollIntervalMs)
        F->>DB: onFollower()
        DB-->>F: null (leader not done yet)
    end

    Note over F: Loop exits (deadline passed)
    F->>DB: onFollower() [last-chance]
    DB-->>F: value (leader just finished)
    F-->>F: return value
Loading

Reviews (2): Last reviewed commit: "test(oauth): exercise last-chance read i..." | Re-trigger Greptile

Comment thread apps/sim/lib/concurrency/__tests__/leader-lock.test.ts
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit f3120cf. Configure here.

@waleedlatif1 waleedlatif1 merged commit 3d9a1c4 into staging May 22, 2026
10 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/oauth-follower-last-chance branch May 22, 2026 06:07
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