bugfix(production): Prevent cancel if units were 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)

Before:

mRefund_before_FFB.mp4

After :

coming soon

[greptile-apps]

Greptile Summary

This PR fixes a real money-refund exploit (issue #133) where canceling a batch production entry mid-way through would return the full purchase price even though some units had already been delivered. The fix is applied identically to both the base-game (Generals/) and Zero Hour (GeneralsMD/) variants and is gated behind #if !RETAIL_COMPATIBLE_CRC to avoid breaking replays/CRC-checked retail builds.

Key change:

• A guard in cancelUnitCreate returns early when getProductionQuantityRemaining() < getProductionQuantity(), meaning cancellation of a partially-produced batch entry becomes a no-op.

Critical issue:
The no-op return leaves the ProductionEntry in m_productionQueue without removing or freeing it. Two other callers depend on cancelUnitCreate always removing the entry:

1. cancelAndRefundAllProduction loops calling cancelUnitCreate(m_productionQueue->getProductionID()), advancing only because the head entry is removed each time. With the noop, m_productionQueue never changes and the loop exhausts its 100-iteration safety limit without clearing the queue — leaving partially-produced entries as permanent orphans when a building is sold or destroyed.
2. The script-disable path (line 696–709) calls cancelUnitCreate to flush a disallowed unit every update frame; if the noop fires, the entry is never removed and the code will hit that branch again every frame for the rest of the match.

Not safe to merge — the noop early return breaks cancelAndRefundAllProduction and the script-disable cleanup path, introducing memory leaks and stuck-production bugs.

Confidence Score: 1/5

• Not safe to merge — the noop early return breaks two critical callers and will cause observable bugs in production.
• The exploit fix is logically correct in intent, but the chosen implementation (returning without removing the queue entry) breaks two other callers that depend on cancelUnitCreate always removing the entry from the queue. This will cause observable bugs: buildings with partially-produced units that are sold will silently fail to clear their queue, and script-disabled units that have partially produced will stay stuck in a busy-loop state.
• Both Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp and GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp require attention. The fix must ensure entries are always removed from the queue, not just conditionally refunded.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[cancelUnitCreate called] --> B{Find entry in queue<br/>with matching productionID}
    B -- Not found --> Z[Return — no-op]
    B -- Found --> C{#if !RETAIL_COMPATIBLE_CRC<br/>remaining < total?}
    C -- Yes: some units<br/>already produced --> D["⚠️ return early<br/>(entry stays in queue!)"]
    C -- No: nothing produced yet --> E[Refund full cost<br/>via money->deposit]
    E --> F[removeFromProductionQueue]
    F --> G[deleteInstance]
    G --> H[Return]

    D -.->|Caller: cancelAndRefundAllProduction<br/>loops on same head entry<br/>100× without clearing queue| X["❌ Queue not cleared<br/>(orphaned entry, no free)"]
    D -.->|Caller: script-disable path<br/>hits this branch every frame| Y["❌ Stuck production<br/>(disallowed unit never removed)"]

Loading

_{Last reviewed commit: 3c23298}

[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.

[greptile-apps]

Critical: Early return breaks cancelAndRefundAllProduction and script-disable cleanup

The early return at line 477 exits without removing the entry from m_productionQueue. This breaks two callers that depend on cancelUnitCreate always removing the entry:

1. cancelAndRefundAllProduction (line 1140): This function loops up to 100 times, calling cancelUnitCreate(m_productionQueue->getProductionID()) and expecting the head entry to be removed each iteration. With this early return, m_productionQueue never changes — the loop exhausts all 100 iterations on the same stuck entry without clearing the queue. Partially-produced batch entries become permanent orphans when the building is sold/destroyed.

2. Script-disabled unit path (line 696–709): When a script disables a unit type mid-production, this code calls cancelUnitCreate to flush the entry. If one unit was already produced, the early return leaves the entry active. The next frame, the update loop hits this condition again and calls cancelUnitCreate repeatedly on the same entry — a busy-loop / stuck production state.

The exploit fix is logically correct (no refund for partially-produced units), but the implementation should still remove the entry. The safest fix is to refund conditionally but always remove:

if( production->getProductionQuantityRemaining() == production->getProductionQuantity() )
{
    Money *money = player->getMoney();
    money->deposit( production->m_objectToProduce->calcCostToBuild( player ), TRUE, FALSE );
}

removeFromProductionQueue( production );
deleteInstance(production);
return;

Prompt To Fix With AI

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

Comment:
**Critical: Early return breaks `cancelAndRefundAllProduction` and script-disable cleanup**

The early `return` at line 477 exits without removing the entry from `m_productionQueue`. This breaks two callers that depend on `cancelUnitCreate` always removing the entry:

1. **`cancelAndRefundAllProduction`** (line 1140): This function loops up to 100 times, calling `cancelUnitCreate(m_productionQueue->getProductionID())` and expecting the head entry to be removed each iteration. With this early return, `m_productionQueue` never changes — the loop exhausts all 100 iterations on the same stuck entry without clearing the queue. Partially-produced batch entries become permanent orphans when the building is sold/destroyed.

2. **Script-disabled unit path** (line 696–709): When a script disables a unit type mid-production, this code calls `cancelUnitCreate` to flush the entry. If one unit was already produced, the early return leaves the entry active. The next frame, the update loop hits this condition again and calls `cancelUnitCreate` repeatedly on the same entry — a busy-loop / stuck production state.

The exploit fix is logically correct (no refund for partially-produced units), but the implementation should still remove the entry. The safest fix is to refund conditionally but always remove:

```cpp
if( production->getProductionQuantityRemaining() == production->getProductionQuantity() )
{
    Money *money = player->getMoney();
    money->deposit( production->m_objectToProduce->calcCostToBuild( player ), TRUE, FALSE );
}

removeFromProductionQueue( production );
deleteInstance(production);
return;
```

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

[tintinhamans]

Hmm, maybe I need to add a forceCancel parameter for these cases.

[greptile-apps]

That's a solid approach! A forceCancel parameter would give you explicit control over the two different scenarios:

1. Player-initiated cancel (default, forceCancel=false): Block if units already produced (your current bugfix intent)
2. System cleanup (forceCancel=true): Always remove the entry regardless of production state, used by cancelAndRefundAllProduction and the script-disable path

The implementation would look something like:

void ProductionUpdate::cancelUnitCreate( ProductionID productionID, bool forceCancel = false )
{
    // ... find production ...
    
    Player *player = getObject()->getControllingPlayer();
#if !RETAIL_COMPATIBLE_CRC
    bool partiallyProduced = production->getProductionQuantityRemaining() < production->getProductionQuantity();
    
    // Block player-initiated cancel if units already produced
    if( !forceCancel && partiallyProduced )
    {
        return;
    }
    
    // Only refund if nothing was produced yet
    if( !partiallyProduced )
    {
        Money *money = player->getMoney();
        money->deposit( production->m_objectToProduce->calcCostToBuild( player ), TRUE, FALSE );
    }
#endif
    
    // Always remove the entry (either forceCancel=true or full refund case)
    removeFromProductionQueue( production );
    deleteInstance(production);
}

Then call sites:

• cancelAndRefundAllProduction: cancelUnitCreate(id, true)
• Script-disable path: cancelUnitCreate(id, true)
• Player UI cancel: cancelUnitCreate(id, false) or just cancelUnitCreate(id) (default)

This preserves the exploit fix while handling the cleanup scenarios correctly.

Changes since review.