fix(server): take MAX+1 positions inside the transaction that inserts

Days, todo items, packing items and bags, packing template categories and
items, collections, collection labels and road-trip vias took their next
position as MAX(...) + 1 in one statement and inserted in the next, with
any other request free to run in between: two adds at once read the same
MAX and landed on the same position (the road-trip test pinned exactly
that duplicate). The read now sits in the transaction that inserts, and
the transaction holds the connection, so the second add waits and takes
the next position. A collection label's limit and name check move into
the same transaction, so two labels of one name added at once end in one
label and a 409.

Each site gets a test that adds several rows at once and reads back one
position each.

server/src/nest/collections/collections.service.ts

@@ -133,11 +133,8 @@ export class CollectionsService {
     @InjectRepository(CollectionMembers) private readonly members: CollectionMembersRepository,
     @InjectRepository(CollectionLabels) private readonly labels: CollectionLabelsRepository,
     @InjectRepository(Categories) private readonly categories: CategoriesRepository,
-    // Plan 3h Task 2 (part B) — savePlace onward. `collectionPlaces`/
-    // `collectionPlaceRatings` own this service's own two remaining tables;
-    // `trips`/`tripMembers`/`tripPlaces`/`tripPlaceRatings`/`tags`/`users`
-    // are 3b/3c's, injected directly (AP1-AP6 each become a
-    // `TripsRepository.findAccessible` call, replacing `this.db.canAccessTrip`).
+    // The saved places and their ratings are this service's own; the trip-side
+    // repositories below are injected directly for the copy-to-trip paths.
     @InjectRepository(CollectionPlaces) private readonly collectionPlaces: CollectionPlacesRepository,
     @InjectRepository(CollectionPlaceRatings) private readonly collectionPlaceRatings: CollectionPlaceRatingsRepository,
     @InjectRepository(Trips) private readonly trips: TripsRepository,
@@ -594,8 +591,9 @@ export class CollectionsService {
   }
 
   async createCollection(userId: number, body: CollectionCreateRequest): Promise<Collection> {
-    const max = await this.collectionsRepo.maxSortOrder(userId);
-    const id = await this.collectionsRepo.insertCollection({
+    // The MAX read and the insert in one transaction: two lists created at once
+    // cannot take the same position.
+    const id = await this.uow.transactional(async () => this.collectionsRepo.insertCollection({
       owner_id: userId,
       name: body.name,
       description: body.description ?? null,
@@ -603,8 +601,8 @@ export class CollectionsService {
       icon: body.icon ?? 'Bookmark',
       cover_image: body.cover_image ?? null,
       links: serializeLinks(body.links),
-      sort_order: max + 1,
-    });
+      sort_order: (await this.collectionsRepo.maxSortOrder(userId)) + 1,
+    }));
     const col = await this.getCollectionRow(id);
     return { ...col, is_owner: true };
   }
@@ -1286,13 +1284,15 @@ export class CollectionsService {
     await this.assertCanEdit(userId, collectionId);
     const trimmed = name.trim();
     if (!trimmed) httpError(400, 'Label name is required');
-    const count = await this.labels.countByCollection(collectionId);
-    if (count >= MAX_LABELS_PER_COLLECTION) httpError(400, `A list can have at most ${MAX_LABELS_PER_COLLECTION} labels`);
-    if (await this.labels.nameExists(collectionId, trimmed)) {
-      httpError(409, 'A label with this name already exists');
-    }
-    const nextSort = (await this.labels.maxSortOrder(collectionId)) + 1;
-    const newId = await this.labels.insertLabel({ collection_id: collectionId, name: trimmed, color: color ?? '#6366f1', sort_order: nextSort });
+    // The limit, the name check, the position and the insert in one transaction,
+    // so two labels added at once can neither both pass the checks nor share a position.
+    const newId = await this.uow.transactional(async () => {
+      const count = await this.labels.countByCollection(collectionId);
+      if (count >= MAX_LABELS_PER_COLLECTION) httpError(400, `A list can have at most ${MAX_LABELS_PER_COLLECTION} labels`);
+      if (await this.labels.nameExists(collectionId, trimmed)) httpError(409, 'A label with this name already exists');
+      const nextSort = (await this.labels.maxSortOrder(collectionId)) + 1;
+      return await this.labels.insertLabel({ collection_id: collectionId, name: trimmed, color: color ?? '#6366f1', sort_order: nextSort });
+    });
     await this.notifyCollectionUsers(collectionId, socketId, 'collections:updated');
     return this.getLabelById(newId);
   }

server/src/nest/days/days.service.ts

@@ -304,18 +304,12 @@ export class DaysService {
     // of defect `PlacesService.create` was fixed for (A-M1).
     const tripIdNum = toRowId(tripId);
     if (tripIdNum === null) throw new HttpException({ error: 'Trip not found' }, 404);
-    // DY5: `DaysRepository.maxDayNumber` already folds the no-rows case to 0
-    // (`?? 0`); the `|| 0` here is the legacy expression kept verbatim so a
-    // stored 0 still becomes 1 — the same outcome either fold produces, but
-    // the ruling is to keep the literal formula in the service.
-    const maxDayNumber = await this.daysRepo.maxDayNumber(tripIdNum);
-    const dayNumber = (maxDayNumber || 0) + 1;
-
-    const day = await this.daysRepo.createDay({
-      trip_id: tripIdNum,
-      day_number: dayNumber,
-      date: date || null,
-      notes: notes || null,
+    // DY5: the next day number and the row that takes it in one transaction, so
+    // two days added at once cannot both read the same MAX. `|| 0` keeps a stored
+    // 0 becoming 1, as the legacy formula did.
+    const day = await this.uow.transactional(async () => {
+      const dayNumber = ((await this.daysRepo.maxDayNumber(tripIdNum)) || 0) + 1;
+      return await this.daysRepo.createDay({ trip_id: tripIdNum, day_number: dayNumber, date: date || null, notes: notes || null });
     });
     return { ...day, assignments: [] };
   }

server/src/nest/packing/packing.service.ts

@@ -277,12 +277,13 @@ export class PackingService {
     ownerId?: number,
   ) {
     if (data.bag_id != null && !(await this.bagInTrip(tripId, data.bag_id))) return { invalidBag: true } as const;
-    const maxOrder = await this.itemsRepo.maxSortOrder(tripId);
-    const sortOrder = (maxOrder !== null ? maxOrder : -1) + 1;
     const qty = Math.max(1, Math.min(999, Number(data.quantity) || 1));
     const isPrivate = this.visibilityToPrivate(data.visibility, data.is_private);
 
+    // The MAX read and the insert share the transaction, so two items added at
+    // once cannot take the same position.
     const itemId = await this.uow.transactional(async () => {
+      const sortOrder = ((await this.itemsRepo.maxSortOrder(tripId)) ?? -1) + 1;
       const id = await this.itemsRepo.insertItem({
         trip_id: tripId,
         name: data.name,
@@ -490,12 +491,11 @@ export class PackingService {
   // ── Bulk Import ────────────────────────────────────────────────────────────
 
   async bulkImport(tripId: string | number, items: ImportItem[], ownerId?: number) {
-    const maxOrder = await this.itemsRepo.maxSortOrder(tripId);
-    let sortOrder = (maxOrder !== null ? maxOrder : -1) + 1;
-
     const created: any[] = [];
 
     await this.uow.transactional(async () => {
+      // Read inside the transaction, like createItem.
+      let sortOrder = ((await this.itemsRepo.maxSortOrder(tripId)) ?? -1) + 1;
       for (const item of items) {
         if (!item.name?.trim()) continue;
         const checked = item.checked ? 1 : 0;
@@ -615,13 +615,15 @@ export class PackingService {
   }
 
   async createBag(tripId: string | number, data: { name: string; color?: string; weight_limit_grams?: number | null }) {
-    const maxOrder = await this.bagsRepo.maxSortOrder(tripId);
-    const newId = await this.bagsRepo.insertBag({
-      trip_id: tripId,
-      name: data.name.trim(),
-      color: data.color || '#6366f1',
-      sort_order: (maxOrder ?? -1) + 1,
-      weight_limit_grams: data.weight_limit_grams ?? null,
+    const newId = await this.uow.transactional(async () => {
+      const maxOrder = await this.bagsRepo.maxSortOrder(tripId);
+      return await this.bagsRepo.insertBag({
+        trip_id: tripId,
+        name: data.name.trim(),
+        color: data.color || '#6366f1',
+        sort_order: (maxOrder ?? -1) + 1,
+        weight_limit_grams: data.weight_limit_grams ?? null,
+      });
     });
     return await this.bagsRepo.findById(newId);
   }
@@ -682,13 +684,13 @@ export class PackingService {
 
     if (templateItems.length === 0) return null;
 
-    const maxOrder = await this.itemsRepo.maxSortOrder(tripId);
-    let sortOrder = (maxOrder !== null ? maxOrder : -1) + 1;
     const isPrivate = ownerId != null ? this.visibilityToPrivate(visibility) : 0;
     const owner = isPrivate ? ownerId! : null;
 
     const added: any[] = [];
     await this.uow.transactional(async () => {
+      // Read inside the transaction, like createItem.
+      let sortOrder = ((await this.itemsRepo.maxSortOrder(tripId)) ?? -1) + 1;
       for (const ti of templateItems) {
         // Weight, count and bag ride along since #1131; a bag the trip lacks is
         // created, the way the import does it.
@@ -837,8 +839,10 @@ export class PackingService {
     if (!name?.trim()) return { error: 'Category name is required', status: 400 };
     const template = await this.templatesRepo.findById(templateId);
     if (!template) return { error: 'Template not found', status: 404 };
-    const maxOrder = await this.templateCategoriesRepo.maxSortOrder(templateId);
-    const newId = await this.templateCategoriesRepo.insertCategory(templateId, name.trim(), (maxOrder ?? -1) + 1);
+    const newId = await this.uow.transactional(async () => {
+      const maxOrder = await this.templateCategoriesRepo.maxSortOrder(templateId);
+      return await this.templateCategoriesRepo.insertCategory(templateId, name.trim(), (maxOrder ?? -1) + 1);
+    });
     return { category: await this.templateCategoriesRepo.findById(newId) };
   }
 
@@ -863,8 +867,10 @@ export class PackingService {
     if (!name?.trim()) return { error: 'Item name is required', status: 400 };
     const cat = await this.templateCategoriesRepo.findInTemplate(catId, templateId);
     if (!cat) return { error: 'Category not found', status: 404 };
-    const maxOrder = await this.templateItemsRepo.maxSortOrder(catId);
-    const newId = await this.templateItemsRepo.insertTemplateItem({ category_id: catId, name: name.trim(), sort_order: (maxOrder ?? -1) + 1 });
+    const newId = await this.uow.transactional(async () => {
+      const maxOrder = await this.templateItemsRepo.maxSortOrder(catId);
+      return await this.templateItemsRepo.insertTemplateItem({ category_id: catId, name: name.trim(), sort_order: (maxOrder ?? -1) + 1 });
+    });
     return { item: await this.templateItemsRepo.findById(newId) };
   }
 

server/src/nest/roadtrip/roadtrip.service.ts

@@ -107,15 +107,17 @@ export class RoadtripService {
   async create(dayId: string | number, input: { after_order_index: number; lat: number; lng: number; sequence?: number }): Promise<RoadtripVia> {
     const dayIdNum = this.requireDayId(dayId);
     // Appended after whatever already follows that stop, unless the caller says where.
-    // RT4 (`nextSequence`) then RT5 (`insertVia`), un-transacted (R7 — pin, don't fix):
-    // two concurrent adds on one leg can read the same next sequence.
-    const sequence = input.sequence ?? (await this.viasRepo.nextSequence(dayIdNum, input.after_order_index));
-    const id = await this.viasRepo.insertVia({
-      day_id: dayIdNum,
-      after_order_index: input.after_order_index,
-      sequence,
-      lat: input.lat,
-      lng: input.lng,
+    // RT4 (`nextSequence`) and RT5 (`insertVia`) in one transaction, so two adds on
+    // one leg at once cannot read the same next sequence.
+    const id = await this.uow.transactional(async () => {
+      const sequence = input.sequence ?? (await this.viasRepo.nextSequence(dayIdNum, input.after_order_index));
+      return await this.viasRepo.insertVia({
+        day_id: dayIdNum,
+        after_order_index: input.after_order_index,
+        sequence,
+        lat: input.lat,
+        lng: input.lng,
+      });
     });
     return (await this.viasRepo.findById(id))!;
   }

server/src/nest/todo/todo.service.ts

@@ -64,13 +64,15 @@ export class TodoService {
   async createItem(tripId: string | number, data: {
     name: string; category?: string | null; due_date?: string | null; description?: string | null; assigned_user_id?: number | null; priority?: number;
   }) {
-    const maxOrder = await this.todoItemsRepo.maxSortOrder(tripId);
-    const sortOrder = (maxOrder !== null ? maxOrder : -1) + 1;
-
-    const id = await this.todoItemsRepo.insertItem({
-      trip_id: tripId, name: data.name, category: data.category || null, sort_order: sortOrder,
-      due_date: data.due_date || null, description: data.description || null,
-      assigned_user_id: data.assigned_user_id || null, priority: data.priority || 0,
+    // The next position and the row that takes it in one transaction, so two items
+    // added at once cannot both read the same MAX.
+    const id = await this.uow.transactional(async () => {
+      const maxOrder = await this.todoItemsRepo.maxSortOrder(tripId);
+      return await this.todoItemsRepo.insertItem({
+        trip_id: tripId, name: data.name, category: data.category || null, sort_order: (maxOrder ?? -1) + 1,
+        due_date: data.due_date || null, description: data.description || null,
+        assigned_user_id: data.assigned_user_id || null, priority: data.priority || 0,
+      });
     });
 
     return this.todoItemsRepo.findById(id);

server/tests/unit/nest/collections.service.test.ts

@@ -146,6 +146,23 @@ describe('collections CRUD + visibility', () => {
     expect((await svc.listCollections(b.id)).collections).toHaveLength(0);
   });
 
+  it('COLLECTIONS-SVC-001b: lists and labels created at the same time take one position each', async () => {
+    const a = createUser(testDb).user;
+    // Without the transaction every one read the same MAX and they shared a position.
+    const lists = await Promise.all(['A', 'B', 'C'].map((name) => svc.createCollection(a.id, { name })));
+    expect(lists.map((l) => l.sort_order).sort((x, y) => x - y)).toEqual([0, 1, 2]);
+    const labels = await Promise.all(['Food', 'Sights', 'Bars'].map((name) => svc.createLabel(a.id, lists[0].id, name)));
+    expect(labels.map((l) => l.sort_order).sort((x, y) => x - y)).toEqual([0, 1, 2]);
+  });
+
+  it('COLLECTIONS-SVC-001c: two labels of one name created at the same time: one lands, the other is a 409', async () => {
+    const a = createUser(testDb).user;
+    const list = await svc.createCollection(a.id, { name: 'Tokyo' });
+    const results = await Promise.allSettled([svc.createLabel(a.id, list.id, 'Food'), svc.createLabel(a.id, list.id, 'Food')]);
+    expect(results.filter((r) => r.status === 'fulfilled')).toHaveLength(1);
+    expect(results.find((r) => r.status === 'rejected')).toMatchObject({ reason: { status: 409 } });
+  });
+
   it('COLLECTIONS-SVC-002: getCollection 404 for a non-member', async () => {
     const a = createUser(testDb).user;
     const b = createUser(testDb).user;

server/tests/unit/nest/days.service.test.ts

@@ -241,6 +241,16 @@ describe('create (service)', () => {
     expect(d2.day_number).toBe(2);
   });
 
+  it('DAY-SVC-009b: days added at the same time take one day number each', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id);
+    // Without the transaction both read the same MAX and both became day 1.
+    await Promise.all([svc.create(trip.id), svc.create(trip.id), svc.create(trip.id)]);
+    const numbers = (testDb.prepare('SELECT day_number FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { day_number: number }[])
+      .map((d) => d.day_number);
+    expect(numbers).toEqual([1, 2, 3]);
+  });
+
   it('DAY-SVC-010 — returns day with empty assignments array', async () => {
     const { user } = createUser(testDb);
     const trip = createTrip(testDb, user.id);

server/tests/unit/nest/packing.service.test.ts

@@ -224,6 +224,22 @@ function seedTemplate(userId: number, itemNames: string[]): number {
   return templateId;
 }
 
+describe('template positions', () => {
+  it('PACK-SVC-002b: template categories and items added at the same time take one position each', async () => {
+    const { user } = createUser(testDb);
+    const templateId = seedTemplate(user.id, ['Tent']);
+    // seedTemplate's own category holds position 0 and its item position 0.
+    const categories = await Promise.all(['Kitchen', 'Sleep', 'Clothes'].map((name) => svc.createTemplateCategory(String(templateId), name)));
+    const catIds = categories.map((c) => (c as { category: { id: number } }).category.id);
+    expect((testDb.prepare('SELECT sort_order FROM packing_template_categories WHERE template_id = ? ORDER BY sort_order').all(templateId) as { sort_order: number }[])
+      .map((r) => r.sort_order)).toEqual([0, 1, 2, 3]);
+
+    await Promise.all(['Stove', 'Pot', 'Lighter'].map((name) => svc.createTemplateItem(String(templateId), String(catIds[0]), name)));
+    expect((testDb.prepare('SELECT sort_order FROM packing_template_items WHERE category_id = ? ORDER BY sort_order').all(catIds[0]) as { sort_order: number }[])
+      .map((r) => r.sort_order)).toEqual([0, 1, 2]);
+  });
+});
+
 describe('applyTemplate', () => {
   it('PACK-SVC-003: adds template items to a trip packing list', async () => {
     const { user } = createUser(testDb);
@@ -332,6 +348,18 @@ describe('createBag / deleteBag', () => {
     expect(bag.name).toBe('Carry-On');
   });
 
+  it('PACK-SVC-005b: bags and items created at the same time take one position each', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id);
+    // Without the transaction every one of them read the same MAX and landed on 0.
+    await Promise.all(['Carry-On', 'Checked', 'Daypack'].map((name) => svc.createBag(trip.id, { name })));
+    await Promise.all(['Tent', 'Stove', 'Mat'].map((name) => svc.createItem(trip.id, { name }, user.id)));
+    const orders = (table: string) => (testDb.prepare(`SELECT sort_order FROM ${table} WHERE trip_id = ? ORDER BY sort_order`).all(trip.id) as { sort_order: number }[])
+      .map((r) => r.sort_order);
+    expect(orders('packing_bags')).toEqual([0, 1, 2]);
+    expect(orders('packing_items')).toEqual([0, 1, 2]);
+  });
+
   it('PACK-SVC-006: deleteBag removes the bag and returns true', async () => {
     const { user } = createUser(testDb);
     const trip = createTrip(testDb, user.id);

server/tests/unit/nest/roadtrip.service.test.ts

@@ -326,15 +326,11 @@ describe('RoadtripService', () => {
     expect(await service.tracksForTrip(f.tripA.id)).toEqual([]);
   });
 
-  // R7 (Plan 3d Task 7 whole-plan review, item 12 — flag + pin, not fix, the
-  // MEMBERS-SVC-018 shape): `create`'s `nextSequence` (MAX(sequence)+1) READ
-  // and its `insertVia` WRITE are two separate statements with no lock
-  // between them. Measured under real concurrency (`Promise.allSettled`, no
-  // mocks): both calls succeed — there is no UNIQUE constraint to lose a
-  // race on, unlike `trip_members` — but both land on the SAME `sequence`
-  // (`0`), a genuine duplicate the un-serialized read-then-write leaves
-  // behind. Today's actual outcome, pinned; not a fix.
-  it('R7: two concurrent creates on the same leg both succeed but can land on the SAME sequence (unserialized nextSequence read)', async () => {
+  // R7: `create`'s `nextSequence` (MAX(sequence)+1) read and its `insertVia`
+  // write run in one transaction, which holds the connection, so two concurrent
+  // creates (`Promise.allSettled`, no mocks) take one sequence each. Before,
+  // both landed on sequence 0.
+  it('R7: two concurrent creates on the same leg both succeed and take consecutive sequences', async () => {
     const results = await Promise.allSettled([
       service.create(f.dayA1.id, { after_order_index: 0, lat: 1, lng: 1 }),
       service.create(f.dayA1.id, { after_order_index: 0, lat: 2, lng: 2 }),
@@ -342,8 +338,7 @@ describe('RoadtripService', () => {
     expect(results.every((r) => r.status === 'fulfilled')).toBe(true);
     const rows = await service.listForDay(f.dayA1.id);
     expect(rows).toHaveLength(2);
-    // Both landed on the same leg, at the same sequence — the duplicate R7 flags.
     expect(rows.map((r) => r.after_order_index)).toEqual([0, 0]);
-    expect(rows.map((r) => r.sequence)).toEqual([0, 0]);
+    expect(rows.map((r) => r.sequence).sort((a, b) => a - b)).toEqual([0, 1]);
   });
 });

server/tests/unit/nest/todo.service.test.ts

@@ -128,6 +128,16 @@ describe('listItems and createItem', () => {
     expect(b.sort_order).toBe(a.sort_order + 1);
   });
 
+  it('TODO-SVC-006b: items created at the same time still take one position each', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id);
+    // Without the transaction both read the same MAX and both landed on 0.
+    await Promise.all(['A', 'B', 'C'].map((name) => svc.createItem(trip.id, { name })));
+    const orders = (testDb.prepare('SELECT sort_order FROM todo_items WHERE trip_id = ? ORDER BY sort_order').all(trip.id) as { sort_order: number }[])
+      .map((r) => r.sort_order);
+    expect(orders).toEqual([0, 1, 2]);
+  });
+
   it('TODO-SVC-007: createItem stores optional fields', async () => {
     const { user } = createUser(testDb);
     const trip = createTrip(testDb, user.id);