ordered presubmit builds processing

[ievdokdm]

fix: flutter/flutter#189029

[gemini-code-assist]

Code Review

This pull request introduces ordered presubmit processing for LUCI build notifications by adding a new PresubmitOrderedSubscription endpoint, introducing an OrderingKeyTag to scheduled builds, and updating the PubSub service to support ordering keys. It also refactors the existing PresubmitLuciSubscription to inherit from a new base PresubmitSubscription class. The review feedback highlights an invalid Dart syntax error in the base class (override == .neutral ? .neutral : null) and suggests refactoring the common message parsing and validation logic into the base class's post method to eliminate code duplication, along with cleaning up unused imports.

[ievdokdm]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors presubmit build status processing by extracting shared logic from PresubmitLuciSubscription into a new base class PresubmitSubscription, and introduces PresubmitOrderedSubscription to support sequential processing of LUCI notifications via PubSub ordering keys. It also adds configuration flags to control this feature and updates LuciBuildService to attach ordering keys to scheduled builds. The review feedback highlights three potential runtime issues in the newly created PresubmitSubscription class: a potential decompression crash if buildLargeFields is empty, a potential null pointer exception from force-unwrapping userData.checkRunId, and a potential type cast error when parsing presubmit_max_attempts.

[gemini-code-assist]

Decompressing an empty list of bytes with ZLibDecoder().decodeBytes will throw a FormatException or ArchiveException. To prevent potential crashes, defensively check if buildsPubSub.buildLargeFields is not empty before attempting to decompress and merge it.

Suggested change

	// Add build fields that are stored in a separate compressed buffer.
	build.mergeFromBuffer(
	const ZLibDecoder().decodeBytes(buildsPubSub.buildLargeFields),
	);
	// Add build fields that are stored in a separate compressed buffer if present.
	if (buildsPubSub.buildLargeFields.isNotEmpty) {
	build.mergeFromBuffer(
	const ZLibDecoder().decodeBytes(buildsPubSub.buildLargeFields),
	);
	}

[gemini-code-assist]

In the non-unified check run flow, userData.checkRunId is expected to be non-null. However, force-unwrapping it with ! can cause a runtime crash if it is ever null. Adding a defensive null check and logging an error is safer and prevents crashing the handler.

Suggested change

	await _githubChecksService.updateCheckStatus(
	checkRunId: userData.checkRunId!,
	build: build,
	luciBuildService: _luciBuildService,
	slug: userData.commit.slug,
	rescheduled: rescheduled,
	conclusionOverride: override,
	summaryPrepend: suppressedMessage,
	);
	if (userData.checkRunId == null) {
	log.error('checkRunId is null for non-unified check run');
	} else {
	await _githubChecksService.updateCheckStatus(
	checkRunId: userData.checkRunId!,
	build: build,
	luciBuildService: _luciBuildService,
	slug: userData.commit.slug,
	rescheduled: rescheduled,
	conclusionOverride: override,
	summaryPrepend: suppressedMessage,
	);
	}

[gemini-code-assist]

Directly casting properties['presubmit_max_attempts'] as int can throw a TypeError at runtime if the value is parsed as a string or is null. It is safer to parse it defensively to handle different potential types gracefully.

    if (!properties.containsKey('presubmit_max_attempts')) {
      // Give any test in the merge queue another try... its expensive otherwise.
      return tags.containsType<InMergeQueueBuildTag>()
          ? LuciBuildService.kMergeQueueMaxRetries
          : 1;
    }
    final maxAttemptsValue = properties['presubmit_max_attempts'];
    if (maxAttemptsValue is int) {
      return maxAttemptsValue;
    } else if (maxAttemptsValue is String) {
      return int.tryParse(maxAttemptsValue) ?? 1;
    }
    return 1;