fix(thread-service): serialize sequence-number assignment per thread

[bokelley]

Summary

`server/tests/unit/thread-service.test.ts` was misfiled. Its own header described the tests as integration tests requiring a running Postgres, and it was guarded with `describe.skipIf(!process.env.DATABASE_URL)`. The unit job never sets `DATABASE_URL`, and the integration job globs only `server/tests/integration/`, so all 39 tests skipped in both jobs and ran nowhere.

Why this surfaced now

Audited the actual coverage of #3094's CI fix (#3292) and discovered the gap. Of 60 integration files, 59 run and 1 is intentionally env-gated on `COMPARE_LIVE=1` (`creative-agent-comparison`). The thread-service file was the one quiet anomaly #3094's audit didn't catch — the file lived in `unit/`, not `integration/`, so the survey of `integration/` coverage missed it.

What changed

• `git mv` from `server/tests/unit/thread-service.test.ts` to `server/tests/integration/thread-service.test.ts`. Relative imports (`../../src/...`) resolve identically — both directories are at the same depth from `src/`.
• Header refresh: "ThreadService Unit Tests" → "ThreadService Integration Tests" + a comment explaining the location and why the `skipIf` stays as defense-in-depth.
• No code changes; no test changes.

Coverage gain

39 tests on `ThreadService` (createOrCreateThread, addMessage, listThreads, paging, ON CONFLICT semantics) now actually run on every PR via the post-#3292 `server-integration` job. Previously they existed but couldn't catch any regression because they never executed.

Why `--empty` changeset

Test reorganization, no protocol-spec surface change.

Test plan

• `npx tsc --project server/tsconfig.json --noEmit` — clean.
• File rename at same depth → relative imports resolve.
• Verify the `server-integration` CI job picks it up and reports 39 passing tests on this PR's CI run.

🤖 Generated with Claude Code

[bokelley]

Extended this PR with the actual fix. The file move alone unmasked a real concurrency bug — moving the test was step 1, fixing what it caught is step 2.

Bug

ThreadService.addMessage assigned sequence_number via SELECT COALESCE(MAX(sequence_number), 0) + 1 ... INSERT inside a READ COMMITTED transaction. Two concurrent calls on the same thread observe the same MAX and write the same next value. Schema's (thread_id, sequence_number) index is non-unique, so collisions are silent — getThreadMessages's ORDER BY sequence_number ASC then returns nondeterministic ordering between duplicates.

Affects parallel Slack thread posts, webhook bursts, and fast tool-call sequences.

Fix

pg_advisory_xact_lock(hashtext(thread_id)) at the top of the transaction. Same pattern as billing/org-intake-lock. Serializes only on the same thread; cross-thread inserts run unimpeded; lock release is automatic on COMMIT/ROLLBACK.

Deferred

Defense-in-depth UNIQUE(thread_id, sequence_number) constraint is intentionally not in this PR. Production rows likely contain duplicates from the racey code path; adding the constraint without a dedup pass first would break the migration. Follow-up PR after a dedup audit.

Branch rebased onto current main (pulls in #4031 IA changes). The should handle concurrent addMessage calls with correct sequence numbers test should now pass for the right reason.

[bokelley]

Got it — advisory lock serializes per-thread with no cross-thread contention, and deferring the UNIQUE constraint until after a dedup audit is the right call. Clean approach.

---

Generated by Claude Code