Skip to content

Keep job status bounded and store error detail separately - #3515

Merged
chubes4 merged 2 commits into
mainfrom
fix-3514
Sep 18, 2026
Merged

chubes4 merged 2 commits into
mainfrom
fix-3514

Conversation

@chubes4

@chubes4 chubes4 commented Sep 18, 2026

Copy link
Copy Markdown
Member

Addresses the status column half of #3514. See the correction comment on that issue — the retention/delete failure described in the original report is not a Data Machine bug and is not touched here.

What was broken

wp_datamachine_jobs.status was an unbounded free-text column storing entire exception messages, including absolute filesystem paths and third-party error strings. Real values from one site:

failed - Exception: …WorktreeContextInjector::has_owner_terminal_disposable_cleanup_signal():
  Argument #1 ($metadata) must be of type array, null given, called in /Users/…/plugins/dat…
failed - wp-ai-client request failed: … cURL error 28: Operation timed out after 513455 milliseconds …

Consequences:

  • GROUP BY status produced one group per distinct error string rather than per state, so the column could not be aggregated
  • every unique error text was a distinct value, defeating any index on status
  • row size inflated for exactly the rows that matter least
  • absolute filesystem paths landed in a data column

What this changes

Separates state from detail: status becomes bounded to the canonical states, with error detail stored in its own field. Includes a schema migration and a backfill that splits existing compound values at the established "<status> - <detail>" boundary, preserving the detail rather than discarding it.

Readers keep working across the migration, and status becomes safe to index and aggregate.

Verification

  • lint clean on all changed files
  • tests/job-status-normalization-smoke.php passes
  • tests/job-status-presentation-smoke.php passes

Honest gap: the two new PHPUnit suites (tests/Unit/Core/Database/JobStatusStorageTest.php, tests/Unit/Core/JobStatusBoundedStorageTest.php, 208 lines total) could not be executed locally — they require WP_UnitTestCase and no WordPress test harness is installed here (/tmp/wordpress-tests-lib absent). I confirmed that is environmental and pre-existing rather than caused by this change, but it means that test code has not actually run. Worth a CI run before merge.

Note on the TypeError from the issue

The original issue also flagged 332 jobs failing with has_owner_terminal_disposable_cleanup_signal(): Argument #1 ($metadata) must be of type array, null given. I traced it and it is not actionable here:

They are historical residue predating the guards. No fix is required and none is attempted.


AI assistance disclosure. Implemented by xai/grok-4.6 via opencode run, from a task specification I wrote after aggregating the jobs table by status on a live site. I reviewed the diff, ran lint and the smoke tests, and separately traced the TypeError to its actual repository and guard commits rather than accepting the issue's original framing.

Process note. Finalization happened outside Homeboy. homeboy agent-task cook admitted the task and then failed to claim its own detached Cook handoff on every attempt, with a healthy local daemon. Completed through an authorized direct-runtime fallback in an isolated worktree off origin/main (be9b5d5eb).

status was an unbounded free-text column, so GROUP BY could not aggregate
by state and exception text (including filesystem paths) landed in an
indexed column.

Add nullable status_reason, write only canonical base states, and backfill
compound "<status> - <detail>" values without discarding detail. Readers
still compose display from leftover compound rows, the new column, or
engine_data.job_status_reason during rollout.

resolve_status_match()/status_match_sql() prefix matching is only needed
while JobStatusMigration is incomplete. Once status is bounded, exact IN
matching is sufficient.

The TypeError (has_owner_terminal_disposable_cleanup_signal() receiving
null) is thrown when a worktree listing row has no metadata. That is a
missing record, not a callee-null contract, so the caller should pass [].
Data Machine now stores that exception in status_reason rather than status.
The DB round-trip test built its expectation with str_repeat(), leaving a
trailing space, then asserted the stored status_reason matched it exactly.
Storage trims on write, so the assertion failed on whitespace the layer
deliberately discards rather than on any behaviour under test.

JobStatusBoundedStorageTest is left unchanged: it exercises toStorage()
directly, where no trim occurs, and it passes.
@chubes4
chubes4 merged commit 79e67cd into main Sep 18, 2026
28 of 30 checks passed
@chubes4
chubes4 deleted the fix-3514 branch September 18, 2026 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant