Move Edge App deploy orchestration to the server

[rusko124]

Issue: #2580

What

screenly edge-app deploy no longer orchestrates a deploy step by step. Two server endpoints do it instead:

POST /v3/edge-apps/{id}/deploy/preview	what would change, and which files the platform is still missing
POST /v3/edge-apps/{id}/deploy	settings, version, files and publish, in one transaction

Requires the server side from Screenly/Screenly#2558.

Why

The old flow read the manifest, diffed and wrote settings one at a time, created a version row, uploaded files against that revision, then published it. Two problems:

• Orphan revisions. A failure between creating the version and publishing it left a half-built revision behind, and nothing cleaned it up.
• Upload-then-decide. Files had to be uploaded against a revision that already existed, so the client couldn't know whether a deploy was needed until after it had uploaded everything.

How

Files upload as staged assets through the existing /v4/assets route — app_id set, app_revision left null — and the deploy that follows claims them. Nothing is created until the content is already there, so a failed deploy leaves no version behind.

The preview call also lets the CLI stop early. Three outcomes now:

Edge App successfully deployed. Revision: 8.
Settings updated. No new revision needed. Revision: 7.
Edge App is already up to date.

The last one returns straight after the preview, without posting a deploy at all.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[sergey-borovkov]

Reviewed the diff and ran cargo check --all-targets + cargo test on the branch (clean, 189 pass). The direction is good — the client shrinks a lot — but a few of the removed client-side guards look like they need either a server-side equivalent or to stay.

Must fix

1. Staged-asset polling is app-scoped, so one failed asset blocks every future deploy — src/commands/edge_app/app.rs:280

get_staged_processing_statuses now queries app_id=eq.{id}&app_revision=is.null&status=neq.finished. Staged assets keep app_revision = null until a deploy claims them, and nothing deletes them on failure.

If a file fails processing (status = "error"), wait_for_assets_processing returns AssetProcessingError, the deploy aborts, and the errored row stays staged forever. Every subsequent screenly edge-app deploy hits the same row and fails — even after the user deletes the offending file — with no CLI command to clear it. The same mechanism makes a stuck non-error/non-finished asset burn the full 1000 s wait, and lets two concurrent deploys of the same app observe each other's staging. The old revision-scoped query gave each deploy a fresh scope. Needs either a filter that identifies this deploy's assets, or cleanup of staged assets on failure.

2. The printed remediation command isn't valid CLI syntax — src/commands/edge_app/app.rs:205

The message says Re-run with --delete-missing-settings to remove., but cli.rs:393 declares delete_missing_settings: Option<bool>, so the flag requires a value:

$ screenly edge-app deploy --delete-missing-settings
error: a value is required for '--delete-missing-settings <DELETE_MISSING_SETTINGS>' but none was supplied

Should read --delete-missing-settings true.

3. #[serde(default)] on deploy_needed turns a response-shape mismatch into a silent no-op success — src/api/edge_app/deploy.rs:81

DeployPreview defaults every field and has no deny_unknown_fields. If /deploy/preview returns 200 with a body that doesn't carry deploy_needed (renamed, wrapped in an envelope, error payload served with 200), it deserializes to false, deploy() returns early at app.rs:208, the CLI prints "Edge App is already up to date." and exits 0. The user's changed files are never deployed and nothing signals failure. The field that gates the entire operation should be required.

4. The index.html guard was dropped with no client-side replacement — src/commands/edge_app/app.rs:188

ensure_edge_app_has_all_necessary_files and CommandError::MissingRequiredFile are gone. A file-entrypoint app whose directory has no index.html now sends a file_tree without it and proceeds to a full deploy instead of failing immediately. Unless the new endpoint rejects that, the user gets a published revision that won't load on the player. Can you confirm the server enforces it? Otherwise the local check is worth keeping — it fails in a second instead of after a full upload.

5. Removing VirtualIndexHtml can leave a remote-entrypoint deploy with an empty file tree — src/commands/edge_app/app.rs:187

That guard was added in 8dd2f8e with the comment "the backend rejects publishing a version with no asset signatures", precisely because screenly edge-app create --entrypoint <URL> writes no index.html — only screenly_inject.js. A remote-entrypoint app whose directory contains just screenly.yml (inject script deleted or ignored) now produces file_tree = {} and a deploy with zero assets, which is the case the guard existed to prevent. Same question: has the publish-side check been relaxed, or does this regress?

6. deploy no longer calls update_entrypoint_value, so entrypoint URIs stop propagating — src/commands/edge_app/app.rs:224

The only remaining caller is instance.rs:73 (edge-app instance update). For EntrypointType::RemoteLocal the value comes from instance.yml's entrypoint_uri, which isn't part of DeployPayload — the server can't derive it from the manifest. So: user edits entrypoint_uri, runs screenly edge-app deploy, the screenly_entrypoint setting is never updated and the player keeps loading the old URL. If this is intentional, it needs a doc/changelog note that instance update is now required.

Non-blocking

7. created also defaults to false — src/api/edge_app/deploy.rs:92. revision is required in the same struct but created isn't. If /deploy omits it, a successful new-revision deploy takes the second arm at cli.rs:928 and prints "Settings updated. No new revision needed. Revision: N." — the opposite of what happened. The user-facing message depends entirely on this field.

8. 60 s timeout on an endpoint that now does strictly more work — src/api/edge_app/deploy.rs:153. DEPLOY_TIMEOUT_SECONDS = 60 now covers settings, version, files and publish in one transaction; the old client flow had no single 60 s ceiling over that sequence (uploads used 3600 s). A large app whose transaction runs long returns CommandError::Request while the server may still commit — an outcome the CLI can't reconcile.

9. deploy_preview discards a 409's outstanding payload — src/api/edge_app/deploy.rs:106. post_deploy deliberately hands 409 bodies back to the caller, but deploy_preview rejects anything non-200 with a bare WrongResponseStatus(409). If preview ever answers 409 the user sees a naked status code instead of the "not uploaded: …; still processing: …" message that deploy()'s Conflict arm builds from the identical body.

[rusko124]

Thanks — this caught two things I'd have shipped. Fixed five, pushing back on three, and one needs your knowledge of the entrypoint feature.

Fixed

1. Staged-asset polling. You're right that the app-wide scope is the bug, and it's worse than a block: each retry adds a staged row while the errored one stays, so the pile grows for the full 7 days.

Fixed by scoping the poll to the ids this run uploaded, rather than by deleting anything. upload_single_asset already sent Prefer: return=representation and threw the body away — it now returns the created id, and get_processing_statuses filters id=in.(…). The app_revision=is.null filter is gone, since id subsumes it.

That covers all three sub-points: a stale errored row has a different id and stays invisible until the sweep collects it on schedule, a concurrent deploy's rows are invisible, and a foreign stuck asset can't burn the 1000 s wait. A file this run uploaded that fails still aborts immediately with its error text, which is the behaviour the check exists for.

2. Flag syntax. Correct — cli.rs:392 was Option<bool> under a plain #[arg], so clap demanded a value. Rather than fix the message, fixed the flag: num_args = 0..=1, default_missing_value = "true". --delete-missing-settings now works bare, and --delete-missing-settings true still works, so no existing invocation breaks. Plain ArgAction::SetTrue would have broken the value form — which is the only form that works in 1.2.2.

3 / 7. #[serde(default)] on deploy_needed and created. Both dropped, required now. Same class of bug: a defaulted field that gates the operation turns a shape mismatch into a silent success.

6. Entrypoint URI. Confirmed regression — store_global_entrypoint returns early for anything but REMOTE_GLOBAL, and REMOTE_LOCAL reads entrypoint_uri from instance.yml, which isn't in DeployPayload. update_entrypoint_value is called again, on both the post-deploy path and the early return. The second call is the one that matters: if only entrypoint_uri changed, deploy_needed is false, and without it the value would never propagate. Left it unrestricted rather than gating on RemoteLocal — for RemoteGlobal it's a redundant idempotent PATCH, and the branch costs more than it saves.

Pushing back

4. index.html. The server does enforce it — DeploySerializer.validate, serializers.py:135. But the consequence is inverted: preview and deploy share that serializer and preview runs first, so a file-entrypoint app without index.html fails on request one, before a byte is uploaded. It fails faster than the old local check, not after a full upload.

8. Timeout. The 3600 s is on file uploads and hasn't moved — upload_single_asset still uses it. /deploy uploads nothing: it's a JSON body of path → signature plus the manifest, and DB work on the server. App size grows the payload, not the transaction. The one thing that can stretch it is pg_advisory_xact_lock behind a concurrent deploy of the same app, which is seconds. 60 s is also the house value for the rest of the JSON surface (commands/mod.rs:205). Keeping it.

9. deploy_preview and 409. EdgeAppDeployPreviewView.post returns Response(preview_deploy(...)) unconditionally — there is no 409 path. And now that deploy_needed is required, an unexpected 200 body fails loudly rather than deserialising to a no-op. I'd rather not add conflict plumbing for a branch that can't fire.

Needs your call

5. Empty file tree. The constraint VirtualIndexHtml was working around doesn't exist on this path. Nothing in /deploy requires a minimum asset count — not DeploySerializer, not ensure_assets_processed (it only looks at rows still processing), not any constraint on internal.screenly_assets. That check lived in the publish endpoint this PR replaces, so the placeholder was guarding something that's gone.

An empty file_tree is also only reachable for a remote-entrypoint app whose directory holds nothing but screenly.yml — file-entrypoint is already rejected by the index.html check on the first request.

What I can't answer: does a zero-asset version behave on the player for a remote entrypoint? You wrote 8dd2f8e. If it's fine, this closes. If it isn't, I'd add the guard to DeploySerializer server-side rather than write a placeholder file into the user's directory.

Tests

Three deploy tests reworked — with id scoping, a deploy that uploads nothing no longer polls at all, so the 409 test lost its status mock and the processing-failure test needed a real upload to have something to poll.

Added test_deploy_when_local_entrypoint_uri_set_and_no_new_revision_needed_should_update_setting for #6, covering exactly the case the fix exists for: entrypoint_uri changed, deploy_needed false, setting still propagates. Verified it fails without the fix.

190 pass, clippy and nightly rustfmt clean.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[sergey-borovkov]

Re-reviewed cd9c0487. Verified on the branch: cargo build, cargo clippy --all-targets clean, 190 tests pass. Approving — nothing blocking left. One non-blocking item inline, plus two nits below.

Confirmed fixed

1. Asset polling. Scoping to the ids this run uploaded is the right shape — better than deleting rows, since it needs no cleanup path and is correct under concurrency for free. The empty-list early return matters too: a deploy that uploads nothing now skips the poll entirely instead of issuing a query that would match everything.

2. Flag. Fixing the flag rather than the message was the better call. Verified against the built binary — bare, true, false, and --delete-missing-settings --path X all parse, and --path isn't swallowed as the value.

3 / 7. Defaults. Both gone.

6. Entrypoint URI. Restored on both paths, and the early-return call is the one that matters. Agree on leaving it unrestricted — the branch costs more than the redundant PATCH. test_deploy_when_local_entrypoint_uri_set_and_no_new_revision_needed_should_update_setting is exactly the right test.

Pushbacks — all three accepted

4. You're right and I had it backwards: preview and deploy sharing the validation means it fails on request one, before an upload, which is faster than the local check was.

8. Fair — I conflated the upload timeout with the deploy call. A JSON body plus server-side DB work doesn't grow with app size, and matching the house value for the rest of the JSON surface is the right default.

9. Agreed, no conflict plumbing for a branch that can't fire.

5. Empty file tree — closing it

The guard was working around the publish-side rejection this PR replaces, not a player constraint. For a remote entrypoint the player loads the remote URL; the local tree isn't what renders, and screenly_inject.js reaches the device through js_injection rather than the asset tree. A zero-asset version is fine.

It's also narrower than it looks: create --entrypoint writes screenly_inject.js, and nothing excludes it — it isn't in the exclusion list in utils.rs:24, and no .ignore is written (app.rs:558 asserts that). So a fresh remote app has a one-file tree; empty requires the user to delete or ignore that file. Not worth a guard on either side. Drop it.

Nits — take or leave

• outstanding.pending is no longer waited on, so a file left processing by an interrupted earlier run now returns the 409 "still processing" instead of the CLI waiting it out. The message is clear and actionable, so this seems like the right trade rather than something to fix.
• update_entrypoint_value moved after api.deploy, so a failed setting PATCH leaves a published revision with a stale URL and a non-zero exit. It self-heals on re-run via the early-return path, which is arguably why it's fine.

[Copilot]

🟡 Changes recommended

A new warning path in EdgeAppCommand::deploy writes to stdout, which can interfere with machine-readable stdout consumers (notably the MCP tool response).

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

Review details

• Files reviewed: 17/17 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It rewires a core deploy workflow across multiple layers (CLI, command logic, API surface, and tests) and depends on new server behavior that should be validated by a human reviewer against real platform responses and edge cases.

Review details

• Files reviewed: 17/17 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The deploy flow currently trusts server-provided missing file paths for uploads without validating they’re safe relative paths within the edge-app directory, which can enable unintended local file exfiltration when using a custom server URL.

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

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/api/edge_app/deploy.rs:58

• If the server returns a 409 without any outstanding details, OutstandingFiles::fmt currently formats to an empty string, resulting in an unhelpful error like Deploy rejected: . Consider emitting a fallback message when no details are present.

• Files reviewed: 17/17 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The deploy payload can send a manifest id that disagrees with the actual app id being deployed to when an override is used, which can break server-side validation/behavior.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/commands/edge_app/app.rs:231

• The deploy payload serializes the manifest as-is, including id. If the caller supplies an app_id override (CLI arg/env) that differs from the manifest’s id, the request can send a mismatched manifest.id to the server while targeting /v3/edge-apps/{actual_app_id}/..., which can cause server-side validation errors or confusing behavior.
  src/api/edge_app/deploy.rs:168
• post_deploy logs the response body using response.text()? inside the error branch; if reading the body fails, this changes the returned error away from the intended status-based WrongResponseStatus. Read the body best-effort for logging so the original status error is preserved.
  src/cli.rs:939
• The deploy command now orchestrates a full deploy (preview/upload/deploy), but the CLI error text still says "Failed to upload Edge App", which is misleading for users (and for scripts parsing stderr).

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

[Copilot]

🔵 Needs a closer look

It fundamentally changes the Edge App deploy workflow and depends on new server behavior, making production impact high even though tests cover many paths.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/api/edge_app/deploy.rs:167

• In the non-OK response branch, debug!("Response: …", response.text()? ) can turn an otherwise clear WrongResponseStatus into a Request error if reading the body fails, since the log statement uses ?. Logging the body should be best-effort and not affect the returned error.

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

[Copilot]

🟢 Approval recommended

The new server-orchestrated deploy flow is consistently wired end-to-end with updated tests, with only a minor error-message robustness tweak suggested.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/api/edge_app/deploy.rs:60

• OutstandingFiles formats to an empty string when all lists are empty, which can yield an unhelpful error like Deploy rejected: if the server responds with {} / missing fields (the struct defaults everything). Consider providing a fallback message when parts is empty so the error is always actionable.

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

[Copilot]

🔵 Needs a closer look

Server-provided filenames/errors are interpolated into user-facing error strings without escaping, which risks terminal control-sequence injection and should be hardened before approval.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/api/edge_app/deploy.rs:44

• file.path and file.error come from the server and are embedded into a user-visible error string without escaping. This allows control characters/ANSI escapes/newlines from a malicious (or misconfigured) API base URL to be printed to the user’s terminal. Escaping these fields keeps the message safe while still informative.

This issue also appears on line 48 of the same file.

src/api/edge_app/deploy.rs:54

• missing/pending filenames in OutstandingFiles are server-provided and are joined directly into the displayed error string. Escaping these strings avoids terminal control-sequence injection and keeps error output one-line and predictable.

        let mut parts = Vec::new();
        if !self.missing.is_empty() {
            parts.push(format!("not uploaded: {}", self.missing.join(", ")));
        }
        if !self.pending.is_empty() {
            parts.push(format!("still processing: {}", self.pending.join(", ")));
        }

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

[Copilot]

🔵 Needs a closer look

It significantly changes the core deploy workflow and shifts correctness onto new server endpoints/semantics, so it warrants final human review despite the accompanying test updates.

Review details

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