Since my last review at d6000a1f, one more round of fixes landed at the current head (aee819f). I walked all my prior threads (all resolved) and read the new code directly.
createVersion in build_saga.go now treats malformed Git provenance as non-fatal. Lines 472–479 wrap the json.Unmarshal in a warn-and-continue path, matching the existing behavior in beginDeployment. A process that supplies malformed deploy_git_info_json no longer fails the entire build after the image is already complete — it just logs a warning and proceeds with no Source on the AppVersion.
TestBuildSaga_MalformedGitInfoDoesNotBlockDeployment verifies the fix end-to-end. The test seeds bad JSON ("{"), runs the full saga, and asserts that the app has an active version and that version.Source is empty. This is exactly the right coverage.
TestDeployVersionMissingAppDoesNotCreateLock (formerly TestDeployVersionFailureReleasesLock) is now accurate. The test was updated alongside the earlier code changes — name, comment, and assertion all agree that the app-lookup preflight rejects before Begin, so no lock is acquired. The newLockTestClient fixture also pre-creates a real AppVersion entity so TestUpdateStatusReleasesLock has something concrete to activate against.
TestUpdateFailedDeploymentReleasesLock gained assertion on error_message. It now also verifies that the deprecated build_logs field is absent from the response, which correctly documents that embedded log blobs are no longer stored in the entity.
deployTracker()'s sync.Once (lines 873–880) — the race fix from the previous round is intact.SetConfiguration's ErrNotFound early-return inside the loop fires with settled = false, so the deferred FailIfUnsettled correctly cleans up the lock. ✓PreviousVersionID is gone — search confirms no trace.panic in server.go is gone — StatusInterrupted case returns a proper cond.ValidationFailure instead (lines 207–209 and 235–236).The PR is architecturally coherent and the test coverage for the new code paths is solid. Ready for human review.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1094,
"verdict": "ready",
"event": "comment",
"summary": "## Re-review: still ready for human review\n\nSince my last review at `d6000a1f`, one more round of fixes landed at the current head (`aee819f`). I walked all my prior threads (all resolved) and read the new code directly.\n\n### What changed since my last review\n\n**`createVersion` in `build_saga.go` now treats malformed Git provenance as non-fatal.** Lines 472–479 wrap the `json.Unmarshal` in a warn-and-continue path, matching the existing behavior in `beginDeployment`. A process that supplies malformed `deploy_git_info_json` no longer fails the entire build after the image is already complete — it just logs a warning and proceeds with no `Source` on the `AppVersion`.\n\n**`TestBuildSaga_MalformedGitInfoDoesNotBlockDeployment` verifies the fix end-to-end.** The test seeds bad JSON (`\"{\"`), runs the full saga, and asserts that the app has an active version and that `version.Source` is empty. This is exactly the right coverage.\n\n**`TestDeployVersionMissingAppDoesNotCreateLock` (formerly `TestDeployVersionFailureReleasesLock`) is now accurate.** The test was updated alongside the earlier code changes — name, comment, and assertion all agree that the app-lookup preflight rejects before `Begin`, so no lock is acquired. The `newLockTestClient` fixture also pre-creates a real `AppVersion` entity so `TestUpdateStatusReleasesLock` has something concrete to activate against.\n\n**`TestUpdateFailedDeploymentReleasesLock` gained assertion on `error_message`.** It now also verifies that the deprecated `build_logs` field is absent from the response, which correctly documents that embedded log blobs are no longer stored in the entity.\n\n### Things I verified that still look clean\n\n- `deployTracker()`'s `sync.Once` (lines 873–880) — the race fix from the previous round is intact.\n- `SetConfiguration`'s `ErrNotFound` early-return inside the loop fires with `settled = false`, so the deferred `FailIfUnsettled` correctly cleans up the lock. ✓\n- `PreviousVersionID` is gone — search confirms no trace.\n- The `panic` in `server.go` is gone — `StatusInterrupted` case returns a proper `cond.ValidationFailure` instead (lines 207–209 and 235–236).\n\nThe PR is architecturally coherent and the test coverage for the new code paths is solid. Ready for human review.",
"posted_to_pr": true,
"draft": true
}The current head (8c8f869) adds a meaningful round of fixes since my last review. I walked all my prior threads (all resolved), checked the CodeRabbit findings from the most recent automated pass, and read the new code directly.
deployTracker race fixed (servers/app/app.go lines 52–54, 873–879). The lazy initializer is now guarded by sync.Once. AppInfo gained a deployOnce sync.Once field, and NewAppInfo eagerly sets Deploy so the Once is a no-op in the normal path. The path is safe to call concurrently. This was the concrete data-race CodeRabbit flagged; it's correctly fixed.
SetConfiguration now participates in tracking. It calls Begin, defers FailIfUnsettled on any non-settled exit (including the ErrNotFound-early-return path, which correctly fires the deferred Fail and releases the lock), and completes activation through SetAppVersion + ActivateAtRevision. The OCC retry loop continues to guard against concurrent config writers. The lock-then-OCC-retry model is coherent and the comment explains it.
Consensus loop skips empty sources (controllers/deploymentattempts/controller.go lines 267–268). A deployment without Git metadata now just gets a continue rather than locking consensus to an empty value. The guard at line 277 (if !haveConsensus || consensus.Empty()) provides belt-and-braces.
TestDeployVersionMissingAppDoesNotCreateLock accurately tests what it claims. The test name, comment, and assertion all match: the app-lookup preflight now rejects before Begin, so no lock is acquired. The previous test was testing the wrong failure point with the wrong assertion; this one tests the right thing.
The panic in server.go is gone. I confirmed via search — it was removed in an earlier round and there's no trace of it in the current head.
deployTracker()'s sync.Once correctly handles the struct-literal case (where Deploy starts nil) and the NewAppInfo case (where Deploy is pre-set). The Once fires once per AppInfo, whichever path is first.SetConfiguration's loop (line 354) correctly triggers FailIfUnsettled via the deferred closure, releasing the lock. No stranding.settleReconciledSuccess now calls MarkPreviousActiveAs using the post-write settled record, and the supersede status respects the rollback operation path. Clean.compat.go's statusFromSchema maps unknown non-empty outcomes to StatusInterrupted rather than "". A record from a newer runtime can't become an inert lock holder.Begin's Subject/AuthMethod assignment is conditional — only overwrites when the parameter is non-empty.source.go's @-stripping scopes the search to the host component (before : or /), not the whole string.The remaining pre-existing notes (ocireg/registry.go BaseContext lifetime and boot_runner.go sandbox sweep deadline) are outside this PR's diff and not regressions from it. The PR is ready for human review.
Verdict: ready
The latest commit (d3b506e2) addresses the one remaining open CodeRabbit finding I noted last time — malformed deploy_git_info_json aborting a deploy after the image build had already finished.
createVersion git-info handling (servers/build/build_saga.go lines 472–479). Malformed JSON now logs a warning and continues with an empty Source, matching the behavior of beginDeployment. The new test TestBuildSaga_MalformedGitInfoDoesNotBlockDeployment seeds deploy_git_info_json: "{" and asserts the saga completes, the app gets an active version, and version.Source is empty. That's exactly what I'd want to see covering this path.
beginDeploy now tracks all non-ephemeral builds (servers/build/deploy_tracking.go). The old if req == nil early-return is gone — a nil DeployRequest no longer suppresses tracking. Tests were renamed and updated to match the new semantics (TestBeginDeployWithoutRequestStillTracksAttempt, TestBeginDeployAllowsEmptyLegacyClusterID). This is consistent with the PR's goal of making server-owned tracking the default.
activate returns error instead of being void. The test TestActivateKeepsLockWhenActivationDidNotCommit now asserts require.Error(t, rec.activate(ctx)) and checks the lock stays held — confirming the no-release behavior is intentional and tested. TestActivateSurvivesCancelledContext asserts StatusSucceeded post-activation, consistent with the new activation path through CommitActivation.
PreviousVersionID removed. Not present anywhere in the codebase — confirmed.
beginDeployment / activateDeployment actions (the harness now registers them), so it exercises the deployment-record path too, not just the version creation.FailIfUnsettled lost its second string argument; the call site in failOnError updated accordingly.The remaining open notes (components/ocireg/registry.go BaseContext context lifetime and boot_runner.go sandbox sweep deadline) remain outside this PR's diff and are pre-existing — not regressions. This is ready for human review.
Verdict: ready
Since my last review (d6000a1f), this PR has been revised again at 71214e0e. I went through the new changes carefully and checked all previously open concerns.
sync.Once data race fix (servers/app/app.go). deployTracker() now uses deployOnce sync.Once to guard the lazy r.Deploy initialization, closing the race CodeRabbit caught. NewAppInfo pre-populates Deploy so the Once only fires for struct-literal callers.
Empty-source consensus skip (controllers/deploymentattempts/controller.go line 267). The migrateVersion consensus loop now skips deployments where SourceFromGitInfo returns an empty source, so an API-driven or rollback deployment with no git metadata can no longer poison the consensus and block version migration.
panic removed from UpdateDeploymentStatus (servers/deployment/server.go). The StatusInterrupted arm now returns cond.ValidationFailure rather than panicking the RPC handler.
VersionActivator abstraction (api/app/envvar.go). SetEnvVarsWithActivator / DeleteEnvVarsWithActivator thread a VersionActivator callback through createNewVersion, so the AppInfo env-var paths (setEnvVars, DeleteEnvVar) supply a lifecycle-aware activator that records the version on the attempt and uses ActivateAtRevision — while callers that don't provide an activator default to the old direct patch, preserving the downgrade window.
SetConfiguration acquires a deployment lock (servers/app/app.go line 325). Begin is now called before the OCC loop with OperationConfigChange. The settled flag drives a deferred FailIfUnsettled so the attempt is cleaned up on every failure path — including the early "app not found" return inside the loop, which sets settled = false and lets the defer fire correctly.
SetConfiguration app-not-found early return at line 354 leaves settled = false, so the defer fires FailIfUnsettled, marks the attempt failed, and releases the lock. Correct.FailIfUnsettled swallows ErrConflict so deferred calls on already-settled records (successful activation) are silent no-ops. Correct.StatusInterrupted guard at line 207-209 (before the switch) and the one inside the switch (line 235-236) are redundant but harmless — both return the same validation error.settleReconciledSuccess uses the post-update settled record for both MarkPreviousActiveAs and release. Correct.The remaining open CodeRabbit notes (components/ocireg/registry.go BaseContext and components/server/boot_runner.go sandbox sweep) are in files outside this PR's diff and are pre-existing concerns, not regressions.
The architecture is coherent and the implementation is self-consistent. This is ready for human review.
Verdict: ready
Since my last review, the PR went through another significant revision. The mechanism for the deployment lock changed substantially: the lock is now stored on the App entity (app.deployment_lock) rather than as a standalone deploy-lock/<app> entity. That's a meaningful architectural improvement — it collapses the app pointer CAS and the lock write into a single revision-guarded entity, so CommitActivation can atomically prove lock ownership while swinging the serving pointers. I went through this carefully.
Lock model redesigned. lock.go no longer creates a standalone lock entity. acquireApp / releaseApp do revision-guarded patches on the App entity; CommitActivation and CommitActivationAtRevision re-use that same entity for the serving-pointer swing in one atomic revision guard. The old LockID function is gone.
Backward compatibility is explicit and layered. legacy_lock.go retains the old deploy-lock/<app> standalone entity as a compatibility shadow. Acquire takes the legacy lock first (closing the cross-version admission race that an older binary might exploit), then takes the canonical app lock. Release clears the canonical lock first (so newer binaries stop being blocked), then releases the legacy shadow. Blocking() checks the legacy lock first so a lock held by an older runtime is visible. The ordering is correct in both directions.
activate reordering. The activation path now: reads the record → calls CommitActivation (which is the CAS that proves lock ownership while swinging pointers) → then writes the terminal record via t.update. If the terminal write fails after CommitActivation succeeds, the code logs and returns nil (line 349), deliberately deferring to reconciliation rather than reporting failure. The reconciler detects the committed app pointers and calls settleReconciledSuccess. I verified settled is only referenced after the nil-error path, so there's no nil dereference risk.
settleReconciledSuccess now includes MarkPreviousActiveAs. CodeRabbit's concern is addressed — reconciled success properly settles the previously-active legacy record.
TestDeployVersionFailureReleasesLock renamed and corrected. The test is now TestDeployVersionMissingAppDoesNotCreateLock, which accurately describes what it tests and which assertion is made. The lock assertion is no longer vacuous — it now tests the right thing.
stealable's not-found treatment (returns false, lease is recovery bound) is consistent with the admission reservation mechanism.acquireApp's unclaimed-app path: current.Revision from readApp guards the patch on app.ID. ✓releaseApp retries on conflict because app revisions move for unrelated changes; the loop correctly re-reads and distinguishes "app revision moved" from "successor took the lock". ✓compat.go's statusFromSchema now maps unknown outcomes to StatusInterrupted (not empty), so a record written by a newer runtime can't become an inert non-terminal lock holder in an older one. ✓controller.go migration loop now logs and continues per-record failures rather than bailing early, advancing the cursor after each page. ✓boundedFailureSummary truncates at a valid UTF-8 boundary. ✓The architecture is coherent, the dual-lock compatibility story is clearly thought through, and the crash-window recovery path via reconciliation is self-consistent. No blockers.
Verdict: ready
This is still a draft, so I'm judging readiness for human review. It's ready.
The author (via the AI code agent) has addressed all three of my substantive inline concerns, and several of the CodeRabbit suggestions from the same round landed too.
settleReconciledSuccess / stale rec (my item 1): Fully resolved. The function was rewritten around an update closure that captures the post-write record into settled, and t.release(ctx, settled) uses that — no more ambiguity about which snapshot is being released.
StatusInProgress no-op (my item 3): Fully resolved. Lines 221–223 in server.go now carry an explicit comment: "Older clients reassert running after creation. Lifecycle progress and lock renewal are server-owned now, so a valid self-transition remains an intentionally idempotent no-op during the compatibility window." That's exactly what I asked for.
file:// sanitization (my item 4): Fully resolved. source.go lines 47–52 now have an explicit guard with a comment explaining why, rather than relying incidentally on the empty-host branch.
scp-style @ scope (CodeRabbit): Also landed — sanitizeRepository now searches @ only within the host component (before the first : or /), fixing the path-fragment truncation risk.
My original concern was that a lock acquired before the deployment record was published would be held for the full 30-minute TTL with no reconciliation path. The mechanism is now significantly different: ReserveAdmission creates an anonymous placeholder at the deployment ID (with _admitting status) before lock acquisition, so an older runtime looking for a "missing record" finds a record — it just has an internal status. This narrows the gap considerably. The stealable function in lock.go at lines 273–276 also now has a comment explaining that absence is not proof the holder is dead and the lease is the recovery bound. That's enough — I'm satisfied this is documented and the implementation is self-consistent. I'm closing this thread.
The architecture is coherent: the acquire-before-publish ordering is backed by a reservation placeholder that keeps older runtimes honest, CommitActivation guards the serving-pointer swing with the same app revision that proves lock ownership, and settleReconciledSuccess closes the post-activation crash window without needing the old ReleaseLock backstop. The migration controller's phased approach is safe to restart at any point. The test coverage I can see covers the key paths. This is ready for human review.
Posted to the PR's comment threads when you submit.
settleReconciledSuccess now captures the post-write record into settled inside the update closure, and calls t.release(ctx, settled) — no stale snapshot in sight. This is clean. Closing.ReserveAdmission now creates a placeholder at the deployment ID before lock acquisition, so an older runtime's stealable check finds a record (with _admitting status) rather than a missing one — which materially narrows the orphan window. The stealable function at lines 273–276 also now has a comment explaining that absence is not positive evidence of death and the lease is the recovery bound. I'm satisfied this is adequately documented and the implementation is self-consistent. Closing.Verdict: ready
This is a draft, so I'm judging readiness for human review. It's close — the design is coherent and mostly well-executed — but there are a few concrete issues I'd want addressed or consciously accepted before it graduates.
boundedFailureSummary truncating at a valid UTF-8 boundary rather than a raw byte offset is the right call, and it's tested. The sanitizeRepository stripping credentials before they become durable in Source is exactly the kind of defensive move that prevents a security regret later. The settleReconciledSuccess + interrupt split in Reconcile neatly closes the activation crash window without the old ReleaseLock backstop that left records in an indeterminate state. And the sequential migration phases with idempotent canonical checks mean the backfill can be restarted safely at any point.
1. settleReconciledSuccess releases the lock on the stale pre-update snapshot (rec)
In tracker.go, after settleReconciledSuccess calls t.update(...) to write the terminal outcome, it calls t.release(ctx, rec) using the original rec argument — the one read before the update loop. That's the AppName the release needs, so it does release the right app's lock, but it reads as confusing and fragile: if rec.Deployment.AppName or ID were ever derived from the updated record, this would silently release the wrong thing. A small clarification here would help (use a named variable for the settled record, or document the reliance on immutable fields).
2. SetConfiguration in app.go — lock held across the OperationConfigChange begin, but the OCC retry loop creates new versions on each iteration while the lock is held
The tracker's Begin acquires the lock before the loop, and then each loop iteration creates a fresh AppVersion + ConfigVersion pair before calling SetAppVersion/Activate. When Activate returns ErrConflict, those entities are deleted (best-effort), and the loop continues — still holding the lock from the first Begin. This is likely intentional (config changes shouldn't race with a concurrent build deploy), but the lock comment says OperationConfigChange doesn't create the app if missing; if the app lookup inside Begin returns ErrNotFound at the top but SetConfiguration expects to proceed, there is a subtle gap. More concretely: OperationConfigChange resolves app only for the app.ActiveDeployment != "" auto-source path and for the AppID == "" case — but the Begin result's App field is then unused by SetConfiguration (which re-reads the app inside the loop via r.EC.EAC().Get). This duplication isn't wrong but it means the lock was acquired against app state that may diverge from what the loop sees. Worth a comment explaining that the lock serves as the serialization mechanism while the read-merge-write retries handle the OCC.
3. stealable changed semantics for missing deployment records
The old code in lock.go treated a missing deployment record as stealable ("holding deployment no longer exists"). The new code explicitly treats it as not stealable, with the comment "Begin deliberately acquires before it creates the record." This is correct given that lock acquisition now happens before store creation in Begin, but it means an orphaned lock from a process that acquired the lock and then crashed before creating the record will hold for the full DefaultLockTTL (30 min). The reconciler's interrupt path won't fire either (it looks up the record first and returns early on not-found). This is a conscious trade-off that's explained in the comment, but the operational implication — up to 30 min lock hold with no record — should be called out somewhere visible (e.g., an operator runbook note or at least a comment on Begin noting the window is bounded by the TTL).
4. sanitizeRepository returns "" for file:// URLs
source_test.go asserts that file:///home/user/private/repo maps to "". The code achieves this because the u.Host is empty for file:// paths. This is the right behavior security-wise (don't store local paths), but there's no explicit guard — it falls through to the generic check u.Scheme == "" || u.Host == "". An http:// URL with a path but no host would similarly return "". That's probably fine, but a comment noting this intentional sanitization of file-scheme would help reviewers trust the code rather than wonder if it's accidental.
5. Minor: UpdateDeploymentStatus for the StatusInProgress arm calls Transition but doesn't write
In server.go around line 221, the StatusInProgress case calls deploylifecycle.Transition(rec.Status(), deploylifecycle.StatusInProgress) and then falls through to the if err != nil check. If the transition is already in_progress → in_progress (valid), err is nil but nothing is written. The response re-reads and returns the unchanged record. That's a benign no-op, but it's surprising — a client calling UpdateDeploymentStatus(status=in_progress) gets 200 OK with no change. Worth a comment or an explicit early return.
The design and the bulk of the implementation are sound. Item 1 (stale rec in settleReconciledSuccess) and item 3 (the 30-min orphan-lock window) are the ones I'd most want explicitly acknowledged before merge.
settleReconciledSuccess calls t.release(ctx, rec) with the pre-update snapshot. The AppName and ID fields are immutable so this works correctly, but it reads as fragile — a future reader may not know whether rec is the version before or after the update. Consider extracting the lock-release field explicitly, or at minimum add a comment: // rec.Deployment.AppName and ID are immutable; release uses the same identity after the update.Begin and then crashes before creating the deployment record will hold the lock for the full 30-minute TTL, because the missing record is now treated as not stealable. This is correct given the new acquire-before-create ordering, but the operational implication is significant. Please add a comment here (and possibly on Begin itself) noting that the TTL is the recovery bound for this window, and that reconciliation can't help because Reconcile returns early for missing records.StatusInProgress arm validates the transition but doesn't write anything — if in_progress → in_progress is valid, the handler returns 200 OK with no effect. This is a silent no-op for a client that sends status=in_progress on an already-running deployment. At minimum add a comment; ideally return an explicit early response or reject the request.Verdict: caveats