Conversation
Agent-Logs-Url: https://github.com/durable-workflow/workflow/sessions/d1646bb7-d46e-4808-8a4e-4ebf909aa1f7 Co-authored-by: rmcdaniel <1130888+rmcdaniel@users.noreply.github.com>
rmcdaniel
left a comment
There was a problem hiding this comment.
This is closer, but I still found one real regression in the dedupe heuristic.
src/Exception.php:56 now skips writing whenever the nearest prior log is also Workflow\\Exception. That fixes the signal-retry shape, but it still suppresses a legitimate second exception if a workflow catches one failure and immediately yields another failing activity with no signal/timer/success log in between.
I verified this with the real sample app and real queue workers:
1.0.69: a workflow that catches failure A and then immediately catches failure B completes, withWorkflow\\Exceptionlogs at indexes0and1.1.0.70: the same workflow gets stuck after the first exception.- This PR: the same workflow still gets stuck after the first exception, because the second real failure is treated like a stale sibling duplicate.
So I think the current previousLogIsException() check is still too broad. The remaining distinction is not just "previous log was an exception"; it is more like "this exception belongs to a stale parallel branch the workflow has not advanced past".
I’d also want a regression test for this consecutive-caught-failures shape in addition to the new signal test, because tests/Unit/ExceptionTest.php currently only proves the signal-advance case.
Agent-Logs-Url: https://github.com/durable-workflow/workflow/sessions/400020c3-e0d1-467b-8694-8f8f62b4e78b Co-authored-by: rmcdaniel <1130888+rmcdaniel@users.noreply.github.com>
rmcdaniel
left a comment
There was a problem hiding this comment.
Two issues still look blocking on the latest probe version.
-
src/Workflow.phpnow callsWorkflowStub::getContext()unconditionally fromsetContext(). On a fresh worker process there is no existing static context yet, so the first workflow job can fail before any exception-dedupe logic runs. I reproduced this in the real sample app on the exact PR branch: the firstExceptionLoggingRetryWorkflowexecution failed immediately withTypeError: Workflow\WorkflowStub::getContext(): Return value must be of type stdClass, null returned, and the workflow moved straight tofailedwith no replay logs. -
After locally null-guarding that crash just to continue the experiment, the child-workflow case is still not preserved. The real child-failure saga in the sample app leaves the parent workflow in
waitingwith only logs at indexes0and3, no parent compensation log, and the compensation activity log ending up under child workflow62instead of parent61. So the updated probe fixes the signal-retry and consecutive-caught-activity cases, but it still regresses the original child-workflow behavior that#348was fixing.
I think the new unit coverage is directionally good, but it still misses the live child-workflow shape from #347/#348: tests/Unit/ExceptionTest.php exercises parallel activities, not parallel child workflows.
Agent-Logs-Url: https://github.com/durable-workflow/workflow/sessions/3527603f-0ed5-4b5c-9bb8-bba1b504cb2d Co-authored-by: rmcdaniel <1130888+rmcdaniel@users.noreply.github.com>
Addressed in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Workflow.phpso exception dedupe no longer depends on static context initializationcomposer ecsandvendor/bin/phpunit tests/Unit/ExceptionTest.php