diff --git a/specs/016-load-concurrency-testing/checklists/requirements.md b/specs/016-load-concurrency-testing/checklists/requirements.md index 2272d90..c86cad6 100644 --- a/specs/016-load-concurrency-testing/checklists/requirements.md +++ b/specs/016-load-concurrency-testing/checklists/requirements.md @@ -39,3 +39,42 @@ explicit `OPEN BUSINESS DECISION` (FR-009) rather than invented — this is intentional, not a gap requiring [NEEDS CLARIFICATION]. - All items pass; no revision iterations were needed. + +## Implementation-time findings + +- **All three suspected real races were confirmed real, then fixed.** Before the fix, firing 20 + genuinely concurrent assignment attempts at the same ticket reliably threw an unhandled + Postgres unique-constraint error once the new `assignments_one_current_per_ticket` partial + index was in place (proving the race existed even before the retry logic was added) — after + the fix (bounded retry with jitter in `AssignmentRepository.createAssignment`), it holds + consistently across 10 repeated runs. Escalation idempotency was proven the same way: the + database-level unique-violation is visibly caught and absorbed in the logs during the test, + confirming the fix actually engages under a genuine race rather than sitting untested. +- **The ticket-status optimistic-concurrency mechanism (User Story 4) needed no fix** — proven + correct on the first run, exactly as research.md's Assumptions predicted. +- **A real, pre-existing test-infrastructure issue was found and resolved along the way**: the + throwaway integration-test Postgres database had accumulated a very large number of tickets + over this project's long development history, and the ticket-code generator's own + documented "rare race between two concurrent creates" (a read-then-increment sequence number + scoped by code prefix) became a frequent occurrence at that accumulated volume — manifesting + as dozens of unrelated integration-test failures when the full suite ran, unrelated to any + change in this feature. Confirmed by direct reproduction (a debug run showing the literal + `Unique constraint failed on the fields: (code)` error) and by re-running the exact same + suite cleanly (122/124 passing, matching the project's known accepted baseline) after + dropping and recreating the throwaway database and replaying its full migration history + (`prisma migrate deploy`, 12 migrations including this feature's own). This is a test- + infrastructure hygiene finding, not a defect in this feature's own code. +- **Two additional integration-test failures seen only in the full-suite run (never in + isolation)** were confirmed to be pre-existing cross-file contamination inherent to this + suite's shared-database, non-fully-isolated hierarchy/agent scoping (already acknowledged in + comments elsewhere in the suite, e.g. sla-escalation-flow.test.ts's own note about a + wildcard SLA policy leaking across concurrently-running files) — re-running the two affected + files together in isolation passed cleanly (13/13), ruling out this feature's own changes as + the cause. +- The autocannon-based load-test tooling (User Story 5) surfaced a real, non-obvious cost + consideration: ticket creation asynchronously triggers a real, billed Anthropic API call for + that ticket's first AI diagnosis turn (005-ai-support) — this applies to both the + ticket-creation and AI-support-flow load scripts, not only the latter as initially assumed. + All three scripts were run once at a small, explicitly bounded scale (confirmed with the + project owner beforehand) rather than an open-ended duration, specifically to keep this real + cost small and predictable. diff --git a/specs/016-load-concurrency-testing/tasks.md b/specs/016-load-concurrency-testing/tasks.md index 82e7db6..f23e645 100644 --- a/specs/016-load-concurrency-testing/tasks.md +++ b/specs/016-load-concurrency-testing/tasks.md @@ -19,7 +19,7 @@ All file paths are relative to `supporthub-api/` (repo root). ## Phase 1: Setup -- [ ] T001 [P] Add `autocannon` as a devDependency (`package.json`) and add +- [x] T001 [P] Add `autocannon` as a devDependency (`package.json`) and add `tests/load/reports/` to `.gitignore` (run artifacts, not fixtures) --- @@ -30,7 +30,7 @@ All file paths are relative to `supporthub-api/` (repo root). schema change, see research.md §4) and US5 (no schema dependency) do not need this phase and can proceed in parallel with it. -- [ ] T002 In `prisma/schema.prisma`, add `version Int @default(0)` to `SLARun`; generate one +- [x] T002 In `prisma/schema.prisma`, add `version Int @default(0)` to `SLARun`; generate one migration (`npx prisma migrate dev --name concurrency_guards`) that also includes, as raw SQL, `CREATE UNIQUE INDEX assignments_one_current_per_ticket ON assignments (ticket_id) WHERE is_current = true;` and `CREATE UNIQUE INDEX escalation_events_ticket_rule_unique ON @@ -51,17 +51,17 @@ assignment. **Independent Test**: Run `tests/concurrency/assignment-race.test.ts` alone against the throwaway Postgres — it creates its own ticket and needs nothing from US2-US5. -- [ ] T003 [US1] Write `tests/concurrency/assignment-race.test.ts`: create one ticket, fire +- [x] T003 [US1] Write `tests/concurrency/assignment-race.test.ts`: create one ticket, fire >=20 genuinely concurrent assignment attempts at it (via the real assignment engine/service entry point, not the repository directly), then query `assignments` directly and assert exactly one row has `is_current = true` for that ticket (depends on T002) -- [ ] T004 [US1] Fix `AssignmentRepository.createAssignment` in +- [x] T004 [US1] Fix `AssignmentRepository.createAssignment` in `src/modules/orchestration/assignments/repository/assignment.repository.ts` to catch the `assignments_one_current_per_ticket` unique-violation (Prisma `P2002`) and retry the whole supersede-then-create transaction, bounded to 3 attempts, per research.md §1 (depends on T002) -- [ ] T005 [US1] Re-run `assignment-race.test.ts` at least 10 times in a row (or extend the test +- [x] T005 [US1] Re-run `assignment-race.test.ts` at least 10 times in a row (or extend the test with its own internal repeat loop) confirming zero failures — SC-001 (depends on T003, T004) **Checkpoint**: Quickstart Scenario 1 passes against real infrastructure, consistently. @@ -76,20 +76,20 @@ it in one internally-consistent state. **Independent Test**: Run `tests/concurrency/sla-race.test.ts` alone against the throwaway Postgres — it creates its own ticket + SLA run and needs nothing from US1/US3/US4/US5. -- [ ] T006 [US2] Replace `SlaRunRepository.update` with `updateWithVersion(id, expectedVersion, +- [x] T006 [US2] Replace `SlaRunRepository.update` with `updateWithVersion(id, expectedVersion, data)` in `src/modules/orchestration/sla/repository/sla-run.repository.ts`, mirroring `TicketsRepository.updateStatus`'s atomic `updateMany({where:{id, version: expectedVersion}, data:{...data, version:{increment:1}}})` pattern exactly (depends on T002) -- [ ] T007 [US2] Update `pause`, `resume`, `complete`, and `runBreachDetectionSweep` in +- [x] T007 [US2] Update `pause`, `resume`, `complete`, and `runBreachDetectionSweep` in `src/modules/orchestration/sla/service/sla.service.ts` to call `updateWithVersion` with each run's last-read version, and to re-read + recompute + retry (bounded to 3 attempts) on a version-conflict `null` result, per research.md §2 (depends on T006) -- [ ] T008 [US2] Write `tests/concurrency/sla-race.test.ts`: create a ticket with an active SLA +- [x] T008 [US2] Write `tests/concurrency/sla-race.test.ts`: create a ticket with an active SLA run, fire concurrent `pause`/`resume` calls and a `runBreachDetectionSweep()` pass against it, then query the run directly and assert its final state is internally consistent (never `paused` with `pausedAt: null`, never a legitimately `breached` run silently reverted to `running`) (depends on T007) -- [ ] T009 [US2] Re-run `sla-race.test.ts` at least 10 times confirming zero +- [x] T009 [US2] Re-run `sla-race.test.ts` at least 10 times confirming zero contradictory-state outcomes — SC-002 (depends on T008) **Checkpoint**: Quickstart Scenario 2 passes against real infrastructure, consistently. @@ -106,22 +106,22 @@ throwaway Postgres — it creates its own ticket + escalation rule and needs not US1/US2/US4/US5 (though it exercises the same `Assignment` table US1 protects, as a cross-check). -- [ ] T010 [US3] Fix `EscalationEventRepository.create` in +- [x] T010 [US3] Fix `EscalationEventRepository.create` in `src/modules/orchestration/escalation/repository/escalation-event.repository.ts` to catch the `escalation_events_ticket_rule_unique` unique-violation (Prisma `P2002`) and return the pre-existing row for that `(ticketId, ruleId)` pair via a `findFirst` fallback instead of throwing, per research.md §3 (depends on T002) -- [ ] T011 [US3] Confirm `EscalationService.fire` in +- [x] T011 [US3] Confirm `EscalationService.fire` in `src/modules/orchestration/escalation/service/escalation.service.ts` behaves correctly when `create` returns a pre-existing event (it must not also re-run `assignToSpecificNode` for a duplicate trigger) — adjust `fire` if needed so a duplicate-conflict short-circuits before reassignment (depends on T010) -- [ ] T012 [US3] Write `tests/concurrency/escalation-idempotency.test.ts`: create a ticket +- [x] T012 [US3] Write `tests/concurrency/escalation-idempotency.test.ts`: create a ticket eligible for a specific escalation rule, call the real trigger path (e.g. `escalationService.handleBreach`) twice concurrently for the identical trigger, then query `escalation_events` and `assignments` directly and assert exactly one of each resulted (depends on T011) -- [ ] T013 [US3] Re-run `escalation-idempotency.test.ts` at least 10 times confirming zero +- [x] T013 [US3] Re-run `escalation-idempotency.test.ts` at least 10 times confirming zero duplicate outcomes — SC-003 (depends on T012) **Checkpoint**: Quickstart Scenario 3 passes against real infrastructure, consistently. @@ -137,7 +137,7 @@ concurrency. throwaway Postgres — no dependency on T002 or any other user story (research.md §4: no implementation change expected). -- [ ] T014 [US4] Write `tests/concurrency/ticket-status-race.test.ts`: create a ticket at a +- [x] T014 [US4] Write `tests/concurrency/ticket-status-race.test.ts`: create a ticket at a known status/version, fire >=20 genuinely concurrent `ticketsRepository.updateStatus` calls all starting from that same version, and assert exactly one returns the updated ticket while every other call returns `null` — SC-004 @@ -155,18 +155,18 @@ named critical endpoint groups. **Independent Test**: Run each `tests/load/*.load.ts` script alone against a real running dev server — no dependency on T002 or any other user story. -- [ ] T015 [P] [US5] Create `tests/load/autocannon.config.ts`: a shared runner helper wrapping +- [x] T015 [P] [US5] Create `tests/load/autocannon.config.ts`: a shared runner helper wrapping `autocannon`'s programmatic API, producing the report shape from data-model.md (`requestsPerSec`, `latencyP50Ms`/`P90Ms`/`P99Ms`, `non2xxCount`, `rateLimitedCount`), printing a console summary and writing JSON to `tests/load/reports/` (depends on T001) -- [ ] T016 [P] [US5] Create `tests/load/ticket-creation.load.ts` using the T015 helper against +- [x] T016 [P] [US5] Create `tests/load/ticket-creation.load.ts` using the T015 helper against `POST /v1/support/requests` (depends on T015) -- [ ] T017 [P] [US5] Create `tests/load/ai-support-flow.load.ts` using the T015 helper against +- [x] T017 [P] [US5] Create `tests/load/ai-support-flow.load.ts` using the T015 helper against the AI support flow's own endpoints (depends on T015) -- [ ] T018 [P] [US5] Create `tests/load/admin-reporting.load.ts` using the T015 helper, signing +- [x] T018 [P] [US5] Create `tests/load/admin-reporting.load.ts` using the T015 helper, signing in as the seeded admin first, against the 015-reporting-dashboards endpoints (depends on T015) -- [ ] T019 [US5] Run all three scripts against a real running dev server, confirm each produces +- [x] T019 [US5] Run all three scripts against a real running dev server, confirm each produces a report, and run each twice to confirm consistent-shape output for comparison — SC-005 (depends on T016, T017, T018) @@ -176,14 +176,14 @@ server — no dependency on T002 or any other user story. ## Phase 8: Polish & Cross-Cutting Concerns -- [ ] T020 Update `specs/016-load-concurrency-testing/checklists/requirements.md` Notes with any +- [x] T020 Update `specs/016-load-concurrency-testing/checklists/requirements.md` Notes with any implementation-time findings -- [ ] T021 `npx tsc --noEmit` / `npm run lint` / `npx tsx scripts/check-architecture.ts` clean -- [ ] T022 Full existing unit + integration + concurrency suite re-run (throwaway DB), confirming +- [x] T021 `npx tsc --noEmit` / `npm run lint` / `npx tsx scripts/check-architecture.ts` clean +- [x] T022 Full existing unit + integration + concurrency suite re-run (throwaway DB), confirming no regression in 007-orchestration-assignment's, 008-sla-escalation's, 012-admin-list-views's, and 015-reporting-dashboards's own existing coverage of `Assignment`/`SLARun`/`EscalationEvent` -- [ ] T023 Mark all of this file's checkboxes complete once verified +- [x] T023 Mark all of this file's checkboxes complete once verified ---