Skip to content

Server parity: accept PHP operator metrics read across its redispatch steps - #708

Merged
rmcdaniel merged 5 commits into
mainfrom
rust-325-opmetrics-race
Oct 25, 2026
Merged

rmcdaniel merged 5 commits into
mainfrom
rust-325-opmetrics-race

Conversation

@rmcdaniel

@rmcdaniel rmcdaniel commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

Part of #325.

Problem

On #673's parity run 39995620421, the Shared Server fixtures failed on PostgreSQL 17 and MySQL 8.0. Published PHP 2.11.1 failed the operator-metrics contract at waiting_after_redispatch; the diff came before any PHP pass line.

PHP's repair pass redispatches the overdue task in two steps:

  1. TaskWatchdog::recoverExistingTask raises the repair count and projects the run as repair_needed, in one transaction.
  2. TaskDispatcher::markDispatched stamps the dispatch, and refreshRunSummary projects the run again, separately.

Published Workflow aca7928484's OperatorMetrics::snapshot reads each value with its own query, with no shared snapshot, even inside one section. So one response can straddle the steps. The contract refused two such shapes from CI:

  • MySQL 8.0: task dispatched, run still marked repair_needed;
  • PostgreSQL 17: tasks read before the redispatch, backlog after it.

Security's review of the first head (3fa5cd5288, comment 6373392835) found that a section-level model was not enough. A forward-only change can split the tasks section itself, and a timestamp read before its count can outlive it.

Change

  • The contract models PHP's four states: overdue, repairing, dispatched, and redispatched.

  • For published PHP only, the stage follows the actual query order of the eight values that depend on the state:

    • runs: oldest_repair_needed_at, then repair_needed;
    • tasks: oldest_dispatch_overdue_since and oldest_unhealthy_at, then dispatch_overdue, max_repair_count, and the unhealthy recount;
    • backlog: unhealthy_tasks, then repair_needed_runs.

    A response passes when a sequence of states that never goes backwards in that order produces exactly its counts and timestamps.

  • Health reads its own snapshot after the metrics. Each warning follows the field it reads: task_transport follows tasks.unhealthy, routing_health follows tasks.dispatch_overdue, and durable_resume_paths follows backlog.repair_needed_runs.

  • Native must still report one consistent state, with each timestamp present exactly when its count is.

  • The contract header now names published Server 2.11.1.

  • The decision and the review correction are recorded in docs/rust-server-port-checkpoints/2026-10-23-operator-metrics-redispatch-states.md.

Checks

  • Security's counterexample and the kept-timestamp shape pass for PHP; the counterexample fails for native.
  • Five backward steps fail: a recount after the dispatch count, a backlog after the recount, a count after its timestamp, a run timestamp ahead of the later reads, and a health recount after its dispatch count.
  • An exhaustive test covers all 256 combinations of the eight values. The 37 a forward-only order produces pass; the other 219 fail.
  • Both original CI shapes still pass for PHP and fail against the old contract.
  • Server-parity unit tests at 6b6757e7e: 5,504 tests, 5,502 pass, 1 intended skip. The one failure (PhpImageContextTest) runs Docker and cannot run inside the plain Node container; it is unrelated to this change.

rmcdaniel and others added 3 commits October 23, 2026 18:25
PHP's repair pass raises the task's repair count and projects the run in
one transaction, then stamps the dispatch and projects the run again in a
separate step. Operator metrics reads runs, tasks and backlog without a
shared snapshot, so one response can straddle those steps. Model the four
PHP states and accept reads that never go backwards in read order; native
must still report one consistent state.
@rmcdaniel

Copy link
Copy Markdown
Member Author

Security review finding — blocking on exact head 3fa5cd528895c6bb28b16b8e32065a6a25527251

The new monotonic matcher still assumes the complete tasks section was read at one repair state. Published Server 2.11.1 does not provide that atomicity. Its exact locked Workflow source aca7928484083635e68a5a384b2dcecef842108e runs separate queries inside OperatorMetrics::taskMetrics: it captures oldestDispatchOverdueSince, later queries dispatch_overdue, later queries max_repair_count, and only then independently recomputes unhealthy.

A real forward-only 0 → 1 → 2 transition can therefore return:

  • runs at state 0;
  • task dispatch_overdue=1 at state 0;
  • task max_repair_count=1 at state 1;
  • task unhealthy=0 at state 2;
  • backlog unhealthy_tasks=0 and repair_needed_runs=1 at state 2.

The exact current phpRedispatchExpectation rejects that forward-only shape because no single tasksAt(state) has dispatch_overdue=1, max_repair_count=1, and unhealthy=0. My bounded counterexample against this head reports intra_task_straddle_accepted: false. The earlier 16-shape enumeration only proved section-level sequences; it did not cover the PHP queries within a section.

There is a second manifestation in the same source: oldest_unhealthy_at is derived from timestamps captured before the returned unhealthy count is recomputed. A 0 → 2 transition inside taskMetrics can legitimately retain the timestamp while returning unhealthy=0, but the current generic timestamp assertion rejects it before the new matcher runs.

Please extend the PHP-only model and focused tests to the actual relevant query order inside taskMetrics, including the timestamp/count straddle, while keeping native strict and still rejecting backward sequences. Then return one new exact head and hosted qualification. Do not merge this head. The current hosted run remains useful historical evidence for the two original shapes, but it cannot close this review finding.

Non-blocking accuracy cleanup on that next head: the contract header still says published PHP 2.5.14, while this fixture is now bound to published Server 2.11.1; the PR body’s 4,792-test count is also stale versus the exact-head local result of 5,280 tests (5,279 pass, one intended skip).

Published Workflow aca7928484 reads each operator metrics value with its own
query, so one section can straddle a repair. The PHP-only model now matches
eight state-dependent reads in their real order, including the timestamps
read before their counts, and reads each health warning from its own field.
Native stays strict.
@rmcdaniel

Copy link
Copy Markdown
Member Author

Security follow-up on corrected head 6b6757e: the exact pinned-image comparator command passes 2,031/2,031 tests, including all 63 corrected operator-metrics cases; the next isolated commands pass 10/10 and 7/7. Hosted run 40017239524 nevertheless has one later failure in execution-and-codec job 120644918290, with five database cells still active, so this head is not accepted or merge-ready. It is also now five non-overlapping commits behind main f67f281. Preserve the current run for its decisive evidence, classify the exact failed command when logs release, then integrate current main and return a fresh exact head for required checks.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security follow-up on Shared Server fixtures run 40017239524, attempt 1:

  • The changed matcher is not the failing surface. PHP, embedded and Rust records all passed, including operator-metrics; the exact pinned-image local discrimination also passed all 2,031 comparator cases.
  • execution-and-codec job 120644918290 later failed when the published Python patch-replacement worker exhausted a typed SQLite backend_lock_pressure poll while the concurrent maintenance loop logged a locked TaskRepair update. The observer intentionally does not retry poll pressure, so retain this as a separate lifecycle/reliability finding.
  • MySQL 8.0 job 120644918636 later returned a timely Python/PHP query result but observed the terminal submission without its response acknowledgement before teardown. The PHP query-authority harness already waits inside the original budget for that acknowledgement before stopping; the Python harness does not. Align the bounded observation before treating a retry pass as qualification.

Attempt 2 is repeatability evidence and does not erase either original. This head also cannot be the final merge candidate: main is now 0667ba7 and 13 commits ahead. Integrate current main, retain/link the two owning corrections, and run a fresh exact head before merge.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security terminal disposition for retry run 40017239524: both selectively rerun jobs passed on unchanged stale head 6b6757e7e20aace841935a7ebc504e8737ded896.

  • execution-and-codec job 120691072578 passed every step, including the published query-authority and SDK temporary-database-pressure cohorts.
  • MySQL 8.0 job 120691072894 passed every step, including the exact query acknowledgment path that attempt 1 ended with submission=1 / acknowledgment=0.

This establishes that both original observations are nondeterministic; it does not erase or supersede them. The query teardown race still requires accepted #720, and the SQLite typed poll refusal remains owned by #721 / Python SDK #218. This #708 head is also behind current main. Do not merge or treat retry success as publication qualification: integrate the owning corrections and current main, re-review the exact combined head, then run a fresh complete hosted matrix. Original failed jobs 120644918290 and 120644918636 remain evidence.

@sonarqubecloud

Copy link
Copy Markdown

@rmcdaniel
rmcdaniel merged commit ce14b38 into main Oct 25, 2026
26 of 27 checks passed
@rmcdaniel
rmcdaniel deleted the rust-325-opmetrics-race branch October 25, 2026 03:58
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