Fix: build semver and duplicate entries for release id and versions#253
Fix: build semver and duplicate entries for release id and versions#253yuvrajjsingh0 wants to merge 1 commit intomainfrom
Conversation
|
Review changes with Changed Files
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces semantic versioning infrastructure for builds by adding version columns to the database schema, implementing retry mechanisms with advisory locking for build creation, adding Maven metadata synchronization, integrating Superposition RID updates, and establishing conflict-based error handling for version collisions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BuildService
participant Database as Database<br/>(Builds Table)
participant S3 as S3<br/>(Maven Metadata)
participant Superposition
Client->>BuildService: build(org, app, release_id, ...)
BuildService->>Database: Check existing build for release_id
alt Existing build found
BuildService-->>Client: Return cached build version
else No existing build
BuildService->>BuildService: Start retry loop (max 10 attempts)
loop Retry until success or max attempts
BuildService->>Database: Claim new version with UNIQUE constraints<br/>(org, app, major.minor.patch)
alt Unique constraint collision
BuildService->>Database: Fetch existing build for release_id
alt Build found
BuildService-->>Client: Return existing version
else Build not found
BuildService->>BuildService: Retry version claim
end
else Success
BuildService->>BuildService: Generate build artifacts
BuildService->>S3: update_maven_metadata()<br/>(read, merge, upload)
BuildService->>Superposition: update_superposition_build_rid()<br/>(set last_created_build_rid)
BuildService->>Database: Mark build as complete
BuildService-->>Client: Return new version
end
end
alt Previous build exists (non-forced)
BuildService->>BuildService: Spawn background build task<br/>(async, same function)
BuildService-->>Client: Return immediately
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 4
🤖 Fix all issues with AI agents
In
`@airborne_server/migrations/2026-02-13-094547_add_version_columns_to_builds/up.sql`:
- Around line 30-38: The migration creates unique indexes
builds_unique_release_idx, builds_unique_version_idx and
builds_unique_semver_idx but will fail if duplicate rows already exist; before
running these CREATE UNIQUE INDEX statements add a deduplication step that
removes duplicates for the grouping keys (organisation, application, release_id)
and (organisation, application, build_version) and (organisation, application,
major_version, minor_version, patch_version) — e.g. find rows with the same
group and delete all but the intended canonical row (keep by max(id) or newest
created_at), or alternatively write the migration to abort with a clear message
if duplicates exist so the duplicates can be cleaned manually; ensure the dedupe
targets the same columns used by builds_unique_release_idx,
builds_unique_version_idx and builds_unique_semver_idx and runs before the
CREATE UNIQUE INDEX statements.
- Around line 7-14: The backfill UPDATE uses split_part and CAST on
build_version and will fail on malformed strings; modify the UPDATE to only
touch rows where build_version matches a strict semver regex (e.g., add "AND
build_version ~ '^[0-9]+\\.[0-9]+\\.[0-9]+$'" to the WHERE clause) so
CAST(split_part(... ) AS INTEGER) is safe for
major_version/minor_version/patch_version, and separately ensure duplicate
(organisation, application, release_id) and (organisation, application,
build_version) tuples are cleaned or handled before running the CREATE UNIQUE
INDEX statements (dedupe or skip/create CONCURRENTLY after cleanup) to avoid
index creation failures.
In `@airborne_server/src/build.rs`:
- Around line 493-563: The update_maven_metadata function is labeled "Must be
called under advisory lock" but build() never acquires one, causing a
read-modify-write race on maven-metadata.xml; fix by acquiring a consistent
advisory lock in build() around the call to update_maven_metadata (derive a lock
key from org+app or another stable identifier), call your existing DB advisory
lock helper (acquire then release) before/after invoking update_maven_metadata,
and only proceed with the metadata update while the lock is held; alternatively,
if you prefer eventual consistency, remove the misleading comment and/or
implement S3 conditional writes (use If-Match/ETag with retry) inside
update_maven_metadata (in push_file_byte_arr) and document the chosen approach.
- Around line 579-619: When an existing build is found the code updates
Superposition via update_superposition_build_rid but returns early without
attempting to call update_maven_metadata; add a call to update_maven_metadata
(using the same context: &state, &workspace_name, &org, &app, &release_id and
any required dimensions/metadata args) before returning existing.build_version,
and mirror the error handling used for update_superposition_build_rid (log
errors with tracing::error and continue to return the build version) so a
previously failed maven metadata upload can be retried on this early-return
path.
🧹 Nitpick comments (3)
airborne_server/src/build.rs (3)
646-752: Retry loop increments onlypatchbut the initial candidate may also collide onrelease_id.The retry logic on unique violation (line 749) only bumps
version_candidate.patch += 1. This correctly handles version-number collisions. However, consider this scenario:
- Request A for release_id=X starts, queries latest version = 1.0.5, candidate = 1.0.6
- Request B for release_id=Y starts, queries latest version = 1.0.5, candidate = 1.0.6
- Request A inserts (org, app, 1.0.6, release_id=X) → success
- Request B tries (org, app, 1.0.6, release_id=Y) → unique violation on version
- Request B checks if release_id=Y exists → no → bumps to 1.0.7 → retries → success
This works correctly. The logic is sound. One minor concern: the
__UNIQUE_VIOLATION__sentinel string approach works but is fragile — if someone changes the string in one place but not the other, the retry silently breaks. Consider using a dedicated error variant or constant.Extract sentinel as a constant
+const UNIQUE_VIOLATION_SENTINEL: &str = "__UNIQUE_VIOLATION__"; + // In the insert match: - )) => Err(ABError::Conflict("__UNIQUE_VIOLATION__".to_string())), + )) => Err(ABError::Conflict(UNIQUE_VIOLATION_SENTINEL.to_string())), // In the check: - ABError::Conflict(msg) if msg == "__UNIQUE_VIOLATION__" + ABError::Conflict(msg) if msg == UNIQUE_VIOLATION_SENTINEL
1013-1051: Background build has no deduplication guard — multiple concurrent requests can spawn redundant background tasks.If several non-force requests arrive while
last_built_rid != release_id, each one will spawn a separatetokio::spawnbackground build. The retry loop inbuild()will handle the DB-level dedup via unique constraints, but redundant S3 uploads (zip/aar/pom/maven-metadata) will still execute, wasting resources and amplifying the maven metadata race condition.Consider using a concurrency-limiting mechanism (e.g., a
DashMaportokio::sync::Semaphorekeyed by(org, app, release_id)) to ensure at most one background build runs per release.
655-662: Usetry_from()instead ofas i32cast for safer version component conversion.SemVer fields are
u32, and casting toi32will silently wrap on overflow for values exceedingi32::MAX(2,147,483,647). While practically unlikely for version numbers, defensive casting with error handling is preferred:Suggested safe cast
- major_version: ver_insert.major as i32, - minor_version: ver_insert.minor as i32, - patch_version: ver_insert.patch as i32, + major_version: i32::try_from(ver_insert.major).map_err(|_| { + ABError::BadRequest("Version component exceeds i32 range".into()) + })?, + minor_version: i32::try_from(ver_insert.minor).map_err(|_| { + ABError::BadRequest("Version component exceeds i32 range".into()) + })?, + patch_version: i32::try_from(ver_insert.patch).map_err(|_| { + ABError::BadRequest("Version component exceeds i32 range".into()) + })?,
airborne_server/migrations/2026-02-13-094547_add_version_columns_to_builds/up.sql
Show resolved
Hide resolved
airborne_server/migrations/2026-02-13-094547_add_version_columns_to_builds/up.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@airborne_server/src/build.rs`:
- Around line 863-864: The call to update_maven_metadata leaves the DB row stuck
in BUILDING if it returns an error; handle that error explicitly: wrap
update_maven_metadata(&org, &app, &new_build_version, &state).await and on
Err(e) either perform the same cleanup used for artifact upload failures (call
the existing cleanup function that removes the build row / reverts state) or
mark the build READY and log a warning (call the same routine used to set build
status to READY, e.g., set_build_status/mark_build_ready with &state) and emit a
processLogger.warn or similar including the error; implement Option 2 (mark
READY + warning) if you prefer, but ensure the error path updates the build row
state (or deletes it) instead of leaving it in BUILDING.
🧹 Nitpick comments (3)
airborne_server/src/build.rs (3)
493-493: Misleading comment: "advisory lock" is not actually used.The comment states "Must be called under advisory lock" but the
build()function relies on UNIQUE constraint retries rather than PostgreSQL advisory locks. Consider updating the comment to reflect the actual concurrency mechanism, or acknowledge that concurrent maven metadata updates may result in last-write-wins behavior.📝 Suggested comment update
-/// Update maven-metadata.xml in S3. Must be called under advisory lock. +/// Update maven-metadata.xml in S3. +/// Note: Concurrent updates may result in last-write-wins; the UNIQUE constraint +/// on build versions ensures no duplicate version numbers are created.
678-810: Version claim loop looks solid with a minor improvement opportunity.The retry loop correctly handles UNIQUE constraint violations by re-querying the latest version and incrementing. The 5-second deadline prevents infinite loops.
Consider exponential backoff instead of fixed 10ms delay to reduce database pressure under high contention:
📝 Optional: Exponential backoff
version_candidate = increment_build_version(refreshed); attempt += 1; - tokio::time::sleep(std::time::Duration::from_millis(10)).await; + let backoff_ms = std::cmp::min(10 * (1 << attempt), 500); + tokio::time::sleep(std::time::Duration::from_millis(backoff_ms)).await; }
970-970: Inconsistent log level for errors.Lines 970 and 1003 use
info!to log failures when updating Superposition, but the messages describe errors. Consider usingerror!orwarn!for consistency with other error logging in this file.📝 Suggested fix
.map_err(|e| { - info!("Failed to update default config for build rid: {:?}", e); + error!("Failed to update default config for build rid: {:?}", e); ABError::InternalServerError(.map_err(|e| { - info!("Failed to create context override for build rid: {:?}", e); + error!("Failed to create context override for build rid: {:?}", e); ABError::InternalServerError(Also applies to: 1003-1003
yuvrajjsingh0
force-pushed
the
fix-build-semver
branch
from
97a270c to
4a5768f
Compare
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes