Hold the map set while committing a reserved transaction - #8252
Conversation
CommittableTx::commit() brackets apply_changes with lock_map_set() / unlock_map_set() when the transaction creates maps, so that Store::add_dynamic_map() does not mutate the Store's map set while another thread reads it. ReservedTx::commit_reserved() applies changes the same way but never took that lock. A reserved transaction does create maps - the first signature creates the signature tables - so add_dynamic_map() could write to maps with no lock held, racing a reader holding maps_lock in Store::get_map(). ThreadSanitizer reports this as a race in Store::get_map_internal. The lock has to be taken by the caller rather than inside add_dynamic_map(), because commit() already holds it by that point and ccf::pal::Mutex is not recursive. Store::commit() holds commit_lock when it reaches commit_reserved(), so this introduces a commit_lock -> maps_lock edge. There is no reverse edge: commit_lock is acquired in exactly one place, and commit() releases the map set before calling Store::commit(). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 75d99c5d-6efa-4048-8032-8c78b97208d9
There was a problem hiding this comment.
Pull request overview
Fixes a data race in the KV store map-set (Store::maps) by ensuring reserved transactions hold the map-set lock while applying changes that may create new maps, matching the locking already done for regular commits. This prevents Store::add_dynamic_map() from mutating the maps container concurrently with readers in Store::get_map().
Changes:
- Add
lock_map_set()/unlock_map_set()bracketing aroundapply_changes()inReservedTx::commit_reserved()when the reserved tx creates maps. - Add a TSAN-focused regression test that runs concurrent
Store::get_map()lookups while committing reserved transactions that create maps.
Custom instructions used:
.github/copilot-instructions.md
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/kv/committable_tx.h |
Hold the store map-set lock during commit_reserved() when applying changes that create maps. |
src/kv/test/kv_test.cpp |
Add a TSAN regression test exercising concurrent map creation (via reserved tx commit) and lookups. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Use a conditional RAII guard around apply_changes in both regular and reserved transaction commits, so the map set lock is released during stack unwinding. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f8b32bd5-6fc7-4161-99f3-60cd0b9b5380
DescriptionComparing 4 available runs from this branch (#8252) against the trend of the last 30 Each chart plots every benchmark as an axis, with values normalized so 100 is the EWMA baseline of recent Axis labels show the latest branch value and its difference from the main EWMA baseline, where 0% is on the baseline. They are coloured green where the latest run improves on the baseline, red where it regresses, and grey where the difference is within one std dev of the baseline (within noise). Higher is better for throughput and rate, lower for latency and memory. A benchmark which does not exist on Throughput (tx/s)---
config:
radar:
width: 620
height: 620
marginTop: 90
marginRight: 220
marginBottom: 60
marginLeft: 220
axisLabelFactor: 1.12
curveTension: 0.08
theme: base
themeCSS: |
.radarCurve-0{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-1{fill:color-mix(in srgb, #62B5E5 40%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-2{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-3{fill:var(--color-canvas-default,var(--bgColor-default,#fff))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarAxisLabel,.radarTitle{fill:var(--color-fg-default,var(--fgColor-default,#111827))!important;color:var(--color-fg-default,var(--fgColor-default,#111827))!important}
.radarCurve-4{stroke-width:1.5px!important;stroke-opacity:0.30!important}
.radarCurve-5{stroke-width:1.5px!important;stroke-opacity:0.40!important}
.radarCurve-6{stroke-width:1.5px!important;stroke-opacity:0.50!important}
.radarCurve-7{stroke-width:1.75px!important;stroke-opacity:1.00!important}
.radarAxisLabel:nth-of-type(1){fill:#808A94!important}
.radarAxisLabel:nth-of-type(2){fill:#808A94!important}
.radarAxisLabel:nth-of-type(3){fill:#E5484D!important}
.radarAxisLabel:nth-of-type(4){fill:#808A94!important}
.radarAxisLabel:nth-of-type(5){fill:#808A94!important}
.radarAxisLabel:nth-of-type(6){fill:#808A94!important}
.radarAxisLabel:nth-of-type(7){fill:#808A94!important}
themeVariables:
cScale0: "#62B5E5"
cScale1: "#62B5E5"
cScale2: "#62B5E5"
cScale3: "#62B5E5"
cScale4: "#F97316"
cScale5: "#F97316"
cScale6: "#F97316"
cScale7: "#F97316"
radar:
axisColor: "#9CA3AF"
graticuleColor: "#E5E7EB"
graticuleOpacity: 0
axisStrokeWidth: 1
curveOpacity: 0
---
radar-beta
axis b1["Basic Blocking 100ms: 3,049 tx/s ▬ 0%"]
axis b2["Basic Blocking 20ms: 15,093 tx/s ▬ 0%"]
axis b3["Basic Blocking 2ms: 29,758 tx/s ▼ 7%"]
axis b7["Basic JS: 7,491 tx/s ▬ +21%"]
axis b10["Historical Queries: 520,076 tx/s ▬ +26%"]
axis b13["L…g Certificate Blocking: 23,523 tx/s ▬ -4%"]
axis b15["Logging JWT Blocking: 14,890 tx/s ▬ 0%"]
curve stddev2_high["main EWMA + 2 std dev"]{100.44, 100.58, 111.44, 143.50, 186.40, 112.62, 102.20}
curve stddev1_high["main EWMA + 1 std dev"]{100.22, 100.29, 105.72, 121.75, 143.20, 106.31, 101.10}
curve stddev1_low["main EWMA - 1 std dev"]{99.78, 99.71, 94.28, 78.25, 56.80, 93.69, 98.90}
curve stddev2_low["main EWMA - 2 std dev"]{99.56, 99.42, 88.56, 56.50, 13.60, 87.38, 97.80}
curve branch_0["#8252 (3 runs earlier)"]{100.34, 100.07, 101.31, 123.19, 132.67, 101.29, 101.20}
curve branch_1["#8252 (2 runs earlier)"]{99.82, 100.44, 101.69, 124.73, 143.56, 100.88, 100.84}
curve branch_2["#8252 (1 run earlier)"]{100.02, 100.29, 100.93, 122.62, 146.24, 98.80, 100.54}
curve branch_3["#8252"]{99.78, 99.69, 92.75, 121.05, 125.62, 95.61, 99.79}
graticule polygon
max 247
ticks 0
showLegend false
Latency (ms)---
config:
radar:
width: 620
height: 620
marginTop: 90
marginRight: 220
marginBottom: 60
marginLeft: 220
axisLabelFactor: 1.12
curveTension: 0.08
theme: base
themeCSS: |
.radarCurve-0{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-1{fill:color-mix(in srgb, #62B5E5 40%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-2{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-3{fill:var(--color-canvas-default,var(--bgColor-default,#fff))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarAxisLabel,.radarTitle{fill:var(--color-fg-default,var(--fgColor-default,#111827))!important;color:var(--color-fg-default,var(--fgColor-default,#111827))!important}
.radarCurve-4{stroke-width:1.5px!important;stroke-opacity:0.30!important}
.radarCurve-5{stroke-width:1.5px!important;stroke-opacity:0.40!important}
.radarCurve-6{stroke-width:1.5px!important;stroke-opacity:0.50!important}
.radarCurve-7{stroke-width:1.75px!important;stroke-opacity:1.00!important}
.radarAxisLabel:nth-of-type(1){fill:#808A94!important}
.radarAxisLabel:nth-of-type(2){fill:#808A94!important}
.radarAxisLabel:nth-of-type(3){fill:#808A94!important}
.radarAxisLabel:nth-of-type(4){fill:#808A94!important}
.radarAxisLabel:nth-of-type(5){fill:#E5484D!important}
.radarAxisLabel:nth-of-type(6){fill:#808A94!important}
.radarAxisLabel:nth-of-type(7){fill:#808A94!important}
.radarAxisLabel:nth-of-type(8){fill:#E5484D!important}
.radarAxisLabel:nth-of-type(9){fill:#808A94!important}
.radarAxisLabel:nth-of-type(10){fill:#808A94!important}
themeVariables:
cScale0: "#62B5E5"
cScale1: "#62B5E5"
cScale2: "#62B5E5"
cScale3: "#62B5E5"
cScale4: "#F97316"
cScale5: "#F97316"
cScale6: "#F97316"
cScale7: "#F97316"
radar:
axisColor: "#9CA3AF"
graticuleColor: "#E5E7EB"
graticuleOpacity: 0
axisStrokeWidth: 1
curveOpacity: 0
---
radar-beta
axis b0["Basic Blocking 100ms: 99 ms ▬ 0%"]
axis b1["Basic Blocking 20ms: 19 ms ▬ 0%"]
axis b2["Basic Blocking 2ms: 9 ms ▬ -1%"]
axis b6["Basic JS: 40 ms ▬ +1%"]
axis b8["Commit Latency 16ms: 5.9 ms ▲ 24%"]
axis b9["Commit Latency 1ms: 1.94 ms ▬ -1%"]
axis b10["Commit Latency 256ms: 213 ms ▬ 0%"]
axis b11["Historical Queries: 60 ms ▲ 16%"]
axis b13["Logging Certificate Blocking: 22 ms ▬ +1%"]
axis b15["Logging JWT Blocking: 20 ms ▬ 0%"]
curve stddev2_high["main EWMA + 2 std dev"]{100.00, 100.00, 107.27, 113.55, 136.06, 105.16, 102.29, 115.02, 114.12, 100.00}
curve stddev1_high["main EWMA + 1 std dev"]{100.00, 100.00, 103.64, 106.78, 118.03, 102.58, 101.15, 107.51, 107.06, 100.00}
curve stddev1_low["main EWMA - 1 std dev"]{100.00, 100.00, 96.36, 93.22, 81.97, 97.42, 98.85, 92.49, 92.94, 100.00}
curve stddev2_low["main EWMA - 2 std dev"]{100.00, 100.00, 92.73, 86.45, 63.94, 94.84, 97.71, 84.98, 85.88, 100.00}
curve branch_0["#8252 (3 runs earlier)"]{100.00, 100.00, 98.96, 98.13, 90.98, 97.20, 100.49, 106.26, 96.50, 100.00}
curve branch_1["#8252 (2 runs earlier)"]{100.00, 100.00, 98.96, 98.13, 109.80, 98.40, 100.32, 100.46, 96.50, 100.00}
curve branch_2["#8252 (1 run earlier)"]{100.00, 100.00, 98.96, 98.13, 95.90, 100.28, 100.79, 100.46, 101.10, 100.00}
curve branch_3["#8252"]{100.00, 100.00, 98.96, 100.65, 124.31, 99.08, 100.31, 115.92, 101.10, 100.00}
graticule polygon
max 162
min 38
ticks 0
showLegend false
Memory (bytes)---
config:
radar:
width: 620
height: 620
marginTop: 90
marginRight: 220
marginBottom: 60
marginLeft: 220
axisLabelFactor: 1.12
curveTension: 0.08
theme: base
themeCSS: |
.radarCurve-0{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-1{fill:color-mix(in srgb, #62B5E5 40%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-2{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-3{fill:var(--color-canvas-default,var(--bgColor-default,#fff))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarAxisLabel,.radarTitle{fill:var(--color-fg-default,var(--fgColor-default,#111827))!important;color:var(--color-fg-default,var(--fgColor-default,#111827))!important}
.radarCurve-4{stroke-width:1.5px!important;stroke-opacity:0.30!important}
.radarCurve-5{stroke-width:1.5px!important;stroke-opacity:0.40!important}
.radarCurve-6{stroke-width:1.5px!important;stroke-opacity:0.50!important}
.radarCurve-7{stroke-width:1.75px!important;stroke-opacity:1.00!important}
.radarAxisLabel:nth-of-type(1){fill:#808A94!important}
.radarAxisLabel:nth-of-type(2){fill:#2DA44E!important}
.radarAxisLabel:nth-of-type(3){fill:#808A94!important}
.radarAxisLabel:nth-of-type(4){fill:#E5484D!important}
.radarAxisLabel:nth-of-type(5){fill:#808A94!important}
.radarAxisLabel:nth-of-type(6){fill:#808A94!important}
.radarAxisLabel:nth-of-type(7){fill:#2DA44E!important}
themeVariables:
cScale0: "#62B5E5"
cScale1: "#62B5E5"
cScale2: "#62B5E5"
cScale3: "#62B5E5"
cScale4: "#F97316"
cScale5: "#F97316"
cScale6: "#F97316"
cScale7: "#F97316"
radar:
axisColor: "#9CA3AF"
graticuleColor: "#E5E7EB"
graticuleOpacity: 0
axisStrokeWidth: 1
curveOpacity: 0
---
radar-beta
axis b1["Basic Blocking 100ms: 90.4 MiB ▬ 0%"]
axis b2["Basic Blocking 20ms: 90.1 MiB ▼ 2%"]
axis b3["Basic Blocking 2ms: 93.3 MiB ▬ -1%"]
axis b7["Basic JS: 94.2 MiB ▲ 13%"]
axis b10["Historical Queries: 150 MiB ▬ 0%"]
axis b13["Logging Certificate Blocking: 118 MiB ▬ 0%"]
axis b15["Logging JWT Blocking: 90.2 MiB ▼ 1%"]
curve stddev2_high["main EWMA + 2 std dev"]{101.70, 102.29, 101.50, 124.58, 102.35, 101.16, 101.46}
curve stddev1_high["main EWMA + 1 std dev"]{100.85, 101.14, 100.75, 112.29, 101.18, 100.58, 100.73}
curve stddev1_low["main EWMA - 1 std dev"]{99.15, 98.86, 99.25, 87.71, 98.82, 99.42, 99.27}
curve stddev2_low["main EWMA - 2 std dev"]{98.30, 97.71, 98.50, 75.42, 97.65, 98.84, 98.54}
curve branch_0["#8252 (3 runs earlier)"]{98.72, 96.87, 98.66, 112.64, 100.01, 100.08, 101.08}
curve branch_1["#8252 (2 runs earlier)"]{101.28, 98.68, 99.02, 111.31, 100.61, 99.66, 100.47}
curve branch_2["#8252 (1 run earlier)"]{99.18, 98.41, 99.40, 111.06, 99.97, 98.99, 99.36}
curve branch_3["#8252"]{100.13, 97.61, 99.34, 113.31, 100.16, 99.94, 99.10}
graticule polygon
max 142
min 58
ticks 0
showLegend false
Rate (ops/s)---
config:
radar:
width: 620
height: 620
marginTop: 90
marginRight: 220
marginBottom: 60
marginLeft: 220
axisLabelFactor: 1.12
curveTension: 0.08
theme: base
themeCSS: |
.radarCurve-0{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-1{fill:color-mix(in srgb, #62B5E5 40%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-2{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-3{fill:var(--color-canvas-default,var(--bgColor-default,#fff))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarAxisLabel,.radarTitle{fill:var(--color-fg-default,var(--fgColor-default,#111827))!important;color:var(--color-fg-default,var(--fgColor-default,#111827))!important}
.radarCurve-4{stroke-width:1.5px!important;stroke-opacity:0.30!important}
.radarCurve-5{stroke-width:1.5px!important;stroke-opacity:0.40!important}
.radarCurve-6{stroke-width:1.5px!important;stroke-opacity:0.50!important}
.radarCurve-7{stroke-width:1.75px!important;stroke-opacity:1.00!important}
.radarAxisLabel:nth-of-type(1){fill:#808A94!important}
.radarAxisLabel:nth-of-type(2){fill:#808A94!important}
.radarAxisLabel:nth-of-type(3){fill:#808A94!important}
.radarAxisLabel:nth-of-type(4){fill:#808A94!important}
.radarAxisLabel:nth-of-type(5){fill:#808A94!important}
.radarAxisLabel:nth-of-type(6){fill:#E5484D!important}
themeVariables:
cScale0: "#62B5E5"
cScale1: "#62B5E5"
cScale2: "#62B5E5"
cScale3: "#62B5E5"
cScale4: "#F97316"
cScale5: "#F97316"
cScale6: "#F97316"
cScale7: "#F97316"
radar:
axisColor: "#9CA3AF"
graticuleColor: "#E5E7EB"
graticuleOpacity: 0
axisStrokeWidth: 1
curveOpacity: 0
---
radar-beta
axis b0["CHAMP get: 37,835,541 ops/s ▬ +1%"]
axis b1["CHAMP put: 5,541,995 ops/s ▬ +2%"]
axis b2["KV deserialisation: 1,626,281 ops/s ▬ -1%"]
axis b3["KV serialisation: 1,402,328 ops/s ▬ +1%"]
axis b4["KV s…t deserialisation: 4,161 ops/s ▬ -1%"]
axis b5["KV snapshot serialisation: 4,016 ops/s ▼ 10%"]
curve stddev2_high["main EWMA + 2 std dev"]{103.78, 104.45, 104.47, 103.27, 102.23, 110.64}
curve stddev1_high["main EWMA + 1 std dev"]{101.89, 102.22, 102.24, 101.64, 101.11, 105.32}
curve stddev1_low["main EWMA - 1 std dev"]{98.11, 97.78, 97.76, 98.36, 98.89, 94.68}
curve stddev2_low["main EWMA - 2 std dev"]{96.22, 95.55, 95.53, 96.73, 97.77, 89.36}
curve branch_0["#8252 (3 runs earlier)"]{101.28, 101.14, 100.07, 100.56, 99.11, 100.51}
curve branch_1["#8252 (2 runs earlier)"]{101.23, 101.17, 100.56, 101.40, 99.69, 101.54}
curve branch_2["#8252 (1 run earlier)"]{101.36, 99.97, 102.07, 101.42, 96.38, 100.84}
curve branch_3["#8252"]{100.67, 101.99, 99.41, 101.40, 98.94, 89.82}
graticule polygon
max 119
min 81
ticks 0
showLegend false
|
Use jthread stop tokens so failed assertions or exceptions stop and join every reader before stack unwinding destroys the thread collection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f8b32bd5-6fc7-4161-99f3-60cd0b9b5380
Closes #8250.
Independent of the other open concurrency work - based on
main, touches no file that #8242..#8246 or #8249 touch.The race
CommittableTx::commit()bracketsapply_changeswith the map set lock when the transaction creates maps:ReservedTx::commit_reserved()applies changes the same way, through the sameapply_changes, but never took that lock.That matters because a reserved transaction does create maps - the first signature creates the signature tables. So
Store::add_dynamic_map()reacheswith no lock held, while another thread is inside
Store::get_map()holdingmaps_lockand searching the samestd::map. ThreadSanitizer reports it as a race inStore::get_map_internal:Note the reader is correctly synchronised. It is the writer that is not.
The fix
Do in
commit_reserved()whatcommit()already does.The lock has to be taken by the caller rather than inside
add_dynamic_map():commit()already holds it by the time it gets there, andccf::pal::Mutexis not recursive, so locking inside would deadlock the existing path.Lock ordering
Store::commit()holdscommit_lockwhen it reachescommit_reserved()viapending_tx->call(), so this adds acommit_lock -> maps_lockedge. There is no reverse edge:commit_lockis acquired in exactly one place (the top ofStore::commit), andcommit()releases the map set before it callsStore::commit(). The othermaps_lockholders (compact, rollback, snapshot, clone) never callStore::commit().Testing
Reserved transaction map creation is serialised with lookupsinkv_testreproduces the race directly: four reader threads callingStore::get_map()while the main thread commits reserved transactions that each create a map.Verified by reproduction, not just by inspection:
kv_testis clean under TSAN (36084 assertions).Two details the test depends on, both commented in place: the readers look up names in the range being inserted, so their search path traverses the nodes being written (looking up an unrelated key mostly misses them and the race does not surface); and nothing in the writer loop may take
maps_lock, since that would order the write against the readers and hide it.Runs in ~1.9s normally, ~11.7s under TSAN.
No CHANGELOG entry: no user-facing API or behaviour change.
Labelled
run-long-test.