Stop disposing the shared SQLite chunk command on every read

[leroysquad]

Summary

• Shared chunk commands are no longer disposed on every read. SqliteCommand.Dispose calls SqliteConnection.RemoveCommand, which walks an unsynchronized list. A concurrent dispose threw IndexOutOfRangeException on the chunk thread and shut the server down.
• GetPlayerData and SetPlayerData now hold transactionLock for the whole call, including command create and dispose. A fresh join hits that path on the main thread while the chunk thread is saving.
• SetMapRegions creates and disposes its command inside the same lock. SetMulti takes the lock again; it is re-entrant.
• Temporary chunk commands are created and disposed under that lock. The shared prepared commands stay alive until Close/Dispose.

Type

• Bug fix
• Performance
• Refactor or cleanup
• New feature
• Docs or build

Checklist

• The SQLite patch applies with git apply --check against an ilspycmd 10.1.0.8386 decompile of VintagestoryLib.dll.
• Full scripts/bootstrap.ps1 and Release build on this head
• scripts/smoke-test.ps1 reaches WorldReady
• Atlas scenarios, including the chunk persistence ones
• Every new edit has a // Stratum marker
• No vanilla source committed

Test plan

• 1000-bot join while chunks save, with no RemoveCommand exception
• Save and reload a chunk column so the shared prepared command is reused
• ChunkPersistenceScenarios still persist a modified column across unload

Notes

The rarer unlocked callers (GetGameData, QuantityChunks, GetAllChunks, ForAllChunks, DeleteChunk(ulong, string), IntegrityCheck, Vacuum) are unchanged.

[Zaldaryon]

I did not find a code-level defect in this diff. The prepared-command overloads now keep connection-owned commands alive, and the temporary-command overloads hold transactionLock through creation, execution, and disposal.

I am requesting changes because the validation gates for this head are not complete:

• The PR description omits the repository checklist, and its three test-plan items remain unchecked. CONTRIBUTING.md requires a real server start and Atlas scenarios for chunk I/O changes, plus before and after measurements for hot-path changes.
• On this exact head, scripts/bootstrap.ps1 failed to apply 12 other patches. This PR's SQLite patch did apply. The build then failed with six errors in the incomplete tree, so I could not run smoke or scenarios.
• The head is based on c5c3aee, while the live indev ref is 66a8530. Please rebase onto current indev, restore and complete the PR checklist, then rerun the bootstrap, build, and required runtime checks. Request review again when the results are available.

The only completed GitHub check is the Discord notification.

[Pixnop]

The change itself is right and I would keep all of it. I checked it against the Microsoft.Data.Sqlite 10.0.9 in Lib/: SqliteDataReader.Dispose resets the statement, so the shared commands can be reused without the per-read dispose, they now stay prepared between reads instead of being re-prepared every time, and DisposeCommands() in Close/Dispose still finalizes them. Here, scripts/bootstrap.sh applied all 195 patches, the Release build has no warning in this file, and make scenarios is 49/49. I also reproduced the crash outside the repo with a small harness on the game's own Microsoft.Data.Sqlite.dll: a thread disposing the shared command on every read while another thread creates and disposes commands throws in RemoveCommand within 8 seconds, and the pattern in this PR ran 2.46 million reads in the same time with no exception. So the hottest path into RemoveCommand is gone. One thing before merge, then notes.

1. The other side of the race is still unlocked, so the same crash stays reachable in the same load test. In the 10.0.9 dll, CreateCommand adds to SqliteConnection._commands and Dispose walks that list and calls RemoveAt, with no lock, and BeginTransaction and Commit each create and dispose a command internally to run BEGIN/COMMIT. With the indev lock in place, whatever the GetChunk dispose collided with cannot have held transactionLock. GetPlayerData (generated SQLiteDbConnectionv2.cs:213-238, create at 215, dispose at 236) is the obvious candidate: it runs on the main thread from ConnectedClient.LoadOrCreatePlayerData (ConnectedClient.cs:203) on the first join of each uid since boot, so for every fresh bot. At this head chunkdbthread still adds to and removes from the same list through SetMulti (BeginTransaction at 471, Commit at 487) from SaveDirtyUnloadedChunks, SaveDirtyMapRegions and Stratum's incremental dirty flush, and SetMapRegions creates its command at 460 before the lock is taken and disposes it after it is released. A join that overlaps one of those still throws from RemoveCommand: on chunkdbthread that is the same "Exception during Process" shutdown, on the main thread the joining client is disconnected. The harness shows the lock does nothing when only one side holds it: the temporary-command pattern from this PR, created and disposed under the lock, still threw after 523 reads next to a thread that creates and disposes commands without it. The same gap has a second symptom: if a chunk-thread transaction begins between GetPlayerData's CreateCommand and ExecuteReader, ExecuteReader throws "Execute requires the command to have a transaction object" and that join fails too.

The fix is small. Wrap the bodies of GetPlayerData and SetPlayerData in lock (transactionLock) (the lock is re-entrant, so the nested GetPlayerData call inside SetPlayerData is fine), and in SetMapRegions create and dispose the command inside the lock, or prepare a shared one in OnOpened the way setMapChunksCmd already is. The rarer unlocked callers (GetGameData, QuantityChunks, GetAllChunks, ForAllChunks, DeleteChunk(ulong, string), IntegrityCheck, Vacuum) can follow in another PR. Then the 1000-bot join is the test that matters: please rerun it, tick the test plan and restore the checklist from the PR template.

Notes, not blocking:

• No scenario guards this change. With using DbCommand dbCommand = preparedCmd; put back in both prepared-command overloads, all 49 scenarios stay green. ChunkPersistenceScenarios.ModifiedColumn_Should_SurviveUnloadReload_When_SavedBeforeUnload and RepeatedCycles_Should_PersistEveryMutation_When_ColumnIsRecycled do run the changed path (ChunkIo is off by default), so they cover test-plan item 3: name them there. A scenario with a background thread calling GetPlayerData while columns load and unload would fail on indev only some of the time, but once point 1 is in it becomes a usable regression test.
• GetChunk(ulong, string) and ChunkExists(ulong, string) have no caller and are not on IGameDbConnection, so the locks added there change nothing at runtime. The comment at lines 358-359 says the lock keeps the temporary command's dispose from racing RemoveCommand on another thread, which only holds against threads that take the same lock.
• The patch is not what scripts/extract-patches.sh produces: the index line still says ..a7ed24d, which is indev's blob (the result is 4d2950c), and the hunk header is @@ -320,46 +320,76 @@ with no function context where the script gives @@ -318,50 +318,80 @@ public class SQLiteDbConnectionv2 .... It applies fine, but the next re-extract will rewrite it. Re-running the script fixes it.
• CONTRIBUTING asks for a before and after measurement on chunk IO hot paths. Reads should be faster now that they no longer re-prepare, but there is no number yet.
• The bootstrap.ps1 failure on 12 other patches does not come from this diff: the PR only touches this one patch file, so those 12 are byte-identical to the base, and bootstrap.sh applies all of them on Linux. Worth retrying on Windows after the rebase.
• The head is one commit behind indev (#344, another file) and merges cleanly; a maintainer can run /rebase.

[leroysquad]

Pushed 5040313. GetPlayerData and SetPlayerData now hold transactionLock for the whole call, and SetMapRegions creates and disposes its command inside that lock. The shared chunk command is still not disposed on read. I have not rerun the 1000-bot join on this commit.

[leroysquad]

@Pixnop @Zaldaryon 5040313 holds transactionLock through GetPlayerData, SetPlayerData, and SetMapRegions, on top of not disposing the shared chunk command. Ready for another look. The 1000-bot join has not been rerun on this commit.

[Zaldaryon]

The locking additions in 5040313 (GetPlayerData, SetPlayerData, and SetMapRegions) resolve the unprotected caller race against SqliteConnection.RemoveCommand. The diff applies cleanly against current indev.

What still blocks approval:

1. The branch remains based on c5c3aee. Current indev is at a3966e2 following the merges of #344, #352, and #353. On Windows with ilspycmd 10.1.0.8386, running scripts/bootstrap.ps1 on c5c3aee fails to apply 12 other patches, leaving the tree incomplete. On a3966e2, bootstrap.ps1 and the Release build succeed with 0 errors. Please rebase onto current indev.
2. The 1000-bot join load test has not been rerun on this head. Because this fix addresses a race between main-thread joins and background chunk saves in RemoveCommand, verifying that load scenario without exceptions is required.
3. The PR checklist items for bootstrap, server smoke test, and Atlas scenarios remain unchecked in the description, as do the three test-plan items.
4. Hot-path read measurements before and after the change required by CONTRIBUTING.md are still open.

Non-blocking:

• The patch header for SQLiteDbConnectionv2.cs.patch still carries an outdated index line and lacks class context in the hunk header. Running scripts/extract-patches.ps1 will normalize it.

[Pixnop]

5040313 is the fix I asked for. GetPlayerData and SetPlayerData hold transactionLock from CreateCommand to the last dispose, the nested GetPlayerData re-enters the same monitor, and SetMapRegions now creates and disposes its command inside the lock that SetMulti takes again. Every BeginTransaction was already under that lock, so the "Execute requires the command to have a transaction object" case from my last review is closed too. I found no new defect and no lock-order problem: the save paths take savingLock then transactionLock, the join path holds no other lock, and nothing under transactionLock waits on another lock. It also makes SetPlayerData's check-then-insert atomic, so two saves for the same uid can no longer insert two rows.

Here, scripts/bootstrap.sh applied all 195 patches, the Release build has no new warning, and make scenarios is 49/49. Merged with current indev (a3966e2) it is 195 patches, a clean build and 53/53. I also drove the real SQLiteDbConnectionv2 from each built VintagestoryLib.dll with two threads for 20 seconds, one doing chunk thread work (SetChunks, SetMapRegions, GetChunk, ChunkExists) and one doing joins (GetPlayerData, SetPlayerData). With indev's patch every run threw 1,231 to 1,462 RemoveCommand exceptions, plus 13 to 37 "Execute requires" in GetPlayerData. With this head, three runs (about 7.3 million chunk operations and 675,000 joins) threw nothing. Taking the GetPlayerData lock out brings the errors back in every run, taking the SetPlayerData one out in two runs of three. Taking the SetMapRegions one out gave nothing, even in a map-region-only loop, so that part is defensive in my test, but it costs nothing and I would keep it. For the CONTRIBUTING measurement, a single-thread read benchmark on a file database (2 KB blobs, 3 runs of 5 s) gives GetChunk at 2.96 to 2.98 µs instead of 4.11 to 4.26 µs on indev (about 29% faster) and ChunkExists at 2.42 to 2.46 µs instead of 3.65 to 3.71 µs (about 34%). Putting the per-read dispose back returns to the indev numbers. Feel free to copy these into a Performance numbers section. Two things before I approve, then notes.

1. The 1000-bot join still has to run, and it should now also show how long a join waits. With the lock, a fresh join waits on the main thread for whatever transaction the chunk thread has open. The periodic save paths cap their batches (100, 200 and 300 chunks in ServerSystemLoadAndSaveGame.cs), and in my harness a GetPlayerData call behind back-to-back batches of 300 chunks of 32 KB waited 7.7 ms at p50 and 106 to 126 ms at worst, which is no worse than indev under the same load (p50 9.8 ms, p90 62 ms). The unload path has no cap: SaveDirtyUnloadedChunks writes the whole dirty-unloaded backlog in one SetChunks and one SetMapChunks (ServerSystemUnloadChunks.cs:286 and :290, and :536/:540 for generating columns), so a join that lands there holds the tick for every connected player until that commit ends. On indev the join did not wait for it (it ran inside the open transaction, or crashed at its edges). Keep the lock, it is what closes the race. For the rerun on the rebased head, time the wait for the lock in GetPlayerData (a Stopwatch around the lock, logged when it goes over one tick) and report it next to the RemoveCommand result. If the wait shows up, capping those unload writes at 300 per transaction like SaveAllDirtyLoadedChunks is enough, here or in a follow-up. Then tick the test plan.

2. The checklist is still not the template's. The current list is custom and leaves out "extract-patches ran clean", the dotnet build line and "Tested on a real server start". The first one would be false today: the patch still carries index 0c59a67..a7ed24d (indev's blob, the result is 0eb2e83) and hunk headers without function context (@@ -212,59 +212,67 @@ where the script gives @@ -210,63 +210,71 @@ public class SQLiteDbConnectionv2 ...). Running scripts/extract-patches.sh or .ps1 after the rebase fixes the header and makes that item true. Please use the template and tick what you ran.

Notes, not blocking:

• Still no scenario guards this. I ran ChunkPersistenceScenarios three times with each lock taken out, with the per-read dispose put back, and with indev's whole patch, and all 15 runs stayed green. Test-plan item 2 cannot catch a regression either, since a disposed command just re-prepares. Please name ModifiedColumn_Should_SurviveUnloadReload_When_SavedBeforeUnload and RepeatedCycles_Should_PersistEveryMutation_When_ColumnIsRecycled in item 3. A scenario with a background thread calling GetPlayerData while columns load and unload would now make a real regression test: under that kind of load indev's patch throws thousands of times in 20 seconds in my harness.
• GetChunk(ulong, string) and ChunkExists(ulong, string) still have no caller, so their locks change nothing at runtime and the comments on them claim more than they do.
• The head is 3 commits behind indev (#344, #352, #353), none touches this file, it merges cleanly, and the bootstrap.ps1 failure Zaldaryon saw on c5c3aee does not happen on a3966e2; a maintainer can run /rebase.