Fix/sheet write conflicts - #89
Merged
Merged
Conversation
added 10 commits
August 24, 2026 15:16
…s own edits A sheet is one JSON blob in one column, so changing a field means reading the whole thing, altering it and writing it all back. Twenty-two places do that, and nothing held anything across the gap between the read and the write - so two writers to the same sheet each built their new blob from the same starting point and whichever wrote second discarded the other's change. No error, no conflict, no retry: the hit points come back. Two different players never collided, since every query is scoped by username. The collision is one sheet with two writers, which is an ordinary moment at a table - a GM applying damage while that player edits their gear, an automatic effect landing mid-edit, one person with the sheet open in two tabs. sheets/mutate.js is a queue per sheet id. Per id rather than one global lock, because writes to different sheets are genuinely independent and during a fight they are constant. A test pins the race as it stands - two unguarded writes, one change lost - so the helper's tests are measured against a demonstrated fault rather than a described one. updateSheetField is converted, being the highest-traffic writer. The rest follow: a mutex only helps when both colliding writers take it, so this is protection between converted writers only until the others move over.
The two that actually collide with a player editing their own sheet. Defender damage wrote back a blob parsed several steps earlier, before the roll was even resolved. The defender is very likely looking at that sheet while it is being shot at, so an edit made in between was erased. It now applies SP ablation and shield absorption to a fresh read. Death saves were worse. The sheet was parsed, then the write waited out the client's five-second dice animation before writing the whole blob back - so anything edited while the dice were rolling was discarded, and a death save is exactly the moment a table is busiest. Only the penalty changes, and it is now applied to whatever the sheet says when the animation ends. Both are also simpler for it: neither needs to carry a stale parsed copy across several callbacks any more.
An import replaces the whole sheet, so a concurrent edit does not merely lose a field, it disappears entirely. It also carried the occupancy fields across a replace from the stale read - so a player seated between the read and the write was turned out of their car by an import that had no business touching it. Both now read at write time. patchSheet covers the shape almost every remaining writer has: load, spread, set one key, write it all back. The spread is exactly what carried a stale copy of every other field, so spreading over a fresh read instead means only the fields being patched are contested. Enemy hull damage and the moving toggle use it.
Eight writers in one file, all of the same shape: load the sheet, spread it, set one field, write the whole thing back. The spread is what carried a stale copy of every other field, so seating someone during a fight could discard whatever they had just edited. The choke point now takes the change rather than a whole sheet, which makes the staleness impossible to reintroduce at a call site by accident. Emptying a seat removes fields rather than setting them, so it goes through mutateSheet with clearOccupancy applied to a fresh read instead. Worth recording how the missing require surfaced: `node -e "require(...)"` still reported the module loading fine, because an undefined helper is only a problem when it is called. What actually caught it was the test suite hanging - a callback that never fired - rather than failing. A module that loads is not a module that works.
The GM editing a player's sheet is the pairing most likely to collide with that player editing it themselves, and it was a plain read-modify-write. The bulk operations - LUCK reset, Edge reset, CWN rest, grant Edge - scanned every sheet, computed a new blob for each, then wrote them all back. On a table of six that is six whole-sheet writes built from a scan that had already gone stale by the time the last one landed. The scan now decides only *who* is affected; the value is computed against what the sheet actually says at write time. Strain and Edge grants are the ones where that shows as a wrong number rather than a missing field, since both are relative to the current value. Granting Edge also now clamps to the maximum inside the write, so two grants arriving together are +2 rather than one of them being silently lost.
The last seven. Four of them are relative changes - spending LUCK on a roll, spending it on an attack, mage effort, the stun track - and those are where a lost update reads as a wrong number rather than a missing field: spend three LUCK while something else writes the sheet and you could get it back. Two write to somebody else's sheet at the moment they are least able to notice. Stabilising a downed character sets frail and clears the round counter while that player is very likely staring at the sheet; the failure branch bumps the count towards death the same way. The vehicle HP mirror already re-read by hand, with a comment explaining why - this makes that re-read actually hold rather than merely be likely.
routes/locations.js was the remainder: mirroring a renamed token onto its NPC sheet, clearing the down-state counters when someone is healed above 0, and the CWN stim heal's strain cost. The healing one re-checks at write time and declines to write at all if the counters are already clear - that is a moment when the same sheet is being written from several directions at once. The migration in db.js deliberately stays as it is, documented in place: it runs once inside the schema-setup block, before the server listens and before any socket exists, so there is nothing for it to race with - and routing it through a helper that takes `db` as an argument while `db` is still being built would be worse than the problem. A test now walks the backend and fails if any file other than the queue itself writes a sheet directly. The guarantee only holds when both sides of a collision take the queue, so one forgotten call site quietly reopens the hole for every writer it can meet. Sabotage-checked: adding a stray UPDATE to a route makes it fail and names the file.
Found while checking the coverage was real rather than assumed, and it is the half of the bug a queue alone does not fix. Serialising the writes stopped them clobbering each other's other fields, but hull damage, vehicle HP and stun drain each worked out the new value from what they read before queueing - so two hits both subtracted from the same starting number and one of them did nothing. The patch is now computed inside the write for every relative value. Two tests pin the difference: three hits totalling ten leave a 20 HP hull at 10 when the value is worked out at write time, and never at 10 when it is worked out before. Also fixes a flake I introduced earlier: the upload cleanup test slept a fixed 50ms waiting on an async unlink, which passed alone and failed under a full suite. It polls now.
The three files whose descriptions were now incomplete rather than wrong: the socket handlers, which are the bulk of the writers; the sheet routes, whose table-wide resets changed shape; and db.js, which holds the one write that deliberately bypasses the queue and should say so where someone would look.
Caught by CI, which is the only place it could have been. The guard walks the backend from its own location, and I picked that apart by hand: a file URL's pathname is `/F:/x` on Windows and `/home/x` on Linux, so the `.slice(1)` that made the first one usable turned the second into a relative path and the scan found nothing to scan. fileURLToPath is the API for this and handles both. It is already used the same way in upload_constraints.test.js, which shipped in 1.10.0 and passed on this runner, so the form is proven there rather than merely reasoned about. Swept the rest: the other three tests that reach for a repository file pass a URL object straight to readFileSync, which Node resolves itself. This was the only hand-rolled one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Pre-merge checklist
Code quality:
Version & Release:
frontend/package.jsonversiondocker-compose.ymlAPP_VERSIONCHANGELOG.mdwith release notesBefore merging to main:
Related issues