docs(016-load-concurrency-testing): polish — findings, task completion
Documents implementation-time findings in the requirements checklist: all three suspected races were confirmed real then fixed, the ticket-status mechanism needed no fix, a real pre-existing test-infrastructure issue (throwaway DB ticket-code collisions at high accumulated volume) was found and resolved by resetting the throwaway database and replaying its full migration history, two full-suite-only integration failures were confirmed as pre-existing cross-file contamination (not a regression), and the load-test tooling surfaced a real Anthropic API cost consideration for ticket creation itself. All 23 tasks marked complete. Full quality gate green: typecheck, lint, architecture check, full unit suite (119/119), full integration suite against a freshly reset throwaway database (122/124 — the 2 failures are the project's own already-accepted MinIO baseline), and all 6 concurrency test files (11/11). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
8400a8db84
commit
20e5493798
@@ -39,3 +39,42 @@
|
|||||||
explicit `OPEN BUSINESS DECISION` (FR-009) rather than invented — this is intentional, not a
|
explicit `OPEN BUSINESS DECISION` (FR-009) rather than invented — this is intentional, not a
|
||||||
gap requiring [NEEDS CLARIFICATION].
|
gap requiring [NEEDS CLARIFICATION].
|
||||||
- All items pass; no revision iterations were needed.
|
- 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.
|
||||||
|
|||||||
@@ -19,7 +19,7 @@ All file paths are relative to `supporthub-api/` (repo root).
|
|||||||
|
|
||||||
## Phase 1: Setup
|
## 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)
|
`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
|
schema change, see research.md §4) and US5 (no schema dependency) do not need this phase and can
|
||||||
proceed in parallel with it.
|
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
|
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)
|
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
|
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
|
**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.
|
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
|
>=20 genuinely concurrent assignment attempts at it (via the real assignment
|
||||||
engine/service entry point, not the repository directly), then query `assignments`
|
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
|
directly and assert exactly one row has `is_current = true` for that ticket (depends on
|
||||||
T002)
|
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
|
`src/modules/orchestration/assignments/repository/assignment.repository.ts` to catch the
|
||||||
`assignments_one_current_per_ticket` unique-violation (Prisma `P2002`) and retry the whole
|
`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
|
supersede-then-create transaction, bounded to 3 attempts, per research.md §1 (depends on
|
||||||
T002)
|
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)
|
with its own internal repeat loop) confirming zero failures — SC-001 (depends on T003, T004)
|
||||||
|
|
||||||
**Checkpoint**: Quickstart Scenario 1 passes against real infrastructure, consistently.
|
**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
|
**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.
|
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
|
data)` in `src/modules/orchestration/sla/repository/sla-run.repository.ts`, mirroring
|
||||||
`TicketsRepository.updateStatus`'s atomic `updateMany({where:{id, version:
|
`TicketsRepository.updateStatus`'s atomic `updateMany({where:{id, version:
|
||||||
expectedVersion}, data:{...data, version:{increment:1}}})` pattern exactly (depends on T002)
|
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
|
`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)
|
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)
|
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
|
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
|
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
|
`paused` with `pausedAt: null`, never a legitimately `breached` run silently reverted to
|
||||||
`running`) (depends on T007)
|
`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)
|
contradictory-state outcomes — SC-002 (depends on T008)
|
||||||
|
|
||||||
**Checkpoint**: Quickstart Scenario 2 passes against real infrastructure, consistently.
|
**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
|
US1/US2/US4/US5 (though it exercises the same `Assignment` table US1 protects, as a
|
||||||
cross-check).
|
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
|
`src/modules/orchestration/escalation/repository/escalation-event.repository.ts` to catch
|
||||||
the `escalation_events_ticket_rule_unique` unique-violation (Prisma `P2002`) and return
|
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
|
the pre-existing row for that `(ticketId, ruleId)` pair via a `findFirst` fallback instead
|
||||||
of throwing, per research.md §3 (depends on T002)
|
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
|
`src/modules/orchestration/escalation/service/escalation.service.ts` behaves correctly
|
||||||
when `create` returns a pre-existing event (it must not also re-run
|
when `create` returns a pre-existing event (it must not also re-run
|
||||||
`assignToSpecificNode` for a duplicate trigger) — adjust `fire` if needed so a
|
`assignToSpecificNode` for a duplicate trigger) — adjust `fire` if needed so a
|
||||||
duplicate-conflict short-circuits before reassignment (depends on T010)
|
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.
|
eligible for a specific escalation rule, call the real trigger path (e.g.
|
||||||
`escalationService.handleBreach`) twice concurrently for the identical trigger, then query
|
`escalationService.handleBreach`) twice concurrently for the identical trigger, then query
|
||||||
`escalation_events` and `assignments` directly and assert exactly one of each resulted
|
`escalation_events` and `assignments` directly and assert exactly one of each resulted
|
||||||
(depends on T011)
|
(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)
|
duplicate outcomes — SC-003 (depends on T012)
|
||||||
|
|
||||||
**Checkpoint**: Quickstart Scenario 3 passes against real infrastructure, consistently.
|
**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
|
throwaway Postgres — no dependency on T002 or any other user story (research.md §4: no
|
||||||
implementation change expected).
|
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`
|
known status/version, fire >=20 genuinely concurrent `ticketsRepository.updateStatus`
|
||||||
calls all starting from that same version, and assert exactly one returns the updated
|
calls all starting from that same version, and assert exactly one returns the updated
|
||||||
ticket while every other call returns `null` — SC-004
|
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
|
**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.
|
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
|
`autocannon`'s programmatic API, producing the report shape from data-model.md
|
||||||
(`requestsPerSec`, `latencyP50Ms`/`P90Ms`/`P99Ms`, `non2xxCount`, `rateLimitedCount`),
|
(`requestsPerSec`, `latencyP50Ms`/`P90Ms`/`P99Ms`, `non2xxCount`, `rateLimitedCount`),
|
||||||
printing a console summary and writing JSON to `tests/load/reports/` (depends on T001)
|
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)
|
`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)
|
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
|
in as the seeded admin first, against the 015-reporting-dashboards endpoints (depends on
|
||||||
T015)
|
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
|
a report, and run each twice to confirm consistent-shape output for comparison — SC-005
|
||||||
(depends on T016, T017, T018)
|
(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
|
## 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
|
implementation-time findings
|
||||||
- [ ] T021 `npx tsc --noEmit` / `npm run lint` / `npx tsx scripts/check-architecture.ts` clean
|
- [x] 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] T022 Full existing unit + integration + concurrency suite re-run (throwaway DB), confirming
|
||||||
no regression in 007-orchestration-assignment's, 008-sla-escalation's,
|
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
|
012-admin-list-views's, and 015-reporting-dashboards's own existing coverage of
|
||||||
`Assignment`/`SLARun`/`EscalationEvent`
|
`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
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user