fix(platform): re-inline sub-rules on redeploy and rollback (silent corruption)#161
Closed
Pama-Lee wants to merge 6 commits into
Closed
fix(platform): re-inline sub-rules on redeploy and rollback (silent corruption)#161Pama-Lee wants to merge 6 commits into
Pama-Lee wants to merge 6 commits into
Conversation
The ReleaseExecutionStatus and ReleaseRequestStatus enums both include rollback_failed, but the CHECK constraints from 0008 never did. A failed rollback therefore could not persist its terminal status: the UPDATE hit the constraint, the error was swallowed, the execution stayed rollback_in_progress, and the worker re-claimed and re-ran the failing rollback every 2s poll, forever. Rebuild both CHECKs with the full status set the code writes.
The 0020 unique index guarantees at most one active execution per release request, but it cannot see across requests: every direct publish/redeploy mints a fresh internal request, so two concurrent publishes of the same ruleset (or a publish racing a governed execute) would still both roll out. Add a target-level guard - any active execution for the same project + ruleset + environment now returns 409 from both execute_release_request and the direct-publish enqueue path. Also map the 0020 unique-index violation to 409 instead of 500 for the loser of two concurrent executes of the same request.
The old safety net (fail_stuck_active_executions) was dead code: both bootstrap callers passed false, and enabling it would have been wrong anyway - it had no grace period and would kill in-flight rollouts on every restart, and its rollback_failed write predates the 0022 CHECK fix. Remove it. The poll loop already self-heals claimable executions (the session advisory lock releases when a dead worker's connection drops), but a terminal execution is not claimable, so a worker crash between settling the execution and settling the surrounding state stranded rows forever: - dispatched deployments whose execution already finished (studio polls them indefinitely) - executing requests whose latest execution already finished - executing requests that never got an execution row Add a 60s reconciliation task in the worker that repairs all three, with grace periods so it never races the worker's own settle writes. Completed-via-rollback executions are told apart from completed rollouts by their rollback_in_progress -> completed history row, so requests settle to rolled_back exactly as the worker would have written. Repairs are recorded in the request history and are idempotent, healing rows stranded before this task existed.
A rolling_out / rollback_in_progress execution is worker-claimable, and build_rolling_context / build_rollback_context hard-errored when a row they depend on was gone (a bound server pruned as stale >30min or deleted by an admin, the request, environment, draft, or rollback snapshot). The spawned worker task only logged that error and dropped its advisory lock without writing any status, so the execution stayed claimable and was re-claimed and re-failed every 2s poll forever. The reconciler could not help — it only repairs executions that already reached a terminal status. Distinguish a permanent missing dependency (SetupDependencyMissing, from a None lookup) from a transient store error (propagated, retried next poll). On a permanent failure, settle the execution terminally (Failed, or RollbackFailed for a rollback) along with its request and any direct-publish deployment row, instead of looping.
Three handlers took an id from the path and fetched by that id alone, after only a coarse auth check — and server ids are derivable (sha256 of the engine URL), so the ids are guessable across orgs: - get_server / get_server_metrics / get_server_health ignored the caller's claims entirely. Any authenticated user could read another org's engine (url, version, capabilities) and, via metrics/health, trigger a control-plane NATS RPC to it. Now an org-owned server requires the caller to be a member of that org; global (unowned) servers stay readable, matching delete_server's gating. - get_release_execution_for_request / list_release_execution_events checked the permission on the path org/project but then fetched by the unscoped release_id / execution_id. An admin in their own org could read another org's execution state and event stream with a guessed id. Both now resolve the release through the org+project scoped getter (404 on mismatch), and the events handler additionally verifies the execution belongs to that release. Also scope the engine-proxy fallback: when a project's environment has no bound server (the auto-created production env has none, so this is common), the fallback pool is now restricted to the caller's org plus deliberately-global servers, never another org's engine — previously it picked the most-recently-registered server anywhere, routing org A's eval traffic and ruleset payloads to org B's engine.
A direct publish stores the *authored* (un-inlined) studio draft in its deployment row, and rows predating inlining have no sub-rule graph either. Redeploy and rollback-to-version then dispatched that stored snapshot verbatim: its SubRule steps referenced a graph that wasn't there, and studio->engine conversion drops a dangling reference silently (ConvertError::SubRuleNotFound is defined but never returned). The 'same version' redeployed or rolled back to therefore ran with its sub-rule logic missing, changing decisions with no error surfaced. Re-inline the snapshot at both dispatch points before rolling it out. Inlining is idempotent (an already-inlined graph is skipped via the sub_rules.contains_key guard), so it heals old/direct-publish rows and no-ops on already-inlined ones (governed releases). Redeploy inlines before creating the deployment row so a now-missing sub-rule fails fast without orphaning a Dispatched deployment; rollback maps an unassemblable snapshot to SetupDependencyMissing so it settles as RollbackFailed instead of looping.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Collaborator
Author
|
Superseded by a re-parented PR onto current main (stacked-squash history conflict). Same change. |
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.
Fixes the highest-severity finding from the 2026-07-09 publish-path audit: redeploy and rollback-to-version silently drop a ruleset's sub-rule logic. Stacked on #160 (base =
fix/platform-cross-org-isolation); GitHub retargets tomainas the stack merges.The bug (silent data corruption)
A direct publish stores the authored (un-inlined) studio draft in its deployment row (
ruleset_draft.rs— the row is built fromdraft.draftbefore sub-rules are inlined), and rows predating inlining have no sub-rule graph either. Redeploy (redeploy) and rollback-to-version (build_rollback_context) then dispatched that stored snapshot verbatim:SubRulesteps reference a graph that isn't in the payload, andstudio→engineconversion drops a dangling sub-rule reference silently (ConvertError::SubRuleNotFoundis defined but never returned — the conversion builds the step with no existence check).So the "same version" you redeploy or roll back to runs with its sub-rule decision logic missing, changing outcomes on the engines with no error surfaced. The governed release path is unaffected (it inlines and freezes
target_ruleset_snapshotat request-creation time).The fix
Re-inline the snapshot at both dispatch points before rolling it out. Inlining is idempotent — an already-inlined graph is skipped via the
sub_rules.contains_keyguard ininline_sub_rules_with_manifest— so this:Details:
400) without orphaning aDispatcheddeployment; the new row also stores the inlined artifact.SetupDependencyMissing, so it settles asRollbackFailedvia the reconciler path from fix(platform): release-correctness — rollback_failed CHECK, concurrent-rollout guard, crash reconciler #158 instead of looping every 2s.Note on semantics
For an un-inlined stored snapshot there is no frozen sub-rule copy, so re-inlining pulls the sub-rule's current content — the best (and only) available, and strictly better than dispatching a dangling reference. Already-inlined (governed) snapshots keep their frozen content untouched.
Verification
cargo test -p ordo-platform(40 tests) +clippy --all-targetsclean. The idempotency the fix relies on is guaranteed by thecontains_keyguard (this crate has no DB-backed test harness to exercise inlining end-to-end; worth a cluster smoke-test: publish a ruleset with a sub-rule, redeploy it, confirm the engine still runs the sub-rule branch).