Labels
No labels
priority/could
priority/must
priority/should
priority/wont
status/blocked
status/claimed
status/done-migrated
type/bug
type/feature
type/infra
type/tech-debt
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
robert/todo#63
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Tech Debt: Template Seeding (
#24) Is Not Atomic and Costs N Sequential Round TripsReported by: Backend Engineer, spotted during
#24's code review, 2026-07-22Symptom
CreateTodoListCommandHandler(added in#24, List Templates) commits the new list + ownermembership via
SaveChangesAsync, then loops over the chosen template's seed titles calling thefull
CreateTodoCommandhandler once per title:Two compounding issues, both accepted as an intentional trade-off in
#24's design note (to avoid anuntested interaction between batch
INSERTs and thetrg_set_todo_nr_per_listPostgres trigger),but not fully worked through:
CreateTodoCommanddoes its own separate save. If iteration k fails (transient DB error, afuture catalog title that fails
TodoTitlevalidation, cancellation), the list is leftpersisted with a partial set of seeded todos while the whole
CreateTodoListCommandthrows backto the client as a failure. The user sees an error for a list that actually exists with partial
contents, and may retry, creating a duplicate list.
AuthorizeTodoListAccessForCurrentUserQuery+AuthorizeTodoListIsNotArchivedQueryDB checks(both already guaranteed by the owner-membership row just committed), its own insert +
GetTodoQueryre-fetch, its own WS broadcast, and its ownCreateActivityEventCommand. A 6-itemtemplate (Groceries) costs roughly 25-30 DB round trips and fans out 6 rapid WS/activity events
for one user action. This is fine at today's template sizes but scales linearly — a future larger
template, or a future "duplicate list"/"import" feature reusing this same handler, would turn
list creation into a multi-second operation with a jarring flood of client-visible updates.
Expected behaviour
Either:
dbContext.AddRange(todoEntities); SaveChangesAsync()inside thesame transaction as the list + membership insert) verified against
trg_set_todo_nr_per_list— PostgresBEFORE INSERT ROWtriggers on a multi-row statement seeeach prior sibling row within the same statement (command counter increments per row), so this is
likely already safe but was never actually tested, only assumed risky and avoided; or
hand-typed todos, at minimum wrap the whole
CreateTodoListCommandhandler body in one DBtransaction so a mid-loop failure rolls back the list + membership + any partially-seeded todos
instead of leaving an inconsistent partial list.
Acceptance criteria
CreateTodoCommandcall) leaves no persisted list/membership/todos — full rollback, not partial state
trg_set_todo_nr_per_list— the exact gap#24's design note flagged as untestedtodo creation
for larger templates (either by batching or by explicitly justifying why the per-item cost is
acceptable at a stated max template size)
Notes for whoever picks this up
Not urgent — current templates cap at 6 items and this is a "Could" priority feature — but flag it
before templates grow or before a "duplicate list"/"import" story reuses this handler's pattern.
Agents involved
Blockers
None.
design (
63_template_seeding_atomicity_and_batching_design.md)#63— Design NoteDecision: transactional wrapping, not batch insert
The story offered two options: a tested batch-insert path, or wrapping the whole handler in one
transaction while keeping the per-item pipeline for parity. Chose the latter — and it turns out to
be a near-zero-cost change:
CqsTodo/DbContext/DbTransactionDecorator.csandCqsTodo/DbContext/DbContextFactory.cs(which doubles asIDbTransaction) already implementexactly this mechanism, but nothing in the codebase actually opts a handler into it yet — it was
built and never wired up.
DbContextFactoryis registered.AddScoped, so onceIDbTransaction.Start()opens a connection+transaction, every subsequent.CreateContext()callwithin that same DI scope (including the ones
CreateTodoCommandHandlermakes internally, since itresolves the same scoped
IDbContextFactory) transparently shares that connection/transaction.Applied via the existing
SetupHandlerAttribute(params Type[] decoratorTypes)constructor —[SetupHandler(typeof(DbTransactionDecorator<,>))]onCreateTodoListCommandHandler— no newinfrastructure needed, just opting this one handler in. Confirmed the open-generic decorator type
gets closed against the specific handler's
IHandler<TReq,TRes>at registration time(
ServiceCollectionExtensions.Decorate'sdecoratorType.MakeGenericType(...)branch) — the samepattern
AuthorizationAndValidationDecorator<,,>already uses for authorization/validationdecorators, just via the attribute shorthand instead of manual
.WithDecorator(...)calls.Why this resolves AC
#2differently than expectedThe story's AC
#2("prove the chosen approach is safe againsttrg_set_todo_nr_per_list") waswritten assuming a batch-insert approach, where the concern is real (a multi-row
AddRange+one
SaveChangesAsyncneeds the trigger to see each row's siblings via the command counter withinone statement). Transactional wrapping keeps the existing sequential per-item
INSERTs — eachstill its own statement, each still able to see prior siblings via the transaction's own visibility
(same as today, transaction or not) — so the specific batch-insert-vs-trigger concern doesn't
apply. Verified via a new test that a multi-item template still gets correct sequential
Nr/SortOrdervalues under the transaction wrapper.What this doesn't fix
Round-trip count is unchanged — each seed title still re-runs the full single-todo pipeline (its
own authorization/validation queries, its own re-fetch, its own WS broadcast, its own activity
event). Per AC
#4's "either... or explicitly justify," justified as acceptable at today's statedmax template size (6 items,
Groceries) rather than batching: wrapping every per-itemCreateContext()call in one already-open connection/transaction actually removes the connection-open overhead
each call previously paid, so latency at 6 items is a modest improvement over the pre-
#63baseline,not a regression — just not the larger structural fix (batch insert) that would be warranted if
template sizes or a future "duplicate list" feature pushed item counts materially higher.
A second, more severe bug caught by code review (authorization silently dropped)
After fixing the open-generic crash above,
/code-reviewfound something worse in the fixedversion:
SetupHandlerAttribute(params Type[] decoratorTypes)routes throughServiceCollectionExtensions.SetupHandler(Type, Action<IHandlerRegistrationConfig>), which onlyruns the decorator-adding lambda — it never calls the handler's own static
ConfigureDi. The bare[SetupHandler]attribute instead goes through the other overload, which explicitly invokesConfigureDifor any handler implementingIHaveDiConfig<T>.CreateTodoListCommandHandler's ownConfigureDiis what callsWithAuthorization(AuthorizeIsCurrentUserAuthenticatedQuery)—switching the attribute to its parameterized form silently dropped that call, meaning
unauthenticated requests no longer got a clean rejection; they'd proceed into
Handle, getnullback from
getCurrentUserIdHandler, and crash onuserId!.Valuewith a bareInvalidOperationExceptioninstead ofAuthorizeIsCurrentUserAuthenticatedQuery's intendedUnauthorizedAccessException. This is exactly the kind of gapdotnet buildcannot catch (it's aregistration-time behavior difference between two attribute constructor overloads, not a type
error) and the earlier live-startup check didn't happen to exercise either, since it only checked
that registration didn't throw, not that the right decorators ended up applied.
Fixed by reverting to bare
[SetupHandler]and adding the transaction decorator insideConfigureDiitself instead:config.WithDecorator(typeof(DbTransactionDecorator<CreateTodoListCommand, TodoListDto>)).WithAuthorization(...)— decorator order matters here too: applying thetransaction decorator before
WithAuthorizationmakes authorization the outermost wrapper, soan unauthenticated request is rejected before a DB connection/transaction is ever opened, not
after. Re-verified live against the real running app: an unauthenticated
POST /api/CreateTodoListCommandnow correctly returns "Not logged in" (403), where the brokenversion would have returned the bare
InvalidOperationExceptionmessage instead.A real bug caught by live-verifying, not just
dotnet buildSetupHandlerAttribute(params Type[] decoratorTypes)'sWithDecoratorvalidates the decoratortype (
_interfaceType.IsAssignableFrom(decorator)) before closing an open generic against thehandler's own request/result types — passing
typeof(DbTransactionDecorator<,>)(open, matchinghow
AuthorizeAndValidationDecorator<,,>looks like it's used at first glance) fails that check,since an open generic type is never assignable to a closed interface. This is invisible to
dotnet build—AddFromSetupAttributesruns reflectively at real app startup, not at compiletime — so the first attempt looked fine until the app was actually run: starting the built DLL
against the sandbox's reachable
db/redisnetwork (ConnectionStrings__TodoDB/__redispointed at
db/redis, per the established "live-verify without Docker" technique) crashedimmediately with
Failed to apply decorators for handler CreateTodoListCommandHandler: DbTransactionDecorator<, > does not implement interface IHandler<CreateTodoListCommand, TodoListDto>. Fixed by passing the already-closed type instead —typeof(DbTransactionDecorator<CreateTodoListCommand, TodoListDto>), which is known at theattribute-declaration site since the handler already names its own request/result types — and
confirmed the fixed version starts cleanly (health check returns 200) where the open-generic
version did not start at all.
A narrow, accepted trade-off: WS broadcast now fires slightly before commit
Traced every consumer of the two Rx streams this handler feeds.
membershipSubject's onlyconsumer (
UserScopedTodoListChangePublisher'smembershipChangessubscription) just adds thelist id to an in-memory
HashSet(an access-control filter for that WS connection) — it neverreads the DB, so it's unaffected by the row not being durably committed yet.
changeSubject.Created(...)(the list's own "created" broadcast) is different: it already firedat the same point in the handler before this change (after the whole seeding loop), but before
#63every prior write in the handler had already autocommitted by the time it fired — there was noin-flight transaction. After
#63, that same broadcast now fires while the outer transaction isstill open, with
DbTransactionDecorator.Commit()running immediately afterward (the very nextline once
Handle()returns, no otherawaitin between on the server side). A client thatreacts to the WS "list created" event by immediately fetching that list's todos over a fresh HTTP
request (a separate DB connection, which can't see uncommitted rows) could, in a vanishingly
narrow window, see the list but not yet its seeded todos. Accepted rather than building a
post-commit-hook mechanism into
DbTransactionDecoratorfor this one caller: the window requiresa full client round-trip (receive the WS push, issue a new fetch) to complete strictly faster than
the server's own single, un-awaited-elsewhere
Commit()call, which in practice will not happenover real network latency; worst case is a client-visible fetch that's momentarily one write
behind, self-correcting on the next poll/refresh with no data corruption or persistent
inconsistency.
What this fixes
A mid-seed failure (a template item's
CreateTodoCommandthrowing partway through, or anydownstream side effect it triggers — activity event, notification) now rolls back the list,
membership row, and every already-"saved" seed todo in the same request, instead of leaving a
persisted list with partial contents that a client-visible error tells the user failed outright
(risking a retry that creates a duplicate list). ## Test coverage boundary (honest limitation)
DbTestContext<T>(this codebase's existing unit-test harness for handlers) instantiates the rawhandler class directly and never goes through
AddFromSetupAttributes/DI — so it cannot exercisewhether a decorator is actually applied, for this handler or any of the existing auth/validation
decorators either (a pre-existing gap in test methodology, not new to this ticket). No test in
CreateTodoListCommandHandlerTests.cswas added to simulate a mid-seed failure and assert fullrollback, because doing so meaningfully (proving the real transaction rolls back real rows)
requires resolving the handler through the actual DI container against a real Postgres, which this
test harness doesn't support today and building that harness is out of scope for this ticket.
Verification instead relied on: the existing, already-passing
DbTransactionDecoratorTests.cs(proves the general Start/Commit/Rollback contract against mocks), a real app-startup check (see
above — proves this handler's specific decorator registration resolves correctly, which is where
the actual bug was), and Postgres's own transaction guarantee (once
Rollback()issues a realROLLBACK, atomicity is the database's guarantee, not something this ticket needs to separatelyprove).
CreateTodoListCommandHandlerTests.cs's existing Moq-based tests confirm the seedingbusiness logic itself (which titles, blank template) is unaffected by the attribute change.
Attempted to also live-verify the actual seeding path (create a real user, log in, create a
templated list, confirm sequential
Nr/SortOrdervalues) against the running app, butCreateUserCommandreturned an unrelated 500 ("Object reference not set to an instance of anobject") that reproduces identically on the pre-
#63build too — confirmed not caused by thischange, but not chased further since it's outside this ticket's scope. The
Nr/trigger-visibilityquestion this ticket's AC#2 originally worried about doesn't depend on new mechanics anyway (see
above: the per-item sequential inserts are unchanged, and Postgres triggers already see prior
writes within the same open transaction/session regardless of whether that transaction spans one
statement or several) — so this is a reasoning-based, not empirically re-verified, closure of that
specific AC.