Added check=true validation

[sougata-progress]

CHEF-34690

[Copilot]

🟡 Changes recommended

Unresolved atomicity, metric, error-handling, and test issues remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds check=true compatibility validation for channel-to-channel promotions, including conflict detection, locking, rollback handling, and integration tests.

Changes:

• Validates package dependency closures and returns HTTP 409 conflicts.
• Adds advisory locking and transactional promotion handling.
• Expands conflict, snapshot, rollback, and concurrency tests.

File summaries

File	Summary and final review comments
test/builder-api/src/channels.js	Adds validation and concurrency tests. Critical (1 vote): Configure requests to accept both 200 and 409 responses.
components/builder-db/src/models/channel.rs	Adds advisory locking for promotions.
components/builder-api/src/server/resources/channels.rs	Implements validation and transactional promotion. Critical (3 votes): Ensure all promotion paths share atomic locking/transaction handling. Moderate (3 votes): Preserve the channel-to-channel request metric. Moderate (1 vote): Preserve non-NotFound lookup errors.
components/builder-api/src/server/helpers.rs	Computes channel package dependency closures.

Review details

Suppressed comments (1)

components/builder-api/src/server/resources/channels.rs:354

• Channel::get(...).ok() turns every lookup error into None, not only NotFound. A database/query failure is therefore treated as an empty source closure, so the compatibility check can be skipped instead of returning the underlying error; preserve non-NotFound errors here.

            let source_channel_id = Channel::get(&origin, &ch_source, conn).ok().map(|c| c.id);

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

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

[Copilot]

🟡 Changes recommended

Three critical concurrency and error-handling issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved transaction error-handling and advisory-lock ordering issues could cause incorrect results or deadlocks.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

components/builder-api/src/server/helpers.rs:294

• The new compatibility closure depends on tdeps, but the added API tests only exercise a direct neurosis/testapp version conflict and a clean closure; none verifies that two otherwise-compatible heads are rejected when they bring different transitive dependency idents. A regression in this loop could therefore make check=true miss dependency conflicts. Add an integration case using a package with a dependency (for example the existing testapp3 fixture) and assert the dependency-only conflict returns 409.

    for pkg in &head_packages {
        idents.push(pkg.ident.clone());
        idents.extend(pkg.tdeps.iter().cloned());
    }

components/builder-api/src/server/resources/channels.rs:371

• This lock set does not cover package deletion: resources/pkgs.rs removes all origin_channel_packages rows for a package without acquiring the same advisory lock. A deletion can commit between the closure read and the later package-id read/write here, so check=true may validate a different membership set (or race the association insert) and return a false conflict or a non-atomic result. Coordinate package deletion with the same channel/package lock before relying on this transaction for check/write atomicity.

        // Serialize this whole read-then-write sequence (check and promote)
        // against every other path that can mutate either channel's package
        // membership -- bulk or single-package promote/demote -- so a
        // concurrent write can't land between this request's check and its
        // own write, on either the source or the target side. Held for the
        // duration of the transaction.
        lock_channels(&origin, ch_source.as_str(), ch_target.as_str(), conn)?;

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved issues remain around error propagation and incomplete locking coverage for concurrent deletions and package uploads.

Review details

Suppressed comments (3)

components/builder-api/src/server/resources/channels.rs:388

• ok() discards every Channel::get error, not just NotFound. A database/transaction error is therefore treated as an absent source and the code continues into the package lookup/write path, masking the original failure as a later generic error (and potentially changing the returned status). Match NotFound explicitly if an absent source is meant to remain an empty source, and propagate all other errors.

        let source_channel_id = Channel::get(&origin, &ch_source, conn).ok().map(|c| c.id);

components/builder-api/src/server/resources/channels.rs:359

• These advisory locks only coordinate callers that take the new lock; delete_channel and package deletion still remove channel memberships without it. A concurrent delete can remove the target after the compatibility check or before promote_packages, causing a rollback/error or a successful response whose result is immediately deleted, so the read/check/write atomicity claimed here is incomplete. Make membership/channel deletion take the same per-channel lock before relying on this concurrency guarantee.

        lock_channels(&origin, ch_source.as_str(), ch_target.as_str(), conn)?;

components/builder-api/src/server/resources/channels.rs:359

• These advisory locks do not cover the package-upload path: Package::create inserts/updates a package and then calls OriginChannelPackage::promote for unstable without acquiring lock_channel (builder-db/src/models/package.rs:558-588). An upload can add a source package after channel_package_closure runs but before the package list is read, so it can be promoted without compatibility validation. Acquire the same lock around that mutation (and cover other channel-membership deletion paths) before relying on this transaction for atomic validation.

        lock_channels(&origin, ch_source.as_str(), ch_target.as_str(), conn)?;

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite