Move deployment lifecycle server-side#975
Conversation
Deployment tracking used to be babysat entirely by the CLI: the client created the record with a "pending-build" placeholder, drove it through phases, patched in the real version after the build, and marked it active. The server was a dumb CRUD store behind that, so a client crash could strand a record, and the "lock" that stopped two deploys colliding was a list-then-create with no transactional guarantee plus a 30-minute timeout. This introduces deploylifecycle as a standalone package so both the deployment server and the build server can own the record instead of the client. It centralizes the previously-scattered rules into one place: - an explicit transition() table for the status state machine, with an init check that fails fast if a status is ever added without deciding whether it is terminal - an atomic deploy lock, a new deployment_lock entity acquired via a compare-and-create so two racing deploys cannot both win, with expiry and terminal-state stealing to reconcile drift - indexed queries to replace the O(n) full scan that backed every history read, lock check, and activation - a Tracker (Begin/SetPhase/SetAppVersion/Activate/Fail/Cancel) that is the single surface both build paths call The lock and the queries are scoped to the app, not app+cluster. A coordinator's entity store is a loopback into its own etcd, so it only ever holds this cluster's deployments, and the client-supplied cluster_id is unreliable (a manual deploy sends the cluster name, a CI/OIDC deploy sends the raw address). Keying on it would let those two deploys of the same app run concurrently. This carries forward the reasoning MIR-1465 established for the read path. It has no callers yet; the wiring lands in the next commit.
With the deploylifecycle package in place, this hands ownership of the deployment record to the servers that actually do the work. The build server now owns the record for a build. A new DeployRequest parameter on buildFromTar/buildFromPrepared is the ownership signal: when present, the server creates the record, advances its phases as the build runs, and settles it at the end; when absent, it touches no records at all. That absence is deliberate. It is exactly what an older CLI sends, so an old client keeps driving its own record and there is never a second one for the same deploy. Both build paths are wired: the plain path calls the tracker directly, and the saga path adds three compensating actions (begin/record-version/activate) ordered by data dependency. The deployment id is streamed back to the client on a new status arm so it can display and cancel a deployment it did not create. The deployment server is rebuilt on the same package. The three duplicated list-then-create lock blocks are gone, the state transitions are centralized, and the "pending-build" / "failed-<id>" sentinels are dropped (normalized away on read for legacy records). Rollback and env-var deploys go through the tracker too. The five old client-driven RPCs stay wired, reimplemented over the package and marked deprecated, and a new GetDeployLock lets the CLI pre-flight the lock so a blocked deploy fails fast with a rich message instead of after a wasted upload. The CLI becomes a viewer. It gates on whether the server advertises the new parameter: against a new server it sends the DeployRequest and reads the record off the stream, and against an old one it falls back to the legacy client-driven path unchanged, so this release's CLI still works against last release's server. Care is taken so a settle that fails after the version is already live (most often a client disconnect at activation) still releases the lock, rather than stranding the record in_progress and blocking the app's next deploy for the full lock TTL. Closes MIR-681
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe change introduces server-owned deployment lifecycle tracking across build and deployment RPCs. It adds deployment progress and request models, deployment-lock persistence, lifecycle state validation, revision-safe storage, app-scoped lock acquisition, and tracker operations. Build sagas and status streams create, update, and settle server-owned deployments. Deployment APIs and CLI paths add lock checks, lifecycle handoff, failure handling, progress propagation, and history normalization. Unit, integration, cross-version, blackbox, and fixture tests cover these behaviors. Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit:
Review
This is a significant, well-thought-out change that replaces a racy "list then create" deployment concurrency check with a proper compare-and-swap locking scheme. The core design is sound, and the test suite is genuinely thorough — concurrent lock tests, race hooks that exercise the read-write window, FailIfUnsettled as a defer-safe API, init() as a compile-time state-machine consistency check. These are real engineering choices, not perfunctory coverage.
I have one concrete concern and one design point worth noting.
Two separate Tracker instances, same store
Builder creates deploylifecycle.NewTracker(log, eas) and DeploymentServer creates its own deploylifecycle.NewTracker(log, eac). Both talk to the same entity store under the hood, and the struct comment in DeploymentServer acknowledges this ("both acquire the same app-scoped lock"). This is fine for the lock — the entity-store CAS makes that correct regardless. But Tracker exposes its internal clockFn (the now field) as a package-internal test seam, and the two live instances are independently configured, which is slightly surprising for operators reading the code.
More concretely: UpdateDeploymentStatus in DeploymentServer still does its own direct d.EAC.Put rather than calling through d.tracker.update. That means it bypasses the Tracker's read-modify-write loop and just does a single Get + Put at a fixed revision. A race between two concurrent UpdateDeploymentStatus calls (or between one of those and a build-side SetPhase) will still silently win last-write-wins, same as before. This is the deprecated client-driven path, and the comment at releaseDeployLock acknowledges it. But it's worth being conscious that only the lock release is threaded through Tracker; the record write is not.
The "lock but not record" window in Begin
In Tracker.Begin, a record is created first, and then the lock is acquired. If the lock Acquire fails, discard is called to remove the record. If discard itself fails (network hiccup), the record is leaked in in_progress with no lock — it will sit there forever as apparent evidence of a stuck deployment. This is noted in the discard log message, but the orphaned record will show in ListDeployments and GetDeployLock (as stealable because the holding deployment is gone). The practical impact is low — stealable returns true if the record is gone, so the next deployment won't be blocked — but a leaked in_progress record in history is noisy. A migration path (e.g., a periodic sweeper, or doing the lock acquire before the record create) would close this gap cleanly. The current design is fine to ship, but worth flagging.
Minor: UpdateDeploymentStatus still inline-validates phase list
In UpdateDeploymentStatus, status validation was unified with deploylifecycle.ParseStatus, which is good. But UpdateDeploymentPhase still uses its own inline validPhases map (lines ~280–289) rather than deploylifecycle.ParsePhase. This is a pre-existing inconsistency made slightly more visible by the new pattern — not a blocker, but easy to fix.
🍪 full review note · comment /biscuit review to run biscuit again.
Inline comments
servers/deployment/server.go:289
UpdateDeploymentPhase validates the phase inline with a local validPhases map, while the new UpdateDeploymentStatus correctly delegates to deploylifecycle.ParseStatus. This creates two sources of truth for valid phases — if a phase is ever added to allPhases in lifecycle.go, this handler will silently accept the old set only. Consider replacing the inline map with deploylifecycle.ParsePhase(newPhase) to match the pattern established one method above.
🤖 Prompt for AI Agents
In servers/deployment/server.go around lines
280-289, UpdateDeploymentPhase validates the phase
using an inline local map. Replace the local map
and the if-check with a call to
deploylifecycle.ParsePhase(newPhase), returning
its error if non-nil (consistent with how
UpdateDeploymentStatus now calls
deploylifecycle.ParseStatus). Remove the
now-unused validPhases map.
pkg/deploylifecycle/tracker.go:122
discard logs an error but leaves the record behind on failure. A leaked in_progress record with no lock entity looks like a stuck deployment in ListDeployments. The practical block is mitigated because stealable returns true when the holding deployment is not found, but a dangling in_progress record in history is noisy for operators. Worth documenting the consequence here and/or marking the leaked record as failed in a follow-up.
🤖 Prompt for AI Agents
In pkg/deploylifecycle/tracker.go at the discard
function (around line 122), if the Delete call
fails, the record is left as in_progress with no
lock. Update the error log message to note that
this leaks an in_progress record that may appear
in history as a stuck deployment, and add a TODO
comment describing a follow-up to handle this
(e.g., retry with a background goroutine, or mark
the record failed before deleting it).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/build/build_v1alpha/rpc.gen.go (1)
1985-2002: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake
deploymentomission-based on the wire —args.data.Deployment = &deploymentkeeps the field present even whendeploymentisnil, so ephemeral deploys won’t look the same as an omitted field. If absence is the ownership signal, only set this whendeployment != nil.🤖 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 `@api/build/build_v1alpha/rpc.gen.go` around lines 1985 - 2002, Update BuilderClient.BuildFromTar so args.data.Deployment is assigned only when deployment is non-nil; leave it unset for nil input to preserve omission-based wire semantics.
🧹 Nitpick comments (10)
pkg/deploylifecycle/tracker.go (1)
325-352: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe read-modify-write retry has no backoff.
Up to 100 immediate
Get+Putround trips per caller means concurrent phase updates (build progress, cancel, activate all touch the same record) hammer the entity store and can livelock each other.Locks.Acquirealready has a sleep seam for exactly this; the tracker should too. Also consider returning aconderror on exhaustion so callers can classify it instead of only being able to match on the string.♻️ Back off between conflict retries
// Someone else wrote first. Re-read and re-decide: the mutation may not // even be legal against the new state. t.log.Debug("retrying deployment update after conflict", "deployment_id", deploymentID, "attempt", attempt+1) + + // Yield before re-reading, so contending writers do not spin against + // the entity store in lockstep. + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(updateRetryBackoff(attempt)): + } }With a small helper alongside
updateRetryLimit:// updateRetryBackoff grows with jitter so contending writers separate instead // of retrying in lockstep. func updateRetryBackoff(attempt int) time.Duration { base := time.Duration(attempt+1) * 2 * time.Millisecond if base > 100*time.Millisecond { base = 100 * time.Millisecond } return base + time.Duration(rand.Int63n(int64(base))) }🤖 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 `@pkg/deploylifecycle/tracker.go` around lines 325 - 352, Update the read-modify-write retry loop in the tracker method to sleep with an increasing, jittered backoff after each cond.ErrConflict{} and before the next Get, using a helper alongside updateRetryLimit similar to Locks.Acquire’s sleep seam. Preserve immediate returns for non-conflict errors, and wrap the exhaustion error with an appropriate cond error so callers can classify it while retaining the deployment and attempt context.pkg/deploylifecycle/store.go (1)
118-121: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLexical timestamp sort is only chronological within one UTC offset.
time.RFC3339formatting keeps the local offset, so a record stamped2026-07-23T09:00:00-05:00sorts before2026-07-23T13:00:00Zeven though they are the same instant. Server-written records all come from one clock, but records created by clients (legacyCreateDeploymentsupplieddeployed_by.timestamp) can carry other offsets and would order wrongly in history. Parsing before comparing removes the assumption.♻️ Compare parsed instants, falling back to lexical order
- // Timestamps are RFC3339, so lexical order is chronological order. sort.Slice(records, func(i, j int) bool { - return records[i].Deployment.DeployedBy.Timestamp > records[j].Deployment.DeployedBy.Timestamp + a, b := records[i].Deployment.DeployedBy.Timestamp, records[j].Deployment.DeployedBy.Timestamp + // Offsets differ across writers, so compare instants, not strings. + at, aErr := time.Parse(time.RFC3339, a) + bt, bErr := time.Parse(time.RFC3339, b) + if aErr == nil && bErr == nil { + return at.After(bt) + } + return a > b })🤖 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 `@pkg/deploylifecycle/store.go` around lines 118 - 121, Update the sorting comparator around records[i].Deployment.DeployedBy.Timestamp to parse both RFC3339 timestamps and compare their time instants rather than raw strings. If either timestamp cannot be parsed, preserve deterministic behavior by falling back to the existing lexical comparison.servers/build/saga_builder.go (1)
134-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the
DeployRequestunwrap.This four-line
HasDeployment()/Deployment()dance is now repeated at Lines 199-202 here and atservers/build/build.goLines 949-952 and 1116-1119. A tiny package helper (e.g.deployRequestFromArgs) would keep all four call sites identical.🤖 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 `@servers/build/saga_builder.go` around lines 134 - 137, Extract the repeated optional DeployRequest unwrapping into a shared package helper, such as deployRequestFromArgs, and replace the HasDeployment()/Deployment() blocks in the saga builder and both build.go call sites with it. Preserve the existing nil result when no deployment is present.servers/build/build.go (1)
1182-1192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold
ephemeralFromArgsinto this helper to keep one defaulting rule.
saga_builder.goLines 316-325 (ephemeralFromArgs) now duplicates the same label/TTL defaulting logic in the same package. Having the "24h" default and empty-label semantics in two places invites drift between the plain and saga paths.♻️ Suggested delegation (in `servers/build/saga_builder.go`)
func ephemeralFromArgs(args buildArgs) *ephemeralOpts { return ephemeralOptsFromArgs(args.HasEphemeralLabel(), args.EphemeralLabel(), args.HasEphemeralTtl(), args.EphemeralTtl()) }🤖 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 `@servers/build/build.go` around lines 1182 - 1192, Update ephemeralFromArgs in saga_builder.go to delegate to ephemeralOptsFromArgs using the corresponding buildArgs label/TTL presence and value accessors. Remove its duplicated defaulting logic so both build paths share the single 24h default and empty-label behavior implemented by ephemeralOptsFromArgs.servers/build/build_saga_deploy_test.go (2)
162-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rollback coverage for the tracked path.
The suite covers success, untracked, and blocked-by-lock, but never a build that fails after
begin-deployment, which is whereundoBeginDeploymentruns. A test that forces a later action to fail and then asserts the record reachedfailedandLocks().Blocking(ctx, app)is nil would pin down the compensation contract (and would exercise the cancelled-context concern raised onbuild_saga.goLines 660-672).Want me to draft that test?
🤖 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 `@servers/build/build_saga_deploy_test.go` around lines 162 - 179, Add a tracked-build rollback test alongside TestBuildSaga_Tracked_SecondBuildBlockedByLock that makes a later saga action fail after begin-deployment, then assert execution fails, the deployment record reaches failed, and h.builder.deploy.Locks().Blocking(ctx, "demo") returns nil. Use the test harness and tracked build inputs to exercise undoBeginDeployment, including verification that compensation completes despite the saga context being cancelled.
47-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis action list will drift from
registerBuildSaga.The registration chain is a verbatim copy of
build_saga.goLines 798-811 with onlyactionBuildImagestubbed, so a future action added to the production saga silently won't be exercised here. Consider extracting a shared builder that takes the build-image implementation as a parameter and having bothregisterBuildSagaand the harness use it.🤖 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 `@servers/build/build_saga_deploy_test.go` around lines 47 - 65, The test saga registration duplicates the production action chain and can drift from registerBuildSaga. Extract a shared saga-builder helper based on the existing registration sequence, parameterizing the build-image implementation, then have both registerBuildSaga and the test harness use it while preserving the test’s stubBuildImage behavior.servers/deployment/server.go (1)
590-624: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cluster_idis required but unused.
Blockingis app-scoped, so the validatedcluster_idonly ever reaches a log line. Rejecting a request for a field that has no effect is a small API wart; either document it as reserved for forward compatibility or relax the requirement.🤖 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 `@servers/deployment/server.go` around lines 590 - 624, Update GetDeployLock so cluster_id is not required when it has no effect on the app-scoped Blocking lookup; remove the corresponding validation and unused local variable while preserving app_name validation and existing lock handling. Alternatively, if the API contract must retain the field, explicitly document cluster_id as reserved for forward compatibility rather than using it only in logging.servers/deployment/cross_version_test.go (1)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the create result before dereferencing
Deployment().The sibling tests assert
!created.HasError()first; here a blocked or empty result turns into a nil dereference panic instead of a readable failure.♻️ Match the sibling tests
created, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) require.NoError(t, err) + require.False(t, created.HasError() && created.Error() != "", "first deploy should succeed: %s", created.Error()) + require.True(t, created.HasDeployment() && created.Deployment() != nil)🤖 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 `@servers/deployment/cross_version_test.go` around lines 93 - 98, In the deployment creation flow, update the test around client.CreateDeployment to assert that created.HasError() is false before calling created.Deployment().Id(). Match the guard pattern used by sibling tests, while preserving the existing require.NoError check and subsequent UpdateFailedDeployment behavior.cli/commands/deploy.go (1)
1213-1244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse this helper on the
--versionpath too.The
DeployVersionbranch near Lines 169-187 still renders its own copy of this block, so the two will drift (it already lacks the closing "Please wait…" guidance). Swapping it forprintDeploymentBlocked(ctx, result.LockInfo())finishes the consolidation this helper starts.🤖 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 `@cli/commands/deploy.go` around lines 1213 - 1244, Update the DeployVersion branch to call printDeploymentBlocked(ctx, result.LockInfo()) instead of rendering its own deployment-blocked message. Remove the duplicated formatting there while preserving the existing result handling and return behavior.testdata/slow-build/Dockerfile.miren (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe lock-contention window is only deterministic on a cold cache. Both sites depend on
RUN sleep 25actually running; the layer has no per-run input, so BuildKit serves it from cache after the first build and the first deploy releases the lock before the second one starts.
testdata/slow-build/Dockerfile.miren#L1-L9: add a cache-bustingARG(or other per-run input) ahead of the sleep so the layer is never reused.blackbox/deployment_lifecycle_test.go#L103-L144: once the fixture is uncacheable, drop the "on a warm local cache the window shrinks" caveat from the comment so it no longer documents a known flake.🤖 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 `@testdata/slow-build/Dockerfile.miren` around lines 1 - 9, Add a per-run cache-busting ARG or equivalent input before the sleep in testdata/slow-build/Dockerfile.miren (lines 1-9), ensuring the RUN sleep 25 layer is rebuilt every deployment. Update blackbox/deployment_lifecycle_test.go (lines 103-144) to remove the comment caveat about the lock window shrinking on a warm local cache; no other behavior changes are needed.
🤖 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 `@cli/commands/deploy.go`:
- Around line 825-831: Update maybeReportBlocked to compare
BlockingDeploymentId() with getDeploymentID(), reporting an unrelated holder as
blocked only when this deployment never received a record. In the server-owned
failure path around maybeReportBlocked, retain the rich blocked message but also
print the collected build/compiler errors before returning err, so diagnostics
are not suppressed.
In `@pkg/deploylifecycle/lock.go`:
- Around line 172-180: Update sanitizeKeyPart, used by LockID, to encode
app-name characters injectively rather than only replacing slashes, ensuring
distinct names such as a/b and a_b produce different lock IDs. Preserve the
deploy-lock prefix and apply the escaping consistently to the full app name
before constructing entity.Id.
In `@servers/build/saga_builder.go`:
- Around line 246-254: Normalize or reject empty DeployRequest cluster IDs
consistently at both build entry points, including the saga setup around
deployReq.ClusterId() and the plain path invoking Begin. Ensure a request with
an empty cluster_id either follows the same no-tracking behavior in both paths
or is rejected before tracking begins.
In `@servers/deployment/lock_integration_test.go`:
- Around line 158-185: The comment for
TestDeployVersionActivationDoesNotStrandLock misstates the test by describing a
concurrent deprecated CreateDeployment race that is not implemented. Reword the
function comment to describe the actual sequential assertions: a completed
DeployVersion releases its lock and allows a subsequent deploy, without
referring to CreateDeployment or racing.
In `@servers/deployment/server.go`:
- Around line 874-884: The failure settlement in servers/deployment/server.go at
lines 874-884 must use a detached, 15-second timeout-bounded context for
tracker.Fail, and invoke releaseDeployLock with that context if Fail errors,
matching the success-path handling around lines 890-899. In
servers/build/build_saga.go at lines 660-672, keep the saga.Get lookup on the
original context but pass settleContext(ctx) to FailIfUnsettled.
- Around line 831-845: The source deployment lookup around
listDeploymentsInternal must search beyond the newest 100 records so older
sourceVersionId deployments still populate gitInfo and sourceDeploymentID. Raise
the lookup limit or use an app-version-scoped query, while preserving the
existing newest-match behavior and error handling.
---
Outside diff comments:
In `@api/build/build_v1alpha/rpc.gen.go`:
- Around line 1985-2002: Update BuilderClient.BuildFromTar so
args.data.Deployment is assigned only when deployment is non-nil; leave it unset
for nil input to preserve omission-based wire semantics.
---
Nitpick comments:
In `@cli/commands/deploy.go`:
- Around line 1213-1244: Update the DeployVersion branch to call
printDeploymentBlocked(ctx, result.LockInfo()) instead of rendering its own
deployment-blocked message. Remove the duplicated formatting there while
preserving the existing result handling and return behavior.
In `@pkg/deploylifecycle/store.go`:
- Around line 118-121: Update the sorting comparator around
records[i].Deployment.DeployedBy.Timestamp to parse both RFC3339 timestamps and
compare their time instants rather than raw strings. If either timestamp cannot
be parsed, preserve deterministic behavior by falling back to the existing
lexical comparison.
In `@pkg/deploylifecycle/tracker.go`:
- Around line 325-352: Update the read-modify-write retry loop in the tracker
method to sleep with an increasing, jittered backoff after each
cond.ErrConflict{} and before the next Get, using a helper alongside
updateRetryLimit similar to Locks.Acquire’s sleep seam. Preserve immediate
returns for non-conflict errors, and wrap the exhaustion error with an
appropriate cond error so callers can classify it while retaining the deployment
and attempt context.
In `@servers/build/build_saga_deploy_test.go`:
- Around line 162-179: Add a tracked-build rollback test alongside
TestBuildSaga_Tracked_SecondBuildBlockedByLock that makes a later saga action
fail after begin-deployment, then assert execution fails, the deployment record
reaches failed, and h.builder.deploy.Locks().Blocking(ctx, "demo") returns nil.
Use the test harness and tracked build inputs to exercise undoBeginDeployment,
including verification that compensation completes despite the saga context
being cancelled.
- Around line 47-65: The test saga registration duplicates the production action
chain and can drift from registerBuildSaga. Extract a shared saga-builder helper
based on the existing registration sequence, parameterizing the build-image
implementation, then have both registerBuildSaga and the test harness use it
while preserving the test’s stubBuildImage behavior.
In `@servers/build/build.go`:
- Around line 1182-1192: Update ephemeralFromArgs in saga_builder.go to delegate
to ephemeralOptsFromArgs using the corresponding buildArgs label/TTL presence
and value accessors. Remove its duplicated defaulting logic so both build paths
share the single 24h default and empty-label behavior implemented by
ephemeralOptsFromArgs.
In `@servers/build/saga_builder.go`:
- Around line 134-137: Extract the repeated optional DeployRequest unwrapping
into a shared package helper, such as deployRequestFromArgs, and replace the
HasDeployment()/Deployment() blocks in the saga builder and both build.go call
sites with it. Preserve the existing nil result when no deployment is present.
In `@servers/deployment/cross_version_test.go`:
- Around line 93-98: In the deployment creation flow, update the test around
client.CreateDeployment to assert that created.HasError() is false before
calling created.Deployment().Id(). Match the guard pattern used by sibling
tests, while preserving the existing require.NoError check and subsequent
UpdateFailedDeployment behavior.
In `@servers/deployment/server.go`:
- Around line 590-624: Update GetDeployLock so cluster_id is not required when
it has no effect on the app-scoped Blocking lookup; remove the corresponding
validation and unused local variable while preserving app_name validation and
existing lock handling. Alternatively, if the API contract must retain the
field, explicitly document cluster_id as reserved for forward compatibility
rather than using it only in logging.
In `@testdata/slow-build/Dockerfile.miren`:
- Around line 1-9: Add a per-run cache-busting ARG or equivalent input before
the sleep in testdata/slow-build/Dockerfile.miren (lines 1-9), ensuring the RUN
sleep 25 layer is rebuilt every deployment. Update
blackbox/deployment_lifecycle_test.go (lines 103-144) to remove the comment
caveat about the lock window shrinking on a warm local cache; no other behavior
changes are needed.
🪄 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
Run ID: 9449b17e-148a-4f6c-b72e-2c02c1ef6191
📒 Files selected for processing (35)
api/build/build_v1alpha/rpc.gen.goapi/build/rpc.ymlapi/core/core_v1alpha/schema.gen.goapi/core/schema.ymlapi/deployment/deployment_v1alpha/rpc.gen.goapi/deployment/rpc.ymlblackbox/deployment_lifecycle_test.gocli/commands/deploy.gopkg/deploylifecycle/lifecycle.gopkg/deploylifecycle/lifecycle_test.gopkg/deploylifecycle/lock.gopkg/deploylifecycle/lock_test.gopkg/deploylifecycle/store.gopkg/deploylifecycle/store_test.gopkg/deploylifecycle/tracker.gopkg/deploylifecycle/tracker_test.goservers/build/build.goservers/build/build_saga.goservers/build/build_saga_deploy_test.goservers/build/deploy_tracking.goservers/build/deploy_tracking_test.goservers/build/saga_builder.goservers/build/status_registry.goservers/build/status_registry_test.goservers/deployment/cross_version_test.goservers/deployment/lock_integration_test.goservers/deployment/server.goservers/deployment/server_test.gotestdata/build-error/.miren/app.tomltestdata/build-error/Procfiletestdata/build-error/go.modtestdata/build-error/main.gotestdata/slow-build/.miren/app.tomltestdata/slow-build/Dockerfile.mirentestdata/slow-build/serve.sh
| // Seeding deploy_cluster_id is what turns on the server-owned deployment | ||
| // record: the begin-deployment action skips without it. Absent for older | ||
| // clients (no DeployRequest) and for ephemeral builds. | ||
| if deployReq != nil && eph == nil { | ||
| sb = sb.Input("deploy_cluster_id", deployReq.ClusterId()) | ||
| if gitJSON := marshalDeployGitInfo(deployReq); gitJSON != "" { | ||
| sb = sb.Input("deploy_git_info_json", gitJSON) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tracking signal differs between the two build paths.
Here tracking hinges on the value of deployReq.ClusterId() (beginDeployment skips when it is empty), while the plain path in servers/build/build.go Line 950 keys off the presence of the DeployRequest and calls Begin with whatever cluster id it got — including "". A client that sends a DeployRequest with an empty cluster_id therefore gets a lock-holding record on the plain path and no record at all on the saga path. Consider rejecting or normalizing an empty cluster_id at the entry points so both paths agree.
🤖 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 `@servers/build/saga_builder.go` around lines 246 - 254, Normalize or reject
empty DeployRequest cluster IDs consistently at both build entry points,
including the saga setup around deployReq.ClusterId() and the plain path
invoking Begin. Ensure a request with an empty cluster_id either follows the
same no-tracking behavior in both paths or is rejected before tracking begins.
- Detach the failure-path settles (DeployVersion's Fail, the saga's undoBeginDeployment) so a client disconnect during failure cannot strand the record in_progress with the lock held until its TTL. The success paths already did this; the failure paths were the gap. - On the CLI, only show "deployment blocked" when the build never got its own record. Once we have one, a held lock afterward just means an unrelated deploy started in that window, and reporting "blocked" would hide the build errors that actually failed this deploy. Errors now print regardless. - Make the plain and saga build paths agree on the tracking signal: an empty cluster_id means "not tracked" on both, instead of the plain path creating a lock-holding record the saga path would skip. - Use deploylifecycle.ParsePhase in UpdateDeploymentPhase, matching the ParseStatus change, so the accepted phase set has one source of truth. - Drop the lock-key sanitize: with the key now single-part, rewriting slashes to underscores was the only thing that let two distinct app names collide on one lock. - Reword a test comment that described a race the test doesn't perform.
1 participant
Deployment tracking was babysat entirely by the CLI. The client created the record with a "pending-build" placeholder, drove it through phases, patched in the real version after the build, and marked it active. The server was a dumb CRUD store behind that, so a client crash could strand a record, and the "lock" that stopped two deploys colliding was a list-then-create with no transactional guarantee plus a 30-minute timeout. Every history read, lock check, and activation did an O(n) scan over every deployment ever created.
This moves ownership of the record to the servers that actually do the work. The first commit adds a standalone
deploylifecyclepackage: an explicit state-machine transition table, an atomic deploy lock (adeployment_lockentity acquired via compare-and-create, so two racing deploys cannot both win), indexed queries instead of the full scan, and a Tracker that is the single surface both build paths call. The second commit wires it in.The build server now owns the record for a build. A new DeployRequest parameter on the build RPCs is the ownership signal: present means the server creates and drives the record; absent means it touches no records at all, which is exactly what an older CLI sends, so there is never a second record for one deploy. Both the plain and saga build paths are covered (the saga gets three compensating actions). The CLI becomes a viewer, gating on whether the server advertises the parameter so this release's CLI still works against last release's server. The deployment server is rebuilt on the same package: the three duplicated lock blocks are gone, the sentinels are dropped, rollback and env-var deploys go through the tracker, and the old client-driven RPCs stay wired but deprecated (plus a GetDeployLock for a fast pre-flight).
The lock and the queries are app-scoped, not app+cluster. This picks up where MIR-1465 left off: a coordinator's store is a loopback into its own etcd so it only holds its own cluster's deployments, and the client-supplied cluster_id is unreliable (a manual deploy sends the cluster name, a CI deploy sends the raw address). Keying the lock on it would let two deploys of the same app run concurrently, and filtering history on it hid CI deploys. The record still stores cluster_id for display; nothing load-bearing depends on it.
Closes MIR-681