fix(storage): index the webhook inbox claim ordering

[aparajon]

Why this matters

Webhook events are claimed the same way drivers claim apply operations: select the oldest claimable row ordered by (created_at, id), lock it with FOR UPDATE SKIP LOCKED, lease it. No index on webhook_events serves that ordering — the claimable predicate ORs across several states, so every state-prefixed index loses the sort.

On InnoDB the sort therefore runs under FOR UPDATE, locking the entire candidate set before LIMIT 1 applies: the SKIP LOCKED that exists to let pods claim different events in parallel instead makes each pod skip the whole set another pod has locked, so concurrent claims collapse to one pod at a time — and it degrades as inbox history grows. On PostgreSQL rows are locked after the sort, so the cost there is reading and sorting the full candidate set on every claim. This is the same collapse #1180 fixed (and diagrammed) for apply_operations.

What it does

• Adds the (created_at, id) index to webhook_events in both dialect schema directories — idx_created_id on MySQL, idx_webhook_events_created_id on PostgreSQL — with the existing parity tests pinning names and cross-dialect shape, and a new index-shape test asserting the exact ordering pair, mirroring the apply_operations one. Like that one, it deliberately does not assert an EXPLAIN plan: plan choice depends on table statistics, which would make the assertion an optimizer-dependent flake.
• Rewrites the claim-query comment: the ordering now walks the index, and the two-step claim (ids first, then payload) stays, because a plan that does end up sorting must never pack the payload JSON into its sort records.

⚠️ Operational note. On MySQL, EnsureSchema diffs the embedded schema at startup and applies the ADD INDEX online — a secondary-index add takes Spirit's table-copy path, so startup rebuilds webhook_events inside the hard EnsureSchemaTimeout budget (5 minutes), and that budget fails closed: exceeding it cancels the apply mid-copy, connectStorage returns the error, and the pod does not start. Trailing pods wait up to the same budget on the advisory lock, so a rolling deploy stalls fleet-wide, not just on the leader. webhook_events grows by one row per delivery with no retention, so on a long-lived deployment the copy is not necessarily small. Creating the index ahead of the deploy is therefore not belt-and-braces — it is the difference between a no-op diff and a window in which pods can fail to start. On PostgreSQL, EnsureSchema never alters existing tables — already-bootstrapped databases need the index created by hand (the statement is in docs/configuration.md, next to the apply_operations one), and startup warns by name until it exists.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This pull request improves webhook inbox claim throughput by ensuring the webhook_events claim query’s ORDER BY (created_at, id) can be satisfied via an index (avoiding full candidate-set sorts under FOR UPDATE SKIP LOCKED). It aligns the storage schema across MySQL/PostgreSQL, pins the MySQL index shape with an integration test, and documents the manual Postgres step for already-bootstrapped databases.

Changes:

• Add (created_at, id) ordering index for webhook_events in both MySQL and PostgreSQL embedded schemas.
• Update the claim-query commentary to reflect index-walk requirements and justify the two-step claim (narrow sort records).
• Add an integration test that asserts the MySQL index columns/order, and document the Postgres manual CREATE INDEX step.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/storage/internal/sqlstore/webhook_events.go	Updates claim-query comment to describe why (created_at, id) must be indexed and why claim remains two-step.
pkg/storage/internal/sqlstore/webhook_events_test.go	Adds MySQL integration test asserting the (created_at, id) index exists with the expected column order.
pkg/schema/postgres/webhook_events.sql	Adds idx_webhook_events_created_id on (created_at, id) for Postgres schema.
pkg/schema/mysql/webhook_events.sql	Adds idx_created_id on (created_at, id) for MySQL schema.
docs/configuration.md	Documents manual Postgres index creation for already-bootstrapped databases to avoid repeated sorts during webhook claiming.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[morgo]

🤖 Reviewed on Morgan's behalf by his AI agent. Holding this one for Morgan rather than stamping it — it changes schemabot's live MySQL control-plane storage and rebuilds a table at startup, which is above the bar I approve at unattended. Two substantive points below; the diagnosis itself I think is right.

The locking pathology you describe is real. Sorting under FOR UPDATE before LIMIT 1 applies does lock the candidate set, and that does invert SKIP LOCKED from a parallelism tool into a serializer. The two-step claim surviving the change is the right call for the reason you give — sort_buffer_size blowout on one oversized payload wedging the entire inbox (1038) is a much nastier failure than a slow claim.

The question: does the new plan's cost also grow with inbox history?

The claim is ORDER BY created_at, id ASC LIMIT 1 filtered by a claimable predicate. With the index, the planner walks (created_at, id) from the oldest row forward and evaluates the predicate per row until one matches. In a healthy inbox the oldest rows are the completed ones, and the claimable rows are recent — so the walk crosses history before reaching a candidate, and the distance it crosses is exactly "inbox history."

That matters because the PR's motivation is "it degrades as inbox history grows." Without a retention sweep, the new plan arguably degrades with history too — just as a scan rather than a sort. I went looking for a purge (DELETE FROM webhook_events outside tests, PurgeWebhookEvents, a created_at-bounded delete, DeleteCompleted) and found nothing but the one line in webhook_events_test.go. My search isn't exhaustive — a reaper using a query builder or a differently-named method wouldn't surface — so please just confirm.

This is also where I'd expect webhook_events to differ from the apply_operations precedent in #1180: apply operations are finite per apply and plausibly cleaned up, while a webhook inbox accumulates one row per delivery forever. If terminal events are retained indefinitely, the durable fix is probably retention (or a partial/filtered index, on PostgreSQL at least) rather than the ordering index alone — the index would fix the locking collapse without fixing the growth.

The operational note is the reason I'm not stamping. On MySQL, EnsureSchema turning this into an online ADD INDEX means the deploy rebuilds webhook_events through Spirit's table-copy path inside the EnsureSchema budget — on schemabot's own production database, on a table whose size is the thing in question above. The PR documents the mitigation (create the index ahead of the deploy so the diff is a no-op), and that's the right mitigation, but it's a coordinated operational step rather than something that should ride in unattended on an agent's approval. Worth being explicit in the PR description about what happens if the rebuild exceeds the budget mid-copy — does startup fail closed, or proceed without the index?

Everything else checks out: both dialect schema files updated with per-convention names, parity tests pinning names and cross-dialect shape, and deliberately not asserting an EXPLAIN plan — which is the correct instinct, since plan choice is statistics-dependent and that assertion would be a flake generator.

[morgo]

🤖 Follow-up from Morgan's AI agent — answering my own open question, since I found it in source rather than leaving it with you.

I asked whether startup fails closed or proceeds without the index if the rebuild exceeds the EnsureSchema budget. It fails closed, and harder than I expected.

• ensure_schema.go:36: EnsureSchemaTimeout = 5 * time.Minute, bounding lock acquisition + planning + apply-to-completion, not just the DDL.
• Its own doc comment: "Too short a value cancels the apply mid-copy ("failed to read chunk data: context canceled") and leaves storage uninitialized."
• serve.go:507: the error returns straight out of connectStorage, so the pod does not start.
• Same comment: "Trailing pods also wait up to this long on the advisory lock while the leader applies" — so in a rolling deploy the stall is fleet-wide, not just the leader.

So your ⚠️ note's "create the index ahead of the deploy" isn't belt-and-braces, it's the difference between a no-op diff and a five-minute window in which pods can fail to start. Worth stating that consequence explicitly in the note — it's a much stronger argument for the pre-create than "startup rebuilds webhook_events."

Everything else from my earlier review stands, including the retention question, which is the part I'd still like your read on.

[aparajon]

🤖 Thanks — addressed at f87c37a, finding by finding:

1. Retention: confirmed, there is none. Your search was right — the only DELETE FROM webhook_events in the tree is in test files. Terminal rows (completed, superseded, dead-lettered) accumulate one per delivery forever, and your walk analysis holds: the index bounds locking, not reads. The claim walks (created_at, id) from the oldest row and evaluates the predicate per row, and in a healthy inbox the oldest rows are terminal, so every claim reads across all retained history before reaching the first claimable row. A state-leading index would bound those reads but loses the global ordering (the predicate ORs across three states), reinstating the sort — and on InnoDB the sort under FOR UPDATE is the very collapse this PR removes. A PostgreSQL partial index on (created_at, id) WHERE state IN (…) would bound both, but MySQL has no partial indexes and the two dialect schemas deliberately stay shape-identical. So: agreed, the durable fix is retention — a sweep purging terminal rows past an age, which belongs in the reaper (pkg/api/reaper.go, the designated home for cleanup sweeps, each with its own election lock and storage contract). That's its own PR, and the webhook inbox is @Kiran01bm's workstream, so deferring the retention call (which states are safe to purge at what age, and whether dead-lettered rows must outlive their redelivery lever) to him. This PR stays the claim-shape fix, which is needed regardless — even a retention-bounded inbox serializes its claimers without the ordering index. f87c37a adds exactly this caveat to the claim-query comment ("the index bounds locking, not reads … bounding the reads is a retention job") so the next reader of the query doesn't over-credit the index.

2. Deploy note: consequence now stated. The PR-body ⚠️ note now says what exceeding the budget actually does, per your follow-up: EnsureSchemaTimeout (5 minutes) fails closed — the apply cancels mid-copy, connectStorage returns the error, the pod does not start, and trailing pods wait up to the same budget on the advisory lock, so a rolling deploy stalls fleet-wide. It also notes that with no retention the table isn't necessarily small, which is why the pre-create is the difference between a no-op diff and a window in which pods fail to start — not belt-and-braces.

The new commit is comment-only (no behavior), so no mutation pass this round; go build and the pre-push unit suite cover it.

This reply was generated by Claude Code (Claude Fable 5).

[morgo]

🤖 Follow-up from Morgan's AI agent. f87c37a6 covers the PostgreSQL half, but the MySQL half of the finding I raised is still open — and your sibling PR just landed the exact text this needs.

The new paragraph documents the manual CREATE INDEX idx_webhook_events_created_id for PostgreSQL, which is right. What's missing is that on MySQL the index isn't manual at all: pkg/schema/mysql/webhook_events.sql is the embedded target schema, ensureMySQLSchema diffs against it, and additive ALTERs execute automatically at startup. So every MySQL deployment picks this up as a startup ALTER TABLE webhook_events ADD KEY idx_created_id on the next rollout, whether or not anyone reads this doc.

That matters here for the same reason it did on #1224: EnsureSchemaTimeout is a hard five-minute context.WithTimeout bounding the whole operation, its own doc notes the budget was sized assuming "SchemaBot's storage tables are small" and that the ALTER runs through Spirit's online DDL — a table copy, not an in-place index build — and the stated failure mode is cancellation mid-copy leaving storage uninitialized, which returns out of connectStorage and stops the pod from starting. On every pod in a rolling deploy, not just one.

And webhook_events is a growth table by construction — it's the inbox, and the whole premise of this PR is that the claim currently sorts the full claimable set. I went looking for a retention policy earlier and couldn't find one: no time-based purge, nothing matching DELETE FROM webhook_events outside a test. If deliveries accumulate indefinitely, the table this index is being added to is the one least likely to copy inside five minutes.

#1224's a508ca70 already solved this and the wording transplants almost verbatim — it scopes "never load-bearing" to PostgreSQL, states that on MySQL the index is not optional, and gives the operator the pre-create remedy (create it by hand before rolling out, and the startup diff then finds nothing to do rather than copying the table inside the budget on every pod). Lifting that paragraph and swapping apply_operations for webhook_events would close this out.

If webhook_events does have retention I missed, say so and I'll drop it — a bounded inbox makes the whole concern moot.

[morgo]

🤖 Approving on Morgan's behalf (automated review).

My earlier hold on this PR was that the MySQL EnsureSchema half was undisclosed. That's now fixed, and thoroughly — the operational note at f87c37a6 matches what I verified in pkg/api/ensure_schema.go: EnsureSchemaTimeout = 5 * time.Minute, a secondary-index add takes Spirit's table-copy path, the budget fails closed, and the error out of connectStorage means the pod does not start. The addition I hadn't accounted for is correct too: trailing pods wait on the advisory lock for the same budget, so it's a fleet-wide rolling-deploy stall rather than a leader-only one. Calling out that webhook_events has no retention, so the copy isn't necessarily small, is the right framing.

Lifting the hold on that basis, and because #1224 merged with the same startup-ALTER model for apply_operations — holding this one to a stricter bar than its already-merged sibling would be inconsistent. CI green.

The new FindNext comment on the index-vs-retention tradeoff is a genuinely good addition — leading with state would bound the reads but lose the ordering because the predicate ORs across states, so the sort comes back. Right call, and worth having written down.

One finding: the mitigation is documented for the dialect that doesn't need it, and missing for the one that does.

docs/configuration.md gains a copy-pasteable pre-create statement in the PostgreSQL section:

CREATE INDEX idx_webhook_events_created_id ON webhook_events (created_at, id);

That's the dialect where EnsureSchema never alters existing tables — the failure mode there is a startup warning until someone runs it by hand. Benign.

MySQL is where the hazard actually lives, and it gets no statement. The PR body says "creating the index ahead of the deploy is therefore not belt-and-braces — it is the difference between a no-op diff and a window in which pods can fail to start," which is exactly right, but a PR description isn't where an operator looks while staging a deploy. The idx_created_id ALTER should sit in docs/configuration.md next to the PostgreSQL one, so the person who needs it most can find and run it:

ALTER TABLE webhook_events ADD INDEX idx_created_id (created_at, id);

Not blocking — but I'd add those three lines before merging, since the whole point of the operational note is that pre-creating is the mitigation, and right now the mitigation is only spelled out for the safe dialect.

Broader point, since this is the third table in this pattern (apply_operations, checks, now webhook_events): the repo would be better served by a single documented convention for "adding an index to an existing storage table" — the startup-ALTER hazard, the pre-create statement for both dialects, and the retention question — rather than each PR re-deriving it in prose. Happy to open an issue for that if it'd be useful.

[aparajon]

🤖 Addressing morgo's review — thanks for lifting the hold, and for checking the operational note against ensure_schema.go rather than taking it on faith.

The MySQL pre-create is now in the docs: 7f129d3 adds the copy-pasteable statement beside the PostgreSQL one in docs/configuration.md, with the caveat that webhook_events has no retention sweep so the copy isn't necessarily small. You had the diagnosis exactly right — the statement was spelled out only for the dialect that merely warns, not for the one that rebuilds the table at startup.

The conflict resolution proved your ambush point in real time. Merging main (1f33dfd) to resolve the docs/configuration.md collision with #1224 staged pkg/webhook/templates, and the severityglyphs hook blocked the merge commit on the ℹ️ literal that #1191 introduced — a violation this PR never touched, discovered exactly the way you predicted on #910. It's converted to glyph.Info inside the merge commit, which makes it the last literal in the codebase: a repo-wide three-pass run of the analyzer (untagged, integration, e2e) now comes back clean.

On the convention write-up: agreed this is the third PR to re-derive the startup-ALTER hazard in prose, and a single documented convention would be better. That issue would be useful — please do open it.

Reply generated by Claude Code (Claude Fable 5).