Bulk operations aren't actually atomic despite the docblock claiming so #33

New Issue

2026-08-31T21:29:42-04:00

jared commented

2026-08-31 21:29:42 -04:00

Severity: Medium

models/BulkOperationsModel.php::processBulkOperation() (~line 99 docblock) states the transaction "ensures atomicity — either all tickets are updated or none are (rolled back on failure)" — but this only happens when $atomic = true is passed. The only real caller, api/bulk_operation.php (~line 106), never passes it, so it defaults to false.

Impact: In production, a bulk operation with partial per-ticket failures (e.g. 8 succeed, 2 hit a disallowed workflow transition) commits the 8 successes anyway — actual behavior is best-effort, not atomic, contradicting the documented contract. Admins performing a bulk action may believe it's all-or-nothing when it isn't.

Fix: Either pass $atomic = true from api/bulk_operation.php if all-or-nothing is the desired UX, or fix the docblock to accurately describe the default best-effort behavior and surface partial-failure results clearly to the admin.

**Severity:** Medium `models/BulkOperationsModel.php::processBulkOperation()` (~line 99 docblock) states the transaction "ensures atomicity — either all tickets are updated or none are (rolled back on failure)" — but this only happens when `$atomic = true` is passed. The only real caller, `api/bulk_operation.php` (~line 106), never passes it, so it defaults to `false`. **Impact:** In production, a bulk operation with partial per-ticket failures (e.g. 8 succeed, 2 hit a disallowed workflow transition) commits the 8 successes anyway — actual behavior is best-effort, not atomic, contradicting the documented contract. Admins performing a bulk action may believe it's all-or-nothing when it isn't. **Fix:** Either pass `$atomic = true` from `api/bulk_operation.php` if all-or-nothing is the desired UX, or fix the docblock to accurately describe the default best-effort behavior and surface partial-failure results clearly to the admin.

jared added the priority/medium reliability labels 2026-09-08 10:15:43 -04:00

jared referenced this issue from a commit

2026-09-11 21:56:34 -04:00

Fix atomicity docblock and surface per-ticket bulk-op errors (#33)

jared referenced this issue from a commit

2026-09-11 21:58:46 -04:00

Merge development into main: bulk-op atomicity docs + double-submit guard (#33, #36)

jared commented

2026-09-11 21:58:59 -04:00

Fixed in commit 844677b (merged to main in 4aa83ff).

processBulkOperation()'s docblock claimed the transaction 'ensures atomicity - either all tickets are updated or none are,' but that's only true when $atomic = true is passed, and the only real caller (api/bulk_operation.php) never passes it — the actual default is best-effort: per-ticket failures are skipped and recorded, and every other ticket in the batch still commits. Reworded the docblock to describe the actual default behavior.

Also found and fixed a related gap while surfacing this properly: the model already collected per-ticket failure reasons into $result['errors'] (assets/js/dashboard.js's bulkResultMessage() already reads data.errors to render them), but api/bulk_operation.php's success response dropped that field entirely, so admins only ever saw a bare 'N succeeded, M failed' count with no way to see which tickets failed or why. Added 'errors' to the response when present.

Verified against real MariaDB: a bulk_status operation against a Closed ticket (no transition defined) and an Open ticket (Open->Pending defined) correctly processed 1/1, and the API response now includes errors: ["Ticket ...: transition not allowed (Closed -> Pending)"].

Fixed in commit 844677b (merged to main in 4aa83ff). processBulkOperation()'s docblock claimed the transaction 'ensures atomicity - either all tickets are updated or none are,' but that's only true when $atomic = true is passed, and the only real caller (api/bulk_operation.php) never passes it — the actual default is best-effort: per-ticket failures are skipped and recorded, and every other ticket in the batch still commits. Reworded the docblock to describe the actual default behavior. Also found and fixed a related gap while surfacing this properly: the model already collected per-ticket failure reasons into $result['errors'] (assets/js/dashboard.js's bulkResultMessage() already reads data.errors to render them), but api/bulk_operation.php's success response dropped that field entirely, so admins only ever saw a bare 'N succeeded, M failed' count with no way to see which tickets failed or why. Added 'errors' to the response when present. Verified against real MariaDB: a bulk_status operation against a Closed ticket (no transition defined) and an Open ticket (Open->Pending defined) correctly processed 1/1, and the API response now includes errors: ["Ticket ...: transition not allowed (Closed -> Pending)"].

jared closed this issue

2026-09-11 21:58:59 -04:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: LotusGuild/tinker_tickets#33