Skip to content

docs(skill): record why a retried merge-request PUT cannot double-apply (#616 follow-up) - #617

Merged
padak merged 2 commits into
mainfrom
claude/confident-wright-96d25a
Aug 20, 2026
Merged

docs(skill): record why a retried merge-request PUT cannot double-apply (#616 follow-up)#617
padak merged 2 commits into
mainfrom
claude/confident-wright-96d25a

Conversation

@padak

@padak padak commented Aug 20, 2026

Copy link
Copy Markdown
Member

Follow-up to #616 (issue #599). Documentation only — no code change, and none is needed.

The question

The reviewer on #616 flagged, non-blocking and outside that diff, that
RETRY_SAFE_METHODS treats every PUT as retry-safe while
client/merge_requests.py uses PUT for four action-style transitions:
/request-review, /approve, /request-changes, /merge. PUT is formally
idempotent, so the rule follows RFC 9110 — but these endpoints read as commands,
not replacements. A 500 raised after the transition committed would be
retried, potentially firing the transition and its notifications twice.

Worth noting this was never a regression from #616: before it, every method was
retried on a 5xx, so these were already exposed. #616 simply did not narrow them.

The answer: the server refuses the second call, structurally

Verified against the keboola/connection source. The MR lifecycle is a Symfony
Workflow state_machine (MergeRequestLifecycleStateMachine), and such a
transition is enabled only from its declared from places:

Retried call State after attempt 1 What attempt 2 gets
/request-review in_review (or approved via skip_review) enabled only from development422
/request-changes development enabled only from in_review / approved422
/merge in_merge MergeProcessor checks can(MERGE) (only from approved) behind a per-project MySQL table lock → 409 storage.mergeRequests.notReadyToMerge
/approve in_review (self-loop) or approved AddApprovalGuard: "This reviewer has already approved this request." → 422

approve is the only self-loop (in_review -> in_review), which is exactly why
it carries a dedicated guard rather than relying on the place structure.

Notifications cannot double-fire either: emails, audit-log entries and storage
events are emitted by TransitionListener on
workflow.merge_request_lifecycle.completed — from inside apply(), inside
MergeRequestService's transactional(). The side effects are welded to the
transition.

Two things the audit did surface

Both are recorded in the same gotcha:

  1. A retried PUT reports attempt 2's error. The retry loop surfaces the
    last response, so an operation that succeeded on attempt 1 and merely lost
    its response (500 after commit, read timeout) is reported as 422/409.
    That is a reporting hazard, not a state one, and it applies to every retried
    PUT/DELETE in kbagent — not only to merge requests. The guidance is to
    read a 4xx state conflict as "check the MR, this may already be done".

  2. A server-side race in approve. bi_rMergeRequestsApprovals has no
    unique constraint on (mergeRequestId, idAdmin), and
    hasEnoughApprovals() counts rows rather than distinct admins — so two
    overlapping approvals from one admin could satisfy a two-approval
    requirement. Filed as keboola/connection#8209 with the schema and source
    evidence; the duplicate row was not reproduced, and that caveat is stated in
    the issue.

What was deliberately not done

No per-call-site retry_safe=True/False override. That was considered and
rejected when #616 was designed — one auditable method rule beats an opt-out
anyone can flip on the wrong endpoint — and nothing here changes that
trade-off. A client-side workaround would not close the UI double-submit path
for (2) anyway.

Verification

Source-based, against keboola/connection via gh api. A live E2E was not
possible and would not have been conclusive: no reachable project carries both
protected-default-branch and branches-merge-requests, approve requires a
second admin (AddApprovalGuard forbids self-approval), and a 500 raised after
the transaction commits cannot be induced from the client side.

ruff check and ruff format --check clean. The change touches one Markdown
file, so no test surface is affected.


Open in Devin Review

…ly (#616 follow-up)

The reviewer on #616 flagged that RETRY_SAFE_METHODS treats every PUT as
retry-safe, while client/merge_requests.py uses PUT for four action-style
transitions (/request-review, /approve, /request-changes, /merge). The worry
was that a 5xx raised after the transition committed would be retried and fire
it -- and its notifications -- twice.

Verified against the keboola/connection source: it cannot. The MR lifecycle is
a Symfony Workflow state_machine, so three of the four transitions are refused
structurally on a second call (enabled only from their declared `from` place);
/approve is the one self-loop and carries AddApprovalGuard instead.
Notifications ride workflow.merge_request_lifecycle.completed from inside
apply(), inside MergeRequestService's transactional() -- no transition, no
notification.

No code change. Two things the audit did surface are recorded with it:

- A retried PUT reports attempt 2's error, so an operation that succeeded and
  merely lost its response surfaces as 422/409. That applies to every retried
  PUT/DELETE, not just merge requests.
- bi_rMergeRequestsApprovals has no unique constraint on
  (mergeRequestId, idAdmin) and hasEnoughApprovals() counts rows rather than
  distinct admins. Server-side and narrow; filed as keboola/connection#8209.
  Deliberately not worked around here -- the blanket method rule stays, with
  no per-call-site retry opt-out.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Open in Devin Review

Comment thread plugins/kbagent/skills/kbagent/references/gotchas.md
… upstream issue

The gotcha stated the mechanism conditionally but never said the duplicate row
was not reproduced and the isolation level was not checked -- a caveat the
upstream issue does carry, so the two documents disagreed on how firm the
finding is. It also had no pointer to keboola/connection#8209, leaving a future
reader no handle to re-check it or notice it was fixed.
@padak
padak merged commit d5342db into main Aug 20, 2026
4 checks passed
@padak
padak deleted the claude/confident-wright-96d25a branch August 20, 2026 03:40
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