fixed incremental build logic

[ThyTran1402]

Description

• When incremental builds are active and a native plugin is removed from core, it causes 3 bugs:
  Output_dir not cleaned on full rebuild when all components rebuild because core changed, stale files from the previous build persist in the output directory alongside new artifacts.
  Removed components persist in new manifest: If a component is removed from the input manifest but exists in the previous build manifest, it survives into the new build manifest and the assemble step tries to install it.
  Crash when no previous manifest: run_build.py logs a warning about a missing manifest file, then unconditionally calls BuildManifest.from_path() on the missing file, crashing with FileNotFoundError.

• Fixed:
  Main orchestrator, where all three fixes land
  Fix removed components persist in new manifest by valid_component_names filter
  Reference only, rebuild_plugins() already returns all components when core changes
  Update + add integration tests
  Add unit test for stale-component filtering

Issues Resolved

• [Bug]: Incremental build does not remove old artifacts if the core is rebuilding #5928

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b2a35de)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add valid_component_names filtering to BuildRecorder to prevent stale components in manifest Relevant files: src/build_workflow/build_recorder.py tests/tests_build_workflow/test_build_recorder.py Sub-PR theme: Fix full rebuild detection, output dir cleanup, and crash on missing manifest in run_build.py Relevant files: src/run_build.py tests/test_run_build.py

⚡ Recommended focus areas for review Incorrect Full-Rebuild Detection The is_full_rebuild flag checks if the manifest path does not exist OR if a core component is in list_of_updated_plugins. However, list_of_updated_plugins is the result of commits_diff, which may return component names in various formats. If the core component name doesn't exactly match "OpenSearch" or "OpenSearch-Dashboards" (e.g., due to casing or aliasing), the full rebuild won't be triggered correctly. This logic should be validated against actual return values from commits_diff. is_full_rebuild = not os.path.exists(build_manifest_path) or any( core in list_of_updated_plugins for core in ("OpenSearch", "OpenSearch-Dashboards") ) Missing manifest update on no-change incremental When is_full_rebuild is True and components is empty (no changes detected), the code returns 0 without updating the build manifest. Previously, the build ID was always updated on no-change. Now it's only updated when build_manifest is not None (i.e., not a full rebuild). This means on a full rebuild with no components to build, nothing is written to disk, which may leave the output directory empty with no manifest. Consider whether this edge case needs handling. if not components: logging.info("No commit difference found between any components. Skipping the build.") if build_manifest: build_manifest.build.id = os.getenv("BUILD_NUMBER") or uuid.uuid4().hex build_manifest.to_file(build_manifest_path) logging.info(f"Updating the build ID in the build manifest to {build_manifest.build.id}.") return 0 Incomplete Test Assertion In test_build_incremental_no_prebuild_manifest, the comment says "rmtree skipped since output_dir doesn't exist" but mock_rmtree is not explicitly asserted to NOT be called. The test only asserts mock_makedirs.assert_called(). Adding mock_rmtree.assert_not_called() would make the test more precise and guard against regressions. main() # Must NOT raise FileNotFoundError mock_makedirs.assert_called() # Output dir created (rmtree skipped since output_dir doesn't exist) # BuildRecorder called with no old build_manifest args, kwargs = mock_recorder.call_args self.assertIsNone(args[1] if len(args) > 1 else kwargs.get("build_manifest")) valid_names scope issue valid_names is computed from manifest.components.select() which returns all components in the input manifest. However, when args.incremental is True and build_manifest is not None (partial rebuild), only a subset of components are being rebuilt (components). The valid_names filter correctly prevents stale components from persisting, but it also means components that were previously built and are NOT being rebuilt in this run will be retained from the old manifest only if they are in valid_names. This is the intended behavior per the PR description, but it should be confirmed that manifest.components.select() always reflects the current desired set of components. if args.incremental and build_manifest: valid_names = {c.name for c in manifest.components.select()} build_recorder = BuildRecorder(target, build_manifest, valid_names)

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b2a35de

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Sync serialized data with filtered components When valid_component_names is provided but a component from the old manifest is not in valid_component_names, it is silently dropped. However, self.data still contains the full serialized manifest including all old components. If self.data is later used to reconstruct the manifest, stale components may reappear. Ensure that self.data is also cleaned of components not in valid_component_names, or that the manifest reconstruction relies solely on components_hash. src/build_workflow/build_recorder.py [62-67] if build_manifest: self.data = build_manifest.__to_dict__() self.data["build"]["id"] = target.build_id for component in build_manifest.components.select(): if valid_component_names is None or component.name in valid_component_names: self.components_hash[component.name] = component.__to_dict__() + if valid_component_names is not None and "components" in self.data: + self.data["components"] = [ + c for c in self.data["components"] if c.get("name") in valid_component_names + ] Suggestion importance[1-10]: 7 __ Why: This is a valid potential issue - self.data retains the full serialized manifest including stale components, while components_hash is filtered. If the manifest reconstruction uses self.data for components rather than components_hash, stale components could reappear in the output. This inconsistency could cause real bugs depending on how get_manifest() reconstructs the final manifest.	Medium
General	Clarify incremental build recorder condition When args.incremental is True but is_full_rebuild is also True, build_manifest is None, so BuildRecorder(target) is used — which starts a fresh manifest. However, valid_names is computed from manifest.components.select() but never passed, meaning the valid_component_names filtering feature in BuildRecorder is unused for the full rebuild path. This is consistent with intent, but the condition could be more explicit to avoid confusion. Consider using is_full_rebuild directly in the condition for clarity. src/run_build.py [94-98] -if args.incremental and build_manifest: +if args.incremental and not is_full_rebuild and build_manifest: valid_names = {c.name for c in manifest.components.select()} build_recorder = BuildRecorder(target, build_manifest, valid_names) else: build_recorder = BuildRecorder(target) Suggestion importance[1-10]: 5 __ Why: The suggestion improves code clarity by making the is_full_rebuild condition explicit. The current code already works correctly since build_manifest is None when is_full_rebuild is True, but adding not is_full_rebuild makes the intent clearer and avoids potential confusion for future maintainers.	Low
	Handle empty components during full rebuild When is_full_rebuild is True and components is empty (no changed components detected), the code returns 0 without performing any build, but the output directory was already cleaned. This leaves the output directory empty with no manifest written, which could cause downstream failures. Consider logging a warning or handling this edge case explicitly when a full rebuild is triggered but no components are found. src/run_build.py [69-75] if not components: logging.info("No commit difference found between any components. Skipping the build.") + if is_full_rebuild: + logging.warning("Full rebuild was triggered but no components found to build. Output directory was cleaned.") if build_manifest: build_manifest.build.id = os.getenv("BUILD_NUMBER") or uuid.uuid4().hex build_manifest.to_file(build_manifest_path) logging.info(f"Updating the build ID in the build manifest to {build_manifest.build.id}.") return 0 Suggestion importance[1-10]: 4 __ Why: This is a valid edge case concern - when is_full_rebuild is True but components is empty, the output directory is cleaned but no manifest is written. However, this scenario is unlikely in practice (a full rebuild is triggered by core component changes, which would be in components), and the suggestion only adds a warning log without fixing the underlying issue.	Low

---

Previous suggestions

Suggestions up to commit a3b7c37

Category	Suggestion	Impact
Possible issue	Initialize variable before conditional assignment Initialize build_manifest to None before the conditional block to ensure it's always defined in the scope. This prevents potential NameError when accessing build_manifest later in the code flow. src/run_build.py [60-66] +build_manifest = None if is_full_rebuild: logging.info("Full rebuild triggered. Cleaning output directory and skipping previous build manifest.") if os.path.exists(output_dir): shutil.rmtree(output_dir) os.makedirs(output_dir, exist_ok=True) else: build_manifest = BuildManifest.from_path(build_manifest_path) Suggestion importance[1-10]: 10 __ Why: Critical bug fix. The variable build_manifest is only assigned in the else block (line 66) but is referenced later at line 70. When is_full_rebuild is True, this causes a NameError. Initializing build_manifest = None before the conditional ensures it's always defined in scope.	High
	Prevent undefined variable access When is_full_rebuild is True, build_manifest is never assigned, causing a NameError when not components is True. Ensure build_manifest is initialized to None before the conditional logic to prevent runtime errors. src/run_build.py [68-74] if not components: logging.info("No commit difference found between any components. Skipping the build.") - if build_manifest: + if not is_full_rebuild and build_manifest: build_manifest.build.id = os.getenv("BUILD_NUMBER") or uuid.uuid4().hex build_manifest.to_file(build_manifest_path) logging.info(f"Updating the build ID in the build manifest to {build_manifest.build.id}.") return 0 Suggestion importance[1-10]: 9 __ Why: Critical bug fix. When is_full_rebuild is True, build_manifest is never assigned, causing a NameError at line 70 when checking if build_manifest:. The suggestion correctly identifies this issue and proposes checking not is_full_rebuild to prevent accessing an undefined variable.	High

Suggestions up to commit 12e3e9d

Category	Suggestion	Impact
Possible issue	Initialize variable before conditional Initialize build_manifest to None before the conditional block to ensure it's always defined. This prevents potential NameError when is_full_rebuild is True and the variable is referenced later in the code flow. src/run_build.py [55-66] build_manifest_path = os.path.join(args.distribution, "builds", manifest.build.filename, "manifest.yml") is_full_rebuild = not os.path.exists(build_manifest_path) or any( core in list_of_updated_plugins for core in ("OpenSearch", "OpenSearch-Dashboards") ) +build_manifest = None if is_full_rebuild: logging.info("Full rebuild triggered. Cleaning output directory and skipping previous build manifest.") if os.path.exists(output_dir): shutil.rmtree(output_dir) os.makedirs(output_dir, exist_ok=True) else: build_manifest = BuildManifest.from_path(build_manifest_path) Suggestion importance[1-10]: 10 __ Why: Critical bug fix. The variable build_manifest is only assigned in the else branch (line 66) but is referenced later at line 70. When is_full_rebuild is True, this causes a NameError. Initializing build_manifest = None before the conditional ensures the variable is always defined.	High
	Prevent undefined variable access When is_full_rebuild is True, build_manifest is never assigned, causing a NameError when not components is True. Ensure build_manifest is initialized to None before the conditional logic to prevent runtime errors. src/run_build.py [68-74] if not components: logging.info("No commit difference found between any components. Skipping the build.") - if build_manifest: + if not is_full_rebuild and build_manifest: build_manifest.build.id = os.getenv("BUILD_NUMBER") or uuid.uuid4().hex build_manifest.to_file(build_manifest_path) logging.info(f"Updating the build ID in the build manifest to {build_manifest.build.id}.") return 0 Suggestion importance[1-10]: 9 __ Why: Critical bug fix. When is_full_rebuild is True, build_manifest is never assigned, causing a NameError at line 70 when not components evaluates to True. The suggested fix correctly adds the not is_full_rebuild check to prevent accessing an undefined variable.	High

[github-actions]

Persistent review updated to latest commit a3b7c37

[ThyTran1402]

Hi @peterzhuamazon

Can you help to review it please?

Thank you!

[gaiksaya]

Hi @ThyTran1402

Thank you for the contribution. Can you please check the code review suggestions here: #6003 (comment)

Also can you please fix the tests so that expected behavior is validated?

Thanks!

[gaiksaya]

Can you please confirm what valid_names mean here? Is it all components?

[ThyTran1402]

Yes. So valid_names here is all components in the current build manifest like {"OpenSearch", "common-utils", ...}, and it used to filter out any components that were in the previous build but are no longer valid.

[github-actions]

Persistent review updated to latest commit b2a35de

[ThyTran1402]

Hi @ThyTran1402

Thank you for the contribution. Can you please check the code review suggestions here: #6003 (comment)

Also can you please fix the tests so that expected behavior is validated?

Thanks!

I just initialized build_manifest to None. Can you review it again please?

Thank you!