Skip to content

Commit 81f547e

Browse files
ManukMinasyanManukMinasyan
committed
feat: implement automatic retry logic for position updates to handle conflicts
1 parent 85c6369 commit 81f547e

File tree

4 files changed

+204
-43
lines changed

4 files changed

+204
-43
lines changed

‎database/migrations/add_unique_position_constraint.php.stub‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<?php
2+
3+
use Illuminate\Database\Migrations\Migration;
4+
use Illuminate\Database\Schema\Blueprint;
5+
use Illuminate\Support\Facades\Schema;
6+
7+
return new class extends Migration
8+
{
9+
/**
10+
* Run the migrations.
11+
*
12+
* IMPORTANT: Before running this migration, ensure you have no duplicate
13+
* positions in your table by running:
14+
*
15+
* php artisan flowforge:repair-positions YourModel --column=status
16+
*/
17+
public function up(): void
18+
{
19+
Schema::table('your_table_name', function (Blueprint $table) {
20+
// Add unique constraint on (status_column, position_column) combination
21+
// This prevents duplicate positions within the same column/status
22+
$table->unique(['status_column', 'position_column'], 'unique_position_per_column');
23+
});
24+
}
25+
26+
/**
27+
* Reverse the migrations.
28+
*/
29+
public function down(): void
30+
{
31+
Schema::table('your_table_name', function (Blueprint $table) {
32+
$table->dropUnique('unique_position_per_column');
33+
});
34+
}
35+
};

‎src/Concerns/InteractsWithBoard.php‎

Lines changed: 156 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,12 @@
88
use Filament\Actions\ActionGroup;
99
use Illuminate\Database\Eloquent\Builder;
1010
use Illuminate\Database\Eloquent\Model;
11+
use Illuminate\Database\QueryException;
1112
use Illuminate\Support\Facades\DB;
13+
use Illuminate\Support\Facades\Log;
1214
use InvalidArgumentException;
1315
use Relaticle\Flowforge\Board;
16+
use Relaticle\Flowforge\Exceptions\MaxRetriesExceededException;
1417
use Relaticle\Flowforge\Services\Rank;
1518
use Throwable;
1619

@@ -95,27 +98,168 @@ public function moveCard(
9598
throw new InvalidArgumentException("Card not found: {$cardId}");
9699
}
97100

98-
// Calculate new position using Rank service
99-
$newPosition = $this->calculatePositionBetweenCards($afterCardId, $beforeCardId, $targetColumnId);
101+
// Calculate and update position with automatic retry on conflicts
102+
$newPosition = $this->calculateAndUpdatePositionWithRetry($card, $targetColumnId, $afterCardId, $beforeCardId);
100103

101-
// Use transaction for data consistency
102-
DB::transaction(function () use ($card, $board, $targetColumnId, $newPosition) {
104+
// Emit success event after successful transaction
105+
$this->dispatch('kanban-card-moved', [
106+
'cardId' => $cardId,
107+
'columnId' => $targetColumnId,
108+
'position' => $newPosition,
109+
]);
110+
}
111+
112+
/**
113+
* Calculate position and update card within transaction with pessimistic locking.
114+
* This prevents race conditions when multiple users drag cards simultaneously.
115+
*/
116+
protected function calculateAndUpdatePosition(
117+
Model $card,
118+
string $targetColumnId,
119+
?string $afterCardId,
120+
?string $beforeCardId
121+
): string {
122+
$newPosition = null;
123+
124+
DB::transaction(function () use ($card, $targetColumnId, $afterCardId, $beforeCardId, &$newPosition) {
125+
$board = $this->getBoard();
126+
$query = $board->getQuery();
127+
$positionField = $board->getPositionIdentifierAttribute();
128+
129+
// LOCK reference cards for reading to prevent stale data
130+
$afterCard = $afterCardId
131+
? (clone $query)->where('id', $afterCardId)->lockForUpdate()->first()
132+
: null;
133+
134+
$beforeCard = $beforeCardId
135+
? (clone $query)->where('id', $beforeCardId)->lockForUpdate()->first()
136+
: null;
137+
138+
// Calculate position INSIDE transaction with locked data
139+
$newPosition = $this->calculatePositionBetweenLockedCards(
140+
$afterCard,
141+
$beforeCard,
142+
$targetColumnId
143+
);
144+
145+
// Update card position
103146
$columnIdentifier = $board->getColumnIdentifierAttribute();
104147
$columnValue = $this->resolveStatusValue($card, $columnIdentifier, $targetColumnId);
105-
$positionIdentifier = $board->getPositionIdentifierAttribute();
106148

107149
$card->update([
108150
$columnIdentifier => $columnValue,
109-
$positionIdentifier => $newPosition,
151+
$positionField => $newPosition,
110152
]);
111153
});
112154

113-
// Emit success event after successful transaction
114-
$this->dispatch('kanban-card-moved', [
115-
'cardId' => $cardId,
116-
'columnId' => $targetColumnId,
117-
'position' => $newPosition,
118-
]);
155+
return $newPosition;
156+
}
157+
158+
/**
159+
* Calculate position between locked cards (used within transaction).
160+
*/
161+
protected function calculatePositionBetweenLockedCards(
162+
?Model $afterCard,
163+
?Model $beforeCard,
164+
string $columnId
165+
): string {
166+
if (! $afterCard && ! $beforeCard) {
167+
return $this->getBoardPositionInColumn($columnId, 'bottom');
168+
}
169+
170+
$positionField = $this->getBoard()->getPositionIdentifierAttribute();
171+
172+
$beforePos = $beforeCard?->getAttribute($positionField);
173+
$afterPos = $afterCard?->getAttribute($positionField);
174+
175+
if ($beforePos && $afterPos && is_string($beforePos) && is_string($afterPos)) {
176+
return Rank::betweenRanks(Rank::fromString($afterPos), Rank::fromString($beforePos))->get();
177+
}
178+
179+
if ($beforePos && is_string($beforePos)) {
180+
return Rank::before(Rank::fromString($beforePos))->get();
181+
}
182+
183+
if ($afterPos && is_string($afterPos)) {
184+
return Rank::after(Rank::fromString($afterPos))->get();
185+
}
186+
187+
return Rank::forEmptySequence()->get();
188+
}
189+
190+
/**
191+
* Calculate and update position with automatic retry on conflicts.
192+
* Wraps calculateAndUpdatePosition() with retry logic to handle rare duplicate position conflicts.
193+
*/
194+
protected function calculateAndUpdatePositionWithRetry(
195+
Model $card,
196+
string $targetColumnId,
197+
?string $afterCardId,
198+
?string $beforeCardId,
199+
int $maxAttempts = 3
200+
): string {
201+
$baseDelay = 50; // milliseconds
202+
$lastException = null;
203+
204+
for ($attempt = 1; $attempt <= $maxAttempts; $attempt++) {
205+
try {
206+
return $this->calculateAndUpdatePosition(
207+
$card,
208+
$targetColumnId,
209+
$afterCardId,
210+
$beforeCardId
211+
);
212+
} catch (QueryException $e) {
213+
// Check if this is a unique constraint violation
214+
if (! $this->isDuplicatePositionError($e)) {
215+
throw $e; // Not a duplicate, rethrow
216+
}
217+
218+
$lastException = $e;
219+
220+
// Log the conflict for monitoring
221+
Log::info('Position conflict detected, retrying', [
222+
'card_id' => $card->id,
223+
'target_column' => $targetColumnId,
224+
'attempt' => $attempt,
225+
'max_attempts' => $maxAttempts,
226+
]);
227+
228+
// Max retries reached?
229+
if ($attempt >= $maxAttempts) {
230+
throw new MaxRetriesExceededException(
231+
"Failed to move card after {$maxAttempts} attempts due to position conflicts",
232+
previous: $e
233+
);
234+
}
235+
236+
// Exponential backoff: 50ms, 100ms, 200ms
237+
$delay = $baseDelay * pow(2, $attempt - 1);
238+
usleep($delay * 1000);
239+
240+
// Refresh reference cards before retry (they may have moved)
241+
continue;
242+
}
243+
}
244+
245+
// Should never reach here
246+
throw $lastException ?? new \RuntimeException('Unexpected retry loop exit');
247+
}
248+
249+
/**
250+
* Check if a QueryException is due to unique constraint violation on positions.
251+
*/
252+
protected function isDuplicatePositionError(QueryException $e): bool
253+
{
254+
$errorCode = $e->errorInfo[1] ?? null;
255+
256+
// SQLite: SQLITE_CONSTRAINT (19)
257+
// MySQL: ER_DUP_ENTRY (1062)
258+
// PostgreSQL: unique_violation (23505)
259+
260+
return in_array($errorCode, [19, 1062, 23505]) ||
261+
str_contains($e->getMessage(), 'unique_position_per_column') ||
262+
str_contains($e->getMessage(), 'UNIQUE constraint failed');
119263
}
120264

121265
public function loadMoreItems(string $columnId, ?int $count = null): void

‎src/Exceptions/MaxRetriesExceededException.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Relaticle\Flowforge\Exceptions;
6+
7+
use RuntimeException;
8+
9+
class MaxRetriesExceededException extends RuntimeException
10+
{
11+
//
12+
}

‎tests/Feature/PositionInversionReproductionTest.php‎

Lines changed: 1 addition & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -9,37 +9,7 @@
99
$this->board = Livewire::test(TestBoard::class);
1010
});
1111

12-
/**
13-
* Helper to detect position inversions in a column
14-
*/
15-
function detectInversions(string $modelClass, string $columnValue, string $positionField = 'order_position'): array
16-
{
17-
$records = $modelClass::query()
18-
->where('status', $columnValue)
19-
->whereNotNull($positionField)
20-
->orderBy('id')
21-
->get();
22-
23-
$inversions = [];
24-
for ($i = 0; $i < $records->count() - 1; $i++) {
25-
$current = $records[$i];
26-
$next = $records[$i + 1];
27-
28-
$currentPos = $current->getAttribute($positionField);
29-
$nextPos = $next->getAttribute($positionField);
30-
31-
if (strcmp($currentPos, $nextPos) >= 0) {
32-
$inversions[] = [
33-
'current_id' => $current->id,
34-
'current_pos' => $currentPos,
35-
'next_id' => $next->id,
36-
'next_pos' => $nextPos,
37-
];
38-
}
39-
}
40-
41-
return $inversions;
42-
}
12+
// Note: detectInversions() helper function is defined in ParameterOrderMutationTest.php
4313

4414
describe('Real-World Position Inversion Scenarios', function () {
4515
it('reproduces inversions from rapid sequential moves between same positions', function () {

0 commit comments

Comments
 (0)