Add idempotency key reclaim + tests#2950
Conversation
|
@colonelpanic8 is attempting to deploy a commit to the Hatchet Team on Vercel. A member of the Team first needs to authorize it. |
4ff9dde to
9cc407c
Compare
|
📝 Documentation updates detected! New suggestion: Document idempotency keys feature for workflow triggers Tip: Leave inline comments with |
9cc407c to
165ad17
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends workflow triggering to support idempotency keys end-to-end, adds server-side “reclaim” behavior for keys stuck on terminal workflow runs (with configurable TTL/recheck throttling), and expands repository tests/migrations to support the new last_denied_at tracking.
Changes:
- Add optional
idempotency_key/idempotencyKeyto workflow trigger APIs and SDK clients (Go/TS/Python) and propagate it to trigger options. - Implement idempotency key reclaim + deny throttling (
last_denied_at) in the Go repository, plus controller-side cleanup on terminal status updates. - Add migration/schema updates and expand repository tests (including concurrency and mixed-key scenarios); add Nix dev shell tooling.
Reviewed changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
sql/schema/v1-core.sql |
Adds last_denied_at column to idempotency key table for deny throttling. |
cmd/hatchet-migrate/migrate/migrations/20260205120000_v1_0_76.sql |
Migration to add/drop last_denied_at. |
api-contracts/workflows/workflows.proto |
Adds idempotency_key to v0 trigger request. |
api-contracts/v1/workflows.proto |
Adds idempotency_key to v1 trigger request. |
pkg/repository/trigger.go |
Implements key creation/claiming + reclaim-on-terminal + duplicate handling plumbing. |
pkg/repository/shared.go / pkg/repository/repository.go / pkg/config/* |
Threads new TTL/recheck config through repository construction and server config. |
pkg/repository/sqlcv1/* |
Adds new SQL queries for OLAP status reads, terminal fallback, and idempotency key operations/fields. |
internal/services/controllers/olap/* |
Deletes idempotency keys on terminal task/DAG status updates. |
internal/services/admin/* |
Accepts idempotency key in admin trigger APIs; maps duplicate-key to AlreadyExists. |
sdks/typescript/* |
Adds idempotencyKey in TS types/clients and regenerates protos; improves protoc generation script. |
sdks/python/* |
Adds idempotency_key plumbing in client and regenerates protos/stubs. |
pkg/repository/idempotency_trigger_test.go / pkg/repository/*test* |
Adds extensive idempotency tests and improves Postgres test harness reuse. |
flake.nix / flake.lock |
Adds Nix dev shell with Go/protoc/lint tooling. |
Comments suppressed due to low confidence (1)
pkg/repository/trigger.go:2180
idempotencyKeyToExternalIdsis a map keyed only by idempotency key, so if the incomingoptsslice contains the same idempotency key more than once, earlier entries are silently overwritten. Later, those overwritten opts will be skipped because their (key, externalId) pair won’t be present inkeyClaimantPairToWasClaimed, but the key also won’t appear induplicateKeys/denyUpdateKeys, so the caller gets a partial trigger with no explicit signal. Consider detecting duplicate idempotency keys within the same request and either (a) returning a deterministic error, or (b) including them induplicateKeys(and logging/updatinglast_denied_at) so the behavior is observable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
165ad17 to
c2f15b0
Compare
c2f15b0 to
8198092
Compare
|
📝 Documentation updates detected! New suggestion: Document idempotency keys feature for workflow triggers Tip: Enable auto-create PR in your Docs Collection to publish suggestions automatically 🤖 |
8198092 to
1f50106
Compare
024daaf to
5cf68f6
Compare
|
Hey @colonelpanic8 👋 Apologies for the delay here. We're currently in the middle of a release week but should have more capacity next week to give this a review |
138aaf7 to
ba36276
Compare
ba36276 to
4ed01ca
Compare
4ed01ca to
7874136
Compare
7874136 to
8e99535
Compare
|
hi @colonelpanic8 - closing this as folded into #4045. feel free to take a look there and let me know if you have thoughts! |
cool. how similar is the interface to what I had originally? do you know when this will get merged? |
It's somewhat similar, but there are important differences. One big one is that idempotency is configured on the task / workflow directly via a CEL expression that uses the input or additional metadata, as opposed to being passed at runtime (as this wouldn't work for events, webhooks, etc.). Feel free to take a look through the docs and examples there! Hoping to merge in the next day or two. |
|
@mrkaye97 Thanks for pointing me to #4045. I took a detailed look at the current implementation, and I’m pretty disappointed that this PR was closed as “folded into” #4045, because the behavior in #4045 does not actually subsume the behavior I implemented here. The distinction is key derivation versus key lifetime. #4045 adds a useful workflow-level mechanism for deriving a key from a CEL expression, but it implements fixed-window deduplication:
What I need is active-execution idempotency:
Those semantics cannot be reproduced by choosing a different fixed TTL. For example, suppose run A claims key K at 00:00, runs for two minutes, and completes at 00:02. A trigger at 00:01 should be rejected, but a trigger at 00:03 should be allowed immediately. With a 24-hour TTL, #4045 rejects the legitimate 00:03 trigger for almost a day. With a one-minute TTL, it can admit a duplicate while A is still running. No fixed TTL provides “for the lifetime of the active execution.” This matters concretely for Railbird. A streaming There are things I like in #4045: workflow-level CEL expressions, broader trigger-path coverage, and returning the existing run ID on collision are all improvements. But those are orthogonal to whether a claim is fixed-TTL or released on terminal status. I don’t think it is accurate to describe #2950 as folded into #4045 while the central lifecycle behavior is absent. After reviewing #4045’s current structure, this also does not look architecturally difficult to support as an opt-in policy. One possible design would be:
The terminal task/DAG hooks used by this PR still exist, and #4045 already records the claimant external ID and propagates the evaluated idempotency key onto runs, so the two designs appear compatible rather than mutually exclusive. The SQL must condition deletion on the claimant ID so an old run cannot delete a newer claim after TTL-based reuse. Would you be open to preserving this as an idempotency lifecycle option in #4045 or accepting a focused follow-up PR that layers terminal release/reclaim onto the new expression-based design? This behavior is the reason I implemented #2950, and fixed-window deduplication does not meet that requirement. |
Summary
last_denied_atand clean up keys on terminal status updateslast_denied_atplus expanded idempotency tests (including concurrency/mixed-key cases)Testing
CGO_ENABLED=0 go test ./pkg/repository -run Idempotency -count=1CGO_ENABLED=0 go test ./... -count=1nix develop -c golangci-lint run ./... --config .golangci.yml --new-from-rev=origin/mainNotes
20260205120000_v1_0_76.sqlbefore deploying server changes.