fix(platform): prune dead org-domain doors and harden the live ones - #3253
Merged
Conversation
The support_cases domain (routes, service, ~620 lines) was mounted at /api/app/support-cases but nothing ever called it: no app adapter, no page, no REST or sandbox door, no handler_names entry, no docs — only its own integration probe. It also accepted a body-supplied contactId and wrote it raw into support_cases.contact_id without an org check. Delete the domain, its mount in app.ts and its probe. The three tables (app.support_cases, app.support_case_comments, app.support_case_activity) stay: a rolling deploy keeps the previous image serving while the new one migrates, so schema follows deprecate-then-delete; the ledger row marks them retired and awaiting a later drop. Finding: org-misc-1.
Five 0.4-ported doors have no 0.5 consumer — no app adapter, no REST or sandbox bridge, no handler_names entry, no docs; only their own integration probes reached them: - GET /approvals and GET /approvals/counts (listApprovals, countApprovalsByStatus) — the inbox page did not land; the docs say approvals are not a member surface. - GET /feedback and GET /feedback/mine/:messageId (listMessageFeedback, getMyMessageFeedback) — the metrics page reads /stats and /recent. - POST /products/:productId/translations (upsertProductTranslation). - POST /branding/reset (resetBranding) — the UI clears through DELETE /images/:type + an empty save. - GET /tts/capability — the player learns availability from the synthesize result's errorCode. Delete the routes and service functions, and re-point the integration probes at the doors the app actually uses (thread latch + /stats for feedback, delete + empty save for branding, one-row read for products). Findings: org-misc-2, org-misc-3, org-misc-4, org-misc-5, org-misc-6.
lib/shared/schemas/approvals.ts (FEEDBACK_KEY, the human-input field /
response / request-metadata schemas, the location-request schema) has no
importer in any workspace: the human-input consumer shipped as
backend/domains/chat/questions.ts with its own row shapes. The file was
hidden from knip by a parking entry whose stated reason ("types declared
for the parked consumers") no longer holds.
Remove the file, its knip.config.ts ignore line, and the pointer comment
in core/approvals/types.ts.
Finding: org-misc-8.
core/feedback/stats.ts exported isUnattributedAgentSlug and isFeedbackSyntheticAgentSlug for a UI consumer that does not exist, and declared its own ArenaVerdict/ARENA_VERDICTS beside the shared one in lib/shared/arena.ts (feedback/service.ts carried a third copy). The '__unattributed__' ranking sentinel was declared four times — stats.ts, chat/health.ts, analytics/feedback/types.ts and the chat-health page — so a change to either the slug or the verdict set would desynchronise the reducer from the tables that label it. Delete the dead helpers, take ArenaVerdict/ARENA_VERDICTS from lib/shared/arena.ts everywhere, and give UNATTRIBUTED_AGENT_SLUG one home in lib/shared/constants/usage.ts (next to the other synthetic agent-slug sentinels) imported by the reducer, chat health, and both analytics pages. Finding: org-misc-9.
…door decideApproval refused only the two review-gate kinds toward their dedicated respond doors and then flipped any other pending row to executing|rejected. A chat question lives on app.approvals as a `human_input_request` row and is settled by the person's next message in the thread; every reader in chat/questions.ts matches status = 'pending'. So an authenticated POST /approvals/:id/decide on such a row moved it to a state no consumer reads: the question was never settled or answered, and the one-pending-per-thread guard let a new ask be minted over it. Fold the refusals into one DEDICATED_RESPOND_DOORS table that now also names human_input_request; the door answers 409 APPROVAL_REQUIRES_DEDICATED_RESPOND and leaves the row pending. Unit test over a sql stand-in, and the approvals integration lane seeds a question row and asserts 409 + pending. Finding: org-misc-v2.
GET /api/app/products forwarded `limit` through Number() unvalidated: a
negative value reached Postgres as `LIMIT -4` ("LIMIT must not be
negative") and a fractional one as an uncastable bigint, both surfacing
as a 500 any org member could trigger from the query string. The sibling
contacts listing has always validated the same parameters with zod.
The route now parses search/status/category/limit/cursor through the
same zod shape (limit int 1..200, positive integer keyset cursor) and
answers 400 `invalid query` on failure. Routes test covers the refused
values and the forwarded ones.
Finding: org-misc-v1.
The TTS audio serve (GET /tts/audio/:chunkId) and the sandbox-blob stage route both fetched a presigned S3 URL with a bare `fetch(url)` — no signal, no timeout — and then streamed the body to the client. A store that accepts the connection and hangs (or a BYO endpoint that stalls) pinned the request and its socket indefinitely; under a per-chunk audio player those accumulate with no recovery path. One helper, `fetchPresignedObject` in backend/lib/object-store.ts, now serves both doors: the header wait is capped (30 s → TimeoutError, which the routes' existing catch turns into the 502 they already answer), the timer is cleared once headers arrive so a large body is never cut mid-stream, and the caller's request signal is forwarded before and after headers so a departed client tears the upstream stream down. Finding: org-misc-12.
reserveChunk locked the (message_id, chunk_index) row with SELECT … FOR UPDATE and branched on it. With no row present nothing is locked, so two concurrent first reserves (two tabs, a double-tap) both reached the INSERT; the loser violated tts_audio_chunks_message_index and, since synthesizeChunk calls the reserve unwrapped and the route rethrows anything that is not a TtsError, surfaced as a raw 500 — the player took that as a hard synthesis failure instead of polling the in-flight attempt. The header comment claimed racers "serialize at the constraint under FOR UPDATE". The reserve transaction now takes pg_advisory_xact_lock over the (message, index) key before the read, so the second racer waits, sees the winner's committed pending row and takes the existing-row branch (`in-flight`) — with no rate-limit token spent and no duplicate watchdog job. reserveChunk is exported for the real-Postgres race proof: a new integration lane fires two concurrent reserves of one fresh chunk and expects exactly one `reserved`, one `pending-in-flight`, one row. Unit test pins the lock-before-read order and the in-flight branch. Finding: org-misc-11.
Contact email uniqueness lived as a check-then-act probe on the bulk import only: no lock, SELECT then INSERT outside any transaction, blind to trash. The single create door never checked at all (the create dialog already mapped a CONTACT_DUPLICATE_EMAIL the backend never sent, and the REST reference has promised 409 for a duplicate email since the door opened). The mail-ingest shim carried a third, inline copy of find-or-create: it re-attached a conversation to a TRASHED contact, minted rows with no audit row / contact.created event / realtime hint, and accepted any `source` string. contacts/service.ts's own findOrCreateContactByEmail had no caller. One set of helpers now serves all three doors: a pg_advisory_xact_lock per (org, email) taken before the lookup (the pattern main already used in the shim), a LIVE-row lookup (trashed rows are out of the directory — listing, count and palette hide them — so they neither block a re-create nor adopt a new conversation), one INSERT, and one record step for the audit row + event + hint. - createContact refuses a live twin with 409 CONTACT_DUPLICATE_EMAIL. - bulkCreateContacts runs each item in its own transaction so the lock holds until the row lands; the per-row codes `duplicate_email` / `duplicate_external_id` the import dialog maps are unchanged; the external-id key gets the same lock, always after the email lock. - findOrCreateContactByEmail is the system-lane door (actor `system` on the audit row); the conversations shim delegates to it and pins `source` to the CONTACT_SOURCES vocabulary (the live caller passes `conversation`). - listContacts drops the includeTrashed option nothing passed. No unique index: live deployments may already hold twins from the unchecked era, and merging them is a product decision; the lookup orders oldest-first so pre-existing twins answer the same id every time. Tests: contacts/service.test.ts (lock-before-lookup order and key, live only lookup, 409 on a twin, no lock without email, RBAC, system-actor audit for the ingest lane, per-item transactions and lock order in bulk); shim.test.ts asserts the delegation. Integration lanes: the app door answers 409 on a duplicate and exactly one of two concurrent creates of one new email lands (200 + 409, one row); a trashed contact's email is free again; the ingest race lane now also checks the system audit row and that a trashed twin yields a fresh live contact. Findings: org-misc-7, org-misc-10.
The first run of the new probe sent a padded email, which the route's zod email schema refuses at the boundary (400 invalid body) before the duplicate check can answer 409. Case is what the normalization folds; padding is the schema's 400. Finding: org-misc-10 (probe only).
larryro
marked this pull request as ready for review
September 5, 2026 16:33
799c319 deleted five doors nothing called, but only the support_cases and tts ledger rows were amended: the approvals row still advertised list/counts and the filtered-listing probe, the feedback row the org insights feed, the branding row the reset write, the products row the translations upsert — four rows documenting routes that now 404. Append the same RETIRED (fix campaign 2026-09) note the tts row carries to each of the four, naming the deleted route + service function and the door the app actually uses instead, and shrink the approvals route summary to get/decide. Findings: org-misc-2, org-misc-3, org-misc-4, org-misc-5 (PR #3253 review, blocking 1).
fetchPresignedObject forwarded the caller's abort with a `{ once: true }`
listener that only ever fired on abort, so a fetch that timed out or
failed before headers left a closure on the request signal until the
request ended. Remove it in the finally when no headers arrived; after
headers it must stay, because the body still streams under that signal.
Unit test pins both halves.
Also correct the lockContactEmail doc comment: the "second writer sees
the first's commit" story holds only on the READ COMMITTED doors. The
app door runs createContact under transactSerializable, where the lock
is the transaction's first statement and fixes the snapshot before the
winner commits; convergence there is SSI (40001 on the loser) plus the
wrapper's retry, whose fresh snapshot yields the 409 — as the concurrent
itest observed. Spell that out so nobody drops the wrapper believing the
lock alone suffices.
Findings: org-misc-10, org-misc-12 (PR #3253 review, non-blocking notes).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Small org domains: prune the 0.4-ported doors nothing in 0.5 calls (the whole
support_casesdomain plus five single routes), delete the unused approvals zod vocabulary, converge the feedback sentinel/verdict vocabulary on one home, and harden the doors that stay: one locked find-or-create path for contacts (409 on a duplicate email, the promise the REST reference already made), a per-chunk lock on the first TTS reserve, a bounded fetch for the presigned objects the backend proxies, a validated products listing query, and the generic approvals decide door refusing chat-question rows.Tables
app.support_cases,app.support_case_comments,app.support_case_activityare NOT dropped in this change (rolling deploy keeps the previous image serving; deprecate-then-delete). They are marked retired inservices/platform/backend/MIGRATION.mdand await a later drop migration.Findings fixed
sourcestring) now delegates tofindOrCreateContactByEmailin contacts/service.ts (system-actor audit row + event + hint;sourcepinned to CONTACT_SOURCES); unusedincludeTrashedoption dropped.pg_advisory_xact_lockper (org, email) before a LIVE-row lookup and lands through one insert;createContactrefuses a twin with 409CONTACT_DUPLICATE_EMAIL(the code the create dialog already mapped; the status docs/develop/api-reference.md already promises); bulk runs each item in its own transaction so the lock holds until the row lands, external id gets the same lock (always after the email lock). No unique index — live deployments may hold twins from the unchecked era; oldest-first lookup makes them answer deterministically.reserveChunktakespg_advisory_xact_lockover (message, index) before theFOR UPDATEread (zero rows lock nothing); the second racer now reads the winner's pending row and answersin-flightinstead of a unique-violation 500.reserveChunkexported for the real-Postgres race lane.fetchPresignedObject(backend/lib/object-store.ts) serves both the TTS audio serve and the sandbox-blob stage: 30 s header-wait cap (→ the existing 502 path), timer cleared once headers arrive, caller signal forwarded before and after headers.invalid queryinstead of a Postgres 500.human_input_requestrows with 409 APPROVAL_REQUIRES_DEDICATED_RESPOND (one DEDICATED_RESPOND_DOORS table); unit + integration lane.Skipped
None.
Tests & gates observed
bun run check(worktree root):Tasks: 40 successful, 40 total— format:check green; every workspace lintFound 0 warnings and 0 errors;@tale/platform:test: Test Files 531 passed (531) / Tests 73365 passed (73365);@tale/platform:test:ui: Test Files 456 passed (456) / Tests 3512 passed (3512);@tale/ui:test 124/1181,@tale/shared:test 22 files,@tale/web:test 23/201,@tale/docs:test 31/201.bun run knip:check: exit 0 (one pre-existing configuration hint:cron-parserin services/platform knip.config.ts ignoreDependencies).bunx tsc --noEmit(services/platform): 0 errors.bunx oxlint --type-awareon every touched path: 0 findings.Test Files 523 passed (523) / Tests 6126 passed (6126).bun run checkagain from the root:Tasks: 40 successful, 40 total;@tale/platform:test: Test Files 531 passed (531) / Tests 73366 passed (73366);@tale/platform:test:ui: Test Files 456 passed (456) / Tests 3512 passed (3512); every lintFound 0 warnings and 0 errors;bunx tsc --noEmit0 errors; oxlint + opengrep on the touched files 0 findings;bun run knip:checkexit 0 (same pre-existing cron-parser hint). The integration proof was not re-run for the repair: it touches a ledger row, a doc comment and a listener cleanup insidefetchPresignedObject(no route, domain behaviour, job or schema changed).[itest] 474/476 checks passed across 135/135 lanes, no truncation — red:contacts CRUD + normalization + trash(my probe sent a padded email the route schema refuses with 400; every other clause incl. the concurrent 200+409 pair passed) andwebdav re-home(pre-existing red on main, not this theme).[itest] 475/476 checks passed across 135/135 lanes, noRUN TRUNCATED— the one red iswebdav re-home (protocol + tree + locks + visibility on pg), red on main before this campaign (WebDAV theme, not this branch).New lanes:
tts: two concurrent first reserves of one chunk → one reserved, one in-flight;mail ingest: four concurrent find-or-creates …now also asserts the system audit row and that a trashed twin yields a fresh live contact;contacts CRUD + normalization + trashnow asserts 409 CONTACT_DUPLICATE_EMAIL, concurrent app-door creates → 200,409 / one row, and a trashed contact's email is free again.Notes for the reviewer
POST /api/app/contactsandPOST /api/v1/contactsnow answer 409CONTACT_DUPLICATE_EMAILfor an email that names a LIVE contact of the org (trashed rows do not block a re-create). The REST reference already documented 409 for a duplicate email in all three locales, so no docs change. Bulk import keeps its per-rowduplicate_email/duplicate_external_idcodes, but both lookups now ignore trashed rows (they used to count a trashed row as a duplicate) — the directory's own semantics (listing, count, palette all hide trash).createContactundertransactSerializable; the concurrent-create itest proves the lock + SSI retry converge on 200/409 with one row. The REST door and the shim run under plainsql.begin(READ COMMITTED), where the post-lock lookup sees the winner's commit directly.reserveChunkis now an export used by the integration proof only (the app door remainssynthesizeChunk).fetchPresignedObjectheader timeout is a new constant (PRESIGNED_FETCH_HEADER_TIMEOUT_MS = 30_000), not an env knob.webdav re-homeis red on main independently of this branch.Review round 2 (repair of the adversarial review)
Blocking:
services/platform/backend/MIGRATION.mdnow carry the sameRETIRED (fix campaign 2026-09)note the tts and support_cases rows had, each naming the deleted route + service function and the door the app uses instead; the approvals route summary reads(get/decide; org-member)and its integration line no longer claims the filtered listing + counts.merge-treeagainst origin/main is CLEAN), and the PR squash-merges, so the header that lands on main is this PR's title (67 chars, commitlint-shaped). commitlint's configured limit (100) passes it; only the AGENTS.md prose bound of 72 is exceeded, by one character. Every header added in the repair is ≤69.Non-blocking:
lockContactEmaildoc comment (c6c4463): rewritten to state the real mechanism per door — READ COMMITTED doors converge on the lock alone; the app door'stransactSerializablefixes the snapshot at the lock statement, so convergence there is SSI (40001 on the loser) + the wrapper's retry, and the wrapper is load-bearing.fetchPresignedObjectabort listener (c6c4463): removed infinallywhen the fetch rejected before headers; kept after headers because the body still streams under the caller's signal. Unit test pins both halves.limit, an unknownstatus,search> 200 chars orcategory> 100 chars now answer 400 instead of being silently dropped — the contacts contract; the app only sends enum statuses and page sizes ≤ 200. Noted here since the commit message did not spell it out.sourcenowz.enum(CONTACT_SOURCES), name stored trimmed without an email fallback: the only caller passesconversationand already falls back to the address — intentional tightening, no behaviour change today.app.support_cases*: not filed as an issue from this branch; the ledger row is the tracking record (deprecate-then-delete once the previous image is out of rotation).