Add nil check to FactoryUnit OnStopBuild

[lL1l1]

Description of the proposed changes

Fixes this error from game #26485007

LUA Main thread
@c:\programdata\faforever\gamedata\lua.nx5\lua\sim\units\factoryunit.lua 129
Table, nil, "FactoryBuild", nil, nil, "attempt to call method `GetFractionComplete' (a nil value)", "...rever\gamedata\lua.nx5\lua\sim\units\factoryunit.lua(141): attempt to call method `GetFractionComplete' (a nil value)
stack traceback:
    ...rever\gamedata\lua.nx5\lua\sim\units\factoryunit.lua(141): in function <...rever\gamedata\lua.nx5\lua\sim\units\factoryunit.lua:129>"

Testing done on the proposed changes

I'm not sure how to reproduce this.

Checklist

- [ ] Changes are annotated, including comments where useful

• Changes are documented in a changelog snippet according to the guidelines.
• Request 2-3 reviewers from the list of reviewers and their areas of knowledge.

Summary by CodeRabbit

• Bug Fixes
  • Fixed a critical crash that occurred when canceling factory unit production. The issue caused unexpected errors during build cancellation under certain conditions, affecting factory unit reliability. This fix ensures stable gameplay and improved factory performance during all production operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR fixes a nil value error in FactoryUnit.OnStopBuild by adding a guard check to ensure unitBeingBuilt is not destroyed before accessing its GetFractionComplete property, along with a corresponding changelog entry documenting the fix for issue #7029.

Changes

Cohort / File(s)	Summary
Changelog Entry changelog/snippets/fix.7029.md	Added changelog snippet documenting the fix for issue #7029.
FactoryUnit Nil Check lua/sim/units/FactoryUnit.lua	Added nil guard check in OnStopBuild to verify unitBeingBuilt is not destroyed before evaluating GetFractionComplete() and destroying it.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

type: bug, area: sim

Suggested reviewers

• 4z0t
• speed2CZ

Poem

🐰 A guard so small, a check so neat,
Prevents the nil that breaks the beat!
FactoryUnit now stands tall and strong,
No more the crash of things gone wrong! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description includes the error context and a changelog snippet, but misses two uncompleted checklist items: requesting 2-3 reviewers and the author explicitly states they cannot verify the fix works.	Request 2-3 reviewers from the designated list and clarify testing strategy or acceptance criteria for this defensive fix since reproduction wasn't possible.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main change: adding a nil check to FactoryUnit OnStopBuild to fix a runtime error.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into develop

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@lua/sim/units/FactoryUnit.lua`:
- Around line 140-143: The current block calls IsDestroyed(unitBeingBuilt)
without first ensuring unitBeingBuilt is non-nil; update the conditional to
first check unitBeingBuilt before calling IsDestroyed and GetFractionComplete so
it follows the project's nil-safe pattern (e.g., guard with "if unitBeingBuilt
and not IsDestroyed(unitBeingBuilt) and unitBeingBuilt:GetFractionComplete() < 1
then unitBeingBuilt:Destroy() end"). Modify the section that references
unitBeingBuilt, IsDestroyed, GetFractionComplete, and Destroy to perform the
explicit nil check first.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Find the IsDestroyed function definition
rg -n 'IsDestroyed\s*=' --type lua -A 10 | head -50

Repository: FAForever/fa

Length of output: 4372

---

🏁 Script executed:

# Also search in Core.lua or where it's typically defined
fd -e lua | xargs grep -l 'IsDestroyed' | head -10

Repository: FAForever/fa

Length of output: 957

---

🏁 Script executed:

# Check usage of IsDestroyed with potentially nil values
rg -n 'IsDestroyed\(' --type lua -B 2 -A 2 | head -100

Repository: FAForever/fa

Length of output: 7110

---

🏁 Script executed:

# Search for IsDestroyed function definition in engine files
rg -n 'function IsDestroyed' --type lua -A 5

Repository: FAForever/fa

Length of output: 270

---

🏁 Script executed:

# Check the FactoryUnit.lua file to see both line 102 and 140-143
cat lua/sim/units/FactoryUnit.lua | head -150 | tail -60

Repository: FAForever/fa

Length of output: 2424

---

The fix requires an explicit nil check before IsDestroyed().

The codebase consistently uses patterns like if value and not IsDestroyed(value) or if (not value) or IsDestroyed(value) to guard against nil. Since IsDestroyed is an engine function (implemented in C++, not Lua), it does not safely handle nil values. The current fix at lines 140-143 violates this pattern and risks the same nil-related crash if unitBeingBuilt is nil.

The fix should be:

-        if not IsDestroyed(unitBeingBuilt) and unitBeingBuilt:GetFractionComplete() < 1 then
+        if unitBeingBuilt and not IsDestroyed(unitBeingBuilt) and unitBeingBuilt:GetFractionComplete() < 1 then

🤖 Prompt for AI Agents

In `@lua/sim/units/FactoryUnit.lua` around lines 140 - 143, The current block
calls IsDestroyed(unitBeingBuilt) without first ensuring unitBeingBuilt is
non-nil; update the conditional to first check unitBeingBuilt before calling
IsDestroyed and GetFractionComplete so it follows the project's nil-safe pattern
(e.g., guard with "if unitBeingBuilt and not IsDestroyed(unitBeingBuilt) and
unitBeingBuilt:GetFractionComplete() < 1 then unitBeingBuilt:Destroy() end").
Modify the section that references unitBeingBuilt, IsDestroyed,
GetFractionComplete, and Destroy to perform the explicit nil check first.