Skip to content

fix session deadlock#8217

Merged
onursatici merged 1 commit into
developfrom
os/session-deadlock
Jun 2, 2026
Merged

fix session deadlock#8217
onursatici merged 1 commit into
developfrom
os/session-deadlock

Conversation

@onursatici

Copy link
Copy Markdown
Contributor

Summary

MemorySession is registered lazily. Because it implements default we don't need to register it to the session and the first call to ctx.allocator() would hold the session dashmap shard lock and insert the allocator. The problem is when we are in a aggregate function we are already potentially holding the same dashmap shard's lock, because the aggregate functino registry is also read from the session.

because session vars are type id keyed and type id's change on every build, if the aggregate function registry and memory session end up being on the same dashmap shard, we deadlock running an aggregate function.

This is fixing with a band aid, I am eagerly registering memory session because that is the only one that we forgot to register, hence we won't ever write lock the session after initialising now. I think the right solution is to not hold the read lock of the dashmap shard while executing kernels, which I will do in a follow up

Signed-off-by: Onur Satici <onur@spiraldb.com>
@AdamGS

AdamGS commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

I think we should have a baseline structure that isn't dashmap-dependent, instead just a struct with normal children, but it can follow up this change.

@codspeed-hq

codspeed-hq Bot commented Jun 2, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 20.12%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

❌ 4 regressed benchmarks
✅ 1271 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_bool_canonical_into[(1000, 10)] 30.6 µs 45.5 µs -32.83%
Simulation chunked_varbinview_canonical_into[(1000, 10)] 161.6 µs 197.8 µs -18.29%
Simulation chunked_varbinview_into_canonical[(1000, 10)] 176.2 µs 212.4 µs -17.02%
Simulation bitwise_not_vortex_buffer_mut[128] 246.1 ns 275.3 ns -10.6%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing os/session-deadlock (d92d2a0) with develop (81184e7)

Open in CodSpeed

@robert3005

Copy link
Copy Markdown
Contributor

fwiw I merged a change couple mins ago that makes the aggregate session registry not hold the lock

@onursatici onursatici added the changelog/fix A bug fix label Jun 2, 2026
@onursatici onursatici enabled auto-merge (squash) June 2, 2026 14:05
@onursatici onursatici merged commit 84a4a3f into develop Jun 2, 2026
70 of 72 checks passed
@onursatici onursatici deleted the os/session-deadlock branch June 2, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants