fix(push): prevent hang and tag race in apify push (#1131)#1134
fix(push): prevent hang and tag race in apify push (#1131)#1134l2ysho wants to merge 18 commits into
Conversation
Resolves an issue where waitForFinishMillis could be NaN for invalid flag inputs, leading to unexpected behavior.
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)
There was a problem hiding this comment.
in actual version it is ignored, it just show you whole build log to the end. In PR it ends with
Actor build detail https://console.apify.com/actors/k3Ew4ttdaIThULhmR#/builds/0.25.6
Actor detail https://console.apify.com/actors/k3Ew4ttdaIThULhmR
Warning: Build is still running.
…can-exit-0-before-the-latest-tag-is-applied
When --wait-for-finish is passed explicitly, hitting the cap with the build still in READY/RUNNING is the user's chosen bounded wait, not a failure. Only surface BuildTimedOut for these states when the flag was omitted.
Compute a single deadline before the build call and share it between log streaming and the status poll loop. Previously the poll loop started a fresh --wait-for-finish budget after outputJobLog returned, so a log stream that died near the cap could double the user's wait.
Cut the post-build tag-apply budget from 30s to 5s, print a status line so users see why push is still waiting, and drop the non-zero exit code on tag-miss — the build itself succeeded, a lagging tag pointer is a warning, not a failure.
There was a problem hiding this comment.
I think that the current behavior is better, if I understand it correctly:
After the build log stream ends, push now polls the build's actual status until it reaches a
terminal state, instead of trusting the stream (which could close early and leave push waiting forever).
Also, this will help with follow ups: #1135
LGTM
Skip the post-build tag-apply wait when --wait-for-finish=0 so the flag is truly fire-and-forget regardless of how fast the build completed. Document the 0 semantics in the flag help text. Also picks up stale doc regeneration from #1128 (--describe / --search wording).
…can-exit-0-before-the-latest-tag-is-applied
The previous guard was unreachable: when --wait-for-finish is unset, the poll loop only exits on terminal status, so READY/RUNNING branches were dead. Switch the guard to `waitForFinishMillis !== 0` so finite caps that hit the deadline surface a non-zero exit code (useful for scripts), while fire-and-forget (--wait-for-finish=0) still returns 0 on READY. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Edyta <142720610+szaganek@users.noreply.github.com>
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: