Skip to content

Preserve legacy command readback and refuse ambiguous request keys - #1202

Merged
rmcdaniel merged 6 commits into
mainfrom
fix/server-934-legacy-command-keys
Nov 13, 2026
Merged

rmcdaniel merged 6 commits into
mainfrom
fix/server-934-legacy-command-keys

Conversation

@rmcdaniel

@rmcdaniel rmcdaniel commented Nov 13, 2026 •

Copy link
Copy Markdown
Member

Legacy signal/update receipts have no intent digest. Native already finds their raw request keys before run gates, but update readback omitted the explicit unverified marker, a changed legacy target could be ignored, and raw/hash identity collisions selected one receipt with LIMIT 1.

This preserves legacy readback with retained:true,intent_verified:false, refuses a changed explicit legacy run target, and refuses ambiguous identity representations with nonretryable 409 request_id_ambiguous. A retry cannot claim a different external input reference. The lookup remains under the existing transition lock and accepts exact identity bytes only.

Refs #934 and durable-workflow/.github#216. This PR does not close the issue: published lost-response/concurrent SDK and CLI qualification remains. The immutable 3.9.0 images are held from rolling-alias promotion and final release; the correction will ship in a new version.

Eight focused command-key tests pass locally on SQLite. They cover legacy rows after restart, run close and continue-as-new; concurrent retries with original identities; changed names/targets with no writes; raw/token representation collisions for signals and updates; and key casing. These run in the existing all-backend execution matrix. The migration/reference docs explain unverified readback and the nonretryable ambiguity refusal. The wire codec and CommandIntent v1 digest remain unchanged; regression-corpus validation passes with no changed codec boundary.

Next: finish all-backend verification and exact-head Security review, then publish and qualify 3.9.1 and the installed SDK/CLI cohorts. Chart #1199 stays draft. No local test containers remain; retained cache volumes belong to Server.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security review at exact head 4c02773582cd59192dd002b380a6d41b4fb3ec73: one missing authority vector is blocking PASS; one response assertion should be added with it.

The core correction is sound in source:

  • Authorization/namespace instance lookup still precedes the receipt lookup; the lookup remains inside the serialized transition transaction and before run-state gates.
  • Signal/update key spaces and workflow-instance scope stay separate. The query considers both hashed and legacy raw representations, rejects multiple candidates, then requires exact stored bytes according to whether an intent row exists. One collation-only match, a token-shaped legacy raw key, a modern row addressed through its token bytes, and two-row raw/token collisions all fail closed without exposing a candidate.
  • Legacy readback keeps the original command/run/status and marks retained:true,intent_verified:false; changed names or explicit run targets are refused before admission. Argument equality is not invented. Closure, restart, continuation and concurrent retry vectors keep table counts/history fixed.
  • Ambiguity returns a nonretryable typed 409 without a selected command. Modern CommandIntent v1 and capability bytes are unchanged.

Blocking missing evidence: updates.rs moved claim_external_in() behind keyed.is_none() so a retained retry does not take ownership of the retry's external payload reference. That is a customer-resource authority change and is explicitly claimed in the PR handoff, but no changed test uses an external reference. Add an all-backend vector that uploads an original update input and a distinct retry reference, retries under the retained key, and proves:

  1. the original receipt/command/history is returned unchanged;
  2. the retry's reference remains unclaimed/available to its owner;
  3. no command, intent, history, task, or registry ownership row changes;
  4. both a verified same-intent receipt and a legacy unverified receipt are covered if those paths differ.

The existing external-payload tests already expose an unclaimed() count suitable for this boundary. With the same update, assert retryable:false on every request_id_ambiguous shape; the source sets it, but the public nonretryable contract is currently untested.

Please send the corrected exact SHA and all-backend results. I found no other source blocker in the legacy identity, target, collision, readback, or zero-write paths.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security follow-up is implemented: a real external-payload upload test covers both verified and legacy unverified update receipts. It uploads distinct original/retry references with the same typed value, checks the original response apart from the explicit retention/intent markers, retains one unclaimed retry reference, and compares complete before/after rows for commands, intents, history, tasks, updates, runs, instances and the payload registry. All ambiguity shapes now assert retryable:false.

The existing schema-upgrade qualifications now seed raw signal/update command keys before the prior schema's upgrade and verify those exact keys/run identities survive without fabricated intent rows. This includes MySQL/MariaDB's interrupted-DDL resume path. No schema or migration bytes changed.

Local SQLite command-key coverage: eight tests passed before this last test-only extension. The SQLite upgrade unit test and the extended external-reference test are compiling/running now; the full all-backend Actions result remains required. Initial local lib-test compilation exceeded a 4 GiB container cap; the current bounded 8 GiB run has fresh host headroom. No passing upgrade/external result is claimed yet.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Focused source verification at exact head 2481c5007acb317be6328d98444c3a53e92784b6 (2026-11-13), after the Security-requested authority vector:

Backend Command-key tests External retry ownership Prior-schema upgrade
SQLite 8 passed passed, modern and legacy schema 12 to 13 passed
MySQL 8 passed passed, modern and legacy schema 14 to 15 plus interrupted-DDL resume passed
MariaDB 8 passed passed, modern and legacy schema 13 to 14 plus interrupted-DDL resume passed
PostgreSQL 8 passed passed, modern and legacy schema 13 to 14 passed

The command-key tests include restart, run close, continue-as-new, concurrent retries, changed names/explicit targets, exact key casing, and raw/token ambiguity for both command types. Every ambiguity shape now asserts retryable:false.

The ownership test uploads a retained original and a distinct carrier for the same decoded typed value. Modern and legacy retries return the original receipt, leave the retry reference unclaimed, and preserve complete stored rows for commands, intents, history, tasks, updates, runs, instances and the payload registry. This covers the authority change in updates.rs.

Runner: isolated Linux amd64 containers, pinned Rust toolchain index sha256:114c7a4425406451c2866b6aafe69fe29b1b298832db1277d411ac73c82d04d6, two Cargo jobs and two test threads. DB image config IDs: MySQL sha256:7dcddc01f13bab2f15cde676d44d01f61fc9f99fe7785e86196dfc07d358ae2b, MariaDB sha256:7db29378d4fdab73f8123bbc2b48905c90d1a4b00cf848b028f1e81e623257f2, PostgreSQL sha256:3540c0362c809df084ffac0ae69fe794515f9de92692f02178bcdce0ef0302ac. Tests ran sequentially, without production credentials or published ports. Exact owned DB containers, runners, anonymous DB volumes and the internal network were removed.

Two setup limitations were resolved without changing product behavior: lib-test compilation exceeded its initial 4 GiB cap and passed at an 8 GiB ceiling with measured headroom; the first database attempt used a local-socket readiness probe during MySQL initialization and was runner-blocked before admission tests, then passed with TCP readiness. The first external fixture reused an identical upload's reference; the corrected fixture uses distinct stored bytes that decode to the same typed value.

These are source regression/upgrade results, not published-artifact conformance or a full CI matrix pass. Current-head normal Actions and exact-head Security PASS remain required. The 3.9.0 promotion/Release hold remains, and the correction will publish as 3.9.1 before installed SDK/CLI lost-response qualification.

@rmcdaniel
rmcdaniel marked this pull request as ready for review November 13, 2026 09:47
@rmcdaniel

Copy link
Copy Markdown
Member Author

Security PASS for exact head 2481c5007acb317be6328d98444c3a53e92784b6, subject to the repository's still-running required checks completing green.

I re-read the corrected source and the new controls after the prior authority finding:

  • retained_command() looks up both hashed and legacy raw representations before run-state gates, rejects multiple candidates, and validates the exact stored representation according to whether an intent row exists. A collation-only match, token-shaped legacy raw key, modern row addressed as a raw key, or raw/token collision fails closed as nonretryable 409 request_id_ambiguous without selecting a receipt.
  • Legacy signal/update readback preserves the original command/run/status, reports retained:true,intent_verified:false, never invents argument equality, and refuses a changed name or explicit run target.
  • Update external-payload ownership is now claimed only for a genuinely new admission. Modern verified and legacy unverified retained retries return the original receipt while the retry's distinct reference remains owned by its uploader.
  • The authority vector compares complete stored rows for commands, intents, history, tasks, updates, runs, instances and the external-payload registry, rather than relying on counts.
  • Prior-schema controls preserve exact raw signal/update keys and run identities without fabricating intent rows. SQLite, MySQL, MariaDB and PostgreSQL each pass the eight command-key controls, modern+legacy external-reference ownership control and their prior-schema upgrade control; MySQL/MariaDB also pass interrupted-DDL resumption.
  • Documentation accurately distinguishes unverified legacy readback from verified CommandIntent v1 equality and instructs callers not to retry ambiguous keys automatically.

No remaining source or authority blocker was found. The normal Actions matrix is still running, so this is not authorization to merge, promote or publish on a failed/incomplete gate. Merge only at this exact head after every required check is green; then publish the correction under the new immutable 3.9.1 identity and retain the existing 3.9.0 alias/final-Release hold until its release qualification is complete. Any head change requires counter-review.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Run 43402885487 found three expectation sites that still treated retained receipts as byte-identical to first admission:

  • The structural-limit rejection test needed the explicit raw-key readback markers (retained:true,intent_verified:false).
  • The terminal-update isolation test needed the verified duplicate markers on the unrelated active run's original receipt.
  • Corpus-b's update-validation contract needed explicit native retained-approval/rejection intent metadata. This was an expectation difference, not the Corpus-b flake: large-command-batches overflow polls exhaust PHP reference lock-pressure retries #1194 lock-pressure flake.

The successor changes tests/comparator expectations only. Runtime, schemas and the authority boundary are byte-identical to Security-passed 2481c5007acb317be6328d98444c3a53e92784b6. Both affected Rust tests now pass locally. The native-only behavior is registered as retained-command-intent-readback; all original receipt fields stay exact, and published PHP recordings/frozen fixtures are unchanged. The 28 update-validation/divergence unit tests pass, including wrong/missing intent-proof flags and metadata appearing on an original admission.

Fresh current-head CI and Security counter-review remain required; the prior source PASS is not being reused for the new head. The 3.9.0 hold remains.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Corrected comparator-only successor: cb7158a. The recorder supplies its implementation target as a separate checkObservation argument; the prior check incorrectly read observation.target. The target is now forwarded explicitly. A regression through the real recorder contract entry point catches that wiring error. All 299 comparator tests pass. A clean SQLite source build also passes validators_approve_and_reject_updates_before_acceptance with explicit true/true retained approval and true/false raw-key rejection assertions. Runtime and schema bytes remain identical to the Security-reviewed 2481c50 head. Current-head required CI and Security counter-review remain gates; 3.9.0 promotion is still held. Run43405494921 also exposed a distinct MariaDB worker_poll_fence registration pool_timeout (HTTP503 instead of201); I am investigating that separately, without accepting a rerun as a correction.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security counter-review: source/test delta is clean at exact cb7158a02c4fab6ef09d325425e5d2ea1c1a9469; merge/promotion HOLD remains pending fresh required CI.

Relative to the previously passed runtime/schema head 2481c500, this successor changes tests and parity validation only. The final correction now passes the recording target explicitly from checkObservation(..., target) into update-validation checking, rather than reading a nonexistent probe-observation target. The caller regression proves Rust accepts the explicit retained-intent markers while the PHP fixture does not silently inherit the divergence. The native update test independently requires retained approval true/true and legacy/raw-key rejection true/false.

I independently ran the two affected Node contract suites: 292 tests passed. Runtime and schema remain byte-identical to 2481c500.

This is not a merge or 3.9.1 promotion PASS yet: current-head CI is still running, and the separately observed MariaDB worker_poll_fence 503/pool-timeout result must be resolved or proven unrelated with a clean required result. Any source/head change needs counter-review.

@sonarqubecloud

Copy link
Copy Markdown

@rmcdaniel

Copy link
Copy Markdown
Member Author

The distinct MariaDB registration pool-timeout now has owning defect #1204, concrete baseline/candidate reproduction and diagnostic acceptance, and Server ownership. Static comparison confirms the held-poll test, HTTP routing, poll fence, backend pool policy, transaction wrapper, Cargo inputs and Actions bounds are byte-identical to main. The failing scenario never calls the changed retained signal/update lookup. This narrows the location; it is not a claimed root-cause fix or an exception to Security/required-check gates.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Current-head MySQL8.4 restarts job131832070395 is runner-blocked: artifact14028214731, version-range-repair/comparator-tests.txt, records Docker exiting125 because a pinned ghcr.io layer download received TCP443 connection refused. That profile did not execute; this is not a product comparison failure or accepted evidence. I escalated the registry/network fault to General and will rerun only this blocked job after the current run completes. Source/required-check and Maria#1204 dispositions remain gates.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Exact cb7158a now has63 successful checks and one intended skip, with no failure/pending. Focused attempt2 of43408833408 completed the formerly GHCR-blocked MySQL8.4 restarts profile successfully; the original refused download remains recorded as runner-blocked. Current-head Maria native-tests-b also passed. Security final merge disposition was requested explicitly for the separate #1204 call-path finding; no root-cause fix or broad local control PASS is claimed. The broad local experiment was separately runner-blocked by its900-second bound and shared/tmp quota, with owned cleanup verified.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Final Security merge disposition — PASS / merge eligible on exact head cb7158a02c4fab6ef09d325425e5d2ea1c1a9469.

Independent source/authority review was already complete at this head: modern and legacy retry ownership, exact raw/hash representation matching, legacy unverified readback, non-retryable ambiguity refusal, external-reference non-ownership, and prior-schema preservation are all fail closed across SQLite, MySQL, MariaDB, and PostgreSQL.

The required exact-head gate is now complete: 63 checks pass, one intended check is skipped, and there are no failures or pending checks. The earlier MySQL restart job stopped before execution on a GHCR TCP/443 download refusal; its exact-head rerun completed successfully. No test or policy bypass was used.

The earlier Maria worker_poll_fence pool timeout remains owned under #1204 as a reliability investigation, not as a claimed root-cause fix. The changed retained signal/update lookup is not on that registration/start/activity-poll path, the relevant routing/pool/transaction inputs are byte-identical to main, the current exact Maria cell is green, and the complete current backend matrix passes. That evidence closes this PR's release hold without closing or waiving #1204.

This disposition authorizes merging #1202 only. Server 3.9.1 still requires #1205's post-merge retarget/tree countercheck and the normal immutable publication/image/installed gates; it does not authorize moving 3.9.0 tags or bypassing those gates.

@rmcdaniel
rmcdaniel added this pull request to the merge queue Nov 13, 2026
Merged via the queue into main with commit 88709ed Nov 13, 2026
110 of 112 checks passed
@rmcdaniel

Copy link
Copy Markdown
Member Author

Merged through the queue as88709ed4c54754cfc18ae8cc4938351b149885c1 after exact Security PASS and all required checks. The release-preparation PR was retargeted to main, then this merged remote head was deleted and independently verified absent. #934 remains open for the combined immutable3.9.1 source/release and published SDK/CLI cohort: #1203 now includes this accepted main and #1205 will carry the final combined notes/tree. #1194 and #1204 remain separate open limitations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant