feat: experiment idempotency#982
Conversation
|
Review changes with |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces request idempotency support for experiment creation by adding an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Database
Client->>Handler: POST /experiments<br/>(with Idempotency-Key header)
Handler->>Handler: Extract idempotency_key<br/>from CustomHeaders
Handler->>Database: Query for existing experiment<br/>with this idempotency_key
alt Experiment Found
Database-->>Handler: Return existing experiment
Handler-->>Client: 200 OK<br/>(existing experiment)
else No Experiment Found
Database-->>Handler: No result
Handler->>Database: Create new experiment<br/>(with idempotency_key)
Database-->>Handler: Return created experiment
Handler-->>Client: 200 OK<br/>(new experiment)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds experiment-creation idempotency to the experimentation platform by introducing an optional idempotency_key persisted on experiments and wiring it through request header extraction and the create handler.
Changes:
- Add nullable
idempotency_keycolumn + unique index for experiments (migrations + init SQL/templates + Diesel schema/model). - Extract
Idempotency-Keyfrom incoming HTTP requests intoCustomHeaders. - On experiment creation, return an existing experiment when the same
idempotency_keyis provided.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| workspace_template.sql | Adds idempotency_key column and unique index to workspace template schema. |
| docker-compose/postgres/db_init.sql | Adds idempotency_key column and unique index for local dev/test schemas. |
| crates/superposition_types/src/database/schema.rs | Updates Diesel schema for experiments.idempotency_key. |
| crates/superposition_types/src/database/models/experimentation.rs | Adds idempotency_key: Option<String> to the Experiment model. |
| crates/superposition_types/migrations/2026-04-24-000000_experiment_idempotency_key/up.sql | Migration to add column + unique index. |
| crates/superposition_types/migrations/2026-04-24-000000_experiment_idempotency_key/down.sql | Migration rollback to drop index + column. |
| crates/service_utils/src/service/types.rs | Extends CustomHeaders extractor to read Idempotency-Key. |
| crates/experimentation_platform/src/api/experiments/handlers.rs | Implements idempotency lookup and stores key on insert. |
| crates/context_aware_config/src/api/context/handlers.rs | Transaction call formatting cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }), | ||
| idempotency_key: header_val | ||
| .get("Idempotency-Key") | ||
| .and_then(|v| v.to_str().ok().map(String::from)), |
There was a problem hiding this comment.
Idempotency-Key is accepted as-is; an empty/whitespace value would be treated as a real key and could unintentionally de-duplicate unrelated requests (and block new creations due to the unique index). Consider normalizing/validating the header value (e.g., trim and treat empty as None or return a 400).
| .and_then(|v| v.to_str().ok().map(String::from)), | |
| .and_then(|v| v.to_str().ok()) | |
| .map(str::trim) | |
| .filter(|v| !v.is_empty()) | |
| .map(str::to_string), |
| header_val.to_str().map_or(None, |v| Some(v.to_string())) | ||
| }), | ||
| idempotency_key: header_val | ||
| .get("Idempotency-Key") |
There was a problem hiding this comment.
Header lookup here uses a different casing than the existing x-config-tags lookup. While HTTP headers are case-insensitive, using a consistent canonical form (e.g., lowercase) avoids confusion and aligns with how other custom headers are accessed in this codebase.
| .get("Idempotency-Key") | |
| .get("idempotency-key") |
There was a problem hiding this comment.
http crate's HeaderMap which normalizes all header names to lowercase internally
| pub change_reason: ChangeReason, | ||
| pub metrics: Metrics, | ||
| pub experiment_group_id: Option<i64>, | ||
| pub idempotency_key: Option<String>, | ||
| } |
There was a problem hiding this comment.
Adding the new idempotency_key field makes Experiment { ... } struct literals elsewhere require an explicit idempotency_key initializer. At least one existing constructor (crates/experimentation_platform/tests/experimentation_tests.rs’s experiment_gen) currently doesn’t set it and will no longer compile. Update those literals to include idempotency_key: None (or add a defaulting approach if you want to avoid touching many call sites).
| if let Some(ref key) = custom_headers.idempotency_key { | ||
| let existing: Option<Experiment> = dsl::experiments | ||
| .filter(dsl::idempotency_key.eq(key)) | ||
| .select(Experiment::as_select()) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .first(&mut conn) | ||
| .optional()?; | ||
| if let Some(exp) = existing { | ||
| return Ok(HttpResponse::Ok().json(ExperimentResponse::from(exp))); | ||
| } | ||
| } |
There was a problem hiding this comment.
The pre-check for an existing experiment by idempotency_key is not sufficient to guarantee idempotency under concurrency (or retries while the first request is still in-flight). Two requests can both miss this query, both execute the CAC bulk-operations side effects, and then the second insert will hit the unique index and return a 409 instead of returning the existing experiment; it can also leave duplicated/orphaned CAC contexts.
To make this truly idempotent, handle the unique-violation on insert by re-querying and returning the existing experiment (ideally checking the constraint name matches experiments_idempotency_key_idx), and consider “claiming” the idempotency key before performing external side effects (e.g., reservation row/table or advisory lock) so only one request performs CAC writes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/context_aware_config/src/api/context/handlers.rs (1)
284-285: Consider completing the formatting consistency across all transaction calls.The reformatting of this transaction call improves readability, but similar transaction calls at lines 715 and 1211 (
delete_handlerandweight_recompute_handler) still use the older single-line format. For complete consistency, consider applying the same multi-line formatting to those handlers as well.♻️ Suggested formatting for remaining handlers
For
delete_handler(around line 715):- let tags = parse_config_tags(custom_headers.config_tags)?; - let (config_version, deleted_ctx) = db_conn + let tags = parse_config_tags(custom_headers.config_tags)?; + let (config_version, deleted_ctx) = db_conn .transaction::<_, superposition::AppError, _>(|transaction_conn| {For
weight_recompute_handler(around line 1210):let last_modified_time = Utc::now(); - let config_version = - conn.transaction::<_, superposition::AppError, _>(|transaction_conn| { + let config_version = conn + .transaction::<_, superposition::AppError, _>(|transaction_conn| {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/context/handlers.rs` around lines 284 - 285, The transaction call formatting was updated to a multi-line style in the current handler; apply the same multi-line formatting to the other transaction invocations to keep consistency: update the conn.transaction calls inside delete_handler and weight_recompute_handler to use the same multi-line closure form (e.g., break the .transaction::<_, superposition::AppError, _>(|transaction_conn| { ... }) across multiple lines and indent the closure body exactly like the recent change) so all three handlers use the identical layout and indentation.crates/superposition_types/migrations/2026-04-24-000000_experiment_idempotency_key/up.sql (1)
3-3: Prefer a partial unique index for non-null idempotency keys.This index will also store every
NULLkey row. Since lookups and uniqueness are only meaningful when the key is present, use a partial index to reduce bloat/write cost.♻️ Suggested migration tweak
-CREATE UNIQUE INDEX IF NOT EXISTS experiments_idempotency_key_idx ON experiments(idempotency_key); +CREATE UNIQUE INDEX IF NOT EXISTS experiments_idempotency_key_idx +ON experiments(idempotency_key) +WHERE idempotency_key IS NOT NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/migrations/2026-04-24-000000_experiment_idempotency_key/up.sql` at line 3, Replace the current full unique index on experiments(idempotency_key) with a partial unique index that only indexes non-NULL keys to avoid storing NULL rows; update the migration so experiments_idempotency_key_idx is created as UNIQUE ON experiments(idempotency_key) WHERE idempotency_key IS NOT NULL, and ensure the corresponding down migration drops the same index name if present so up/down stay symmetric.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/experimentation_platform/src/api/experiments/handlers.rs`:
- Around line 165-175: The current read-before-write using
custom_headers.idempotency_key and dsl::idempotency_key is race-prone; instead
reserve the idempotency key atomically before calling CAC bulk operations by
inserting a lightweight reservation row (or using an INSERT ... ON CONFLICT DO
NOTHING / RETURNING pattern) into the experiments/idempotency table within the
same DB transaction that creates the Experiment; on conflict, query and return
the existing Experiment (Experiment / ExperimentResponse) and do not run CAC
side effects, and ensure the handler paths that currently use the pre-check (the
shown block and the similar block at lines ~387-397) are replaced to rely on
this atomic reservation/insert + conflict->fetch-and-return flow.
In `@crates/service_utils/src/service/types.rs`:
- Around line 266-268: The idempotency_key extraction must validate and bound
user input: trim the header string, reject empty values, and cap length (e.g.,
255 chars) before storing. Replace the current chain for idempotency_key with
logic that converts header_val.get("Idempotency-Key") to &str, .trim(),
.filter(|s| !s.is_empty()), then .map(|s|
s.chars().take(MAX_IDEMPOTENCY_KEY_LEN).collect::<String>()), where
MAX_IDEMPOTENCY_KEY_LEN is a defined constant; this ensures very large or blank
keys become None or are truncated to a safe length while keeping the same field
name idempotency_key.
---
Nitpick comments:
In `@crates/context_aware_config/src/api/context/handlers.rs`:
- Around line 284-285: The transaction call formatting was updated to a
multi-line style in the current handler; apply the same multi-line formatting to
the other transaction invocations to keep consistency: update the
conn.transaction calls inside delete_handler and weight_recompute_handler to use
the same multi-line closure form (e.g., break the .transaction::<_,
superposition::AppError, _>(|transaction_conn| { ... }) across multiple lines
and indent the closure body exactly like the recent change) so all three
handlers use the identical layout and indentation.
In
`@crates/superposition_types/migrations/2026-04-24-000000_experiment_idempotency_key/up.sql`:
- Line 3: Replace the current full unique index on experiments(idempotency_key)
with a partial unique index that only indexes non-NULL keys to avoid storing
NULL rows; update the migration so experiments_idempotency_key_idx is created as
UNIQUE ON experiments(idempotency_key) WHERE idempotency_key IS NOT NULL, and
ensure the corresponding down migration drops the same index name if present so
up/down stay symmetric.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bae818a-8da8-4047-b535-abc83b325724
📒 Files selected for processing (9)
crates/context_aware_config/src/api/context/handlers.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/service_utils/src/service/types.rscrates/superposition_types/migrations/2026-04-24-000000_experiment_idempotency_key/down.sqlcrates/superposition_types/migrations/2026-04-24-000000_experiment_idempotency_key/up.sqlcrates/superposition_types/src/database/models/experimentation.rscrates/superposition_types/src/database/schema.rsdocker-compose/postgres/db_init.sqlworkspace_template.sql
| if let Some(ref key) = custom_headers.idempotency_key { | ||
| let existing: Option<Experiment> = dsl::experiments | ||
| .filter(dsl::idempotency_key.eq(key)) | ||
| .select(Experiment::as_select()) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .first(&mut conn) | ||
| .optional()?; | ||
| if let Some(exp) = existing { | ||
| return Ok(HttpResponse::Ok().json(ExperimentResponse::from(exp))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Current idempotency flow is race-prone and can duplicate external side effects.
The read-before-write check is non-atomic. Concurrent requests with the same key can both pass the lookup, both call CAC bulk operations, and then one fails on DB unique conflict. That leaves side effects already executed and breaks idempotent-create guarantees.
Please make key reservation atomic before CAC side effects (e.g., dedicated idempotency lock/reservation row or equivalent serialized flow), and ensure conflict paths return the already-created experiment instead of surfacing a conflict.
Also applies to: 387-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/experimentation_platform/src/api/experiments/handlers.rs` around lines
165 - 175, The current read-before-write using custom_headers.idempotency_key
and dsl::idempotency_key is race-prone; instead reserve the idempotency key
atomically before calling CAC bulk operations by inserting a lightweight
reservation row (or using an INSERT ... ON CONFLICT DO NOTHING / RETURNING
pattern) into the experiments/idempotency table within the same DB transaction
that creates the Experiment; on conflict, query and return the existing
Experiment (Experiment / ExperimentResponse) and do not run CAC side effects,
and ensure the handler paths that currently use the pre-check (the shown block
and the similar block at lines ~387-397) are replaced to rely on this atomic
reservation/insert + conflict->fetch-and-return flow.
There was a problem hiding this comment.
This will be resolved post the introduction of advisory lock - writes will be serialized. There won't be any race conditions.
There was a problem hiding this comment.
@sauraww, understood — if an advisory lock is acquired per idempotency key (e.g., via pg_advisory_xact_lock keyed on a hash of the idempotency key) at the start of the handler, concurrent requests for the same key will be serialized and the current read-before-write pattern becomes safe. That's a solid plan.
One thing to keep in mind when implementing the advisory lock: the lock should be taken before the initial idempotency-key lookup so that a concurrent in-flight creation is fully committed (and visible to the subsequent read) by the time the second request proceeds past the lock.
I'll note this for future reviews.
✏️ Learnings added
Learnt from: sauraww
Repo: juspay/superposition PR: 982
File: crates/experimentation_platform/src/api/experiments/handlers.rs:165-175
Timestamp: 2026-04-25T07:51:36.939Z
Learning: In `crates/experimentation_platform/src/api/experiments/handlers.rs`, the non-atomic read-before-write idempotency check in `create_handler` (checking `dsl::idempotency_key` before CAC side effects) is intentionally left as-is and will be made safe by the future introduction of a PostgreSQL advisory lock that serializes concurrent writes for the same idempotency key. Do not flag this as a race condition until the advisory lock mechanism is in place.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ayushjain17
Repo: juspay/superposition PR: 951
File: crates/superposition_provider/src/local_provider.rs:271-336
Timestamp: 2026-04-01T17:53:15.684Z
Learning: In `crates/superposition_provider/src/local_provider.rs`, the `do_refresh()` method intentionally allows `cached_config` and `cached_experiments` to be updated independently (potentially out of sync). Temporary divergence between the two caches is an accepted trade-off by design — do not flag partial/non-atomic cache updates in this file as an issue.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:53.153Z
Learning: Deprecate jsonlogic-based context conditions and migrate to simple key-value pair map conditions across the codebase. Replace jsonlogic::apply usage with superposition_types::apply for condition evaluation. Update all relevant modules (notably Rust files under crates) to use the new approach, remove jsonlogic dependencies where applicable, and run full compilation and tests to catch regressions. Ensure context evaluation logic consistently uses key-value maps and that the architectural change is reflected in unit/integration tests.
| idempotency_key: header_val | ||
| .get("Idempotency-Key") | ||
| .and_then(|v| v.to_str().ok().map(String::from)), |
There was a problem hiding this comment.
Validate and bound Idempotency-Key before accepting it.
This value is user input and gets persisted/indexed. Without length/emptiness checks, very large or blank keys can cause avoidable DB/index failures and noisy retries.
🛡️ Suggested extractor hardening
- let val = CustomHeaders {
+ let idempotency_key = match header_val.get("Idempotency-Key") {
+ Some(v) => {
+ let raw = v
+ .to_str()
+ .map_err(|_| error::ErrorBadRequest("Invalid Idempotency-Key"))?
+ .trim();
+ if raw.is_empty() || raw.len() > 255 {
+ return ready(Err(error::ErrorBadRequest(
+ "Idempotency-Key must be between 1 and 255 characters",
+ )));
+ }
+ Some(raw.to_owned())
+ }
+ None => None,
+ };
+
+ let val = CustomHeaders {
config_tags: header_val.get("x-config-tags").and_then(|header_val| {
header_val.to_str().map_or(None, |v| Some(v.to_string()))
}),
- idempotency_key: header_val
- .get("Idempotency-Key")
- .and_then(|v| v.to_str().ok().map(String::from)),
+ idempotency_key,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/service_utils/src/service/types.rs` around lines 266 - 268, The
idempotency_key extraction must validate and bound user input: trim the header
string, reject empty values, and cap length (e.g., 255 chars) before storing.
Replace the current chain for idempotency_key with logic that converts
header_val.get("Idempotency-Key") to &str, .trim(), .filter(|s| !s.is_empty()),
then .map(|s| s.chars().take(MAX_IDEMPOTENCY_KEY_LEN).collect::<String>()),
where MAX_IDEMPOTENCY_KEY_LEN is a defined constant; this ensures very large or
blank keys become None or are truncated to a safe length while keeping the same
field name idempotency_key.
mahatoankitkumar
force-pushed
the
feat/idempotency
branch
from
6e2f0b3 to
eb3ff7b
Compare
|
@mahatoankitkumar - we need this change in Smithy as well ? |
| header_val.to_str().map_or(None, |v| Some(v.to_string())) | ||
| }), | ||
| idempotency_key: header_val | ||
| .get("Idempotency-Key") |
There was a problem hiding this comment.
adding to Co-pilots suggestion here. We should match against lower-case of the headers and ensure it is lower-cased by the time they reach the application. Does that happen automatically ? Could you check @mahatoankitkumar ?
There was a problem hiding this comment.
Yes its happens automatically, but i'll change it just for consistency
mahatoankitkumar
force-pushed
the
feat/idempotency
branch
2 times, most recently
from
c3e3256 to
28fae45
Compare
mahatoankitkumar
force-pushed
the
feat/idempotency
branch
from
28fae45 to
c9aa6bb
Compare
This pull request introduces idempotency support for experiment creation in the experimentation platform. The main change is the addition of an optional
idempotency_keyfield to theexperimentstable and related application logic, which ensures that duplicate experiment creation requests with the same key are handled safely and return the existing experiment instead of creating a new one. There are also supporting changes to database schema, HTTP request handling, and transaction code cleanup.Experiment Creation Idempotency:
idempotency_keyfield to theExperimentmodel and database schema, including migrations and index creation to enforce uniqueness. [1] [2] [3] [4] [5] [6]idempotency_keyand return it if found, preventing duplicate experiments. [1] [2]HTTP Request and Header Handling:
CustomHeadersstruct and its extraction logic to support reading theIdempotency-Keyfrom incoming HTTP requests, making the key available to handlers. [1] [2]Code Quality and Consistency:
OptionalExtensionto support optional query results.Summary by CodeRabbit