From ef2d8cd30a0c3b1979f09728e3e81e030fa8ab8b Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Fri, 24 Jul 2026 11:10:09 -0700 Subject: [PATCH 1/3] Add deploylifecycle package for server-owned deployments 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. --- api/core/core_v1alpha/schema.gen.go | 109 +++++- api/core/schema.yml | 24 ++ pkg/deploylifecycle/lifecycle.go | 208 +++++++++++ pkg/deploylifecycle/lifecycle_test.go | 180 ++++++++++ pkg/deploylifecycle/lock.go | 482 ++++++++++++++++++++++++++ pkg/deploylifecycle/lock_test.go | 456 ++++++++++++++++++++++++ pkg/deploylifecycle/store.go | 247 +++++++++++++ pkg/deploylifecycle/store_test.go | 303 ++++++++++++++++ pkg/deploylifecycle/tracker.go | 352 +++++++++++++++++++ pkg/deploylifecycle/tracker_test.go | 464 +++++++++++++++++++++++++ 10 files changed, 2815 insertions(+), 10 deletions(-) create mode 100644 pkg/deploylifecycle/lifecycle.go create mode 100644 pkg/deploylifecycle/lifecycle_test.go create mode 100644 pkg/deploylifecycle/lock.go create mode 100644 pkg/deploylifecycle/lock_test.go create mode 100644 pkg/deploylifecycle/store.go create mode 100644 pkg/deploylifecycle/store_test.go create mode 100644 pkg/deploylifecycle/tracker.go create mode 100644 pkg/deploylifecycle/tracker_test.go diff --git a/api/core/core_v1alpha/schema.gen.go b/api/core/core_v1alpha/schema.gen.go index 4cc07975..46e827d0 100644 --- a/api/core/core_v1alpha/schema.gen.go +++ b/api/core/core_v1alpha/schema.gen.go @@ -2339,6 +2339,93 @@ func (o *GitInfo) InitSchema(sb *schema.SchemaBuilder) { sb.String("working_tree_hash", "dev.miren.core/git_info.working_tree_hash", schema.Doc("Hash of working tree if dirty")) } +const ( + DeploymentLockAcquiredAtId = entity.Id("dev.miren.core/deployment_lock.acquired_at") + DeploymentLockAppNameId = entity.Id("dev.miren.core/deployment_lock.app_name") + DeploymentLockDeploymentIdId = entity.Id("dev.miren.core/deployment_lock.deployment_id") + DeploymentLockExpiresAtId = entity.Id("dev.miren.core/deployment_lock.expires_at") +) + +type DeploymentLock struct { + ID entity.Id `json:"id"` + AcquiredAt time.Time `cbor:"acquired_at,omitempty" json:"acquired_at,omitempty"` + AppName string `cbor:"app_name,omitempty" json:"app_name,omitempty"` + DeploymentId string `cbor:"deployment_id,omitempty" json:"deployment_id,omitempty"` + ExpiresAt time.Time `cbor:"expires_at,omitempty" json:"expires_at,omitempty"` +} + +func (o *DeploymentLock) Decode(e entity.AttrGetter) { + o.ID = entity.MustGet(e, entity.DBId).Value.Id() + if a, ok := e.Get(DeploymentLockAcquiredAtId); ok && a.Value.Kind() == entity.KindTime { + o.AcquiredAt = a.Value.Time() + } + if a, ok := e.Get(DeploymentLockAppNameId); ok && a.Value.Kind() == entity.KindString { + o.AppName = a.Value.String() + } + if a, ok := e.Get(DeploymentLockDeploymentIdId); ok && a.Value.Kind() == entity.KindString { + o.DeploymentId = a.Value.String() + } + if a, ok := e.Get(DeploymentLockExpiresAtId); ok && a.Value.Kind() == entity.KindTime { + o.ExpiresAt = a.Value.Time() + } +} + +func (o *DeploymentLock) Is(e entity.AttrGetter) bool { + return entity.Is(e, KindDeploymentLock) +} + +func (o *DeploymentLock) ShortKind() string { + return "deployment_lock" +} + +func (o *DeploymentLock) Kind() entity.Id { + return KindDeploymentLock +} + +func (o *DeploymentLock) EntityId() entity.Id { + return o.ID +} + +func (o *DeploymentLock) Encode() (attrs []entity.Attr) { + if !entity.Empty(o.AcquiredAt) { + attrs = append(attrs, entity.Time(DeploymentLockAcquiredAtId, o.AcquiredAt)) + } + if !entity.Empty(o.AppName) { + attrs = append(attrs, entity.String(DeploymentLockAppNameId, o.AppName)) + } + if !entity.Empty(o.DeploymentId) { + attrs = append(attrs, entity.String(DeploymentLockDeploymentIdId, o.DeploymentId)) + } + if !entity.Empty(o.ExpiresAt) { + attrs = append(attrs, entity.Time(DeploymentLockExpiresAtId, o.ExpiresAt)) + } + attrs = append(attrs, entity.Ref(entity.EntityKind, KindDeploymentLock)) + return +} + +func (o *DeploymentLock) Empty() bool { + if !entity.Empty(o.AcquiredAt) { + return false + } + if !entity.Empty(o.AppName) { + return false + } + if !entity.Empty(o.DeploymentId) { + return false + } + if !entity.Empty(o.ExpiresAt) { + return false + } + return true +} + +func (o *DeploymentLock) InitSchema(sb *schema.SchemaBuilder) { + sb.Time("acquired_at", "dev.miren.core/deployment_lock.acquired_at", schema.Doc("When the lock was taken")) + sb.String("app_name", "dev.miren.core/deployment_lock.app_name", schema.Doc("The app this lock covers. The lock is app-scoped, not app+cluster: a coordinator's store only holds its own cluster's deployments, and the client-supplied cluster_id is unreliable (see MIR-1465).\n"), schema.Indexed) + sb.String("deployment_id", "dev.miren.core/deployment_lock.deployment_id", schema.Doc("The deployment currently holding the lock")) + sb.Time("expires_at", "dev.miren.core/deployment_lock.expires_at", schema.Doc("When the lock may be stolen by another deployment. A holder whose deployment reached a terminal status is stealable before this too.\n"), schema.Indexed) +} + const ( MetadataLabelsId = entity.Id("dev.miren.core/metadata.labels") MetadataNameId = entity.Id("dev.miren.core/metadata.name") @@ -2625,15 +2712,16 @@ func (o *Project) InitSchema(sb *schema.SchemaBuilder) { } var ( - KindApp = entity.Id("dev.miren.core/kind.app") - KindAppVersion = entity.Id("dev.miren.core/kind.app_version") - KindArtifact = entity.Id("dev.miren.core/kind.artifact") - KindConfigVersion = entity.Id("dev.miren.core/kind.config_version") - KindDeployment = entity.Id("dev.miren.core/kind.deployment") - KindMetadata = entity.Id("dev.miren.core/kind.metadata") - KindOidcBinding = entity.Id("dev.miren.core/kind.oidc_binding") - KindProject = entity.Id("dev.miren.core/kind.project") - Schema = entity.Id("dev.miren.core/schema.v1alpha") + KindApp = entity.Id("dev.miren.core/kind.app") + KindAppVersion = entity.Id("dev.miren.core/kind.app_version") + KindArtifact = entity.Id("dev.miren.core/kind.artifact") + KindConfigVersion = entity.Id("dev.miren.core/kind.config_version") + KindDeployment = entity.Id("dev.miren.core/kind.deployment") + KindDeploymentLock = entity.Id("dev.miren.core/kind.deployment_lock") + KindMetadata = entity.Id("dev.miren.core/kind.metadata") + KindOidcBinding = entity.Id("dev.miren.core/kind.oidc_binding") + KindProject = entity.Id("dev.miren.core/kind.project") + Schema = entity.Id("dev.miren.core/schema.v1alpha") ) func init() { @@ -2644,9 +2732,10 @@ func init() { (&Artifact{}).InitSchema(sb) (&ConfigVersion{}).InitSchema(sb) (&Deployment{}).InitSchema(sb) + (&DeploymentLock{}).InitSchema(sb) (&Metadata{}).InitSchema(sb) (&OidcBinding{}).InitSchema(sb) (&Project{}).InitSchema(sb) }) - schema.RegisterEncodedSchema("dev.miren.core", "v1alpha", []byte("\x1f\x8b\b\x00\x00\x00\x00\x00\x00\xff\xac[I\x93\xec8\x11\xfe\x1bl\xc32\xec\x03x\xe61\x100,\x13\xc0\x85\b.\xfc\x04\x87\xca\xca*\xab˖\xfc$\xb9\xba\x9b\x1b;\x04\xfc\n^?\x82?\bg\xc2\xda,˲,\xb9ޥCJ;?\xa5sSJ\xd9\xf5\x82)\xea\x81b\xb8U=\xe1@\xab\x86q\x80+\xa1X\xfc\xe7qI\xfdp\xa2Vh\x18\xfe\xadxx\xf0\x14\r\x83\xe6\xfb\xdf\x19\xb3\x1e\x11\x1a\x80\x9e\xcf\x04:,\xfe\xf8\xe6D\xf0\xd3W\xd6\xcc\x15j$\xb9A}\x03.\b\xa3Z\xae\x80&\x9f\a8\x11\xbc\tA(\x91\x04uu\xc3\xe8\x99\\4D@\xf3!>\x17\x81\x188{\x80F*ދ\x9d\x18\xa6\x8b\x91\xe3r{\x85\xba\xa1E\xdd\xc0I\x8f\xf8s=}w\x83\x86\xe1\xe9\xf31\x95\x19\x14\xad\xb6[\xf0\x86y\x98\xa3\xba\xdf+\xa1\xbf\x10\a\xa8\xd8#\x05\xae\x96\x00=\x9c\x84>\v\xc9\t\xbd$\x05\xb7_\xb9B\xd6\xf6撜\x91\x95>t\t\xfb4G\xfc?)\xf1C\rY\x84ɱ\xd4\x12\x93\x1e\x17V\xfa\xf2\x16G\x8f(9\x83жj\xdd\xcc\xfbn\xc5\xff\xcd=\xfe\x1a\x93\x8b\x85a!\xd1C{\x99о\xb8\x85&$\x92\xa3P g3\x9ex1б\xbfN\x7f\xea\x1b\xeaF\x10\xff:k\xa7^\xa9[3\x990h\x11oZr\x83\xf5\x82\xf65\xf3\xbfu\xb3P\x0f_O#\xf8\xa9\xf5\xea\x13B\x9c\xafm\xe3\x9cF\xd2\xe1\xbac\x17\xed\xea\x0f\u07bc\x00\xa5\xe9F!\x81\xd7\x04k\x14o\x1e\xa2|#\x81\xc2\xfa\xa1\x03\t\xb8F\xda\xc4݂\x12\x86nB;z\b\xb8>=k\xed\xf8\x84\t\x87LȌ\x02\x95\xf3Ș>\x80\xad\xe2\xb0\xf9\x192\xae6\x05RI҃\x90\xa8ש\x92\xcc\xd3r\xbcP+H8\x13\xcc\xea\xbb\xed\xe9\xc8n\xaa&\xec#g\f\xfbFN\xb8\xff3\xba\az \x15\xc2=\xa1\xb5dW\xb0\x1b\xbbG؋\xfd\x05\xd0V\x01\xfe\xd5\x14\x93)0Mabg\x86\xfde\xe3\xa0\xe6ؽ\x83\xda\xd9;\xa0%\xd2h\x80V\xad\xd1r\xd4\xfa\x97\xb71mh~\x95u\x10\xc5~\xbd\xda:ڎx\x1f\xec\x8a\xe7\xe0s\xe4\xfcC4\xc4,\x82\x85\xd2\x19\xc9N\xf6\x8ac\xc7-\x80\xdfHc\xf2\x99\x9d䆂\xd3H4\xdç\x02\x95\xfcy`\x84j\xffx\xf0桔a\x9c\x18\x84\x81q͋\xd5h\xe2j\b\x95)\xf3\x99/Y\x98\xcf\xd1\xee7\x9f\x85\xcaڬ\x95\x9c\xef\x85'8\x83Pa\"\xae\xbe\x98\xa0\t;2~\x94/\xa3^!Gҿ\xc5s\xf9\xc4^\x9dI\a\xe2YH\xe8\xb5\x15\xbd\xf9n\xbd\xa8\x00:@\x02\xd4~\xcdFm\xcd~I\xdasY\rӳ\x91\xcaz@Ro`\x0f\xde<\x04X\x1d\xc7\x14\xc0\xce)2tA͔\xbaWy\x89\x99W\xb3\r\x9c\xdd\b6\x9c\xad\x9bů\x04\xa0c\r\xeaVH\x96\xabR\x8fA=\xd9~IѢY^K\xc4\x01\xe1\x9a\xd1N\xd7\x1fd\x9e.˟\xf0\f\xae\x99\x05\xf9\x1dԗ\x93\xc9\x15fb\xa31\x99(\xb4S\xbf\x8d\xd55\xceM\x81\u07bc0h\xa6\xe9N\x10T\x05A\x00\xf4\x96\x13\x02\x7f\x8d\xea\x0e\xe8\xad\xc2 \x1aN\x06\xe9\xce\xce>!\xf0\xa2\xf0~o⿂\xd6y3\r\xf6\\ub`\x9c\\\x88^\xebl\xc6{5\xe2\xc4\xc6\xe1\xf5H8\xe8\xfd\xa0u\xb3\xb4}'F\x01T\x10In\xe6$6O\x97\xac1Qu5f\xaa)=\x0eE\xfdL\x84M9\xbe\x8e*=\xcc\xddw&\xe7x\x93̪\xa4\xb7E:\xe8a(\xcf\xea\xe2\xcdr\xee\xe4\x87M\xbe\x8dM*zM\xbd`\xf2N\xbf\xf34\\6\x8d\xa0,4#\xa8\xa9\x87\x90ހ&\x96\xc5\x06\xa4\t\xefp\x03R\x809\xd1\xf7稇)\xf6=\xbb\x84!k\x98\x18\x86\xdaY\x86\xccӅy\xe2\vn\x184\x9a\xee\r\ag\x925\xacs\xe9^\xcf\xe2龑ͺU`y*\xd9\f͈\x13/\x8cxH\xc8\xee\x1c\x02\x87\xbe\x90Nӊ\xfb%v\xc8w\xc64\x83\xbaa\xb4\x199\a\xda\xe8\xbc&b\x0fv\x9c\xe8\xd3\x02'\x8a\xc0\xe7\xbbTx)\x14\x01\xabz\x86\x8d\xce\xd4(t\xb0\x8f2 &\xf3\x12*$\xa2S\x99\xa9j\x9c%i\xe1v?\xce@\x9c\xf27\b)\xea\x01\xb8\xc3Q\xc8c\xfc\xd1b\x85\x8f3V\x10SUQc\xf6Hk\f\x1d\xd2\xc6\x1cV\xd4P\x1dY\xd0\xed(\x15\x84_\xf5\r+j\xaewr\xb3\x86\xb7D\xfatb}'z\x19j\xfdK\".kL84\xeeZ\x86\x85\xc40\x97n\x1c:n\x88\x13t\xea\xc0?t8\xda\xfd\x87\x0e\v\x95_DŽ\x87v\x8bPT̬r\x90CI\x964\xa1\x8e\x1cWF]\x13\x9e$\x1doVq\x13\ue54e;\xb3\xc2ٔ<\xa3\xcc\t7\x06\xc7{\xb8\xd6q\x1e\x94|\xcb\\\x9cD\xef\x00\u05ee\xb4\xec\xaf\a4\xff\xd6\xe7\xc3\x04\x14\f-\xf4\xc0QW\xc3\xd3@8\bۀ\x91\xd1'z#\"=(\xe0\x0f\xb2\x80USP\x87dHܻ>\x8e\x03Jٙ{\xff\x05i\xafaჩr\xb2\x1e\xb9\x06\"\xf34\x04Iݘev\xadSZ*m\\G\x8f\xe6>\xa0\xef\x13\x97HS0\xe9\x7f~'q\x15\xbe\xeaV\x94\x11\xdc\xd4'B1\xa1\x17\x93\xf3\xc2\xc4⿒u\x81\x17\xbd'\xf0Q\xa2\xf7\x99*\x85\x7f7\xc5\xd5t\x88\xf4\xd3V\x86ɔ\x11\xfd\xeaxX=\xdbI\xec\xc1BUr\xa1\xfc\xeb\xc0лB\xa4tr\x0e\xeb\xa1\x15\xf7\x80\xa4\x04n\x9c\xc1Nr\x9d\x81)\xb8\x19-\xba\xe4B\x0fE\xfbQ\xe8\xc4\v$\"\xc4h.\\\xcef\xbc\x17\xdd\v\xfeĕM\xaa\a\xb6\xc0\x10\xe3\xe9\x01\x1au+\xe5\x94\xc8Bb\xae2;\x1fz\xf5\xed*\xb4\x96\t\xdc\xf8]\xb8\x13-_\xcaw\xb4p7]\xe2D\x03,\xda;\r\xf8\xc4\x00\x8d\xae\xb6\xd5h'\x88B\x93\xb9\xe7\xf6\xdb'\x90\xfc\xeez\xb8\xadE\xe12/\xaeU6\xf9^\x16\xe0=\x97\xd2\xc1\nUz\x85\xfc\xff7\xf9a\x91\xe4\xbb\xcd\x06e\xfaOJ1\x97'\xc9k\xc1\t\xf2\x93\"\xb5T\x87\x0e\x8f\x9f\x1e\xfe\x9c\xbd3寏#\x97\x1d5\x7f{|\xa1\xfbN\xa0\xbf9\xbe\xf0\xc1\x83\xe9=+\xbe\xdb\xf3\xea\xd3{z\xc5iA\xbb\x9e\xb7\xdc\xdb\xd81zG\xdac͢\x8f˂\xa4\xb0_T\x18\x1e\xc5\xed\xa4_\x1e\xc1/\xee6\x1d\xfa\x8a\x82fTxٓ\x85\xbfs\xe7Y\x98g3[Y?;\x82z\xa4\xd3\xf5\xab{\x16Z\xb4\xc3\xeeC\x9a{f??\x82\x93\xd9R\xfb\xe9\x11\xec\xe3\x1d\xb7\xc7u\xee\x99{p\xaf\xcad)\xef̽*\xcb8E\u0379B#\x95\xf6\xeeJ+\xa2\xdd\xde^a\xe4g\xb6\xfe~R\x8e\x9auyV\xe8\xa6\x05\x8d\xc3\x03zȸp\xfbQ9\xea\u1af8q\x1dS\xb6\x11Y\xb8\x8f\xef\xb6'\xbf_\x86\xb7\xb3Q\x14\xa2\xa5Z\x99\x85\xfa\xcenp\x16\xfa\xb3\xeesz\xdb{\xb7\xa0\xdc\xe9%\xd9M\xd5B\xb3\x1fk\xb5\x16\x96o\x85\xdd\xd6°\xccj\xc6\x16\xa6\xe8\x92^\xed!qS\xad\xdc\xc2r\xe7h\xa7\xf7\x17\xf7,\xe3\xda\xc1\xf7\xa1؞\xf1!\x1d\x1el)G\xea\x10\x85\x97\xee\xf4\xad\x99\x94\xe0?\xc8\x13\xfcHK/\xfc/\xe48\xb4\xed\xc5\xf8ALfb\xe1\xff+\xed\xac\x91_\x12\xe5%O\a\\V\x0f\xe5m\x1e3x\xb2\x18\xca+\xaef\xb0\x8cJ(/\xb7ϐYeP^\x88̠\x995P\xe9\xe7g\x14@y11C\x1e\xae~\xc4\x1c\x94\x0e-\xdd\x13\xf2\x04H\xbe\x18t \xc3w\xaf\xa2\x9d6`\xfd{\xe4\x06\r\xc3\xd6o\x92ݏXS\xbf\xc0\xdd\xf99\xa4}:\xff\xf6/\xf9\xabI\xff\xc7\x00;?\x12\\\xb4\xc8\xf6~8\xb0\xbc\xf5\xdfm\xa8\x05\x1a\xcci\x13\xfc\x1f\x00\x00\xff\xff\x01\x00\x00\xff\xff\b\xa1 \xad\xf5=\x00\x00")) + schema.RegisterEncodedSchema("dev.miren.core", "v1alpha", []byte("\x1f\x8b\b\x00\x00\x00\x00\x00\x00\xff\xac[Y\xaf\xec8\x11\xfe\x1bl\xc32\xec\x03d\xe62 \x18\x96\x11\xf0\x82\xc4\v?!r'\xd5\x1d\x9fN\xe2\\\xdb\xe9s\x0eo\xec \xf8\x15\xdcs\x91\xf8\x81\xf0\x8c\xe2-N\xc5q\xec\xf4}iٕ\xd4g\xa76\x97]헺'\x1d\xf45܊\x8er苊q\x80+\xedk\xf1\x9f\xc7%\xf5ÉZ\x90a\xf8\xb7\xe2\xe1\xe8)\x19\x06\xcd\xf7\xbfs\xcd:B{\x04z>Shk\xf1\xc77'Z?}e\xcd\\\x90J\xd2\x1b\x947\xe0\x82\xb2^\xcf\v\xd1\xe4\xf3\x00'ZoBОJJڲb\xfd\x99^4\x04\xa2\xf9\x10\x9f\v@\f\x9c=@%\x15\xef\xc5v\f\xd3\xc5\xcc\xe3r{Eڡ!\xed\xc0iG\xf8s9}wE\x86\xe1\xe9\xf3!\x91\x19\x14-\xb6\x1bz\xc3\u07b8\x9eZ\xa7O\x8c\xb5\xc1\xc5\xc4q\xfb\xd6s\t\xd8M\xd0c\x1c7\x87\x81\t*\x19ף?x}\x8c\x81s$\x87!\x1a\xa2s\xa4\xa9\x81\xb9\xbe\xbd\xc5\xf5\xc8\xf8\x95\xf6\x97Rr\x80\xb2!B\xab\xf8\xf5\x9a\x9c\x9c1^\xa8\x9c\x90\x83\xe2\xf2\xeczh\x88\xd0\xe2\x02\xdd\xc4S.\xb6y\x05\x1by\x05\xe5L\xb1\xa1F\x06\x9f와\x8f\x1cN\xd42\x02\xce\x04\xb3\xfan\xbb;\xb2\x8b\xaaq\xfb\xc0\x1eþ\x91\xe2\xee\xff\f\xae\x81\x1eHA\xea\x8e\xf6\xa5dW\xb0\v\xbbG\xd8\xf3\xfd\x05\xd0V\x02\xfe\xd5\x18\x93I0Mbb{\x86\xfdec\xa3\xe6ؽ\x8d\xda\xd9۠E\xc2(B+\xd6h)b\xfd\xcbې44\xbf\x8a:\xa4\xaf\xfd|\xb5q\xb4\x9d\xe9}\xb0;=\a\x9f2\xcf?\x04]\xcc\"X(\x1d\x91lg/9v\xdc\x02\xf8\x8dV&\x9e\xd9N\xaa+8\x89\x04\xdd\xcd|*\xf4\x92?\x0f\x8c\xf6\xda>\x1e\xbc>\x9e%\xf6\x13\x8300\xaeyk՚\xb8*\xda˘\xfa̗,\xd4\xe7h\xf7\xab\xcfB%-\xd6j\x9e\xef\xe1\x1d\x9cA(j*\xae\xfe4A\x13v\xe6\xf8Q\xfa\x1c\xf5\b)3\xfd[8\x96O\xecř\xb6 \x9e\x85\x84Nk\xd1\xeb\xef\xe6\x8b\n\xa0\x05\"@\xad\xd7l\xd4\xda얤=\x93\xd50\x1d\x1b{Y\x0eD\xea\x05\xec\xc1\xebc\x80\xd5vL\x01\xec\xec\"\xb1\tj\xa6ع\xcaKH\xbd\x9am\xe0\xecFk\xc3ٸ^\xf8H\x00ZV\x91v\x85d\xb9\n\xf5\x18ԓ\xed\x97\x14-\x18\xe5\xf5\x8c8\x90\xbad}\xab\xf3\x0f:w\x97\xe9\x0fރkfA\x7f\a\xe5\xe5db\x85\xe9Xo\x8c\x06\nm\xd4oCy\x8d3S\xe8o\x9e\x1bTSw\xc7\t\x8a\f'\x80\xfe\x96\xe2\x02\x7f\r\xca\x0e\xfa[Q\x83\xa88\x1d\xa4\xdb;\xfb\x04dE\xf8|o⿂\x96y55\xf6Lub`\x9c^\xa8\x1e\xebl\xda{9\xe2\xc4\xc6\xe1\xf5H9\xe8\xf5\xa0q\xbd\xb8~'F\x01\xbd\xa0\x92\xde\xccNl\xee.YCS\xd5٘ɦt\x1bO\xf53\x016e\xf8ګt3uݙ\x8c\xe3M4\xaa\xd2\xce&頛x>\xab\x837˹\x13\x1f6\xf96\x16\xa9\xe01\xf5\x82\xc9\xdb\xfd\xce]\x1fF\xa0`h\xa0\x03N\xda\x12\x9e\x06\xcaA\xd8\x02\x8c\f>\xd1\v\x11\xed@\x01\x7f\x90\x04\xac\x8a\x82\xda%1q\xef\xf88\f(ek\xce\xfd\x17\xa4\xbd\x82\x85\x0f\xa6\xd2\xc9r\xe4\x1a\x88\xce]\f\x12;1K\xacZǤ\x94[\xb8\x0en\xcd}@\xdf&.\x81\xa2`\xd4\xfe\xfcJ\xe2\xca}թ(\xa3uU\x9eh_\xd3\xfebb\x1e\x0e,\xfe+I\ax\xc1s\x02\x1f%x\x9e\xa9B\xf8wc\\UKh7-e5\x9d\"\xa2\x9f\x1d\x0f\xabg;\x81\x1d\rTD\aJ?\x0e\xc4օ\x91\xe2\xc1\x19\xe7C+\xee\x81H\t\xdc\x18\x83\xed\xa4\x1a\x03Sp3Zpȅ\x1c\xb2\xd6#l\xc4\v$*\xc4h\x0e\\Φ\xbd\xe7\xdd\v\xfeȑM\xac\x06\xb6\xc0\x10\xe3\xe9\x01*u*\xe5\x84\xc801U\x98\xad\x0f\xbd\xfav\xe5Z\xcb\x00n\xec\x0e\xafD˗\xd2\r\r\xaf\xa6K\x9c\xa0\x83\x05k\xa7\x88O\fP\xe9l[\xb5v\x9c\b\xab\xcc=\xb7\xdf>\x81\xa4W\xd7\xf1\xb2\x16\x84K<\xb8V\xd1\xe4{I\x80\xf7\x1cJ\xa3\x11\x8a\xf8\b\xe9\xff7\xf9a\xd6\xccw\x8b\rJ\xf5\x9f\xe4b.w\x92\u05cc\x1d\xe4'Yb)\x0em\x1e?=\xfc9{{\xca_\x1fG\xce\xdbj\xfe\xf6\xf8@\xf7\xed@\x7fs|\xe0\x83\x1b\xd3{F|\xb7\xfbէ\xf7\xf4\x88Ӏv\xces\x92\xcczQ\xa6{d\x97\x93~y\x04?\xbb\xdat\xe8+2\x8aQ\xf8\xb0'\t\x7f\xe7\xcc33\xce&\x96\xb2~v\x04\xf5H\xa5\xebW\xf7\f\xb4(\x87݇4\xd7\xcc~~\x04'\xb1\xa4\xf6\xd3#\xd8\xc7+n\x8f\xeb\xd83\xd7\xe0^\xe5\xcd%\xbf2\xf7*/\xe2d\x15\xe72\x95\x94[\xbb\xcb͈vk{\x99\x9e\x9fX\xfa\xfbI>j\xd2\xe1Y\xa6\x99f\x14\x0e\x0f\xc8!\xe1\xc0\xedG\xf9\xa8\x87\x8f\xe2ƵO\xd9Bd\xe6:\xbe[\x9e\xfc~\x1e\xde\xceB\x91\x89\x16+ef\xca;\xb9\xc0\x99iϺ\xce\xe9-\xef\xed\x82r\xa7\x95$\x17U3\xd5~\xacԚ\x99\xbeeV[3\xdd2\xa9\x18\x9b\x19\xa2sj\xb5\x87\xa6\x1b+\xe5f\xa6;G+\xbd\xbf\xb8g\x18W\x0e\xbe\x0f\xc5\u058c\x0f\xc9\xf0`I9\x90\x87(\xbcx\xa5oͤ&\xfe\x83\xb4\x89\x1f)\xe9\xe1\x7f!\x87\xa1m-\xc6wb:\x133\xff\xaf\xb43FzJ\x94\x16<\x1dp^>\x94\xb6x\xcc\xe0\xd1d(-\xb9\x9a\xc1\x122\xa1\xb4\xd8>C&\xa5Ai.2\x83&\xe6@\xb9\x9f\x9f\x90\x00\xa5\xf9\xc4\fy8\xfb\x11\xb3S:\xb4xMț@\xf4ET\x81\\\xd56\xd0m\xbb\xb2e\xd5ո\xd4\xea_\x8e˷ҏ\x84q\xa9\r\x01\x15\xa4\xd2vb\x8b\x9cW\x9f\xb0\xacmn\xdf@2Pi\xf7\xf8p\x85\nìoEt\xf1\xeb\x10\xb8.\x8c\x01Q\x19\xf7!T\xbc\x8d\xd7}\x10\"~\xf9*\x9a)\x93\xd2\x17\xcb+2\f[\x97\xcb\xddm\xe4\xd8U\xea\x9d{\xad\xf6\xe9|\x893z\xfdտձs\xdbsQ\xebܻ\x01\xb2,\xdf\xecVF\xb1+$\xd4{\xb0ԓ\xdc\xe7\xff\x00\x00\x00\xff\xff\x01\x00\x00\xff\xff\xff\xc0\xea:\xf3?\x00\x00")) } diff --git a/api/core/schema.yml b/api/core/schema.yml index 11ceaccf..43df2531 100644 --- a/api/core/schema.yml +++ b/api/core/schema.yml @@ -360,6 +360,30 @@ kinds: type: string doc: Git repository remote URL + deployment_lock: + app_name: + type: string + doc: > + The app this lock covers. The lock is app-scoped, not app+cluster: a + coordinator's store only holds its own cluster's deployments, and the + client-supplied cluster_id is unreliable (see MIR-1465). + indexed: true + + deployment_id: + type: string + doc: The deployment currently holding the lock + + acquired_at: + type: time + doc: When the lock was taken + + expires_at: + type: time + doc: > + When the lock may be stolen by another deployment. A holder whose + deployment reached a terminal status is stealable before this too. + indexed: true + config_version: app: type: ref diff --git a/pkg/deploylifecycle/lifecycle.go b/pkg/deploylifecycle/lifecycle.go new file mode 100644 index 00000000..376f6862 --- /dev/null +++ b/pkg/deploylifecycle/lifecycle.go @@ -0,0 +1,208 @@ +// Package deploylifecycle owns the deployment record state machine: the set of +// valid statuses and phases, and the transitions between them. +// +// It exists as its own package because both servers/deployment and servers/build +// need it, and servers/build importing servers/deployment would be backwards. +package deploylifecycle + +import ( + "fmt" + "strings" + + "miren.dev/runtime/pkg/cond" +) + +// Status is the lifecycle state of a deployment record. +type Status string + +const ( + StatusInProgress Status = "in_progress" + StatusActive Status = "active" + StatusSucceeded Status = "succeeded" + StatusFailed Status = "failed" + StatusRolledBack Status = "rolled_back" + StatusCancelled Status = "cancelled" +) + +// Phase is the fine-grained progress of a deployment that is still in_progress. +type Phase string + +const ( + PhasePreparing Phase = "preparing" + PhaseBuilding Phase = "building" + PhasePushing Phase = "pushing" + PhaseActivating Phase = "activating" +) + +// allStatuses and allPhases are ordered for stable error messages. They are +// unexported so an importer cannot mutate the sets derived from them; use +// Statuses() / Phases() for a copy. +var ( + allStatuses = []Status{ + StatusInProgress, StatusActive, StatusSucceeded, + StatusFailed, StatusRolledBack, StatusCancelled, + } + allPhases = []Phase{PhasePreparing, PhaseBuilding, PhasePushing, PhaseActivating} +) + +// Statuses returns a copy of every recognized status, in a stable order. +func Statuses() []Status { return append([]Status(nil), allStatuses...) } + +// Phases returns a copy of every recognized phase, in a stable order. +func Phases() []Phase { return append([]Phase(nil), allPhases...) } + +// terminalStatuses is the explicit set of statuses with no outbound +// transitions. Stating it here — rather than inferring "terminal" from a +// missing validTransitions entry — is what makes the init check below able to +// catch a status that was added without deciding whether it is terminal. +var terminalStatuses = map[Status]struct{}{ + StatusSucceeded: {}, + StatusFailed: {}, + StatusRolledBack: {}, + StatusCancelled: {}, +} + +// validTransitions maps a status to the statuses reachable from it. +// +// in_progress is reachable from itself so that a no-op update (the client +// re-asserting the state it is already in) is not an error. +var validTransitions = map[Status][]Status{ + StatusInProgress: {StatusInProgress, StatusActive, StatusFailed, StatusCancelled}, + StatusActive: {StatusSucceeded, StatusRolledBack}, +} + +// init fails fast if the three sources of truth ever drift: every status must be +// either terminal or have outbound transitions, and never both. A new status +// added to allStatuses without a decision here panics at startup rather than +// silently becoming terminal — which the lock manager would read as "abandoned". +func init() { + for _, s := range allStatuses { + _, terminal := terminalStatuses[s] + _, hasEdges := validTransitions[s] + if terminal == hasEdges { + panic("deploylifecycle: status " + string(s) + + " must be exactly one of terminal or transitionable") + } + } + for from := range validTransitions { + if !from.Valid() { + panic("deploylifecycle: validTransitions references unknown status " + string(from)) + } + } +} + +// Valid reports whether s is a status this system recognizes. Records read back +// from storage can carry anything, so callers validating stored data should use +// this rather than assuming. +func (s Status) Valid() bool { + _, ok := statusSet[s] + return ok +} + +// Terminal reports whether s admits no further transitions. A deployment in a +// terminal status holds no lock and will not change again. +func (s Status) Terminal() bool { + _, ok := terminalStatuses[s] + return ok +} + +func (s Status) String() string { return string(s) } + +// Valid reports whether p is a phase this system recognizes. +func (p Phase) Valid() bool { + _, ok := phaseSet[p] + return ok +} + +func (p Phase) String() string { return string(p) } + +var ( + statusSet = indexOf(allStatuses) + phaseSet = indexOf(allPhases) +) + +func indexOf[T comparable](values []T) map[T]struct{} { + set := make(map[T]struct{}, len(values)) + for _, v := range values { + set[v] = struct{}{} + } + return set +} + +// ParseStatus converts a wire/stored string into a Status, rejecting unknown +// values with a validation failure. +func ParseStatus(s string) (Status, error) { + status := Status(s) + if !status.Valid() { + return "", cond.ValidationFailure("invalid-status", + "status must be one of: "+joinStatuses(allStatuses)) + } + return status, nil +} + +// ParsePhase converts a wire/stored string into a Phase, rejecting unknown +// values with a validation failure. +func ParsePhase(s string) (Phase, error) { + phase := Phase(s) + if !phase.Valid() { + return "", cond.ValidationFailure("invalid-phase", + "phase must be one of: "+joinPhases(allPhases)) + } + return phase, nil +} + +// Transition reports whether a deployment may move from one status to another. +// It is the single authority on that question: every write path goes through it +// rather than re-deriving the rules. +// +// A rejected transition is a conflict, not a validation failure — the request is +// well-formed, it just lost a race or arrived against a record that has already +// finished. +func Transition(from, to Status) error { + if !to.Valid() { + return cond.ValidationFailure("invalid-status", + "status must be one of: "+joinStatuses(allStatuses)) + } + + for _, allowed := range validTransitions[from] { + if allowed == to { + return nil + } + } + + return cond.Conflict("deployment-status", + fmt.Sprintf("cannot transition deployment from %s to %s", from, to)) +} + +// CheckPhase reports whether a phase may be recorded against a deployment in the +// given status. Phases describe work still in flight, so they are meaningless +// once a deployment has left in_progress. +func CheckPhase(status Status, phase Phase) error { + if !phase.Valid() { + return cond.ValidationFailure("invalid-phase", + "phase must be one of: "+joinPhases(allPhases)) + } + + if status != StatusInProgress { + return cond.Conflict("deployment-status", + fmt.Sprintf("cannot set phase on deployment in %s state", status)) + } + + return nil +} + +func joinStatuses(statuses []Status) string { + parts := make([]string, len(statuses)) + for i, s := range statuses { + parts[i] = string(s) + } + return strings.Join(parts, ", ") +} + +func joinPhases(phases []Phase) string { + parts := make([]string, len(phases)) + for i, p := range phases { + parts[i] = string(p) + } + return strings.Join(parts, ", ") +} diff --git a/pkg/deploylifecycle/lifecycle_test.go b/pkg/deploylifecycle/lifecycle_test.go new file mode 100644 index 00000000..e889ee46 --- /dev/null +++ b/pkg/deploylifecycle/lifecycle_test.go @@ -0,0 +1,180 @@ +package deploylifecycle + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "miren.dev/runtime/pkg/cond" +) + +// cond.ErrValidationFailure carries no Is method, so errors.Is would compare it +// by field equality. errors.As is the assertion that actually means "this kind +// of error". ErrConflict is matched the same way for symmetry. +func isValidationFailure(err error) bool { + var target cond.ErrValidationFailure + return errors.As(err, &target) +} + +func isConflict(err error) bool { + var target cond.ErrConflict + return errors.As(err, &target) +} + +// allowedTransitions is the expected state machine, written out independently of +// the implementation so the matrix test below is a real check rather than a +// restatement of validTransitions. +var allowedTransitions = map[Status]map[Status]bool{ + StatusInProgress: { + StatusInProgress: true, + StatusActive: true, + StatusFailed: true, + StatusCancelled: true, + }, + StatusActive: { + StatusSucceeded: true, + StatusRolledBack: true, + }, + StatusSucceeded: {}, + StatusFailed: {}, + StatusRolledBack: {}, + StatusCancelled: {}, +} + +// TestTransitionMatrix exercises every (from, to) pair, so a new status or a +// widened edge cannot be added without updating the expectation above. +func TestTransitionMatrix(t *testing.T) { + for _, from := range Statuses() { + for _, to := range Statuses() { + from, to := from, to + t.Run(string(from)+"->"+string(to), func(t *testing.T) { + err := Transition(from, to) + + if allowedTransitions[from][to] { + require.NoError(t, err) + return + } + + require.Error(t, err) + assert.True(t, isConflict(err), + "a rejected transition is a conflict, got %T: %v", err, err) + assert.Contains(t, err.Error(), string(from)) + assert.Contains(t, err.Error(), string(to)) + }) + } + } +} + +func TestTransitionToUnknownStatusIsValidationFailure(t *testing.T) { + err := Transition(StatusInProgress, Status("banana")) + + require.Error(t, err) + assert.True(t, isValidationFailure(err), + "an unrecognized target status is malformed input, not a conflict; got %T", err) +} + +// A record whose stored status we do not recognize must not become a wildcard +// that transitions anywhere. +func TestTransitionFromUnknownStatusIsRejected(t *testing.T) { + for _, to := range Statuses() { + err := Transition(Status("banana"), to) + require.Error(t, err, "unknown source status must not reach %s", to) + } +} + +func TestTerminal(t *testing.T) { + terminal := map[Status]bool{ + StatusSucceeded: true, + StatusFailed: true, + StatusRolledBack: true, + StatusCancelled: true, + } + + for _, s := range Statuses() { + assert.Equal(t, terminal[s], s.Terminal(), "Terminal() for %s", s) + } + + assert.False(t, Status("banana").Terminal(), + "an unrecognized status is not terminal — it is not a status at all") +} + +// Every status must be classified exactly once: terminal, or transitionable. +// This is the invariant the package init() enforces at startup; asserting it in +// a test as well gives a clear failure rather than a panic during import. +func TestEveryStatusIsClassifiedExactlyOnce(t *testing.T) { + for _, s := range Statuses() { + _, terminal := terminalStatuses[s] + _, hasEdges := validTransitions[s] + assert.NotEqual(t, terminal, hasEdges, + "status %s must be exactly one of terminal or transitionable", s) + } +} + +// Statuses() and Phases() must hand out copies, so a caller cannot corrupt the +// package's own sets. +func TestAccessorsReturnCopies(t *testing.T) { + s := Statuses() + require.NotEmpty(t, s) + s[0] = Status("corrupted") + assert.NotEqual(t, Status("corrupted"), Statuses()[0], "Statuses() must return a copy") + + p := Phases() + require.NotEmpty(t, p) + p[0] = Phase("corrupted") + assert.NotEqual(t, Phase("corrupted"), Phases()[0], "Phases() must return a copy") +} + +func TestParseStatus(t *testing.T) { + for _, s := range Statuses() { + got, err := ParseStatus(string(s)) + require.NoError(t, err) + assert.Equal(t, s, got) + } + + _, err := ParseStatus("banana") + require.Error(t, err) + assert.True(t, isValidationFailure(err)) + assert.Contains(t, err.Error(), "cancelled", + "the message should list every accepted status") + + _, err = ParseStatus("") + require.Error(t, err) +} + +func TestParsePhase(t *testing.T) { + for _, p := range Phases() { + got, err := ParsePhase(string(p)) + require.NoError(t, err) + assert.Equal(t, p, got) + } + + _, err := ParsePhase("banana") + require.Error(t, err) + assert.True(t, isValidationFailure(err)) + + _, err = ParsePhase("") + require.Error(t, err) +} + +func TestCheckPhase(t *testing.T) { + for _, p := range Phases() { + require.NoError(t, CheckPhase(StatusInProgress, p), + "%s is valid while in_progress", p) + } + + for _, s := range Statuses() { + if s == StatusInProgress { + continue + } + + err := CheckPhase(s, PhaseBuilding) + require.Error(t, err, "phases are meaningless in %s", s) + assert.True(t, isConflict(err), "got %T", err) + } + + err := CheckPhase(StatusInProgress, Phase("banana")) + require.Error(t, err) + assert.True(t, isValidationFailure(err), + "an unrecognized phase is malformed input, not a conflict; got %T", err) +} diff --git a/pkg/deploylifecycle/lock.go b/pkg/deploylifecycle/lock.go new file mode 100644 index 00000000..02addc89 --- /dev/null +++ b/pkg/deploylifecycle/lock.go @@ -0,0 +1,482 @@ +package deploylifecycle + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + "time" + + "miren.dev/runtime/api/core/core_v1alpha" + "miren.dev/runtime/api/entityserver/entityserver_v1alpha" + "miren.dev/runtime/pkg/cond" + "miren.dev/runtime/pkg/entity" +) + +// DefaultLockTTL is how long a lock survives without its holder finishing. It +// backstops a deploy whose driver died without releasing; a holder that reaches +// a terminal status is stealable sooner than this. +const DefaultLockTTL = 30 * time.Minute + +// lockAcquireLimit bounds the steal/retry loop. Each iteration means another +// caller beat us to a write, so the cap is a runaway backstop rather than a +// contention budget — set high deliberately. +const lockAcquireLimit = 100 + +// ErrLockHeld reports that a live deployment already holds the lock. It is an +// expected outcome, not a failure — match against it with errors.Is. +var ErrLockHeld = errors.New("deployment lock held") + +// LockHeldError carries the blocking holder along with the error, so a caller +// several layers up can render "who is in the way" without re-reading the lock +// and risking a different answer. +type LockHeldError struct { + Holder *Holder +} + +func (e *LockHeldError) Error() string { + return fmt.Sprintf("deployment lock for %s held by %s", + e.Holder.AppName, e.Holder.DeploymentID) +} + +func (e *LockHeldError) Is(target error) bool { + return target == ErrLockHeld +} + +// HolderFrom extracts the blocking holder from an error returned by Acquire or +// Begin, reporting false for any other error. +func HolderFrom(err error) (*Holder, bool) { + var held *LockHeldError + if errors.As(err, &held) { + return held.Holder, true + } + return nil, false +} + +// Holder describes the deployment currently holding a lock. +type Holder struct { + AppName string + DeploymentID string + AcquiredAt time.Time + ExpiresAt time.Time + Revision int64 +} + +// Expired reports whether the lock has outlived its TTL. +func (h *Holder) Expired(now time.Time) bool { + return !h.ExpiresAt.After(now) +} + +// Held reports whether this holder still owns the lock: a released tombstone has +// no owner, and an expired lock no longer protects anything. +// +// It deliberately says nothing about whether the holding deployment is still +// alive — that needs the record, so use Locks.Blocking for the full answer. +func (h *Holder) Held(now time.Time) bool { + return h.DeploymentID != "" && !h.Expired(now) +} + +// StatusLookup reports the stored status of a deployment record. Acquire uses it +// so a lock whose holder has already finished — the record says failed, the lock +// says running — can be taken immediately instead of waiting out the TTL. +type StatusLookup func(ctx context.Context, deploymentID string) (Status, error) + +// Locks manages the deploy lock entity for an app. +// +// Acquisition is a compare-and-create against the entity store, so two callers +// racing to start a deploy cannot both win. That is the whole point of the type: +// the previous scheme listed in-progress deployments and then created a record, +// which admitted an interleaving where both callers saw an empty list. +// +// The lock is 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 anyway (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. See +// MIR-1465. +type Locks struct { + eac *entityserver_v1alpha.EntityAccessClient + log *slog.Logger + ttl time.Duration + status StatusLookup + + // now is a test seam for expiry; the zero value means time.Now. + now clockFn + + // sleep is a test seam for the contention backoff; nil means time.Sleep. + sleep func(time.Duration) +} + +// lockRetryBackoff is the base delay between contended acquire attempts. Each +// retry means another caller won a write, so a brief pause keeps a burst of +// contenders from hammering the entity store; it is capped low because a deploy +// lock is rarely contended and we don't want to add real latency to the rare +// case that is. +const lockRetryBackoff = 5 * time.Millisecond + +// lockRetryBackoffMax caps the linear growth so a pathological loop still yields +// promptly. +const lockRetryBackoffMax = 100 * time.Millisecond + +// backoff pauses before a contended retry. attempt is 0-based; the first retry +// waits one base interval. The initial acquire (attempt 0 in Acquire's loop is +// the first try, which never calls this) pays no delay. +func (l *Locks) backoff(attempt int) { + d := time.Duration(attempt) * lockRetryBackoff + if d > lockRetryBackoffMax { + d = lockRetryBackoffMax + } + if d <= 0 { + return + } + if l.sleep != nil { + l.sleep(d) + return + } + time.Sleep(d) +} + +// clockFn is the test seam shared by the types in this package. A nil clockFn +// reads the real clock, so the zero value is always usable. +type clockFn func() time.Time + +func (c clockFn) Now() time.Time { + if c == nil { + return time.Now() + } + return c() +} + +// NewLocks builds a lock manager. status may be nil, in which case a held lock +// is only stealable once expired. +func NewLocks(log *slog.Logger, eac *entityserver_v1alpha.EntityAccessClient, status StatusLookup) *Locks { + return &Locks{ + eac: eac, + log: log.With("module", "deploylifecycle.locks"), + ttl: DefaultLockTTL, + status: status, + } +} + +// WithTTL returns a copy holding locks for d instead of DefaultLockTTL. +func (l *Locks) WithTTL(d time.Duration) *Locks { + clone := *l + clone.ttl = d + return &clone +} + +// LockID is the deterministic entity id for an app's deploy lock. Determinism is +// what makes create-if-absent a mutual exclusion primitive: every contender +// computes the same key. +func LockID(appName string) entity.Id { + return entity.Id(fmt.Sprintf("deploy-lock/%s", sanitizeKeyPart(appName))) +} + +// sanitizeKeyPart keeps a slash in a name from splitting the key into a +// different shape. +func sanitizeKeyPart(s string) string { + return strings.ReplaceAll(s, "/", "_") +} + +// Acquire takes the deploy lock for deploymentID, returning the holder it +// established. +// +// If another live deployment holds it, Acquire returns that holder along with +// ErrLockHeld. Re-acquiring a lock this same deployment already holds succeeds +// and refreshes the expiry, so a retried call is not an error. +func (l *Locks) Acquire(ctx context.Context, appName, deploymentID string) (*Holder, error) { + if appName == "" || deploymentID == "" { + return nil, cond.ValidationFailure("missing-field", + "app_name and deployment_id are both required to acquire a deploy lock") + } + + id := LockID(appName) + + for attempt := 0; attempt < lockAcquireLimit; attempt++ { + mine := l.holderFor(appName, deploymentID) + + res, err := l.eac.Ensure(ctx, l.encode(id, mine)) + if err != nil { + return nil, fmt.Errorf("failed to acquire deploy lock: %w", err) + } + + if res.Created() { + mine.Revision = res.Revision() + l.log.Debug("acquired deploy lock", + "app", appName, "deployment_id", deploymentID) + return mine, nil + } + + // Someone holds it. Whether we may take it over depends on who they are + // and whether they are still alive. + current, err := l.Get(ctx, appName) + if err != nil { + if errors.Is(err, cond.ErrNotFound{}) { + // Released between our Ensure and our read — try again. + l.backoff(attempt + 1) + continue + } + return nil, err + } + + if current.DeploymentID == deploymentID { + return l.refresh(ctx, id, current, mine) + } + + stealable, reason := l.stealable(ctx, current) + if !stealable { + return current, &LockHeldError{Holder: current} + } + + l.log.Warn("stealing deploy lock", + "app", appName, + "from_deployment_id", current.DeploymentID, + "to_deployment_id", deploymentID, + "reason", reason) + + if stealRaceHook != nil { + stealRaceHook() + } + + taken, err := l.replace(ctx, id, current.Revision, mine) + if err != nil { + if errors.Is(err, cond.ErrConflict{}) { + // Another contender moved the lock first. Re-read and decide + // again rather than assuming we lost outright. + l.backoff(attempt + 1) + continue + } + return nil, err + } + return taken, nil + } + + return nil, fmt.Errorf("gave up acquiring deploy lock for %s after %d attempts", + appName, lockAcquireLimit) +} + +// refresh extends an expiry for the deployment that already holds the lock. +func (l *Locks) refresh(ctx context.Context, id entity.Id, current, mine *Holder) (*Holder, error) { + refreshed, err := l.replace(ctx, id, current.Revision, mine) + if err != nil { + if errors.Is(err, cond.ErrConflict{}) { + // Someone rewrote the lock under us. current is still ours by + // deployment id, so report it rather than failing the deploy. + return current, nil + } + return nil, err + } + return refreshed, nil +} + +// stealable reports whether a held lock may be taken over, and why. +func (l *Locks) stealable(ctx context.Context, h *Holder) (bool, string) { + if h.Expired(l.now.Now()) { + return true, "expired" + } + + if h.DeploymentID == "" { + // A lock with no owner cannot be reconciled against a record; treat it + // as debris rather than an indefinite block. + return true, "no owner recorded" + } + + if l.status == nil { + return false, "" + } + + status, err := l.status(ctx, h.DeploymentID) + if err != nil { + if errors.Is(err, cond.ErrNotFound{}) { + // The record is gone, so nothing will ever release this lock. + return true, "holding deployment no longer exists" + } + l.log.Warn("could not read holding deployment; treating lock as held", + "deployment_id", h.DeploymentID, "error", err) + return false, "" + } + + if status.Terminal() { + return true, "holding deployment already " + status.String() + } + + return false, "" +} + +// stealRaceHook is a test seam. When non-nil, Acquire invokes it after deciding +// a held lock is stealable but before the guarded replace, letting a test +// interleave a concurrent steal so the replace loses its compare-and-swap. It is +// always nil in production. +var stealRaceHook func() + +// releaseRaceHook is a test seam. When non-nil, Release invokes it after reading +// the holder but before writing the release, letting a white-box test +// deterministically interleave a concurrent steal into that window. It is always +// nil in production. Mirrors deleteEntityRaceHook in pkg/entity/store.go. +var releaseRaceHook func() + +// Release drops the lock, but only if deploymentID still holds it. +// +// The release is a revision-guarded write rather than a delete: between reading +// the holder and acting on it, another deployment may legitimately steal the +// lock — a deployment that has just finished is exactly what makes a lock +// stealable — and an unguarded delete would then remove the successor's lock, +// letting a third deployment start alongside it. The entity server's delete +// takes no caller revision, so the write has to be a Replace to be safe. +// +// The released lock is left behind as a tombstone: owned by nobody, already +// expired. Acquire treats that as free. +func (l *Locks) Release(ctx context.Context, appName, deploymentID string) error { + current, err := l.Get(ctx, appName) + if err != nil { + if errors.Is(err, cond.ErrNotFound{}) { + return nil + } + return err + } + + if current.DeploymentID != deploymentID { + l.log.Debug("not releasing deploy lock held by another deployment", + "app", appName, + "holder", current.DeploymentID, "caller", deploymentID) + return nil + } + + if releaseRaceHook != nil { + releaseRaceHook() + } + + // No DeploymentID: the lock is owned by nobody once released. + released := &Holder{ + AppName: appName, + AcquiredAt: current.AcquiredAt, + ExpiresAt: l.now.Now(), + } + + if _, err := l.replace(ctx, LockID(appName), current.Revision, released); err != nil { + if errors.Is(err, cond.ErrConflict{}) { + // The lock moved between our read and our write, so it is no longer + // ours to release. Whoever holds it now keeps it. + l.log.Debug("deploy lock moved before release; leaving it alone", + "app", appName, "caller", deploymentID) + return nil + } + return fmt.Errorf("failed to release deploy lock: %w", err) + } + + l.log.Debug("released deploy lock", + "app", appName, "deployment_id", deploymentID) + return nil +} + +// Blocking returns the holder that would block a new deployment for this app, or +// nil if nothing would. +// +// This is the question callers actually have — a pre-flight check before +// uploading a build context, or the lock info rendered in a "deployment +// blocked" message — and it is not the same as "a lock entity exists". A +// released tombstone, an expired lock, and a lock whose deployment already +// finished all read as free. +func (l *Locks) Blocking(ctx context.Context, appName string) (*Holder, error) { + current, err := l.Get(ctx, appName) + if err != nil { + if errors.Is(err, cond.ErrNotFound{}) { + return nil, nil + } + return nil, err + } + + if stealable, _ := l.stealable(ctx, current); stealable { + return nil, nil + } + + return current, nil +} + +// Get returns the current holder, or cond.ErrNotFound if the lock is free. +func (l *Locks) Get(ctx context.Context, appName string) (*Holder, error) { + id := LockID(appName) + + res, err := l.eac.Get(ctx, string(id)) + if err != nil { + return nil, err + } + + var stored core_v1alpha.DeploymentLock + stored.Decode(&entityAttrs{entity: res.Entity()}) + + return &Holder{ + AppName: stored.AppName, + DeploymentID: stored.DeploymentId, + AcquiredAt: stored.AcquiredAt, + ExpiresAt: stored.ExpiresAt, + Revision: res.Entity().Revision(), + }, nil +} + +func (l *Locks) holderFor(appName, deploymentID string) *Holder { + now := l.now.Now() + return &Holder{ + AppName: appName, + DeploymentID: deploymentID, + AcquiredAt: now, + ExpiresAt: now.Add(l.ttl), + } +} + +func (l *Locks) encode(id entity.Id, h *Holder) []entity.Attr { + stored := &core_v1alpha.DeploymentLock{ + AppName: h.AppName, + DeploymentId: h.DeploymentID, + AcquiredAt: h.AcquiredAt, + ExpiresAt: h.ExpiresAt, + } + + return append(stored.Encode(), entity.Ref(entity.DBId, id)) +} + +// replace takes over the lock entity under a revision guard, so two callers that +// both judged the lock stealable cannot both succeed. +func (l *Locks) replace(ctx context.Context, id entity.Id, revision int64, h *Holder) (*Holder, error) { + res, err := l.eac.Replace(ctx, l.encode(id, h), revision) + if err != nil { + return nil, err + } + + taken := *h + taken.Revision = res.Revision() + return &taken, nil +} + +// entityAttrs adapts an RPC entity to the AttrGetter the generated decoders want. +type entityAttrs struct { + entity *entityserver_v1alpha.Entity +} + +func (e *entityAttrs) Get(id entity.Id) (entity.Attr, bool) { + if id == entity.DBId { + return entity.Ref(entity.DBId, entity.Id(e.entity.Id())), true + } + + for _, attr := range e.entity.Attrs() { + if attr.ID == id { + return attr, true + } + } + return entity.Attr{}, false +} + +func (e *entityAttrs) GetAll(id entity.Id) []entity.Attr { + var out []entity.Attr + for _, attr := range e.entity.Attrs() { + if attr.ID == id { + out = append(out, attr) + } + } + return out +} + +func (e *entityAttrs) Attrs() []entity.Attr { + return e.entity.Attrs() +} diff --git a/pkg/deploylifecycle/lock_test.go b/pkg/deploylifecycle/lock_test.go new file mode 100644 index 00000000..3629c4ea --- /dev/null +++ b/pkg/deploylifecycle/lock_test.go @@ -0,0 +1,456 @@ +package deploylifecycle + +import ( + "context" + "errors" + "io" + "log/slog" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "miren.dev/runtime/pkg/cond" + "miren.dev/runtime/pkg/entity/testutils" +) + +type fakeClock struct { + mu sync.Mutex + now time.Time +} + +func (c *fakeClock) Now() time.Time { + c.mu.Lock() + defer c.mu.Unlock() + return c.now +} + +func (c *fakeClock) Advance(d time.Duration) { + c.mu.Lock() + defer c.mu.Unlock() + c.now = c.now.Add(d) +} + +func newTestLocks(t *testing.T, status StatusLookup) (*Locks, *fakeClock) { + t.Helper() + + inmem, cleanup := testutils.NewInMemEntityServer(t) + t.Cleanup(cleanup) + + log := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelError})) + clock := &fakeClock{now: time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC)} + + locks := NewLocks(log, inmem.EAC, status) + locks.now = clock.Now + locks.sleep = func(time.Duration) {} // don't add real latency to contention tests + + return locks, clock +} + +func TestAcquireFreeLock(t *testing.T) { + ctx := context.Background() + locks, clock := newTestLocks(t, nil) + + holder, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + assert.Equal(t, "web", holder.AppName) + assert.Equal(t, "dep-1", holder.DeploymentID) + assert.Equal(t, clock.Now(), holder.AcquiredAt) + assert.Equal(t, clock.Now().Add(DefaultLockTTL), holder.ExpiresAt) +} + +func TestAcquireBlockedByLiveHolder(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return StatusInProgress, nil + }) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + holder, err := locks.Acquire(ctx, "web", "dep-2") + require.ErrorIs(t, err, ErrLockHeld) + require.NotNil(t, holder, "the blocked caller needs the holder to explain the block") + assert.Equal(t, "dep-1", holder.DeploymentID) +} + +// The same deployment retrying its own acquire must not deadlock itself. +func TestAcquireIsIdempotentForSameDeployment(t *testing.T) { + ctx := context.Background() + locks, clock := newTestLocks(t, func(context.Context, string) (Status, error) { + return StatusInProgress, nil + }) + + first, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + clock.Advance(time.Minute) + + second, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + assert.Equal(t, "dep-1", second.DeploymentID) + assert.True(t, second.ExpiresAt.After(first.ExpiresAt), + "re-acquiring should extend the expiry") +} + +func TestAcquireStealsExpiredLock(t *testing.T) { + ctx := context.Background() + locks, clock := newTestLocks(t, func(context.Context, string) (Status, error) { + // Still claims to be running — expiry alone must be enough. + return StatusInProgress, nil + }) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + clock.Advance(DefaultLockTTL + time.Second) + + holder, err := locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err) + assert.Equal(t, "dep-2", holder.DeploymentID) +} + +// Lock and record can drift apart: the build failed but the lock outlived it. +// The next deploy should not have to wait out the full TTL for that. +func TestAcquireStealsLockWhoseDeploymentIsTerminal(t *testing.T) { + ctx := context.Background() + + status := StatusInProgress + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return status, nil + }) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + _, err = locks.Acquire(ctx, "web", "dep-2") + require.ErrorIs(t, err, ErrLockHeld, "still running, so still blocking") + + status = StatusFailed + + holder, err := locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err, "a finished deployment must not keep blocking") + assert.Equal(t, "dep-2", holder.DeploymentID) +} + +func TestAcquireStealsLockWhoseDeploymentIsGone(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return "", cond.NotFound("deployment", "dep-1") + }) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + holder, err := locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err, "nothing will ever release a lock whose record vanished") + assert.Equal(t, "dep-2", holder.DeploymentID) +} + +// A lookup that errors for reasons other than absence must fail closed: we do +// not know the holder is dead, so we must not steal from it. +func TestAcquireDoesNotStealWhenStatusUnreadable(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return "", errors.New("entity store unavailable") + }) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + _, err = locks.Acquire(ctx, "web", "dep-2") + require.ErrorIs(t, err, ErrLockHeld) +} + +// Without a StatusLookup the only ground for stealing is expiry. +func TestAcquireWithoutStatusLookupBlocksUntilExpiry(t *testing.T) { + ctx := context.Background() + locks, clock := newTestLocks(t, nil) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + _, err = locks.Acquire(ctx, "web", "dep-2") + require.ErrorIs(t, err, ErrLockHeld) + + clock.Advance(DefaultLockTTL) + + _, err = locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err) +} + +func TestReleaseFreesTheLock(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return StatusInProgress, nil + }) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + require.NoError(t, locks.Release(ctx, "web", "dep-1")) + + holder, err := locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err) + assert.Equal(t, "dep-2", holder.DeploymentID) +} + +// A deployment that was stolen from must not release its successor's lock on +// the way out — that would hand a third deploy a lock it never won. +func TestReleaseByNonHolderIsIgnored(t *testing.T) { + ctx := context.Background() + locks, clock := newTestLocks(t, nil) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + clock.Advance(DefaultLockTTL) + + _, err = locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err) + + require.NoError(t, locks.Release(ctx, "web", "dep-1"), + "releasing a lock you no longer hold is a no-op, not an error") + + holder, err := locks.Get(ctx, "web") + require.NoError(t, err) + assert.Equal(t, "dep-2", holder.DeploymentID, "dep-2 must still hold the lock") +} + +// The race that an unguarded delete lost. Release reads the holder, and in that +// window the holder's deployment finishes and a queued deploy legitimately +// steals the lock. Releasing must not then free the successor's lock, because a +// third deploy would start alongside it. +func TestReleaseDoesNotFreeALockStolenInTheReadWindow(t *testing.T) { + ctx := context.Background() + + status := StatusInProgress + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return status, nil + }) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + // Interleave dep-2's steal between dep-1's read of the holder and its write + // of the release — the exact window a check-then-act delete cannot guard. + var stole bool + releaseRaceHook = func() { + releaseRaceHook = nil // once, and not re-entrantly + + status = StatusFailed // dep-1 has finished, so its lock is stealable + _, err := locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err) + stole = true + } + t.Cleanup(func() { releaseRaceHook = nil }) + + require.NoError(t, locks.Release(ctx, "web", "dep-1")) + require.True(t, stole, "the hook must have run, or this proves nothing") + + holder, err := locks.Get(ctx, "web") + require.NoError(t, err) + assert.Equal(t, "dep-2", holder.DeploymentID, + "dep-1's release must not have taken dep-2's lock") + + status = StatusInProgress // dep-2 is running + + blocking, err := locks.Blocking(ctx, "web") + require.NoError(t, err) + require.NotNil(t, blocking, "dep-2 still holds the lock") + assert.Equal(t, "dep-2", blocking.DeploymentID) + + _, err = locks.Acquire(ctx, "web", "dep-3") + require.ErrorIs(t, err, ErrLockHeld, + "a third deploy must not be able to start alongside dep-2") +} + +// A released lock leaves a tombstone rather than disappearing, so the free/held +// distinction has to survive that. +func TestBlockingReportsOnlyRealBlockers(t *testing.T) { + ctx := context.Background() + + status := StatusInProgress + locks, clock := newTestLocks(t, func(context.Context, string) (Status, error) { + return status, nil + }) + + blocking, err := locks.Blocking(ctx, "web") + require.NoError(t, err) + assert.Nil(t, blocking, "a lock that never existed blocks nobody") + + _, err = locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + blocking, err = locks.Blocking(ctx, "web") + require.NoError(t, err) + require.NotNil(t, blocking) + assert.Equal(t, "dep-1", blocking.DeploymentID) + + status = StatusFailed + blocking, err = locks.Blocking(ctx, "web") + require.NoError(t, err) + assert.Nil(t, blocking, "a lock held for a finished deployment blocks nobody") + + status = StatusInProgress + require.NoError(t, locks.Release(ctx, "web", "dep-1")) + + blocking, err = locks.Blocking(ctx, "web") + require.NoError(t, err) + assert.Nil(t, blocking, "a released tombstone blocks nobody") + + _, err = locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err) + clock.Advance(DefaultLockTTL + time.Second) + + blocking, err = locks.Blocking(ctx, "web") + require.NoError(t, err) + assert.Nil(t, blocking, "an expired lock blocks nobody") +} + +func TestHeld(t *testing.T) { + now := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + held := &Holder{DeploymentID: "dep-1", ExpiresAt: now.Add(time.Minute)} + assert.True(t, held.Held(now)) + + expired := &Holder{DeploymentID: "dep-1", ExpiresAt: now.Add(-time.Minute)} + assert.False(t, expired.Held(now)) + + tombstone := &Holder{ExpiresAt: now.Add(time.Minute)} + assert.False(t, tombstone.Held(now), "a lock with no owner is not held") +} + +func TestReleaseUnheldLockIsNoOp(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, nil) + + require.NoError(t, locks.Release(ctx, "web", "dep-1")) +} + +func TestGetFreeLockReportsNotFound(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, nil) + + _, err := locks.Get(ctx, "web") + require.Error(t, err) + assert.True(t, errors.Is(err, cond.ErrNotFound{}), "got %T: %v", err, err) +} + +// The race the old list-then-create scheme lost: many callers starting a deploy +// for the same app+cluster at once. +func TestConcurrentAcquireYieldsExactlyOneWinner(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return StatusInProgress, nil + }) + + const contenders = 16 + + var ( + wg sync.WaitGroup + mu sync.Mutex + winners []string + ) + + start := make(chan struct{}) + for i := 0; i < contenders; i++ { + id := "dep-" + string(rune('a'+i)) + wg.Add(1) + go func() { + defer wg.Done() + <-start + + holder, err := locks.Acquire(ctx, "web", id) + if err != nil { + assert.ErrorIs(t, err, ErrLockHeld, "a loser must lose cleanly") + return + } + mu.Lock() + winners = append(winners, holder.DeploymentID) + mu.Unlock() + }() + } + + close(start) + wg.Wait() + + require.Len(t, winners, 1, "exactly one deployment may hold the lock") + + holder, err := locks.Get(ctx, "web") + require.NoError(t, err) + assert.Equal(t, winners[0], holder.DeploymentID, + "the stored lock must agree with the caller that thinks it won") +} + +// A steal that loses the compare-and-swap must back off before retrying, rather +// than spin against the entity store. +func TestAcquireBacksOffOnStealConflict(t *testing.T) { + ctx := context.Background() + + // status makes the holder always stealable, so a contender keeps trying. + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return StatusFailed, nil + }) + + var sleeps int + locks.sleep = func(time.Duration) { sleeps++ } + + // Seed a stealable holder. + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + // Force the replace to conflict once by advancing the lock's revision out + // from under the next acquire: another steal lands between read and write. + stealRaceHook = func() { + stealRaceHook = nil + _, err := locks.Acquire(ctx, "web", "dep-x") + require.NoError(t, err) + } + t.Cleanup(func() { stealRaceHook = nil }) + + _, err = locks.Acquire(ctx, "web", "dep-2") + require.NoError(t, err) + assert.Positive(t, sleeps, "a contended steal must back off before retrying") +} + +func TestAcquireRequiresIdentifiers(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, nil) + + for _, tc := range []struct{ app, deployment string }{ + {"", "dep-1"}, + {"web", ""}, + } { + _, err := locks.Acquire(ctx, tc.app, tc.deployment) + require.Error(t, err, "app=%q deployment=%q", tc.app, tc.deployment) + } +} + +func TestLockIDIsStable(t *testing.T) { + assert.Equal(t, LockID("web"), LockID("web")) + assert.NotEqual(t, LockID("web"), LockID("api")) +} + +// The lock is app-scoped: the same app is one lock regardless of cluster, but +// different apps are different locks. +func TestLocksAreScopedPerApp(t *testing.T) { + ctx := context.Background() + locks, _ := newTestLocks(t, func(context.Context, string) (Status, error) { + return StatusInProgress, nil + }) + + _, err := locks.Acquire(ctx, "web", "dep-1") + require.NoError(t, err) + + // A second deploy of the same app is blocked, even if the client meant a + // different cluster — the coordinator only serves one cluster, and the + // client cluster string is unreliable. + _, err = locks.Acquire(ctx, "web", "dep-2") + require.ErrorIs(t, err, ErrLockHeld, "the same app is a single lock") + + _, err = locks.Acquire(ctx, "api", "dep-3") + require.NoError(t, err, "a different app is a different lock") +} diff --git a/pkg/deploylifecycle/store.go b/pkg/deploylifecycle/store.go new file mode 100644 index 00000000..a02ae2c8 --- /dev/null +++ b/pkg/deploylifecycle/store.go @@ -0,0 +1,247 @@ +package deploylifecycle + +import ( + "context" + "errors" + "fmt" + "log/slog" + "sort" + "time" + + "miren.dev/runtime/api/core/core_v1alpha" + "miren.dev/runtime/api/entityserver/entityserver_v1alpha" + "miren.dev/runtime/pkg/cond" + "miren.dev/runtime/pkg/entity" +) + +// pendingBuildSentinel and the failed- pattern are values older clients wrote +// into app_version before a build had produced one. The field is optional now, so +// they are normalized away on read rather than migrated. +const pendingBuildSentinel = "pending-build" + +// Record is a deployment entity together with what a caller needs to write it +// back: the revision it was read at, and the raw entity for short-id rendering. +type Record struct { + Deployment *core_v1alpha.Deployment + Entity *entityserver_v1alpha.Entity + Revision int64 +} + +// Status returns the record's status as a typed value. +func (r *Record) Status() Status { + return Status(r.Deployment.Status) +} + +// AppVersion returns the recorded app version, with legacy placeholder values +// reported as empty — they never named a real version. +func (r *Record) AppVersion() string { + version := r.Deployment.AppVersion + if version == pendingBuildSentinel || isFailedSentinel(version, string(r.Deployment.ID)) { + return "" + } + return version +} + +func isFailedSentinel(version, deploymentID string) bool { + return deploymentID != "" && version == "failed-"+deploymentID +} + +// Query selects deployments. An empty field means "any". +// +// There is deliberately no cluster filter: a coordinator's store only holds its +// own cluster's deployments, and the client-supplied cluster_id is unreliable, +// so filtering on it would hide legitimate deploys (see MIR-1465). +type Query struct { + AppName string + Status Status + + // Limit caps the result after sorting newest-first. Zero means no cap. + Limit int +} + +// Store reads and writes deployment records. +// +// Every query goes through an index when the filters allow one. The previous +// implementation listed every deployment ever created and filtered in memory on +// each history query, lock check, and activation. +type Store struct { + eac *entityserver_v1alpha.EntityAccessClient + log *slog.Logger + + // now is a test seam; the zero value means time.Now. + now clockFn +} + +func NewStore(log *slog.Logger, eac *entityserver_v1alpha.EntityAccessClient) *Store { + return &Store{ + eac: eac, + log: log.With("module", "deploylifecycle.store"), + } +} + +// Get reads one deployment record. +func (s *Store) Get(ctx context.Context, deploymentID string) (*Record, error) { + res, err := s.eac.Get(ctx, deploymentID) + if err != nil { + return nil, err + } + + return recordFrom(res.Entity()), nil +} + +// Status reports just the status of a deployment, and satisfies StatusLookup so +// the lock manager can tell a live holder from a finished one. +func (s *Store) Status(ctx context.Context, deploymentID string) (Status, error) { + rec, err := s.Get(ctx, deploymentID) + if err != nil { + return "", err + } + return rec.Status(), nil +} + +// List returns matching deployments, newest first. +func (s *Store) List(ctx context.Context, q Query) ([]*Record, error) { + res, err := s.eac.List(ctx, s.index(q)) + if err != nil { + return nil, fmt.Errorf("failed to list deployments: %w", err) + } + + records := make([]*Record, 0, len(res.Values())) + for _, e := range res.Values() { + rec := recordFrom(e) + if !q.matches(rec.Deployment) { + continue + } + records = append(records, rec) + } + + // 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 + }) + + if q.Limit > 0 && len(records) > q.Limit { + records = records[:q.Limit] + } + + return records, nil +} + +// index picks the narrowest index the query's filters allow. Filters it does not +// cover are still applied in memory, but over a far smaller set than a kind scan. +func (s *Store) index(q Query) entity.Attr { + switch { + case q.AppName != "": + return entity.String(core_v1alpha.DeploymentAppNameId, q.AppName) + case q.Status != "": + return entity.String(core_v1alpha.DeploymentStatusId, string(q.Status)) + default: + return entity.Ref(entity.EntityKind, core_v1alpha.KindDeployment) + } +} + +func (q Query) matches(dep *core_v1alpha.Deployment) bool { + if q.AppName != "" && dep.AppName != q.AppName { + return false + } + if q.Status != "" && dep.Status != string(q.Status) { + return false + } + return true +} + +// Put writes a record back under its revision, so a concurrent writer causes a +// conflict rather than a silent overwrite. +func (s *Store) Put(ctx context.Context, rec *Record) error { + ent := &entityserver_v1alpha.Entity{} + ent.SetId(string(rec.Deployment.ID)) + ent.SetAttrs(rec.Deployment.Encode()) + ent.SetRevision(rec.Revision) + + if _, err := s.eac.Put(ctx, ent); err != nil { + return fmt.Errorf("failed to write deployment %s: %w", rec.Deployment.ID, err) + } + + return nil +} + +// Create writes a new deployment record and returns it with its assigned id. +func (s *Store) Create(ctx context.Context, dep *core_v1alpha.Deployment) (*Record, error) { + ent := &entityserver_v1alpha.Entity{} + ent.SetAttrs(dep.Encode()) + + res, err := s.eac.Put(ctx, ent) + if err != nil { + return nil, fmt.Errorf("failed to create deployment: %w", err) + } + + dep.ID = entity.Id(res.Id()) + + // Read back so the caller holds a revision it can write against. + return s.Get(ctx, res.Id()) +} + +// Delete removes a deployment record. +func (s *Store) Delete(ctx context.Context, deploymentID string) error { + if _, err := s.eac.Delete(ctx, deploymentID); err != nil { + if errors.Is(err, cond.ErrNotFound{}) { + return nil + } + return fmt.Errorf("failed to delete deployment %s: %w", deploymentID, err) + } + return nil +} + +// MarkPreviousActiveAs moves the deployments that were active for this app into +// a settled status, leaving the incoming deployment alone. +func (s *Store) MarkPreviousActiveAs(ctx context.Context, appName, exceptID string, target Status) error { + previous, err := s.List(ctx, Query{ + AppName: appName, + Status: StatusActive, + }) + if err != nil { + return err + } + + for _, rec := range previous { + if string(rec.Deployment.ID) == exceptID { + continue + } + + if err := Transition(rec.Status(), target); err != nil { + s.log.Warn("skipping previous deployment that cannot settle", + "deployment_id", rec.Deployment.ID, "from", rec.Status(), "to", target, "error", err) + continue + } + + rec.Deployment.Status = string(target) + if rec.Deployment.CompletedAt == "" { + rec.Deployment.CompletedAt = s.now.Now().Format(time.RFC3339) + } + + if err := s.Put(ctx, rec); err != nil { + // One superseded record failing to settle must not fail the deploy + // that superseded it; it is bookkeeping, and the next activation + // will try again. + s.log.Error("failed to settle previous active deployment", + "deployment_id", rec.Deployment.ID, "target", target, "error", err) + continue + } + + s.log.Info("settled previous active deployment", + "deployment_id", rec.Deployment.ID, "status", target, "app", appName) + } + + return nil +} + +func recordFrom(e *entityserver_v1alpha.Entity) *Record { + var dep core_v1alpha.Deployment + dep.Decode(&entityAttrs{entity: e}) + + return &Record{ + Deployment: &dep, + Entity: e, + Revision: e.Revision(), + } +} diff --git a/pkg/deploylifecycle/store_test.go b/pkg/deploylifecycle/store_test.go new file mode 100644 index 00000000..62ff9590 --- /dev/null +++ b/pkg/deploylifecycle/store_test.go @@ -0,0 +1,303 @@ +package deploylifecycle + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "miren.dev/runtime/api/core/core_v1alpha" + "miren.dev/runtime/pkg/cond" + "miren.dev/runtime/pkg/entity" + "miren.dev/runtime/pkg/entity/testutils" +) + +func newTestStore(t *testing.T) *Store { + t.Helper() + + inmem, cleanup := testutils.NewInMemEntityServer(t) + t.Cleanup(cleanup) + + log := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelError})) + return NewStore(log, inmem.EAC) +} + +// seed writes a deployment and returns its id. at controls sort order. +func seed(t *testing.T, s *Store, app, cluster string, status Status, at time.Time) string { + t.Helper() + + rec, err := s.Create(context.Background(), &core_v1alpha.Deployment{ + AppName: app, + ClusterId: cluster, + Status: string(status), + Phase: string(PhasePreparing), + DeployedBy: core_v1alpha.DeployedBy{ + Timestamp: at.Format(time.RFC3339), + }, + }) + require.NoError(t, err) + + return string(rec.Deployment.ID) +} + +// TestIndexSelection pins the index each query shape uses. Getting this wrong +// does not break correctness — the in-memory filters still apply — it silently +// reintroduces the full scan this type exists to remove. +func TestIndexSelection(t *testing.T) { + s := newTestStore(t) + + tests := []struct { + name string + query Query + want entity.Attr + }{ + { + name: "app name", + query: Query{AppName: "web"}, + want: entity.String(core_v1alpha.DeploymentAppNameId, "web"), + }, + { + name: "app name wins over status", + query: Query{AppName: "web", Status: StatusActive}, + want: entity.String(core_v1alpha.DeploymentAppNameId, "web"), + }, + { + name: "status only", + query: Query{Status: StatusInProgress}, + want: entity.String(core_v1alpha.DeploymentStatusId, "in_progress"), + }, + { + name: "unfiltered falls back to a kind scan", + query: Query{}, + want: entity.Ref(entity.EntityKind, core_v1alpha.KindDeployment), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := s.index(tc.query) + assert.Equal(t, tc.want.ID, got.ID) + assert.True(t, tc.want.Value.Equal(got.Value), + "want %v, got %v", tc.want.Value, got.Value) + }) + } +} + +func TestListFiltersByAppAndStatus(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + base := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + want := seed(t, s, "web", "prod", StatusInProgress, base) + seed(t, s, "api", "prod", StatusInProgress, base) // different app + seed(t, s, "web", "prod", StatusActive, base) // different status + + got, err := s.List(ctx, Query{AppName: "web", Status: StatusInProgress}) + require.NoError(t, err) + + require.Len(t, got, 1) + assert.Equal(t, want, string(got[0].Deployment.ID)) +} + +// A deploy from CI and a manual deploy stamp the same app with different +// cluster_id strings; List must return both, never filter one out by cluster. +func TestListDoesNotFilterByCluster(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + base := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + seed(t, s, "web", "garden", StatusSucceeded, base) // manual deploy + seed(t, s, "web", "34.122.229.118:8443", StatusSucceeded, base.Add(time.Hour)) // CI/OIDC deploy + + got, err := s.List(ctx, Query{AppName: "web"}) + require.NoError(t, err) + assert.Len(t, got, 2, "both deploys of the same app must show, regardless of cluster string") +} + +func TestListSortsNewestFirstAndLimits(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + base := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + oldest := seed(t, s, "web", "prod", StatusSucceeded, base) + middle := seed(t, s, "web", "prod", StatusSucceeded, base.Add(time.Hour)) + newest := seed(t, s, "web", "prod", StatusSucceeded, base.Add(2*time.Hour)) + + got, err := s.List(ctx, Query{AppName: "web"}) + require.NoError(t, err) + require.Len(t, got, 3) + assert.Equal(t, []string{newest, middle, oldest}, ids(got)) + + limited, err := s.List(ctx, Query{AppName: "web", Limit: 2}) + require.NoError(t, err) + assert.Equal(t, []string{newest, middle}, ids(limited), + "the limit must keep the newest, not an arbitrary two") +} + +func TestListUnfilteredReturnsEverything(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + base := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + seed(t, s, "web", "prod", StatusActive, base) + seed(t, s, "api", "staging", StatusFailed, base) + + got, err := s.List(ctx, Query{}) + require.NoError(t, err) + assert.Len(t, got, 2) +} + +func TestListEmptyResult(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + + got, err := s.List(ctx, Query{AppName: "nonexistent"}) + require.NoError(t, err) + assert.Empty(t, got) +} + +func TestStatusLookup(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + + id := seed(t, s, "web", "prod", StatusInProgress, time.Now()) + + status, err := s.Status(ctx, id) + require.NoError(t, err) + assert.Equal(t, StatusInProgress, status) + + _, err = s.Status(ctx, "deployment/missing") + require.Error(t, err) + assert.True(t, errors.Is(err, cond.ErrNotFound{}), + "a missing record must report NotFound so the lock can steal from it; got %T", err) +} + +// The Store satisfies the lock manager's StatusLookup without an adapter, which +// is what lets an abandoned lock be reconciled against its record. +func TestStoreSatisfiesStatusLookup(t *testing.T) { + s := newTestStore(t) + var _ StatusLookup = s.Status +} + +func TestAppVersionNormalizesLegacySentinels(t *testing.T) { + tests := []struct { + name string + id string + version string + want string + }{ + {name: "real version", id: "deployment/1", version: "app_version/abc", want: "app_version/abc"}, + {name: "pending-build", id: "deployment/1", version: "pending-build", want: ""}, + {name: "failed sentinel", id: "deployment/1", version: "failed-deployment/1", want: ""}, + {name: "unset", id: "deployment/1", version: "", want: ""}, + { + name: "failed sentinel for another deployment is left alone", + id: "deployment/1", + version: "failed-deployment/2", + want: "failed-deployment/2", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + rec := &Record{Deployment: &core_v1alpha.Deployment{ + ID: entity.Id(tc.id), + AppVersion: tc.version, + }} + assert.Equal(t, tc.want, rec.AppVersion()) + }) + } +} + +func TestMarkPreviousActiveAs(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + base := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + previous := seed(t, s, "web", "garden", StatusActive, base) + incoming := seed(t, s, "web", "garden", StatusActive, base.Add(time.Hour)) + otherApp := seed(t, s, "api", "garden", StatusActive, base) + // Same app, a different cluster_id string (the CI/manual split). App-scoped + // activation settles it too — it is the same app on the same coordinator. + otherClusterString := seed(t, s, "web", "34.122.229.118:8443", StatusActive, base) + + require.NoError(t, s.MarkPreviousActiveAs(ctx, "web", incoming, StatusSucceeded)) + + assertStatus(t, s, previous, StatusSucceeded) + assertStatus(t, s, incoming, StatusActive) + assertStatus(t, s, otherApp, StatusActive) + assertStatus(t, s, otherClusterString, StatusSucceeded) + + rec, err := s.Get(ctx, previous) + require.NoError(t, err) + assert.NotEmpty(t, rec.Deployment.CompletedAt, "a settled deployment is completed") +} + +func TestMarkPreviousActiveAsRolledBack(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + base := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + previous := seed(t, s, "web", "prod", StatusActive, base) + incoming := seed(t, s, "web", "prod", StatusActive, base.Add(time.Hour)) + + require.NoError(t, s.MarkPreviousActiveAs(ctx, "web", incoming, StatusRolledBack)) + + assertStatus(t, s, previous, StatusRolledBack) +} + +// Only active deployments are settled. An in-progress one is someone else's +// live deploy and must not be trampled. +func TestMarkPreviousActiveAsLeavesInProgressAlone(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + base := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + running := seed(t, s, "web", "prod", StatusInProgress, base) + incoming := seed(t, s, "web", "prod", StatusActive, base.Add(time.Hour)) + + require.NoError(t, s.MarkPreviousActiveAs(ctx, "web", incoming, StatusSucceeded)) + + assertStatus(t, s, running, StatusInProgress) +} + +func TestPutRejectsStaleRevision(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + + id := seed(t, s, "web", "prod", StatusInProgress, time.Now()) + + stale, err := s.Get(ctx, id) + require.NoError(t, err) + + fresh, err := s.Get(ctx, id) + require.NoError(t, err) + fresh.Deployment.Phase = string(PhaseBuilding) + require.NoError(t, s.Put(ctx, fresh)) + + stale.Deployment.Phase = string(PhasePushing) + err = s.Put(ctx, stale) + require.Error(t, err, "a write against a superseded revision must not silently win") + assert.True(t, isConflict(err), + "callers distinguish a lost race from a broken store, so it must stay a conflict through the wrap; got %v", err) +} + +func ids(records []*Record) []string { + out := make([]string, len(records)) + for i, r := range records { + out[i] = string(r.Deployment.ID) + } + return out +} + +func assertStatus(t *testing.T, s *Store, id string, want Status) { + t.Helper() + + rec, err := s.Get(context.Background(), id) + require.NoError(t, err) + assert.Equal(t, want, rec.Status(), "status of %s", id) +} diff --git a/pkg/deploylifecycle/tracker.go b/pkg/deploylifecycle/tracker.go new file mode 100644 index 00000000..026d5f51 --- /dev/null +++ b/pkg/deploylifecycle/tracker.go @@ -0,0 +1,352 @@ +package deploylifecycle + +import ( + "context" + "errors" + "fmt" + "log/slog" + "time" + + "miren.dev/runtime/api/core/core_v1alpha" + "miren.dev/runtime/api/entityserver/entityserver_v1alpha" + "miren.dev/runtime/pkg/cond" +) + +// updateRetryLimit bounds the read-modify-write loop. A conflict means another +// writer touched the record between our read and our write, so retrying is the +// correct response; the cap only exists to stop a pathological loop. +const updateRetryLimit = 100 + +// Tracker is the deployment lifecycle as a set of operations, and the surface +// the build paths call. It exists so the record is a byproduct of the work +// actually happening rather than something a client narrates. +// +// Every mutation goes through it, which is what makes the state machine and the +// lock unavoidable rather than merely available. +type Tracker struct { + store *Store + locks *Locks + log *slog.Logger + + // now is a test seam; the zero value means time.Now. + now clockFn +} + +// NewTracker wires a tracker over the entity store. The lock manager is given +// the store's status lookup, so an abandoned lock can be reconciled against the +// record it claims to be holding for. +func NewTracker(log *slog.Logger, eac *entityserver_v1alpha.EntityAccessClient) *Tracker { + store := NewStore(log, eac) + + return &Tracker{ + store: store, + locks: NewLocks(log, eac, store.Status), + log: log.With("module", "deploylifecycle.tracker"), + } +} + +// Store exposes the underlying store for read paths (history, lock inspection) +// that do not mutate the lifecycle. +func (t *Tracker) Store() *Store { return t.store } + +// Locks exposes the lock manager for read-only inspection, such as the +// pre-flight check a client makes before uploading a build context. +func (t *Tracker) Locks() *Locks { return t.locks } + +// BeginParams describes a deployment about to start. +type BeginParams struct { + AppName string + ClusterID string + + // AppVersion is normally empty: a forward deploy does not know its version + // until the build produces one. Rollback knows it up front. + AppVersion string + + GitInfo core_v1alpha.GitInfo + DeployedBy core_v1alpha.DeployedBy + + // SourceDeploymentID records what this deployment was derived from, for + // rollback and redeploy provenance. + SourceDeploymentID string +} + +// Begin creates the deployment record and takes the deploy lock for it. +// +// If another live deployment holds the lock, Begin returns a *LockHeldError +// (matching errors.Is(err, ErrLockHeld)) describing the blocker, and leaves no +// record behind. +func (t *Tracker) Begin(ctx context.Context, params BeginParams) (*Record, error) { + if params.AppName == "" || params.ClusterID == "" { + return nil, cond.ValidationFailure("missing-field", + "app_name and cluster_id are required to begin a deployment") + } + + deployedBy := params.DeployedBy + if deployedBy.Timestamp == "" { + deployedBy.Timestamp = t.now.Now().Format(time.RFC3339) + } + + rec, err := t.store.Create(ctx, &core_v1alpha.Deployment{ + AppName: params.AppName, + ClusterId: params.ClusterID, + AppVersion: params.AppVersion, + Status: string(StatusInProgress), + Phase: string(PhasePreparing), + DeployedBy: deployedBy, + GitInfo: params.GitInfo, + SourceDeploymentId: params.SourceDeploymentID, + }) + if err != nil { + return nil, err + } + + deploymentID := string(rec.Deployment.ID) + + if _, err := t.locks.Acquire(ctx, params.AppName, deploymentID); err != nil { + // We hold no lock, so this record describes a deploy that will never + // run. Leaving it would show up in history as a deployment stuck + // in_progress forever. + t.discard(ctx, rec) + return nil, err + } + + t.log.Info("began deployment", + "deployment_id", deploymentID, + "app", params.AppName, + "cluster", params.ClusterID) + + return rec, nil +} + +// discard removes a record that never became a real deployment. +func (t *Tracker) discard(ctx context.Context, rec *Record) { + if err := t.store.Delete(ctx, string(rec.Deployment.ID)); err != nil { + t.log.Error("failed to discard deployment record that never acquired its lock", + "deployment_id", rec.Deployment.ID, "error", err) + } +} + +// SetPhase records fine-grained progress. Phases only mean something while a +// deployment is in flight, so setting one on a settled record is a conflict. +func (t *Tracker) SetPhase(ctx context.Context, deploymentID string, phase Phase) error { + return t.update(ctx, deploymentID, func(rec *Record) error { + if err := CheckPhase(rec.Status(), phase); err != nil { + return err + } + + rec.Deployment.Phase = string(phase) + return nil + }) +} + +// SetAppVersion records the version the build produced. It replaces the +// "pending-build" placeholder the client used to write at creation time. +func (t *Tracker) SetAppVersion(ctx context.Context, deploymentID, appVersionID string) error { + if appVersionID == "" { + return cond.ValidationFailure("missing-field", "app_version_id is required") + } + + return t.update(ctx, deploymentID, func(rec *Record) error { + if rec.Status() != StatusInProgress { + return cond.Conflict("deployment-status", + fmt.Sprintf("cannot set app version on deployment in %s state", rec.Status())) + } + + rec.Deployment.AppVersion = appVersionID + return nil + }) +} + +// Activate marks the deployment live and settles the one it replaced as +// succeeded. The lock is released: the deploy is over. +func (t *Tracker) Activate(ctx context.Context, deploymentID string) error { + return t.activate(ctx, deploymentID, StatusSucceeded) +} + +// ActivateRollback is Activate for a rollback, which settles the deployment it +// replaced as rolled_back rather than succeeded. +func (t *Tracker) ActivateRollback(ctx context.Context, deploymentID string) error { + return t.activate(ctx, deploymentID, StatusRolledBack) +} + +func (t *Tracker) activate(ctx context.Context, deploymentID string, supersede Status) error { + var settled *Record + + err := t.update(ctx, deploymentID, func(rec *Record) error { + // The version is required here rather than at creation: this is the + // point where a deployment claims to be running a specific image. + if rec.AppVersion() == "" { + return cond.ValidationFailure("missing-field", + "cannot activate a deployment with no app version") + } + + if err := Transition(rec.Status(), StatusActive); err != nil { + return err + } + + rec.Deployment.Status = string(StatusActive) + rec.Deployment.CompletedAt = t.now.Now().Format(time.RFC3339) + settled = rec + return nil + }) + if err != nil { + return err + } + + // Bookkeeping past this point must not fail an activation that already + // happened — the new version is live either way. + if err := t.store.MarkPreviousActiveAs(ctx, + settled.Deployment.AppName, deploymentID, supersede); err != nil { + t.log.Error("failed to settle previously active deployments", + "deployment_id", deploymentID, "error", err) + } + + t.release(ctx, settled) + return nil +} + +// Fail records a failed deployment and releases the lock. +// +// A deployment that was cancelled stays cancelled: the operator's action is the +// more meaningful account of what happened, and the build failing afterwards is +// a consequence of it. +func (t *Tracker) Fail(ctx context.Context, deploymentID, errorMessage, buildLogs string) error { + var settled *Record + + err := t.update(ctx, deploymentID, func(rec *Record) error { + if rec.Status() == StatusCancelled { + // A build failing after an operator cancelled it does not change + // what happened: the cancellation is the real account. Keep that + // reason and preserve the logs for diagnosis rather than + // overwriting the message with a downstream symptom. + rec.Deployment.BuildLogs = buildLogs + settled = rec + return nil + } + + if err := Transition(rec.Status(), StatusFailed); err != nil { + return err + } + rec.Deployment.Status = string(StatusFailed) + rec.Deployment.ErrorMessage = errorMessage + rec.Deployment.BuildLogs = buildLogs + rec.Deployment.CompletedAt = t.now.Now().Format(time.RFC3339) + settled = rec + return nil + }) + if err != nil { + return err + } + + t.release(ctx, settled) + return nil +} + +// FailIfUnsettled records a failure only if the deployment has not already +// finished, reporting success either way. +// +// It exists for deferred error handlers, which fire on every exit path including +// the ones that follow a successful activation. Such a handler should not have +// to reason about the state machine: "record a failure unless the deploy already +// finished" is exactly what a defer wants, and a deployment that is already +// active, succeeded or cancelled has a better account of itself than a late +// error does. +func (t *Tracker) FailIfUnsettled(ctx context.Context, deploymentID, errorMessage, buildLogs string) error { + err := t.Fail(ctx, deploymentID, errorMessage, buildLogs) + if err == nil || !errors.Is(err, cond.ErrConflict{}) { + return err + } + + t.log.Debug("not recording failure against an already-settled deployment", + "deployment_id", deploymentID, "error_message", errorMessage) + return nil +} + +// Cancel stops an in-flight deployment and releases the lock. +func (t *Tracker) Cancel(ctx context.Context, deploymentID, reason string) error { + var settled *Record + + err := t.update(ctx, deploymentID, func(rec *Record) error { + if err := Transition(rec.Status(), StatusCancelled); err != nil { + return err + } + + rec.Deployment.Status = string(StatusCancelled) + rec.Deployment.CompletedAt = t.now.Now().Format(time.RFC3339) + if reason != "" { + rec.Deployment.ErrorMessage = reason + } + settled = rec + return nil + }) + if err != nil { + return err + } + + t.release(ctx, settled) + return nil +} + +// ReleaseLock frees the deploy lock held by a deployment without changing the +// record, looking the app+cluster up from the record itself. +// +// It is a backstop for a caller whose activation already made the version live +// but whose record settle failed: releasing the lock keeps a record that will +// never settle from stalling every later deploy of that app+cluster for the +// full lock TTL. Release is a no-op if a newer deployment already holds the lock. +func (t *Tracker) ReleaseLock(ctx context.Context, deploymentID string) error { + rec, err := t.store.Get(ctx, deploymentID) + if err != nil { + return err + } + return t.locks.Release(ctx, rec.Deployment.AppName, deploymentID) +} + +// release drops the deploy lock for a settled deployment. A failure here is +// logged rather than returned: the deployment's own outcome is already recorded, +// and the lock will expire on its own. +func (t *Tracker) release(ctx context.Context, rec *Record) { + err := t.locks.Release(ctx, rec.Deployment.AppName, string(rec.Deployment.ID)) + if err != nil { + t.log.Error("failed to release deploy lock", + "deployment_id", rec.Deployment.ID, + "app", rec.Deployment.AppName, + "error", err) + } +} + +// update applies mutate to a deployment record and writes it back, retrying the +// whole read-modify-write when another writer got there first. +func (t *Tracker) update(ctx context.Context, deploymentID string, mutate func(*Record) error) error { + if deploymentID == "" { + return cond.ValidationFailure("missing-field", "deployment_id is required") + } + + for attempt := 0; attempt < updateRetryLimit; attempt++ { + rec, err := t.store.Get(ctx, deploymentID) + if err != nil { + return err + } + + if err := mutate(rec); err != nil { + return err + } + + err = t.store.Put(ctx, rec) + if err == nil { + return nil + } + + if !errors.Is(err, cond.ErrConflict{}) { + return err + } + + // 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) + } + + return fmt.Errorf("gave up updating deployment %s after %d attempts", + deploymentID, updateRetryLimit) +} diff --git a/pkg/deploylifecycle/tracker_test.go b/pkg/deploylifecycle/tracker_test.go new file mode 100644 index 00000000..f8fe69c1 --- /dev/null +++ b/pkg/deploylifecycle/tracker_test.go @@ -0,0 +1,464 @@ +package deploylifecycle + +import ( + "context" + "io" + "log/slog" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "miren.dev/runtime/api/core/core_v1alpha" + "miren.dev/runtime/pkg/entity/testutils" +) + +func newTestTracker(t *testing.T) (*Tracker, *fakeClock) { + t.Helper() + + inmem, cleanup := testutils.NewInMemEntityServer(t) + t.Cleanup(cleanup) + + log := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelError})) + clock := &fakeClock{now: time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC)} + + tracker := NewTracker(log, inmem.EAC) + tracker.now = clock.Now + tracker.store.now = clock.Now + tracker.locks.now = clock.Now + + return tracker, clock +} + +func begin(t *testing.T, tr *Tracker, app, cluster string) string { + t.Helper() + + rec, err := tr.Begin(context.Background(), BeginParams{AppName: app, ClusterID: cluster}) + require.NoError(t, err) + return string(rec.Deployment.ID) +} + +func TestBeginCreatesRecordAndTakesLock(t *testing.T) { + ctx := context.Background() + tr, clock := newTestTracker(t) + + rec, err := tr.Begin(ctx, BeginParams{ + AppName: "web", + ClusterID: "prod", + GitInfo: core_v1alpha.GitInfo{Sha: "abc123", Branch: "main"}, + }) + require.NoError(t, err) + + assert.Equal(t, StatusInProgress, rec.Status()) + assert.Equal(t, string(PhasePreparing), rec.Deployment.Phase) + assert.Equal(t, "abc123", rec.Deployment.GitInfo.Sha) + assert.Equal(t, clock.Now().Format(time.RFC3339), rec.Deployment.DeployedBy.Timestamp) + assert.Empty(t, rec.AppVersion(), "a forward deploy has no version until the build makes one") + + holder, err := tr.Locks().Get(ctx, "web") + require.NoError(t, err) + assert.Equal(t, string(rec.Deployment.ID), holder.DeploymentID) +} + +// The whole point of taking the lock inside Begin: the second caller must not +// end up with a record it will never be able to finish. +func TestBeginLeavesNoRecordWhenLockIsHeld(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + first := begin(t, tr, "web", "prod") + + _, err := tr.Begin(ctx, BeginParams{AppName: "web", ClusterID: "prod"}) + require.ErrorIs(t, err, ErrLockHeld) + + holder, ok := HolderFrom(err) + require.True(t, ok, "the blocked caller must be able to name the blocker") + assert.Equal(t, first, holder.DeploymentID) + + all, err := tr.Store().List(ctx, Query{AppName: "web"}) + require.NoError(t, err) + assert.Len(t, all, 1, "the losing deploy must not leave a record behind") +} + +func TestBeginRequiresAppAndCluster(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + _, err := tr.Begin(ctx, BeginParams{ClusterID: "prod"}) + require.Error(t, err) + + _, err = tr.Begin(ctx, BeginParams{AppName: "web"}) + require.Error(t, err) +} + +func TestSetPhase(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + + for _, phase := range []Phase{PhaseBuilding, PhasePushing, PhaseActivating} { + require.NoError(t, tr.SetPhase(ctx, id, phase)) + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, string(phase), rec.Deployment.Phase) + } +} + +func TestSetPhaseOnSettledDeploymentIsConflict(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + require.NoError(t, tr.Fail(ctx, id, "build broke", "")) + + err := tr.SetPhase(ctx, id, PhaseBuilding) + require.Error(t, err) + assert.True(t, isConflict(err), "got %T: %v", err, err) +} + +func TestSetAppVersion(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, id, "app_version/v1")) + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, "app_version/v1", rec.AppVersion()) + + require.Error(t, tr.SetAppVersion(ctx, id, ""), "an empty version is not a version") +} + +// The sentinel is gone, so the invariant it used to stand in for has to be +// enforced directly. +func TestActivateRequiresAnAppVersion(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + + err := tr.Activate(ctx, id) + require.Error(t, err) + assert.True(t, isValidationFailure(err), "got %T: %v", err, err) + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, StatusInProgress, rec.Status(), "a refused activation must not settle the record") +} + +func TestActivateSettlesPreviousAndReleasesLock(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + first := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, first, "app_version/v1")) + require.NoError(t, tr.Activate(ctx, first)) + + // The lock must be free, or no further deploy could ever start. + second := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, second, "app_version/v2")) + require.NoError(t, tr.Activate(ctx, second)) + + firstRec, err := tr.Store().Get(ctx, first) + require.NoError(t, err) + assert.Equal(t, StatusSucceeded, firstRec.Status()) + + secondRec, err := tr.Store().Get(ctx, second) + require.NoError(t, err) + assert.Equal(t, StatusActive, secondRec.Status()) + assert.NotEmpty(t, secondRec.Deployment.CompletedAt) +} + +func TestActivateRollbackSettlesPreviousAsRolledBack(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + first := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, first, "app_version/v2")) + require.NoError(t, tr.Activate(ctx, first)) + + rollback := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, rollback, "app_version/v1")) + require.NoError(t, tr.ActivateRollback(ctx, rollback)) + + firstRec, err := tr.Store().Get(ctx, first) + require.NoError(t, err) + assert.Equal(t, StatusRolledBack, firstRec.Status(), + "the version we rolled away from was not a success") +} + +func TestFailRecordsErrorAndReleasesLock(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + require.NoError(t, tr.Fail(ctx, id, "compile error", "line 1\nline 2")) + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, StatusFailed, rec.Status()) + assert.Equal(t, "compile error", rec.Deployment.ErrorMessage) + assert.Equal(t, "line 1\nline 2", rec.Deployment.BuildLogs) + assert.NotEmpty(t, rec.Deployment.CompletedAt) + + blocking, err := tr.Locks().Blocking(ctx, "web") + require.NoError(t, err) + assert.Nil(t, blocking, "a failed deploy must not keep the lock") +} + +// A cancelled deploy whose build then dies must still read as cancelled: the +// operator's action is the real account of what happened. +func TestFailAfterCancelKeepsCancelledButRecordsTheError(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + require.NoError(t, tr.Cancel(ctx, id, "operator cancelled")) + require.NoError(t, tr.Fail(ctx, id, "build aborted", "some logs")) + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, StatusCancelled, rec.Status()) + assert.Equal(t, "operator cancelled", rec.Deployment.ErrorMessage, + "the cancellation reason is the real account and must survive a later build failure") + assert.Equal(t, "some logs", rec.Deployment.BuildLogs, + "the build logs are still worth keeping for diagnosis") +} + +// Refusing active -> failed is correct: the version is live, so the deploy did +// not fail. Fail() must say so rather than quietly rewriting history. +func TestFailAfterActivateIsRefused(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, id, "app_version/v1")) + require.NoError(t, tr.Activate(ctx, id)) + + err := tr.Fail(ctx, id, "late error", "") + require.Error(t, err) + assert.True(t, isConflict(err), "got %T: %v", err, err) + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, StatusActive, rec.Status(), "the live deployment must stay active") +} + +// The deferred-handler form of the same situation: fire unconditionally, and a +// deployment that already finished is left exactly as it was. +func TestFailIfUnsettled(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + t.Run("records a real failure", func(t *testing.T) { + id := begin(t, tr, "web", "prod") + + require.NoError(t, tr.FailIfUnsettled(ctx, id, "compile error", "logs")) + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, StatusFailed, rec.Status()) + assert.Equal(t, "compile error", rec.Deployment.ErrorMessage) + }) + + t.Run("leaves an activated deployment alone", func(t *testing.T) { + id := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, id, "app_version/v1")) + require.NoError(t, tr.Activate(ctx, id)) + + require.NoError(t, tr.FailIfUnsettled(ctx, id, "late error", "logs"), + "a defer firing after a successful deploy is not an error") + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, StatusActive, rec.Status()) + assert.Empty(t, rec.Deployment.ErrorMessage, + "a successful deploy must not be annotated with a spurious error") + }) + + t.Run("still surfaces errors that are not about being settled", func(t *testing.T) { + err := tr.FailIfUnsettled(ctx, "deployment/missing", "boom", "") + require.Error(t, err, "a missing deployment is a real problem, not a settled one") + }) +} + +// ReleaseLock is the backstop for a deploy whose activation settle failed after +// the version already went live: it frees the lock without settling the record, +// so the next deploy is not blocked for the full lock TTL. +func TestReleaseLockFreesAStuckDeployment(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + stuck := begin(t, tr, "web", "prod") // in_progress, lock held + + // While that record holds the lock and has not settled, the next deploy is + // blocked — this is the stall the backstop exists to prevent. + _, err := tr.Begin(ctx, BeginParams{AppName: "web", ClusterID: "prod"}) + require.ErrorIs(t, err, ErrLockHeld) + + require.NoError(t, tr.ReleaseLock(ctx, stuck)) + + // The record is untouched (still in_progress) but the lock is free. + rec, err := tr.Store().Get(ctx, stuck) + require.NoError(t, err) + assert.Equal(t, StatusInProgress, rec.Status(), "ReleaseLock must not change the record") + + next, err := tr.Begin(ctx, BeginParams{AppName: "web", ClusterID: "prod"}) + require.NoError(t, err, "the next deploy must proceed once the lock is released") + assert.NotEqual(t, stuck, string(next.Deployment.ID)) +} + +// ReleaseLock must not free a lock a newer deployment already holds. +func TestReleaseLockIsHolderGuarded(t *testing.T) { + ctx := context.Background() + tr, clock := newTestTracker(t) + + first := begin(t, tr, "web", "prod") + + // A second deploy steals the lock once the first's expires. + clock.Advance(DefaultLockTTL + time.Second) + second := begin(t, tr, "web", "prod") + + // The first deployment releasing its (now superseded) lock must be a no-op. + require.NoError(t, tr.ReleaseLock(ctx, first)) + + holder, err := tr.Locks().Get(ctx, "web") + require.NoError(t, err) + assert.Equal(t, second, holder.DeploymentID, "the second deploy must still hold the lock") +} + +func TestReleaseLockUnknownDeployment(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + err := tr.ReleaseLock(ctx, "deployment/missing") + require.Error(t, err, "cannot release a lock for a record that does not exist") +} + +func TestCancelReleasesLock(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + require.NoError(t, tr.Cancel(ctx, id, "operator cancelled")) + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Equal(t, StatusCancelled, rec.Status()) + assert.Equal(t, "operator cancelled", rec.Deployment.ErrorMessage) + + // The lock is free, so the next deploy proceeds. + next := begin(t, tr, "web", "prod") + assert.NotEqual(t, id, next) +} + +func TestCancelSettledDeploymentIsConflict(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, id, "app_version/v1")) + require.NoError(t, tr.Activate(ctx, id)) + + err := tr.Cancel(ctx, id, "too late") + require.Error(t, err) + assert.True(t, isConflict(err), "got %T: %v", err, err) +} + +func TestOperationsOnUnknownDeployment(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + const missing = "deployment/missing" + + assert.Error(t, tr.SetPhase(ctx, missing, PhaseBuilding)) + assert.Error(t, tr.SetAppVersion(ctx, missing, "app_version/v1")) + assert.Error(t, tr.Activate(ctx, missing)) + assert.Error(t, tr.Fail(ctx, missing, "boom", "")) + assert.Error(t, tr.Cancel(ctx, missing, "")) + + assert.Error(t, tr.SetPhase(ctx, "", PhaseBuilding), "an empty id is not a lookup") +} + +// A deploy whose driver died leaves the lock behind. The next deploy should get +// through on the strength of the record being terminal, without waiting out the +// full TTL. +func TestAbandonedLockIsReclaimedOnceRecordIsTerminal(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + + // Settle the record directly, as an out-of-band reaper would, leaving the + // lock in place. + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + rec.Deployment.Status = string(StatusFailed) + require.NoError(t, tr.Store().Put(ctx, rec)) + + next, err := tr.Begin(ctx, BeginParams{AppName: "web", ClusterID: "prod"}) + require.NoError(t, err, "a lock held for a finished deployment must not block forever") + assert.NotEqual(t, id, string(next.Deployment.ID)) +} + +// Phase updates and a cancel can land at the same moment; the retry loop must +// make one of them lose cleanly rather than clobbering the other. +func TestConcurrentUpdatesConverge(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + id := begin(t, tr, "web", "prod") + + var wg sync.WaitGroup + start := make(chan struct{}) + + for _, phase := range []Phase{PhaseBuilding, PhasePushing, PhaseActivating} { + wg.Add(1) + go func() { + defer wg.Done() + <-start + // Either it applies or it conflicts with the cancel; both are fine, + // a lost update is not. + _ = tr.SetPhase(ctx, id, phase) + }() + } + + wg.Add(1) + go func() { + defer wg.Done() + <-start + _ = tr.Cancel(ctx, id, "raced") + }() + + close(start) + wg.Wait() + + rec, err := tr.Store().Get(ctx, id) + require.NoError(t, err) + assert.Contains(t, []Status{StatusInProgress, StatusCancelled}, rec.Status()) +} + +func TestBeginRecordsProvenanceForRollback(t *testing.T) { + ctx := context.Background() + tr, _ := newTestTracker(t) + + source := begin(t, tr, "web", "prod") + require.NoError(t, tr.SetAppVersion(ctx, source, "app_version/v1")) + require.NoError(t, tr.Activate(ctx, source)) + + rec, err := tr.Begin(ctx, BeginParams{ + AppName: "web", + ClusterID: "prod", + AppVersion: "app_version/v1", + SourceDeploymentID: source, + }) + require.NoError(t, err) + + assert.Equal(t, source, rec.Deployment.SourceDeploymentId) + assert.Equal(t, "app_version/v1", rec.AppVersion(), + "a rollback knows its version up front, unlike a build") +} From cb6c9708f7c1f5248677b7aa71fbcd6f3866c832 Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Fri, 24 Jul 2026 11:10:09 -0700 Subject: [PATCH 2/3] Move deployment lifecycle server-side 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-" 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 --- api/build/build_v1alpha/rpc.gen.go | 339 +++++++- api/build/rpc.yml | 93 +++ api/deployment/deployment_v1alpha/rpc.gen.go | 162 ++++ api/deployment/rpc.yml | 44 +- blackbox/deployment_lifecycle_test.go | 144 ++++ cli/commands/deploy.go | 291 +++++-- servers/build/build.go | 96 ++- servers/build/build_saga.go | 131 +++ servers/build/build_saga_deploy_test.go | 179 +++++ servers/build/deploy_tracking.go | 212 +++++ servers/build/deploy_tracking_test.go | 285 +++++++ servers/build/saga_builder.go | 25 +- servers/build/status_registry.go | 16 + servers/build/status_registry_test.go | 24 +- servers/deployment/cross_version_test.go | 104 +++ servers/deployment/lock_integration_test.go | 231 ++++++ servers/deployment/server.go | 789 +++++++------------ servers/deployment/server_test.go | 18 +- testdata/build-error/.miren/app.toml | 1 + testdata/build-error/Procfile | 1 + testdata/build-error/go.mod | 3 + testdata/build-error/main.go | 8 + testdata/slow-build/.miren/app.toml | 1 + testdata/slow-build/Dockerfile.miren | 9 + testdata/slow-build/serve.sh | 4 + 25 files changed, 2597 insertions(+), 613 deletions(-) create mode 100644 blackbox/deployment_lifecycle_test.go create mode 100644 servers/build/build_saga_deploy_test.go create mode 100644 servers/build/deploy_tracking.go create mode 100644 servers/build/deploy_tracking_test.go create mode 100644 servers/deployment/cross_version_test.go create mode 100644 servers/deployment/lock_integration_test.go create mode 100644 testdata/build-error/.miren/app.toml create mode 100644 testdata/build-error/Procfile create mode 100644 testdata/build-error/go.mod create mode 100644 testdata/build-error/main.go create mode 100644 testdata/slow-build/.miren/app.toml create mode 100644 testdata/slow-build/Dockerfile.miren create mode 100755 testdata/slow-build/serve.sh diff --git a/api/build/build_v1alpha/rpc.gen.go b/api/build/build_v1alpha/rpc.gen.go index eed0aa9e..f9d3b581 100644 --- a/api/build/build_v1alpha/rpc.gen.go +++ b/api/build/build_v1alpha/rpc.gen.go @@ -7,6 +7,7 @@ import ( "github.com/fxamacker/cbor/v2" rpc "miren.dev/runtime/pkg/rpc" + "miren.dev/runtime/pkg/rpc/standard" "miren.dev/runtime/pkg/rpc/stream" ) @@ -20,13 +21,16 @@ type StatusUpdate interface { SetError(string) Log() *LogEntry SetLog(*LogEntry) + Deployment() *DeploymentProgress + SetDeployment(*DeploymentProgress) } type statusUpdate struct { - U_Message *string `cbor:"1,keyasint,omitempty" json:"message,omitempty"` - U_Buildkit *[]byte `cbor:"2,keyasint,omitempty" json:"buildkit,omitempty"` - U_Error *string `cbor:"3,keyasint,omitempty" json:"error,omitempty"` - U_Log **LogEntry `cbor:"4,keyasint,omitempty" json:"log,omitempty"` + U_Message *string `cbor:"1,keyasint,omitempty" json:"message,omitempty"` + U_Buildkit *[]byte `cbor:"2,keyasint,omitempty" json:"buildkit,omitempty"` + U_Error *string `cbor:"3,keyasint,omitempty" json:"error,omitempty"` + U_Log **LogEntry `cbor:"4,keyasint,omitempty" json:"log,omitempty"` + U_Deployment **DeploymentProgress `cbor:"5,keyasint,omitempty" json:"deployment,omitempty"` } func (v *statusUpdate) Which() string { @@ -42,6 +46,9 @@ func (v *statusUpdate) Which() string { if v.U_Log != nil { return "log" } + if v.U_Deployment != nil { + return "deployment" + } return "" } @@ -56,6 +63,7 @@ func (v *statusUpdate) SetMessage(val string) { v.U_Buildkit = nil v.U_Error = nil v.U_Log = nil + v.U_Deployment = nil v.U_Message = &val } @@ -70,6 +78,7 @@ func (v *statusUpdate) SetBuildkit(val []byte) { v.U_Message = nil v.U_Error = nil v.U_Log = nil + v.U_Deployment = nil v.U_Buildkit = &val } @@ -84,6 +93,7 @@ func (v *statusUpdate) SetError(val string) { v.U_Message = nil v.U_Buildkit = nil v.U_Log = nil + v.U_Deployment = nil v.U_Error = &val } @@ -98,9 +108,25 @@ func (v *statusUpdate) SetLog(val *LogEntry) { v.U_Message = nil v.U_Buildkit = nil v.U_Error = nil + v.U_Deployment = nil v.U_Log = &val } +func (v *statusUpdate) Deployment() *DeploymentProgress { + if v.U_Deployment == nil { + return nil + } + return *v.U_Deployment +} + +func (v *statusUpdate) SetDeployment(val *DeploymentProgress) { + v.U_Message = nil + v.U_Buildkit = nil + v.U_Error = nil + v.U_Log = nil + v.U_Deployment = &val +} + type statusData struct { Kind *string `cbor:"0,keyasint,omitempty" json:"kind,omitempty"` statusUpdate @@ -145,6 +171,277 @@ func (v *Status) UnmarshalJSON(data []byte) error { return json.Unmarshal(data, &v.data) } +type deploymentProgressData struct { + DeploymentId *string `cbor:"0,keyasint,omitempty" json:"deployment_id,omitempty"` + Phase *string `cbor:"1,keyasint,omitempty" json:"phase,omitempty"` +} + +type DeploymentProgress struct { + data deploymentProgressData +} + +func (v *DeploymentProgress) HasDeploymentId() bool { + return v.data.DeploymentId != nil +} + +func (v *DeploymentProgress) DeploymentId() string { + if v.data.DeploymentId == nil { + return "" + } + return *v.data.DeploymentId +} + +func (v *DeploymentProgress) SetDeploymentId(deployment_id string) { + v.data.DeploymentId = &deployment_id +} + +func (v *DeploymentProgress) HasPhase() bool { + return v.data.Phase != nil +} + +func (v *DeploymentProgress) Phase() string { + if v.data.Phase == nil { + return "" + } + return *v.data.Phase +} + +func (v *DeploymentProgress) SetPhase(phase string) { + v.data.Phase = &phase +} + +func (v *DeploymentProgress) MarshalCBOR() ([]byte, error) { + return cbor.Marshal(v.data) +} + +func (v *DeploymentProgress) UnmarshalCBOR(data []byte) error { + return cbor.Unmarshal(data, &v.data) +} + +func (v *DeploymentProgress) MarshalJSON() ([]byte, error) { + return json.Marshal(v.data) +} + +func (v *DeploymentProgress) UnmarshalJSON(data []byte) error { + return json.Unmarshal(data, &v.data) +} + +type deployRequestData struct { + ClusterId *string `cbor:"0,keyasint,omitempty" json:"cluster_id,omitempty"` + GitInfo *GitInfo `cbor:"1,keyasint,omitempty" json:"git_info,omitempty"` +} + +type DeployRequest struct { + data deployRequestData +} + +func (v *DeployRequest) HasClusterId() bool { + return v.data.ClusterId != nil +} + +func (v *DeployRequest) ClusterId() string { + if v.data.ClusterId == nil { + return "" + } + return *v.data.ClusterId +} + +func (v *DeployRequest) SetClusterId(cluster_id string) { + v.data.ClusterId = &cluster_id +} + +func (v *DeployRequest) HasGitInfo() bool { + return v.data.GitInfo != nil +} + +func (v *DeployRequest) GitInfo() *GitInfo { + return v.data.GitInfo +} + +func (v *DeployRequest) SetGitInfo(git_info *GitInfo) { + v.data.GitInfo = git_info +} + +func (v *DeployRequest) MarshalCBOR() ([]byte, error) { + return cbor.Marshal(v.data) +} + +func (v *DeployRequest) UnmarshalCBOR(data []byte) error { + return cbor.Unmarshal(data, &v.data) +} + +func (v *DeployRequest) MarshalJSON() ([]byte, error) { + return json.Marshal(v.data) +} + +func (v *DeployRequest) UnmarshalJSON(data []byte) error { + return json.Unmarshal(data, &v.data) +} + +type gitInfoData struct { + Sha *string `cbor:"0,keyasint,omitempty" json:"sha,omitempty"` + Branch *string `cbor:"1,keyasint,omitempty" json:"branch,omitempty"` + Repository *string `cbor:"2,keyasint,omitempty" json:"repository,omitempty"` + IsDirty *bool `cbor:"3,keyasint,omitempty" json:"is_dirty,omitempty"` + WorkingTreeHash *string `cbor:"4,keyasint,omitempty" json:"working_tree_hash,omitempty"` + CommitMessage *string `cbor:"5,keyasint,omitempty" json:"commit_message,omitempty"` + CommitAuthorName *string `cbor:"6,keyasint,omitempty" json:"commit_author_name,omitempty"` + CommitAuthorEmail *string `cbor:"7,keyasint,omitempty" json:"commit_author_email,omitempty"` + CommitTimestamp *standard.Timestamp `cbor:"8,keyasint,omitempty" json:"commit_timestamp,omitempty"` +} + +type GitInfo struct { + data gitInfoData +} + +func (v *GitInfo) HasSha() bool { + return v.data.Sha != nil +} + +func (v *GitInfo) Sha() string { + if v.data.Sha == nil { + return "" + } + return *v.data.Sha +} + +func (v *GitInfo) SetSha(sha string) { + v.data.Sha = &sha +} + +func (v *GitInfo) HasBranch() bool { + return v.data.Branch != nil +} + +func (v *GitInfo) Branch() string { + if v.data.Branch == nil { + return "" + } + return *v.data.Branch +} + +func (v *GitInfo) SetBranch(branch string) { + v.data.Branch = &branch +} + +func (v *GitInfo) HasRepository() bool { + return v.data.Repository != nil +} + +func (v *GitInfo) Repository() string { + if v.data.Repository == nil { + return "" + } + return *v.data.Repository +} + +func (v *GitInfo) SetRepository(repository string) { + v.data.Repository = &repository +} + +func (v *GitInfo) HasIsDirty() bool { + return v.data.IsDirty != nil +} + +func (v *GitInfo) IsDirty() bool { + if v.data.IsDirty == nil { + return false + } + return *v.data.IsDirty +} + +func (v *GitInfo) SetIsDirty(is_dirty bool) { + v.data.IsDirty = &is_dirty +} + +func (v *GitInfo) HasWorkingTreeHash() bool { + return v.data.WorkingTreeHash != nil +} + +func (v *GitInfo) WorkingTreeHash() string { + if v.data.WorkingTreeHash == nil { + return "" + } + return *v.data.WorkingTreeHash +} + +func (v *GitInfo) SetWorkingTreeHash(working_tree_hash string) { + v.data.WorkingTreeHash = &working_tree_hash +} + +func (v *GitInfo) HasCommitMessage() bool { + return v.data.CommitMessage != nil +} + +func (v *GitInfo) CommitMessage() string { + if v.data.CommitMessage == nil { + return "" + } + return *v.data.CommitMessage +} + +func (v *GitInfo) SetCommitMessage(commit_message string) { + v.data.CommitMessage = &commit_message +} + +func (v *GitInfo) HasCommitAuthorName() bool { + return v.data.CommitAuthorName != nil +} + +func (v *GitInfo) CommitAuthorName() string { + if v.data.CommitAuthorName == nil { + return "" + } + return *v.data.CommitAuthorName +} + +func (v *GitInfo) SetCommitAuthorName(commit_author_name string) { + v.data.CommitAuthorName = &commit_author_name +} + +func (v *GitInfo) HasCommitAuthorEmail() bool { + return v.data.CommitAuthorEmail != nil +} + +func (v *GitInfo) CommitAuthorEmail() string { + if v.data.CommitAuthorEmail == nil { + return "" + } + return *v.data.CommitAuthorEmail +} + +func (v *GitInfo) SetCommitAuthorEmail(commit_author_email string) { + v.data.CommitAuthorEmail = &commit_author_email +} + +func (v *GitInfo) HasCommitTimestamp() bool { + return v.data.CommitTimestamp != nil +} + +func (v *GitInfo) CommitTimestamp() *standard.Timestamp { + return v.data.CommitTimestamp +} + +func (v *GitInfo) SetCommitTimestamp(commit_timestamp *standard.Timestamp) { + v.data.CommitTimestamp = commit_timestamp +} + +func (v *GitInfo) MarshalCBOR() ([]byte, error) { + return cbor.Marshal(v.data) +} + +func (v *GitInfo) UnmarshalCBOR(data []byte) error { + return cbor.Unmarshal(data, &v.data) +} + +func (v *GitInfo) MarshalJSON() ([]byte, error) { + return json.Marshal(v.data) +} + +func (v *GitInfo) UnmarshalJSON(data []byte) error { + return json.Unmarshal(data, &v.data) +} + type logEntryData struct { Level *string `cbor:"0,keyasint,omitempty" json:"level,omitempty"` Text *string `cbor:"1,keyasint,omitempty" json:"text,omitempty"` @@ -1024,6 +1321,7 @@ type builderBuildFromTarArgsData struct { EnvVars *[]*EnvironmentVariable `cbor:"3,keyasint,omitempty" json:"envVars,omitempty"` EphemeralLabel *string `cbor:"4,keyasint,omitempty" json:"ephemeral_label,omitempty"` EphemeralTtl *string `cbor:"5,keyasint,omitempty" json:"ephemeral_ttl,omitempty"` + Deployment **DeployRequest `cbor:"6,keyasint,omitempty" json:"deployment,omitempty"` } type BuilderBuildFromTarArgs struct { @@ -1097,6 +1395,17 @@ func (v *BuilderBuildFromTarArgs) EphemeralTtl() string { return *v.data.EphemeralTtl } +func (v *BuilderBuildFromTarArgs) HasDeployment() bool { + return v.data.Deployment != nil +} + +func (v *BuilderBuildFromTarArgs) Deployment() *DeployRequest { + if v.data.Deployment == nil { + return nil + } + return *v.data.Deployment +} + func (v *BuilderBuildFromTarArgs) MarshalCBOR() ([]byte, error) { return cbor.Marshal(v.data) } @@ -1301,6 +1610,7 @@ type builderBuildFromPreparedArgsData struct { EnvVars *[]*EnvironmentVariable `cbor:"3,keyasint,omitempty" json:"envVars,omitempty"` EphemeralLabel *string `cbor:"4,keyasint,omitempty" json:"ephemeral_label,omitempty"` EphemeralTtl *string `cbor:"5,keyasint,omitempty" json:"ephemeral_ttl,omitempty"` + Deployment **DeployRequest `cbor:"6,keyasint,omitempty" json:"deployment,omitempty"` } type BuilderBuildFromPreparedArgs struct { @@ -1374,6 +1684,17 @@ func (v *BuilderBuildFromPreparedArgs) EphemeralTtl() string { return *v.data.EphemeralTtl } +func (v *BuilderBuildFromPreparedArgs) HasDeployment() bool { + return v.data.Deployment != nil +} + +func (v *BuilderBuildFromPreparedArgs) Deployment() *DeployRequest { + if v.data.Deployment == nil { + return nil + } + return *v.data.Deployment +} + func (v *BuilderBuildFromPreparedArgs) MarshalCBOR() ([]byte, error) { return cbor.Marshal(v.data) } @@ -1571,7 +1892,7 @@ func AdaptBuilder(t Builder) *rpc.Interface { InterfaceName: "Builder", Index: 0, Public: false, - Params: []string{"application", "tardata", "status", "envVars", "ephemeral_label", "ephemeral_ttl"}, + Params: []string{"application", "tardata", "status", "envVars", "ephemeral_label", "ephemeral_ttl", "deployment"}, Handler: func(ctx context.Context, call rpc.Call) error { return t.BuildFromTar(ctx, &BuilderBuildFromTar{Call: call}) }, @@ -1601,7 +1922,7 @@ func AdaptBuilder(t Builder) *rpc.Interface { InterfaceName: "Builder", Index: 0, Public: false, - Params: []string{"session_id", "tardata", "status", "envVars", "ephemeral_label", "ephemeral_ttl"}, + Params: []string{"session_id", "tardata", "status", "envVars", "ephemeral_label", "ephemeral_ttl", "deployment"}, Handler: func(ctx context.Context, call rpc.Call) error { return t.BuildFromPrepared(ctx, &BuilderBuildFromPrepared{Call: call}) }, @@ -1661,7 +1982,7 @@ func (v *BuilderClientBuildFromTarResults) VersionShortId() string { return *v.data.VersionShortId } -func (v BuilderClient) BuildFromTar(ctx context.Context, application string, tardata stream.RecvStream[[]byte], status stream.SendStream[*Status], envVars []*EnvironmentVariable, ephemeral_label string, ephemeral_ttl string) (*BuilderClientBuildFromTarResults, error) { +func (v BuilderClient) BuildFromTar(ctx context.Context, application string, tardata stream.RecvStream[[]byte], status stream.SendStream[*Status], envVars []*EnvironmentVariable, ephemeral_label string, ephemeral_ttl string, deployment *DeployRequest) (*BuilderClientBuildFromTarResults, error) { args := BuilderBuildFromTarArgs{} caps := map[rpc.OID]*rpc.InlineCapability{} args.data.Application = &application @@ -1678,6 +1999,7 @@ func (v BuilderClient) BuildFromTar(ctx context.Context, application string, tar args.data.EnvVars = &envVars args.data.EphemeralLabel = &ephemeral_label args.data.EphemeralTtl = &ephemeral_ttl + args.data.Deployment = &deployment var ret builderBuildFromTarResultsData @@ -1793,7 +2115,7 @@ func (v *BuilderClientBuildFromPreparedResults) VersionShortId() string { return *v.data.VersionShortId } -func (v BuilderClient) BuildFromPrepared(ctx context.Context, session_id string, tardata stream.RecvStream[[]byte], status stream.SendStream[*Status], envVars []*EnvironmentVariable, ephemeral_label string, ephemeral_ttl string) (*BuilderClientBuildFromPreparedResults, error) { +func (v BuilderClient) BuildFromPrepared(ctx context.Context, session_id string, tardata stream.RecvStream[[]byte], status stream.SendStream[*Status], envVars []*EnvironmentVariable, ephemeral_label string, ephemeral_ttl string, deployment *DeployRequest) (*BuilderClientBuildFromPreparedResults, error) { args := BuilderBuildFromPreparedArgs{} caps := map[rpc.OID]*rpc.InlineCapability{} args.data.SessionId = &session_id @@ -1810,6 +2132,7 @@ func (v BuilderClient) BuildFromPrepared(ctx context.Context, session_id string, args.data.EnvVars = &envVars args.data.EphemeralLabel = &ephemeral_label args.data.EphemeralTtl = &ephemeral_ttl + args.data.Deployment = &deployment var ret builderBuildFromPreparedResultsData diff --git a/api/build/rpc.yml b/api/build/rpc.yml index 104b9e93..2c05ebbd 100644 --- a/api/build/rpc.yml +++ b/api/build/rpc.yml @@ -5,6 +5,9 @@ imports: stream: path: ../../pkg/rpc/stream/stream.yml import: miren.dev/runtime/pkg/rpc/stream + standard: + path: ../../pkg/rpc/standard/standard.yml + import: miren.dev/runtime/pkg/rpc/standard types: - type: Status @@ -27,6 +30,86 @@ types: - name: log type: LogEntry index: 4 + - name: deployment + type: DeploymentProgress + index: 5 + + - type: DeploymentProgress + doc: > + Progress of the server-managed deployment record backing this build. Sent + once the record exists, and again on each phase change, so the client can + display and cancel a deployment it did not create. + fields: + - name: deployment_id + type: string + index: 0 + doc: The deployment record the server created for this build + - name: phase + type: string + index: 1 + doc: Current phase (preparing, building, pushing, activating) + + - type: DeployRequest + doc: > + Asks the server to own the deployment record for this build: create it, + advance it as the build progresses, and settle it at the end. + + Its PRESENCE is the ownership signal. When absent — which is what an older + client sends, since it does not know this parameter — the server does not + touch deployment records at all and the client is left to drive them + through the deployment service as before. That is what stops a build from + producing two records for one deploy. + fields: + - name: cluster_id + type: string + index: 0 + doc: > + The cluster being deployed to. Client-side configuration the server + cannot infer, so it has to be passed. + - name: git_info + type: GitInfo + index: 1 + doc: Git state of the working tree being deployed (optional) + + - type: GitInfo + doc: Git repository information recorded on the deployment + fields: + - name: sha + type: string + index: 0 + doc: Git commit SHA + - name: branch + type: string + index: 1 + doc: Git branch name + - name: repository + type: string + index: 2 + doc: Git repository URL + - name: is_dirty + type: bool + index: 3 + doc: Whether working tree had uncommitted changes + - name: working_tree_hash + type: string + index: 4 + doc: Hash of working tree if dirty + - name: commit_message + type: string + index: 5 + doc: Git commit message + - name: commit_author_name + type: string + index: 6 + doc: Name of commit author + - name: commit_author_email + type: string + index: 7 + doc: Email of commit author + - name: commit_timestamp + type: standard.Timestamp + index: 8 + doc: When the commit was made - type: LogEntry doc: A structured log message with severity level and extensible fields @@ -221,6 +304,11 @@ interfaces: - name: ephemeral_ttl type: string doc: TTL for ephemeral version (e.g., "48h"), defaults to "24h" when ephemeral_label is set + - name: deployment + type: '*DeployRequest' + doc: > + When set, the server owns the deployment record for this build. + When absent, it creates none — see DeployRequest. results: - name: version type: string @@ -268,6 +356,11 @@ interfaces: - name: ephemeral_ttl type: string doc: TTL for ephemeral version (e.g., "48h"), defaults to "24h" when ephemeral_label is set + - name: deployment + type: '*DeployRequest' + doc: > + When set, the server owns the deployment record for this build. + When absent, it creates none — see DeployRequest. results: - name: version type: string diff --git a/api/deployment/deployment_v1alpha/rpc.gen.go b/api/deployment/deployment_v1alpha/rpc.gen.go index ee6e5639..280f81b1 100644 --- a/api/deployment/deployment_v1alpha/rpc.gen.go +++ b/api/deployment/deployment_v1alpha/rpc.gen.go @@ -1799,6 +1799,88 @@ func (v *DeploymentSetEnvVarsResults) UnmarshalJSON(data []byte) error { return json.Unmarshal(data, &v.data) } +type deploymentGetDeployLockArgsData struct { + AppName *string `cbor:"0,keyasint,omitempty" json:"app_name,omitempty"` + ClusterId *string `cbor:"1,keyasint,omitempty" json:"cluster_id,omitempty"` +} + +type DeploymentGetDeployLockArgs struct { + call rpc.Call + data deploymentGetDeployLockArgsData +} + +func (v *DeploymentGetDeployLockArgs) HasAppName() bool { + return v.data.AppName != nil +} + +func (v *DeploymentGetDeployLockArgs) AppName() string { + if v.data.AppName == nil { + return "" + } + return *v.data.AppName +} + +func (v *DeploymentGetDeployLockArgs) HasClusterId() bool { + return v.data.ClusterId != nil +} + +func (v *DeploymentGetDeployLockArgs) ClusterId() string { + if v.data.ClusterId == nil { + return "" + } + return *v.data.ClusterId +} + +func (v *DeploymentGetDeployLockArgs) MarshalCBOR() ([]byte, error) { + return cbor.Marshal(v.data) +} + +func (v *DeploymentGetDeployLockArgs) UnmarshalCBOR(data []byte) error { + return cbor.Unmarshal(data, &v.data) +} + +func (v *DeploymentGetDeployLockArgs) MarshalJSON() ([]byte, error) { + return json.Marshal(v.data) +} + +func (v *DeploymentGetDeployLockArgs) UnmarshalJSON(data []byte) error { + return json.Unmarshal(data, &v.data) +} + +type deploymentGetDeployLockResultsData struct { + Held *bool `cbor:"0,keyasint,omitempty" json:"held,omitempty"` + LockInfo *DeploymentLockInfo `cbor:"1,keyasint,omitempty" json:"lock_info,omitempty"` +} + +type DeploymentGetDeployLockResults struct { + call rpc.Call + data deploymentGetDeployLockResultsData +} + +func (v *DeploymentGetDeployLockResults) SetHeld(held bool) { + v.data.Held = &held +} + +func (v *DeploymentGetDeployLockResults) SetLockInfo(lock_info *DeploymentLockInfo) { + v.data.LockInfo = lock_info +} + +func (v *DeploymentGetDeployLockResults) MarshalCBOR() ([]byte, error) { + return cbor.Marshal(v.data) +} + +func (v *DeploymentGetDeployLockResults) UnmarshalCBOR(data []byte) error { + return cbor.Unmarshal(data, &v.data) +} + +func (v *DeploymentGetDeployLockResults) MarshalJSON() ([]byte, error) { + return json.Marshal(v.data) +} + +func (v *DeploymentGetDeployLockResults) UnmarshalJSON(data []byte) error { + return json.Unmarshal(data, &v.data) +} + type deploymentDeleteEnvVarsArgsData struct { AppName *string `cbor:"0,keyasint,omitempty" json:"app_name,omitempty"` ClusterId *string `cbor:"1,keyasint,omitempty" json:"cluster_id,omitempty"` @@ -2211,6 +2293,32 @@ func (t *DeploymentSetEnvVars) Results() *DeploymentSetEnvVarsResults { return results } +type DeploymentGetDeployLock struct { + rpc.Call + args DeploymentGetDeployLockArgs + results DeploymentGetDeployLockResults +} + +func (t *DeploymentGetDeployLock) Args() *DeploymentGetDeployLockArgs { + args := &t.args + if args.call != nil { + return args + } + args.call = t.Call + t.Call.Args(args) + return args +} + +func (t *DeploymentGetDeployLock) Results() *DeploymentGetDeployLockResults { + results := &t.results + if results.call != nil { + return results + } + results.call = t.Call + t.Call.Results(results) + return results +} + type DeploymentDeleteEnvVars struct { rpc.Call args DeploymentDeleteEnvVarsArgs @@ -2249,6 +2357,7 @@ type Deployment interface { CancelDeployment(ctx context.Context, state *DeploymentCancelDeployment) error DeployVersion(ctx context.Context, state *DeploymentDeployVersion) error SetEnvVars(ctx context.Context, state *DeploymentSetEnvVars) error + GetDeployLock(ctx context.Context, state *DeploymentGetDeployLock) error DeleteEnvVars(ctx context.Context, state *DeploymentDeleteEnvVars) error } @@ -2300,6 +2409,10 @@ func (reexportDeployment) SetEnvVars(ctx context.Context, state *DeploymentSetEn panic("not implemented") } +func (reexportDeployment) GetDeployLock(ctx context.Context, state *DeploymentGetDeployLock) error { + panic("not implemented") +} + func (reexportDeployment) DeleteEnvVars(ctx context.Context, state *DeploymentDeleteEnvVars) error { panic("not implemented") } @@ -2420,6 +2533,16 @@ func AdaptDeployment(t Deployment) *rpc.Interface { return t.SetEnvVars(ctx, &DeploymentSetEnvVars{Call: call}) }, }, + { + Name: "GetDeployLock", + InterfaceName: "Deployment", + Index: 12, + Public: false, + Params: []string{"app_name", "cluster_id"}, + Handler: func(ctx context.Context, call rpc.Call) error { + return t.GetDeployLock(ctx, &DeploymentGetDeployLock{Call: call}) + }, + }, { Name: "DeleteEnvVars", InterfaceName: "Deployment", @@ -2874,6 +2997,45 @@ func (v DeploymentClient) SetEnvVars(ctx context.Context, app_name string, clust return &DeploymentClientSetEnvVarsResults{client: v.Client, data: ret}, nil } +type DeploymentClientGetDeployLockResults struct { + client rpc.Client + data deploymentGetDeployLockResultsData +} + +func (v *DeploymentClientGetDeployLockResults) HasHeld() bool { + return v.data.Held != nil +} + +func (v *DeploymentClientGetDeployLockResults) Held() bool { + if v.data.Held == nil { + return false + } + return *v.data.Held +} + +func (v *DeploymentClientGetDeployLockResults) HasLockInfo() bool { + return v.data.LockInfo != nil +} + +func (v *DeploymentClientGetDeployLockResults) LockInfo() *DeploymentLockInfo { + return v.data.LockInfo +} + +func (v DeploymentClient) GetDeployLock(ctx context.Context, app_name string, cluster_id string) (*DeploymentClientGetDeployLockResults, error) { + args := DeploymentGetDeployLockArgs{} + args.data.AppName = &app_name + args.data.ClusterId = &cluster_id + + var ret deploymentGetDeployLockResultsData + + err := v.Call(ctx, "GetDeployLock", &args, &ret) + if err != nil { + return nil, err + } + + return &DeploymentClientGetDeployLockResults{client: v.Client, data: ret}, nil +} + type DeploymentClientDeleteEnvVarsResults struct { client rpc.Client data deploymentDeleteEnvVarsResultsData diff --git a/api/deployment/rpc.yml b/api/deployment/rpc.yml index 143b164a..eccaafd7 100644 --- a/api/deployment/rpc.yml +++ b/api/deployment/rpc.yml @@ -13,7 +13,10 @@ interfaces: methods: - name: CreateDeployment index: 0 - doc: Create a new deployment record + doc: > + DEPRECATED: The build server now owns the deployment record lifecycle + (see the build service DeployRequest). Retained so an older CLI still + works against a newer server. Do not add new callers. parameters: - name: app_name type: string @@ -40,7 +43,9 @@ interfaces: - name: UpdateDeploymentStatus index: 1 - doc: Update the status of a deployment + doc: > + DEPRECATED: superseded by server-owned deployment lifecycle. Retained + for older CLIs. Do not add new callers. parameters: - name: deployment_id type: string @@ -58,7 +63,9 @@ interfaces: - name: UpdateDeploymentPhase index: 2 - doc: Update the current phase of a deployment + doc: > + DEPRECATED: superseded by server-owned deployment lifecycle. Retained + for older CLIs. Do not add new callers. parameters: - name: deployment_id type: string @@ -73,7 +80,9 @@ interfaces: - name: UpdateFailedDeployment index: 3 - doc: Update a failed deployment with error details and logs + doc: > + DEPRECATED: superseded by server-owned deployment lifecycle. Retained + for older CLIs. Do not add new callers. parameters: - name: deployment_id type: string @@ -91,7 +100,9 @@ interfaces: - name: UpdateDeploymentAppVersion index: 4 - doc: Update deployment with the actual app version ID after build + doc: > + DEPRECATED: superseded by server-owned deployment lifecycle. Retained + for older CLIs. Do not add new callers. parameters: - name: deployment_id type: string @@ -243,6 +254,29 @@ interfaces: type: '*AccessInfo' doc: Information about how to access the deployed app + - name: GetDeployLock + index: 12 + doc: > + Report whether a deployment lock is currently held for an app+cluster, + and by whom. Read-only. Used by clients as an advisory pre-flight before + uploading a build context, so a blocked deploy can be reported without + wasting the upload. A held=false result means the next deploy would not + be blocked at call time; it is advisory, not a reservation. + parameters: + - name: app_name + type: string + doc: The application name + - name: cluster_id + type: string + doc: The cluster to check + results: + - name: held + type: bool + doc: Whether a deployment lock is currently held + - name: lock_info + type: DeploymentLockInfo + doc: Details of the holding deployment (only set when held) + - name: DeleteEnvVars index: 11 doc: Delete environment variables from an app, creating a new version and deployment record. Returns deployment info for activation polling. diff --git a/blackbox/deployment_lifecycle_test.go b/blackbox/deployment_lifecycle_test.go new file mode 100644 index 00000000..094e08ea --- /dev/null +++ b/blackbox/deployment_lifecycle_test.go @@ -0,0 +1,144 @@ +//go:build blackbox + +package blackbox + +import ( + "encoding/json" + "strings" + "testing" + "time" + + "miren.dev/runtime/blackbox/harness" +) + +// deploymentHistory mirrors the JSON shape of `miren app history --format json`. +type deploymentHistory struct { + App string `json:"app"` + Cluster string `json:"cluster"` + Deployments []deploymentHistoryItem `json:"deployments"` +} + +type deploymentHistoryItem struct { + ID string `json:"id"` + Status string `json:"status"` + AppVersionID string `json:"app_version_id,omitempty"` + Phase string `json:"phase,omitempty"` + ErrorMessage string `json:"error_message,omitempty"` +} + +func appHistory(t *testing.T, m *harness.Miren, name string) []deploymentHistoryItem { + t.Helper() + r := m.MustRun("app", "history", "-a", name, "--format", "json") + var out deploymentHistory + if err := json.Unmarshal([]byte(r.Stdout), &out); err != nil { + t.Fatalf("failed to parse app history: %v\noutput: %s", err, r.Stdout) + } + return out.Deployments +} + +// A successful server-owned deploy must leave exactly one active record whose +// app_version_id is a real version — never the "pending-build" placeholder the +// old client used to write. +func TestDeployHistoryRecordsActiveVersion(t *testing.T) { + c := harness.NewCluster(t) + m := harness.NewMiren(t, c) + + name := harness.UniqueAppName(t, "go-server") + t.Cleanup(func() { m.Run("app", "delete", name, "-f") }) + + m.MustRun("deploy", "-a", name, "-d", m.ContainerPath(c.TestdataDir+"/go-server"), "-f") + harness.WaitForAppReady(t, m, name, 2*time.Minute) + + deployments := appHistory(t, m, name) + if len(deployments) == 0 { + t.Fatal("expected at least one deployment in history") + } + + // Newest first. + latest := deployments[0] + if latest.Status != "active" { + t.Fatalf("expected latest deployment active, got %q", latest.Status) + } + if latest.AppVersionID == "" { + t.Fatal("active deployment must carry a real app version id") + } + if latest.AppVersionID == "pending-build" || strings.HasPrefix(latest.AppVersionID, "failed-") { + t.Fatalf("history leaked a placeholder version: %q", latest.AppVersionID) + } +} + +// A build that fails during compilation must leave a failed record with the +// error captured, and must not surface a placeholder version in history. +func TestFailedBuildLeavesFailedRecord(t *testing.T) { + c := harness.NewCluster(t) + m := harness.NewMiren(t, c) + + name := harness.UniqueAppName(t, "build-error") + t.Cleanup(func() { m.Run("app", "delete", name, "-f") }) + + // The build-error testdata does not compile, so the deploy fails. + m.Run("deploy", "-a", name, "-d", m.ContainerPath(c.TestdataDir+"/build-error"), "-f"). + RequireExitCode(t, 1) + + // The server owns the record, so the failure must be visible in history + // without the client having written anything. + harness.Poll(t, "failed deployment recorded", 90*time.Second, 3*time.Second, + func() (bool, string) { + for _, d := range appHistory(t, m, name) { + if d.Status == "failed" { + if d.AppVersionID == "pending-build" || strings.HasPrefix(d.AppVersionID, "failed-") { + return false, "record still carries a placeholder version" + } + if d.ErrorMessage == "" { + return false, "failed record has no error message" + } + return true, "" + } + } + return false, "no failed deployment yet" + }, + ) +} + +// A second deploy of the same app+cluster, started while the first is still in +// flight, must be blocked by the deploy lock rather than run alongside it. +// +// The slow-build testdata sleeps during its build, giving a wide, deterministic +// window in which the first deploy holds the lock. (On a warm local cache the +// build layer may be reused and the window shrinks; CI runs on a cold cache.) +func TestConcurrentDeployBlockedByLock(t *testing.T) { + c := harness.NewCluster(t) + m := harness.NewMiren(t, c) + + name := harness.UniqueAppName(t, "slow-build") + t.Cleanup(func() { m.Run("app", "delete", name, "-f") }) + + dir := m.ContainerPath(c.TestdataDir + "/slow-build") + + // First deploy runs in the background so we can race a second against it. + first := m.RunCmdBackground(t, nil, "m", "deploy", "-a", name, "-d", dir, "-f") + t.Cleanup(func() { first.Stop() }) + + // Wait until the first deploy has taken the lock (an in-progress record). + harness.Poll(t, "first deploy in progress", 60*time.Second, 500*time.Millisecond, + func() (bool, string) { + for _, d := range appHistory(t, m, name) { + if d.Status == "in_progress" { + return true, "" + } + } + return false, "no in-progress deployment yet" + }, + ) + + // A second concurrent deploy must be refused while the lock is held. The + // slow build keeps the first deploy in flight for ~25s, so this races + // comfortably inside the window. + second := m.Run("deploy", "-a", name, "-d", dir, "-f") + if second.Success() { + t.Fatal("expected the second concurrent deploy to be blocked, but it succeeded") + } + if !second.OutputContains("blocked") { + t.Fatalf("expected a 'blocked' message, got:\nstdout: %s\nstderr: %s", second.Stdout, second.Stderr) + } +} diff --git a/cli/commands/deploy.go b/cli/commands/deploy.go index 9d78f0c6..a8c06936 100644 --- a/cli/commands/deploy.go +++ b/cli/commands/deploy.go @@ -302,6 +302,16 @@ func Deploy(ctx *Context, opts struct { bc := build_v1alpha.NewBuilderClient(cl) + // Decide who owns the deployment record. A server that advertises the + // "deployment" parameter on buildFromTar owns the record itself: it creates + // it, advances it, and settles it as the build runs. Against an older server + // that lacks the parameter, the CLI falls back to driving the record through + // the deprecated deployment RPCs, exactly as before. The probe runs before + // any upload, so an old server costs nothing. + // + // Ephemeral deploys never own a record on either path. + serverOwnsDeployment := ephemeralLabel == "" && cl.HasMethodParam(ctx.Context, "buildFromTar", "deployment") + // Check if deployment is allowed before proceeding remedy, err := deploygating.CheckDeployAllowed(dir) if err != nil { @@ -349,10 +359,31 @@ func Deploy(ctx *Context, opts struct { } } - // Create deployment record for non-ephemeral deploys only. - // Ephemeral deploys don't participate in deployment tracking or locking. + // deployReq is sent on the build call when the server owns the record; its + // presence is what tells the server to take ownership. + var deployReq *build_v1alpha.DeployRequest + if serverOwnsDeployment { + deployReq = &build_v1alpha.DeployRequest{} + deployReq.SetClusterId(ctx.ClusterName) + if gi := buildGitInfoFromGit(gitInfo); gi != nil { + deployReq.SetGitInfo(gi) + } + + // Advisory pre-flight: surface the rich "deployment blocked" message + // without wasting an upload. The authoritative acquisition still happens + // server-side inside the build; this only spares the common case. + if lockRes, lockErr := depClient.GetDeployLock(ctx, name, ctx.ClusterName); lockErr == nil && + lockRes.Held() && lockRes.HasLockInfo() && lockRes.LockInfo() != nil { + printDeploymentBlocked(ctx, lockRes.LockInfo()) + return fmt.Errorf("deployment blocked by lock") + } + } + + // Create deployment record for non-ephemeral deploys only, and only when the + // server does not own the record. Ephemeral deploys don't participate in + // deployment tracking or locking. var deploymentId string - if ephemeralLabel == "" { + if ephemeralLabel == "" && !serverOwnsDeployment { createDepCtx, createDepSpan := deployTracer.Start(ctx.Context, "deploy.create_deployment") createResult, err := depClient.CreateDeployment(createDepCtx, name, ctx.ClusterName, "pending-build", deploymentGitInfo) if err != nil { @@ -364,40 +395,8 @@ func Deploy(ctx *Context, opts struct { createDepSpan.End() if createResult.HasError() && createResult.Error() != "" { - // Check if we have structured lock info if createResult.HasLockInfo() && createResult.LockInfo() != nil { - lockInfo := createResult.LockInfo() - - // Format the deployment lock message - ctx.Printf("\n❌ Deployment blocked:\n\n") - ctx.Printf("Another deployment is already in progress for app '%s' on cluster '%s'.\n\n", - lockInfo.AppName(), lockInfo.ClusterId()) - - ctx.Printf("Existing deployment details:\n") - ctx.Printf(" • Deployment ID: %s\n", ui.DisplayShortID(lockInfo.BlockingDeploymentShortId(), lockInfo.BlockingDeploymentId())) - ctx.Printf(" • Started by: %s\n", lockInfo.StartedBy()) - - if lockInfo.HasStartedAt() && lockInfo.StartedAt() != nil { - startedAt := time.Unix(lockInfo.StartedAt().Seconds(), 0) - ctx.Printf(" • Started at: %s (%s ago)\n", - startedAt.Format("2006-01-02 15:04:05 MST"), - time.Since(startedAt).Round(time.Second)) - } - - ctx.Printf(" • Current phase: %s\n", lockInfo.CurrentPhase()) - - if lockInfo.HasLockExpiresAt() && lockInfo.LockExpiresAt() != nil { - expiresAt := time.Unix(lockInfo.LockExpiresAt().Seconds(), 0) - timeRemaining := time.Until(expiresAt).Round(time.Second) - ctx.Printf(" • Lock expires in: %s\n\n", timeRemaining) - } - - // Build contact message - if lockInfo.StartedBy() != "-" { - ctx.Printf("Please wait for it to complete or contact %s to coordinate.\n", lockInfo.StartedBy()) - } else { - ctx.Printf("Please wait for it to complete.\n") - } + printDeploymentBlocked(ctx, createResult.LockInfo()) } else { // Fall back to plain error message ctx.Printf("\n❌ Deployment blocked:\n\n%s\n", createResult.Error()) @@ -417,18 +416,70 @@ func Deploy(ctx *Context, opts struct { buildCtx, cancelBuild := context.WithCancel(ctx.Context) defer cancelBuild() - // Start goroutine to poll for external cancellation using the cancellationPoller - statusGetter := newDepClientStatusGetter(depClient, ctx.Log) - poller := newCancellationPoller(deploymentId, statusGetter, 2*time.Second) - go func() { - poller.Start(buildCtx, func() { - ctx.Log.Info("Deployment cancelled externally, stopping build") - cancelBuild() + // deploymentId is set up front on the legacy path (from CreateDeployment) and + // arrives mid-stream on the server-owned path (from the build's deployment + // status arm). A mutex guards it because that arm is handled in a stream + // goroutine while the main goroutine reads it for the summary. + var deploymentMu sync.Mutex + setDeploymentID := func(id string) { + deploymentMu.Lock() + deploymentId = id + deploymentMu.Unlock() + } + getDeploymentID := func() string { + deploymentMu.Lock() + defer deploymentMu.Unlock() + return deploymentId + } + + // The cancellation poller starts once we know the deployment id, which is + // immediately on the legacy path and on the first deployment status arm on + // the server-owned path. + var ( + pollerOnce sync.Once + externallyCancelled atomic.Bool + ) + startPoller := func(id string) { + if id == "" { + return + } + pollerOnce.Do(func() { + statusGetter := newDepClientStatusGetter(depClient, ctx.Log) + poller := newCancellationPoller(id, statusGetter, 2*time.Second) + go poller.Start(buildCtx, func() { + ctx.Log.Info("Deployment cancelled externally, stopping build") + externallyCancelled.Store(true) + cancelBuild() + }) }) - }() + } + if !serverOwnsDeployment { + startPoller(deploymentId) + } // Helper to check if we were externally cancelled - wasExternallyCancelled := poller.WasExternallyCancelled + wasExternallyCancelled := externallyCancelled.Load + + // onDeployment records the server-created deployment on the server-owned + // path: it captures the id and starts the cancellation poller the first time + // the build reports it. + // + // Degradation note: on the server-owned path this is the ONLY thing that + // starts the poller and sets deploymentId. The server emits the deployment + // arm at the very start of the build (right after it takes the lock), so in + // practice it always arrives. But if it never does — a dropped arm, or a + // server that advertises the param yet never emits it — external + // `miren deploy cancel` cannot interrupt this CLI build and --summary-json's + // deploy_id is empty. The deploy itself is unaffected: the server still owns + // and settles the record. This is an accepted degradation, not a failure. + onDeployment := func(id, phase string) { + if id == "" { + return + } + setDeploymentID(id) + startPoller(id) + ctx.Log.Info("Server-owned deployment", "deployment_id", id, "phase", phase) + } // Parse environment variables to pass to build server var envVars []*build_v1alpha.EnvironmentVariable @@ -459,10 +510,14 @@ func Deploy(ctx *Context, opts struct { var buildLogs []string var deployWarnings []*build_v1alpha.LogEntry - // Helper function to update deployment phase + // Helper function to update deployment phase. No-op when the server owns the + // record — it advances the phase itself as the build progresses. updateDeploymentPhase := func(phase string) { - if deploymentId != "" { - _, updateErr := depClient.UpdateDeploymentPhase(ctx, deploymentId, phase) + if serverOwnsDeployment { + return + } + if id := getDeploymentID(); id != "" { + _, updateErr := depClient.UpdateDeploymentPhase(ctx, id, phase) if updateErr != nil { ctx.Log.Error("Failed to update deployment phase", "error", updateErr, "phase", phase) } @@ -478,9 +533,13 @@ func Deploy(ctx *Context, opts struct { return slices.Clone(buildErrors), slices.Clone(buildLogs), slices.Clone(deployWarnings) } - // Helper function to update deployment status on failure + // Helper function to update deployment status on failure. No-op when the + // server owns the record — it settles the deployment as failed itself. updateDeploymentOnError := func(errMsg string) { - if deploymentId != "" { + if serverOwnsDeployment { + return + } + if id := getDeploymentID(); id != "" { // Use a separate context with timeout for status updates to ensure they complete // even if the main context is canceled statusCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -492,10 +551,10 @@ func Deploy(ctx *Context, opts struct { logs = strings.Join(errsSnap, "\n") } - _, updateErr := depClient.UpdateFailedDeployment(statusCtx, deploymentId, errMsg, logs) + _, updateErr := depClient.UpdateFailedDeployment(statusCtx, id, errMsg, logs) if updateErr != nil { // Fallback to simple status update - _, updateErr = depClient.UpdateDeploymentStatus(statusCtx, deploymentId, "failed", errMsg) + _, updateErr = depClient.UpdateDeploymentStatus(statusCtx, id, "failed", errMsg) if updateErr != nil { ctx.Log.Error("Failed to update deployment status to failed", "error", updateErr) } @@ -634,11 +693,13 @@ func Deploy(ctx *Context, opts struct { AccessInfo() *build_v1alpha.AccessInfo } buildCall := func(callCtx context.Context, tarReader io.ReadCloser, cb stream.SendStream[*build_v1alpha.Status]) (buildResults, error) { + // deployReq is non-nil only when the server owns the deployment record; + // its presence is the signal that hands ownership to the server. if useOptimized { tarStream := stream.ServeReader(callCtx, tarReader, stream.WithBulkBatching()) - return bc.BuildFromPrepared(callCtx, sessionID, tarStream, cb, envVars, ephemeralLabel, ephemeralTTL) + return bc.BuildFromPrepared(callCtx, sessionID, tarStream, cb, envVars, ephemeralLabel, ephemeralTTL, deployReq) } - return bc.BuildFromTar(callCtx, name, stream.ServeReader(callCtx, tarReader, stream.WithBulkBatching()), cb, envVars, ephemeralLabel, ephemeralTTL) + return bc.BuildFromTar(callCtx, name, stream.ServeReader(callCtx, tarReader, stream.WithBulkBatching()), cb, envVars, ephemeralLabel, ephemeralTTL, deployReq) } var ( @@ -733,7 +794,7 @@ func Deploy(ctx *Context, opts struct { return safeStatus.Send(buildCtx, status) } - cb = createBuildStatusCallback(buildCtx, nil, nil, &buildStateMu, &buildErrors, nil, &deployWarnings, progressHandler) + cb = createBuildStatusCallback(buildCtx, nil, nil, &buildStateMu, &buildErrors, nil, &deployWarnings, progressHandler, onDeployment) results, err = buildCall(buildCtx, r, cb) if err != nil { @@ -761,6 +822,13 @@ func Deploy(ctx *Context, opts struct { return err } + // A server-owned build can fail because another deploy took the lock + // after our advisory pre-flight passed; render the rich blocked + // message rather than a generic build error. + if maybeReportBlocked(ctx, depClient, serverOwnsDeployment, name) { + return err + } + ctx.Printf("\n\nBuild failed with the following errors:\n") errsSnap, _, _ := snapshotBuildState() printBuildErrors(ctx, errsSnap, nil) @@ -821,7 +889,7 @@ func Deploy(ctx *Context, opts struct { return nil } - cb = createBuildStatusCallback(deployCtx, updateCh, buildCh, &buildStateMu, &buildErrors, &buildLogs, &deployWarnings, progressHandler) + cb = createBuildStatusCallback(deployCtx, updateCh, buildCh, &buildStateMu, &buildErrors, &buildLogs, &deployWarnings, progressHandler, onDeployment) results, err = buildCall(deployCtx, r, cb) @@ -860,6 +928,12 @@ func Deploy(ctx *Context, opts struct { return err } + // See the explain-mode branch: a server-owned build can lose the lock + // race after the pre-flight; show the blocked message if so. + if maybeReportBlocked(ctx, depClient, serverOwnsDeployment, name) { + return err + } + ctx.Printf("\n\nBuild failed.\n") errsSnap, logsSnap, _ := snapshotBuildState() printBuildErrors(ctx, errsSnap, logsSnap) @@ -922,13 +996,19 @@ func Deploy(ctx *Context, opts struct { ctx.Printf(" URL: https://%s.%s\n", ephemeralLabel, info.ClusterHostname()) } } + } else if serverOwnsDeployment { + // The server already advanced and settled the deployment record as the + // build ran; the CLI is just a viewer. Nothing to finalize. + deploymentFinalized = true } else { // Normal deploy: update deployment tracking // Update phase to pushing (build completed, now pushing) updateDeploymentPhase("pushing") + legacyDeploymentID := getDeploymentID() + // Update deployment with actual app version ID - _, err = depClient.UpdateDeploymentAppVersion(ctx, deploymentId, appVersionId) + _, err = depClient.UpdateDeploymentAppVersion(ctx, legacyDeploymentID, appVersionId) if err != nil { ctx.Log.Error("Failed to update deployment app version", "error", err) // Continue anyway - the deployment is proceeding @@ -941,7 +1021,7 @@ func Deploy(ctx *Context, opts struct { finalizeCtx, finalizeSpan := deployTracer.Start(ctx.Context, "deploy.finalize") // Mark deployment as active - _, err = depClient.UpdateDeploymentStatus(finalizeCtx, deploymentId, "active", "") + _, err = depClient.UpdateDeploymentStatus(finalizeCtx, legacyDeploymentID, "active", "") if err != nil { // Log error but don't fail - deployment is already done ctx.Log.Error("Failed to update deployment status", "error", err) @@ -1007,7 +1087,7 @@ func Deploy(ctx *Context, opts struct { if results.HasAccessInfo() && results.AccessInfo() != nil { summaryURLs = deployURLs(ctx, results.AccessInfo(), ephemeralLabel) } - writeDeploySummary(ctx, opts.SummaryJSON, deploymentId, appVersionId, summaryURLs) + writeDeploySummary(ctx, opts.SummaryJSON, getDeploymentID(), appVersionId, summaryURLs) return nil } @@ -1107,6 +1187,87 @@ func (s *safeStatusCh) Close() { close(s.ch) } +// maybeReportBlocked renders the rich "deployment blocked" message when a +// server-owned build failed because another deployment holds the lock — the +// pre-flight-passed-then-lost-the-race case, where the build returns a lock +// conflict instead of a build error. It reuses GetDeployLock rather than trying +// to parse lock info out of the build error. Returns true if it handled the +// failure as a block, so the caller can skip the generic "build failed" output. +// +// This is a best-effort heuristic: a server-owned build that fails for any other +// reason releases its own lock, so a Held result here means a *different* deploy +// is in the way — which is exactly when the blocked message is the right thing +// to show. +func maybeReportBlocked(ctx *Context, depClient *deployment_v1alpha.DeploymentClient, serverOwnsDeployment bool, name string) bool { + if !serverOwnsDeployment { + return false + } + lockRes, err := depClient.GetDeployLock(ctx, name, ctx.ClusterName) + if err != nil || !lockRes.Held() || !lockRes.HasLockInfo() || lockRes.LockInfo() == nil { + return false + } + printDeploymentBlocked(ctx, lockRes.LockInfo()) + return true +} + +// printDeploymentBlocked renders the rich "deployment blocked" message shared by +// the server-owned pre-flight and the legacy CreateDeployment path. +func printDeploymentBlocked(ctx *Context, lockInfo *deployment_v1alpha.DeploymentLockInfo) { + ctx.Printf("\n❌ Deployment blocked:\n\n") + ctx.Printf("Another deployment is already in progress for app '%s' on cluster '%s'.\n\n", + lockInfo.AppName(), lockInfo.ClusterId()) + + ctx.Printf("Existing deployment details:\n") + ctx.Printf(" • Deployment ID: %s\n", ui.DisplayShortID(lockInfo.BlockingDeploymentShortId(), lockInfo.BlockingDeploymentId())) + ctx.Printf(" • Started by: %s\n", lockInfo.StartedBy()) + + if lockInfo.HasStartedAt() && lockInfo.StartedAt() != nil { + startedAt := time.Unix(lockInfo.StartedAt().Seconds(), 0) + ctx.Printf(" • Started at: %s (%s ago)\n", + startedAt.Format("2006-01-02 15:04:05 MST"), + time.Since(startedAt).Round(time.Second)) + } + + ctx.Printf(" • Current phase: %s\n", lockInfo.CurrentPhase()) + + if lockInfo.HasLockExpiresAt() && lockInfo.LockExpiresAt() != nil { + expiresAt := time.Unix(lockInfo.LockExpiresAt().Seconds(), 0) + timeRemaining := time.Until(expiresAt).Round(time.Second) + ctx.Printf(" • Lock expires in: %s\n\n", timeRemaining) + } + + if lockInfo.StartedBy() != "-" { + ctx.Printf("Please wait for it to complete or contact %s to coordinate.\n", lockInfo.StartedBy()) + } else { + ctx.Printf("Please wait for it to complete.\n") + } +} + +// buildGitInfoFromGit converts the local git.Info into the build-service GitInfo +// carried on a DeployRequest. Returns nil when no git info is available. +func buildGitInfoFromGit(gitInfo *git.Info) *build_v1alpha.GitInfo { + if gitInfo == nil { + return nil + } + + gi := &build_v1alpha.GitInfo{} + gi.SetSha(gitInfo.SHA) + gi.SetBranch(gitInfo.Branch) + gi.SetIsDirty(gitInfo.IsDirty) + gi.SetWorkingTreeHash(gitInfo.WorkingTreeHash) + gi.SetCommitMessage(gitInfo.CommitMessage) + gi.SetCommitAuthorName(gitInfo.CommitAuthor) + gi.SetCommitAuthorEmail(gitInfo.CommitEmail) + gi.SetRepository(gitInfo.RemoteURL) + + if gitInfo.CommitTimestamp != "" { + if ts, err := time.Parse(time.RFC3339, gitInfo.CommitTimestamp); err == nil { + gi.SetCommitTimestamp(standard.ToTimestamp(ts)) + } + } + return gi +} + // createBuildStatusCallback creates a callback for handling build status updates. // stateMu must be non-nil and guards buildErrors, buildLogs, and deployWarnings // — the callback runs from RPC stream-handler goroutines that race with @@ -1120,12 +1281,24 @@ func createBuildStatusCallback( buildLogs *[]string, deployWarnings *[]*build_v1alpha.LogEntry, progressHandler func(*client.SolveStatus) error, + onDeployment func(deploymentID, phase string), ) stream.SendStream[*build_v1alpha.Status] { vertices := map[string]bool{} // digest → completed return stream.Callback(func(su *build_v1alpha.Status) error { update := su.Update() switch update.Which() { + case "deployment": + // The server-owned deployment record announced itself. This arm only + // ever arrives when the server was handed a DeployRequest (an older + // server never sends it, and a newer one only emits it for a build it + // owns), so on the legacy path it is simply never received. onDeployment + // is non-nil on every path — the guard is defensive only. + if onDeployment != nil { + if dp := update.Deployment(); dp != nil { + onDeployment(dp.DeploymentId(), dp.Phase()) + } + } case "buildkit": sj := update.Buildkit() diff --git a/servers/build/build.go b/servers/build/build.go index f3bb3c02..9006a565 100644 --- a/servers/build/build.go +++ b/servers/build/build.go @@ -35,6 +35,7 @@ import ( "miren.dev/runtime/components/netresolve" "miren.dev/runtime/observability" "miren.dev/runtime/pkg/cond" + "miren.dev/runtime/pkg/deploylifecycle" "miren.dev/runtime/pkg/entity" ephemeralx "miren.dev/runtime/pkg/ephemeral" "miren.dev/runtime/pkg/idgen" @@ -107,6 +108,12 @@ type Builder struct { // When set, uses the shared daemon instead of launching ephemeral sandboxes. BuildKit BuildKitProvider + // deploy owns the server-side deployment record lifecycle for builds whose + // client asked the server to manage it. It is an in-process wrapper over the + // same entity access client the builder already holds, so tracking a + // deployment costs no extra RPC hop. + deploy *deploylifecycle.Tracker + sessions sync.Map // sessionID → *buildSession cacheLocks *appLocks } @@ -125,6 +132,7 @@ func NewBuilder(log *slog.Logger, eas *entityserver_v1alpha.EntityAccessClient, DNSHostname: dnsHostname, DataPath: dataPath, cacheLocks: newAppLocks(), + deploy: deploylifecycle.NewTracker(log, eas), } // Only assign when non-nil: storing a typed-nil *buildkit.Component into the // BuildKitProvider interface would make the field non-nil and defeat the @@ -921,7 +929,7 @@ func computeBuildEnvVars( return result } -func (b *Builder) BuildFromTar(ctx context.Context, state *build_v1alpha.BuilderBuildFromTar) error { +func (b *Builder) BuildFromTar(ctx context.Context, state *build_v1alpha.BuilderBuildFromTar) (retErr error) { args := state.Args() name := args.Application() @@ -930,6 +938,24 @@ func (b *Builder) BuildFromTar(ctx context.Context, state *build_v1alpha.Builder return rpc.AppAccessError(ctx, name) } + status := args.Status() + + eph := ephemeralOptsFromArgs(args.HasEphemeralLabel(), args.EphemeralLabel(), + args.HasEphemeralTtl(), args.EphemeralTtl()) + + // Take the deployment record and its lock before receiving the tar, so a + // build that is blocked by another in-flight deploy fails fast instead of + // after a full upload. + var deployReq *build_v1alpha.DeployRequest + if args.HasDeployment() { + deployReq = args.Deployment() + } + deploy, err := b.beginDeploy(ctx, name, deployReq, eph, status) + if err != nil { + return err + } + defer func() { deploy.failOnError(ctx, retErr) }() + td := args.Tardata() path, err := os.MkdirTemp(b.TempDir, "buildkit-") @@ -939,8 +965,6 @@ func (b *Builder) BuildFromTar(ctx context.Context, state *build_v1alpha.Builder defer os.RemoveAll(path) - status := args.Status() - so := new(build_v1alpha.Status) if status != nil { @@ -978,16 +1002,7 @@ func (b *Builder) BuildFromTar(ctx context.Context, state *build_v1alpha.Builder _, _ = status.Send(ctx, so) } - var eph *ephemeralOpts - if args.HasEphemeralLabel() && args.EphemeralLabel() != "" { - ttl := "24h" - if args.HasEphemeralTtl() && args.EphemeralTtl() != "" { - ttl = args.EphemeralTtl() - } - eph = &ephemeralOpts{label: args.EphemeralLabel(), ttl: ttl} - } - - result, err := b.buildFromDir(ctx, name, path, status, args.EnvVars(), eph) + result, err := b.buildFromDir(ctx, name, path, status, args.EnvVars(), eph, deploy) if err != nil { return err } @@ -1072,7 +1087,7 @@ func (b *Builder) PrepareUpload(ctx context.Context, state *build_v1alpha.Builde return nil } -func (b *Builder) BuildFromPrepared(ctx context.Context, state *build_v1alpha.BuilderBuildFromPrepared) error { +func (b *Builder) BuildFromPrepared(ctx context.Context, state *build_v1alpha.BuilderBuildFromPrepared) (retErr error) { args := state.Args() sessionID := args.SessionId() @@ -1093,6 +1108,21 @@ func (b *Builder) BuildFromPrepared(ctx context.Context, state *build_v1alpha.Bu status := args.Status() + eph := ephemeralOptsFromArgs(args.HasEphemeralLabel(), args.EphemeralLabel(), + args.HasEphemeralTtl(), args.EphemeralTtl()) + + // Take the deployment record and its lock before extracting the delta, so a + // blocked build fails fast (see BuildFromTar). + var deployReq *build_v1alpha.DeployRequest + if args.HasDeployment() { + deployReq = args.Deployment() + } + deploy, err := b.beginDeploy(ctx, name, deployReq, eph, status) + if err != nil { + return err + } + defer func() { deploy.failOnError(ctx, retErr) }() + // Extract partial tar into the session directory if provided td := args.Tardata() if td != nil { @@ -1124,16 +1154,7 @@ func (b *Builder) BuildFromPrepared(ctx context.Context, state *build_v1alpha.Bu _, _ = status.Send(ctx, so) } - var eph *ephemeralOpts - if args.HasEphemeralLabel() && args.EphemeralLabel() != "" { - ttl := "24h" - if args.HasEphemeralTtl() && args.EphemeralTtl() != "" { - ttl = args.EphemeralTtl() - } - eph = &ephemeralOpts{label: args.EphemeralLabel(), ttl: ttl} - } - - result, err := b.buildFromDir(ctx, name, sess.dir, status, args.EnvVars(), eph) + result, err := b.buildFromDir(ctx, name, sess.dir, status, args.EnvVars(), eph, deploy) if err != nil { return err } @@ -1158,10 +1179,22 @@ type ephemeralOpts struct { ttl string } +// ephemeralOptsFromArgs builds ephemeral options from the label/ttl parameters +// shared by both build entry points, returning nil for a non-ephemeral build. +func ephemeralOptsFromArgs(hasLabel bool, label string, hasTTL bool, ttl string) *ephemeralOpts { + if !hasLabel || label == "" { + return nil + } + if !hasTTL || ttl == "" { + ttl = "24h" + } + return &ephemeralOpts{label: label, ttl: ttl} +} + func (b *Builder) buildFromDir(ctx context.Context, name string, path string, status *stream.SendStreamClient[*build_v1alpha.Status], envVars []*build_v1alpha.EnvironmentVariable, - ephemeral *ephemeralOpts) (*buildResult, error) { + ephemeral *ephemeralOpts, deploy *deployTracking) (*buildResult, error) { // -- build.setup span: app config, stack detection, buildkit connect ctx, setupSpan := buildTracer.Start(ctx, "build.setup") @@ -1212,6 +1245,8 @@ func (b *Builder) buildFromDir(ctx context.Context, name string, path string, version: mrv.Version, } + deploy.setPhase(ctx, deploylifecycle.PhaseBuilding) + // runBuildkitBuild is also the saga action's implementation, so the // span structure (buildkit + locate_artifact bundled together) is // slightly coarser than the pre-saga code's two separate spans. @@ -1237,6 +1272,8 @@ func (b *Builder) buildFromDir(ctx context.Context, name string, path string, } bkSpan.End() + deploy.setPhase(ctx, deploylifecycle.PhasePushing) + mrv.Artifact = entity.Id(artifactID) mrv.ImageUrl = finalURL artifactName := strings.TrimPrefix(artifactID, "artifact/") @@ -1363,6 +1400,10 @@ func (b *Builder) buildFromDir(ctx context.Context, name string, path string, } createVerSpan.End() + // The record now names a real version, replacing the placeholder it + // carried while the build ran. + deploy.setAppVersion(ctx, string(id)) + if isEphemeral { b.Log.Info("created ephemeral app version", "app", name, "version", mrv.Version, "label", ephemeral.label, @@ -1371,6 +1412,8 @@ func (b *Builder) buildFromDir(ctx context.Context, name string, path string, // -- build.activate span (only for non-ephemeral versions) activateCtx, activateSpan := buildTracer.Start(ctx, "build.activate") + deploy.setPhase(ctx, deploylifecycle.PhaseActivating) + // Provision addons before activating the version so that AddonAssociation // entities exist when the launcher runs. The addon controller will create // a new AppVersion with addon vars once provisioning completes. @@ -1390,6 +1433,9 @@ func (b *Builder) buildFromDir(ctx context.Context, name string, path string, } activateSpan.End() + // The new version is running: settle the record and release the lock. + deploy.activate(ctx) + b.Log.Info("app version updated", "app", name, "version", mrv.Version) b.checkLocalStorageMigration(ctx, appRec.ID, configSpec, status) diff --git a/servers/build/build_saga.go b/servers/build/build_saga.go index 0200e195..55a0ad8f 100644 --- a/servers/build/build_saga.go +++ b/servers/build/build_saga.go @@ -15,6 +15,7 @@ import ( "miren.dev/runtime/api/build/build_v1alpha" "miren.dev/runtime/api/core/core_v1alpha" "miren.dev/runtime/appconfig" + "miren.dev/runtime/pkg/deploylifecycle" "miren.dev/runtime/pkg/entity" ephemeralx "miren.dev/runtime/pkg/ephemeral" "miren.dev/runtime/pkg/saga" @@ -37,6 +38,9 @@ const ( actionProvisionAddons = "provision-addons" actionSetActiveVer = "set-active-version" actionFinalize = "finalize" + actionBeginDeploy = "begin-deployment" + actionRecordVersion = "record-app-version" + actionActivateDeploy = "activate-deployment" ) // buildSagaDeps holds the collaborators injected into the saga context. @@ -597,6 +601,130 @@ func undoFinalize(_ context.Context, _ finalizeIn, _ finalizeOut) error { return nil } +// --- server-owned deployment record lifecycle --- +// +// Three actions mirror the plain path's tracker calls, so the record is a +// byproduct of the saga rather than something the client narrates. Ordering +// comes from data dependencies, not registration order: begin-deployment +// outputs deployment_id; record-app-version consumes deployment_id + +// app_version_id (so it lands after both begin and create-version); +// activate-deployment consumes deployment_id and the set_active_skipped edge +// (so it lands after set-active-version). +// +// All three no-op when the build is not server-tracked. A build is tracked only +// when the client sent a DeployRequest, which seeds deploy_cluster_id; without +// it deployment_id stays empty and every downstream action skips. Ephemeral +// builds are never tracked. + +type beginDeploymentIn struct { + AppName string `json:"app_name" saga:"app_name"` + StreamID string `json:"stream_id" saga:"stream_id"` + ClusterID string `json:"deploy_cluster_id,omitempty" saga:"deploy_cluster_id,optional"` + GitInfo string `json:"deploy_git_info_json,omitempty" saga:"deploy_git_info_json,optional"` +} + +type beginDeploymentOut struct { + DeploymentID string `json:"deployment_id" saga:"deployment_id"` +} + +func beginDeployment(ctx context.Context, in beginDeploymentIn) (beginDeploymentOut, error) { + // No cluster id means the client did not ask the server to own a record. + if in.ClusterID == "" { + return beginDeploymentOut{}, nil + } + + deps := saga.Get[*buildSagaDeps](ctx) + b := deps.builder + + var gitInfo core_v1alpha.GitInfo + if in.GitInfo != "" { + if err := json.Unmarshal([]byte(in.GitInfo), &gitInfo); err != nil { + b.Log.Warn("ignoring unparseable git info on deployment", "error", err) + } + } + + rec, err := b.deploy.Begin(ctx, deploylifecycle.BeginParams{ + AppName: in.AppName, + ClusterID: in.ClusterID, + GitInfo: gitInfo, + }) + if err != nil { + return beginDeploymentOut{}, fmt.Errorf("begin deployment for %s: %w", in.AppName, err) + } + + id := string(rec.Deployment.ID) + deps.statuses.SenderFor(in.StreamID).SendDeployment(id, string(deploylifecycle.PhasePreparing)) + return beginDeploymentOut{DeploymentID: id}, nil +} + +func undoBeginDeployment(ctx context.Context, in beginDeploymentIn, out beginDeploymentOut) error { + if out.DeploymentID == "" { + return nil + } + deps := saga.Get[*buildSagaDeps](ctx) + // FailIfUnsettled: if a later action already activated the record, leave it + // active — the version is live, and undoSetActiveVersion handles reverting + // that separately. + if err := deps.builder.deploy.FailIfUnsettled(ctx, out.DeploymentID, "build rolled back", ""); err != nil { + return fmt.Errorf("failing rolled-back deployment %s: %w", out.DeploymentID, err) + } + return nil +} + +type recordAppVersionIn struct { + DeploymentID string `json:"deployment_id,omitempty" saga:"deployment_id,optional"` + AppVersionID string `json:"app_version_id" saga:"app_version_id"` +} + +type recordAppVersionOut struct { + Recorded saga.Edge `saga:"app_version_recorded"` +} + +func recordAppVersion(ctx context.Context, in recordAppVersionIn) (recordAppVersionOut, error) { + if in.DeploymentID == "" { + return recordAppVersionOut{}, nil + } + deps := saga.Get[*buildSagaDeps](ctx) + if err := deps.builder.deploy.SetAppVersion(ctx, in.DeploymentID, in.AppVersionID); err != nil { + return recordAppVersionOut{}, fmt.Errorf("recording app version on deployment %s: %w", in.DeploymentID, err) + } + return recordAppVersionOut{}, nil +} + +func undoRecordAppVersion(_ context.Context, _ recordAppVersionIn, _ recordAppVersionOut) error { + // Nothing to undo: begin's undo settles the record. + return nil +} + +type activateDeploymentIn struct { + DeploymentID string `json:"deployment_id,omitempty" saga:"deployment_id,optional"` + SetActiveSkipped bool `json:"set_active_skipped" saga:"set_active_skipped"` + // Anchors this action after record-app-version. + Recorded saga.Edge `saga:"app_version_recorded"` +} + +type activateDeploymentOut struct{} + +func activateDeployment(ctx context.Context, in activateDeploymentIn) (activateDeploymentOut, error) { + // Not tracked, or an ephemeral build whose activation was skipped: nothing + // to activate. + if in.DeploymentID == "" || in.SetActiveSkipped { + return activateDeploymentOut{}, nil + } + deps := saga.Get[*buildSagaDeps](ctx) + if err := deps.builder.deploy.Activate(ctx, in.DeploymentID); err != nil { + return activateDeploymentOut{}, fmt.Errorf("activating deployment %s: %w", in.DeploymentID, err) + } + return activateDeploymentOut{}, nil +} + +func undoActivateDeployment(_ context.Context, _ activateDeploymentIn, _ activateDeploymentOut) error { + // The record is already active and the lock released. If a later step fails, + // undoSetActiveVersion reverts the running version; the record is left as the + // historical account of a deploy that did go live. No compensation here. + return nil +} + // detectBuildStack assembles the BuildStack the same way buildFromDir // does (app.toml.build > Dockerfile.miren > auto) and performs the // supported-stack check for auto so the saga can fail fast rather than @@ -678,6 +806,9 @@ func registerBuildSaga( Action(actionProvisionAddons, provisionAddons).Undo(undoProvisionAddons). Action(actionSetActiveVer, setActiveVersion).Undo(undoSetActiveVersion). Action(actionFinalize, finalize).Undo(undoFinalize). + Action(actionBeginDeploy, beginDeployment).Undo(undoBeginDeployment). + Action(actionRecordVersion, recordAppVersion).Undo(undoRecordAppVersion). + Action(actionActivateDeploy, activateDeployment).Undo(undoActivateDeployment). RegisterTo(registry) } diff --git a/servers/build/build_saga_deploy_test.go b/servers/build/build_saga_deploy_test.go new file mode 100644 index 00000000..e447972a --- /dev/null +++ b/servers/build/build_saga_deploy_test.go @@ -0,0 +1,179 @@ +package build + +import ( + "context" + "log/slog" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "miren.dev/runtime/api/app" + "miren.dev/runtime/api/entityserver" + "miren.dev/runtime/api/entityserver/entityserver_v1alpha" + "miren.dev/runtime/pkg/deploylifecycle" + "miren.dev/runtime/pkg/entity/testutils" + "miren.dev/runtime/pkg/rpc" + "miren.dev/runtime/pkg/saga" +) + +// newDeploySagaHarness is newSagaTestHarness plus the deployment lifecycle +// actions and a Builder wired with a tracker, so the server-owned deployment +// record path can be exercised end to end against the in-memory store. +func newDeploySagaHarness(t *testing.T) *sagaTestHarness { + t.Helper() + log := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + + inmem, cleanup := testutils.NewInMemEntityServer(t) + t.Cleanup(cleanup) + + tempDir := t.TempDir() + rpcClient := rpc.LocalClient(entityserver_v1alpha.AdaptEntityAccess(inmem.Server)) + builder := &Builder{ + Log: log, + EAS: inmem.EAC, + ec: entityserver.NewClient(log, inmem.EAC), + appClient: app.NewClient(log, rpcClient), + TempDir: tempDir, + cacheLocks: newAppLocks(), + deploy: deploylifecycle.NewTracker(log, inmem.EAC), + } + + streams := NewStreamRegistry(tempDir, log) + statuses := NewStatusRegistry() + + registry := saga.NewRegistry() + deps := &buildSagaDeps{builder: builder, streams: streams, statuses: statuses} + err := saga.Define(sagaBuildFromTar). + Using(deps). + Using(log). + Action(actionReceiveTar, receiveTar).Undo(undoReceiveTar). + Action(actionLoadSource, loadSource).Undo(undoLoadSource). + Action(actionGetNextVer, getNextVersion).Undo(undoGetNextVersion). + Action(actionBuildImage, stubBuildImage).Undo(undoBuildImage). + Action(actionPrepareConfig, prepareConfig).Undo(undoPrepareConfig). + Action(actionHandleEphemera, handleEphemeral).Undo(undoHandleEphemeral). + Action(actionCreateConfigVer, createConfigVersion).Undo(undoCreateConfigVersion). + Action(actionCreateVersion, createVersion).Undo(undoCreateVersion). + Action(actionProvisionAddons, provisionAddons).Undo(undoProvisionAddons). + Action(actionSetActiveVer, setActiveVersion).Undo(undoSetActiveVersion). + Action(actionFinalize, finalize).Undo(undoFinalize). + Action(actionBeginDeploy, beginDeployment).Undo(undoBeginDeployment). + Action(actionRecordVersion, recordAppVersion).Undo(undoRecordAppVersion). + Action(actionActivateDeploy, activateDeployment).Undo(undoActivateDeployment). + RegisterTo(registry) + require.NoError(t, err, "registering deploy-tracking build saga") + + executor := saga.NewExecutor( + saga.NewMemoryStorage(), + saga.WithRegistry(registry), + saga.WithLogger(log), + ) + + return &sagaTestHarness{ + t: t, inmem: inmem, builder: builder, + streams: streams, statuses: statuses, + registry: registry, executor: executor, + } +} + +// A build that carries deploy_cluster_id must leave exactly one deployment +// record, active, with its app version recorded — the whole point of the saga +// owning the lifecycle. +func TestBuildSaga_Tracked_CreatesAndActivatesDeployment(t *testing.T) { + ctx := context.Background() + + h := newDeploySagaHarness(t) + h.streams.Register("stream-tracked", makeTar(t, dockerfileTarball(t))) + + err := h.executor.Start(sagaBuildFromTar). + Input("app_name", "demo"). + Input("stream_id", "stream-tracked"). + Input("deploy_cluster_id", "prod"). + WithID("test-tracked"). + Execute(ctx) + require.NoError(t, err) + + records, err := h.builder.deploy.Store().List(ctx, deploylifecycle.Query{AppName: "demo"}) + require.NoError(t, err) + require.Len(t, records, 1, "exactly one deployment record for a tracked build") + + rec := records[0] + assert.Equal(t, deploylifecycle.StatusActive, rec.Status()) + assert.NotEmpty(t, rec.AppVersion(), "the built version must be recorded") + assert.Equal(t, "prod", rec.Deployment.ClusterId) + + // The lock must be free once the deploy settled. + blocking, err := h.builder.deploy.Locks().Blocking(ctx, "demo") + require.NoError(t, err) + assert.Nil(t, blocking, "an activated deployment must not keep the lock") +} + +// The deployment ID must reach the client over the status stream so it can +// display and cancel a deployment it did not create. +func TestBuildSaga_Tracked_EmitsDeploymentProgress(t *testing.T) { + ctx := context.Background() + + h := newDeploySagaHarness(t) + h.streams.Register("stream-emit", makeTar(t, dockerfileTarball(t))) + + rec := &recordingSender{} + h.statuses.Register("stream-emit", rec) + t.Cleanup(func() { h.statuses.Unregister("stream-emit") }) + + err := h.executor.Start(sagaBuildFromTar). + Input("app_name", "demo"). + Input("stream_id", "stream-emit"). + Input("deploy_cluster_id", "prod"). + WithID("test-emit"). + Execute(ctx) + require.NoError(t, err) + + require.NotEmpty(t, rec.Deployments, "the deployment arm must be emitted") + + records, err := h.builder.deploy.Store().List(ctx, deploylifecycle.Query{AppName: "demo"}) + require.NoError(t, err) + require.Len(t, records, 1) + assert.Equal(t, string(records[0].Deployment.ID), rec.Deployments[0].DeploymentID) +} + +// A build with no deploy_cluster_id (an older client, or ephemeral) must leave +// no server-owned record — the additive-safety guarantee. +func TestBuildSaga_Untracked_CreatesNoDeployment(t *testing.T) { + ctx := context.Background() + + h := newDeploySagaHarness(t) + h.streams.Register("stream-untracked", makeTar(t, dockerfileTarball(t))) + + err := h.executor.Start(sagaBuildFromTar). + Input("app_name", "demo"). + Input("stream_id", "stream-untracked"). + WithID("test-untracked"). + Execute(ctx) + require.NoError(t, err) + + records, err := h.builder.deploy.Store().List(ctx, deploylifecycle.Query{AppName: "demo"}) + require.NoError(t, err) + assert.Empty(t, records, "a build with no DeployRequest must not create a record") +} + +// A second tracked build for the same app+cluster, started while the first +// still holds the lock, must fail rather than run alongside it. +func TestBuildSaga_Tracked_SecondBuildBlockedByLock(t *testing.T) { + ctx := context.Background() + + h := newDeploySagaHarness(t) + + // Take the lock with a standalone deployment that never settles. + _, err := h.builder.deploy.Begin(ctx, deploylifecycle.BeginParams{AppName: "demo", ClusterID: "prod"}) + require.NoError(t, err) + + h.streams.Register("stream-blocked", makeTar(t, dockerfileTarball(t))) + err = h.executor.Start(sagaBuildFromTar). + Input("app_name", "demo"). + Input("stream_id", "stream-blocked"). + Input("deploy_cluster_id", "prod"). + WithID("test-blocked"). + Execute(ctx) + require.Error(t, err, "a build blocked by the lock must fail") +} diff --git a/servers/build/deploy_tracking.go b/servers/build/deploy_tracking.go new file mode 100644 index 00000000..cca3170e --- /dev/null +++ b/servers/build/deploy_tracking.go @@ -0,0 +1,212 @@ +package build + +import ( + "context" + "encoding/json" + "log/slog" + "time" + + "miren.dev/runtime/api/build/build_v1alpha" + "miren.dev/runtime/api/core/core_v1alpha" + "miren.dev/runtime/pkg/deploylifecycle" + "miren.dev/runtime/pkg/rpc/standard" + "miren.dev/runtime/pkg/rpc/stream" +) + +// deployTracking threads the server-owned deployment record through a build. +// +// A nil *deployTracking means this build is not tracked — an ephemeral build, or +// an older client that still drives its own deployment record — and every method +// is then a no-op. Call sites therefore never branch on whether tracking is +// active; they call through unconditionally and a nil receiver does the right +// thing. +type deployTracking struct { + tracker *deploylifecycle.Tracker + deploymentID string + status *stream.SendStreamClient[*build_v1alpha.Status] + log *slog.Logger +} + +// beginDeploy creates and locks a deployment record when the client asked the +// server to own it, and returns a tracker for the rest of the build. +// +// It returns nil (not an error) when the build is not server-tracked: no +// DeployRequest, or an ephemeral build, which has no deployment record by +// design. A lock already held by a live deployment is a real error and is +// returned as such — the build must not proceed alongside another one. +func (b *Builder) beginDeploy( + ctx context.Context, + appName string, + req *build_v1alpha.DeployRequest, + ephemeral *ephemeralOpts, + status *stream.SendStreamClient[*build_v1alpha.Status], +) (*deployTracking, error) { + if req == nil || (ephemeral != nil && ephemeral.label != "") { + return nil, nil + } + + clusterID := req.ClusterId() + + rec, err := b.deploy.Begin(ctx, deploylifecycle.BeginParams{ + AppName: appName, + ClusterID: clusterID, + GitInfo: gitInfoFromRequest(req), + }) + if err != nil { + return nil, err + } + + dt := &deployTracking{ + tracker: b.deploy, + deploymentID: string(rec.Deployment.ID), + status: status, + log: b.Log.With("deployment_id", rec.Deployment.ID), + } + + // Announce the record and its opening phase so the client can display and + // cancel a deployment it did not create. + dt.emit(ctx, string(deploylifecycle.PhasePreparing)) + return dt, nil +} + +// setPhase advances the record's phase and tells the client about it. +func (t *deployTracking) setPhase(ctx context.Context, phase deploylifecycle.Phase) { + if t == nil { + return + } + + if err := t.tracker.SetPhase(ctx, t.deploymentID, phase); err != nil { + t.log.Error("failed to set deployment phase", "phase", phase, "error", err) + return + } + t.emit(ctx, string(phase)) +} + +// settleTimeout bounds the detached settle operations below. +const settleTimeout = 15 * time.Second + +// settleContext detaches from ctx's cancellation. Recording the committed state +// of a deploy — the version, the activation, a failure — must complete even when +// the build's context has been cancelled by a client disconnect; otherwise the +// record is stranded in_progress with the deploy lock held until it expires. +func settleContext(ctx context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(context.WithoutCancel(ctx), settleTimeout) +} + +// setAppVersion records the version the build produced. +func (t *deployTracking) setAppVersion(ctx context.Context, appVersionID string) { + if t == nil { + return + } + + settleCtx, cancel := settleContext(ctx) + defer cancel() + + if err := t.tracker.SetAppVersion(settleCtx, t.deploymentID, appVersionID); err != nil { + t.log.Error("failed to record deployment app version", + "app_version_id", appVersionID, "error", err) + } +} + +// activate marks the deployment live once the new version is running. +// +// The version is already live by the time this runs, so the settle uses a +// detached context and, if it still fails, releases the lock directly as a +// backstop — a live deploy must never leave the app stalled behind a record that +// will not settle. +func (t *deployTracking) activate(ctx context.Context) { + if t == nil { + return + } + + settleCtx, cancel := settleContext(ctx) + defer cancel() + + if err := t.tracker.Activate(settleCtx, t.deploymentID); err != nil { + t.log.Error("failed to activate deployment; releasing lock as backstop", "error", err) + if relErr := t.tracker.ReleaseLock(settleCtx, t.deploymentID); relErr != nil { + t.log.Error("failed to release lock after activation error", "error", relErr) + } + } +} + +// failOnError settles the deployment as failed when a build returned an error, +// and does nothing when it succeeded. It is the deferred finalizer for the build +// entry points: pass it the method's returned error and it decides. +// +// It runs its settle on a detached context, because the most common failure is +// the client disconnecting, which cancels the build's context — the very moment +// we most need the record to reach a terminal state and release the lock. +func (t *deployTracking) failOnError(ctx context.Context, retErr error) { + if t == nil || retErr == nil { + return + } + + settleCtx, cancel := settleContext(ctx) + defer cancel() + + if err := t.tracker.FailIfUnsettled(settleCtx, t.deploymentID, retErr.Error(), ""); err != nil { + t.log.Error("failed to record deployment failure", "error", err) + } +} + +// emit sends the deployment progress arm on the build status stream. A send +// failure is not fatal to the build: the record is authoritative, the stream is +// only a view of it. +func (t *deployTracking) emit(ctx context.Context, phase string) { + if t == nil || t.status == nil { + return + } + + progress := &build_v1alpha.DeploymentProgress{} + progress.SetDeploymentId(t.deploymentID) + progress.SetPhase(phase) + + so := new(build_v1alpha.Status) + so.Update().SetDeployment(progress) + + if _, err := t.status.Send(ctx, so); err != nil { + t.log.Debug("failed to send deployment progress to client", "error", err) + } +} + +// marshalDeployGitInfo serializes the request's git info to JSON for seeding +// through the saga's string-keyed input map. Returns "" when there is none. +func marshalDeployGitInfo(req *build_v1alpha.DeployRequest) string { + info := gitInfoFromRequest(req) + if info == (core_v1alpha.GitInfo{}) { + return "" + } + data, err := json.Marshal(info) + if err != nil { + return "" + } + return string(data) +} + +// gitInfoFromRequest converts the build-service git shape into the core entity +// shape stored on the deployment record. +func gitInfoFromRequest(req *build_v1alpha.DeployRequest) core_v1alpha.GitInfo { + if req == nil || !req.HasGitInfo() || req.GitInfo() == nil { + return core_v1alpha.GitInfo{} + } + + gi := req.GitInfo() + + info := core_v1alpha.GitInfo{ + Sha: gi.Sha(), + Branch: gi.Branch(), + Repository: gi.Repository(), + IsDirty: gi.IsDirty(), + WorkingTreeHash: gi.WorkingTreeHash(), + Message: gi.CommitMessage(), + Author: gi.CommitAuthorName(), + CommitAuthorEmail: gi.CommitAuthorEmail(), + } + + if gi.HasCommitTimestamp() && gi.CommitTimestamp() != nil { + info.CommitTimestamp = standard.FromTimestamp(gi.CommitTimestamp()).Format(time.RFC3339) + } + + return info +} diff --git a/servers/build/deploy_tracking_test.go b/servers/build/deploy_tracking_test.go new file mode 100644 index 00000000..0bf7a1cd --- /dev/null +++ b/servers/build/deploy_tracking_test.go @@ -0,0 +1,285 @@ +package build + +import ( + "context" + "io" + "log/slog" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "miren.dev/runtime/api/build/build_v1alpha" + "miren.dev/runtime/api/entityserver" + "miren.dev/runtime/pkg/deploylifecycle" + "miren.dev/runtime/pkg/entity/testutils" + "miren.dev/runtime/pkg/rpc/standard" +) + +func newDeployTestBuilder(t *testing.T) *Builder { + t.Helper() + + inmem, cleanup := testutils.NewInMemEntityServer(t) + t.Cleanup(cleanup) + + log := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelError})) + return &Builder{ + Log: log, + EAS: inmem.EAC, + ec: entityserver.NewClient(log, inmem.EAC), + deploy: deploylifecycle.NewTracker(log, inmem.EAC), + } +} + +func deployRequest(cluster string) *build_v1alpha.DeployRequest { + req := &build_v1alpha.DeployRequest{} + req.SetClusterId(cluster) + return req +} + +// The ownership signal: a build with no DeployRequest must leave the server +// managing no deployment record at all, so an older client keeps driving its +// own and there is never a second record. +func TestBeginDeployWithoutRequestTracksNothing(t *testing.T) { + ctx := context.Background() + b := newDeployTestBuilder(t) + + dt, err := b.beginDeploy(ctx, "web", nil, nil, nil) + require.NoError(t, err) + assert.Nil(t, dt, "no request means no server-owned record") + + all, err := b.deploy.Store().List(ctx, deploylifecycle.Query{AppName: "web"}) + require.NoError(t, err) + assert.Empty(t, all, "the server must not have created a record") +} + +// Ephemeral builds have no deployment record by design, even when a request is +// present. +func TestBeginDeploySkipsEphemeral(t *testing.T) { + ctx := context.Background() + b := newDeployTestBuilder(t) + + dt, err := b.beginDeploy(ctx, "web", deployRequest("prod"), + &ephemeralOpts{label: "pr-42", ttl: "24h"}, nil) + require.NoError(t, err) + assert.Nil(t, dt) + + all, err := b.deploy.Store().List(ctx, deploylifecycle.Query{AppName: "web"}) + require.NoError(t, err) + assert.Empty(t, all) +} + +func TestBeginDeployCreatesExactlyOneRecord(t *testing.T) { + ctx := context.Background() + b := newDeployTestBuilder(t) + + dt, err := b.beginDeploy(ctx, "web", deployRequest("prod"), nil, nil) + require.NoError(t, err) + require.NotNil(t, dt) + + all, err := b.deploy.Store().List(ctx, deploylifecycle.Query{AppName: "web"}) + require.NoError(t, err) + require.Len(t, all, 1) + assert.Equal(t, dt.deploymentID, string(all[0].Deployment.ID)) + assert.Equal(t, deploylifecycle.StatusInProgress, all[0].Status()) + assert.Equal(t, "prod", all[0].Deployment.ClusterId) +} + +// A build blocked by another in-flight deploy must surface the lock error, and +// must not leave a second record behind. +func TestBeginDeployPropagatesLockHeld(t *testing.T) { + ctx := context.Background() + b := newDeployTestBuilder(t) + + _, err := b.beginDeploy(ctx, "web", deployRequest("prod"), nil, nil) + require.NoError(t, err) + + dt, err := b.beginDeploy(ctx, "web", deployRequest("prod"), nil, nil) + require.ErrorIs(t, err, deploylifecycle.ErrLockHeld) + assert.Nil(t, dt) + + all, err := b.deploy.Store().List(ctx, deploylifecycle.Query{AppName: "web"}) + require.NoError(t, err) + assert.Len(t, all, 1, "the blocked build must not create a second record") +} + +// Every method on a nil *deployTracking must be a safe no-op, because the +// untracked build path calls straight through without checking. +func TestDeployTrackingNilIsNoOp(t *testing.T) { + ctx := context.Background() + var dt *deployTracking + + assert.NotPanics(t, func() { + dt.setPhase(ctx, deploylifecycle.PhaseBuilding) + dt.setAppVersion(ctx, "app_version/v1") + dt.activate(ctx) + dt.failOnError(ctx, assert.AnError) + dt.failOnError(ctx, nil) + dt.emit(ctx, "building") + }) +} + +func TestFailOnError(t *testing.T) { + ctx := context.Background() + b := newDeployTestBuilder(t) + + t.Run("records a failure", func(t *testing.T) { + dt, err := b.beginDeploy(ctx, "web", deployRequest("prod"), nil, nil) + require.NoError(t, err) + + dt.failOnError(ctx, assert.AnError) + + rec, err := b.deploy.Store().Get(ctx, dt.deploymentID) + require.NoError(t, err) + assert.Equal(t, deploylifecycle.StatusFailed, rec.Status()) + assert.Equal(t, assert.AnError.Error(), rec.Deployment.ErrorMessage) + }) + + t.Run("does nothing on success", func(t *testing.T) { + dt, err := b.beginDeploy(ctx, "api", deployRequest("prod"), nil, nil) + require.NoError(t, err) + + dt.failOnError(ctx, nil) + + rec, err := b.deploy.Store().Get(ctx, dt.deploymentID) + require.NoError(t, err) + assert.Equal(t, deploylifecycle.StatusInProgress, rec.Status(), + "a build that returned no error must stay in_progress") + }) +} + +// The finalizer must still settle the record when the build's own context was +// cancelled — the client disconnecting is exactly when the lock most needs +// releasing. +func TestFailOnErrorSettlesEvenWhenContextCancelled(t *testing.T) { + b := newDeployTestBuilder(t) + + dt, err := b.beginDeploy(context.Background(), "web", deployRequest("prod"), nil, nil) + require.NoError(t, err) + + cancelled, cancel := context.WithCancel(context.Background()) + cancel() + + dt.failOnError(cancelled, assert.AnError) + + rec, err := b.deploy.Store().Get(context.Background(), dt.deploymentID) + require.NoError(t, err) + assert.Equal(t, deploylifecycle.StatusFailed, rec.Status()) +} + +// When activation fails after the version is already live, the plain path must +// release the lock as a backstop so the app is not stalled behind a record that +// will never settle. Activation is forced to fail deterministically by moving +// the record to a state Transition-to-active rejects while the lock is held. +func TestActivateReleasesLockWhenSettleFails(t *testing.T) { + ctx := context.Background() + b := newDeployTestBuilder(t) + + rec, err := b.beginDeploy(ctx, "web", deployRequest("prod"), nil, nil) + require.NoError(t, err) + + // Drive the record to "active" out of band, keeping the lock. Activate's + // transition (active -> active) is then a conflict, so tracker.Activate + // errors without releasing the lock — the exact leak scenario. + stored, err := b.deploy.Store().Get(ctx, rec.deploymentID) + require.NoError(t, err) + stored.Deployment.Status = string(deploylifecycle.StatusActive) + require.NoError(t, b.deploy.Store().Put(ctx, stored)) + + // Precondition: the lock is still held (active is non-terminal, so not + // stealable), which would block the next deploy. + blocking, err := b.deploy.Locks().Blocking(ctx, "web") + require.NoError(t, err) + require.NotNil(t, blocking, "lock should still be held before the backstop runs") + + rec.activate(ctx) + + // The backstop released the lock even though the settle failed. + blocking, err = b.deploy.Locks().Blocking(ctx, "web") + require.NoError(t, err) + assert.Nil(t, blocking, "a failed activation must still free the lock") +} + +// A client disconnect (cancelled context) at activation must not prevent the +// settle: the detached context lets it complete. +func TestActivateSurvivesCancelledContext(t *testing.T) { + ctx := context.Background() + b := newDeployTestBuilder(t) + + rec, err := b.beginDeploy(ctx, "web", deployRequest("prod"), nil, nil) + require.NoError(t, err) + require.NoError(t, b.deploy.SetAppVersion(ctx, rec.deploymentID, "app_version/v1")) + + cancelled, cancel := context.WithCancel(ctx) + cancel() + + rec.activate(cancelled) + + settled, err := b.deploy.Store().Get(ctx, rec.deploymentID) + require.NoError(t, err) + assert.Equal(t, deploylifecycle.StatusActive, settled.Status(), + "activation must complete on a detached context despite a cancelled parent") + + blocking, err := b.deploy.Locks().Blocking(ctx, "web") + require.NoError(t, err) + assert.Nil(t, blocking, "a completed activation releases the lock") +} + +func TestGitInfoFromRequest(t *testing.T) { + t.Run("nil request", func(t *testing.T) { + assert.Equal(t, "", gitInfoFromRequest(nil).Sha) + }) + + t.Run("request without git info", func(t *testing.T) { + info := gitInfoFromRequest(deployRequest("prod")) + assert.Equal(t, "", info.Sha) + }) + + t.Run("full conversion", func(t *testing.T) { + ts := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + + gi := &build_v1alpha.GitInfo{} + gi.SetSha("abc123") + gi.SetBranch("main") + gi.SetRepository("git@github.com:acme/web.git") + gi.SetIsDirty(true) + gi.SetWorkingTreeHash("deadbeef") + gi.SetCommitMessage("fix the thing") + gi.SetCommitAuthorName("Ada") + gi.SetCommitAuthorEmail("ada@example.com") + gi.SetCommitTimestamp(standard.ToTimestamp(ts)) + + req := &build_v1alpha.DeployRequest{} + req.SetGitInfo(gi) + + info := gitInfoFromRequest(req) + assert.Equal(t, "abc123", info.Sha) + assert.Equal(t, "main", info.Branch) + assert.Equal(t, "git@github.com:acme/web.git", info.Repository) + assert.True(t, info.IsDirty) + assert.Equal(t, "deadbeef", info.WorkingTreeHash) + assert.Equal(t, "fix the thing", info.Message) + assert.Equal(t, "Ada", info.Author) + assert.Equal(t, "ada@example.com", info.CommitAuthorEmail) + + // Stored as an RFC3339 string in the server's local zone, matching the + // old deployment server; compare the instant, not the rendering. + gotTS, err := time.Parse(time.RFC3339, info.CommitTimestamp) + require.NoError(t, err) + assert.True(t, gotTS.Equal(ts), "want %s, got %s", ts, gotTS) + }) +} + +func TestEphemeralOptsFromArgs(t *testing.T) { + assert.Nil(t, ephemeralOptsFromArgs(false, "", false, "")) + assert.Nil(t, ephemeralOptsFromArgs(true, "", false, ""), "an empty label is not ephemeral") + + got := ephemeralOptsFromArgs(true, "pr-1", false, "") + require.NotNil(t, got) + assert.Equal(t, "pr-1", got.label) + assert.Equal(t, "24h", got.ttl, "ttl defaults when unset") + + got = ephemeralOptsFromArgs(true, "pr-1", true, "48h") + require.NotNil(t, got) + assert.Equal(t, "48h", got.ttl) +} diff --git a/servers/build/saga_builder.go b/servers/build/saga_builder.go index 81fa08cb..4486d965 100644 --- a/servers/build/saga_builder.go +++ b/servers/build/saga_builder.go @@ -131,8 +131,13 @@ func (s *SagaBuilder) BuildFromTar(ctx context.Context, state *build_v1alpha.Bui } }() + var deployReq *build_v1alpha.DeployRequest + if args.HasDeployment() { + deployReq = args.Deployment() + } + executionID := "build-from-tar-" + streamID - if err := s.startBuild(ctx, executionID, name, streamID, args.EnvVars(), ephemeralFromArgs(args)); err != nil { + if err := s.startBuild(ctx, executionID, name, streamID, args.EnvVars(), ephemeralFromArgs(args), deployReq); err != nil { return err } @@ -191,8 +196,13 @@ func (s *SagaBuilder) BuildFromPrepared(ctx context.Context, state *build_v1alph } }() + var deployReq *build_v1alpha.DeployRequest + if args.HasDeployment() { + deployReq = args.Deployment() + } + executionID := "build-from-prepared-" + sessionID - if err := s.startBuild(ctx, executionID, name, sessionID, args.EnvVars(), ephemeralFromArgs(args)); err != nil { + if err := s.startBuild(ctx, executionID, name, sessionID, args.EnvVars(), ephemeralFromArgs(args), deployReq); err != nil { return err } @@ -216,6 +226,7 @@ func (s *SagaBuilder) startBuild( executionID, appName, streamID string, cliEnvVars []*build_v1alpha.EnvironmentVariable, eph *ephemeralOpts, + deployReq *build_v1alpha.DeployRequest, ) error { sb := s.executor.Start(sagaBuildFromTar). Input("app_name", appName). @@ -232,6 +243,16 @@ func (s *SagaBuilder) startBuild( } } + // 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) + } + } + if err := sb.Execute(ctx); err != nil { return fmt.Errorf("build saga: %w", err) } diff --git a/servers/build/status_registry.go b/servers/build/status_registry.go index 89e7af47..04d06d9b 100644 --- a/servers/build/status_registry.go +++ b/servers/build/status_registry.go @@ -41,6 +41,11 @@ type StatusSender interface { // SendLog emits a structured log entry. Currently only the // pre-saga "warn" path for local storage migration uses this. SendLog(level, text string, fields ...*build_v1alpha.LogField) + + // SendDeployment emits the server-owned deployment record's ID and + // current phase, so the client can display and cancel a deployment it + // did not create. + SendDeployment(deploymentID, phase string) } // noopStatusSender is the zero-cost StatusSender used during recovery @@ -52,6 +57,7 @@ func (noopStatusSender) SendPhase(string) {} func (noopStatusSender) SendBuildkit([]byte) {} func (noopStatusSender) SendError(string, ...any) {} func (noopStatusSender) SendLog(string, string, ...*build_v1alpha.LogField) {} +func (noopStatusSender) SendDeployment(string, string) {} // rpcStatusSender adapts the existing per-request SendStreamClient into // the StatusSender interface so saga actions don't have to know about @@ -116,6 +122,16 @@ func (r *rpcStatusSender) SendError(format string, args ...any) { r.send(so) } +func (r *rpcStatusSender) SendDeployment(deploymentID, phase string) { + progress := &build_v1alpha.DeploymentProgress{} + progress.SetDeploymentId(deploymentID) + progress.SetPhase(phase) + + so := new(build_v1alpha.Status) + so.Update().SetDeployment(progress) + r.send(so) +} + func (r *rpcStatusSender) SendLog(level, text string, fields ...*build_v1alpha.LogField) { so := new(build_v1alpha.Status) entry := &build_v1alpha.LogEntry{} diff --git a/servers/build/status_registry_test.go b/servers/build/status_registry_test.go index 0c199241..5db08cec 100644 --- a/servers/build/status_registry_test.go +++ b/servers/build/status_registry_test.go @@ -11,12 +11,18 @@ import ( // sequence of progress messages a saga action emitted. Thread-safe so // it works regardless of which goroutine the action runs on. type recordingSender struct { - mu sync.Mutex - Messages []string - Phases []string - Buildkit [][]byte - Errors []string - Logs []recordedLog + mu sync.Mutex + Messages []string + Phases []string + Buildkit [][]byte + Errors []string + Logs []recordedLog + Deployments []recordedDeployment +} + +type recordedDeployment struct { + DeploymentID string + Phase string } type recordedLog struct { @@ -55,6 +61,12 @@ func (r *recordingSender) SendLog(level, text string, fields ...*build_v1alpha.L r.Logs = append(r.Logs, recordedLog{Level: level, Text: text, Fields: fields}) } +func (r *recordingSender) SendDeployment(deploymentID, phase string) { + r.mu.Lock() + defer r.mu.Unlock() + r.Deployments = append(r.Deployments, recordedDeployment{DeploymentID: deploymentID, Phase: phase}) +} + func TestStatusRegistry_UnregisteredIDReturnsNoop(t *testing.T) { reg := NewStatusRegistry() // SenderFor must always return a usable sender; the noop is what diff --git a/servers/deployment/cross_version_test.go b/servers/deployment/cross_version_test.go new file mode 100644 index 00000000..c5d54af2 --- /dev/null +++ b/servers/deployment/cross_version_test.go @@ -0,0 +1,104 @@ +package deployment + +import ( + "context" + "io" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + deployment_v1alpha "miren.dev/runtime/api/deployment/deployment_v1alpha" + "miren.dev/runtime/pkg/deploylifecycle" + "miren.dev/runtime/pkg/entity/testutils" + "miren.dev/runtime/pkg/rpc" +) + +// These tests pin the "exactly one owner per build" guarantee across CLI/server +// versions. The deployment server (which an older CLI drives through +// CreateDeployment) and the build server (which a newer CLI drives through the +// DeployRequest param) hold their locks in the same deploylifecycle namespace +// over the same entity store. A deploy driven by one must therefore contend +// with a deploy driven by the other — they cannot both hold the lock, so a +// client that somehow triggered both would produce one record, not two. +// +// The build server's ownership is modeled here by a bare deploylifecycle.Tracker +// over the same store, which is exactly what servers/build constructs; importing +// servers/build here would be circular. + +func sharedStoreClientAndTracker(t *testing.T) (*deployment_v1alpha.DeploymentClient, *deploylifecycle.Tracker) { + t.Helper() + + inmem, cleanup := testutils.NewInMemEntityServer(t) + t.Cleanup(cleanup) + + server, err := newTestDeploymentServer(t, slog.Default(), inmem) + require.NoError(t, err) + + client := &deployment_v1alpha.DeploymentClient{ + Client: rpc.LocalClient(deployment_v1alpha.AdaptDeployment(server)), + } + + log := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelError})) + buildTracker := deploylifecycle.NewTracker(log, inmem.EAC) + + return client, buildTracker +} + +// Old CLI took the lock via CreateDeployment; a server-owned build for the same +// app+cluster must lose the lock rather than start a second deploy. +func TestCreateDeploymentBlocksServerOwnedBuild(t *testing.T) { + ctx := context.Background() + client, buildTracker := sharedStoreClientAndTracker(t) + + created, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + require.False(t, created.HasError() && created.Error() != "") + + _, err = buildTracker.Begin(ctx, deploylifecycle.BeginParams{AppName: "web", ClusterID: "prod"}) + require.ErrorIs(t, err, deploylifecycle.ErrLockHeld, + "a server-owned build must not run alongside a CLI-driven deployment") + + // And exactly one in-progress record exists. + records, err := buildTracker.Store().List(ctx, deploylifecycle.Query{ + AppName: "web", Status: deploylifecycle.StatusInProgress, + }) + require.NoError(t, err) + assert.Len(t, records, 1, "the blocked build must not have created a second record") +} + +// Symmetric case: a server-owned build holds the lock; an old CLI's +// CreateDeployment for the same app+cluster is blocked. +func TestServerOwnedBuildBlocksCreateDeployment(t *testing.T) { + ctx := context.Background() + client, buildTracker := sharedStoreClientAndTracker(t) + + _, err := buildTracker.Begin(ctx, deploylifecycle.BeginParams{AppName: "web", ClusterID: "prod"}) + require.NoError(t, err) + + blocked, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err, "the block is a domain outcome, not an RPC error") + require.True(t, blocked.HasError() && blocked.Error() != "", + "CreateDeployment must be blocked by the server-owned build's lock") + assert.Contains(t, blocked.Error(), "blocked") + assert.True(t, blocked.HasLockInfo() && blocked.LockInfo() != nil) +} + +// Once the CLI-driven deployment settles, the lock frees and a server-owned +// build proceeds — the two versions hand off cleanly. +func TestLockHandsOffBetweenVersions(t *testing.T) { + ctx := context.Background() + client, buildTracker := sharedStoreClientAndTracker(t) + + created, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + + // Old CLI settles its record (build failed). + _, err = client.UpdateFailedDeployment(ctx, created.Deployment().Id(), "build broke", "") + require.NoError(t, err) + + // New server-owned build now gets through. + rec, err := buildTracker.Begin(ctx, deploylifecycle.BeginParams{AppName: "web", ClusterID: "prod"}) + require.NoError(t, err, "a settled CLI deployment must free the lock for the next build") + assert.Equal(t, deploylifecycle.StatusInProgress, rec.Status()) +} diff --git a/servers/deployment/lock_integration_test.go b/servers/deployment/lock_integration_test.go new file mode 100644 index 00000000..bee21786 --- /dev/null +++ b/servers/deployment/lock_integration_test.go @@ -0,0 +1,231 @@ +package deployment + +import ( + "context" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "miren.dev/runtime/api/core/core_v1alpha" + deployment_v1alpha "miren.dev/runtime/api/deployment/deployment_v1alpha" + "miren.dev/runtime/pkg/entity/testutils" + "miren.dev/runtime/pkg/rpc" +) + +func newLockTestClient(t *testing.T) (*deployment_v1alpha.DeploymentClient, *testutils.InMemEntityServer) { + t.Helper() + + inmem, cleanup := testutils.NewInMemEntityServer(t) + t.Cleanup(cleanup) + + server, err := newTestDeploymentServer(t, slog.Default(), inmem) + require.NoError(t, err) + + return &deployment_v1alpha.DeploymentClient{ + Client: rpc.LocalClient(deployment_v1alpha.AdaptDeployment(server)), + }, inmem +} + +// A second CreateDeployment for the same app+cluster must be blocked by the +// lock the first one took, and must return structured lock info. +func TestCreateDeploymentTakesAndReportsLock(t *testing.T) { + ctx := context.Background() + client, _ := newLockTestClient(t) + + first, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + require.False(t, first.HasError() && first.Error() != "", "first deploy should succeed: %s", first.Error()) + + second, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + require.True(t, second.HasError() && second.Error() != "", "second deploy must be blocked") + assert.Contains(t, second.Error(), "blocked") + require.True(t, second.HasLockInfo() && second.LockInfo() != nil) + assert.Equal(t, first.Deployment().Id(), second.LockInfo().BlockingDeploymentId()) +} + +// The lock is app-scoped: a different app is a different lock, but the same app +// with a different client cluster string is still the same lock (a coordinator +// serves one cluster, and the client cluster string is unreliable — MIR-1465). +func TestCreateDeploymentLockIsScopedPerApp(t *testing.T) { + ctx := context.Background() + client, _ := newLockTestClient(t) + + _, err := client.CreateDeployment(ctx, "web", "garden", "pending-build", nil) + require.NoError(t, err) + + otherApp, err := client.CreateDeployment(ctx, "api", "garden", "pending-build", nil) + require.NoError(t, err) + assert.False(t, otherApp.HasError() && otherApp.Error() != "", "a different app is a different lock") + + // Same app, a different cluster_id string (the CI/manual split) must still + // be blocked — otherwise the two would deploy the same app concurrently. + sameAppOtherClusterString, err := client.CreateDeployment(ctx, "web", "34.122.229.118:8443", "pending-build", nil) + require.NoError(t, err) + assert.True(t, sameAppOtherClusterString.HasError() && sameAppOtherClusterString.Error() != "", + "the same app is a single lock regardless of the cluster string") + assert.Contains(t, sameAppOtherClusterString.Error(), "blocked") +} + +// Settling a deployment through the deprecated status update must release the +// lock, so the next deploy proceeds. +func TestUpdateStatusReleasesLock(t *testing.T) { + ctx := context.Background() + client, _ := newLockTestClient(t) + + first, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + deploymentID := first.Deployment().Id() + + // A version must be recorded before activation is meaningful; the old CLI + // did this via UpdateDeploymentAppVersion. + _, err = client.UpdateDeploymentAppVersion(ctx, deploymentID, "app_version/v1") + require.NoError(t, err) + + _, err = client.UpdateDeploymentStatus(ctx, deploymentID, "active", "") + require.NoError(t, err) + + blocked, err := client.GetDeployLock(ctx, "web", "prod") + require.NoError(t, err) + assert.False(t, blocked.Held(), "an activated deployment must not keep the lock") + + next, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + assert.False(t, next.HasError() && next.Error() != "", "the next deploy must not be blocked") +} + +func TestUpdateFailedDeploymentReleasesLock(t *testing.T) { + ctx := context.Background() + client, _ := newLockTestClient(t) + + first, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + + _, err = client.UpdateFailedDeployment(ctx, first.Deployment().Id(), "build broke", "logs") + require.NoError(t, err) + + blocked, err := client.GetDeployLock(ctx, "web", "prod") + require.NoError(t, err) + assert.False(t, blocked.Held(), "a failed deployment must release the lock") +} + +func TestCancelDeploymentReleasesLock(t *testing.T) { + ctx := context.Background() + client, _ := newLockTestClient(t) + + first, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + + cancel, err := client.CancelDeployment(ctx, first.Deployment().Id(), "") + require.NoError(t, err) + require.True(t, cancel.Success()) + + blocked, err := client.GetDeployLock(ctx, "web", "prod") + require.NoError(t, err) + assert.False(t, blocked.Held(), "a cancelled deployment must release the lock") +} + +func TestGetDeployLockReportsHolder(t *testing.T) { + ctx := context.Background() + client, _ := newLockTestClient(t) + + free, err := client.GetDeployLock(ctx, "web", "prod") + require.NoError(t, err) + assert.False(t, free.Held(), "no deploy has run, so nothing is held") + + first, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + + held, err := client.GetDeployLock(ctx, "web", "prod") + require.NoError(t, err) + require.True(t, held.Held()) + require.True(t, held.HasLockInfo() && held.LockInfo() != nil) + assert.Equal(t, first.Deployment().Id(), held.LockInfo().BlockingDeploymentId()) +} + +func TestGetDeployLockValidatesArgs(t *testing.T) { + ctx := context.Background() + client, _ := newLockTestClient(t) + + _, err := client.GetDeployLock(ctx, "", "prod") + require.Error(t, err) + + _, err = client.GetDeployLock(ctx, "web", "") + require.Error(t, err) +} + +// DeployVersion must not strand the lock if its activation settle fails: a +// second deploy started while the first's record holds the lock must be blocked, +// but once the first settles the lock frees. This exercises the backstop path by +// racing the deprecated CreateDeployment lock against a rollback deploy. +func TestDeployVersionActivationDoesNotStrandLock(t *testing.T) { + ctx := context.Background() + client, inmem := newLockTestClient(t) + + // Seed an app and version so DeployVersion can activate it. + _, err := inmem.Client.Create(ctx, "web", &core_v1alpha.App{}) + require.NoError(t, err) + _, err = inmem.Client.Create(ctx, "web-v1", &core_v1alpha.AppVersion{Version: "web-v1"}) + require.NoError(t, err) + + // A successful rollback-style deploy must leave the lock free afterward. + res, err := client.DeployVersion(ctx, "web", "prod", "web-v1", false, nil, "", "") + require.NoError(t, err) + require.False(t, res.HasError() && res.Error() != "", "deploy should succeed: %s", res.Error()) + + lock, err := client.GetDeployLock(ctx, "web", "prod") + require.NoError(t, err) + assert.False(t, lock.Held(), "a completed DeployVersion must release the lock") + + // And a subsequent deploy is not blocked. + res2, err := client.DeployVersion(ctx, "web", "prod", "web-v1", false, nil, "", "") + require.NoError(t, err) + assert.False(t, res2.HasError() && res2.Error() != "", "the next deploy must not be blocked") +} + +// The "pending-build" placeholder an older client writes must render as empty +// in history, not as the sentinel. +func TestPendingBuildSentinelNormalizedInHistory(t *testing.T) { + ctx := context.Background() + client, _ := newLockTestClient(t) + + created, err := client.CreateDeployment(ctx, "web", "prod", "pending-build", nil) + require.NoError(t, err) + assert.Equal(t, "", created.Deployment().AppVersionId(), + "the pending-build placeholder must not surface to clients") + + // And through the list path. + list, err := client.ListDeployments(ctx, "web", "prod", "", 10) + require.NoError(t, err) + require.Len(t, list.Deployments(), 1) + assert.Equal(t, "", list.Deployments()[0].AppVersionId()) +} + +// A legacy failed- sentinel already in storage must also normalize to empty. +func TestFailedSentinelNormalizedInHistory(t *testing.T) { + ctx := context.Background() + client, inmem := newLockTestClient(t) + + id, err := inmem.Client.Create(ctx, "legacy-dep", &core_v1alpha.Deployment{ + AppName: "web", + ClusterId: "prod", + Status: "failed", + }) + require.NoError(t, err) + + // Rewrite app_version to the legacy failed- sentinel now that we know the id. + dep := &core_v1alpha.Deployment{ + ID: id, + AppName: "web", + ClusterId: "prod", + Status: "failed", + AppVersion: "failed-" + string(id), + } + require.NoError(t, inmem.Client.Update(ctx, dep)) + + got, err := client.GetDeploymentById(ctx, string(id)) + require.NoError(t, err) + assert.Equal(t, "", got.Deployment().AppVersionId(), + "a legacy failed- sentinel must render as empty") +} diff --git a/servers/deployment/server.go b/servers/deployment/server.go index 3d96e915..67917364 100644 --- a/servers/deployment/server.go +++ b/servers/deployment/server.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "log/slog" - "sort" "strings" "time" @@ -16,6 +15,7 @@ import ( "miren.dev/runtime/api/entityserver/entityserver_v1alpha" "miren.dev/runtime/api/ingress" "miren.dev/runtime/pkg/cond" + "miren.dev/runtime/pkg/deploylifecycle" "miren.dev/runtime/pkg/entity" ephemeralx "miren.dev/runtime/pkg/ephemeral" "miren.dev/runtime/pkg/idgen" @@ -30,12 +30,16 @@ type DeploymentServer struct { AppClient *appclient.Client IngressClient *ingress.Client DNSHostname string + + // tracker owns the deployment record state machine and the deploy lock. The + // build server holds its own Tracker over the same entity store, and both + // acquire the same app-scoped lock, so a client that drives a record through + // the deprecated RPCs still contends with a server-owned build. + tracker *deploylifecycle.Tracker } var _ deployment_v1alpha.Deployment = (*DeploymentServer)(nil) -const deploymentLockTimeout = 30 * time.Minute - func NewDeploymentServer(log *slog.Logger, eac *entityserver_v1alpha.EntityAccessClient, ec *aes.Client, appClient *appclient.Client, dnsHostname string) (*DeploymentServer, error) { return &DeploymentServer{ Log: log.With("module", "deployment"), @@ -44,9 +48,65 @@ func NewDeploymentServer(log *slog.Logger, eac *entityserver_v1alpha.EntityAcces AppClient: appClient, IngressClient: ingress.NewClient(log, eac), DNSHostname: dnsHostname, + tracker: deploylifecycle.NewTracker(log, eac), }, nil } +// lockBlockable is the shared result surface for the deploy paths that report a +// held lock back to the client as a structured domain outcome. +type lockBlockable interface { + SetError(string) + SetLockInfo(*deployment_v1alpha.DeploymentLockInfo) +} + +// lockInfoFor builds the structured lock info for a held lock. The holder comes +// from the lock; the display details (phase, who started it, short id) come from +// the blocking record. +func (d *DeploymentServer) lockInfoFor(ctx context.Context, holder *deploylifecycle.Holder) *deployment_v1alpha.DeploymentLockInfo { + info := &deployment_v1alpha.DeploymentLockInfo{} + info.SetAppName(holder.AppName) + info.SetBlockingDeploymentId(holder.DeploymentID) + info.SetLockExpiresAt(standard.ToTimestamp(holder.ExpiresAt)) + + displayEmail := "-" + if rec, err := d.tracker.Store().Get(ctx, holder.DeploymentID); err == nil { + // The lock is app-scoped, so cluster for display comes from the blocking + // record rather than the lock. + info.SetClusterId(rec.Deployment.ClusterId) + info.SetCurrentPhase(rec.Deployment.Phase) + info.SetBlockingDeploymentShortId(shortIDFromRPCEntity(rec.Entity)) + + if email := rec.Deployment.DeployedBy.UserEmail; email != "" && + email != "unknown@example.com" && email != "user@example.com" { + displayEmail = email + } + if ts, err := time.Parse(time.RFC3339, rec.Deployment.DeployedBy.Timestamp); err == nil { + info.SetStartedAt(standard.ToTimestamp(ts)) + } + } + info.SetStartedBy(displayEmail) + + return info +} + +// reportLockBlocked fills results with the structured "deployment blocked" +// domain outcome for a held lock. +func (d *DeploymentServer) reportLockBlocked(ctx context.Context, results lockBlockable, holder *deploylifecycle.Holder) { + results.SetLockInfo(d.lockInfoFor(ctx, holder)) + results.SetError("deployment blocked by existing in-progress deployment") +} + +// releaseDeployLock drops the deploy lock held by a deployment that has just +// settled through one of the deprecated client-driven methods. A failure to +// release is logged, not returned: the record already reached its terminal +// state, and the lock will expire on its own. +func (d *DeploymentServer) releaseDeployLock(ctx context.Context, appName, deploymentID string) { + if err := d.tracker.Locks().Release(ctx, appName, deploymentID); err != nil { + d.Log.Error("failed to release deploy lock", + "deployment_id", deploymentID, "app", appName, "error", err) + } +} + func (d *DeploymentServer) CreateDeployment(ctx context.Context, req *deployment_v1alpha.DeploymentCreateDeployment) error { args := req.Args() results := req.Results() @@ -64,171 +124,43 @@ func (d *DeploymentServer) CreateDeployment(ctx context.Context, req *deployment appName := args.AppName() clusterId := args.ClusterId() - appVersionId := args.AppVersionId() if !rpc.AllowApp(ctx, appName) { return rpc.AppAccessError(ctx, appName) } - // Check for existing in_progress deployments for this app - existingDeployments, err := d.listDeploymentsInternal(ctx, appName, "in_progress", 1) - if err != nil { - d.Log.Error("Failed to check for existing deployments", "error", err) - return cond.Error("failed to check deployment lock") - } - - if len(existingDeployments) > 0 { - // Found an existing in_progress deployment - existing := existingDeployments[0].deployment - existingEnt := existingDeployments[0].entity - - // Parse the deployment timestamp - // Treat malformed/empty timestamps as expired to avoid blocking deployments - deploymentTime, err := time.Parse(time.RFC3339, existing.DeployedBy.Timestamp) - if err != nil { - d.Log.Warn("Deployment has malformed timestamp, treating as expired", - "deployment_id", string(existing.ID), - "timestamp", existing.DeployedBy.Timestamp, - "error", err) - // Fall through to mark as failed below - deploymentTime = time.Time{} // Zero time, will be treated as expired - } - - // Check if the existing deployment is expired (older than deploymentLockTimeout) - isExpired := deploymentTime.IsZero() || time.Since(deploymentTime) >= deploymentLockTimeout - if !isExpired { - // Deployment is still within the lock timeout - return structured lock info - lockExpiresAt := deploymentTime.Add(deploymentLockTimeout) - - // Format user email for display - displayEmail := existing.DeployedBy.UserEmail - if displayEmail == "" || displayEmail == "unknown@example.com" || displayEmail == "user@example.com" { - displayEmail = "-" - } - - // Create structured lock info - lockInfo := &deployment_v1alpha.DeploymentLockInfo{} - lockInfo.SetAppName(appName) - lockInfo.SetClusterId(clusterId) - lockInfo.SetBlockingDeploymentId(string(existing.ID)) - lockInfo.SetBlockingDeploymentShortId(shortIDFromRPCEntity(existingEnt)) - lockInfo.SetStartedBy(displayEmail) - lockInfo.SetStartedAt(standard.ToTimestamp(deploymentTime)) - lockInfo.SetCurrentPhase(existing.Phase) - lockInfo.SetLockExpiresAt(standard.ToTimestamp(lockExpiresAt)) - - results.SetLockInfo(lockInfo) - - // Also set a simple error message for backwards compatibility - results.SetError("deployment blocked by existing in-progress deployment") - return nil - } - - // Existing deployment is expired, mark it as failed - d.Log.Warn("Found expired in_progress deployment, marking as failed", - "deployment_id", string(existing.ID), - "age", time.Since(deploymentTime)) - - // Update the expired deployment to failed status - // We need to call the internal method since we're in the server, not using the client - existing.Status = "failed" - existing.ErrorMessage = fmt.Sprintf("Deployment timed out after %v", deploymentLockTimeout) - existing.CompletedAt = time.Now().Format(time.RFC3339) - - // Update entity - updateAttrs := existing.Encode() - updateEntity := &entityserver_v1alpha.Entity{} - updateEntity.SetId(string(existing.ID)) - updateEntity.SetAttrs(updateAttrs) - - // Get the current revision for the update - // Note: If another process updates this entity between our Get and Put, - // the Put will fail with a revision conflict. This is acceptable because - // it means another process already handled this expired deployment. - // We continue creating the new deployment either way. - if existingEntity, err := d.EAC.Get(ctx, string(existing.ID)); err == nil { - updateEntity.SetRevision(existingEntity.Entity().Revision()) - if _, err := d.EAC.Put(ctx, updateEntity); err != nil { - d.Log.Error("Failed to mark expired deployment as failed", "error", err) - // Continue anyway - we'll create the new deployment - } - } else { - d.Log.Error("Failed to get expired deployment for update", "error", err) - // Continue anyway - we'll create the new deployment - } + var gitInfo core_v1alpha.GitInfo + if args.HasGitInfo() { + gitInfo = gitInfoFromRPC(args.GitInfo()) } - // Get user info from context (will be implemented with auth integration) - // For now, leave empty - the CLI display will show "-" for unknown users - userId := "" - userName := "" - userEmail := "" - - // Create deployment entity - now := time.Now() - - deployment := &core_v1alpha.Deployment{ + // Begin creates the record and takes the deploy lock atomically, replacing + // the old list-then-create-with-timeout dance. The "pending-build" sentinel + // the older CLI passes is normalized away — app_version is optional now. + rec, err := d.tracker.Begin(ctx, deploylifecycle.BeginParams{ AppName: appName, - AppVersion: appVersionId, - ClusterId: clusterId, - Status: "in_progress", - Phase: "preparing", - DeployedBy: core_v1alpha.DeployedBy{ - UserId: userId, - UserName: userName, - UserEmail: userEmail, - Timestamp: now.Format(time.RFC3339), - }, - } - - // Add git info if provided - if args.HasGitInfo() && args.GitInfo() != nil { - gitInfo := args.GitInfo() - deployment.GitInfo = core_v1alpha.GitInfo{ - Sha: gitInfo.Sha(), - Branch: gitInfo.Branch(), - Message: gitInfo.CommitMessage(), - Author: gitInfo.CommitAuthorName(), - IsDirty: gitInfo.IsDirty(), - WorkingTreeHash: gitInfo.WorkingTreeHash(), - CommitAuthorEmail: gitInfo.CommitAuthorEmail(), - Repository: gitInfo.Repository(), - } - - // Handle optional timestamp - if gitInfo.HasCommitTimestamp() && gitInfo.CommitTimestamp() != nil { - deployment.GitInfo.CommitTimestamp = standard.FromTimestamp(gitInfo.CommitTimestamp()).Format(time.RFC3339) - } - } - - // Create entity - attrs := deployment.Encode() - rpcEntity := &entityserver_v1alpha.Entity{} - rpcEntity.SetAttrs(attrs) - - putResp, err := d.EAC.Put(ctx, rpcEntity) + ClusterID: clusterId, + AppVersion: normalizeAppVersion(args.AppVersionId(), ""), + GitInfo: gitInfo, + }) if err != nil { - d.Log.Error("Failed to create deployment entity", "error", err) + if holder, ok := deploylifecycle.HolderFrom(err); ok { + d.reportLockBlocked(ctx, results, holder) + return nil + } + d.Log.Error("Failed to create deployment", "error", err) return cond.Error("failed to create deployment") } - // Set the deployment ID from the entity server response - deployment.ID = entity.Id(putResp.Id()) - - // Convert to RPC response - deploymentInfo := d.toDeploymentInfo(deployment) - if depEnt, err := d.EAC.Get(ctx, putResp.Id()); err == nil { - versionShortIDs := d.resolveShortIDs(ctx, []string{deployment.AppVersion}) - enrichDeploymentShortIDs(deploymentInfo, depEnt.Entity(), versionShortIDs) - } + deploymentInfo := d.toDeploymentInfo(rec.Deployment) + versionShortIDs := d.resolveShortIDs(ctx, []string{rec.Deployment.AppVersion}) + enrichDeploymentShortIDs(deploymentInfo, rec.Entity, versionShortIDs) results.SetDeployment(deploymentInfo) d.Log.Info("Created deployment", - "deployment_id", putResp.Id(), + "deployment_id", rec.Deployment.ID, "app", appName, - "cluster", clusterId, - "version", appVersionId, - "user", userEmail) + "cluster", clusterId) return nil } @@ -248,18 +180,10 @@ func (d *DeploymentServer) UpdateDeploymentStatus(ctx context.Context, req *depl deploymentId := args.DeploymentId() newStatus := args.Status() - // Validate status value - validStatuses := map[string]bool{ - "in_progress": true, - "active": true, - "succeeded": true, - "failed": true, - "rolled_back": true, - "cancelled": true, - } - if !validStatuses[newStatus] { - return cond.ValidationFailure("invalid-status", - "status must be one of: in_progress, active, succeeded, failed, rolled_back") + // Validate against the single source of truth so the accepted set and the + // error message can never drift (the old inline message omitted "cancelled"). + if _, err := deploylifecycle.ParseStatus(newStatus); err != nil { + return err } // Get existing deployment @@ -298,7 +222,7 @@ func (d *DeploymentServer) UpdateDeploymentStatus(ctx context.Context, req *depl // If marking as active, mark all other active deployments for this app/cluster as succeeded if newStatus == "active" { - err = d.markPreviousActiveAs(ctx, deployment.AppName, deployment.ClusterId, deploymentId, "succeeded") + err = d.markPreviousActiveAs(ctx, deployment.AppName, deploymentId, "succeeded") if err != nil { d.Log.Error("Failed to mark previous active deployments as succeeded", "error", err) // Don't fail the whole operation, just log the error @@ -318,6 +242,13 @@ func (d *DeploymentServer) UpdateDeploymentStatus(ctx context.Context, req *depl return cond.Error("failed to update deployment") } + // The deployment has left in_progress, so release the deploy lock this + // record's Begin took. Release is a no-op if a newer deployment already + // holds it. + if newStatus != "in_progress" { + d.releaseDeployLock(ctx, deployment.AppName, deploymentId) + } + // Convert to RPC response deploymentInfo := d.toDeploymentInfo(&deployment) results.SetDeployment(deploymentInfo) @@ -449,10 +380,8 @@ func (d *DeploymentServer) UpdateFailedDeployment(ctx context.Context, req *depl deployment.BuildLogs = buildLogs deployment.CompletedAt = time.Now().Format(time.RFC3339) - // Update app version to failed pattern if it's still pending - if string(deployment.AppVersion) == "pending-build" { - deployment.AppVersion = fmt.Sprintf("failed-%s", deploymentId) - } + // The "pending-build" placeholder is left in place and normalized away on + // read, rather than rewritten to a "failed-" sentinel. // Update entity updateAttrs := deployment.Encode() @@ -467,6 +396,9 @@ func (d *DeploymentServer) UpdateFailedDeployment(ctx context.Context, req *depl return cond.Error("failed to update deployment") } + // A failed (or already-cancelled) deployment holds the lock no longer. + d.releaseDeployLock(ctx, deployment.AppName, deploymentId) + // Convert to RPC response deploymentInfo := d.toDeploymentInfo(&deployment) results.SetDeployment(deploymentInfo) @@ -655,6 +587,42 @@ func (d *DeploymentServer) GetActiveDeployment(ctx context.Context, req *deploym return nil } +func (d *DeploymentServer) GetDeployLock(ctx context.Context, req *deployment_v1alpha.DeploymentGetDeployLock) error { + args := req.Args() + results := req.Results() + + if !args.HasAppName() || args.AppName() == "" { + return cond.ValidationFailure("missing-field", "app_name is required") + } + if !args.HasClusterId() || args.ClusterId() == "" { + return cond.ValidationFailure("missing-field", "cluster_id is required") + } + + appName := args.AppName() + clusterId := args.ClusterId() + + if !rpc.AllowApp(ctx, appName) { + return rpc.AppAccessError(ctx, appName) + } + + // Blocking folds in expiry and terminal-holder reconciliation, so a released + // or dead lock reports as free rather than as a phantom block. + holder, err := d.tracker.Locks().Blocking(ctx, appName) + if err != nil { + d.Log.Error("Failed to read deploy lock", "app", appName, "cluster", clusterId, "error", err) + return cond.Error("failed to read deploy lock") + } + + if holder == nil { + results.SetHeld(false) + return nil + } + + results.SetHeld(true) + results.SetLockInfo(d.lockInfoFor(ctx, holder)) + return nil +} + func (d *DeploymentServer) CancelDeployment(ctx context.Context, req *deployment_v1alpha.DeploymentCancelDeployment) error { args := req.Args() results := req.Results() @@ -670,13 +638,14 @@ func (d *DeploymentServer) CancelDeployment(ctx context.Context, req *deployment // Get the deployment by ID (resolves short IDs via the entity server) deploymentResp, err := d.EAC.Get(ctx, deploymentId) if err != nil { + // "not found" is a domain outcome the client renders; a failure to reach + // the store is infrastructure and surfaces as an RPC error. if errors.Is(err, cond.ErrNotFound{}) { results.SetError("deployment not found") - } else { - d.Log.Error("Failed to get deployment", "deployment_id", deploymentId, "error", err) - results.SetError("failed to get deployment") + return nil } - return nil + d.Log.Error("Failed to get deployment", "deployment_id", deploymentId, "error", err) + return cond.Error("failed to get deployment") } // Use the resolved entity ID for all subsequent operations @@ -712,10 +681,12 @@ func (d *DeploymentServer) CancelDeployment(ctx context.Context, req *deployment _, err = d.EAC.Put(ctx, updateEntity) if err != nil { d.Log.Error("Failed to cancel deployment", "deployment_id", deploymentId, "error", err) - results.SetError("failed to cancel deployment") - return nil + return cond.Error("failed to cancel deployment") } + // A cancelled deployment holds the lock no longer. + d.releaseDeployLock(ctx, deployment.AppName, deploymentId) + results.SetSuccess(true) d.Log.Info("Cancelled deployment", @@ -857,170 +828,85 @@ func (d *DeploymentServer) DeployVersion(ctx context.Context, req *deployment_v1 // --- Normal (non-ephemeral) deploy path below --- - // Check for existing in_progress deployments (deployment lock) - existingDeployments, err := d.listDeploymentsInternal(ctx, appName, "in_progress", 1) - if err != nil { - d.Log.Error("Failed to check for existing deployments", "error", err) - results.SetError("failed to check deployment lock") - return nil - } - - if len(existingDeployments) > 0 { - existing := existingDeployments[0].deployment - existingEnt := existingDeployments[0].entity - - deploymentTime, parseErr := time.Parse(time.RFC3339, existing.DeployedBy.Timestamp) - if parseErr != nil { - deploymentTime = time.Time{} - } - - isExpired := deploymentTime.IsZero() || time.Since(deploymentTime) >= deploymentLockTimeout - if !isExpired { - lockExpiresAt := deploymentTime.Add(deploymentLockTimeout) - - displayEmail := existing.DeployedBy.UserEmail - if displayEmail == "" || displayEmail == "unknown@example.com" || displayEmail == "user@example.com" { - displayEmail = "-" - } - - lockInfo := &deployment_v1alpha.DeploymentLockInfo{} - lockInfo.SetAppName(appName) - lockInfo.SetClusterId(clusterId) - lockInfo.SetBlockingDeploymentId(string(existing.ID)) - lockInfo.SetBlockingDeploymentShortId(shortIDFromRPCEntity(existingEnt)) - lockInfo.SetStartedBy(displayEmail) - lockInfo.SetStartedAt(standard.ToTimestamp(deploymentTime)) - lockInfo.SetCurrentPhase(existing.Phase) - lockInfo.SetLockExpiresAt(standard.ToTimestamp(lockExpiresAt)) - - results.SetLockInfo(lockInfo) - results.SetError("deployment blocked by existing in-progress deployment") - return nil - } - - // Expired lock — mark as failed and continue - d.Log.Warn("Found expired in_progress deployment, marking as failed", - "deployment_id", string(existing.ID), - "age", time.Since(deploymentTime)) - - existing.Status = "failed" - existing.ErrorMessage = fmt.Sprintf("Deployment timed out after %v", deploymentLockTimeout) - existing.CompletedAt = time.Now().Format(time.RFC3339) - - updateAttrs := existing.Encode() - lockUpdateEntity := &entityserver_v1alpha.Entity{} - lockUpdateEntity.SetId(string(existing.ID)) - lockUpdateEntity.SetAttrs(updateAttrs) - - if existingEntity, getErr := d.EAC.Get(ctx, string(existing.ID)); getErr == nil { - lockUpdateEntity.SetRevision(existingEntity.Entity().Revision()) - if _, putErr := d.EAC.Put(ctx, lockUpdateEntity); putErr != nil { - d.Log.Error("Failed to mark expired deployment as failed", "error", putErr) + // Find the source deployment — the most recent deployment with this + // app_version_id — for git info and rollback provenance. + var gitInfo core_v1alpha.GitInfo + var sourceDeploymentID string + if allDeployments, listErr := d.listDeploymentsInternal(ctx, appName, "", 100); listErr != nil { + d.Log.Error("Failed to list deployments for source lookup", "error", listErr) + } else { + for _, dwe := range allDeployments { + if dwe.deployment.AppVersion == sourceVersionId { + gitInfo = dwe.deployment.GitInfo + sourceDeploymentID = string(dwe.deployment.ID) + break // listDeploymentsInternal returns newest first } } } - // Find the source deployment — the most recent deployment with this app_version_id - allDeployments, err := d.listDeploymentsInternal(ctx, appName, "", 100) + // Begin creates the record and takes the deploy lock atomically, contending + // with any server-owned build for the same app+cluster. + rec, err := d.tracker.Begin(ctx, deploylifecycle.BeginParams{ + AppName: appName, + ClusterID: clusterId, + AppVersion: appVersionId, + GitInfo: gitInfo, + SourceDeploymentID: sourceDeploymentID, + }) if err != nil { - d.Log.Error("Failed to list deployments for source lookup", "error", err) - // Continue without source info - } - - var sourceDeployment *core_v1alpha.Deployment - for _, dwe := range allDeployments { - if dwe.deployment.AppVersion == sourceVersionId { - sourceDeployment = dwe.deployment - break // listDeploymentsInternal returns newest first + if holder, ok := deploylifecycle.HolderFrom(err); ok { + d.reportLockBlocked(ctx, results, holder) + return nil } + d.Log.Error("Failed to create deployment", "error", err) + results.SetError("failed to create deployment") + return nil } - // Create new deployment entity - now := time.Now() + deployment := rec.Deployment + newDeploymentId := string(deployment.ID) - deployment := &core_v1alpha.Deployment{ - AppName: appName, - AppVersion: appVersionId, - ClusterId: clusterId, - Status: "in_progress", - Phase: "activating", - DeployedBy: core_v1alpha.DeployedBy{ - Timestamp: now.Format(time.RFC3339), - }, + // This path has no build; it goes straight to activation. + if phaseErr := d.tracker.SetPhase(ctx, newDeploymentId, deploylifecycle.PhaseActivating); phaseErr != nil { + d.Log.Error("Failed to set activating phase", "error", phaseErr) } - // Copy git info and source ID from the source deployment - if sourceDeployment != nil { - deployment.GitInfo = sourceDeployment.GitInfo - deployment.SourceDeploymentId = string(sourceDeployment.ID) - } + if err := d.AppClient.SetActiveVersion(ctx, appName, string(appVersion.ID)); err != nil { + d.Log.Error("Failed to set active version", "error", err, "app", appName, "version_id", string(appVersion.ID)) - // Create entity - attrs := deployment.Encode() - rpcEntity := &entityserver_v1alpha.Entity{} - rpcEntity.SetAttrs(attrs) + failMsg := fmt.Sprintf("failed to activate version: %v", err) + if failErr := d.tracker.Fail(ctx, newDeploymentId, failMsg, ""); failErr != nil { + d.Log.Error("Failed to mark deployment as failed", "error", failErr) + } - putResp, err := d.EAC.Put(ctx, rpcEntity) - if err != nil { - d.Log.Error("Failed to create deployment entity", "error", err) - results.SetError("failed to create deployment") + results.SetError(failMsg) return nil } - deployment.ID = entity.Id(putResp.Id()) - newDeploymentId := putResp.Id() - - { - // Normal deploy: activate the version - if err := d.AppClient.SetActiveVersion(ctx, appName, string(appVersion.ID)); err != nil { - d.Log.Error("Failed to set active version", "error", err, "app", appName, "version_id", string(appVersion.ID)) - - deployment.Status = "failed" - deployment.ErrorMessage = fmt.Sprintf("failed to activate version: %v", err) - deployment.CompletedAt = time.Now().Format(time.RFC3339) - if current, getErr := d.EAC.Get(ctx, newDeploymentId); getErr == nil { - failEntity := &entityserver_v1alpha.Entity{} - failEntity.SetId(newDeploymentId) - failEntity.SetAttrs(deployment.Encode()) - failEntity.SetRevision(current.Entity().Revision()) - if _, putErr := d.EAC.Put(ctx, failEntity); putErr != nil { - d.Log.Error("Failed to mark deployment as failed", "error", putErr) - } - } - - results.SetError(fmt.Sprintf("failed to activate version: %v", err)) - return nil - } - - // Activation succeeded — mark deployment active - deployment.Status = "active" - deployment.CompletedAt = time.Now().Format(time.RFC3339) - if current, getErr := d.EAC.Get(ctx, newDeploymentId); getErr == nil { - activeEntity := &entityserver_v1alpha.Entity{} - activeEntity.SetId(newDeploymentId) - activeEntity.SetAttrs(deployment.Encode()) - activeEntity.SetRevision(current.Entity().Revision()) - if _, putErr := d.EAC.Put(ctx, activeEntity); putErr != nil { - d.Log.Error("Failed to update deployment status to active", "error", putErr) - } - } + activate := d.tracker.Activate + if isRollback { + activate = d.tracker.ActivateRollback + } + // SetActiveVersion already made the version live, so settle on a detached + // context: a client that cancels the RPC now must not strand the record + // in_progress with the lock held. If the settle still fails, release the + // lock directly so later deploys aren't blocked for the full lock TTL. + settleCtx, cancelSettle := context.WithTimeout(context.WithoutCancel(ctx), 15*time.Second) + defer cancelSettle() + if err := activate(settleCtx, newDeploymentId); err != nil { + d.Log.Error("Failed to activate deployment; releasing lock", "error", err) + d.releaseDeployLock(settleCtx, appName, newDeploymentId) + } - // Mark previous active deployments - targetStatus := "succeeded" - if isRollback { - targetStatus = "rolled_back" - } - if err := d.markPreviousActiveAs(ctx, appName, clusterId, newDeploymentId, targetStatus); err != nil { - d.Log.Error("Failed to mark previous active deployments", "error", err) - // Don't fail — the new deployment is already created and active - } + // Re-read so the response reflects the settled state. + if settled, getErr := d.tracker.Store().Get(settleCtx, newDeploymentId); getErr == nil { + deployment = settled.Deployment + rec = settled } deploymentInfo := d.toDeploymentInfo(deployment) - if depEnt, getErr := d.EAC.Get(ctx, newDeploymentId); getErr == nil { - versionShortIDs := d.resolveShortIDs(ctx, []string{deployment.AppVersion}) - enrichDeploymentShortIDs(deploymentInfo, depEnt.Entity(), versionShortIDs) - } + versionShortIDs := d.resolveShortIDs(ctx, []string{deployment.AppVersion}) + enrichDeploymentShortIDs(deploymentInfo, rec.Entity, versionShortIDs) results.SetDeployment(deploymentInfo) accessInfo := d.getAccessInfo(ctx, appName, "") @@ -1139,109 +1025,47 @@ func (d *DeploymentServer) createEnvVarDeployment(ctx context.Context, appName, appVersionId := mutResult.VersionID - // Check for existing in_progress deployments (deployment lock) - existingDeployments, err := d.listDeploymentsInternal(ctx, appName, "in_progress", 1) - if err != nil { - d.Log.Error("Failed to check for existing deployments", "error", err) - results.SetError("failed to check deployment lock") - return nil - } - - if len(existingDeployments) > 0 { - existing := existingDeployments[0].deployment - existingEnt := existingDeployments[0].entity - - deploymentTime, parseErr := time.Parse(time.RFC3339, existing.DeployedBy.Timestamp) - if parseErr != nil { - deploymentTime = time.Time{} - } - - isExpired := deploymentTime.IsZero() || time.Since(deploymentTime) >= deploymentLockTimeout - if !isExpired { - lockExpiresAt := deploymentTime.Add(deploymentLockTimeout) - - displayEmail := existing.DeployedBy.UserEmail - if displayEmail == "" || displayEmail == "unknown@example.com" || displayEmail == "user@example.com" { - displayEmail = "-" - } - - lockInfo := &deployment_v1alpha.DeploymentLockInfo{} - lockInfo.SetAppName(appName) - lockInfo.SetClusterId(clusterId) - lockInfo.SetBlockingDeploymentId(string(existing.ID)) - lockInfo.SetBlockingDeploymentShortId(shortIDFromRPCEntity(existingEnt)) - lockInfo.SetStartedBy(displayEmail) - lockInfo.SetStartedAt(standard.ToTimestamp(deploymentTime)) - lockInfo.SetCurrentPhase(existing.Phase) - lockInfo.SetLockExpiresAt(standard.ToTimestamp(lockExpiresAt)) - - results.SetLockInfo(lockInfo) - results.SetError("deployment blocked by existing in-progress deployment") - return nil - } - - // Expired lock — mark as failed and continue - d.Log.Warn("Found expired in_progress deployment, marking as failed", - "deployment_id", string(existing.ID), - "age", time.Since(deploymentTime)) - - existing.Status = "failed" - existing.ErrorMessage = fmt.Sprintf("Deployment timed out after %v", deploymentLockTimeout) - existing.CompletedAt = time.Now().Format(time.RFC3339) - - updateAttrs := existing.Encode() - updateEntity := &entityserver_v1alpha.Entity{} - updateEntity.SetId(string(existing.ID)) - updateEntity.SetAttrs(updateAttrs) - - if existingEntity, getErr := d.EAC.Get(ctx, string(existing.ID)); getErr == nil { - updateEntity.SetRevision(existingEntity.Entity().Revision()) - if _, putErr := d.EAC.Put(ctx, updateEntity); putErr != nil { - d.Log.Error("Failed to mark expired deployment as failed", "error", putErr) - } - } - } - - // Create new deployment entity - now := time.Now() - - deployment := &core_v1alpha.Deployment{ + // An env-var change has no build, so the record goes straight from Begin to + // active. Begin takes the deploy lock, contending with any server-owned + // build for the same app+cluster. + rec, err := d.tracker.Begin(ctx, deploylifecycle.BeginParams{ AppName: appName, + ClusterID: clusterId, AppVersion: appVersionId, - ClusterId: clusterId, - Status: "active", - Phase: "activating", - DeployedBy: core_v1alpha.DeployedBy{ - Timestamp: now.Format(time.RFC3339), - }, - CompletedAt: now.Format(time.RFC3339), - } - - // Create entity - attrs := deployment.Encode() - rpcEntity := &entityserver_v1alpha.Entity{} - rpcEntity.SetAttrs(attrs) - - putResp, err := d.EAC.Put(ctx, rpcEntity) + }) if err != nil { - d.Log.Error("Failed to create deployment entity", "error", err) + if holder, ok := deploylifecycle.HolderFrom(err); ok { + d.reportLockBlocked(ctx, results, holder) + return nil + } + d.Log.Error("Failed to create deployment", "error", err) results.SetError("failed to create deployment") return nil } - deployment.ID = entity.Id(putResp.Id()) - newDeploymentId := putResp.Id() + newDeploymentId := string(rec.Deployment.ID) - // Mark previous active deployments as succeeded - if err := d.markPreviousActiveAs(ctx, appName, clusterId, newDeploymentId, "succeeded"); err != nil { - d.Log.Error("Failed to mark previous active deployments", "error", err) + // The new version is already applied by the caller; settle immediately, + // which marks the previous active deployment succeeded and releases the lock. + // Settle on a detached context and release the lock as a backstop so a + // cancelled RPC or transient store error can't strand the record in_progress + // with the lock held (see DeployVersion for the same reasoning). + settleCtx, cancelSettle := context.WithTimeout(context.WithoutCancel(ctx), 15*time.Second) + defer cancelSettle() + if err := d.tracker.Activate(settleCtx, newDeploymentId); err != nil { + d.Log.Error("Failed to activate env var deployment; releasing lock", "error", err) + d.releaseDeployLock(settleCtx, appName, newDeploymentId) } - deploymentInfo := d.toDeploymentInfo(deployment) - if depEnt, getErr := d.EAC.Get(ctx, newDeploymentId); getErr == nil { - versionShortIDs := d.resolveShortIDs(ctx, []string{deployment.AppVersion}) - enrichDeploymentShortIDs(deploymentInfo, depEnt.Entity(), versionShortIDs) + deployment := rec.Deployment + if settled, getErr := d.tracker.Store().Get(settleCtx, newDeploymentId); getErr == nil { + deployment = settled.Deployment + rec = settled } + + deploymentInfo := d.toDeploymentInfo(deployment) + versionShortIDs := d.resolveShortIDs(ctx, []string{deployment.AppVersion}) + enrichDeploymentShortIDs(deploymentInfo, rec.Entity, versionShortIDs) results.SetDeployment(deploymentInfo) accessInfo := d.getAccessInfo(ctx, appName, "") @@ -1312,53 +1136,66 @@ func (d *DeploymentServer) getAccessInfo(ctx context.Context, appName string, ep // Internal helper methods -// listDeploymentsInternal lists deployments from this cluster's entity store, -// optionally filtered by app name and status. It does not filter by cluster: -// the store is a loopback into this coordinator's own etcd, so every deployment -// it holds already belongs to this cluster. The cluster_id stamped on a -// deployment is client-supplied and inconsistent (a manual deploy sends the -// cluster name, a CI/OIDC deploy sends the raw address), so filtering on it -// would hide legitimate deploys — see MIR-1465. +// listDeploymentsInternal lists deployments from this cluster's store, filtered +// by app and status. It does not filter by cluster: the store is a loopback into +// this coordinator's own etcd, so every deployment it holds already belongs to +// this cluster, and the client-supplied cluster_id is unreliable (a manual +// deploy sends the cluster name, a CI/OIDC deploy sends the raw address), so +// filtering on it would hide legitimate deploys. See MIR-1465. func (d *DeploymentServer) listDeploymentsInternal(ctx context.Context, appName, status string, limit int) ([]deploymentWithEntity, error) { - // List all deployments by type - listResp, err := d.EAC.List(ctx, entity.Ref(entity.EntityKind, core_v1alpha.KindDeployment)) + // Backed by the shared indexed store, which selects an index from the + // filters instead of scanning every deployment ever created. + records, err := d.tracker.Store().List(ctx, deploylifecycle.Query{ + AppName: appName, + Status: deploylifecycle.Status(status), + Limit: limit, + }) if err != nil { d.Log.Error("Failed to list deployments", "error", err) return nil, cond.Error("failed to list deployments") } - // Get the entity values - entities := listResp.Values() - - // Decode and filter deployments - deployments := make([]deploymentWithEntity, 0) - for _, e := range entities { - // List already returns full entity data with attributes, no need to fetch again - var dep core_v1alpha.Deployment - decodeEntity(e, &dep) - - // Apply filters - if appName != "" && dep.AppName != appName { - continue - } - if status != "" && dep.Status != status { - continue - } - - deployments = append(deployments, deploymentWithEntity{deployment: &dep, entity: e}) + deployments := make([]deploymentWithEntity, 0, len(records)) + for _, rec := range records { + deployments = append(deployments, deploymentWithEntity{deployment: rec.Deployment, entity: rec.Entity}) } + return deployments, nil +} - // Sort by timestamp (newest first) using efficient sort.Slice - sort.Slice(deployments, func(i, j int) bool { - return deployments[i].deployment.DeployedBy.Timestamp > deployments[j].deployment.DeployedBy.Timestamp - }) +// normalizeAppVersion drops the legacy placeholder values older clients wrote +// into app_version before a build had produced one. The field is optional now, +// so these are reported as empty rather than migrated. +func normalizeAppVersion(version, deploymentID string) string { + if version == "pending-build" { + return "" + } + if deploymentID != "" && version == "failed-"+deploymentID { + return "" + } + return version +} - // Apply limit after sorting - if limit > 0 && len(deployments) > limit { - deployments = deployments[:limit] +// gitInfoFromRPC converts the deployment-service git shape into the core entity +// shape stored on the record. +func gitInfoFromRPC(gi *deployment_v1alpha.GitInfo) core_v1alpha.GitInfo { + if gi == nil { + return core_v1alpha.GitInfo{} } - return deployments, nil + info := core_v1alpha.GitInfo{ + Sha: gi.Sha(), + Branch: gi.Branch(), + Message: gi.CommitMessage(), + Author: gi.CommitAuthorName(), + IsDirty: gi.IsDirty(), + WorkingTreeHash: gi.WorkingTreeHash(), + CommitAuthorEmail: gi.CommitAuthorEmail(), + Repository: gi.Repository(), + } + if gi.HasCommitTimestamp() && gi.CommitTimestamp() != nil { + info.CommitTimestamp = standard.FromTimestamp(gi.CommitTimestamp()).Format(time.RFC3339) + } + return info } func (d *DeploymentServer) toDeploymentInfo(deployment *core_v1alpha.Deployment) *deployment_v1alpha.DeploymentInfo { @@ -1366,7 +1203,9 @@ func (d *DeploymentServer) toDeploymentInfo(deployment *core_v1alpha.Deployment) info.SetId(string(deployment.ID)) info.SetAppName(deployment.AppName) - info.SetAppVersionId(deployment.AppVersion) + // Normalize legacy placeholder versions to empty so history renders "—" + // rather than "pending-build" or "failed-". + info.SetAppVersionId(normalizeAppVersion(deployment.AppVersion, string(deployment.ID))) info.SetClusterId(deployment.ClusterId) info.SetStatus(deployment.Status) info.SetPhase(deployment.Phase) @@ -1474,55 +1313,11 @@ type deploymentWithEntity struct { entity *entityserver_v1alpha.Entity } -// markPreviousActiveAs marks all active deployments for the given app/cluster with the target status, -// except for the specified currentDeploymentId -func (d *DeploymentServer) markPreviousActiveAs(ctx context.Context, appName, clusterId, currentDeploymentId, targetStatus string) error { - // List all active deployments for this app - deployments, err := d.listDeploymentsInternal(ctx, appName, "active", 100) - if err != nil { - return err - } - - // Update each active deployment (except the current one) to the target status - for _, dwe := range deployments { - dep := dwe.deployment - if string(dep.ID) == currentDeploymentId { - continue - } - - dep.Status = targetStatus - if dep.CompletedAt == "" { - dep.CompletedAt = time.Now().Format(time.RFC3339) - } - - // Update entity - updateAttrs := dep.Encode() - updateEntity := &entityserver_v1alpha.Entity{} - updateEntity.SetId(string(dep.ID)) - updateEntity.SetAttrs(updateAttrs) - - // Get current entity to get revision - currentResp, err := d.EAC.Get(ctx, string(dep.ID)) - if err != nil { - d.Log.Error("Failed to get deployment for update", "deployment_id", dep.ID, "error", err) - continue - } - updateEntity.SetRevision(currentResp.Entity().Revision()) - - _, err = d.EAC.Put(ctx, updateEntity) - if err != nil { - d.Log.Error("Failed to mark deployment", "deployment_id", dep.ID, "target_status", targetStatus, "error", err) - continue - } - - d.Log.Info("Marked previous active deployment", - "deployment_id", dep.ID, - "target_status", targetStatus, - "app", appName, - "cluster", clusterId) - } - - return nil +// markPreviousActiveAs settles the deployments that were active for this app, +// leaving currentDeploymentId alone. Backed by the shared store's status-indexed +// implementation. +func (d *DeploymentServer) markPreviousActiveAs(ctx context.Context, appName, currentDeploymentId, targetStatus string) error { + return d.tracker.Store().MarkPreviousActiveAs(ctx, appName, currentDeploymentId, deploylifecycle.Status(targetStatus)) } // decodeEntity is a helper to decode RPC entity to struct diff --git a/servers/deployment/server_test.go b/servers/deployment/server_test.go index 9e2ea53f..5d2ecb02 100644 --- a/servers/deployment/server_test.go +++ b/servers/deployment/server_test.go @@ -1118,20 +1118,16 @@ func TestDeployVersion(t *testing.T) { t.Fatalf("Failed to create app version: %v", err) } - // Create an in-progress deployment (blocking) - _, err = inmem.Client.Create(ctx, "blocking-dep", &core_v1alpha.Deployment{ - AppName: "locked-app", - ClusterId: "cluster1", - AppVersion: "pending-build", - Status: "in_progress", - Phase: "building", - DeployedBy: core_v1alpha.DeployedBy{ - Timestamp: time.Now().Format(time.RFC3339), - }, - }) + // Establish a blocking in-progress deployment that actually holds the + // deploy lock, via the same path a real deploy uses. A bare in_progress + // record no longer blocks — the lock does. + blockRes, err := client.CreateDeployment(ctx, "locked-app", "cluster1", "pending-build", nil) if err != nil { t.Fatalf("Failed to create blocking deployment: %v", err) } + if blockRes.HasError() && blockRes.Error() != "" { + t.Fatalf("Blocking deployment unexpectedly failed: %s", blockRes.Error()) + } // Try to deploy — should be blocked result, err := client.DeployVersion(ctx, "locked-app", "cluster1", "locked-app-v1", false, nil, "", "") diff --git a/testdata/build-error/.miren/app.toml b/testdata/build-error/.miren/app.toml new file mode 100644 index 00000000..b21a6f6d --- /dev/null +++ b/testdata/build-error/.miren/app.toml @@ -0,0 +1 @@ +name = "build-error" diff --git a/testdata/build-error/Procfile b/testdata/build-error/Procfile new file mode 100644 index 00000000..fbb888e1 --- /dev/null +++ b/testdata/build-error/Procfile @@ -0,0 +1 @@ +web: /bin/app diff --git a/testdata/build-error/go.mod b/testdata/build-error/go.mod new file mode 100644 index 00000000..c4ed19fe --- /dev/null +++ b/testdata/build-error/go.mod @@ -0,0 +1,3 @@ +module build-error + +go 1.25 diff --git a/testdata/build-error/main.go b/testdata/build-error/main.go new file mode 100644 index 00000000..735f0066 --- /dev/null +++ b/testdata/build-error/main.go @@ -0,0 +1,8 @@ +package main + +// This file intentionally does not compile: the blackbox suite uses it to +// exercise a build that fails during the buildkit stage, so the server settles +// the deployment record as failed with the compiler error as its logs. +func main() { + this is not valid go +} diff --git a/testdata/slow-build/.miren/app.toml b/testdata/slow-build/.miren/app.toml new file mode 100644 index 00000000..c64abe0f --- /dev/null +++ b/testdata/slow-build/.miren/app.toml @@ -0,0 +1 @@ +name = "slow-build" diff --git a/testdata/slow-build/Dockerfile.miren b/testdata/slow-build/Dockerfile.miren new file mode 100644 index 00000000..d9908632 --- /dev/null +++ b/testdata/slow-build/Dockerfile.miren @@ -0,0 +1,9 @@ +FROM alpine:3.20 +# A deliberately slow build so a deploy holds the deployment lock long enough +# for the blackbox suite to race a second deploy against it. On a cold cache +# (CI, first run) this sleeps; the lock is what the test is actually asserting. +RUN sleep 25 +COPY serve.sh /serve.sh +RUN chmod +x /serve.sh +EXPOSE 3000 +ENTRYPOINT ["/serve.sh"] diff --git a/testdata/slow-build/serve.sh b/testdata/slow-build/serve.sh new file mode 100755 index 00000000..4b752c03 --- /dev/null +++ b/testdata/slow-build/serve.sh @@ -0,0 +1,4 @@ +#!/bin/sh +while true; do + printf "HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok" | nc -l -p 3000 +done From 0261da6dfb6346d60ba9eb35f59494878b0d476c Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Fri, 24 Jul 2026 17:48:28 -0700 Subject: [PATCH 3/3] Address review feedback - 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. --- cli/commands/deploy.go | 48 ++++++++++----------- pkg/deploylifecycle/lock.go | 14 +++--- servers/build/build_saga.go | 9 +++- servers/build/deploy_tracking.go | 7 +++ servers/build/deploy_tracking_test.go | 16 +++++++ servers/deployment/lock_integration_test.go | 30 ++++++++++--- servers/deployment/server.go | 34 +++++++-------- 7 files changed, 100 insertions(+), 58 deletions(-) diff --git a/cli/commands/deploy.go b/cli/commands/deploy.go index a8c06936..b4f67d7f 100644 --- a/cli/commands/deploy.go +++ b/cli/commands/deploy.go @@ -822,12 +822,10 @@ func Deploy(ctx *Context, opts struct { return err } - // A server-owned build can fail because another deploy took the lock - // after our advisory pre-flight passed; render the rich blocked - // message rather than a generic build error. - if maybeReportBlocked(ctx, depClient, serverOwnsDeployment, name) { - return err - } + // If the build never got its own record because another deploy held + // the lock, add the rich blocked banner — but still show any build + // errors below rather than instead of them. + maybeReportBlocked(ctx, depClient, serverOwnsDeployment, getDeploymentID(), name) ctx.Printf("\n\nBuild failed with the following errors:\n") errsSnap, _, _ := snapshotBuildState() @@ -928,11 +926,9 @@ func Deploy(ctx *Context, opts struct { return err } - // See the explain-mode branch: a server-owned build can lose the lock - // race after the pre-flight; show the blocked message if so. - if maybeReportBlocked(ctx, depClient, serverOwnsDeployment, name) { - return err - } + // See the explain-mode branch: only add the blocked banner when we + // never got our own record, and still print build errors below. + maybeReportBlocked(ctx, depClient, serverOwnsDeployment, getDeploymentID(), name) ctx.Printf("\n\nBuild failed.\n") errsSnap, logsSnap, _ := snapshotBuildState() @@ -1187,27 +1183,27 @@ func (s *safeStatusCh) Close() { close(s.ch) } -// maybeReportBlocked renders the rich "deployment blocked" message when a -// server-owned build failed because another deployment holds the lock — the -// pre-flight-passed-then-lost-the-race case, where the build returns a lock -// conflict instead of a build error. It reuses GetDeployLock rather than trying -// to parse lock info out of the build error. Returns true if it handled the -// failure as a block, so the caller can skip the generic "build failed" output. +// maybeReportBlocked prints the rich "deployment blocked" banner when a +// server-owned build failed because it never got its own record — another +// deployment held the lock the whole time (the pre-flight-passed-then-lost-the- +// race case, where the build returns a lock conflict instead of a build error). // -// This is a best-effort heuristic: a server-owned build that fails for any other -// reason releases its own lock, so a Held result here means a *different* deploy -// is in the way — which is exactly when the blocked message is the right thing -// to show. -func maybeReportBlocked(ctx *Context, depClient *deployment_v1alpha.DeploymentClient, serverOwnsDeployment bool, name string) bool { - if !serverOwnsDeployment { - return false +// It is deliberately gated on ownDeploymentID being empty. Once our build has a +// record, the failure is ours: the server releases our lock on failure, so a +// held lock afterward just means an unrelated deploy started in that window, and +// reporting "blocked" would wrongly hide the compiler/build errors that actually +// failed this deploy. It never suppresses those errors either way — the caller +// prints them regardless; this only adds context when the real cause was +// contention. +func maybeReportBlocked(ctx *Context, depClient *deployment_v1alpha.DeploymentClient, serverOwnsDeployment bool, ownDeploymentID, name string) { + if !serverOwnsDeployment || ownDeploymentID != "" { + return } lockRes, err := depClient.GetDeployLock(ctx, name, ctx.ClusterName) if err != nil || !lockRes.Held() || !lockRes.HasLockInfo() || lockRes.LockInfo() == nil { - return false + return } printDeploymentBlocked(ctx, lockRes.LockInfo()) - return true } // printDeploymentBlocked renders the rich "deployment blocked" message shared by diff --git a/pkg/deploylifecycle/lock.go b/pkg/deploylifecycle/lock.go index 02addc89..75be7479 100644 --- a/pkg/deploylifecycle/lock.go +++ b/pkg/deploylifecycle/lock.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "log/slog" - "strings" "time" "miren.dev/runtime/api/core/core_v1alpha" @@ -169,14 +168,13 @@ func (l *Locks) WithTTL(d time.Duration) *Locks { // LockID is the deterministic entity id for an app's deploy lock. Determinism is // what makes create-if-absent a mutual exclusion primitive: every contender // computes the same key. +// +// The app name is the sole variable component, so it is used raw. It must not be +// rewritten (e.g. slashes to underscores): that would let two distinct names +// collide on one lock. As the last path segment the name is unambiguous even +// when it contains a slash. func LockID(appName string) entity.Id { - return entity.Id(fmt.Sprintf("deploy-lock/%s", sanitizeKeyPart(appName))) -} - -// sanitizeKeyPart keeps a slash in a name from splitting the key into a -// different shape. -func sanitizeKeyPart(s string) string { - return strings.ReplaceAll(s, "/", "_") + return entity.Id("deploy-lock/" + appName) } // Acquire takes the deploy lock for deploymentID, returning the holder it diff --git a/servers/build/build_saga.go b/servers/build/build_saga.go index 55a0ad8f..2a42149f 100644 --- a/servers/build/build_saga.go +++ b/servers/build/build_saga.go @@ -662,10 +662,17 @@ func undoBeginDeployment(ctx context.Context, in beginDeploymentIn, out beginDep return nil } deps := saga.Get[*buildSagaDeps](ctx) + + // Settle on a detached context: compensation often runs because the client + // disconnected, and that must not leave the record in_progress with the lock + // held until its TTL. + settleCtx, cancel := settleContext(ctx) + defer cancel() + // FailIfUnsettled: if a later action already activated the record, leave it // active — the version is live, and undoSetActiveVersion handles reverting // that separately. - if err := deps.builder.deploy.FailIfUnsettled(ctx, out.DeploymentID, "build rolled back", ""); err != nil { + if err := deps.builder.deploy.FailIfUnsettled(settleCtx, out.DeploymentID, "build rolled back", ""); err != nil { return fmt.Errorf("failing rolled-back deployment %s: %w", out.DeploymentID, err) } return nil diff --git a/servers/build/deploy_tracking.go b/servers/build/deploy_tracking.go index cca3170e..bcb4124d 100644 --- a/servers/build/deploy_tracking.go +++ b/servers/build/deploy_tracking.go @@ -46,6 +46,13 @@ func (b *Builder) beginDeploy( } clusterID := req.ClusterId() + // An empty cluster_id means "not tracked", matching the saga path's + // begin-deployment action (which skips on the same condition). Without this + // the two paths would disagree: the plain path would create a lock-holding + // record for an empty cluster_id while the saga path created none. + if clusterID == "" { + return nil, nil + } rec, err := b.deploy.Begin(ctx, deploylifecycle.BeginParams{ AppName: appName, diff --git a/servers/build/deploy_tracking_test.go b/servers/build/deploy_tracking_test.go index 0bf7a1cd..7ad00c00 100644 --- a/servers/build/deploy_tracking_test.go +++ b/servers/build/deploy_tracking_test.go @@ -53,6 +53,22 @@ func TestBeginDeployWithoutRequestTracksNothing(t *testing.T) { assert.Empty(t, all, "the server must not have created a record") } +// A DeployRequest with an empty cluster_id means "not tracked", so the plain +// path agrees with the saga path (whose begin-deployment action skips on the +// same condition) instead of creating a lock-holding record. +func TestBeginDeploySkipsEmptyClusterID(t *testing.T) { + ctx := context.Background() + b := newDeployTestBuilder(t) + + dt, err := b.beginDeploy(ctx, "web", deployRequest(""), nil, nil) + require.NoError(t, err) + assert.Nil(t, dt, "an empty cluster_id must not create a server-owned record") + + all, err := b.deploy.Store().List(ctx, deploylifecycle.Query{AppName: "web"}) + require.NoError(t, err) + assert.Empty(t, all) +} + // Ephemeral builds have no deployment record by design, even when a request is // present. func TestBeginDeploySkipsEphemeral(t *testing.T) { diff --git a/servers/deployment/lock_integration_test.go b/servers/deployment/lock_integration_test.go index bee21786..3a374740 100644 --- a/servers/deployment/lock_integration_test.go +++ b/servers/deployment/lock_integration_test.go @@ -155,11 +155,10 @@ func TestGetDeployLockValidatesArgs(t *testing.T) { require.Error(t, err) } -// DeployVersion must not strand the lock if its activation settle fails: a -// second deploy started while the first's record holds the lock must be blocked, -// but once the first settles the lock frees. This exercises the backstop path by -// racing the deprecated CreateDeployment lock against a rollback deploy. -func TestDeployVersionActivationDoesNotStrandLock(t *testing.T) { +// A completed DeployVersion must release the deploy lock it took, so the app is +// not left blocked. Runs two DeployVersions in sequence and checks the lock is +// free between them and that the second is not blocked. +func TestDeployVersionReleasesLockWhenDone(t *testing.T) { ctx := context.Background() client, inmem := newLockTestClient(t) @@ -184,6 +183,27 @@ func TestDeployVersionActivationDoesNotStrandLock(t *testing.T) { assert.False(t, res2.HasError() && res2.Error() != "", "the next deploy must not be blocked") } +// A DeployVersion whose activation fails must still release the lock, so the +// failure does not block the app's next deploy. SetActiveVersion is made to fail +// by pointing at a version whose app entity does not exist. +func TestDeployVersionFailureReleasesLock(t *testing.T) { + ctx := context.Background() + client, inmem := newLockTestClient(t) + + // A version with no corresponding app entity: activation will fail. + _, err := inmem.Client.Create(ctx, "ghost-v1", &core_v1alpha.AppVersion{Version: "ghost-v1"}) + require.NoError(t, err) + + res, err := client.DeployVersion(ctx, "ghost", "prod", "ghost-v1", false, nil, "", "") + require.NoError(t, err) + require.True(t, res.HasError() && res.Error() != "", "deploy should have failed to activate") + + // The failure must not have stranded the lock. + lock, err := client.GetDeployLock(ctx, "ghost", "prod") + require.NoError(t, err) + assert.False(t, lock.Held(), "a failed DeployVersion must release the lock") +} + // The "pending-build" placeholder an older client writes must render as empty // in history, not as the sentinel. func TestPendingBuildSentinelNormalizedInHistory(t *testing.T) { diff --git a/servers/deployment/server.go b/servers/deployment/server.go index 67917364..3744f3f1 100644 --- a/servers/deployment/server.go +++ b/servers/deployment/server.go @@ -276,16 +276,10 @@ func (d *DeploymentServer) UpdateDeploymentPhase(ctx context.Context, req *deplo deploymentId := args.DeploymentId() newPhase := args.Phase() - // Validate phase value - validPhases := map[string]bool{ - "preparing": true, - "building": true, - "pushing": true, - "activating": true, - } - if !validPhases[newPhase] { - return cond.ValidationFailure("invalid-phase", - "phase must be one of: preparing, building, pushing, activating") + // Validate against the single source of truth so the accepted set and the + // message can never drift from lifecycle.go's phase list. + if _, err := deploylifecycle.ParsePhase(newPhase); err != nil { + return err } // Get existing deployment @@ -866,6 +860,13 @@ func (d *DeploymentServer) DeployVersion(ctx context.Context, req *deployment_v1 deployment := rec.Deployment newDeploymentId := string(deployment.ID) + // Every terminal settle below runs on a detached context: once we start + // settling, a client that cancels the RPC must not strand the record + // in_progress with the lock held until its TTL. The most likely moment for + // that cancellation is right here, in the failure path. + settleCtx, cancelSettle := context.WithTimeout(context.WithoutCancel(ctx), 15*time.Second) + defer cancelSettle() + // This path has no build; it goes straight to activation. if phaseErr := d.tracker.SetPhase(ctx, newDeploymentId, deploylifecycle.PhaseActivating); phaseErr != nil { d.Log.Error("Failed to set activating phase", "error", phaseErr) @@ -875,8 +876,9 @@ func (d *DeploymentServer) DeployVersion(ctx context.Context, req *deployment_v1 d.Log.Error("Failed to set active version", "error", err, "app", appName, "version_id", string(appVersion.ID)) failMsg := fmt.Sprintf("failed to activate version: %v", err) - if failErr := d.tracker.Fail(ctx, newDeploymentId, failMsg, ""); failErr != nil { - d.Log.Error("Failed to mark deployment as failed", "error", failErr) + if failErr := d.tracker.Fail(settleCtx, newDeploymentId, failMsg, ""); failErr != nil { + d.Log.Error("Failed to mark deployment as failed; releasing lock", "error", failErr) + d.releaseDeployLock(settleCtx, appName, newDeploymentId) } results.SetError(failMsg) @@ -887,12 +889,8 @@ func (d *DeploymentServer) DeployVersion(ctx context.Context, req *deployment_v1 if isRollback { activate = d.tracker.ActivateRollback } - // SetActiveVersion already made the version live, so settle on a detached - // context: a client that cancels the RPC now must not strand the record - // in_progress with the lock held. If the settle still fails, release the - // lock directly so later deploys aren't blocked for the full lock TTL. - settleCtx, cancelSettle := context.WithTimeout(context.WithoutCancel(ctx), 15*time.Second) - defer cancelSettle() + // SetActiveVersion already made the version live. If the settle still fails, + // release the lock directly so later deploys aren't blocked for the full TTL. if err := activate(settleCtx, newDeploymentId); err != nil { d.Log.Error("Failed to activate deployment; releasing lock", "error", err) d.releaseDeployLock(settleCtx, appName, newDeploymentId)