Repository navigation
fix: delete retained payloads after pruning commits - #911
Conversation
|
Early Security protocol/source review for exact head A durable outbox must replay I exercised an isolated allowed Beside it, the PR test Required contract correction for this PRs stated recovery guarantee: make replay-safe deletion explicit in the public driver contract (an already-absent object succeeds), document the compatibility requirement for custom drivers, and add a custom-driver behavioral control that proves a lost acknowledgement converges without restoring pruned references. If the contract is intentionally narrower, the API/status/docs must state that non-idempotent custom drivers can remain pending and require operator recovery rather than claiming generic durable retry. This is not a hold on unrelated work, backend qualification, Rust, RCs, or publication. I found no evidence that the new outbox loses database references before commit, and the existing built-in path passed. Reviewed runtime file SHA-256: |
|
A second exact-head boundary is confirmed for the same new eventual-cleanup contract. On That response was accurate about the namespace/catalog deletion but silent about unfinished provider-data deletion. Before #911, a provider failure kept the namespace and returned an error; #911 intentionally changes that to committed deletion plus retry. The namespace-delete response now needs the same truthful pending/failed/blocked cleanup evidence as the retention and explicit cleanup paths, with a non-success-complete signal such as HTTP 207 while cleanup remains pending. Otherwise an operator cannot distinguish complete deletion from eventual cleanup using the destructive request result. Isolated fixture: 1 test / 7 assertions passed, SHA-256 Please add the namespace response/status regression alongside the in-progress namespace recovery controls. This is scoped to #911s changed behavior and does not hold unrelated work or continuing backend qualification. |
|
Exact-head follow-up on I staged two pending deletions, then created a retained The cause is concrete in the retry path: Please make retry fencing use the same complete retained-source inventory (or one shared inventory) and add at least the schedule-memo regression so a later column addition cannot silently diverge. Synthetic diff SHA-256: This finding is scoped to #911s new retry behavior. It does not hold unrelated work, Rust, backend qualification, or publication. |
|
Exact-head retained-source follow-up on 3bacb51: workflow instance metadata is another reproducible hole in the new retry fence. I staged two pending deletions by pruning under a synthetic provider outage, then inserted a retained workflow_instances row whose memo held a structurally valid external-payload reference to one candidate, with no registry row. After provider recovery and retryPending(), both provider objects were gone and both intentions were cleared, while the retained instance memo still pointed at the now-missing URI. The isolated counterfactual passed 1 test / 7 assertions because it asserted that observed breakage. Published Workflow 2.15.12 permits JSON-like nested memo values and copies start memo into workflow_instances.memo. The retry inventory scans run, typed-memo, history and schedule state but not WorkflowInstance::memo. The namespace-cleanup inventory likewise accepts instance IDs but has no workflow_instances source, so an instance-only retained/imported copy is neither fenced nor collected. Please include workflow_instances memo (and any other genuinely reference-bearing instance metadata established by the shared inventory) in the same unified source list requested in the prior schedule finding, with a regression showing a pending retry cannot break a retained instance reference. Synthetic diff SHA-256: 42212148501007f4c61a108997ed8d913631e2ffcfb63088af67ed0557571b8d. Reviewed cleanup source SHA-256 remains 4063e36aa4f68d08633a8630f7244e627343eceb2322643d887379d59ca9ae6d. This extends the existing #911 retained-source correction target. It is not an unrelated hold on Rust, backend qualification, RC work, or publication. |
|
One more exact source in the same retained-inventory correction is now reproduced: schedule-history payloads with no workflow run. On 3bacb51 I staged two pending deletions, inserted a normal-shape ScheduleCreated history row with workflow_run_id null and a valid external reference under payload.action.input, then retried after provider recovery. Both objects were deleted and both intentions cleared while the schedule-history row remained with the broken reference. The isolated counterfactual passed 1 test / 7 assertions because it asserted that result; diff SHA-256 8ab67719c1457eaf0f04b95006cea1d7d754a14af80191719b211ceedbe8a9e2. Published Workflow 2.15.12 records ScheduleCreated and ScheduleUpdated snapshots with spec/action before any run exists, so null-run schedule-history rows are normal, not only synthetic legacy data. retainedPayloadColumns currently limits schedule history to retained run IDs, excluding those rows, even though namespace cleanup already treats all schedule-history payloads in the namespace as reference-bearing. Please cover null-run schedule-history payloads in the shared retained-source inventory and regression set. This is part of the existing #911 source-completeness finding, not a new unrelated gate. |
|
Security exact-head finding on Workflow 2.15.12 treats these as live payload owners: Two isolated synthetic controls on this exact head each established a resolvable stored reference before pruning, shared it with the closed run, then observed the retention pass report both provider objects deleted while the independent owner row and exact stored reference remained:
Both ran separately with Impact: pruning one run can delete provider bytes still required by a catalog default or an already accepted/scheduled attempt. The correction needs both the current catalog binding and the immutable dispatch snapshot in the retained-source inventory (with appropriate namespace ownership/join semantics). This is scoped to #911; no unrelated work or publication is held. |
|
Implemented the remaining demonstrated retained-owner corrections at Ten added controls cover pre-prune and retry ownership, cross-namespace byte preservation, and actual owner cleanup. The complete eight-file integration run passes 197 tests / 2,685 assertions / zero skips, including all 52 controlled database interleavings. Tested source-file hashes match the pushed head. Normal published Core 2.15.12 is unchanged across all 2,259 files and 12 runtime origins before/after. The namespace query ceiling stays unchanged and passes. The earlier namespace HTTP 207/evidence, seven schedule fields and custom typed-absence replay corrections were accepted by Security. Final-head database/restart/release-contract qualification and exact Security review remain in progress; ordinary CI is active. The PR remains draft, and no published-image acceptance is claimed. |
|
Security delta review complete for exact head 494c0f6, whose runtime correction is 47ecf28. The later merge from current main adds no PHP runtime delta to this correction. The shared referenceSources inventory now drives candidate collection, namespace retained fencing, and post-commit retry fencing. I checked the previously demonstrated gaps specifically: workflow_instances memo/visibility labels; namespace-wide schedule history including null-run rows; service-operation handler bindings; and dispatch_options through the service-call target-owned namespace join. The candidate OR scopes and retained complement scopes match the rows NamespaceLifecycleCleanup removes, including service-call dispatches and update-validation tasks. The added pre-prune/retry controls also exercise preservation across namespaces followed by actual owner cleanup. Local source checks: git diff --check passes; the four reviewed file SHA-256 values are aa18c08568822e7bba3983ba96844cf716d821b36390f1f7e8113df537d9fffa, 032418eac361c42446e5a4de6383c51551abf5b7fda3776fd96eb232574abd1e, 5b54e4d0f6778bf54b47319701ccb73847bd9ce5e0abd5ec7672e55f62ce38a7, and 334ff144d39f3a2e6645d7fbadfe43187bd4291e717170d75bd5702c3c99de71 respectively. GitHub reports zero open code-scanning alerts for this PR. I found no new security defect in this delta and no Security source hold remains. The PR is still draft and exact-head CI is still running; those remain ordinary owner/merge gates, not Security blockers. |
|
rmcdaniel
left a comment
rmcdaniel
left a comment
There was a problem hiding this comment.
Maintainer review of exact 494c0f6724fc6e207e110ece72ccb42e486de164, incorporating current main cbf1937d. The post-commit deletion protocol preserves committed reference ownership, canonical object-lock/backup-hold order, per-intent failure isolation, saved-policy recovery and idempotent typed-absence acknowledgement. The single inventory and complementary namespace scopes include all demonstrated retained owners; primary pruned metadata is excluded. Security accepted this exact delta in comment6557180416.
Actual qualification on November 1 UTC:
| Profile | Retention cases | Assertions | Outcome |
|---|---|---|---|
| SQLite 3.40.1, matching-source integration subset | 28 | 293 | pass, no skips |
| MySQL 8.0.46 | 28 | 293 | pass, no skips |
| MariaDB 10.11.19, native driver | 28 | 293 | pass, no skips |
| MariaDB 10.11.19, mysql driver | 28 | 293 | pass, no skips |
| PostgreSQL 17.11 | 28 | 293 | pass, no skips |
Eight full integration files pass197/2685 with all52 database races enabled and zero skips (11:08:07–11:09:45; every tested source hash matches the reviewed head). Four final-head external DB profiles completed11:10:26–11:17:34. All three full release-image contract files plus published-PHP selectors pass141/1629 (11:17:34–11:17:52). A real second process recovers both saved-policy intentions after namespace deletion (HTTP207, namespace/configuration absent, 2 pending/files ->0 pending/files;11:17:52–11:17:58). All2259 publishedCore2.15.12 files/12 runtime origins match before/after; style passes17 changedPHP files. The existing namespace query bound remains unchanged.
Exact-head Sonar at11:10:50: gateOK,0BUG,0VULNERABILITY,0hotspots to review;8CODE_SMELL findings remain open. Specific maintenance disposition: the three complexity findings cover the ownership/grouped-scan loops and namespace release coordination, retained together to keep the complete inventory and lock ordering inspectable; functional ownership/race and query-cap controls pass. The three generic RuntimeExceptions reject absent backup coordination, staging outside a transaction, and ambiguous intent identity before deletion; they are internal invariant failures, not successful cleanup. The five-return factory uses explicit disabled/local/filesystem/unsupported branches. The unused private namespace parameter has no behavioral effect. These are accepted maintenance debt for this P1 repair, not false positives or functional/security failures; no gate/check was disabled or waived.
Ordinary hosted PHP/performance checks are still active. This review supports ready/enqueue through the normal protected gates; it does not claim a merged change, fixed published image or installed acceptance.
… the prune commits Run retention deleted a pruned run's provider objects inside the prune's transaction, so a later SQL or commit failure rolled back the registrations but not the deleted bytes (Security, durable-workflow#325 comment 6555755439). Native now follows published Server 2.11.8 (durable-workflow#911, durable-workflow#789): a prune records each object's deletion in its transaction, under the object lock, with the namespace's storage policy, and deletes the object only after the commit. A provider failure leaves the deletion pending; the retention and cleanup passes retry it first, and a new registration of the object fences it. Schema version 8 adds the native-owned dw_external_payload_deletions on SQLite, PostgreSQL, MySQL and MariaDB, with catalogs captured from MySQL 8.0.46, MariaDB 10.11.19 and PostgreSQL 17.11. The retention pass reports external_payload_cleanup; the cleanup routes report retention_cleanup and the pass answers 207 while a deletion is pending. Namespace delete now settles the namespace's external payloads (from durable-workflow#908) on the same model: both passes record deletions, the answer carries external_payload_cleanup and is 207 while any is pending, and a cleanup pass also retries deletions of deleted namespaces, which native has no scheduled command to reach. Regressions mirror Security's counterfactual: a disposable trigger aborting the registration delete rolls back the prune or namespace delete with the object, registration and state intact. Both fixtures pass unchanged against a fresh published 2.11.8 harness; the namespace delete probe also retries the SQLite reference's backend_lock_pressure refusal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…bases Server 2.11.8 (Core 2.15.12) was published today, and native refused every 2.11.8 database as unqualified. Core 2.15.12 adds no migration; 2.11.8's only change is Server's runtime_external_payload_deletions table (durable-workflow#911, durable-workflow#789). Two layouts record it, php-2.11.8 from php-2.11.3 and php-2.11.8-from-2.9.7 from php-2.11.3-from-2.9.7. The deltas were captured from real published-image databases on SQLite, MySQL 8.0, MariaDB 10.11 and PostgreSQL 17 (fresh, and 2.9.7 migrated), and native's upgrade-inspect then named every real 2.11.8 database by its layout. Takeover carries each pending deletion over to native's dw_external_payload_deletions with its id, object, saved policy, attempts, failure and times, so native retries it as PHP does; it deletes no object, then drops PHP's table. Inspection reports external_payload_deletion_inspection, and takeover refuses before any change while a pending deletion names a storage driver native does not serve or a failure PHP does not record. MySQL and MariaDB run it as resumable journal step 11. A php2118 CI cohort takes over a published 2.11.8 source end to end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Retention could delete provider bytes, fail on a later provider operation or database update, and roll back references to objects that were already gone. Record deletion intentions with released references, commit pruning, then remove provider bytes. Failed cleanup remains inspectable and retryable, including after namespace deletion using the saved storage policy.
Use one retained-source inventory across collection, namespace cleanup and retry fencing. It covers committed run/stream state, instance memo and labels, all schedule fields and pre-run schedule history, service catalogs, target-owned dispatch snapshots and validation tasks. Canonical object locks precede the scan and backup hold; new ownership preserves bytes. Typed already-absent custom-provider deletion settles replay. Namespace deletion and explicit cleanup report HTTP207 while provider work remains; the command reports pending work with a nonzero exit.
Merged through the protected queue as
fa3fc2f2f6b8935ea4a6eea47f6a9221e2adafffand delivered in PHP Server2.11.8, with successful image and matching chart publication.Validation:
This delivers the rollback-integrity repair in #789. Its original SDK/Nexus ingress/recovery, caller-completion409 and pre-kill evidence remain open in that issue. No Cloud deployment is claimed.