Skip to content

fix(worker): log why an exhausted active revision pass failed - #3352

Merged
freeqaz merged 2 commits into
mainfrom
fix-1743-log
Oct 23, 2026
Merged

freeqaz merged 2 commits into
mainfrom
fix-1743-log

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Problem

When a deployment has already published the active pointer and its last retry fails (or an active revision's maintenance pass exhausts its retries), continueExhaustedActiveRevision ends the pass with a worker.completed line that carries no failure fields. Every other ended pass logs cause (plus dependency, status and the rest when known), so an exhausted post-publish deploy or maintenance pass during an outage logged only code: DEPENDENCY_UNAVAILABLE, with no hint of what failed.

Change

  • apps/controller/src/worker.ts: finalizeRevision passes its failureLogFields into continueExhaustedActiveRevision, which spreads them into its worker.completed line, for both :reconcile and :maintenance:N keys. No other behaviour changes.
  • Tests: a shared withUnavailableIAM(unavailable) driver transform in tests/helpers/postgres-worker-revision-support.mjs replaces three inline copies of the IAM-outage driver (two in the teardown file, one in the credential-sources file).
  • The exhausted repository maintenance case now asserts the logged cause. Two new cases cover an exhausted deployment of the published active revision, with and without a declared maintenance interval. They assert the cause, that the runtime keeps running, and that no maintenance is queued.

Open, not in this PR: whether that exhausted deployment should also start the revision's maintenance chain. Today it doesn't, so the published runtime has nothing checking on it until the next deploy. Queuing maintenance on its own would be wrong: the maintenance gate ends any pass whose :reconcile work failed as REVISION_SUPERSEDED, and for repository revisions that retires the running runtime. That needs a design decision. The new cases pin today's behaviour (no maintenance queued) and point at the question, so a fix will have to update them on purpose.

Verification

  • The three cause assertions fail on main 5370e97 (cause is undefined) and pass with the fix.
  • postgres-worker-agent-revision-teardown.test.mjs: 31/31 on local PostgreSQL, then pinned 20/20 at 2 CPUs (the later commit only edits a comment).
  • postgres-worker-agent-revision-credential-sources.test.mjs: 43/43. Its two cases that use the helper are pinned 20/20 at 2 CPUs.
  • Internal review: approve. Its one nit (say which new case pins the open question) is applied.
  • tsc --build tsconfig.json tests/tsconfig.json, eslint and prettier are clean on the touched files.

Risks

Low. This is one more set of log fields on an existing line, plus a test-only helper refactor. The new fields are the same ones the other ended passes already log; none of them is a secret.

continueExhaustedActiveRevision ended a published deployment's last retry
(and an exhausted maintenance pass) with a worker.completed line that had
no failure fields, unlike every other ended pass. Pass the pass's
failureLogFields through so the line carries cause/dependency/status.

Tests: a shared withUnavailableIAM helper replaces three inline copies of
the IAM-outage driver; the exhausted repository maintenance case and two
new exhausted-published-deployment cases assert the logged cause. The new
cases pin today's behaviour of queuing no maintenance chain, which is
finding 1743's open design question.
@freeqaz
freeqaz requested a review from a team as a code owner October 23, 2026 20:07
@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 23, 2026
@clawsweeper

clawsweeper Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR adds existing failure diagnostics to exhausted active-revision pass logs and shares the tests’ IAM-outage fixture.

Example: An active revision’s maintenance pass exhausts its attempts while IAM throws an Error.

  • Before: worker.completed reports code DEPENDENCY_UNAVAILABLE without cause.
  • After: The same event also reports cause Error.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, source-supported repair with passing PostgreSQL CI and meaningful regression coverage; behavioral evidence remains at the in-process harness boundary.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored PR is outside the ordinary contributor proof gate; supplied validation covers the PostgreSQL worker harness, without a shipped-entry-point log artifact.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Operators cannot identify the classified failure behind an exhausted active-revision pass from its completion log.
Reason: This restores documented failure diagnostics without changing lifecycle behavior, and the collaborator-authored PR establishes direction.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it repairs a current diagnostic omission with a focused change, and I found no introduced correctness or security defect.

Priority: P2
Reviewed head: f85a5e87e003d8bc7ef94e6c78c0903912cc6adf

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: No shipped-worker outage trace is supplied; the reported PostgreSQL runs exercise the real worker with fixture Drivers.
Agent review details

How this fits together

The durable controller worker consumes PostgreSQL revision work, reconciles Agent runtimes through Drivers, and emits structured completion events for operators.

flowchart LR
  A[PostgreSQL work queue] --> B[Controller worker]
  B --> C[Revision reconciliation]
  C --> D[IAM and Compute Drivers]
  D --> E[Failure classification]
  E --> F[Exhausted active revision completion]
  F --> G[Structured worker log]
Loading

Technical review

Best possible solution:

Preserve the existing continuation lifecycle and forward its already-classified diagnostics through the existing completion event.

Do we have a high-confidence way to reproduce the issue?

Current main demonstrably drops failureLogFields on the exhausted-active-revision branch; the PR reports three assertions failing on base and passing after the fix, but I did not execute tests.

Is this the best way to solve the issue?

Forwarding the existing safe diagnostics at the existing owner is the smallest complete repair; the separately acknowledged maintenance-chain question need not expand this logging fix.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 5370e972f8b1.

Provenance checked

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • freeqaz: Raw commit 18dd816 adds apps/controller/src/worker.ts:3535 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 18dd81637e3d; files: apps/controller/src/worker.ts)

Review metrics

Metric Value Why it matters
Production versus test changes Production +4/-1; tests and helpers +104/-67 The production repair is minimal; test growth covers both exhausted deployment variants and shares three existing outage-driver implementations.

Labels

Label changes:

  • add P2: Missing diagnostics impede investigation of exhausted revision work but do not themselves change runtime availability or lifecycle.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: Missing diagnostics impede investigation of exhausted revision work but do not themselves change runtime availability or lifecycle.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 23, 2026, 4:20 PM ET / 20:20 UTC.

@freeqaz
freeqaz merged commit 5c901c0 into main Oct 23, 2026
76 checks passed
@freeqaz
freeqaz deleted the fix-1743-log branch October 23, 2026 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant