Repository navigation
feat(db): four additive migrations, and the delivery health, durability and retention they unlock - #63
Open
houko wants to merge 3 commits into
Open
feat(db): four additive migrations, and the delivery health, durability and retention they unlock#63houko wants to merge 3 commits into
houko wants to merge 3 commits into
Conversation
… losing a response event - Every finished response loaded every enabled workflow's full published definition and discarded most of it in memory. The denormalised trigger column narrows the candidate set in the query instead, served by the widened index. `matchWorkflowsForResponse` stays the sole authority on whether a workflow fires, so a column that has drifted from its definition can only cost a wasted fetch — never a run that should not have happened. - The workflow list counted every run ever recorded, once per row. The "Runs" column is rendered, so the count could not simply be dropped; it is one grouped query now. - A Redis blip dropped the response pipeline event silently: no webhook, no follow-up, no billing event, and nothing recording it. The job id is now derived from the event, the response and its updated timestamp, which makes a retry idempotent without any schema at all, and the outbox carries what genuinely needs durability.
…gers, pipeline durability and retention Every statement is additive and non-blocking. `Integration` gains three delivery-health columns; the `NOT NULL DEFAULT 0` is a constant default, which PostgreSQL stores as a missing value rather than rewriting the heap on every version this project supports — the lowest is 15, per `.squawk.toml`. `Workflow` gains a nullable `triggerSurveyId` with a partial, re-runnable backfill, and its index widens to a superset of the old one, so dropping the old loses no access path. `ResponsePipelineOutbox` is a new table. `Organization` gains two nullable retention windows. A null `triggerSurveyId` means "matches every survey", never "matches none". That is what makes the partial backfill safe: a row it did not reach — draft, disabled, a different trigger type, a malformed definition — still reaches the in-memory matcher exactly as before. Reading null as an empty match would silently stop those workflows firing. A null retention window likewise means keep forever: the sweep only visits organizations that have set one. Indexes are created without CONCURRENTLY for the reason the repository already records in an earlier migration: `prisma migrate deploy` sends the file as one multi-statement script, PostgreSQL runs it in an implicit transaction, and a concurrent build aborts with 25001. One unrelated line moved: `zod/organizations.ts` carried a type assertion on `isAISmartToolsEnabled` that `no-unnecessary-type-assertion` rejects. It is pre-existing, and only surfaces because the pre-commit hook runs type-aware rules over files a commit touches.
…nd classify the new table - Notion, Airtable, Google Sheets and Slack deliveries record their outcome against the new columns, and integrations get the retry the webhook path already had, reusing that mechanism rather than a second one. This only became honest work once Notion and Airtable started reading their HTTP responses: a health surface built before that would have reported a permanently broken integration as healthy. - Display and WorkflowRun grew without bound; only the AuthZed outbox was pruned. A batched sweep prunes both now, following that pruner's shape. The window is per-organization configuration rather than a constant, because how long a customer's data is kept is not a decision a job function gets to make, and null means keep forever — an organization that has configured nothing is never visited. - `ResponsePipelineOutbox` is classified in the authorization resource inventory. That guard exists so every new model has someone state what it is in the authorization model; this one is internal delivery infrastructure nobody is granted access to, like the AuthZed outbox beside it. Twelve fixtures across six files gained the new columns, which is the type system working rather than churn: `consecutiveFailures` is NOT NULL, so every stand-in for a persisted row has to carry it. In `organization/service.test.ts` the fixture also doubled as the expectation, and that no longer holds — `select` deliberately does not fetch the retention windows, so the mapper cannot return them. The expectation strips them rather than the query growing a column nothing reads.
🚨 PR Size WarningThis PR has approximately 1585 lines of changes (1519 additions, 66 deletions across 25 files). Large PRs (>800 lines) are significantly harder to review and increase the chance of merge conflicts. Consider splitting this into smaller, self-contained PRs. 💡 Suggestions:
📊 What was counted:
📚 Guidelines:
If this large PR is unavoidable (e.g., migration, dependency update, major refactor), please explain in the PR description why it couldn't be split. |
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.
What & why
Was: Every finished response loaded every enabled workflow's full published definition and threw most of it away. A Redis blip dropped the pipeline event with nothing recording it.
DisplayandWorkflowRungrew forever. An integration that kept failing looked exactly like one that kept working.Now: The candidate set is narrowed in SQL, the event survives a blip, both tables are swept on a configured window, and a failing integration says so.
Open gaps.Where to look
20260922131000_add_workflow_trigger_survey_id— the partial backfill, and why null must mean "all"enqueue-response-completed-runs.ts— the matcher stays the authorityretention-sweep.ts— batching, and what an unconfigured organization getsBreaking changes
Integration, one onWorkflow, two onOrganization, one new tableWorkflowindex[workspaceId, status][workspaceId, status, triggerSurveyId]DisplayandWorkflowRunpruned on a per-organization windowMigrations & env
20260922130000_add_integration_delivery_health,20260922131000_add_workflow_trigger_survey_id,20260922132000_add_response_pipeline_outbox,20260922133000_add_organization_retention_windows.triggerSurveyIdbackfill runs inside its migration and is bounded by the workflow tables, which hold one row per hand-authored workflow. It is guarded byIS NULL, so re-running it is safe.How this was tested
Rerun:
pnpm --filter @forma/web test --project=unit lib/organization/service.test lib/integration/service.test lib/authorization/resource-inventory.test instrumentation-jobs.test modules/workflows/lib/runner modules/response-pipeline/lib/outbox-drain.test lib/jobs/retention && pnpm --filter @forma/workflows testCoverage
Migration safety, stated per statement
Integration— oneALTER TABLE, oneACCESS EXCLUSIVElock for the catalog update, no rewrite. TheNOT NULL DEFAULT 0is a constant default, stored as a missing value since PostgreSQL 11; the lowest major this project supports is 15 (packages/database/.squawk.toml).Workflow— nullableTEXTadd (catalog only), then a backfill joiningWorkflowto aDISTINCT ONoverWorkflowVersion, both of which hold one row per authored workflow. Then the index swap.CONCURRENTLY, for the reason an earlier migration in this repository already records:prisma migrate deploysends the file as one multi-statement script, PostgreSQL runs it in an implicit transaction, and a concurrent build aborts with25001.ResponsePipelineOutbox— new table, so nothing to lock.lock_timeoutso a blocked run rolls back cleanly and can be re-run rather than queueing behind a long reader.Twelve fixtures across six files gained the new columns. That is the type system working, not churn —
consecutiveFailuresisNOT NULL, so every stand-in for a persisted row must carry it. One of those fixtures doubled as its own expectation;selectdeliberately does not fetch the retention windows, so the expectation strips them rather than the query growing a column nothing reads.Open gaps
squawkwas not run locally — it is configured in this repo but not installed here, so the per-statement safety above is reasoning over the SQL, not a tool's verdict. CI's judgement is the one that counts.Workflowindex swap.Response/Displaygaining their ownworkspaceId(a backfill across the largest table, which needs its own rollout plan), retiringSurvey.questions(a destructive drop that should span two releases), and the contacts trigram index (needsCREATE EXTENSION pg_trgm, which a self-hosted database user may not be able to grant). Each deserves its own PR.Risks
Workflowindex swap drops the old index in the same transaction that builds the new one. Past thelock_timeoutthe whole migration rolls back cleanly, but on a busy instance it may need a retry.triggerSurveyIdcan drift from the published definition if a future write path forgets it. The matcher covers correctness; what drifts is only how much the query narrows.Note
AI model used —
claude-opus-5[1m](Claude Code), reasoning effortunknown.