feat(webhook): dispatch unlock commands through the durable inbox

[Kiran01bm]

Summary

WH-9b-iii of the durable-webhook workstream: schemabot unlock is the third issue_comment command routed through the durable inbox, joining apply and apply-confirm (#922). An acknowledged unlock now survives process restarts and transient infrastructure failures instead of dying with an in-process goroutine.

What

• The request path keeps its synchronous routing/usage gates and the ack reaction, then enqueues the delivery into the durable inbox instead of dispatching on a goSafe goroutine (durableIssueCommentCommandReady now accepts action.Unlock).
• The leased driver re-parses the stored comment and routes it onto unlockCommandCore (refactor(webhook): expose retry disposition for unlock commands #923); its (retry, err) disposition drives lease completion, scheduled retry, or terminal failure — identical to the apply/apply-confirm flow.
• unlockCommandCore gains a parent-context parameter: the synchronous wrapper passes context.Background() (unchanged behavior), while the driver passes its run context so lease loss or shutdown cancels in-flight work.
• Review hardening: releases are bounded to locks that predate the delivery's received-at (a re-driven or redelivered unlock can no longer release a lock acquired after the command was issued), lost release races (ErrLockNotFound/ErrLockNotOwned) are terminal-idempotent instead of retried, and an admitted-but-unrouted durable command is now Warn + metric instead of a silent Info swallow.
• Tests: unlock added to the queue-and-ack matrix; driver-through-core tests pin the no-locks terminal answer, the retryable lock-lookup failure, the freshness cutoff, and that cancelling the drive context stops in-flight lock releases; the refactor(webhook): expose retry disposition for unlock commands #923 disposition contract suite is updated for the new signature.

Why

Without durable dispatch, a deploy or crash after the 👀 ack silently drops the unlock — the user saw the acknowledgment but the locks stay held, and GitHub won't redeliver an acked webhook. With #922's driver and #923's disposition core both on main, this slice is pure wiring plus the freshness bound that durability makes necessary for a lock-mutating command.

Remaining WH-9b slices: rollback / rollback-confirm / controls, and the #806 lock-path disposition contract tests.

Before / after

Before:
  issue_comment (unlock) ── gates ── ack ── goSafe goroutine ── unlockCommandCore
                                              │
                                              └── deploy/crash → work lost

After:
  issue_comment (unlock) ── gates ── ack ── durable inbox row
                                              │
                                              └── leased driver ── unlockCommandCore(receivedAt)
                                                     │  (retry,err) disposition
                                                     ├── terminal → complete
                                                     └── retryable → re-drive (capped)

Deferred review follow-ups

• User-visible terminal comment when a durable delivery exhausts its attempt budget — pre-existing posture shared with apply/apply-confirm; tracked as PRF-8 in the review-findings tracker.
• Extract the shared durable-driver test harness before the next command slice copies it a fourth time — tracked as PRF-15 in the same tracker.

References

• WH-9b-i durable apply/apply-confirm driver: feat(webhook): dispatch apply and apply-confirm commands durably #922
• WH-9b-ii unlock disposition core: refactor(webhook): expose retry disposition for unlock commands #923
• Tracker: https://github.com/squareup/personal-kmuddukrishna/blob/main/design-stuff/schemabot-webhook-reliability-tracker/task-tracker.md

[Copilot]

Pull request overview

Routes schemabot unlock through the same durable issue_comment inbox/driver pipeline as apply and apply-confirm, ensuring acknowledged unlock requests survive process restarts and transient failures and are re-driven according to the core’s (retry, err) disposition.

Changes:

• Added durable-dispatch enqueue path for unlock in the issue_comment webhook handler and enabled durable-driver routing to unlockCommandCore.
• Updated unlockCommandCore to accept a parent context so durable-driver cancellation (lease loss/shutdown) can cancel in-flight unlock work while preserving request-path behavior.
• Extended the durable-driver and disposition contract tests to cover queued unlock, terminal no-locks behavior, and retryable lock-lookup failures.

Reviewed changes

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

File	Description
pkg/webhook/issue_comment.go	Enqueues unlock into the durable inbox when enabled; driver re-parses and routes unlock to unlockCommandCore; expands “durably dispatched command” eligibility.
pkg/webhook/apply_handlers.go	Threads a parent context into unlockCommandCore and preserves synchronous wrapper behavior by passing context.Background().
pkg/webhook/durable_issue_comment_test.go	Adds durable queue/ack coverage for unlock and new driver-through-core unlock tests for terminal and retryable outcomes.
pkg/webhook/unlock_error_contract_test.go	Updates unlock core contract tests for the new context-bearing unlockCommandCore signature.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 58cc5b7.

Verdict: ship-ready once two small findings are addressed — a silent-answer gap on a concurrency edge, and a documentation correction on what the freshness bound actually guarantees under redelivery. The wiring itself is correct and consistent with the apply/apply-confirm slices, and I verified the failure mode that would have been catastrophic here (a zero-time freshness cutoff) cannot occur.

Finding 1: when every matched lock vanishes concurrently, the acked command ends with no answer

The new ErrLockNotFound/ErrLockNotOwned branch continues past both the error collection and the per-lock success comment. For a partial race that's right — the released subset still gets its success comment (and the test pins it). But when every matched lock hits that branch, the loop posts nothing, releaseErrs is empty, and the core returns (false, nil): the delivery completes terminally and the user who saw the 👀 ack gets no comment at all. The code comment justifies the skip with "a concurrent unlock … got there first" — but its own second example, an apply's stale-lock cleanup, posts no unlock answer, so nobody answers. This is the same acked-command-must-answer principle this PR itself enforces on the unrouted-command path (Warn + metric instead of a silent Info). Suggested fix: track whether anything was released; if the loop ends with nothing released, nothing failed, and at least one already-gone target, post a small terminal answer ("the matched locks were already released").

Finding 2 (docs): redelivery refreshes the cutoff — the body and godoc overstate the invariant

The PR body says "a re-driven or redelivered unlock can no longer release a lock acquired after the command was issued", and the godoc says the bound holds "arbitrarily later via redelivery". Neither is what the code does: GitHub Redeliver reuses the delivery GUID, and reopenTerminalWebhookEvent resets received_at to the redelivery receipt (pkg/storage/mysqlstore/webhook_events.go:106). So a redelivered unlock can release locks acquired after the original command, as long as they predate the redelivery. The webhook_events.go doc hunk this PR touches has the same drift ("rather than … releasing a lock acquired after the original command"). The behavior itself is defensible — Redeliver is a deliberate operator action to re-run the command now, and the auth and active-apply gates re-run against current state — but this is the safety invariant of a lock-mutating command, so the docs must state the real bound: locks predating the latest delivery receipt; re-drives of one delivery keep the original cutoff, Redeliver refreshes it.

Observation (non-blocking): the cutoff compares two clocks

lock.CreatedAt comes from the storage DB's clock (column default on insert — the Acquire INSERT doesn't set it), while receivedAt is the webhook pod's time.Now(). If the DB clock runs ahead of the pod clock, a lock genuinely older than the command can be skipped with the stale-command message (fail-safe direction: nothing wrongly released, and the message tells the user to re-comment); if it runs behind, the bound just narrows toward pre-PR behavior, which is still a strict improvement. With NTP-managed skew this is sub-second noise against a bound whose races are human-scale, so it's fine — worth a one-line comment at most, or deriving received_at from the DB clock if a natural opportunity arises.

Verified solid: the cutoff can never be the zero time — Create defaults ReceivedAt to time.Now() when unset, received_at is in the shared column list, and the claim scan populates it back, so a driver always sees a real receipt time (a zero cutoff would have made durable unlock skip every lock as "newer" and release nothing); the freshness bound is sound against release-then-reacquire because Release/ForceRelease are DELETEs — a re-acquired lock is a new row with a new created_at; the gate ordering is right (freshness filter → stale-command answer → no-locks fan-out silence → act point → per-database actor authorization → fail-closed active-apply verification → releases); ErrLockNotFound vs ErrLockNotOwned are distinguished in the store after a zero-row DELETE, and both correctly converge with the command's goal; the cancelled-drive test proves the drive context reaches the core's storage calls and the claim is released for re-drive rather than completed; both driver drop paths (ready-check miss and unrouted default) now Warn with distinct metric statuses, closing the two-enumerations trap the code comment describes; the synchronous wrapper is behavior-identical (context.Background(), time.Now()); the queue-and-ack matrix, terminal/retryable dispositions, the force-unlock-vs-newer-CLI-lock case, and the freshness cutoff are all pinned by tests. Build, vet, and pkg/webhook + pkg/storage/... with -race green locally; CI green 32/32.

This review was generated by Claude Code (claude-fable-5).

[aparajon]

🤖 Approving per the adversarial review above — findings are small and non-blocking (an answer-gap on a narrow concurrency edge, and a doc correction on the redelivery cutoff). Stamped by Armand's agent (claude-fable-5).