fix: repair and contain a custom NPC's product-behaviour ownership failure

[r-melvin]

Summary

A custom NPC's product behaviour can stop a save from loading:

NullReferenceException at ScheduleOne.NPCs.Behaviour.ConsumeProductBehaviour.OnStartServer ()
  at FishNet.Managing.Server.ServerObjects.SetupSceneObjects (...)

0.4.7's ConsumeProductBehaviour.OnStartServer is base.OnStartServer(); Npc.OnNPCDeinitialized += ..., where Npc is beh.Npc. Both references are set only by GetComponentInParent in Awake. An instance whose Awake ran before it was parented keeps nulls, and the exception aborts FishNet's SetupSceneObjects for every scene object after it. Seen on the inactive Consume Product behaviour of an S1API NPC created for The Big Pimpin's escorts. S1UMF has carried a workaround for this.

The change. A Harmony patch on ConsumeProductBehaviour.OnStartServer, for S1API's own NPCs only:

• Prefix: repairs the two references, as Awake would have.
• Finalizer: if the behaviour still has no NPC, contains that exception and logs it, at most 20 times. Only the deinitialize subscription is lost, since base.OnStartServer has run. Any other exception surfaces as before.

A test fails if a game member the patch relies on goes missing. On Mono, that includes the auto-property backing fields it writes.

Compatibility

• Public/protected API: none changed. One new internal static method on NPCPatches; the patch class is private.
• Existing defaults and behavior: nothing changes for intact behaviours or non-S1API objects. For an S1API NPC with a missing owner, a load that aborted now continues.
• Stable IDs, saves, and network payloads: none touched.

Validation

Mono

dotnet build S1API.sln -c MonoMelon --no-restore -p:AutomateLocalDeployment=false: 0 errors, 0 warnings. dotnet test ... -c MonoMelon: 741 passed (740 on beta plus 1 new).

IL2CPP

dotnet build S1API.sln -c Il2CppMelon --no-restore -p:AutomateLocalDeployment=false: 0 errors, 0 warnings. dotnet test ... -c Il2CppMelon: 721 passed (720 plus 1 new).

Runtime evidence

How it was tested. Automated runs on 0.4.7f7 load a save, teleport to each custom NPC, record whether its model is shown and its network state, return to the menu and load again. They used a combined build of #332 to #339.

• Same mods on both runtimes: IL2CPP and Mono "Alternate", with BigWillyMod, The Big Pimpin, Drug Expansion, S1MAPI and SteamNetworkLib. IL2CPP ran without Polyfill, on the game's own interop assembly.
• IL2CPP with about 60 mods: including Polyfill, Siesta and S1UMF.
• Saves: an early save and a late one where The Big Pimpin's intro is played through.

I also played the IL2CPP case by hand.

• IL2CPP: Contained an OnStartServer failure on 'Consume Product (Inactive) (Disabled)' on the first load, and both loads finish, with and without S1UMF installed.
• Mono: the failure did not occur in any run, so this patch did nothing there. I haven't shown whether it can occur on Mono.

Documentation

XML <remarks> on the patch class. No DocFX changes.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 37 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 76a2daca-847d-459b-b529-f853cc456fd5

📥 Commits

Reviewing files that changed from the base of the PR and between db1de39 and 8860d65.

📒 Files selected for processing (2)

• S1API.Tests/NPCs/NPCBehaviourOwnershipTests.cs
• S1API/Internal/Patches/NPCPatches.cs

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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

[ifBars]

I would like a supported-lifecycle reproduction before we accept this, for the same reason discussed on #338 and #339.

The NPC guidance requires configuring defaults in ConfigurePrefab, doing runtime setup in OnCreated, and letting S1API own instancing. Big Pimpin's manual construction sequence does not establish that this fails when following that contract. S1API already runs RepairBehaviourOwnership during prefab preparation and instance behaviour initialization. Please provide a minimal NPC using automatic instancing that shows where ownership is missed or undone, along with the exception and runtime/version.

The finalizer also suppresses any exception once it finds a custom NPC with missing ownership. It never checks which exception occurred or whether base.OnStartServer completed. That can hide a different initialization failure and leave a partially initialized object, despite the description saying unrelated exceptions still surface.

I would prefer repairing the owning construction or activation path and reusing the existing ownership repair. If a native patch is still necessary, please narrow it around a demonstrated failure and test that unrelated exceptions remain visible. The current member-existence test does not exercise either the repair or the finalizer. Leaving this open so we can work through that reproduction.

[r-melvin]

Closing this for the same reason as #338 and #339: it doesn't reproduce through S1API's documented NPC lifecycle.

The only evidence for this PR was the inactive Consume Product behaviour on an S1API NPC created for The Big Pimpin 1.0.11's escorts. Those are constructed manually with new from its Main-scene callback, the path described in your review on #338.

The reproduction I posted on #338 never hit this exception. That was one NPC written as the documented Physical NPC skeleton, on unmodified beta (6b9ebe7), on IL2CPP and on Mono, each loading a save, returning to the menu and loading again. No ConsumeProductBehaviour.OnStartServer exception occurred, and both saves finished loading normally. That covers the host only: I couldn't test a joining client.

So there's no failure in S1API's own lifecycle for this patch to repair. The workaround goes back into S1UMF, gated to Big Pimpin 1.0.11 and the game builds it was measured on, where it stands down as soon as either changes.