Skip to content
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.

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

SKUs: if the request id is not present, backfill using item id #21505

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

evq
Copy link
Member

@evq evq commented Jan 5, 2024

Resolves brave/brave-browser#35158

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@evq evq requested a review from husobee January 5, 2024 23:23
@evq evq requested a review from a team as a code owner January 5, 2024 23:23
Copy link
Contributor

@husobee husobee left a comment

Choose a reason for hiding this comment

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

lgtm

@evq evq marked this pull request as draft January 8, 2024 18:59
@evq evq marked this pull request as ready for review January 11, 2024 00:22
@evq
Copy link
Member Author

evq commented Jan 11, 2024

apologies for the hiatus, I temporarily turned this PR back into a draft while I investigated the interaction of this with some other pending changes server side. I've attached my notes on the various scenarios below.


before server side changes

current upgrade issue scenario:

  1. credentials generated
  2. credentials submitted to old endpoint
  3. fails, e.g. due to conflict
  4. user upgrades to new version
  5. user is prompted to log in / click "refresh" ( DELETE is called )
  6. credentials already generated are submitted with new random request id

case A: success

case B (success but times out):

  1. credentials submit but time out
  2. retry occurs, new random id
  3. submit with new random id fails due to conflict, goto 5

upgrade issue scenario after this PR:

1-6: ...

case A: success

case B:

  1. credentials submit but time out
  2. retry occurs, deterministic id
  3. submit with same id succeeds, goto 7

case C (multi device, request id conflict):

  1. credentials fail to submit due to request id conflict
  2. goto 5

after pending server side changes

current upgrade issue scenario:

1-6: ...
7. submit will always fail due to a conflict since DELETE does not delete

upgrade issue scenario after this PR:

1-6:...

case A: success

case B:

7-9: same as post-PR case B above

case C:

7-9: same as post-PR case C above

@evq evq merged commit 1b9401d into master Jan 11, 2024
21 checks passed
@evq evq deleted the skus/backfill-request-id branch January 11, 2024 00:34
@github-actions github-actions bot added this to the 1.63.x - Nightly milestone Jan 11, 2024
evq added a commit that referenced this pull request Jan 16, 2024
kjozwiak pushed a commit that referenced this pull request Jan 17, 2024
….62.x) (#21571)

* Uplift of #20820 (squashed) to beta

* Uplift of #21505 (squashed) to beta

---------

Co-authored-by: eV <[email protected]>
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.

SKUs: First credential refresh after upgrade to current nightly fails
3 participants