Reapply "Fix factories not immediately building queue after finished upgrades or cancelled land/navy units"" #6912
Conversation
This reverts commit cc96b62.
WalkthroughAdds a changelog entry and updates factory upgrade handling by adding an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)lua/sim/units/FactoryUnit.lua (3)
🔇 Additional comments (2)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
changelog/snippets/fix.6912.md(1 hunks)lua/sim/units/FactoryUnit.lua(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lua/sim/units/FactoryUnit.lua (2)
lua/system/class.lua (1)
State(138-166)engine/Sim.lua (1)
NotifyUpgrade(1050-1051)
🔇 Additional comments (3)
changelog/snippets/fix.6912.md (1)
1-1: LGTM!The changelog entry is clear, concise, and provides appropriate context about the fix being re-applied and the desync investigation findings.
lua/sim/units/FactoryUnit.lua (2)
2-3: LGTM!The imports follow the established pattern in this file for upvaluing parent class methods for performance, and
UnitOnStopBuildis appropriately used in the newUpgradingState.OnStopBuildimplementation.
472-494: I'm unable to clone the repository to verify the review comment through code inspection. However, I can provide guidance based on what we know:The review comment raises a valid defensive programming concern about calling
unitBuilding:GetFractionComplete()without prior nil/destroyed checks. This is a reasonable concern in game engine code where units can be destroyed or cancelled mid-upgrade.However, without access to the codebase, I cannot:
- Verify how
StructureUnit.UpgradingState.OnStopBuildhandles this- Confirm if
IsDestroyed()is actually used in this codebase- Check what defensive patterns are used elsewhere in
FactoryUnit.luaRewritten review comment:
Guard against nil/destroyed
unitBuildingbefore calling GetFractionComplete().At line 481,
unitBuilding:GetFractionComplete()is called without verifying the unit is still valid. While normal upgrade completion is safe, edge cases like upgrade cancellation or unit destruction during the transition could cause crashes.OnStopBuild = function(self, unitBuilding, order) UnitOnStopBuild(self, unitBuilding, order) self:EnableDefaultToggleCaps() - if unitBuilding:GetFractionComplete() == 1 then + if unitBuilding and not IsDestroyed(unitBuilding) + and unitBuilding:GetFractionComplete() == 1 + then NotifyUpgrade(self, unitBuilding)Verify this mirrors defensive patterns used elsewhere in this file and confirm
IsDestroyed()is the correct utility function for this codebase.
lua/sim/units/FactoryUnit.lua
Outdated
| -- Instantly clear the build area so the next build can start, since unit `Destroy` doesn't do so. | ||
| self.UnitBeingBuilt:SetCollisionShape('None') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let me examine the FactoryUnit.lua file to understand the context around lines 204-205
cd /tmp && git clone --depth 1 https://github.com/FAForever/fa.git 2>/dev/null || true
cat -n lua/sim/units/FactoryUnit.lua | sed -n '195,215p'Repository: FAForever/fa
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for OnFailedToBuild definition and where it's called from
rg -n "OnFailedToBuild" --type lua -B 3 -A 10Repository: FAForever/fa
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check how UnitBeingBuilt is assigned and cleared in this file
rg -n "UnitBeingBuilt\s*=" --type lua -B 2 -A 2Repository: FAForever/fa
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Look for Destroy calls on UnitBeingBuilt to understand when it might become nil
rg -n "UnitBeingBuilt.*Destroy\|Destroy.*UnitBeingBuilt" --type lua -B 3 -A 3Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if there's a repository already cloned/available
pwd
ls -la
git remote -v 2>/dev/null || echo "Not a git repo in current directory"Repository: FAForever/fa
Length of output: 3049
🏁 Script executed:
#!/bin/bash
# Search for FactoryUnit.lua in standard locations
fd "FactoryUnit.lua" / 2>/dev/null | head -20Repository: FAForever/fa
Length of output: 1207
🏁 Script executed:
#!/bin/bash
# Read the context around lines 204-205 in FactoryUnit.lua
cat -n /home/jailuser/git/lua/sim/units/FactoryUnit.lua | sed -n '195,220p'Repository: FAForever/fa
Length of output: 1250
🏁 Script executed:
#!/bin/bash
# Find the OnFailedToBuild function definition
rg -n "OnFailedToBuild\s*=" /home/jailuser/git/lua/sim/units/FactoryUnit.lua -A 15Repository: FAForever/fa
Length of output: 1403
🏁 Script executed:
#!/bin/bash
# Check all places where UnitBeingBuilt is set to nil
rg -n "UnitBeingBuilt\s*=" /home/jailuser/git/lua/sim/units/FactoryUnit.lua -B 2 -A 2Repository: FAForever/fa
Length of output: 666
🏁 Script executed:
#!/bin/bash
# Look for the Build and OnBuildEnd functions to understand lifecycle
rg -n "function.*Build\|OnBuild" /home/jailuser/git/lua/sim/units/FactoryUnit.lua -B 1 -A 5 | head -100Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Find where UnitBeingBuilt is assigned
rg -n "self\.UnitBeingBuilt\s*=" /home/jailuser/git/lua/sim/units/FactoryUnit.lua -B 1 -A 1Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look for the Build function or StartBuild
rg -n "function.*Build\|Build\s*=\s*function" /home/jailuser/git/lua/sim/units/FactoryUnit.lua -B 1 -A 10 | head -80Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for when UnitBeingBuilt could be nil or cleared
rg -n "UnitBeingBuilt.*nil\|nil.*UnitBeingBuilt" /home/jailuser/git/lua/sim/units/FactoryUnit.lua -B 2 -A 2Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look at the full OnBuildEnd and related functions to understand the build lifecycle
cat -n /home/jailuser/git/lua/sim/units/FactoryUnit.lua | sed -n '300,400p'Repository: FAForever/fa
Length of output: 4119
🏁 Script executed:
#!/bin/bash
# Search for where UnitBeingBuilt is first assigned or created
rg -n "UnitBeingBuilt" /home/jailuser/git/lua/sim/units/FactoryUnit.lua | head -40Repository: FAForever/fa
Length of output: 857
🏁 Script executed:
#!/bin/bash
# Look at lines 100-160 to find Build or related functions
cat -n /home/jailuser/git/lua/sim/units/FactoryUnit.lua | sed -n '100,160p'Repository: FAForever/fa
Length of output: 3002
🏁 Script executed:
#!/bin/bash
# Search parent class StructureUnit for OnFailedToBuild
rg -n "OnFailedToBuild" /home/jailuser/git/lua/sim/units/StructureUnit.lua -B 2 -A 10Repository: FAForever/fa
Length of output: 1166
🏁 Script executed:
#!/bin/bash
# Check what StructureUnitOnFailedToBuild does
rg -n "OnFailedToBuild" /home/jailuser/git/lua/sim/units/ --type lua -B 2 -A 5 | grep -A 10 "StructureUnit"Repository: FAForever/fa
Length of output: 6950
🏁 Script executed:
#!/bin/bash
# Search for calls to OnFailedToBuild across the codebase
rg -n "OnFailedToBuild\(" /home/jailuser/git/lua/sim/ --type lua -B 3 -A 1 | head -100Repository: FAForever/fa
Length of output: 7371
🏁 Script executed:
#!/bin/bash
# Look at the parent class Unit's OnFailedToBuild
rg -n "OnFailedToBuild" /home/jailuser/git/lua/sim/unit.lua -B 2 -A 10 | head -50Repository: FAForever/fa
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Check if there's a Build function in FactoryUnit or parent classes that sets UnitBeingBuilt
rg -n "function.*Build\s*\(" /home/jailuser/git/lua/sim/units/FactoryUnit.lua -B 2 -A 15 | head -150Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look for where the build thread fails and calls OnFailedToBuild
rg -n "Build\|Failed" /home/jailuser/git/lua/sim/units/FactoryUnit.lua | grep -i "failed\|function" | head -30Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look for where UnitBeingBuilt is first assigned in StructureUnit or factory
rg -n "self\.UnitBeingBuilt\s*=" /home/jailuser/git/lua/sim/units/StructureUnit.lua -B 3 -A 1 | head -100Repository: FAForever/fa
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Look for the Build method in StructureUnit
cat -n /home/jailuser/git/lua/sim/units/StructureUnit.lua | sed -n '1,100p'Repository: FAForever/fa
Length of output: 4907
🏁 Script executed:
#!/bin/bash
# Search for Build in StructureUnit
rg -n "Build\s*=\s*function" /home/jailuser/git/lua/sim/units/StructureUnit.lua -A 30 | head -100Repository: FAForever/fa
Length of output: 2748
🏁 Script executed:
#!/bin/bash
# Look more broadly for build-related functions that set UnitBeingBuilt
rg -n "UnitBeingBuilt" /home/jailuser/git/lua/sim/units/StructureUnit.lua | head -50Repository: FAForever/fa
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Look at Unit.OnStartBuild around line 2849 to see the flow
rg -n "OnStartBuild.*function\|OnFailedToBuild" /home/jailuser/git/lua/sim/Unit.lua | grep -A 2 -B 2 "2849\|OnStartBuild"Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Read Unit.lua around line 2849
cat -n /home/jailuser/git/lua/sim/Unit.lua | sed -n '2830,2870p'Repository: FAForever/fa
Length of output: 2473
🏁 Script executed:
#!/bin/bash
# Search for where OnFailedToBuild is called in Unit.lua
rg -n "OnFailedToBuild\|OnStartBuild" /home/jailuser/git/lua/sim/Unit.lua | head -20Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Confirm the flow: when does OnFailedToBuild get called vs when UnitBeingBuilt is assigned
cat -n /home/jailuser/git/lua/sim/units/StructureUnit.lua | sed -n '593,650p'Repository: FAForever/fa
Length of output: 3025
🏁 Script executed:
#!/bin/bash
# Check if there are any other early returns or conditions in StructureUnit.OnStartBuild
cat -n /home/jailuser/git/lua/sim/units/StructureUnit.lua | sed -n '593,650p' | grep -E "return|if|end"Repository: FAForever/fa
Length of output: 1425
Based on my verification, I have confirmed the issue. The execution path is:
- Unit.OnStartBuild (Unit.lua line 2849) can call
self:OnFailedToBuild()when a restricted unit is attempted - This returns
false(line 2852) - StructureUnit.OnStartBuild (line 595) detects this false return and immediately exits at line 596
- Critically,
self.UnitBeingBuiltis only assigned at line 598, which is NEVER reached when the function returns early - FactoryUnit.OnFailedToBuild (line 205) then tries to access
self.UnitBeingBuilt:SetCollisionShape('None')on a nil value
This is a real and critical nil access bug. The review comment's concern is valid and the suggested fix is appropriate.
Nil access to self.UnitBeingBuilt is possible when OnFailedToBuild is called via restricted unit build failure.
When a restricted unit build attempt is made, Unit.OnStartBuild() calls OnFailedToBuild() before self.UnitBeingBuilt is ever assigned in StructureUnit.OnStartBuild() (line 598). The code at line 205 attempts to call SetCollisionShape('None') on an uninitialized value, causing a crash.
Add the nil check as suggested:
OnFailedToBuild = function(self)
-- Instantly clear the build area so the next build can start, since unit `Destroy` doesn't do so.
+ local unitBeingBuilt = self.UnitBeingBuilt
+ if unitBeingBuilt and not IsDestroyed(unitBeingBuilt) then
+ unitBeingBuilt:SetCollisionShape('None')
+ end
- self.UnitBeingBuilt:SetCollisionShape('None')
StructureUnitOnFailedToBuild(self)
self.FactoryBuildFailed = true
self:StopBuildFx()
ChangeState(self, self.IdleState)
end,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- Instantly clear the build area so the next build can start, since unit `Destroy` doesn't do so. | |
| self.UnitBeingBuilt:SetCollisionShape('None') | |
| OnFailedToBuild = function(self) | |
| -- Instantly clear the build area so the next build can start, since unit `Destroy` doesn't do so. | |
| local unitBeingBuilt = self.UnitBeingBuilt | |
| if unitBeingBuilt and not IsDestroyed(unitBeingBuilt) then | |
| unitBeingBuilt:SetCollisionShape('None') | |
| end | |
| StructureUnitOnFailedToBuild(self) | |
| self.FactoryBuildFailed = true | |
| self:StopBuildFx() | |
| ChangeState(self, self.IdleState) | |
| end, |
🤖 Prompt for AI Agents
In lua/sim/units/FactoryUnit.lua around lines 204-205,
FactoryUnit.OnFailedToBuild may call
self.UnitBeingBuilt:SetCollisionShape('None') when self.UnitBeingBuilt is nil
(build failed before assignment); add a nil-check before accessing
UnitBeingBuilt and only call SetCollisionShape('None') if self.UnitBeingBuilt is
non-nil (e.g., if self.UnitBeingBuilt then
self.UnitBeingBuilt:SetCollisionShape('None') end), ensuring no other logic
assumes UnitBeingBuilt exists when it may not.
Reverts #6835
Desyncs still happen, so this must not be the source of the desyncs. It would be nice to have the changes anyway.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.