[Backend][Safety] Route bulk card writers through the archive-race guard - #2134
Conversation
External import, starter-pack apply, and archive-item restore all checked Board.IsArchived and then wrote cards without joining the Board concurrency-token predicate introduced in #2110, so an archive committing after the check was silently accepted. Each writer now calls Board.RecordCardMutation() before its SaveChanges, which touches the board without advancing its token. EF issues a conditional UPDATE against the token read before the archived-state check, so a racing archive turns the write into DomainException(Conflict) -> ErrorCodes.Conflict -> 409 instead of seeding an archived board. The token is deliberately not advanced, so independent writers on the same board still do not invalidate each other.
One race test and one independence test per writer. The race is forced without timing: the archive runs to completion on a second connection from inside a callback the writer itself invokes after its archived-state check and before its SaveChanges - the adapter's Parse for import, manifest validation for the starter pack, and the destination-column read for restore. Mutation-checked: with the three RecordCardMutation() calls reverted, exactly the three race tests fail, each reporting IsSuccess=True - the silent acceptance this issue describes - while the three independence tests still pass, so they are not merely mirroring the guard. The restore tests seed a real user because AuditLog.UserId is a genuine foreign key; an invented actor id fails the insert on a constraint and would mask the concurrency outcome.
ADR-0063 described the non-advancing conditional board update as a CardService property. The three bulk card writers now share it, so the decision record says so rather than leaving a future reader to conclude those paths are still a bare IsArchived check.
|
Review gate record: one fresh-context adversarial review completed at head f19fb1c — zero CRITICAL, zero HIGH. Verified clean: tracked-read dependency at all four guard sites (GetByIdWithDetailsAsync has no AsNoTracking, no QueryTrackingBehavior override, and the restore planner/executor share one identity-map instance); single-SaveChanges atomicity per writer (rejected restore stays RestoreStatus.Available); an exhaustive card-writer census found no missed writer (BoardJsonExportImportService always constructs a new Board; proposal ops route through the guarded CardService); independence semantics genuinely covered (TouchAndAdvance would fail the 3 independence tests); 409 mapping verified per entry point with the concurrency catch ordered before the transient-retry arm. Dispositions of the three non-blocking observations:
The fix push is docs-only (ADR text), so no fresh review round is owed; CI re-proves at the new head. |
… comments and ADR line, add the Activity selector to the recency consequence, right-size a test name
Summary
Post-merge follow-up from #2110. External import, starter-pack apply, and archive-item restore each
checked
Board.IsArchivedand then wrote cards without joining the Board concurrency-tokenpredicate, so an archive committing after the check was accepted silently.
Each writer now calls
Board.RecordCardMutation()before itsSaveChanges. That touches the boardwithout advancing its token, so EF issues a conditional
UPDATE Boards ... WHERE ConcurrencyToken = ?against the token read before the archived-state check. A racing archive advanced that token, the
update matches zero rows, and
UnitOfWork.SaveChangesAsyncmaps the failure toDomainException(ErrorCodes.Conflict)→409. This is exactly the mechanism #2110 established forsingle-card writes, applied verbatim — no new domain API, no migration, no schema change.
Race scenario per writer
ExternalImportService.ImportToBoardAsyncboard.IsArchivedafterGetByIdWithDetailsAsyncConflict, transaction rolled back, no imported card survivesStarterPackApplyService.ApplyToBoardAsyncboard.IsArchivedafterGetByIdWithDetailsAsyncConflict, rollback drops the pack's labels, columns, and seed cards togetherArchiveRecoveryService.RestoreArchiveItemAsyncRestorePlanner's "cannot restore to an archived board"Conflict; because the card insert, the stale board update, andArchiveItem.MarkAsRestoredshare oneSaveChanges, a rejected restore also leaves the archive item still restorable rather than consumedAll three services already had a
catch (DomainException)that returnsResult.Failure(ex.ErrorCode, ...),and all three controllers already route failures through
ToErrorActionResult(), whoseResultExtensionsmap sendsErrorCodes.Conflictto409. Verified end to end — no controller ormapping change was needed.
Independent writer semantics preserved
The token is deliberately not advanced by any bulk writer, matching #2110's boundary. Two writers
that read the same board still both succeed; the change closes the archive race without serializing
unrelated writes. Each writer has an explicit regression for this, including one pairing a
starter-pack apply with an external import on the same board.
Scope note
RestoreExecutor.RestoreColumnAsynctakes the same guard asRestoreCardAsync.RestorePlanner'sarchived-target check governs every non-board restore, so guarding only the card half would have
left the identical check-then-act window open on the same predicate, in the same method pair.
Tests added
backend/tests/Taskdeck.Api.Tests/ArchivedBoardBulkWriterConcurrencyTests.cs— 6 tests, one race andone independence regression per writer. The race is forced deterministically, not by timing: the
archive runs to completion on a second connection from inside a callback the writer itself invokes
after its archived check and before its
SaveChanges(the adapter'sParse, manifest validation,and the destination-column read respectively). Real EF Core, real migrations, real SQLite file; the
only mocked behavior is the one translation the production
UnitOfWorkperforms.Mutation-checked. With the three
RecordCardMutation()calls reverted tomain, exactly thethree race tests fail — each reporting
IsSuccess=True, the silent acceptance this issue describes —while the three independence tests still pass, confirming they are not merely mirroring the guard.
Verification
dotnet test backend/tests/Taskdeck.Application.Tests/Taskdeck.Application.Tests.csproj -c Release -m:1— 3875 passed, 0 faileddotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1— 2479 passed, 0 failed, 4 skipped (6 m 47 s)node scripts/check-docs-governance.mjs— passedgit diff --check— cleanNot run: frontend, E2E, and Infrastructure/Domain/Architecture suites — the change is Application-layer
only, adds no dependency, and touches no schema or migration.
Documentation
ADR-0063 described the non-advancing conditional board update as a
CardServiceproperty. Amended sothe decision record states the bulk writers now share it, rather than leaving a reader to conclude
those paths are still a bare
IsArchivedcheck.docs/STATUS.mdand the masterplan are unchanged:shipped reality does not move until this merges.
Tracking
Closes #2114
Refs #2110.