bugfix(production): Prevent cancelling production when units already produced

[tintinhamans]

This pull request updates the logic for refunding player resources when canceling unit production, addressing a bug where players could receive a refund even after some units had already been produced.

Once production->getProductionQuantityRemaining() < production->getProductionQuantity(), it is no longer possible to cancel production.

Resolves #133 (China Red Guard can be built for free when construction is cancelled in last moment)

This guard is skipped when forceCancel = TRUE is passed, which is used by:

• cancelAndRefundAllProduction() - building destroyed/sold, full refund is intentional
• The script-disallow path - production blocked externally, not player-initiated

Before:

mRefund_before_FFB.mp4

After :

mRefund_after_FFB.mp4

[greptile-apps]

Greptile Summary

This PR fixes the "China Red Guard free unit" exploit (issue #133) by adding a forceCancel parameter to cancelUnitCreate, preventing player-initiated cancellations from issuing a refund once at least one unit from a batch has already been produced. The change is applied to both Generals/ and GeneralsMD/ code paths and is guarded by #if !RETAIL_COMPATIBLE_CRC to preserve vanilla behaviour in retail-compatible builds.

Key changes:

• cancelUnitCreate gains Bool forceCancel = FALSE; when false and some units in the batch have already been produced, the cancel is silently blocked
• cancelAndRefundAllProduction (building sold/destroyed) now passes TRUE, so the full-refund path is unaffected ✅
• The update() script-disallow path is modified: under !RETAIL_COMPATIBLE_CRC it only cancels if no units have been produced yet; for partially-produced batches it falls through to normal production, allowing remaining disallowed units to complete ⚠️
• cancelAllUnitsOfType still calls cancelUnitCreate without forceCancel, meaning partially-produced batch entries for a given unit type are silently left in the queue ⚠️

Confidence Score: 3/5

• The core exploit fix is correct, but two secondary paths (script-disallow fallthrough and cancelAllUnitsOfType) have unresolved logic issues that could allow disallowed units to be created or batches to silently not cancel.
• The primary bugfix (player-initiated cancel exploit) is well-implemented and cancelAndRefundAllProduction is correctly updated. However, the script-disallow path now allows a unit type explicitly forbidden by script to continue producing if it's mid-batch, which is a behavior change that bypasses the script restriction. Additionally, cancelAllUnitsOfType silently fails to cancel in-progress batch entries, breaking its documented contract. Both issues exist identically in Generals and GeneralsMD.
• Both ProductionUpdate.cpp files — specifically the script-disallow block in update() (around line 706 in Generals, 707 in GeneralsMD) and the cancelAllUnitsOfType function (line 517 / 518).

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp	Core fix for the free-unit exploit is sound (forceCancel guard in cancelUnitCreate, cancelAndRefundAllProduction correctly passes TRUE), but the script-disallow fallthrough allows partially-produced disallowed batches to complete, and cancelAllUnitsOfType silently skips in-progress batch entries.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp	Mirrors the Generals changes exactly; carries the same script-disallow fallthrough issue and cancelAllUnitsOfType silent-failure concern.
Generals/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h	Correctly adds Bool forceCancel = FALSE default parameter to both the pure-virtual interface declaration and the concrete class declaration; no issues.
GeneralsMD/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h	Mirrors the Generals header change; signature update is clean and consistent.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Player clicks Cancel] --> B{forceCancel?}
    B -- TRUE\ncancelAndRefundAllProduction /\nonDie --> C[Always remove entry + full refund]
    B -- FALSE\nplayer-initiated --> D{RETAIL_COMPATIBLE_CRC?}
    D -- YES --> E[Original: refund + remove always]
    D -- NO --> F{getProductionQuantityRemaining\n< getProductionQuantity?}
    F -- YES\nsome units already produced --> G[Early return — no refund,\nno removal, cancel blocked]
    F -- NO\nnothing produced yet --> H[Full refund + remove entry]

    I[update loop — script disallows unit type] --> J{RETAIL_COMPATIBLE_CRC?}
    J -- YES --> K[Original: cancelUnitCreate\nrefund + remove always]
    J -- NO --> L{getProductionQuantityRemaining\n== getProductionQuantity?}
    L -- YES\nnothing produced yet --> M[cancelUnitCreate + return]
    L -- NO\npartial batch in progress --> N[Fall through: production\ncontinues to completion\ndespite script disallow ⚠️]

Loading

Comments Outside Diff (2)

1. Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp, line 706-712 (link)

   Script-disallowed units still produced for partial batches

   When allowedToBuild() returns FALSE for a non-dozer unit that is mid-batch (e.g. 1 of 4 Red Guards already produced), the new code silently skips the cancellation and falls through to m_framesUnderConstruction++ and the normal production path. As a result, the remaining 3 Red Guards will continue to be spawned on subsequent frames, even though the script has explicitly forbidden this unit type.

   The original behavior cancelled immediately (with a full refund). The new behavior leaves the partially-produced batch completing indefinitely. If the intent of the script-disable path is to stop disallowed unit creation, passing forceCancel = TRUE to the existing call would achieve that while still preserving the refund semantic for unpaid units:

   #else
       // TheSuperHackers @bugfix arcticdolphin 13/03/2026 Force-cancel script-disallowed entries regardless of partial production.
       cancelUnitCreate(production->getProductionID(), TRUE);
       return UPDATE_SLEEP_NONE;
   #endif

   If the "finish naturally" design is truly intentional, the comment should explain why the game design prefers granting disallowed units over blocking them, since this is a non-obvious departure from the original behaviour.

   The same issue is present in GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp at line 707–712.

2. Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp, line 517 (link)

   cancelAllUnitsOfType silently skips partially-produced batch entries

   cancelAllUnitsOfType calls cancelUnitCreate without forceCancel = TRUE. Under !RETAIL_COMPATIBLE_CRC, if a batch entry has at least one unit already produced (getProductionQuantityRemaining() < getProductionQuantity()), cancelUnitCreate returns early without removing the entry. The caller saves production->m_next into temp and advances correctly, so there is no crash — but the partially-produced entry is silently left in the queue.

   The function's documented contract is to cancel all production of a given unit type. With this change it no longer fulfils that contract for in-progress batch entries.

   Passing forceCancel = TRUE here restores the expected behaviour:

   cancelUnitCreate( production->getProductionID(), TRUE );

   The identical issue is present in GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp at line 518.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp
Line: 706-712

Comment:
**Script-disallowed units still produced for partial batches**

When `allowedToBuild()` returns `FALSE` for a non-dozer unit that is mid-batch (e.g. 1 of 4 Red Guards already produced), the new code silently skips the cancellation and falls through to `m_framesUnderConstruction++` and the normal production path. As a result, the remaining 3 Red Guards will continue to be spawned on subsequent frames, even though the script has explicitly forbidden this unit type.

The original behavior cancelled immediately (with a full refund). The new behavior leaves the partially-produced batch completing indefinitely. If the intent of the script-disable path is to **stop** disallowed unit creation, passing `forceCancel = TRUE` to the existing call would achieve that while still preserving the refund semantic for unpaid units:

```cpp
#else
    // TheSuperHackers @bugfix arcticdolphin 13/03/2026 Force-cancel script-disallowed entries regardless of partial production.
    cancelUnitCreate(production->getProductionID(), TRUE);
    return UPDATE_SLEEP_NONE;
#endif
```

If the "finish naturally" design is truly intentional, the comment should explain why the game design prefers granting disallowed units over blocking them, since this is a non-obvious departure from the original behaviour.

The same issue is present in `GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp` at line 707–712.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp
Line: 517

Comment:
**`cancelAllUnitsOfType` silently skips partially-produced batch entries**

`cancelAllUnitsOfType` calls `cancelUnitCreate` without `forceCancel = TRUE`. Under `!RETAIL_COMPATIBLE_CRC`, if a batch entry has at least one unit already produced (`getProductionQuantityRemaining() < getProductionQuantity()`), `cancelUnitCreate` returns early without removing the entry. The caller saves `production->m_next` into `temp` and advances correctly, so there is no crash — but the partially-produced entry is silently left in the queue.

The function's documented contract is to cancel **all** production of a given unit type. With this change it no longer fulfils that contract for in-progress batch entries.

Passing `forceCancel = TRUE` here restores the expected behaviour:

```cpp
cancelUnitCreate( production->getProductionID(), TRUE );
```

The identical issue is present in `GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp` at line 518.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: b6a117c}

[Caball009]

Looks good to me.

[xezon]

Does this mean when cancelling the Red Guard after 1 guy exited, no refund is made, but only guy is given? This would be not great.

It better refunds the remaining fair value.

Or it should not be possible to cancel as soon as partial production has finished.

[tintinhamans]

It better refunds the remaining fair value.

I feel like this could be abused, you could get 1 Red Guard then for $150 if we simply halved refund.

Or it should not be possible to cancel as soon as partial production has finished.

Yea, that was my other proposal - noop cancelling if production->getProductionQuantityRemaining() < production->getProductionQuantity()

[xezon]

I feel like this could be abused, you could get 1 Red Guard then for $150 if we simply halved refund.

Maybe this is fine. Adds a new skill to the game? For example if the China guy just needs 1 Red Guard to capture an Oil Derrick nearby. China can need any help it can get. And it does not affect the stronger China Infantry General which is good too.

@Stubbjax thoughts?

[Stubbjax]

China can need any help it can get.

Except free units via well-timed production queue cancellations. 😛

It does feel like a rather unorthodox place to intentionally install a skill differentiator. Is a reasonable player likely to deliberately risk cancelling the whole unit for the sake of saving $150? I imagine it's more commonly a fluke that leads to confusion or surprise, which is generally better to minimise where possible. It is fairly trivial and would be incredibly rare either way so I don't feel too strongly about it in this regard.

The main reservation I have with the partial refund approach is when it comes to an indivisible BuildCost ÷ QuantityModifier. Tank General's Red Guard costs $375 - how would half of this be refunded without $1 being unaccounted for?

Preventing cancellation once the first unit has exited seems like the cleanest approach to me.

[tintinhamans]

OK I have updated this now to prevent cancelling entirely if production->getProductionQuantityRemaining() < production->getProductionQuantity().

AFAIK this time window is always quite short anyways so should have minimal impact.

[xezon]

Needs fixing.

Changes since review.

[tintinhamans]

@greptileai You mention cancelAllUnitsOfType() as a potential issue but AFAIK nothing ever calls this.

[greptile-apps]

You're right - I searched and found zero calls to cancelAllUnitsOfType(). That concern doesn't apply here since the function isn't actually used. My mistake!

[xezon]

Is this force cancel very necessary? Can we not just let the partial production finish?

[tintinhamans]

forceCancel is needed for following:

cancelAndRefundAllProduction(): Called when the building is sold/destroyed. No building left to produce from - the queue must be cleared. This function loops calling cancelUnitCreate on the head entry, expecting removal each iteration. Without forceCancel, a partially-produced entry would never be removed, and the loop could spin 100 times on the same stuck entry.

Script-disable path in update(): if a script disallows a unit type mid-production, without forceCancel the entry stays in the queue, and the next update() frame hits the same disallow condition again.

[tintinhamans]

Only in these cases they do get full refund - I don't know how else you would want to handle this but if you have another idea let me know.

[tintinhamans]

Oh, I think I understand what you mean.. I guess we can allow it to finish for the script case.

[xezon]

Maybe we can make it work consistently? Nothing is able to force partial refunds. This avoids unexpected edge cases or behaviors that would need testing otherwise.

If some loop gets stuck, then maybe write the loop in a way it does not get stuck with a production that can no longer be cancelled? Or will this cause other problems?

[tintinhamans]

So in case of building being destroyed during partial production, no refund?

[xezon]

If that keeps the code simpler I would say just let the pending production die alongside the building.

Do we have other opinions?