Data

{
  "owner": "mirendev",
  "repo": "runtime",
  "number": 975,
  "verdict": "caveats",
  "event": "comment",
  "summary": "## Review\n\nThis is a significant, well-thought-out change that replaces a racy \"list then create\" deployment concurrency check with a proper compare-and-swap locking scheme. The core design is sound, and the test suite is genuinely thorough — concurrent lock tests, race hooks that exercise the read-write window, `FailIfUnsettled` as a defer-safe API, `init()` as a compile-time state-machine consistency check. These are real engineering choices, not perfunctory coverage.\n\nI have one concrete concern and one design point worth noting.\n\n---\n\n### Two separate `Tracker` instances, same store\n\n`Builder` creates `deploylifecycle.NewTracker(log, eas)` and `DeploymentServer` creates its own `deploylifecycle.NewTracker(log, eac)`. Both talk to the same entity store under the hood, and the struct comment in `DeploymentServer` acknowledges this (\"both acquire the same app-scoped lock\"). This is fine for the lock — the entity-store CAS makes that correct regardless. But `Tracker` exposes its internal `clockFn` (the `now` field) as a package-internal test seam, and the two live instances are independently configured, which is slightly surprising for operators reading the code.\n\nMore concretely: `UpdateDeploymentStatus` in `DeploymentServer` still does its own direct `d.EAC.Put` rather than calling through `d.tracker.update`. That means it bypasses the `Tracker`'s read-modify-write loop and just does a single `Get + Put` at a fixed revision. A race between two concurrent `UpdateDeploymentStatus` calls (or between one of those and a build-side `SetPhase`) will still silently win last-write-wins, same as before. This is the **deprecated** client-driven path, and the comment at `releaseDeployLock` acknowledges it. But it's worth being conscious that only the lock release is threaded through `Tracker`; the record write is not.\n\n### The \"lock but not record\" window in `Begin`\n\nIn `Tracker.Begin`, a record is created first, and then the lock is acquired. If the lock `Acquire` fails, `discard` is called to remove the record. If `discard` itself fails (network hiccup), the record is leaked in `in_progress` with no lock — it will sit there forever as apparent evidence of a stuck deployment. This is noted in the `discard` log message, but the orphaned record will show in `ListDeployments` and `GetDeployLock` (as `stealable` because the holding deployment is gone). The practical impact is low — `stealable` returns true if the record is gone, so the next deployment won't be blocked — but a leaked `in_progress` record in history is noisy. A migration path (e.g., a periodic sweeper, or doing the lock acquire *before* the record create) would close this gap cleanly. The current design is fine to ship, but worth flagging.\n\n### Minor: `UpdateDeploymentStatus` still inline-validates phase list\n\nIn `UpdateDeploymentStatus`, status validation was unified with `deploylifecycle.ParseStatus`, which is good. But `UpdateDeploymentPhase` still uses its own inline `validPhases` map (lines ~280–289) rather than `deploylifecycle.ParsePhase`. This is a pre-existing inconsistency made slightly more visible by the new pattern — not a blocker, but easy to fix.",
  "comments": [
    {
      "path": "servers/deployment/server.go",
      "line": 289,
      "side": "RIGHT",
      "body": "`UpdateDeploymentPhase` validates the phase inline with a local `validPhases` map, while the new `UpdateDeploymentStatus` correctly delegates to `deploylifecycle.ParseStatus`. This creates two sources of truth for valid phases — if a phase is ever added to `allPhases` in `lifecycle.go`, this handler will silently accept the old set only. Consider replacing the inline map with `deploylifecycle.ParsePhase(newPhase)` to match the pattern established one method above.",
      "ai_prompt": "In servers/deployment/server.go around lines 280-289, UpdateDeploymentPhase validates the phase using an inline local map. Replace the local map and the if-check with a call to deploylifecycle.ParsePhase(newPhase), returning its error if non-nil (consistent with how UpdateDeploymentStatus now calls deploylifecycle.ParseStatus). Remove the now-unused validPhases map."
    },
    {
      "path": "pkg/deploylifecycle/tracker.go",
      "line": 122,
      "side": "RIGHT",
      "body": "`discard` logs an error but leaves the record behind on failure. A leaked `in_progress` record with no lock entity looks like a stuck deployment in `ListDeployments`. The practical block is mitigated because `stealable` returns true when the holding deployment is not found, but a dangling `in_progress` record in history is noisy for operators. Worth documenting the consequence here and/or marking the leaked record as `failed` in a follow-up.",
      "ai_prompt": "In pkg/deploylifecycle/tracker.go at the discard function (around line 122), if the Delete call fails, the record is left as in_progress with no lock. Update the error log message to note that this leaks an in_progress record that may appear in history as a stuck deployment, and add a TODO comment describing a follow-up to handle this (e.g., retry with a background goroutine, or mark the record failed before deleting it)."
    }
  ],
  "posted_to_pr": true
}