Repository navigation
Bound local-activity timestamps and completion-wide persistence work #930
Copy link
Copy link
Open
Description
Problem
The package-owned record_local_activity contract has two availability boundaries that are unsafe when a workflow-task completion is treated as a whole:
duration_ms, heartbeatelapsed_ms, and retrybackoff_secondsaccept every non-negative platform integer, but the contract defines no safe timestamp range. The Rust Server port at Rust #325: record local activities like Server 2.11.1 server#766 reaches unchecked Chrono construction/arithmetic with these values and can unwind the request handler instead of returning a deterministic protocol refusal. The PHP implementation uses the same unbounded values in Carbon timestamp arithmetic, so the owning contract needs one portable bound and explicit refusal behavior.- The limits of 100 commands per completion and 1,000 heartbeats per local-activity command compose to at least 100,300 history events in one transaction. Minimal JSON for that batch fits under the default request-body ceiling. Each heartbeat history event also repeats the running activity snapshot, including
arguments, so a large valid payload can be persisted hundreds or thousands of times. Current history-budget thresholds are reporting/advisory and do not refuse this write before mutation.
Security review evidence and exact Rust line references: durable-workflow/server#766 (comment)
Required contract and implementation outcome
- Define portable maximums or checked conversion rules for all three timing fields. Extreme and out-of-range values must receive a stable 4xx protocol refusal, never an exception/panic.
- Preflight a deterministic whole-completion budget for projected durable events/rows and serialized history bytes before any mutation. Per-command heartbeat limits alone are insufficient.
- Either stop repeating large immutable payloads in every heartbeat snapshot or include all repeated bytes in the admission budget without changing replay semantics accidentally.
- Keep ownership/capability checks first where the existing contract requires them, but complete all resource/timestamp validation before the first durable write.
- Add exact-boundary and one-over tests for timing, aggregate heartbeat/event work, and a large valid argument with heartbeats. Refusals must preserve the task lease and leave executions, attempts, history, projections, and receipts unchanged.
- Qualify the package change through the PHP Server and the Rust port on SQLite, PostgreSQL, and MySQL. The Rust port must consume the package-owned rule rather than inventing a native-only limit.
Do not treat continue_as_new_recommended as enforcement; this admission guard protects the current transition before it can create the oversized history.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels
Activity
rmcdaniel commentedon Oct 25, 2026
Design for review before implementation. I own this under Server #325. The package ships the rule first; the PHP Server and the Rust port consume it.
1. Timing maxima. These are new public constants on
WorkflowCommandNormalizer, published inlocalActivityCommandContract(). Each out-of-range value gets the normalizer's ordinary 422 field error.attempts.*.duration_msattempts.*.heartbeats.*.elapsed_msattempts.*.backoff_secondsThe bridge back-dates each attempt from
now()by at most its duration plus the following backoff. With at most 100 attempts, the worst case is about 800 days. That stays representable in Carbon, in Chrono, and in every backend's timestamp column, including MySQLTIMESTAMP, which cannot go below 1970. No arithmetic can overflow before validation has run.2. Whole-completion event budget. Before the first write, the bridge projects the history events of the whole completion:
record_local_activitycommand counts 1 scheduled event, then per attempt 1 started, its heartbeats and a retry if it isn't the last attempt, then 1 terminal event;If the total exceeds the existing
StructuralLimits::historyTransactionSizeLimit()(workflows.v2.structural_limits.history_transaction_size, default 5,000), the completion is refused with the existing structural-limit refusal. That is the same guard the embeddedWorkflowExecutoralready applies to one transaction. The rule's budget is shared, not native-only, and the Rust port reads the same config and default.3. Repeated snapshot bytes. Every local-activity event repeats the activity snapshot, including the encoded
arguments. Before writing, the bridge projectsevents_c × strlen(arguments_c)for each command and sums it across the completion. If the sum exceedsHistoryBudget::sizeBytesHardThreshold()(default 5 MiB), the completion is refused. The arguments are compared as the stored encoded strings, so PHP and Rust count identical bytes.I'd rather budget the repetition than drop
argumentsfrom the heartbeat snapshots. Dropping them would change the published history shape, and every replay and export consumer would have to follow.Order. Ownership and capability checks still come first. Then the normalizer runs, then the budget, then the first write. On refusal there is no execution, attempt, history or projection row, and no receipt, and the lease stays usable.
Tests. Each timing field at the maximum and at one over, plus a value equal to the platform's largest integer. The event budget at exactly 5,000 and at 5,001. A large valid argument with heartbeats at the byte boundary and one over. A refusal followed by the same task completing normally. These run on SQLite, PostgreSQL and MySQL in the package, then through the PHP Server and the Rust port (Server #766).
Please say if any constant or the byte measure should differ before I write it.
🤖 Generated with Claude Code
rmcdaniel commentedon Oct 25, 2026
Security design review: the timing caps are a reasonable portable envelope, but the proposed byte/event admission design needs correction before implementation.
HistoryBudget::sizeBytesHardThreshold()is currently an advisory continue-as-new threshold. Reusing it as a hard protocol refusal silently changes an operator-facing configuration from guidance to admission control. Add an explicitly hard structural byte limit with documented zero/custom semantics, or make the compatibility and migration decision explicit rather than repurposing the advisory key.sum(events_c * strlen(arguments_c))is not serialized history-byte accounting. It omits event type and JSON structure, the repeated activity and attempt snapshots, heartbeatdetails, retry messages and metadata, terminal result/failure data, and events emitted by other commands plus signal-resume, condition-resolution, waiting, and terminal handling. Preflight every history event the transaction can emit with the same canonical measure used byHistoryBudget(strlen(event_type) +canonical serialized payload bytes), or use a documented conservative upper bound that cannot undercount.recordAppliedSignalForSignalResume()andrecordSatisfiedConditionWaitForSignalResume(), because those can mutate before the current command loop. Boundary tests should cover exact/one-over event and canonical-byte totals, aggregate non-history rows, custom and disabled configuration semantics, arithmetic overflow, every timing field, and a refused completion followed by successful reuse of the unchanged lease on all three databases.Please revise the design before writing the package implementation. The Rust port should consume the resulting published constants and formulas exactly.
rmcdaniel commentedon Oct 25, 2026
Revised design after review 6402480418. Nothing is published, and the earlier draft stays unpushed.
1. Timing fields (unchanged caps, checked math)
attempts.*.duration_msattempts.*.heartbeats.*.elapsed_msattempts.*.backoff_secondsThese are public
WorkflowCommandNormalizerconstants. An out-of-range value gets its own 422 field error, even on a retried attempt, where today the missing-backoff message would replace it.Portable bound. The bridge back-dates attempts from the completion instant by at most
MAX_LOCAL_ACTIVITY_ATTEMPTS × (604,800,000 + 86,400 × 1,000)= 69,120,000,000 ms.PHP_INT_MAX; Rust useschecked_*.local_activity_timestamp_out_of_range. The minimum is1970-01-01T00:00:01Zfor MySQLTIMESTAMPand0001-01-01elsewhere.TimeDelta::try_millisecondsandchecked_sub_signed.2. Whole-completion meter, in the history writer itself
Projecting every path ahead of time would re-implement the bridge's branches (signal resume, condition resolution, waiting, every command, terminal handling and parent notifications) and could drift from them. Instead, the completion transaction carries a meter that charges what is actually written:
WorkflowHistoryEvent::record()is the only PHP writer of history, and native'sappendis the only Rust one. Each charges one event, plus its bytes in theHistoryBudgetmeasure:strlen(event_type)plus thejson_encode(payload, JSON_UNESCAPED_SLASHES|JSON_UNESCAPED_UNICODE)length.completed: falsewithreason: history_transaction_limit_exceededand names the dimension. The task row's lease change is rolled back too, so the same lease can complete a smaller completion.recordAppliedSignalForSignalResume()andrecordSatisfiedConditionWaitForSignalResume(), so it covers them.Work before a refusal is bounded by the limits themselves. Results are deterministic: the same input reaches the same limit at the same write, and both implementations charge the same events, bytes and rows.
Limits. All are hard
StructuralLimits. A value of 0 disables that dimension, as every structural limit documents.structural_limits.history_transaction_sizestructural_limits.history_transaction_bytesDW_V2_LIMIT_HISTORY_TRANSACTION_BYTES/WORKFLOW_V2_LIMIT_HISTORY_TRANSACTION_BYTESstructural_limits.history_transaction_rowsHistoryBudgetstays advisory.The 16 MiB default allows several MiB of legitimate payloads plus thousands of ~1.3 KiB fixed snapshots. The row default is four per event at the event limit.
3. Cheap preflight for the amplifier
The normalizer still refuses, as a 422 before the transaction, a completion whose local activities alone exceed the event limit. That count is exact:
2 + attempts + heartbeats + (attempts − 1)per command, summed with checked addition. The meter remains the authority for every other path and for bytes and rows.Tests (package, then PHP Server and Rust on SQLite, PostgreSQL and MySQL)
PHP_INT_MAX/i64::MAX. The aggregate and storage-minimum guards are also driven directly.0) limits.The Rust port takes the same constants, the same measure and the same configuration keys.
🤖 Generated with Claude Code
rmcdaniel commentedon Oct 25, 2026
Security review of the revised design: the new hard StructuralLimits, writer-level event/byte meter, pre-signal placement, checked timing math, and advisory-only HistoryBudget resolve the prior findings. Two boundaries still need to be explicit before implementation.
ExternalPayloads::externalizeForNamespace()before their first model/history write;applyRecordLocalActivity()externalizes arguments, result, and exception first. That callsExternalPayloadStorageDriver::put(), which is outside the database transaction. A later byte/row/event refusal can therefore leave a durable object even though every database row and the lease roll back. The returned URI can also change the eventual serialized event size. Do not delete blindly on failure because a content-addressed object may already be shared. Either complete a conservative admission pass before anyput, or add an explicit transaction-aware object staging/promotion contract with safe reference accounting and rollback. Add a fake-driver regression proving a refused completion performs no put or leaves no orphan across every externalizable command path.The timing section and exact local-activity event formula are accepted for implementation once those two boundaries are incorporated. Keep limit exceptions outside the transaction body so nothing catches one and continues, and fail closed if the canonical JSON byte measure cannot be produced.
rmcdaniel commentedon Oct 25, 2026
Design v3: this addresses the two remaining boundaries in 6402567710. The timing caps, the exact local-activity event formula, the hard
StructuralLimitsand the writer-level event/byte meter stay as accepted in v2.1. No external put before admission: staged, content-addressed references
Both built-in drivers derive the URI from the content alone, so the reference is known before anything is written:
FilesystemExternalPayloadStoragebuildsscheme://authority/[prefix/]codec/aa/sha256.LocalFilesystemExternalPayloadStoragebuildsfile://root/codec/aa/sha256.The change:
PredictableExternalPayloadStorageDriver extends ExternalPayloadStorageDriverwithuriFor(string $sha256, string $codec): string, and implement it on both drivers. The URI the method returns must equal whatput()returns for the same input. A test enforces that for both drivers and for prefixed and unprefixed configurations.ExternalPayloads::externalizeForNamespace()does not callput(). It computes the SHA-256 and the final stored envelope fromuriFor(), and registers the bytes with the transaction's stage. The rows and events carry the final reference, so the meter charges the exact bytes that will be committed.put()must return the predicted URI; a mismatch throws and rolls back.uriFor()fail closed. A metered completion that would externalize through one is refused before any put withreason: external_payload_staging_unsupported. Below the threshold, nothing is externalized and nothing changes.2. Rows: known cardinality, charged before each write
The meter's row dimension charges each completion write site its row count before the statement runs, rather than counting ORM calls:
complete()and from the signal-resume and condition paths, including query-builder, raw and bulk writes, in a table in the PR. Each entry names its file, its line and how its cardinality is charged. A test compares the meter's row total with actual per-table row deltas for single-row, bulk and cancellation completions. The Rust port charges the same sites with the same counts.Exceptions and failure modes
DB::transactioncallback. No inner code catches and continues.json_encodefailure), the meter fails closed withreason: history_transaction_measure_unavailable, instead of falling back toserialize().Implementation starts once you accept or adjust this.
🤖 Generated with Claude Code
rmcdaniel commentedon Oct 25, 2026
Security review of design v3: URI prediction, zero-put limit refusal, fail-closed custom drivers, and precharged write-site cardinality resolve the two original boundaries in principle. Three details must be added before implementation.
With those additions, v3 is accepted for implementation. Keep the meter/stage transaction-local, throw limit/measure failures outside inner catches, and preserve unchanged-lease reuse on refusal. No live or production action was performed.
rmcdaniel commentedon Oct 25, 2026
Write-site audit of
DefaultWorkflowTaskBridge::complete()for the row and event accounting (read-only, against the package atb2a33691plus my local, unpushed #930 draft). Bridge line numbers come from one snapshot, so the method names are the stable references.What changes in the #930 plan
The fixed "at most 4 events" for other commands is wrong. Several commands and every terminal path write state-driven numbers of events:
ParentClosePolicyEnforcer, which cascades into open children recursively;cancel_selection_operation(up tomember_sizeleaves, each a full child cascade);start_service_operation(≤10 handler attempts, each a nested start, signal or update);The writer-level meter charges all of these as they're written. The normalizer's early check will count local activities exactly and every other command as one event, a lower bound, and will be described that way.
checkpointCommands()(thecheckpointLocalActivity*andcheckpointCancellationScopePrefixentry points) runs signal resume and command application outsidecomplete(). It gets the same meter.Deadline paths inside
complete()(timeoutIfDeadlineExpired,cancelIfCleanupDeadlineExpired) do state-driven bulk work. Today it commits even whencomplete()answerscompleted: false. Under the meter, a run whose deadline close exceeds the budget would roll back on every completion attempt. Before choosing between metering them and leaving them to the unmetered watchdog close, I'll confirm the watchdog still closes such runs.Rows not tied to events:
CommandSequencebackfill;The precharge table in the PR will list each, with its bound.
Findings outside #930's scope (for Security triage)
start_service_operationpasses worker-chosennamespace,caller_namespace,principal_*(roles and claims included) andservice_call_idfrom the command into the service control plane. This may let a worker choose authorization context or namespace, or update another caller's service call.start_service_operationretries sleep for their backoff inside the completion transaction, holding the task and run locks. A synchronous update wait polls and sleeps for a worker-chosenwait_timeout_secondswith no upper bound.projectRunBestEffortand the parent-close enforcer catchThrowable. The meter's tripped flag still refuses before commit, and the PR will include a test proving that.I'll take the #930 items in that order. The separate findings need owning issues.
🤖 Generated with Claude Code
Answer the completion budget review (#930, phase A)
Scope lifecycle event collection to each transaction attempt (#930, p…
Merge pull request #934 from durable-workflow/fix/930-local-activity-…
rmcdaniel commentedon Oct 26, 2026
Phase A is done on both sides.
4eb49a20.086ba7fafa9bf76eb10a28d537ef3b5b448dea02.Found by the native acceptance matrix. A MySQL summary query that filesorted whole history rows ("Out of sort memory" for any event over 256 KiB) is fixed in the native Server.
This issue stays open for Phase B: pre-write
remaining + 1accounting for the cascades and the other command paths, plus #933's pending-count bounds and a bounded recovery path for oversized legacy cleanup.