Skip to content

test(worker): pin the late stop recheck and the logged status filters - #1587

Merged
freeqaz merged 2 commits into
mainfrom
test-gaps-20
Oct 7, 2026
Merged

freeqaz merged 2 commits into
mainfrom
test-gaps-20

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

A mutation sweep of apps/controller/src/worker.ts on main (after #1553 and #1554) left three real test gaps. They survive all 33 test files that import the controller worker or the PostgreSQL work queue:

  • Late stop at publication. The worker checks that the Agent still runs before it stages the candidate. A stop can commit after that check, while a before-commit Compute Driver activates the candidate. The publication transaction re-reads the Agent under its row lock and must leave the candidate unpublished. Nothing tested that recheck. Removing it CAS-publishes the candidate and then retires the predecessor that the stop owns.
  • Logged Status reason filter. A failure's logged reason must be one CamelCase word (a Kubernetes Status reason). A free-text HTTP status text such as Service Unavailable was not tested, so dropping the filter went unnoticed.
  • Logged HTTP status range. Only an integer from 100 to 599 is logged as status. A Driver error with another numeric code, such as a gRPC client error (code: 14, UNAVAILABLE), was not tested, so accepting any number went unnoticed.

Change

This change is test-only. It adds two tests to tests/integration/postgres-worker-agent-revision.test.mjs and uses the existing worker-revision fixture helpers.

  • "a stop committed after the last running check never publishes the candidate"
    • A before-commit Driver blocks the candidate's activation, the test requests a stop, and then it releases the activation.
    • It asserts that the Agent ends stopped with no active revision and that no revision was retired.
    • It asserts the exact stop sequence: the candidate stops itself, then the stop work stops the still-serving predecessor and cleans up the candidate.
    • It asserts that the deployment fails with REVISION_STOPPED.
  • "an activation failure logs only an HTTP status and a one-word Status reason"
    • The test throws a transient Kubernetes failure whose cause has a free-text reason, then an error with a gRPC numeric code.
    • It asserts that the worker.completed pending fields keep status 503, drop the reason, and log no status for code 14.

Verification

PR conflicted files (main / this head) test-file hunks (main / this head)
#913 0 / 0 0 / 0
#851 13 / 13 2 / 2
#307 54 / 54 2 / 2
#252 58 / 58 2 / 2
#144 30 / 30 4 / 4
#115 27 / 27 2 / 2

Risks

  • This is a test-only change. The late-stop test blocks one activation on a promise. The test releases it in a finally block, and every wait is bounded.
  • The stop order is deterministic: the queue does not claim the Agent's stop work while the candidate's work is claimed.

…us filters

A stop committed after the worker's last running check is only seen when
publication locks the Agent: the candidate must stay unpublished and must
not retire its predecessor. The logged failure fields must drop a status
text with spaces and a non-HTTP numeric code such as a gRPC status.
… stop

Review follow-up: the candidate stops itself, then the stop work stops the
still-serving predecessor before cleaning up the candidate. Release the
blocked activation even if the stop request fails.
@freeqaz
freeqaz requested a review from a team as a code owner October 7, 2026 04:36
@clawsweeper

clawsweeper Bot commented Oct 7, 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 P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 7, 2026
@clawsweeper

clawsweeper Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed October 7, 2026, 12:39 AM ET / 04:39 UTC.

ClawSweeper review

What this changes

Adds PostgreSQL worker tests for a stop committed during activation and for filtering unsafe reason text and non-HTTP status codes from logs.

Merge readiness

✅ Ready for maintainer review

Keep open: the two tests add useful regression coverage beyond current main, with no actionable patch defect found. Repository policy reserves this PR for maintainer triage.

Priority: P3
Reviewed head: 46ab382fd97dcd9a0311e9322efc825f38f63ddd

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) Focused, useful regression coverage with clear production boundaries and no actionable defect found.
Proof confidence 🌊 off-meta tidepool Not applicable: Verified repository write access exempts this author from the external-contributor proof gate. The tests exercise production worker publication and logging with PostgreSQL and controlled Compute behavior; reported passes are supplemental and were not independently executed. No stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Verified repository write access exempts this author from the external-contributor proof gate. The tests exercise production worker publication and logging with PostgreSQL and controlled Compute behavior; reported passes are supplemental and were not independently executed. No stored-data contract changes.
Evidence reviewed 8 items Pinned introduced change: The verified base-to-head delta adds 143 lines to one integration test file and changes no production code, schema, dependencies, or automation.
Production publication boundary: Current main checks running intent before before-commit activation, then locks the Agent and rechecks desired state before publishing. A stopped candidate is shut down without entering predecessor retirement. The new test targets this later boundary rather than duplicating the existing preparation race.
Independent logging assertions: The worker admits only integer HTTP codes from 100 through 599 and bare alphabetic Status reasons. Existing adjacent coverage proves accepted status/reason values; the added case protects rejection of free-text reasons and code 14 through real worker events.
Findings None None.
Security None None.

How this fits together

The OpenClaw Control Plane worker consumes durable deployment and stop requests from PostgreSQL and invokes Compute Drivers. It publishes active revisions, records deployment outcomes, and emits filtered diagnostic events.

flowchart TD
  A[Deployment and stop requests] --> B[PostgreSQL work queue]
  B --> C[Controller worker]
  C --> D[Compute activation]
  D --> E[Locked publication recheck]
  E --> F[Active revision or stopped deployment]
  D --> G[Filtered diagnostic events]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test growth production +0, tests +143; 2 cases added Coverage expands through existing fixtures without adding runtime code or test-only production seams.

Technical review

Best possible solution:

Retain these focused scenarios in the existing PostgreSQL worker suite to protect publication ordering and diagnostic filtering.

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

Not applicable: this PR adds regression coverage rather than reporting a current-main failure. Source inspection confirms the exercised boundaries; no tests were run during review.

Is this the best way to solve the issue?

Yes: extending the existing PostgreSQL fixture keeps the scenarios at their production worker boundary and adds rejection and race coverage absent from adjacent cases.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 533f50c76676.

Labels

Label changes:

  • add P3: This is focused regression coverage for existing safeguards, with no product behavior change.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Verified repository write access exempts this author from the external-contributor proof gate. The tests exercise production worker publication and logging with PostgreSQL and controlled Compute behavior; reported passes are supplemental and were not independently executed. No stored-data contract changes.

Label justifications:

  • P3: This is focused regression coverage for existing safeguards, with no product behavior change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Verified repository write access exempts this author from the external-contributor proof gate. The tests exercise production worker publication and logging with PostgreSQL and controlled Compute behavior; reported passes are supplemental and were not independently executed. No stored-data contract changes.

Evidence

What I checked:

  • Pinned introduced change: The verified base-to-head delta adds 143 lines to one integration test file and changes no production code, schema, dependencies, or automation. (tests/integration/postgres-worker-agent-revision.test.mjs:5347, 46ab382fd97d)
  • Production publication boundary: Current main checks running intent before before-commit activation, then locks the Agent and rechecks desired state before publishing. A stopped candidate is shut down without entering predecessor retirement. The new test targets this later boundary rather than duplicating the existing preparation race. (apps/controller/src/worker.ts:3491, 533f50c76676)
  • Independent logging assertions: The worker admits only integer HTTP codes from 100 through 599 and bare alphabetic Status reasons. Existing adjacent coverage proves accepted status/reason values; the added case protects rejection of free-text reasons and code 14 through real worker events. (apps/controller/src/worker.ts:177, 533f50c76676)
  • Supported integration fixture: The existing fixture composes the production worker, PostgreSQL State and queue, native IAM, and controller stop/status methods. Compute supplies controlled boundary observations; the worker retains lifecycle and logging decisions. These tests do not claim live Kubernetes or model execution. (tests/helpers/postgres-worker-revision-fixture.mjs:398, 46ab382fd97d)
  • Prior merged coverage remains distinct: test(postgres): simplify revision-worker fixtures and failure scenarios #1553 merged fixture simplification, and test(controller): pin namespace failure reasons, image inspect bound and base image variable #1554 merged Namespace-reason coverage. Neither supplies the two introduced scenarios, so they are useful provenance rather than replacement PRs.
  • Verified author permission: Live GitHub metadata reports CONTRIBUTOR association, while the permission endpoint confirms freeqaz has write permission. This establishes trusted repository access for the contributor-proof exemption despite the conflicting supplied association fields.

Likely related people:

  • freeqaz: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • freeqaz-openai: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@freeqaz
freeqaz merged commit 276965d into main Oct 7, 2026
44 checks passed
@freeqaz
freeqaz deleted the test-gaps-20 branch October 7, 2026 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. 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