Skip to content

Profiles: Make browser commit API call idempotent #2728

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

Jump to bottom
Merged
merged 10 commits into from
Jul 23, 2025
Merged

Profiles: Make browser commit API call idempotent #2728

merged 10 commits into from
Jul 23, 2025

Conversation

ikreymer
Copy link
Member

  • Fix race condition related to browser commit time
  • The profile commit request waits for browser to actual finish, and profile saved. This can cause request to time out, resulting in a retry, in which the browser has already been closed.
  • With these changes, the commit is now 'idempotent' and returns a waiting_for_browser until the profile is actually committed.

@ikreymer ikreymer requested a review from tw4l July 14, 2025 08:05
@tw4l
Copy link
Member

tw4l commented Jul 14, 2025 •
edited
Loading

Testing this on dev, the POST request to create the profile is being followed shortly after by a 422 response to the follow-up GET request. It seems like the browser is redirecting to the detail page before the profile is actually fully saved:

Screenshot 2025-07-14 at 12 25 51 PM

This is following the intitial POST, which returns {"detail": "waiting_for_browser"}

Edit: This may have actually just been a frontend caching issue! Tried in a new browser and it worked as expected. Will test more when I get a chance.

@ikreymer ikreymer force-pushed the profile-fix-commit-race-condition branch from 8ffb56c to 437e47f Compare July 14, 2025 18:06
@ikreymer ikreymer requested a review from SuaYoo July 14, 2025 19:14
tw4l
tw4l approved these changes Jul 15, 2025
View reviewed changes
Copy link
Member

@tw4l tw4l left a comment

Choose a reason for hiding this comment

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

Confirmed that the previous issue I had in testing was just related to my browser's cache. On an incognito window in Chrome this is working perfectly, and code changes look good.

ikreymer added 7 commits July 15, 2025 18:06
@ikreymer
profile cleanup:
fdb65af 
- fix race condition if commit takes too long
- return 'waiting_for_browser' immediately on initial commit, keep returning until browser actually committed
- update frontend to retry after waiting_for_browser received
@ikreymer
reformat
f3dc25b
@ikreymer
fix lint
52c6f0f
@ikreymer
always update expireTime when checking browser state
a412be1
@ikreymer
tests: fix commit to existing test
6ff3401
@ikreymer
tests: update timeouts
86e3ce8
@ikreymer
fix tests: move read-only check, catch exception in do_commit
837d1cc
@ikreymer ikreymer force-pushed the profile-fix-commit-race-condition branch from 437e47f to 837d1cc Compare July 16, 2025 01:06
SuaYoo
SuaYoo reviewed Jul 17, 2025
View reviewed changes
@SuaYoo SuaYoo self-requested a review July 17, 2025 01:13
ikreymer
ikreymer commented Jul 22, 2025
View reviewed changes
ikreymer
ikreymer commented Jul 22, 2025
View reviewed changes
@ikreymer ikreymer merged commit 8107b05 into main Jul 23, 2025
27 checks passed
@ikreymer ikreymer deleted the profile-fix-commit-race-condition branch July 23, 2025 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tw4l tw4l tw4l approved these changes

@SuaYoo SuaYoo Awaiting requested review from SuaYoo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ikreymer @tw4l @SuaYoo