plan(016-load-concurrency-testing): design assignment/SLA/escalation race fixes
research.md nails down the exact mechanism for each real race the audit found: a partial unique index for assignment double-assignment, a Ticket-style version counter for SLA pause/resume/sweep, and a partial unique index for escalation-rule idempotency — each traced to the specific repository/service code that has the gap today. data-model.md and plan.md carry the resulting schema and repository-contract changes; quickstart.md defines the real-infra verification steps for each user story. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
saqib mir
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
9f52d51003
commit
015ef62b71
@@ -0,0 +1,63 @@
|
||||
# Data Model: Load and Concurrency Testing
|
||||
|
||||
All changes below are additive to existing models — no existing column is removed or
|
||||
retyped, and no existing consumer (012-admin-list-views, 015-reporting-dashboards) needs any
|
||||
change, since none of them write to `Assignment`/`SLARun`/`EscalationEvent` directly (all writes
|
||||
already go through the repositories being changed here).
|
||||
|
||||
## `SLARun` (existing model, one new field)
|
||||
|
||||
| Field | Type | Notes |
|
||||
|---|---|---|
|
||||
| `version` | `Int @default(0)` | NEW. Optimistic-concurrency counter, identical convention to `Ticket.version` (003-ticketing). Incremented on every successful `updateWithVersion` call. |
|
||||
|
||||
Migration: additive `ALTER TABLE sla_runs ADD COLUMN version INTEGER NOT NULL DEFAULT 0;` —
|
||||
every existing row defaults to `0`, which is exactly the version any in-flight or future
|
||||
`updateWithVersion` call expects for a run nobody has updated since this migration ran.
|
||||
|
||||
## `Assignment` (existing model, no column change — one new index)
|
||||
|
||||
New raw partial unique index (Prisma schema DSL cannot express a partial predicate directly, so
|
||||
this is added via a raw-SQL migration step, same approach already used elsewhere in this
|
||||
project for Postgres-specific constraints):
|
||||
|
||||
```sql
|
||||
CREATE UNIQUE INDEX assignments_one_current_per_ticket
|
||||
ON assignments (ticket_id)
|
||||
WHERE is_current = true;
|
||||
```
|
||||
|
||||
Enforces at the database level: a ticket may have at most one `Assignment` row with
|
||||
`isCurrent = true` at any moment, closing the race research.md §1 describes. The existing
|
||||
non-unique `@@index([ticketId, isCurrent])` is unaffected and stays for the repository's own
|
||||
`findCurrent` lookup.
|
||||
|
||||
## `EscalationEvent` (existing model, no column change — one new index)
|
||||
|
||||
```sql
|
||||
CREATE UNIQUE INDEX escalation_events_ticket_rule_unique
|
||||
ON escalation_events (ticket_id, rule_id)
|
||||
WHERE rule_id IS NOT NULL;
|
||||
```
|
||||
|
||||
Enforces at the database level: a given rule may fire at most once per ticket over that
|
||||
ticket's lifetime (manual escalations, where `rule_id IS NULL`, are explicitly excluded and
|
||||
remain repeatable). Closes the race research.md §3 describes.
|
||||
|
||||
## Repository contract changes
|
||||
|
||||
### `SlaRunRepository`
|
||||
|
||||
- `update(id, data)` → **replaced** by `updateWithVersion(id, expectedVersion, data): Promise<SLARun | null>`, mirroring `TicketsRepository.updateStatus`'s exact shape: an atomic `updateMany({where:{id, version: expectedVersion}, data:{...data, version:{increment:1}}})`, returning the fresh row on success (count === 1) or `null` on a stale-version mismatch. Every existing call site (`pause`, `resume`, `complete`, `runBreachDetectionSweep`) is updated to pass its own last-read `version` and to retry (re-read + recompute + re-call) up to 3 times on a `null` result before giving up silently (matching the sweep's own existing no-throw, best-effort style — these are internal transitions with no HTTP caller waiting on a 409).
|
||||
|
||||
### `AssignmentRepository`
|
||||
|
||||
- `createAssignment(data)` — same signature and return type; internally catches a Prisma `P2002` on the new `assignments_one_current_per_ticket` index and retries the entire transaction (bounded to 3 attempts) before rethrowing.
|
||||
|
||||
### `EscalationEventRepository`
|
||||
|
||||
- `create(data)` — same signature; internally catches a Prisma `P2002` on the new `escalation_events_ticket_rule_unique` index and returns the pre-existing row for that `(ticketId, ruleId)` pair (a `findFirst({where:{ticketId, ruleId}})` fallback) instead of throwing, so `EscalationService.fire`'s caller sees a normal `EscalationEvent` either way — a duplicate trigger is invisible to the caller, not an error.
|
||||
|
||||
## Test-only entities (not persisted — in-memory test scaffolding)
|
||||
|
||||
- **Load test report** (`tests/load/`): `{ endpoint: string; connections: number; durationSec: number; requestsPerSec: number; latencyP50Ms: number; latencyP90Ms: number; latencyP99Ms: number; non2xxCount: number; rateLimitedCount: number }` — printed to console and written as JSON under `tests/load/reports/<endpoint>-<timestamp>.json` (gitignored) for each run, satisfying FR-008's separation of rate-limited responses from genuine failures.
|
||||
@@ -0,0 +1,132 @@
|
||||
# Implementation Plan: Load and Concurrency Testing
|
||||
|
||||
**Branch**: `016-load-concurrency-testing` | **Date**: 2026-09-09 | **Spec**: [spec.md](./spec.md)
|
||||
|
||||
**Input**: Feature specification from `/specs/016-load-concurrency-testing/spec.md`
|
||||
|
||||
## Summary
|
||||
|
||||
Prove — with real, genuinely-concurrent requests against real Postgres/Redis, never mocked
|
||||
timers — three concurrency guarantees that a prior codebase audit found are NOT currently held
|
||||
(assignment double-assignment, SLA pause/resume/sweep races, escalation duplicate-event risk),
|
||||
fix each real race the tests reveal with a minimal, idiomatic DB-level guard consistent with
|
||||
this codebase's existing patterns, add one new concurrency test proving the existing ticket
|
||||
optimistic-concurrency guarantee holds under genuine concurrency, and add repeatable
|
||||
`autocannon`-based HTTP load-test tooling for the three named critical endpoint groups.
|
||||
|
||||
## Technical Context
|
||||
|
||||
**Language/Version**: TypeScript 5.4 / Node.js >=20
|
||||
|
||||
**Primary Dependencies**: Fastify 4.26, Prisma, ioredis/BullMQ, Vitest (existing stack — no new
|
||||
runtime dependency for the concurrency tests); `autocannon` added as a new devDependency for the
|
||||
load-test tooling (pure npm package, no external binary, scriptable in TS, matches this
|
||||
project's existing Node-native toolchain rather than introducing a separate Go binary like k6)
|
||||
|
||||
**Storage**: PostgreSQL via Prisma (existing `Assignment`, `SLARun`, `EscalationEvent` models —
|
||||
one additive schema change per race fix, see data-model.md), Redis (existing, unchanged)
|
||||
|
||||
**Testing**: Vitest, run against the existing throwaway Docker Postgres/Redis
|
||||
(`supporthub-test-pg`/`supporthub-test-redis`) already used by `tests/concurrency/`; load tests
|
||||
run with `autocannon` against a real running instance of the dev server
|
||||
|
||||
**Target Platform**: Linux/Windows server (existing deployment target, unchanged)
|
||||
|
||||
**Project Type**: Backend service (existing modular monolith, unchanged)
|
||||
|
||||
**Performance Goals**: NEEDS CLARIFICATION resolved in research.md — no business-specified
|
||||
throughput/latency targets exist yet; FR-009 requires these be marked `OPEN BUSINESS DECISION`
|
||||
rather than invented, so this feature ships tooling + a baseline report, not a numeric SLA
|
||||
|
||||
**Constraints**: Every fix must be additive/backward-compatible (no breaking change to existing
|
||||
Assignment/SLARun/EscalationEvent consumers — 012-admin-list-views and 015-reporting-dashboards
|
||||
both already query these tables); every concurrency claim must be proven against real
|
||||
Docker-provisioned infrastructure per this project's standing verification discipline, never
|
||||
asserted from code review alone
|
||||
|
||||
**Scale/Scope**: 3 real races to prove-and-fix (assignment, SLA, escalation), 1 race to prove
|
||||
already-safe (ticket status), 3 endpoint groups to load-test (ticket creation, AI support flow,
|
||||
admin reporting) — entirely within `supporthub-api`, no `supporthub-web` changes
|
||||
|
||||
## Constitution Check
|
||||
|
||||
*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
|
||||
|
||||
| Principle | Check | Status |
|
||||
|---|---|---|
|
||||
| I. SaaS Is Sole Identity Authority | N/A — no identity/tenant/product-access logic touched | PASS |
|
||||
| II. Configuration Over Hardcoding | Load-test pass/fail thresholds are NOT hardcoded — explicitly marked `OPEN BUSINESS DECISION` per FR-009, matching roadmap convention | PASS |
|
||||
| III. Layered Architecture / Module Boundaries | All three fixes stay inside their owning module (`orchestration/assignments`, `orchestration/sla`, `orchestration/escalation`) — repository-layer changes only, no new cross-module imports | PASS |
|
||||
| IV. AI Recommends, Policy Decides | N/A — no AI/tool-permission logic touched | PASS |
|
||||
| V. Evidence-Based Verification | This entire feature IS evidence-based verification — every claimed guarantee must be proven by a real concurrency test against real infra before being considered fixed | PASS (this principle is the feature's own thesis) |
|
||||
| VI. Durable Audit & History | No audit-log shape changes; EscalationEvent's idempotency fix preserves the existing audit row for the winning attempt, silently no-ops the loser rather than deleting anything | PASS |
|
||||
| VII. Concurrency-Safe, Durable Job Handling (NON-NEGOTIABLE) | This feature directly implements this principle's own stated requirement ("Assignment and escalation logic MUST be tested under concurrency... job handlers MUST be idempotent") — it is the principle's own overdue test coverage | PASS — this feature exists to close this exact gap |
|
||||
| VIII. Ticket/Problem Separation | N/A — no Ticket/Problem model changes | PASS |
|
||||
|
||||
No violations. No Complexity Tracking entries needed.
|
||||
|
||||
## Project Structure
|
||||
|
||||
### Documentation (this feature)
|
||||
|
||||
```text
|
||||
specs/016-load-concurrency-testing/
|
||||
├── plan.md # This file
|
||||
├── research.md # Phase 0 output
|
||||
├── data-model.md # Phase 1 output
|
||||
├── quickstart.md # Phase 1 output
|
||||
└── tasks.md # Phase 2 output (/speckit-tasks — not yet created)
|
||||
```
|
||||
|
||||
No `contracts/` directory: this feature adds no new HTTP endpoints or request/response
|
||||
contracts — it hardens existing internal behavior and adds test/tooling infrastructure only.
|
||||
|
||||
### Source Code (repository root)
|
||||
|
||||
```text
|
||||
prisma/
|
||||
└── schema.prisma # +1 field (SLARun.version), +2 raw partial
|
||||
# unique indexes (migration SQL)
|
||||
|
||||
src/modules/orchestration/assignments/
|
||||
├── repository/assignment.repository.ts # createAssignment: catch+retry on the new
|
||||
# partial-unique-index conflict
|
||||
└── ... # (engine/service unchanged)
|
||||
|
||||
src/modules/orchestration/sla/
|
||||
├── repository/sla-run.repository.ts # update() becomes version-checked; add
|
||||
│ updateWithVersion(id, expectedVersion, data)
|
||||
└── service/sla.service.ts # pause/resume/complete: read-modify-retry
|
||||
loop on version conflict (bounded attempts)
|
||||
|
||||
src/modules/orchestration/escalation/
|
||||
├── repository/escalation-event.repository.ts # create(): catch the new partial-unique
|
||||
│ -index conflict, return existing row
|
||||
└── service/escalation.service.ts # fire(): treat a duplicate-conflict as a
|
||||
no-op, not an error
|
||||
|
||||
tests/concurrency/
|
||||
├── round-robin.test.ts # existing — untouched
|
||||
├── queue.test.ts # existing — untouched
|
||||
├── assignment-race.test.ts # NEW — User Story 1 / FR-001
|
||||
├── sla-race.test.ts # NEW — User Story 2 / FR-002
|
||||
├── escalation-idempotency.test.ts # NEW — User Story 3 / FR-003
|
||||
└── ticket-status-race.test.ts # NEW — User Story 4 / FR-004
|
||||
|
||||
tests/load/
|
||||
├── autocannon.config.ts # NEW — shared runner + report shape
|
||||
├── ticket-creation.load.ts # NEW — User Story 5 / FR-007, FR-008
|
||||
├── ai-support-flow.load.ts # NEW
|
||||
└── admin-reporting.load.ts # NEW
|
||||
```
|
||||
|
||||
**Structure Decision**: Single backend project (existing `supporthub-api` modular monolith).
|
||||
Fixes live inside their owning module's existing `repository`/`service` files (Principle III);
|
||||
new tests live in the existing `tests/concurrency/` directory (already established by
|
||||
round-robin.test.ts) plus a new `tests/load/` directory for the load-test tooling, mirroring the
|
||||
existing `tests/{unit,integration,e2e,concurrency}` layout with one new sibling rather than
|
||||
overloading `tests/concurrency/` with non-correctness-proving load scripts.
|
||||
|
||||
## Complexity Tracking
|
||||
|
||||
*No violations — table omitted.*
|
||||
@@ -0,0 +1,85 @@
|
||||
# Quickstart: Load and Concurrency Testing
|
||||
|
||||
Manual + automated verification steps for each user story, against real Docker-provisioned
|
||||
Postgres/Redis — this project's standing rule that a concurrency claim is never accepted from
|
||||
code review alone.
|
||||
|
||||
## Prerequisites
|
||||
|
||||
- Throwaway test infra up: `supporthub-test-pg` (host port 5433), `supporthub-test-redis` (host
|
||||
port 6380) — the same containers `tests/concurrency/round-robin.test.ts` already uses.
|
||||
- For the load tests (User Story 5) only: a real running instance of the API against the real
|
||||
dev infra (`postgres-development`/`redis-development`), reachable at
|
||||
`http://localhost:4501`, plus an ADMIN session token for the reporting endpoints.
|
||||
|
||||
## Scenario 1 — Assignment double-assignment race (User Story 1)
|
||||
|
||||
1. `REDIS_HOST=localhost REDIS_PORT=6380 npx vitest run tests/concurrency/assignment-race.test.ts`
|
||||
2. The test creates one ticket, then fires >=20 concurrent `assignmentEngine.assignToSpecificNode`
|
||||
(or the equivalent orchestration entry point) calls at it against real Postgres.
|
||||
3. **Expected**: the test itself queries `assignments` directly afterward and asserts exactly
|
||||
one row has `is_current = true` for that ticket — not just that one HTTP/service call
|
||||
"won." Repeat the run at least 10 times (or use the test's own internal repeat loop) to
|
||||
confirm SC-001's "zero exceptions across 10 repeated runs."
|
||||
4. Before the fix (research.md §1), this test is expected to fail intermittently; after the
|
||||
fix, it must pass every time.
|
||||
|
||||
## Scenario 2 — SLA pause/resume/sweep race (User Story 2)
|
||||
|
||||
1. `REDIS_HOST=localhost REDIS_PORT=6380 npx vitest run tests/concurrency/sla-race.test.ts`
|
||||
2. The test creates a ticket with an active SLA run, then fires concurrent `pause`/`resume`
|
||||
calls and a `runBreachDetectionSweep()` pass against the same run.
|
||||
3. **Expected**: the run's final DB state (`status`, `pausedAt`, `resumedAt`, `breachedAt`,
|
||||
`firstResponseDueAt`, `resolutionDueAt`) is queried directly and asserted internally
|
||||
consistent — e.g. never `status: 'paused'` with `pausedAt: null`, never a `breached` run
|
||||
silently reverted to `running` by a racing `resume`. Repeat per SC-002.
|
||||
|
||||
## Scenario 3 — Escalation idempotency (User Story 3)
|
||||
|
||||
1. `REDIS_HOST=localhost REDIS_PORT=6380 npx vitest run tests/concurrency/escalation-idempotency.test.ts`
|
||||
2. The test creates a ticket eligible for a specific escalation rule, then calls
|
||||
`escalationService.handleBreach` (or `fire` via its real trigger path) twice concurrently for
|
||||
the identical trigger.
|
||||
3. **Expected**: exactly one `EscalationEvent` row exists afterward for that `(ticketId,
|
||||
ruleId)` pair, and exactly one `Assignment` row resulted from it (cross-checking Scenario 1's
|
||||
own guarantee). Repeat per SC-003.
|
||||
|
||||
## Scenario 4 — Ticket status optimistic concurrency proof (User Story 4)
|
||||
|
||||
1. `REDIS_HOST=localhost REDIS_PORT=6380 npx vitest run tests/concurrency/ticket-status-race.test.ts`
|
||||
2. The test creates a ticket at a known status/version, then fires >=20 concurrent
|
||||
`ticketsRepository.updateStatus` calls all starting from that same version.
|
||||
3. **Expected**: exactly one call returns the updated ticket; every other call returns `null`
|
||||
(stale-version signal); the ticket's final DB status matches the one call that succeeded.
|
||||
This is expected to pass on the very first run (spec.md Assumptions) — a failure here would
|
||||
mean the existing mechanism has a real gap, not that this quickstart step is wrong.
|
||||
|
||||
## Scenario 5 — Load/throughput baseline (User Story 5)
|
||||
|
||||
1. Ensure the real dev API is running (`npm run dev` against `.env.development`) and reachable.
|
||||
2. `npx tsx tests/load/ticket-creation.load.ts`
|
||||
3. `npx tsx tests/load/ai-support-flow.load.ts`
|
||||
4. `npx tsx tests/load/admin-reporting.load.ts` (needs an ADMIN token — the script signs in
|
||||
itself using the same seeded admin credentials this project's E2E suite already uses)
|
||||
5. **Expected**: each script prints a report (requests/sec, `p50`/`p90`/`p99` latency, non-2xx
|
||||
count, rate-limited count) and writes it to `tests/load/reports/`. There is no pass/fail
|
||||
assertion on the numbers themselves (FR-009, `OPEN BUSINESS DECISION`) — the check here is
|
||||
that the tooling runs cleanly end-to-end and produces a comparable, re-runnable report, not
|
||||
that any specific number is hit.
|
||||
6. Run the same script twice in a row and confirm the two reports are comparable in shape
|
||||
(same fields, plausible numbers) — proving SC-005's "consistent-shape output for comparison
|
||||
across runs."
|
||||
|
||||
## What "done" looks like
|
||||
|
||||
- All four new `tests/concurrency/*.test.ts` files pass consistently (not flakily) against real
|
||||
Postgres/Redis, each proving its own user story's guarantee with a direct database assertion,
|
||||
not just an HTTP response check.
|
||||
- Every race the audit found (assignment, SLA, escalation) is fixed in the actual repository
|
||||
code per data-model.md, not merely detected and left alone.
|
||||
- All three `tests/load/*.load.ts` scripts run cleanly against a real running dev server and
|
||||
produce a report.
|
||||
- Full existing quality gate (typecheck, lint, architecture check, full unit + integration
|
||||
suite) stays green — these fixes touch shared repositories (`Assignment`, `SLARun`,
|
||||
`EscalationEvent`) already exercised by 007-orchestration-assignment's, 008-sla-escalation's,
|
||||
012-admin-list-views's, and 015-reporting-dashboards's own existing tests.
|
||||
@@ -0,0 +1,153 @@
|
||||
# Research: Load and Concurrency Testing
|
||||
|
||||
## 1. Assignment double-assignment race
|
||||
|
||||
**Decision**: Add a PostgreSQL partial unique index — `CREATE UNIQUE INDEX
|
||||
assignments_one_current_per_ticket ON assignments (ticket_id) WHERE is_current = true;` — and
|
||||
change `AssignmentRepository.createAssignment` to catch the resulting unique-violation (Prisma
|
||||
`P2002`) and retry the whole supersede-then-create transaction (bounded to 3 attempts, matching
|
||||
this codebase's existing small-bounded-retry convention), rather than surfacing a raw 500.
|
||||
|
||||
**Rationale**: `createAssignment`'s existing transaction (`updateMany({isCurrent:false}) +
|
||||
create({isCurrent:true})`) is correct in isolation but Postgres's default `READ COMMITTED`
|
||||
isolation lets two concurrent transactions each see "no current row to supersede" and both
|
||||
successfully `create` their own `isCurrent:true` row — there is no read-modify-write cycle a
|
||||
version field could guard here (unlike Ticket/SLARun below), because the operation is a
|
||||
create, not an update, and a create can't be conditioned on "no matching row exists" atomically
|
||||
without a DB-level constraint. A partial unique index is the standard, minimal Postgres pattern
|
||||
for "at most one row matching a predicate" and requires no application-level locking. Retrying
|
||||
on conflict (rather than failing the second caller outright) preserves current behavior for the
|
||||
common, non-racing case and correctly resolves the race by making the loser's request apply
|
||||
*after* the winner's, superseding it — exactly the same "last write wins, but exactly once"
|
||||
semantics `createAssignment`'s own docstring already promises for the non-concurrent case.
|
||||
|
||||
**Alternatives considered**:
|
||||
- *Explicit `SERIALIZABLE` transaction isolation*: would also detect the race (as a
|
||||
serialization failure) but requires the exact same catch-and-retry handling as the unique
|
||||
index approach, adds latency to every assignment (not just racing ones), and does nothing to
|
||||
prevent the row from ever being duplicated if a future code path creates an Assignment outside
|
||||
this transaction — a DB constraint is a stronger, more future-proof guarantee.
|
||||
- *Row-level lock (`SELECT ... FOR UPDATE`) on a per-ticket lock row*: works, but requires
|
||||
inventing a new lock-row concept for a case Postgres's own partial unique index already solves
|
||||
natively.
|
||||
|
||||
## 2. SLA pause/resume/sweep race
|
||||
|
||||
**Decision**: Add `version Int @default(0)` to `SLARun`. Replace `SlaRunRepository.update(id,
|
||||
data)` with `updateWithVersion(id, expectedVersion, data)`, mirroring
|
||||
`TicketsRepository.updateStatus`'s existing atomic `updateMany({where:{id, version:
|
||||
expectedVersion}, data:{...data, version:{increment:1}}})` pattern exactly. `SlaService.pause`,
|
||||
`resume`, `complete`, and `runBreachDetectionSweep` each move to a small
|
||||
read-compute-write-retry loop (bounded to 3 attempts): re-read the run fresh on a version
|
||||
conflict, recompute the operation's own delta (e.g. resume's `pausedMs` shift) against the fresh
|
||||
state, and retry the versioned write.
|
||||
|
||||
**Rationale**: Every one of `pause`/`resume`/`complete`/the sweep does an unconditional
|
||||
read-then-`update(run.id, {...})` with no guard — two of these racing (e.g. `resume` and the
|
||||
sweep evaluating the same run at once) can silently clobber each other: the sweep's own
|
||||
`update(run.id, {status:'breached', breachedAt: now})` could be overwritten moments later by a
|
||||
`resume` that read the run *before* the sweep's write and still thinks it's `paused`, un-breaching
|
||||
a run that was legitimately breached and permanently losing that breach from SLA-compliance
|
||||
figures — a real, silent correctness bug, not a hypothetical one. `Ticket` already has exactly
|
||||
this problem solved for its own status field with a `version` counter and an atomic
|
||||
conditional-update; reusing that identical mechanism (rather than inventing a new one) keeps the
|
||||
codebase's concurrency idiom singular and matches Principle III's spirit even though it isn't
|
||||
a cross-module boundary concern.
|
||||
|
||||
**Alternatives considered**:
|
||||
- *Wrap each operation in a Postgres advisory lock keyed by run ID*: works but adds a new
|
||||
locking primitive to the codebase for a problem the existing version-counter idiom already
|
||||
solves; rejected for consistency, not because it wouldn't work.
|
||||
- *A single DB transaction spanning the sweep's read+write for all runs at once*: would only
|
||||
protect the sweep against itself, not against `pause`/`resume` racing it from an unrelated
|
||||
request path — doesn't close the actual gap.
|
||||
|
||||
## 3. Escalation idempotency
|
||||
|
||||
**Decision**: Add a PostgreSQL partial unique index — `CREATE UNIQUE INDEX
|
||||
escalation_events_ticket_rule_unique ON escalation_events (ticket_id, rule_id) WHERE rule_id IS
|
||||
NOT NULL;` — and change `EscalationEventRepository.create` (called from
|
||||
`EscalationService.fire`) to catch the resulting `P2002` and return the already-existing event
|
||||
for that `(ticketId, ruleId)` pair instead of creating a duplicate or throwing.
|
||||
|
||||
**Rationale**: `SLARun.ticketId` is `@unique` and "no reopen-cycle support" (existing schema
|
||||
comment) means a given rule can only ever legitimately fire once per ticket's lifetime for a
|
||||
rule-triggered breach (`handleBreach`'s `ruleId` is always a real rule ID scoped to one specific
|
||||
`triggerType`; `resolution_breach` and `first_response_breach` runs are naturally different
|
||||
rules, so this constraint doesn't conflate the two). Manual escalation
|
||||
(`escalateManually`/`fire(ticketId, ruleId: null, ...)`) is deliberately excluded from the
|
||||
constraint (`WHERE rule_id IS NOT NULL`) because an admin legitimately re-escalating the same
|
||||
ticket manually more than once must keep working exactly as it does today. This directly closes
|
||||
the gap the audit found: `runBreachDetectionSweep`'s `findRunningPastResolutionDueAt` can return
|
||||
the same still-`running` row to two overlapping sweep passes (e.g. a slow sweep still finishing
|
||||
when the next scheduled tick fires, or a duplicate BullMQ job delivery calling `handleBreach`
|
||||
directly) before either pass's own `update(run.id, {status:'breached', ...})` commits — without
|
||||
this constraint, both passes independently call `fire` and each successfully creates its own
|
||||
`EscalationEvent` plus its own `assignToSpecificNode`.
|
||||
|
||||
**Alternatives considered**:
|
||||
- *A dedicated idempotency-key column populated by the caller (e.g. a sweep-run ID)*: more
|
||||
general, but overkill here — the natural, already-unique business key for a rule-triggered
|
||||
escalation genuinely is `(ticketId, ruleId)` given the "no reopen-cycle" constraint already in
|
||||
place; inventing a separate key would duplicate information the schema already expresses.
|
||||
- *Making the sweep single-flight via a Redis lock around the whole sweep function*: would
|
||||
prevent two sweep passes from overlapping, but does not protect against a duplicate BullMQ job
|
||||
calling `handleBreach` directly for the same trigger outside the sweep's own loop — the DB
|
||||
constraint protects the actual invariant regardless of caller, which is the correct place per
|
||||
Principle VII ("job handlers MUST be idempotent").
|
||||
|
||||
## 4. Ticket optimistic-concurrency proof
|
||||
|
||||
**Decision**: No implementation change. Add `tests/concurrency/ticket-status-race.test.ts`
|
||||
firing a batch of genuinely concurrent `TicketsRepository.updateStatus` calls at the same
|
||||
ticket, all from the same starting version, against the real throwaway Postgres, and asserting
|
||||
exactly one succeeds (returns the updated ticket) while every other call returns `null` (the
|
||||
existing stale-version-mismatch signal) — proving FR-004/SC-004 against the mechanism that
|
||||
already exists (see `tests/concurrency/round-robin.test.ts:12`'s own reference to "003-ticketing's
|
||||
optimistic ticket-status concurrency" as prior art that was never itself concurrency-tested).
|
||||
|
||||
**Rationale**: The existing `updateMany({where:{id, version: expectedVersion}, ...})` is a
|
||||
single atomic SQL statement — Postgres itself guarantees only one concurrent `UPDATE` matching
|
||||
that `WHERE` clause can succeed before the row's `version` changes underneath the others. This
|
||||
is sound by construction; the gap is purely "never proven under real concurrency," which this
|
||||
research assumes will simply confirm the existing guarantee (per spec.md's own Assumptions) —
|
||||
but the test is still written to fail loudly if that assumption turns out to be wrong.
|
||||
|
||||
## 5. Load-test tooling choice
|
||||
|
||||
**Decision**: `autocannon` (npm devDependency), invoked via small TypeScript runner scripts
|
||||
under `tests/load/`, one per named endpoint group (ticket creation, AI support flow, admin
|
||||
reporting), each producing a JSON report (`autocannon`'s own `Result` shape: requests/sec,
|
||||
latency `p50`/`p90`/`p99`, non-2xx count) written to `tests/load/reports/` (gitignored — these
|
||||
are run artifacts, not fixtures) plus a printed console summary.
|
||||
|
||||
**Rationale**: `autocannon` is a pure Node.js package (no separate binary to install, unlike
|
||||
k6), is TypeScript-friendly, and its programmatic API (`autocannon({url, connections, duration,
|
||||
requests: [...]}, callback)`) fits scripting multi-step flows (e.g. sign-in once, then hammer an
|
||||
authenticated endpoint) far more naturally than k6's separate-runtime JS dialect — keeping this
|
||||
feature's new tooling inside the same Node/TS toolchain as the rest of the project (Technical
|
||||
Context), consistent with this project's existing minimal-new-tooling bias.
|
||||
|
||||
**Alternatives considered**:
|
||||
- *k6*: the industry-standard load-testing tool with richer scripting and threshold
|
||||
assertions, but ships as a separate Go binary requiring its own install/Docker image outside
|
||||
npm — heavier footprint for a project whose stack is otherwise 100% npm-managed.
|
||||
Reconsider if this project later needs distributed/cloud load generation, which `autocannon`
|
||||
does not support and k6 does.
|
||||
- *artillery*: also npm-native and closer to k6 in scripting richness, but pulls in a much
|
||||
larger dependency tree for YAML-driven scenario files this feature doesn't need — `autocannon`
|
||||
is a lighter fit for three hand-written TS scripts.
|
||||
|
||||
## 6. Load-test pass/fail thresholds
|
||||
|
||||
**Decision**: Per FR-009, no numeric throughput/latency/error-rate threshold is hardcoded as
|
||||
pass/fail. Each load-test report prints its own measured numbers and the tooling exits `0`
|
||||
regardless of the numbers observed (this is a measurement tool, not a gate) — a comment in each
|
||||
script marks the threshold question as `OPEN BUSINESS DECISION` and links back to spec.md
|
||||
Assumptions, so a future feature can wire an explicit pass/fail gate into CI once the business
|
||||
sets a real target.
|
||||
|
||||
**Rationale**: Inventing an arbitrary "must handle 500 req/s at p99 < 200ms" number would
|
||||
violate the roadmap's own explicit rule ("Never hardcode a placeholder value for any of the
|
||||
[open business decisions] and ship it as if it were final") — throughput/latency targets are
|
||||
exactly this kind of business-owned number, not an engineering default.
|
||||
Reference in New Issue
Block a user