[rmcdaniel]

Phase A of #930. It resolves the two availability blockers Security found on Server #766 (durable-workflow/server#766 (comment)), as the owning package contract the Rust port will consume. It does not complete #930. Phase B adds exact pre-write row accounting for the state-driven cascades and the other command paths, and #930 isn't done until it lands. The accepted design is in #930 (v3 6402911532, with the additions in 6402936400). docs/architecture/completion-budget.md describes what this change does.

The second commit answers Security's exact-head review of 287bfe7d (#934 (comment)), point by point below.

What it does

• Timing caps. duration_ms and heartbeat elapsed_ms are capped at 7 days and attempt backoff_seconds at 1 day, each with its own 422 field error. The walk back from completion is summed with checked arithmetic and refused (local_activity_timestamp_out_of_range) if it would reach before the earliest storable timestamp.
• Meter. HistoryTransactionMeter gives every complete() attempt, and each of checkpointCommands' five attempts, a fresh meter:
  • WorkflowHistoryEvent::record() charges each event, its bytes in the HistoryBudget measure, and two rows. Local activity rows are charged before each write.
  • The limits are hard StructuralLimits (0 disables): history_transaction_size (existing, 5,000), history_transaction_bytes (16 MiB), history_transaction_rows (20,000) and external_payload_transaction_bytes (16 MiB). All four appear in StructuralLimits::snapshot(), so the health check, operator metrics and backend capabilities publish them.
  • In Phase A, history_transaction_rows counts history events with their run positions, and local activity rows. Other command writes are not counted until Phase B. The doc, changelog, config comment and enum say so.
  • A refusal trips the meter, rolls back and keeps the lease. A swallowed refusal still refuses before commit, and an unmeasurable event fails closed.
• Lifecycle events after commit. Each metered attempt collects the lifecycle events its body dispatches (workflow completed, failed or continued, activity failed, failure recorded). They are dispatched after DB::transaction() returns, or after the caller's outer transaction commits. A refused attempt, a failed staged put, a failed commit or a retried attempt dispatches none. Collection is scoped to each attempt: a nested attempt that fails is discarded even if the enclosing attempt commits.
• Deadline close. A run's deadline close is unmetered. No other path closes a run while its task is open, so metering it could keep an expired run open forever.
• Early check. After any deadline close and before the worker's first write, the local activities' exact events and rows are checked; other commands count their lower bound. A failure refuses with completion_budget_exceeded.
• External payloads. Every externalized payload is checked against payload_size_bytes and charged to external_payload_transaction_bytes before it is staged or stored.
  • Opt-in staging. A driver that implements the new PredictableExternalPayloadStorageDriver has its payloads staged under the predicted URI and stored only after every limit has passed, before commit. A refusal stores nothing.
  • Collisions. The stage is keyed by driver and URI. If one driver predicts the same URI for different content, hash or codec, the completion is refused with external_payload_staged_uri_collision.
  • Existing drivers. A driver that implements only ExternalPayloadStorageDriver keeps working as before, storing each payload when it is externalized, after the charge. A refused completion can leave those objects unreferenced, at most external_payload_transaction_bytes per attempt.
  • Built-in drivers. The local driver implements the predictable contract: it keeps an existing object only if its length and hash match, and otherwise moves a copy into place only after checking its exact length and hash. The filesystem driver does not claim the contract, since a disk does not guarantee atomic writes.

Security review of 287bfe7d

1. Lifecycle events before commit: fixed by collect/release (above). Covered by Event::fake tests: zero events after a later byte refusal and after a failed staged put, then exactly one each after acceptance. A listener check shows the transaction level is 0 when the event is dispatched. A retried transaction dispatches only the committed attempt's events.
2. Stage collisions: keyed by driver instance and URI, failing closed on any mismatch. Covered by tests for different content, the same content with a different codec, and two drivers predicting one URI.
3. Existing driver compatibility: the predictable interface is now optional. An old-interface driver completes through the bridge and stays within the byte limit when refused.
4. Snapshot: all three new limits are asserted on StructuralLimits::snapshot(), HealthCheck, OperatorMetrics, and BackendCapabilities configured and effective.
5. Row limit claims: narrowed to what Phase A meters.
6. Driver contract: the local driver's guarantee is enforced. Tests cover short writes, a short write reported as whole, same-length wrong bytes and a failed write: none is moved into place or leaves a temp file. Matching objects are kept, partial and corrupt ones are replaced, and content that doesn't match its hash is refused. The filesystem driver no longer advertises the contract.

Hosted gates:

• The architecture doc has no issue references.
• The replay corpus grows by portable-local-activity-timing-caps-cold-replay. It cold-replays a local activity at the caps (7-day attempts, a 1-day backoff). The embedded consumer checks that the bridge records exactly that walk at the caps. It also checks that one past each cap is refused by that field alone, with nothing written.
• Locally, the validator's counterfactual passes: the fixture fails the official consumer at the base revision and passes at this head.

Tests

• HistoryTransactionMeterTest, 14 tests: the exact measure; exact and one-over limits; disabled limits; a swallowed refusal; overflow-checked aggregates; fail-closed measure; staging with zero puts on refusal, dedup with per-occurrence charging and a fresh stage per attempt; raw per-payload and per-transaction limits; a mispredicted URI and a failed put; URI collisions; two drivers; unstaged charges; meters don't nest.
• DefaultWorkflowTaskBridgeLocalActivityTest, 12 tests, adding: an old-interface driver within the byte limit; lifecycle events only after commit; listeners at transaction level 0; a failed staged put dispatches nothing.
• LifecycleEventDispatcherCollectTest, 6 tests: a retried transaction, a failed attempt, release inside an outer transaction (commit and rollback), a nested attempt that rolls back under an outer attempt that commits, a nested transaction retried under an outer collector, and an enclosing attempt that fails.
• ExternalPayloadStorageTest: incomplete and corrupt local writes, replacing damaged objects, hash mismatch, and which drivers claim the contract.
• StructuralLimitsTest: the three limits on every operator surface.
• LocalActivityCommandNormalizationTest: each timing field at its maximum, one over and PHP_INT_MAX; the exact event check.
• Local results:
  • 232 targeted tests pass, including both replay corpus consumers.
  • A wider cohort of 735 tests (lifecycle, legacy events, StateChanged, bridge, checkpoints, cancellation scopes, structural limits, health, metrics, updates, side effects) has seven failures. All of them also fail at the previous head in this sandbox: five need Redis, one is a file-backed SQLite lock, and one is a SQLite-only capability note.
  • phpstan and ecs pass, and so do the corpus validator (static and counterfactual) and its policy tests.

Related

• Native port: Server Deliver cooperative cancellation to completion-waiting Nexus callers #766 (record_local_activity) consumes these constants and formulas exactly.
• Found by the audit and fixed separately: Service operations accept worker-chosen namespace, principal and service_call_id #931 (service-operation authority), Completions sleep inside their transaction while holding locks #932 (sleeps inside the transaction), External-worker completions bypass the pending-count limits #933 (external-worker pending-count limits).

🤖 Generated with Claude Code

[rmcdaniel]

Security review of exact head 287bfe7 requests changes before merge.

1. LifecycleEventDispatcher calls still run synchronously inside the database transaction and before HistoryTransactionMeter finishes all charges and staged payload puts. A later limit refusal, staged put failure, deadlock retry, or commit failure rolls durable state back after public listeners have observed workflow completion/failure, activity failure, or the continued run start. Defer every lifecycle dispatch until the outer commit succeeds, with retry-safe tests proving zero events after a later meter refusal and staged-put failure and exactly one after a successful/retried commit.

2. HistoryTransactionMeter::stage indexes the stage only by predicted URI and keeps the first entry. If one driver predicts the same URI for different content, both references are written but only the first object is stored; the second reference carries a hash/size that can never validate. Two distinct driver instances returning the same URI are also collapsed. Include driver identity in stage identity and fail closed on any same-driver URI collision unless URI, hash, codec, and bytes agree. Cover different-content collision and same-URI/two-driver cases.

3. This is an upgrade regression for the existing stable Contracts API. A custom ExternalPayloadStorageDriver that works today is refused for every above-threshold payload inside complete/checkpoint because the meter now requires the new PredictableExternalPayloadStorageDriver subtype. Contracts are documented as the public extension surface. Preserve existing implementations through an opt-in/capability rollout or provide an explicit compatible migration boundary, and add upgrade coverage using an old-interface driver.

4. StructuralLimits::snapshot omits history_transaction_bytes, history_transaction_rows, and external_payload_transaction_bytes. HealthCheck, OperatorMetrics, and BackendCapabilities all publish that snapshot, so operators cannot see three newly enforced ceilings. Include them and assert all three public surfaces.

5. The public docs and changelog call history_transaction_rows a hard whole-completion ceiling, but Phase A charges only history and local-activity rows. Other command writes can exceed the configured limit while the completion commits. Either narrow the exposed guarantee/config semantics until Phase B or finish the accounting before advertising it as hard.

6. The new Predictable driver contract requires atomic, idempotent, hash-verified puts, but the built-ins do not yet prove that contract. The local driver treats any non-false file_put_contents result as complete and renames without checking exact byte count or hashing the temporary file. The generic Laravel Filesystem abstraction does not guarantee atomic replacement and the implementation does not read back/verify. Make the guarantee true for every implementation that advertises this interface, with partial/corrupt-write tests, or narrow the interface/driver claims.

Hosted evidence is also red at this head: Public Boundary rejects the cross-tracker references in docs/architecture/completion-budget.md, and Validate repository contracts reports replay implementation changes without corpus growth (73 to 73). The downstream build matrix was skipped. Please rerun the full required matrix on the corrected exact head.

I did not run application tests locally. This review is based on the exact source tree, clean diff checks, and the hosted results above.

[rmcdaniel]

Answer to the exact-head review of 287bfe7d (#934 (comment)). New exact head: 938fd40d34680bd4d860d8c2cc3f2272517fe9c4. Phase A only; #930 remains open for Phase B.

1. Lifecycle events before commit. Both metered paths, complete() and the five-attempt checkpointCommands, now run through one meteredTransaction() helper. Each attempt runs inside LifecycleEventDispatcher::collect(), which holds back every lifecycle dispatch its body makes. That includes the unmetered deadline close and continue-as-new start. release() dispatches them after DB::transaction() returns, or through afterCommit inside a caller's still-open transaction. A failed attempt's events are discarded with it, and nothing is released after a failed commit, since DB::transaction() throws. Event scalars are captured at the call, as before. Tests:
   • Event::fake: zero ActivityFailed, FailureRecorded and WorkflowCompleted after a later byte refusal; exactly one each after the same completion is accepted.
   • Zero WorkflowCompleted after a failed staged put, then exactly one after acceptance.
   • A real listener sees transaction level 0.
   • A deadlock-retried transaction dispatches only the committed attempt's events.
   • An outer rollback discards released events.
2. Stage collisions. The stage key is the driver instance (spl_object_id; the stage holds the driver, so the id cannot be reused while staged) plus the URI. A second staging under the same key must match sha256, codec and bytes, or the meter trips with external_payload_staged_uri_collision and nothing is stored. Two drivers predicting one URI keep separate entries, and each stores its own object. Tests cover different content, the same content with another codec, the deduplicated identical case, and two drivers.
3. Existing driver compatibility. PredictableExternalPayloadStorageDriver is now an opt-in capability. A driver implementing only ExternalPayloadStorageDriver keeps its earlier behavior: it stores the payload when it is externalized. The new chargeUnstaged() applies the same per-payload and raw-byte limits before that put. Orphans from a later refusal are bounded by external_payload_transaction_bytes per attempt, and the docs state it. Upgrade coverage uses an old-interface driver: a completion is accepted and resolves the stored object; with room for one payload, the second is refused before its put, and only the first was stored.
4. Snapshot. StructuralLimits::snapshot() includes the three new limits. One test sets distinctive values and asserts each on StructuralLimits::snapshot(), HealthCheck::snapshot(), OperatorMetrics::snapshot(), and BackendCapabilities configured and effective.
5. Row limit claims. The docs, changelog, config comment, enum and StructuralLimits docblock now say that Phase A's history_transaction_rows counts history events with their run positions, and local activity rows. Other command writes are not counted until Phase B, and until then the pending-count limits bound them.
6. Driver contract. The interface lists the guarantees: put returns uriFor(), refuses a hash mismatch, never leaves a partial or corrupt object at the URI, and is idempotent. The local driver makes them true:
   • It keeps an existing object only if its exact length and SHA-256 match.
   • Otherwise it writes aside, checks the written count, size and hash of the temporary file, and renames it into place. The temporary file is removed on every failure.
   • Negative tests inject a short write, a short write reported as whole, same-length wrong bytes and a failed write. Each is refused, leaves no object and no temporary file, and a damaged existing object (partial or same-length corrupt) is replaced.
   • The generic filesystem driver is reverted to the original interface, since a disk does not guarantee atomic replacement.

Hosted gates.

• The architecture doc has no issue references; scripts/check-public-boundary.sh passes.
• New replay evidence: portable-local-activity-timing-caps-cold-replay, a local activity at the caps (two 7-day attempts and a 1-day backoff).
• The embedded consumer checks that the bridge records exactly that walk at the caps. It also checks that one past each cap is refused by the normalizer on that field alone, the bridge refuses, and nothing is written.
• Locally, in an ephemeral container copy, validate-regression-corpus.py passes both with --skip-counterfactual and with the counterfactual enforced. The fixture fails the official consumer at base b2a33691 and passes here. test-regression-corpus-policy.py passes: 67 tests.

Local verification at this head.

• 232 targeted tests pass.
• A wider 735-test cohort has seven failures, each also failing at 287bfe7d in this sandbox: five need Redis, one is a file-backed SQLite lock, and one is a SQLite-only capability note.
• phpstan and ecs are clean.

The full hosted matrix runs on this head.

[rmcdaniel]

Security review of exact head 938fd40d34680bd4d860d8c2cc3f2272517fe9c4 confirms that the six findings on the prior head are materially addressed. One new rollback edge in the event-deferral implementation still needs correction before merge.

LifecycleEventDispatcher::collect() does not create an isolated child scope when a collector is already active. It runs the nested body directly into the parent collector and returns an empty child list. If that nested database/savepoint attempt emits a lifecycle event and then throws or rolls back, an outer body can catch the exception and continue; the rolled-back event remains in the parent list and is later released after the outer commit. A retried inner transaction under an outer collector can likewise retain events from failed attempts and publish duplicates.

This is not only a hypothetical private call shape: the new API and testANestedCollectAddsItsEventsToTheOuterAttempt() explicitly support nesting, while meteredTransaction() calls collect() inside its transaction callback. A host-level collector around a bridge completion therefore defeats the promised “a refused, failed or retried attempt dispatches none” boundary.

Please make each collect() call attempt-scoped (for example, a collector stack): discard the child list when its body throws, and merge/release only a successful child after the corresponding transaction/savepoint succeeds. Add old-source-failing controls for (1) a nested attempt that emits, rolls back, is caught by the outer body, and whose outer transaction commits, and (2) a retried nested transaction under an outer collector. Both must publish only events from committed attempts.

The hosted matrix is still in progress at this review point. Security acceptance remains pending this correction and a terminal green exact-head matrix.

[rmcdaniel]

Answer to #934 (comment). The new exact head is 279900bed0ed47374459e3eb79bd176195895188.

LifecycleEventDispatcher::collect() now keeps a stack of collectors, one per attempt:

• Each call pushes its own list, and dispatches made inside it go to the innermost list.
• If the body throws, that attempt's list is spliced off and discarded before the exception continues. A nested attempt that fails or retries therefore leaves nothing behind, even when the enclosing attempt catches the failure and commits.
• A body that returns gets its own list back. release() hands those events to the enclosing collector when there is one, so they are dispatched or discarded with the enclosing attempt. With no collector open, release() defers to the caller's open transaction through afterCommit, or dispatches immediately when there is none.

Controls, in LifecycleEventDispatcherCollectTest:

1. testANestedAttemptThatRollsBackPublishesNothingWhenTheOuterCommits: the outer attempt emits WorkflowCompleted, and a nested DB::transaction/collect emits WorkflowFailed and throws. The outer body catches it and commits. Result: WorkflowCompleted once, WorkflowFailed never.
2. testARetriedNestedTransactionPublishesOnlyItsCommittedAttempt: inside an outer collector, a nested DB::transaction/collect emits and fails on attempt 1, then succeeds on attempt 2. The outer collector holds exactly one event, dispatched once on release.
3. testAnEnclosingAttemptThatFailsDiscardsItsNestedEvents: a committed nested attempt's released events are discarded when the enclosing attempt fails, and no collector is left open afterwards.

Both of your controls (1 and 2) were run against 938fd40d with this test file and fail there: "The unexpected [WorkflowFailed] event was dispatched" and "actual size 2 matches expected size 1". At this head the 6 dispatcher tests and the 32-test bridge/meter/dispatcher cohort pass, and phpstan and ecs are clean. The hosted matrix is running on this head.

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.7% Duplication on New Code

See analysis details on SonarQube Cloud

[rmcdaniel]

SonarCloud decision at 279900be: the quality gate is OK, with 0 bugs, vulnerabilities and hotspots. It reports four MAJOR maintainability findings. All four are on the two existing transaction closures, now wrapped by meteredTransaction():

• checkpointCommands closure (S138 at 189 lines, S1142 at 20 returns). These are the same findings main already carries at line 1537. The closure body is unchanged apart from the wrapper and indentation.
• complete() closure (S1142 at 13 returns, S138 at 184 lines). Main reports 12 returns at line 1237. This PR adds one return, the completion_budget_exceeded early check. It also adds the two HistoryTransactionMeter::unmetered() deadline-close wrappers, which take the closure past 150 lines.

Decision: accepted for Phase A rather than splitting the closure now. Each return is one of PHP's ordered refusal answers, and their order is the contract. Moving them into helpers would reorder or obscure the answer sequence in the same change that introduces the budget. Phase B restructures these same write paths for pre-write row accounting, and that is where the closure should be split into the ordered steps (claim checks, deadline close, early check, command application). No reliability or security finding is open.

[rmcdaniel]

Security accepts exact head 279900bed0ed47374459e3eb79bd176195895188 for the Phase A boundary reviewed here.

The six findings on 287bfe7d remain materially addressed, and the nested rollback/retry blocker on 938fd40d is corrected: every collect() call now owns an attempt-local scope, a throwing attempt discards that scope, and only a successful child is merged into its still-live parent or released after commit. The added controls cover a caught nested rollback, a retried nested transaction, and failure of an enclosing attempt; the first two were also demonstrated to fail against the prior head.

Hosted evidence is terminal green for this exact head, including pr-unit-contracts, pr-feature-mysql, regression corpus, repository/public-boundary contracts, quality/static analysis, CodeQL, Sonar, and the final build aggregation gate (run 40240365450).

No Security blocker remains for this exact Phase A head. This acceptance does not cover a later head, does not close or pre-approve Phase B in #930, and does not authorize deployment. I did not run application tests locally; the acceptance is based on exact-source review plus the terminal hosted evidence above.

[rmcdaniel]

Merged as 4eb49a20290d48654085696326fc4fe3ad2b19af, pinned to the Security-accepted exact head 279900bed0ed47374459e3eb79bd176195895188 (#934 (comment)).

• Parents are b2a33691 (main) and 279900be. The merge tree equals the head tree.
• The hosted matrix at the head was terminal before the merge: 16 checks succeeded and 7 main-only jobs were skipped.
• SonarCloud's gate was OK; the decision on its findings is recorded at Bound and meter workflow task completions (#930, phase A) #934 (comment).
• Branch fix/930-local-activity-bounds deleted.

This is Phase A only, and #930 stays open for Phase B. No release is implied. The native Server port of this rule is going into Server #766.