Repository navigation
fix(testing): match replay state to the service #795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ae27200
c3d7ae1
3bf2bfa
f8ac9b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,9 @@ | |
| ) | ||
|
|
||
| from aws_durable_execution_sdk_python_testing.clock import real_now | ||
| from aws_durable_execution_sdk_python_testing.operation_state import ( | ||
| prune_replay_children, | ||
| ) | ||
| from aws_durable_execution_sdk_python_testing.exceptions import ( | ||
| IllegalStateException, | ||
| InvalidParameterValueException, | ||
|
|
@@ -479,15 +482,11 @@ def record_invocation_completion( | |
| ) | ||
|
|
||
| def mark_state_delivered(self) -> None: | ||
| """Reset the list of changed operations once an invocation's input | ||
| carries them. | ||
|
|
||
| The service reports, on each invocation, the operations that | ||
| changed after the last state the handler observed. The runner | ||
| delivers state in the invocation input. So the list is reset when | ||
| that input is built, not when the invocation completes: an | ||
| operation that completes while the handler is still running is | ||
| reported on the next invocation. | ||
| """Explicitly clear all pending operation IDs after their delivery. | ||
|
|
||
| The live executor uses versioned ``advance_handler_seen`` acknowledgments | ||
| so failed invocations retain their input updates and concurrent changes | ||
| newer than the acknowledged input snapshot remain pending. | ||
| """ | ||
| self.updated_operation_ids = [] | ||
|
|
||
|
|
@@ -692,6 +691,7 @@ def complete_callback_success( | |
| end_timestamp=now if now is not None else real_now(), | ||
| callback_details=updated_callback_details, | ||
| ) | ||
| self._record_updated_operation(operation.operation_id) | ||
| return self.operations[index] | ||
|
|
||
| def complete_callback_failure( | ||
|
|
@@ -708,8 +708,11 @@ def complete_callback_failure( | |
| self.touch_operation(operation.operation_id) | ||
| updated_callback_details = None | ||
| if operation.callback_details: | ||
| # Match CallbackDetails.from_dict without depending on a store | ||
| # serialization round trip: an empty wire Error has no details. | ||
| updated_callback_details = replace( | ||
| operation.callback_details, error=error | ||
| operation.callback_details, | ||
| error=error if error is not None and error.to_dict() else None, | ||
| ) | ||
|
|
||
| self.operations[index] = replace( | ||
|
|
@@ -718,6 +721,7 @@ def complete_callback_failure( | |
| end_timestamp=now if now is not None else real_now(), | ||
| callback_details=updated_callback_details, | ||
| ) | ||
| self._record_updated_operation(operation.operation_id) | ||
| return self.operations[index] | ||
|
|
||
| def complete_callback_timeout( | ||
|
|
@@ -744,6 +748,7 @@ def complete_callback_timeout( | |
| end_timestamp=now if now is not None else real_now(), | ||
| callback_details=updated_callback_details, | ||
| ) | ||
| self._record_updated_operation(operation.operation_id) | ||
| return self.operations[index] | ||
|
|
||
| def complete_chained_invoke( | ||
|
|
@@ -897,10 +902,9 @@ def page( | |
| ``max_size_bytes`` and, when given, ``max_items``. Second element | ||
| of the tuple is a marker for the next page, or ``None`` when the | ||
| page fits everything.""" | ||
| start_idx = self._resolve_marker(marker) | ||
| return self._walk_page( | ||
| self.snapshot_operations, start_idx, max_size_bytes, max_items | ||
| ) | ||
| operations = prune_replay_children(self.snapshot_operations) | ||
| start_idx = self._resolve_marker(marker, len(operations)) | ||
| return self._walk_page(operations, start_idx, max_size_bytes, max_items) | ||
|
|
||
| def unseen_operations(self) -> list[Operation]: | ||
| """Operations whose ``operation_last_touched_seq`` is strictly | ||
|
|
@@ -920,11 +924,20 @@ def unseen_operations(self) -> list[Operation]: | |
| ] | ||
|
|
||
| def advance_handler_seen(self, seq: int) -> None: | ||
| """Advance the wrapped execution's ``handler_seen_seq`` to | ||
| ``seq`` if that represents forward progress. Monotonic: | ||
| smaller or equal values are ignored.""" | ||
| """Acknowledge delivered updates without retreating the watermark. | ||
|
|
||
| Equal acknowledgments also consume old-format, unversioned update IDs. | ||
| Only versions covered by this delivery may be removed. | ||
| """ | ||
| if seq > self.execution.handler_seen_seq: | ||
| self.execution.handler_seen_seq = seq | ||
| # A checkpoint response or completed invocation delivered this state. | ||
| # Retain changes newer than that acknowledged snapshot. | ||
| self.execution.updated_operation_ids = [ | ||
| operation_id | ||
| for operation_id in self.execution.updated_operation_ids | ||
| if self.execution.operation_last_touched_seq.get(operation_id, 0) > seq | ||
| ] | ||
|
|
||
| # --- internals ------------------------------------------------- | ||
|
|
||
|
|
@@ -953,14 +966,21 @@ def _size_for(self, op: Operation) -> int: | |
| # floor at 1 to guarantee pagination always advances. | ||
| return max(self.execution.operation_size_bytes.get(op.operation_id, 0), 1) | ||
|
|
||
| def _resolve_marker(self, marker: str | None) -> int: | ||
| def _resolve_marker( | ||
| self, marker: str | None, operation_count: int | None = None | ||
| ) -> int: | ||
| if marker is None: | ||
| return 0 | ||
| seq, idx = self._decode_marker(marker) | ||
| if seq != self.pinned_token_sequence: | ||
| msg = "Invalid marker" | ||
| raise InvalidParameterValueException(msg) | ||
| if idx < 0 or idx > len(self.snapshot_operations): | ||
| count = ( | ||
| len(self.snapshot_operations) | ||
| if operation_count is None | ||
| else operation_count | ||
| ) | ||
|
Comment on lines
+978
to
+982
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex AI review · Finding [P3] Simplify the unused marker-validation mode This breaks rules 2, “Simplify before you add,” and 4, “Every branch has a test that fails without it.” The only caller, To suppress repeats, reply |
||
| if idx < 0 or idx > count: | ||
| msg = "Invalid marker" | ||
| raise InvalidParameterValueException(msg) | ||
| return idx | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex AI review · Finding
arf_v1_btzwejpjhgdfzthurynjv4w4rp[P2] Establish regression coverage for live empty-error normalization
Rule 4, “Every branch has a test that fails without it,” is unmet here. The callback-completion regressions supply nonempty errors; the empty-payload regressions construct history or operation objects directly. Reverting this expression to
error=errortherefore leaves those assertions unchanged, allowing the in-memory/filesystem discrepancy to return undetected. Please return to the design phase under rule 8 and establish a regression covering an actual callback completion without diagnostics across the state/history boundary before revising the implementation.To suppress repeats, reply
/ai verdict deferred,/ai verdict false-positive, or/ai verdict out-of-scopeon the first nonblank line. Add rationale on later lines. A new/ai verdict acceptedallows future repeats again.