fix(push): prevent hang and tag race in apify push (#1131)

src/commands/actors/push.ts

@@ -6,7 +6,12 @@ import type { Actor, ActorCollectionCreateOptions, ActorDefaultRunOptions } from
 import open from 'open';
 
 import { fetchManifest } from '@apify/actor-templates';
-import { ACTOR_JOB_STATUSES, ACTOR_SOURCE_TYPES, MAX_MULTIFILE_BYTES } from '@apify/consts';
+import {
+	ACTOR_JOB_STATUSES,
+	ACTOR_JOB_TERMINAL_STATUSES,
+	ACTOR_SOURCE_TYPES,
+	MAX_MULTIFILE_BYTES,
+} from '@apify/consts';
 import { createHmacSignature } from '@apify/utilities';
 
 import { ApifyCommand } from '../../lib/command-framework/apify-command.js';
@@ -25,6 +30,7 @@ import {
 	getLocalUserInfo,
 	getLoggedClientOrThrow,
 	outputJobLog,
+	parseWaitForFinishMillis,
 	printJsonToStdout,
 } from '../../lib/utils.js';
 
@@ -190,9 +196,7 @@ export class ActorsPushCommand extends ApifyCommand<typeof ActorsPushCommand> {
 			buildTag = DEFAULT_BUILD_TAG;
 		}
 
-		const waitForFinishMillis = Number.isNaN(this.flags.waitForFinish)
-			? undefined
-			: Number.parseInt(this.flags.waitForFinish!, 10) * 1000;
+		const waitForFinishMillis = parseWaitForFinishMillis(this.flags.waitForFinish);
 
 		// User can override actorId of pushing Actor.
 		// It causes that we push Actor to this id but attributes in localConfig will remain same.
@@ -359,18 +363,16 @@ Skipping push. Use --force to override.`,
 			waitForFinish: 2, // NOTE: We need to wait some time to Apify open stream and we can create connection
 		});
 
-		try {
-			// While the log is streaming, forward interrupt signals to a
-			// platform-side abort so the build doesn't keep running after the
-			// user gives up waiting (Ctrl+C, SIGTERM from a parent process,
-			// SIGHUP from a closing terminal). The `using` binding guarantees
-			// the listener is removed before we poll for final status.
-			using _signalHandler = useAbortJobOnSignal({
-				apifyClient,
-				kind: 'build',
-				jobId: build.id,
-			});
+		// Forward interrupt signals (Ctrl+C, SIGTERM, SIGHUP) to a platform-side
+		// abort for the lifetime of log streaming AND status polling, so the
+		// build doesn't keep running after the user gives up waiting.
+		using _signalHandler = useAbortJobOnSignal({
+			apifyClient,
+			kind: 'build',
+			jobId: build.id,
+		});
 
+		try {
 			await outputJobLog({ job: build, timeoutMillis: waitForFinishMillis, apifyClient });
 		} catch (err) {
 			warning({ message: 'Can not get log:' });
@@ -379,6 +381,42 @@ Skipping push. Use --force to override.`,
 
 		build = (await apifyClient.build(build.id).get())!;
 
+		// `outputJobLog` can return before the build is actually terminal (stream
+		// 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;
+		while (!ACTOR_JOB_TERMINAL_STATUSES.includes(build.status as never) && Date.now() < deadline) {
+			await new Promise((resolve) => setTimeout(resolve, 1000));
+			build = (await apifyClient.build(build.id).get())!;
+		}
+
+		// Platform updates `taggedBuilds[buildTag]` asynchronously after the
+		// build finishes. Wait until the tag points at this build so callers
+		// (including --json automation) that immediately
+		// `actor.start({ build: buildTag })` don't race it. Capped at 30s so an
+		// unknown platform delay can't stall push forever.
+		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;
+			let tagApplied = false;
+			while (Date.now() < tagDeadline) {
+				const a = await actorClient.get();
+				if (a?.taggedBuilds?.[buildTag]?.buildId === build.id) {
+					tagApplied = true;
+					break;
+				}
+				await new Promise((resolve) => setTimeout(resolve, 1000));
+			}
+			if (!tagApplied) {
+				warning({
+					message: `Build succeeded but tag "${buildTag}" did not update within 30s; subsequent calls referencing this tag may race.`,
+				});
+				process.exitCode = CommandExitCodes.BuildTimedOut;
+			}
+		}
+
 		if (this.flags.json) {
 			printJsonToStdout(build);
 			return;
@@ -403,9 +441,11 @@ Skipping push. Use --force to override.`,
 			// @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;
 			// @ts-expect-error FIX THESE TYPES 😢
 		} else if (build.status === ACTOR_JOB_STATUSES.RUNNING) {
 			warning({ message: 'Build is still running.' });
+			process.exitCode = CommandExitCodes.BuildTimedOut;
 			// @ts-expect-error FIX THESE TYPES 😢
 		} else if (build.status === ACTOR_JOB_STATUSES.ABORTED || build.status === ACTOR_JOB_STATUSES.ABORTING) {
 			warning({ message: 'Build was aborted!' });

src/lib/utils.ts

@@ -607,7 +607,7 @@ export const outputJobLog = async ({
 			}
 		});
 
-		if (timeoutMillis) {
+		if (timeoutMillis !== undefined) {
 			nodeTimeout = setTimeout(() => {
 				stream.destroy();
 				resolve('timeouts');
@@ -840,3 +840,10 @@ export function shellConfigFile(userHomeDirectory: string, shell: ReturnType<typ
 		}
 	}
 }
+
+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;
+	return parsed * 1000;
+}

test/local/lib/parse-wait-for-finish.test.ts

@@ -0,0 +1,23 @@
+import { parseWaitForFinishMillis } from '../../../src/lib/utils.js';
+
+describe('parseWaitForFinishMillis()', () => {
+	it('returns undefined when flag is omitted', () => {
+		expect(parseWaitForFinishMillis(undefined)).toBeUndefined();
+	});
+
+	it('returns undefined for non-numeric input', () => {
+		expect(parseWaitForFinishMillis('abc')).toBeUndefined();
+	});
+
+	it('returns 0 for zero (no wait)', () => {
+		expect(parseWaitForFinishMillis('0')).toBe(0);
+	});
+
+	it('returns undefined for negative values', () => {
+		expect(parseWaitForFinishMillis('-5')).toBeUndefined();
+	});
+
+	it('converts positive seconds to milliseconds', () => {
+		expect(parseWaitForFinishMillis('30')).toBe(30_000);
+	});
+});