Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .clang-format-ignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Third-party and generated; do not format
*node_modules*
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
tree-sitter/
tree-sitter-cypher/

.idea/
.vscode
.cursor
.vs
bazel-*
.clwb
Expand All @@ -13,6 +17,7 @@ history.txt
test_temp/
build/
install/
logs/

compile_commands.json

Expand Down
20 changes: 19 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,25 @@ make example # Examples
# Build with extensions
make extension-build # Build all extensions
make extension-debug # Debug build with extensions
make extension-release # Release build with extensions
make extension-release # Release build with extensions
```

### Windows (PowerShell, without make)

Requires CMake and Ninja in PATH (e.g. from LLVM or Visual Studio).

```powershell
# RelWithDebInfo (generates compile_commands.json for clangd/MCP)
cmake -B build/relwithdebinfo -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo .
cmake --build build/relwithdebinfo --config RelWithDebInfo

# Release
cmake -B build/release -G Ninja -DCMAKE_BUILD_TYPE=Release .
cmake --build build/release --config Release

# Debug
cmake -B build/debug -G Ninja -DCMAKE_BUILD_TYPE=Debug .
cmake --build build/debug --config Debug
```

## Test Commands
Expand Down
8 changes: 8 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,14 @@ if(APPLE)
endif()
if(WIN32)
set(OS_NAME "windows")
# Prevent Windows min/max macros from breaking std::min/std::max (needed for Clang-on-Windows too)
add_compile_definitions(NOMINMAX)
# Required for Windows SDK headers (winnt.h) when using Clang; MSVC branch sets _AMD64_ later
if(CMAKE_SIZEOF_VOID_P EQUAL 8)
add_compile_definitions(_AMD64_)
else()
add_compile_definitions(_X86_)
endif()
endif()
if(UNIX AND NOT APPLE)
set(OS_NAME "linux") # sorry BSD
Expand Down
4 changes: 2 additions & 2 deletions cmake/templates/system_config.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ struct StorageConfig {
static constexpr uint64_t NODE_GROUP_SIZE = static_cast<uint64_t>(1) << NODE_GROUP_SIZE_LOG2;
// The number of CSR lists in a leaf region.
static constexpr uint64_t CSR_LEAF_REGION_SIZE_LOG2 =
std::min(static_cast<uint64_t>(10), NODE_GROUP_SIZE_LOG2 - 1);
(std::min)(static_cast<uint64_t>(10), NODE_GROUP_SIZE_LOG2 - 1);
static constexpr uint64_t CSR_LEAF_REGION_SIZE = static_cast<uint64_t>(1)
<< CSR_LEAF_REGION_SIZE_LOG2;
static constexpr uint64_t CHUNKED_NODE_GROUP_CAPACITY =
std::min(static_cast<uint64_t>(2048), NODE_GROUP_SIZE);
(std::min)(static_cast<uint64_t>(2048), NODE_GROUP_SIZE);

// Maximum size for a segment in bytes
static constexpr uint64_t MAX_SEGMENT_SIZE_LOG2 = @LBUG_MAX_SEGMENT_SIZE_LOG2@;
Expand Down
92 changes: 92 additions & 0 deletions docs/incidents/2026-02-16-connection-close-sigsegv.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# Incident: SIGSEGV when Connection is destroyed while query workers are still running

**Date:** 2026-02-16
**Status:** Resolved
**Severity:** High (crash; observed in Node.js addon and during COPY)
**Component:** Main — Connection / ClientContext lifecycle, Processor task scheduler
**Author:** [@vkozio](https://github.com/vkozio)

---

## Summary

Closing a Connection (or Database) while a query was still executing (or while worker threads were cleaning up after an exception) could lead to **SIGSEGV (signal 11)**. The destructor of `Connection` destroyed `ClientContext` immediately; worker threads in `TaskScheduler` could still hold references to that context (e.g. in `ProcessorTask::finalize()` or when handling exceptions). Those workers then touched already-freed memory. A related symptom was **TransactionManagerException** ("Only one write transaction at a time") when a second write was attempted while the first was still in progress; if the application closed the connection in response, the ensuing teardown could trigger the crash.

---

## Impact

- **Affected:** Any embedding that closes a Connection (or Database) while a query is in flight or immediately after an error — notably the **Node.js addon** (e.g. `connection.close()` or `db.close()` while a query or result iteration is still running).
- **Observed:** SIGSEGV during or shortly after Connection/DB close; in one scenario, TransactionManagerException during COPY followed by Fatal signal 11.
- **User impact:** Process crash; data loss only if the process was in the middle of a write (the DB itself is not corrupted by this bug).

---

## Root cause

### Lifecycle before the fix

1. **Connection** owns a `std::unique_ptr<ClientContext> clientContext`.
2. **Query execution:** `ClientContext::query()` → `QueryProcessor::execute()` → `TaskScheduler::scheduleTaskAndWaitOrError(task, context)`. The `ExecutionContext` passed to the task holds a raw pointer `clientContext`. Worker threads run `ProcessorTask::run()` and, on exit or exception, may call `ProcessorTask::finalize()`, which can access `executionContext->clientContext`.
3. **Connection destructor** (before fix): Set `clientContext->preventTransactionRollbackOnDestruction = true` and then destroyed `clientContext` (via `unique_ptr` reset). There was **no wait** for in-flight queries to finish.
4. If the application closed the Connection while a query was still running (or while a worker was in `finalize()` after an exception), a worker could dereference `clientContext` after it had been destroyed → use-after-free → SIGSEGV.

### Why TransactionManagerException could precede SIGSEGV

With a single write transaction allowed, a second write (e.g. concurrent COPY) throws TransactionManagerException. The application or test might then close the connection. That close ran the destructor while the first write’s workers were still shutting down or finalizing, leading to the same use-after-free and SIGSEGV.

### Existing partial mitigation

`Connection::~Connection()` already set `preventTransactionRollbackOnDestruction = true` so that `ClientContext`’s destructor does not attempt a transaction rollback (which would touch the database and could also crash if the database or connection was already torn down). That avoided one class of crash but did not address workers still holding references to `ClientContext`.

---

## Resolution

### Change: wait for no active query before destroying ClientContext

1. **ClientContext** (`src/include/main/client_context.h`, `src/main/client_context.cpp`):
- Added `std::atomic<uint32_t> activeQueryCount`, `std::mutex mtxForClose`, `std::condition_variable cvForClose`.
- Added `registerQueryStart()`, `registerQueryEnd()`, `waitForNoActiveQuery()`.
- `registerQueryEnd()` decrements the count and notifies the condition variable when it reaches zero.

2. **QueryProcessor::execute()** (`src/processor/processor.cpp`):
- At entry: `context->clientContext->registerQueryStart()`.
- RAII guard (stack object) calls `registerQueryEnd()` in its destructor so it is always called on normal return or on exception.
- Thus every path that runs `scheduleTaskAndWaitOrError` is bracketed by start/end.

3. **Connection::~Connection()** (`src/main/connection.cpp`):
- Before setting `preventTransactionRollbackOnDestruction` and before `clientContext` is destroyed, call `clientContext->waitForNoActiveQuery()`.
- The destructor therefore blocks until all queries that had been started on this connection have completed (and their workers have finished), so no worker can touch `ClientContext` after it is destroyed.

### Files changed

- `src/include/main/client_context.h` — declarations and members.
- `src/main/client_context.cpp` — implementations of register/wait.
- `src/main/connection.cpp` — `waitForNoActiveQuery()` in destructor.
- `src/processor/processor.cpp` — include `client_context.h`, start/end guard in `execute()`.

---

## Verification

- **C API:** `build/relwithdebinfo/test/c_api/c_api_test --gtest_filter="*Connection*"` — all 11 tests passed (CreationAndDestroy, Query, etc.).
- **Node addon:** `make nodejstest` — 163 passed, 0 failed; suite "Resilience (close during/after use)" passed (close while iterating, hasNext/getNext after DB closed, etc.).
- **Copy tests:** `build/relwithdebinfo/test/copy/copy_tests` — 19 tests passed.

---

## Lessons learned

1. **Ownership and worker lifetime:** If worker threads hold raw pointers to an object, the owner must ensure those workers have finished (or have been told to stop) before destroying the object. A simple ref-count or “active operation” count plus condition variable is one way to do that.
2. **Destructors and async work:** Connection/ClientContext teardown is a critical path; document and enforce the rule that no query may be in flight when the destructor runs. The RAII guard in `execute()` keeps the count accurate even when exceptions occur.
3. **Embeddings (Node, etc.):** Applications may close connections on error or on user action without draining results. The core must tolerate “close while busy” by waiting for in-flight work instead of destroying state immediately.

---

## References

- Fix commit: Connection wait for in-flight queries before destroying ClientContext (activeQueryCount, registerQueryStart/End, waitForNoActiveQuery).
- Code: `src/include/main/client_context.h`, `src/main/client_context.cpp`, `src/main/connection.cpp`, `src/processor/processor.cpp`.
- Related: `preventTransactionRollbackOnDestruction` in `Connection::~Connection()` and `ClientContext` destructor (rollback skipped on teardown).
- Node addon tests: `tools/nodejs_api/test/test_resilience.js`, `test/test_connection.js`.
132 changes: 132 additions & 0 deletions docs/incidents/2026-02-16-copy-rel-segment-planner-schema-groups.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
# Incident: CopyRelSegmentTest failure — "Try to partition multiple factorization group"

**Date:** 2026-02-16
**Status:** Resolved
**Severity:** Medium (E2E test failure; no production impact)
**Component:** Planner — COPY FROM for relationship tables
**Author:** [@vkozio](https://github.com/vkozio)

---

## Summary

The E2E test `copy~segmentation.CopyRelSegmentTest` failed with a runtime exception during planning:

```text
Runtime exception: Try to partition multiple factorization group. This should not happen.
```

The failure occurred in `LogicalPartitioner::computeFactorizedSchema()` when planning `COPY <rel_table> FROM (subquery)` for a query source. The root cause was an inconsistent use of schema group checks in `planCopyRelFrom`: the code used `getGroupsPosInScope().size() == 1` to decide whether to add an Accumulate operator, while `LogicalPartitioner` later enforces a single group with `getNumGroups() != 1`. In cases where the plan had more than one factorization group but only one group had expressions in scope, no Accumulate was added and the Partitioner received a multi-group schema and threw.

---

## Impact

- **Affected:** E2E test suite; any use of `COPY <rel_table> FROM (subquery)` when the subquery plan had multiple factorization groups and only one group in scope.
- **Observed:** Test `copy~segmentation.CopyRelSegmentTest` (e.g. Test #2336) failed; subsequent STORAGE_INFO checks in the same test also failed because the first COPY never ran successfully.
- **User impact:** None reported; failure was caught by tests.

---

## Root cause

### Schema APIs involved

In `src/include/planner/operator/schema.h`:

- **`getNumGroups()`** (public) returns `groups.size()` — the total number of factorization groups in the schema.
- **`getGroupsPosInScope()`** returns the set of group positions that have at least one expression in the current scope (projected columns).

So the schema can have `getNumGroups() > 1` while `getGroupsPosInScope().size() == 1` if only one of the groups has expressions in scope.

### Logic before the fix

In `src/planner/plan/plan_copy.cpp`, `planCopyRelFrom()` for `ScanSourceType::QUERY`:

1. Builds a plan with `planQuery(...)`.
2. Decides whether to add an Accumulate to “flatten” to a single data chunk:
- **Condition used:** `if (schema->getGroupsPosInScope().size() == 1) break;`
- So Accumulate was **skipped** when exactly one group had expressions in scope, even if the schema had multiple groups.
3. Then appends IndexScan (primary key lookup) and Partitioner.

`LogicalPartitioner::computeFactorizedSchema()` (in `src/planner/operator/logical_partitioner.cpp`) does:

- `copyChildSchema(0)` then `validateSingleGroup(*schema)`.
- `validateSingleGroup()` throws if `schema.getNumGroups() != 1`.

So the Partitioner requires a **single group** (`getNumGroups() == 1`), not “single group in scope”.

### Failure scenario

1. For some COPY rel-from-query plans, the query plan had **two** factorization groups but only **one** group had expressions in scope.
2. `getGroupsPosInScope().size() == 1` → Accumulate was not added.
3. IndexScan copied the child schema (still two groups) and added its expressions into one group; the schema still had two groups.
4. Partitioner copied that schema and called `validateSingleGroup()` → `getNumGroups() != 1` → exception.

### Inconsistency with node COPY

`planCopyNodeFrom()` already used the stricter check:

```cpp
if (plan.getSchema()->getNumGroups() > 1) {
appendAccumulate(...);
}
```

So node COPY and rel COPY were using different criteria for when to accumulate, and only the node path was aligned with the downstream single-group assumption.

---

## Timeline

| When | What |
|------------|------|
| Test run | `copy~segmentation.CopyRelSegmentTest` failed on first `COPY edges from (unwind range(...) return num, num+1)` with "Try to partition multiple factorization group". |
| Diagnosis | Stack trace pointed to `validateSingleGroup` in `logical_partitioner.cpp`; comparison with `planCopyNodeFrom` and Schema API usage revealed the wrong condition in `planCopyRelFrom`. |
| Fix | Condition in `planCopyRelFrom` changed from `getGroupsPosInScope().size() == 1` to `getNumGroups() <= 1`, and Accumulate comment kept in sync with node path. |

---

## Resolution

**Change (single file):** `src/planner/plan/plan_copy.cpp`

- **Before:**
`if (schema->getGroupsPosInScope().size() == 1) break;`
- **After:**
`if (schema->getNumGroups() <= 1) break;`

So we only skip adding Accumulate when the query plan has at most one factorization group. When it has more than one, we always add Accumulate so that the plan reaching the Partitioner has a single group, satisfying `validateSingleGroup()`.

**Verification:** Re-run the failing test (e.g. with RelWithDebInfo build):

```bash
E2E_TEST_FILES_DIRECTORY=test/test_files build/relwithdebinfo/test/runner/e2e_test --gtest_filter="copy~segmentation.CopyRelSegmentTest"
```

---

## Lessons learned

1. **Use a single, clear contract for “single group”:** Downstream operators (here, Partitioner) that require one group should be matched by one consistent check (e.g. `getNumGroups() <= 1`) at the planning site. Mixing “groups in scope” and “number of groups” led to a mismatch.
2. **Keep similar code paths aligned:** Node and rel COPY-from-query had the same goal (one chunk for Copy operator) but used different schema checks; aligning rel COPY with node COPY (and with Partitioner) would have avoided the bug.
3. **Tests are the canary:** The E2E test exposed a planner path that only fails when the query plan has multiple groups with only one in scope; unit tests that only use single-group plans would not have caught this.

---

## Action items

| Item | Owner | Status |
|------|--------|--------|
| Fix applied and documented | — | Done |
| Consider adding a planner unit test that builds a COPY rel-from-query with a multi-group subquery and asserts no exception and single group before Partitioner | Optional | Open |
| Consider a short comment in `plan_copy.cpp` next to the condition that the Partitioner requires exactly one group (reference to `validateSingleGroup`) | Optional | Open |

---

## References

- Fix: `src/planner/plan/plan_copy.cpp` (condition in `planCopyRelFrom` for `ScanSourceType::QUERY`).
- Failing test: `test/test_files/copy/segmentation.test` — case `CopyRelSegmentTest`.
- Downstream check: `src/planner/operator/logical_partitioner.cpp` — `validateSingleGroup`.
- Schema API: `src/include/planner/operator/schema.h` — `getNumGroups()`, `getGroupsPosInScope()`.
Loading