fix(push): prevent hang and tag race in apify push (#1131)#1134
Conversation
github-actions
Bot
added
the
tested
Resolves an issue where waitForFinishMillis could be NaN for invalid flag inputs, leading to unexpected behavior.
l2ysho
force-pushed
the
1131-apify-push-can-hang-indefinitely-and-can-exit-0-before-the-latest-tag-is-applied
branch
from
fa017eb to
07e801a
Compare
…can-exit-0-before-the-latest-tag-is-applied
There was a problem hiding this comment.
Ok this test is kinda useless :D
| // @ts-expect-error FIX THESE TYPES 😢 | ||
| } else if (build.status === ACTOR_JOB_STATUSES.READY) { | ||
| warning({ message: 'Build is waiting for allocation.' }); | ||
| process.exitCode = CommandExitCodes.BuildTimedOut; |
There was a problem hiding this comment.
This is technically wrong, if I pass in apify push --waitForFinish=5 it should not give me a non-0 exit code when the timer ran out
| // ended early, timeout hit). Poll so the status branches below see the | ||
| // real outcome. With no --wait-for-finish, the flag documents "waits | ||
| // forever", so poll without a deadline. | ||
| const deadline = waitForFinishMillis === undefined ? Infinity : Date.now() + waitForFinishMillis; |
There was a problem hiding this comment.
I feel like the deadline should be computed right before starting the build, not after waiting for the outputLog to finalizr/crash.
also, won't this cause a double-wait? Imagine an actor that takes 4minutes to build, if --waitForFinish is set to 30s, we print for 29s then connection dies for the log stream, we wait another 30s after? Ideally we calculate a delta between build start, and log end, and wait out the remaining time, if any, right?
| if (build.status === ACTOR_JOB_STATUSES.SUCCEEDED && buildTag) { | ||
| // 30s budget is independent of --wait-for-finish: the build is already | ||
| // done, we're only waiting on the platform to update the tag pointer. | ||
| const tagDeadline = Date.now() + 30_000; |
There was a problem hiding this comment.
30s is wayy too much, i'd bail out after 5 max. Also lets print out a message that we are "applying the build tag" (even tho the console does it) just so users know why their terminal is hanging
| export function parseWaitForFinishMillis(flag: string | undefined): number | undefined { | ||
| if (flag === undefined) return undefined; | ||
| const parsed = Number.parseInt(flag, 10); | ||
| if (!Number.isFinite(parsed) || parsed < 0) return undefined; |
There was a problem hiding this comment.
mmmm, can you compare what happens right now with --waitForFinish=0 vs this PR? It's a use case we should support imo (fire-and-forget)
3 participants
Fixes #1131.
apify push had two reliability issues:
status with a cap from --wait-for-finish (defaults to no cap when unset, but exits cleanly on terminal status).
tag to point at the new build (30s budget) before exiting.
Also: