Skip to content

github: Look up whether a comment landed before resending a mutation - #302

Merged
jerry-skydio merged 1 commit into
mainfrom
jerry/revup/main/comments
Sep 25, 2026
Merged

jerry-skydio merged 1 commit into
mainfrom
jerry/revup/main/comments

Conversation

@jerry-skydio

Copy link
Copy Markdown
Collaborator

Github applies mutation fields in order and their effects survive its gateway
giving up, and a transient http failure carries no field results, so replaying
the request posts the comment a second time. A real 502 while uploading a 60 pr
stack duplicated the review graph and patchsets comments on 13 prs, and three
attempts left some prs with three copies each.

Fields now carry a replay_safe flag, and addComment is the only unsafe one:
github allows unlimited identical comments, and echoes clientMutationId rather
than treating it as an idempotency key. Every other mutation is addressed by an
id or is a create that github rejects as UNPROCESSABLE the second time, which
already reads as done.

A request holding an unsafe field is sent once. If that fails transiently, read
back the newest comments of each subject pr and resend only the fields whose
body isn't there yet, within the usual retry budget.

Limitations:

  • The check is a read, so a comment that did land can still be duplicated if
    github serves a stale replica, or if 20 newer comments push it out of the
    window being read.
  • A comment the check can't prove missing (the pr node came back null, or the
    lookup itself failed) counts as added, leaving it for the next upload to add.
  • Exhausting the retry budget raises, so the rest of that mutation and the rest
    of the upload don't run.

@jerry-skydio

Copy link
Copy Markdown
Collaborator Author

Reviews in this chain:
└#302 github: Look up whether a comment landed before resending a mutation

@jerry-skydio

jerry-skydio commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator Author
# head base diff date summary
0 4cbf01c8 cda06a4a diff Sep 23 15:50 PM 4 files changed, 238 insertions(+), 10 deletions(-)
1 ff727d74 c7a2fae5 diff Sep 25 16:55 PM 1 file changed, 4 deletions(-)
2 4ba02662 c7a2fae5 diff Sep 25 16:58 PM 2 files changed, 14 insertions(+), 22 deletions(-)

@jerry-skydio
jerry-skydio force-pushed the jerry/revup/main/comments branch from 4cbf01c to ff727d7 Compare September 25, 2026 23:55
Github applies mutation fields in order and their effects survive its gateway
giving up, and a transient http failure carries no field results, so replaying
the request posts the comment a second time. A real 502 while uploading a 60 pr
stack duplicated the review graph and patchsets comments on 13 prs, and three
attempts left some prs with three copies each.

Fields now carry a replay_safe flag, and addComment is the only unsafe one:
github allows unlimited identical comments, and echoes clientMutationId rather
than treating it as an idempotency key. Every other mutation is addressed by an
id or is a create that github rejects as UNPROCESSABLE the second time, which
already reads as done.

A request holding an unsafe field is sent once. If that fails transiently, read
back the newest comments of each subject pr and resend only the fields whose
body isn't there yet, within the usual retry budget.

Limitations:
- The check is a read, so a comment that did land can still be duplicated if
  github serves a stale replica, or if 20 newer comments push it out of the
  window being read.
- A comment the check can't prove missing (the pr node came back null, or the
  lookup itself failed) counts as added, leaving it for the next upload to add.
- Exhausting the retry budget raises, so the rest of that mutation and the rest
  of the upload don't run.
@jerry-skydio
jerry-skydio force-pushed the jerry/revup/main/comments branch from ff727d7 to 4ba0266 Compare September 25, 2026 23:58
@jerry-skydio
jerry-skydio merged commit 137d009 into main Sep 25, 2026
6 checks passed
@jerry-skydio
jerry-skydio deleted the jerry/revup/main/comments branch September 25, 2026 23:59
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.

3 participants