feat: serve release with unresolved configs - #379
Open
yuvrajjsingh0 wants to merge 1 commit into
Open
Conversation
|
Review changes with Changed Files
|
WalkthroughRelease serving now accepts ChangesExtended release properties
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServeRelease
participant Redis
participant Superposition
Client->>ServeRelease: Request release with extended=true
ServeRelease->>Redis: Read unresolved-properties cache
alt Cache miss
ServeRelease->>Superposition: Fetch filtered config and experiments
Superposition-->>ServeRelease: Return unresolved properties
ServeRelease->>Redis: Store unresolved properties
end
ServeRelease-->>Client: Return resolved release with optional unresolved properties
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@airborne_docs/docs/api-reference/overview.md`:
- Line 96: Update the TTL description in the overview documentation to state
that Redis fallback caching lasts seven days, matching
UNRESOLVED_PROPERTIES_TTL, instead of claiming a 24-hour TTL.
In `@airborne_server/src/organisation/application/dimension.rs`:
- Around line 145-150: Update the cohort-dimension, standard-dimension, and
update mutation flows in
airborne_server/src/organisation/application/dimension.rs at lines 145-150,
177-182, and 330-331: perform invalidate_unresolved_properties after each
weight_recompute() attempt, including when recompute returns an error, then
propagate the recompute error.
In `@airborne_server/src/organisation/application/properties.rs`:
- Around line 326-331: Update put_properties_schema_api and its
rollback_config_update flow so any rollback failure is propagated as an error
instead of merely logged, preventing a successful response when schema writes
may remain partially applied. Preserve the existing rollback handling for
successful rollbacks and ensure all run_fail_end paths (Operation, Join, Panic,
and MissingResult) surface rollback failure.
In `@airborne_server/src/release/utils.rs`:
- Around line 608-619: Update the fetch-and-cache flow around
fetch_unresolved_properties and the cache.set_ex call to prevent a
pre-invalidation response from being written after a workspace mutation. Add or
reuse a per-workspace generation/version check (or equivalent serialization with
invalidation), verify it immediately before writing, and skip the cache write
when the generation changed.
In `@smithy/models/release.smithy`:
- Line 373: Decide release serving is public and make the contract consistent:
in smithy/models/release.smithy lines 373-373 and 387-387, remove `@requiresauth`
from ServeRelease and ServeReleaseV2 while preserving public descriptions; in
airborne_docs/openapi/airborne.openapi.json lines 2093-2093 and 2195-2195,
retain public descriptions and add explicit security: [] overrides for both
operations so the generated contract does not inherit bearer authentication.
🪄 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 Plus
Run ID: 7417cc39-31e4-4220-a220-1acd0599195c
📒 Files selected for processing (9)
airborne_docs/docs/api-reference/overview.mdairborne_docs/openapi/airborne.openapi.jsonairborne_server/src/organisation/application/dimension.rsairborne_server/src/organisation/application/dimension/cohort.rsairborne_server/src/organisation/application/properties.rsairborne_server/src/release.rsairborne_server/src/release/types.rsairborne_server/src/release/utils.rssmithy/models/release.smithy
|
|
||
| ### Caching and freshness | ||
|
|
||
| The bundle is cached in Redis per (organisation, application), so a request with `extended=true` normally costs no extra Superposition round-trip. The cache is dropped explicitly whenever anything that can change the bundle changes: a release is **created, updated, ramped, concluded or discarded**, the **`config.properties` schema** is updated, or a **dimension or cohort** is created, updated or deleted (cohort definitions travel in `dimensions`). A 24-hour TTL backstops anything that bypasses those paths — for example editing the workspace in Superposition directly. If Redis is unavailable the server falls back to Superposition rather than dropping the bundle. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the Redis TTL statement.
UNRESOLVED_PROPERTIES_TTL is seven days, not 24 hours. The current text understates how long direct Superposition changes can remain stale. Either change the implementation to one day or document the seven-day TTL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@airborne_docs/docs/api-reference/overview.md` at line 96, Update the TTL
description in the overview documentation to state that Redis fallback caching
lasts seven days, matching UNRESOLVED_PROPERTIES_TTL, instead of claiming a
24-hour TTL.
Comment on lines
+145
to
+150
| crate::release::utils::invalidate_unresolved_properties( | ||
| &state, | ||
| &organisation, | ||
| &application, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalidate after every dimension mutation outcome.
Each dimension write completes before the separate weight_recompute() call. A recompute error returns before the new invalidation call, so the persisted dimension can leave the unresolved-properties cache stale.
airborne_server/src/organisation/application/dimension.rs#L145-L150: invalidate after the cohort-dimension recompute attempt, then return its error.airborne_server/src/organisation/application/dimension.rs#L177-L182: invalidate after the standard-dimension recompute attempt, then return its error.airborne_server/src/organisation/application/dimension.rs#L330-L331: invalidate after the update recompute attempt, then return its error.
📍 Affects 1 file
airborne_server/src/organisation/application/dimension.rs#L145-L150(this comment)airborne_server/src/organisation/application/dimension.rs#L177-L182airborne_server/src/organisation/application/dimension.rs#L330-L331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@airborne_server/src/organisation/application/dimension.rs` around lines 145 -
150, Update the cohort-dimension, standard-dimension, and update mutation flows
in airborne_server/src/organisation/application/dimension.rs at lines 145-150,
177-182, and 330-331: perform invalidate_unresolved_properties after each
weight_recompute() attempt, including when recompute returns an error, then
propagate the recompute error.
Comment on lines
+326
to
+331
| crate::release::utils::invalidate_unresolved_properties( | ||
| &state_for_cache, | ||
| &organisation, | ||
| &application, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 15 --glob '*.rs' \
-e '\b(run_fail_end|rollback_config_update)\s*\(' \
-e '\bTxnError::(Operation|Join|Panic|MissingResult)\b'Repository: juspay/airborne
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,120p'
echo "== locate properties transaction rollback candidates =="
fd -i 'properties|transaction|release' -e rs . | sed -n '1,200p'
echo "== search relevant identifiers =="
rg -n --glob '*.rs' \
'invalidate_unresolved_properties|run_fail_end|transaction::|rollback|TxnError|Operation|Join|Panic|MissingResult|release::utils' \
.
echo "== diff stat =="
git diff --stat || true
git diff -- airborne_server/src/organisation/application/properties.rs | sed -n '1,240p' || trueRepository: juspay/airborne
Length of output: 29207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== transaction.rs =="
cat -n airborne_server/src/organisation/application/properties/transaction.rs
echo "== properties.rs relevant section =="
sed -n '1,440p' airborne_server/src/organisation/application/properties.rs | cat -n
echo "== release/utils invalidation relevant section =="
sed -n '460,685p' airborne_server/src/release/utils.rs | cat -nRepository: juspay/airborne
Length of output: 37875
Rollback failures must not allow partial property mutations to remain applied.
run_fail_end calls rollback_config_update for Operation, Join, Panic, and MissingResult paths, so the success-only cache invalidation is not the primary concern here. However, rollback_config_update logs rollback failures but does not enforce the partial write is reverted. Add a rollback-failure path that returns an error from put_properties_schema_api or otherwise prevents returning success while schema writes may remain inconsistently applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@airborne_server/src/organisation/application/properties.rs` around lines 326
- 331, Update put_properties_schema_api and its rollback_config_update flow so
any rollback failure is propagated as an error instead of merely logged,
preventing a successful response when schema writes may remain partially
applied. Preserve the existing rollback handling for successful rollbacks and
ensure all run_fail_end paths (Operation, Join, Panic, and MissingResult)
surface rollback failure.
Comment on lines
+608
to
+619
| let fresh = fetch_unresolved_properties(state, workspace_name).await?; | ||
|
|
||
| if let (Some(cache), Some(key)) = (state.redis_cache.as_ref(), cached_key.as_ref()) { | ||
| if let Err(e) = cache.set_ex(key, &fresh, UNRESOLVED_PROPERTIES_TTL).await { | ||
| log::warn!( | ||
| "[UNRESOLVED PROPERTIES] cache write failed for {}/{}: {}", | ||
| organisation, | ||
| application, | ||
| e | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent stale cache writes after invalidation.
A request can miss the cache and start fetching before a mutation. The mutation can then delete the key, and this request can write its pre-mutation response at Line 611 after that delete. This restores stale unresolved properties for up to seven days.
Use a per-workspace generation/version check, or serialize fetch-and-invalidate operations, before writing the fetched value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@airborne_server/src/release/utils.rs` around lines 608 - 619, Update the
fetch-and-cache flow around fetch_unresolved_properties and the cache.set_ex
call to prevent a pre-invalidation response from being written after a workspace
mutation. Add or reuse a per-workspace generation/version check (or equivalent
serialization with invalidation), verify it immediately before writing, and skip
the cache write when the generation changed.
| } | ||
|
|
||
| /// Resolve and return the active release configuration for an application, given the caller's targeting dimensions. This is the endpoint the SDK calls at boot. Public — no auth token required. | ||
| /// Resolve and return the active release configuration for an application, given the caller's targeting dimensions. This is the endpoint the SDK calls at boot. Pass `extended=true` to also receive the unresolved `config.properties` bundle for client-side resolution. Public — no auth token required. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make the release-serving authentication contract consistent.
The descriptions state that these endpoints are public. The Smithy operations retain @requiresauth. The OpenAPI document also applies its global bearer-security requirement because these operations do not set security: [].
Decide whether release serving is public before exposing unresolved_properties. If it is public, remove the auth requirement and generate an explicit OpenAPI security override. If it requires authentication, correct the public-endpoint descriptions.
smithy/models/release.smithy#L373-L373: align theServeReleasedescription and authentication trait.smithy/models/release.smithy#L387-L387: align theServeReleaseV2description and authentication trait.airborne_docs/openapi/airborne.openapi.json#L2093-L2093: align theServeReleaseV2description with its effective security requirement.airborne_docs/openapi/airborne.openapi.json#L2195-L2195: align theServeReleasedescription with its effective security requirement.
📍 Affects 2 files
smithy/models/release.smithy#L373-L373(this comment)smithy/models/release.smithy#L387-L387airborne_docs/openapi/airborne.openapi.json#L2093-L2093airborne_docs/openapi/airborne.openapi.json#L2195-L2195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@smithy/models/release.smithy` at line 373, Decide release serving is public
and make the contract consistent: in smithy/models/release.smithy lines 373-373
and 387-387, remove `@requiresauth` from ServeRelease and ServeReleaseV2 while
preserving public descriptions; in airborne_docs/openapi/airborne.openapi.json
lines 2093-2093 and 2195-2195, retain public descriptions and add explicit
security: [] overrides for both operations so the generated contract does not
inherit bearer authentication.
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.
Summary by CodeRabbit
New Features
extendedparameter to return unresolved configuration, version details, experiments, and targeting data.Bug Fixes