refactor(workflow-executor): suppress spurious timeout log when step fails before timeout#1587
Conversation
… before timeout fires Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (1)
🛟 Help
|
…bleExecutor in timeout test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| // .catch() prevents UnhandledPromiseRejection when execPromise rejects after the timeout has | ||
| // already won the race. Only log when timeoutFired — a rejection before the timeout propagates | ||
| // normally through the race and needs no log here. |
There was a problem hiding this comment.
I think the code is clear enough that the comment isn't necessary, but it's just a preference
There was a problem hiding this comment.
Oherwise Claude says : New comment partially restates the .catch()/UnhandledPromiseRejection rationale already explained in the comment at lines 168–170; could be trimmed to focus only on the timeoutFired guard rationale — file: packages/workflow-executor/src/executors/base-step-executor.ts:179-181
| if (!timeoutMs || timeoutMs <= 0) return this.doExecute(); | ||
|
|
||
| let timer: NodeJS.Timeout | undefined; | ||
| let timeoutFired = false; |
| } | ||
| }, 5_000); | ||
|
|
||
| it('does not log when execPromise rejects before the timeout fires', async () => { |
There was a problem hiding this comment.
Test name should express functional intent, for example : does not log discard message when step rejects before timeout - preferential
|
Considering it's just a log, do we need 'fix' in the commit name ? You could put 'refactor' - which would avoid triggering a build. No big deal. |
…trim comment, rename test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Scra3
changed the title
2 participants
Summary
runWithTimeout()attached a.catch()onexecPromisebeforePromise.raceto preventUnhandledPromiseRejectionon late rejections. It always logged"Step work rejected after timeout — result discarded"— even when the step failed before the timeout fired, producing a misleading log for any normal step error.timeoutFiredboolean flag set totrueonly inside thesetTimeoutcallback. The.catch()now skips the log whentimeoutFiredis false (rejection propagates normally through the race; no log needed).execPromiserejects before the timeout.Test plan
yarn workspace @forestadmin/workflow-executor test --testPathPattern="base-step-executor"— all 30 tests green including the new one'logs a late rejection of the losing promise as info'still passes (real timeout case unchanged)'does not log when execPromise rejects before the timeout fires'passes🤖 Generated with Claude Code
Note
Fix spurious 'result discarded' log in
BaseStepExecutor.runWithTimeoutwhen step fails before timeoutAdds a
timeoutFiredboolean flag inrunWithTimeoutto distinguish between a step rejection caused by the timeout branch versus a normal pre-timeout failure. The 'Step work rejected after timeout — result discarded' log is now only emitted when the timeout actually fires, not on every rejection. A new test in base-step-executor.test.ts asserts that pre-timeout rejections do not produce this log.Changes since #1587 opened
Macroscope summarized 191ddf0.