From d3ff2658ef6af573083cfb2747807f8128b99fed Mon Sep 17 00:00:00 2001 From: Petr Date: Wed, 19 Aug 2026 23:26:56 -0400 Subject: [PATCH 1/2] docs(skill): record why a retried merge-request PUT cannot double-apply (#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. --- .../skills/kbagent/references/gotchas.md | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 8982b2c7..ec132044 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -3849,6 +3849,64 @@ Two consequences for an agent reading an error: `exceptionId` when escalating to Keboola support -- it is the only handle that traces back to the actual server-side exception. +## Retrying a merge-request `PUT` cannot double-apply the transition -- but it can mask a success as a 4xx (since v0.86.0) + +`RETRY_SAFE_METHODS` treats every `PUT` as repeatable on a 5xx, and +`client/merge_requests.py` uses `PUT` for four action-style transitions +(`/request-review`, `/approve`, `/request-changes`, `/merge`). Those read as +commands rather than replacements, so the obvious worry is that a 500 raised +*after* the transition already committed would be retried and fire the +transition -- and its notifications -- a second time. Verified against the +`keboola/connection` source: it cannot. + +**The second call is refused structurally, not incidentally.** The MR +lifecycle is a Symfony Workflow `state_machine` +(`MergeRequestLifecycleStateMachine`), and a `state_machine` 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`) | `request_review` is enabled only from `development` -> **422** | +| `/request-changes` | `development` | enabled only from `in_review` / `approved` -> **422** | +| `/merge` | `in_merge` | `MergeProcessor` checks `can(MERGE)` (enabled only from `approved`) behind a per-project MySQL table lock -> **409** `storage.mergeRequests.notReadyToMerge` | +| `/approve` | `in_review` (self-loop) or `approved` | `AddApprovalGuard` blocks: "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 instead of 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` -- that is, from inside +`apply()`, inside `MergeRequestService`'s `transactional()`. The side effects +are welded to the transition: no transition, no notification. + +**What the retry does cost you: the error you see is attempt 2's.** The retry +loop surfaces the last response, so an operation that *succeeded* on attempt 1 +and merely lost its response (a 500 after commit, or a read timeout) is +reported as `422 Cannot approve in current state` or `409 not ready to merge`. +Read a 4xx state conflict on a merge-request transition as "check the MR, this +may already be done", never as "it failed": fetch the MR and read its `state` +before repeating anything. The same reasoning applies to every retried +`PUT`/`DELETE` in kbagent, not only to merge requests. + +**One residual race, server-side and not kbagent's to fix.** +`AddApprovalGuard` reads the approvals list, and +`MergeRequestService::approve()` inserts the row later in the same +transaction; `bi_rMergeRequestsApprovals` carries indexes but no unique +constraint on `(mergeRequestId, idAdmin)`. Two *overlapping* approve requests +from the same admin could therefore both clear the guard -- and +`hasEnoughApprovals()` counts rows, not distinct admins, so a project +requiring two approvals could reach `approved` on one human. kbagent can only +produce that overlap through a read-timeout retry (30 s read timeout, 1 s +backoff) racing a much shorter transaction, so the window is very narrow. +Nothing was changed in kbagent for it, and there is deliberately no +per-call-site retry opt-out: one auditable method rule beats a flag anyone can +flip on the wrong endpoint. + +Note that the merge-request namespace is Layer 3 only today -- no +`services/` entry and no CLI command group -- so it is reachable from the SDK +client, not from a `kbagent ` command. + ## `token list` is the only way to see what exists -- and it never shows secrets (since v0.86.0) `kbagent token list --project P` (`GET /v2/storage/tokens`) closes the gap where From ff0aaa86a8c08287e8065c225f612c2dc601c039 Mon Sep 17 00:00:00 2001 From: Petr Date: Wed, 19 Aug 2026 23:35:03 -0400 Subject: [PATCH 2/2] docs(skill): mark the approvals race as reported-not-proven, link the 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. --- plugins/kbagent/skills/kbagent/references/gotchas.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index ec132044..a050ed0a 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -3899,6 +3899,12 @@ from the same admin could therefore both clear the guard -- and requiring two approvals could reach `approved` on one human. kbagent can only produce that overlap through a read-timeout retry (30 s read timeout, 1 s backoff) racing a much shorter transaction, so the window is very narrow. +**Read this as reported, not proven:** the missing constraint and the +row-counting come off the schema and the source, but no duplicate row was +reproduced and the transaction isolation level was not checked -- it may be +latent rather than live. It is tracked upstream as +[keboola/connection#8209](https://github.com/keboola/connection/issues/8209); +check there before relying on any of it. Nothing was changed in kbagent for it, and there is deliberately no per-call-site retry opt-out: one auditable method rule beats a flag anyone can flip on the wrong endpoint.