pal-e-docs

Review: Bug: platform-validation OOMKilled at 64Mi + stale alert rule

review-388-2026-03-26-v2 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review (v2) of board item #388 / Forgejo issue #171. The two scope correction comments address the original review's main concerns (wrong repo, invalid PromQL, alert staleness behavior). However, corrections live only in comments -- the issue body is stale and would mislead an agent reading only the body. Line numbers are wrong. PR ordering dependency is undocumented.

Template Completeness

  • [x] Lineage -- present ("standalone" is valid for unplanned bugs)
  • [x] Repo -- present in body BUT only says pal-e-platform; correction in comment #2 identifies two repos (pal-e-platform + pal-e-deployments)
  • [ ] User Story -- MISSING from body and all comments
  • [x] Context -- present as "What Broke" section
  • [ ] File Targets -- MISSING from body; added in comment #2 but with wrong line numbers
  • [x] Acceptance Criteria -- present in body, updated in comment #2
  • [x] Test Expectations -- added in comment #4
  • [ ] Constraints -- MISSING as consolidated section; partially covered in comment #4 (two PRs needed) but missing tofu plan -lock=false, ArgoCD sync ordering
  • [ ] Checklist -- MISSING from body and all comments
  • [x] Related -- present

File Targets

  • [x] pal-e-deployments/overlays/platform-validation/prod/deployment-patch.yaml lines 22-27 -- VERIFIED: resources block with requests.memory=32Mi, limits.memory=64Mi
  • [ ] pal-e-platform/terraform/main.tf -- comment #2 says "lines 230-242" but OOMKilled rule is actually at lines 263-274 (line 264: alert name, line 265: expr, line 266: for=0m). LINE NUMBERS ARE WRONG.
  • [x] OOMKilled expression confirmed: kube_pod_container_status_last_terminated_reason{reason="OOMKilled"} > 0 at line 265
  • [x] for = "0m" confirmed at line 266

Repo Placement

Two repos correctly identified in comment #2: pal-e-deployments (memory limit) and pal-e-platform (alert rule). Forgejo issue is filed on pal-e-platform, which is correct for the alert rule fix. A companion issue or clear cross-repo reference is needed for the pal-e-deployments memory bump. Comment #4 says "two separate PRs needed" which is correct.
ISSUE: The issue body still says only forgejo_admin/pal-e-platform. An agent reading only the body would miss the pal-e-deployments work entirely. All corrections live in comments only.

Dependencies

  • No board item dependencies found -- no items in in_progress or next_up block this ticket
  • Board item #387 (CronJob stale failures, issue #170) is a sibling alert bug in next_up -- no dependency
  • UNDOCUMENTED: PR ordering dependency between the two repos. The memory bump (pal-e-deployments) must merge first or simultaneously to trigger the rollout that clears the stale alert. If the alert rule PR merges first without the memory bump, the alert stays stale until the next deployment.

Acceptance Criteria

Updated criteria in comment #2 are mostly testable:
  • "Memory limit bumped to 128Mi" -- testable via kustomize build and kubectl describe
  • "OOMKilled alert rule updated with for: 15m" -- testable via tofu plan
  • "ArgoCD syncs platform-validation successfully" -- testable via argocd CLI
  • "Current OOMKilled alert clears within 15 minutes" -- testable but DEPENDENT on memory bump triggering a rollout (as comment #3 correctly identifies). If only the for: duration changes, the alert still persists because the metric persists.
  • "tofu plan shows only the for: duration change" -- testable
Comment #4 proposes changes(kube_pod_container_status_last_terminated_reason{reason="OOMKilled"}[1h]) > 0 as an alternative and says "dev agent should evaluate both approaches." This is a reasonable delegation but leaves the acceptance criteria ambiguous -- the agent needs to know which approach is the expected outcome.

Blast Radius

  • Memory limits: 10 other services in pal-e-deployments also use 64Mi limits (mcd-tracker postgres, pal-e-docs app + embedding-worker, svelte-playground, basketball-api postgres, pal-e-mail postgres, playground x2, westsidekingsandqueens). Not a blocker for this ticket -- these are different workloads -- but the same OOMKill risk exists for all of them.
  • Alert rule: The OOMKilled alert rule is global (no namespace filter). The for: 15m change affects ALL namespaces, not just platform-validation. This is fine (desired behavior) but should be noted in the scope.
  • PodRestartStorm alert: The sibling alert at line 252-261 also uses for = "0m". Same staleness pattern could apply but is less problematic since restart count resets on pod recreation. Not in scope but worth noting.

Recommendation

Three actions needed before this ticket is READY:
  • Update issue body -- Consolidate all corrections from comments into the body so an agent reads one authoritative spec. Include both repos, correct file targets with correct line numbers (263-274 not 230-241), test expectations, and constraints.
  • Fix line numbers -- terraform/main.tf OOMKilled rule is at lines 263-274, not 230-241 as stated in comment #2.
  • Document PR ordering -- Explicitly state that pal-e-deployments PR (memory bump) should merge first to trigger rollout, which clears the stale alert. Or state that both PRs can be independent if the agent picks the changes() expression approach (which doesn't depend on rollout).
Optional improvements:
  • Add User Story section (even a brief "As a platform superuser, I want OOMKill alerts to auto-resolve after recovery so that I don't investigate false positives")
  • Add Constraints section (tofu plan -lock=false, two repos = two PRs, ArgoCD sync)
  • Add Checklist section
  • Note that kustomize build test should use kubectl kustomize (kustomize CLI not installed)