From e49778d5aed9754a44f8fc581d571b401cf883dc Mon Sep 17 00:00:00 2001 From: Ufuk Kayserilioglu Date: Thu, 6 Aug 2026 23:39:32 +0300 Subject: [PATCH 1/2] Convert query result rows one row at a time `Rubydex::Query::Result#each` built every row before it yielded the first one. A caller that read one row still paid for all of them, twice: once as `CCell`s in C memory, and once as Ruby objects. The row iterator is now a cursor. `rdx_result_set_rows` builds only the column strings. `rdx_rows_iter_next` releases the cells of the previous row, then converts one row under the graph read lock. The lock is released before the function returns, so the caller may run any code between two calls. `each` now converts one row, yields it, and discards it, unless `rows` already built the whole array. `first` and `find` therefore stop as soon as the block breaks. `rows` keeps its behaviour: it collects every row through the same cursor, freezes the array, and reuses it. The walk also shares its Hash keys. It builds one frozen UTF-8 String per column, and every row of the walk uses those keys. `rb_hash_aset` stores a frozen String key as it is, instead of duplicating and freezing it, so a wide result no longer allocates a key String per cell. Allocations for 300 rows and 2 columns, counted with `GC.stat(:total_allocated_objects)`: | Call | Before | After | | --------------- | ------ | ----- | | `result.first` | 1516 | 19 | | `result.rows` | 1506 | 907 | Two contracts change: - A `CResultRow` from `rdx_rows_iter_next` stays valid only until the next call or until `rdx_rows_iter_free`, not until the free alone. The C extension copies every value into Ruby objects at once, so it complies. - The graph read lock is no longer held for the whole walk. A block that writes to the graph therefore changes how later rows resolve their node cells. A declaration that disappeared falls back to its display name. Holding the lock across `rb_yield` is not an option, because a block that calls `graph.index_source` would then deadlock the thread. The new tests cover the early exit, an `Enumerator` without a block, a `break` and a raising block, and the shared frozen keys. --- ext/rubydex/query.c | 106 ++++++++++++++++++------ rust/rubydex-sys/src/cypher_api.rs | 128 ++++++++++++++++------------- test/graph_test.rb | 90 +++++++++++++++++++- 3 files changed, 241 insertions(+), 83 deletions(-) diff --git a/ext/rubydex/query.c b/ext/rubydex/query.c index 8a171936..caa76eba 100644 --- a/ext/rubydex/query.c +++ b/ext/rubydex/query.c @@ -227,37 +227,88 @@ static VALUE cypher_cell_to_value(VALUE graph_obj, const struct CCell *cell) { } } -// Body function for rb_ensure in Rubydex::Query::Result#rows — walks the iterator and builds the -// rows array. May raise if cell conversion (e.g. handle construction) fails; the ensure function -// frees the iterator regardless. -static VALUE query_rows_yield(VALUE args) { +// Builds the Hash keys of one walk: one frozen UTF-8 String per column. The keys are shared by +// every row of the walk, so a wide result does not allocate a key String per cell. `rb_hash_aset` +// stores a frozen String key as it is, instead of duplicating and freezing it. +static VALUE query_row_keys(struct CRowsIter *iter) { + size_t count = rdx_rows_iter_column_count(iter); + const char *const *columns = rdx_rows_iter_columns(iter); + VALUE keys = rb_ary_new_capa((long)count); + + for (size_t i = 0; i < count; i++) { + rb_ary_push(keys, rb_str_freeze(rb_utf8_str_new_cstr(columns[i]))); + } + + return keys; +} + +// Converts one row of the cursor into a Hash keyed by the shared Strings in `keys`. +static VALUE query_row_to_hash(VALUE graph_obj, VALUE keys, const struct CResultRow *row) { + long column_count = RARRAY_LEN(keys); + VALUE hash = rb_hash_new_capa(column_count); + + for (size_t c = 0; c < row->len && (long)c < column_count; c++) { + rb_hash_aset(hash, RARRAY_AREF(keys, (long)c), cypher_cell_to_value(graph_obj, &row->cells[c])); + } + + return hash; +} + +// Body function for rb_ensure in Rubydex::Query::Result#rows — walks the cursor and collects every +// row. May raise if cell conversion (e.g. handle construction) fails; the ensure function frees the +// cursor regardless. +static VALUE query_rows_collect(VALUE args) { VALUE graph_obj = rb_ary_entry(args, 0); struct CRowsIter *iter = (struct CRowsIter *)(uintptr_t)NUM2ULL(rb_ary_entry(args, 1)); - size_t column_count = rdx_rows_iter_column_count(iter); - const char *const *columns = rdx_rows_iter_columns(iter); + VALUE keys = query_row_keys(iter); VALUE rows = rb_ary_new_capa((long)rdx_rows_iter_len(iter)); struct CResultRow row; while (rdx_rows_iter_next(iter, &row)) { - VALUE hash = rb_hash_new(); - for (size_t c = 0; c < row.len && c < column_count; c++) { - VALUE key = rb_utf8_str_new_cstr(columns[c]); - rb_hash_aset(hash, key, cypher_cell_to_value(graph_obj, &row.cells[c])); - } - rb_ary_push(rows, hash); + rb_ary_push(rows, query_row_to_hash(graph_obj, keys, &row)); } return rows; } -// Ensure function for rb_ensure in Rubydex::Query::Result#rows to always free the iterator. +// Body function for rb_ensure in Rubydex::Query::Result#each — walks the cursor and yields one row +// at a time, so only one row exists as Ruby objects at any moment. A `break` or an exception in the +// block leaves through rb_ensure, which frees the cursor. +static VALUE query_rows_stream(VALUE args) { + VALUE graph_obj = rb_ary_entry(args, 0); + struct CRowsIter *iter = (struct CRowsIter *)(uintptr_t)NUM2ULL(rb_ary_entry(args, 1)); + + VALUE keys = query_row_keys(iter); + + struct CResultRow row; + while (rdx_rows_iter_next(iter, &row)) { + rb_yield(query_row_to_hash(graph_obj, keys, &row)); + } + + return Qnil; +} + +// Ensure function for rb_ensure to always free the cursor. static VALUE query_rows_ensure(VALUE args) { struct CRowsIter *iter = (struct CRowsIter *)(uintptr_t)NUM2ULL(rb_ary_entry(args, 1)); rdx_rows_iter_free(iter); return Qnil; } +// Opens a cursor over the result set's rows and runs `body` with it. The cursor is always freed. +static VALUE query_with_rows(VALUE self, VALUE (*body)(VALUE)) { + QueryResultData *data = query_result_data(self); + + struct CRowsIter *iter = rdx_result_set_rows(data->result_set, rdxi_graph_from_object(data->graph_obj)); + if (iter == NULL) { + rb_raise(rb_eRuntimeError, "failed to create iterator"); + } + + VALUE args = rb_ary_new_from_args(2, data->graph_obj, ULL2NUM((uintptr_t)iter)); + return rb_ensure(body, args, query_rows_ensure, args); +} + /* * call-seq: * rows -> Array[Hash[String, Object]] @@ -268,21 +319,14 @@ static VALUE query_rows_ensure(VALUE args) { * the first call and reused afterwards. */ static VALUE rdxr_query_result_rows(VALUE self) { - QueryResultData *data = query_result_data(self); - - if (!NIL_P(data->rows)) { - return data->rows; - } - - struct CRowsIter *iter = rdx_result_set_rows(data->result_set, rdxi_graph_from_object(data->graph_obj)); - if (iter == NULL) { - rb_raise(rb_eRuntimeError, "failed to create iterator"); + if (!NIL_P(query_result_data(self)->rows)) { + return query_result_data(self)->rows; } - VALUE args = rb_ary_new_from_args(2, data->graph_obj, ULL2NUM((uintptr_t)iter)); - data->rows = rb_ary_freeze(rb_ensure(query_rows_yield, args, query_rows_ensure, args)); + VALUE rows = rb_ary_freeze(query_with_rows(self, query_rows_collect)); + query_result_data(self)->rows = rows; - return data->rows; + return rows; } /* @@ -312,11 +356,21 @@ static VALUE rdxr_query_result_columns(VALUE self) { * * Yields every row as a Hash keyed by RETURN column name. Rubydex::Query::Result is Enumerable, so * +map+, +select+, and the rest of Enumerable work on the rows. + * + * Unless #rows already built the whole array, +each+ converts one row at a time and discards it + * after the block returns. A large result therefore needs memory for one row, not for all of them, + * and +first+ or +find+ stops converting as soon as the block breaks. */ static VALUE rdxr_query_result_each(VALUE self) { RETURN_ENUMERATOR(self, 0, 0); - VALUE rows = rdxr_query_result_rows(self); + VALUE rows = query_result_data(self)->rows; + + if (NIL_P(rows)) { + query_with_rows(self, query_rows_stream); + return self; + } + long length = RARRAY_LEN(rows); for (long i = 0; i < length; i++) { diff --git a/rust/rubydex-sys/src/cypher_api.rs b/rust/rubydex-sys/src/cypher_api.rs index 537211d0..2dee0ca0 100644 --- a/rust/rubydex-sys/src/cypher_api.rs +++ b/rust/rubydex-sys/src/cypher_api.rs @@ -246,11 +246,16 @@ pub struct CResultRow { pub len: usize, } -/// Iterator over structured query result rows. Opaque from the C side — use -/// `rdx_rows_iter_*` methods to work with it. +/// A cursor over an executed result set's rows. It converts one row per `rdx_rows_iter_next` call, +/// so a caller can walk a large result set without a copy of every cell in memory at once. Opaque +/// from the C side — use the `rdx_rows_iter_*` methods to work with it. pub struct CRowsIter { + /// Borrowed from the caller, which must keep it alive for the whole life of the cursor. + result_set: *const CResultSet, + graph: GraphPointer, columns: Box<[*const c_char]>, - rows: Box<[CResultRow]>, + /// Cells of the row that the last `rdx_rows_iter_next` call produced. + current: Vec, index: usize, } @@ -484,46 +489,36 @@ pub unsafe extern "C" fn rdx_result_set_row_count(result_set: *const CResultSet) unsafe { &*result_set }.0.rows.len() } -/// Materializes an executed result set as a row iterator (column names + typed rows), so callers -/// can build their own value/handle objects instead of formatted text. Returns null when -/// `result_set` is null. +/// Opens a cursor over the rows of an executed result set, so callers can build their own +/// value/handle objects instead of formatted text. The cursor converts a row only when +/// `rdx_rows_iter_next` asks for it. Returns null when `result_set` is null. /// /// # Safety /// -/// - `result_set` must be a valid pointer returned by `rdx_query_execute`, or null. -/// - `pointer` must be a valid `GraphPointer` previously returned by this crate. +/// - `result_set` must be a valid pointer returned by `rdx_query_execute`, or null. It must stay +/// alive until `rdx_rows_iter_free` releases the cursor. +/// - `pointer` must be a valid `GraphPointer` previously returned by this crate. It must stay valid +/// for the same span. #[unsafe(no_mangle)] pub unsafe extern "C" fn rdx_result_set_rows(result_set: *const CResultSet, pointer: GraphPointer) -> *mut CRowsIter { if result_set.is_null() { return ptr::null_mut(); } - let result_set = &unsafe { &*result_set }.0; - - with_graph(pointer, |graph| { - let columns: Box<[*const c_char]> = result_set.columns.iter().map(|name| utils::cstring_raw(name)).collect(); - - let rows: Box<[CResultRow]> = result_set - .rows - .iter() - .map(|row| { - let cells: Vec = row.iter().map(|cell| build_cell(graph, cell)).collect(); - let len = cells.len(); - let cells_ptr = if cells.is_empty() { - ptr::null_mut() - } else { - Box::into_raw(cells.into_boxed_slice()).cast::() - }; - CResultRow { cells: cells_ptr, len } - }) - .collect(); - - Box::into_raw(Box::new(CRowsIter { - columns, - rows, - index: 0, - })) - }) + let columns: Box<[*const c_char]> = unsafe { &*result_set } + .0 + .columns + .iter() + .map(|name| utils::cstring_raw(name)) + .collect(); + + Box::into_raw(Box::new(CRowsIter { + result_set, + graph: pointer, + columns, + current: Vec::new(), + index: 0, + })) } /// Frees a result set previously returned by `rdx_query_execute`. @@ -581,12 +576,17 @@ pub unsafe extern "C" fn rdx_rows_iter_len(iter: *const CRowsIter) -> usize { return 0; } let iter = unsafe { &*iter }; - iter.rows.len() + unsafe { &*iter.result_set }.0.rows.len() } -/// Advances the iterator and copies the next row into `out`. Returns `true` if a row was read, -/// `false` if the iterator is exhausted. The copied `CResultRow` is a view into the iterator's -/// owned cells — it remains valid until `rdx_rows_iter_free` is called. +/// Converts the next row and copies a view of it into `out`. Returns `true` if a row was read, +/// `false` if the cursor is exhausted. The cells belong to the cursor, so the copied `CResultRow` +/// stays valid only until the next `rdx_rows_iter_next` call or `rdx_rows_iter_free`, whichever +/// comes first. Read the row's values before calling either. +/// +/// The graph read lock is taken for the conversion of one row and released before this function +/// returns, so a caller may run arbitrary code, including code that writes to the graph, between +/// two calls. /// /// # Safety /// @@ -599,14 +599,26 @@ pub unsafe extern "C" fn rdx_rows_iter_next(iter: *mut CRowsIter, out: *mut CRes } let it = unsafe { &mut *iter }; - if it.index >= it.rows.len() { - return false; - } - let row = it.rows[it.index]; + // The previous row is out of scope for the caller now, so release its cells before the next one. + unsafe { free_cells(&it.current) }; + it.current.clear(); + + let result_set = unsafe { &*it.result_set }; + let Some(row) = result_set.0.rows.get(it.index) else { + return false; + }; it.index += 1; + + it.current = with_graph(it.graph, |graph| { + row.iter().map(|cell| build_cell(graph, cell)).collect() + }); + unsafe { - *out = row; + *out = CResultRow { + cells: it.current.as_mut_ptr(), + len: it.current.len(), + }; } true @@ -656,8 +668,19 @@ unsafe fn free_cell(cell: &CCell) { } } -/// Frees a `CRowsIter` previously returned by `rdx_result_set_rows`, including all column strings, -/// row cells, and nested allocations. +/// Frees every cell of one row. +/// +/// # Safety +/// +/// - `cells` must come from `build_cell`, and nothing else may own their allocations. +unsafe fn free_cells(cells: &[CCell]) { + for cell in cells { + unsafe { free_cell(cell) }; + } +} + +/// Frees a `CRowsIter` previously returned by `rdx_result_set_rows`, including its column strings +/// and the cells of the row it converted last. /// /// # Safety /// @@ -670,20 +693,13 @@ pub unsafe extern "C" fn rdx_rows_iter_free(iter: *mut CRowsIter) { let it = unsafe { Box::from_raw(iter) }; - // Free column C strings (the boxed slice itself is freed when `it` drops). + // The cursor owns the cells of the last row it produced. The `Vec` and the boxed slice of + // column pointers drop with `it`; their contents do not. + unsafe { free_cells(&it.current) }; + for &col in &it.columns { if !col.is_null() { let _ = unsafe { CString::from_raw(col.cast_mut()) }; } } - - // Free cells in each row (the boxed slice of CResultRow is freed when `it` drops). - for row in &it.rows { - if !row.cells.is_null() && row.len > 0 { - let cells = unsafe { Box::from_raw(ptr::slice_from_raw_parts_mut(row.cells, row.len)) }; - for cell in &cells { - unsafe { free_cell(cell) }; - } - } - } } diff --git a/test/graph_test.rb b/test/graph_test.rb index 55a9535a..217ae68a 100644 --- a/test/graph_test.rb +++ b/test/graph_test.rb @@ -1741,7 +1741,7 @@ def test_result_reports_columns_rows_and_emptiness assert_equal(2, result.length) refute_empty(result) - # Enumerable comes from `each`, which walks the same memoized rows. + # Enumerable comes from `each`. It walks the memoized rows once `rows` has built them. assert_equal(["Animal", "Dog"], result.map { |row| row["c.name"] }) assert_same(result.rows, result.rows) @@ -1753,6 +1753,85 @@ def test_result_reports_columns_rows_and_emptiness end end + def test_each_converts_only_the_rows_the_block_consumes + with_context do |context| + context.write!("zoo.rb", 300.times.map { |i| "class Klass#{i}; end" }.join("\n")) + + graph = Rubydex::Graph.new + graph.index_all(context.glob("**/*.rb")) + graph.resolve + + query = Rubydex::Query.parse("MATCH (c:Class) WHERE c.name STARTS WITH 'Klass' RETURN c.name, c.kind") + assert_equal(300, query.run(graph).size) + + # `first` breaks out of `each` after one row, so only that row becomes Ruby objects. `rows` + # converts all 300. Each run gets a fresh result, because `rows` memoizes its array. + early = count_allocations { query.run(graph).first } + every = count_allocations { query.run(graph).rows } + + assert_operator(early * 10, :<, every, "expected `first` to convert one row, not all of them") + end + end + + def test_each_without_a_block_returns_an_enumerator + with_context do |context| + context.write!("zoo.rb", "class Animal; end\nclass Dog < Animal; end\n") + + graph = Rubydex::Graph.new + graph.index_all(context.glob("**/*.rb")) + graph.resolve + + result = Rubydex::Query.parse( + "MATCH (c:Class) WHERE c.name IN ['Animal', 'Dog'] RETURN c.name ORDER BY c.name", + ).run(graph) + + enumerator = result.each + assert_instance_of(Enumerator, enumerator) + assert_equal([{ "c.name" => "Animal" }, { "c.name" => "Dog" }], enumerator.to_a) + end + end + + def test_each_stays_usable_after_a_break_and_after_a_raising_block + with_context do |context| + context.write!("zoo.rb", "class Animal; end\nclass Dog < Animal; end\n") + + graph = Rubydex::Graph.new + graph.index_all(context.glob("**/*.rb")) + graph.resolve + + result = Rubydex::Query.parse( + "MATCH (c:Class) WHERE c.name IN ['Animal', 'Dog'] RETURN c.name ORDER BY c.name", + ).run(graph) + + # Both exits leave the walk through `rb_ensure`, which frees the row cursor. + assert_equal("Animal", result.each { |row| break row["c.name"] }) + assert_raises(RuntimeError) { result.each { raise("boom") } } + + assert_equal(["Animal", "Dog"], result.map { |row| row["c.name"] }) + assert_equal(2, result.rows.length) + end + end + + def test_rows_share_one_frozen_key_per_column + with_context do |context| + context.write!("zoo.rb", "class Animal; end\nclass Dog < Animal; end\n") + + graph = Rubydex::Graph.new + graph.index_all(context.glob("**/*.rb")) + graph.resolve + + rows = Rubydex::Query.parse( + "MATCH (c:Class) WHERE c.name IN ['Animal', 'Dog'] RETURN c.name, c.kind ORDER BY c.name", + ).run(graph).rows + + key = rows.first.keys.first + assert_predicate(key, :frozen?) + assert_equal(Encoding::UTF_8, key.encoding) + # One key object per column serves every row, so a wide result allocates no key per cell. + assert_same(key, rows.last.keys.first) + end + end + def test_render_returns_table_output with_context do |context| context.write!("zoo.rb", <<~RUBY) @@ -1958,4 +2037,13 @@ def assert_diagnostics(expected, actual) def graph_for(context) Rubydex::Graph.configure_for_workspace(context.absolute_path) end + + # Counts the objects that the block allocates. `GC.start` first, so that a pending collection does + # not land inside the measurement. + def count_allocations + GC.start + before = GC.stat(:total_allocated_objects) + yield + GC.stat(:total_allocated_objects) - before + end end From dd9d491abb2c93daaf3c32cfd31af141dcf50158 Mon Sep 17 00:00:00 2001 From: Ufuk Kayserilioglu Date: Fri, 7 Aug 2026 00:11:51 +0300 Subject: [PATCH 2/2] Raise when a query result names a node that is gone Now that rows are built one at a time, the graph can change in the middle of a walk. `build_cell` handled that by falling back to a plain String when it could not resolve a node, which quietly turned a `Declaration` column into a `String` halfway through a result. I hit exactly that while testing the cursor: after a `delete_document` inside an `each` block, the remaining rows came back as `"Cat"` and `"Dog"` instead of handles, with no signal at all. So the fallback is gone. `build_cell` returns the node name as an error, `rdx_rows_iter_next` reports `MissingNode`, `rdx_rows_iter_error` names the node, and the extension raises `Rubydex::StaleQueryResultError`, which is a `Rubydex::QueryError`. The `List` and `Map` arms free the cells they already built before they propagate, so a half-built row leaks nothing. The check is narrow on purpose, and this is not mutation detection: - It only fires when the graph no longer holds a node that the result returned, or when a node id cannot be decoded. - It does not fire for a re-index that keeps the ids. A declaration id comes from the name, so the same names still resolve while the definitions and ancestors behind them may differ. - It says nothing about a handle that a walk already handed out. Those resolve against the graph on each call, exactly like the handles that `Graph#[]` returns. `render`, `columns`, `size`, and `empty?` keep working after the graph changes, since they read the executed result set and never touch the graph. I did not add a graph revision counter for this. It would need a change to the graph pointer in `graph_api.rs`, and it would fire on `load_config`, `set_encoding`, and completion, none of which can drop a node. The missing node is the condition that actually matters here. --- ext/rubydex/query.c | 47 +++++++-- lib/rubydex/errors.rb | 17 +++ rbi/rubydex.rbi | 1 + rust/rubydex-sys/src/cypher_api.rs | 160 ++++++++++++++++++++++------- test/graph_test.rb | 78 ++++++++++++++ 5 files changed, 256 insertions(+), 47 deletions(-) diff --git a/ext/rubydex/query.c b/ext/rubydex/query.c index caa76eba..7cb17e59 100644 --- a/ext/rubydex/query.c +++ b/ext/rubydex/query.c @@ -254,9 +254,24 @@ static VALUE query_row_to_hash(VALUE graph_obj, VALUE keys, const struct CResult return hash; } +// Raises when the graph no longer holds a node that the query returned. Building a string in place +// of the missing handle would silently change the column's type, so the walk stops instead. +NORETURN(static void raise_stale_result(struct CRowsIter *iter)); + +static void raise_stale_result(struct CRowsIter *iter) { + VALUE error_class = rb_const_get(mRubydex, rb_intern("StaleQueryResultError")); + const char *node = rdx_rows_iter_error(iter); + + if (node == NULL) { + rb_raise(error_class, "the graph no longer holds a node that this query returned"); + } + + rb_raise(error_class, "the graph no longer holds `%s`, a node that this query returned", node); +} + // Body function for rb_ensure in Rubydex::Query::Result#rows — walks the cursor and collects every -// row. May raise if cell conversion (e.g. handle construction) fails; the ensure function frees the -// cursor regardless. +// row. May raise if a node is gone, or if cell conversion (e.g. handle construction) fails; the +// ensure function frees the cursor regardless. static VALUE query_rows_collect(VALUE args) { VALUE graph_obj = rb_ary_entry(args, 0); struct CRowsIter *iter = (struct CRowsIter *)(uintptr_t)NUM2ULL(rb_ary_entry(args, 1)); @@ -265,11 +280,17 @@ static VALUE query_rows_collect(VALUE args) { VALUE rows = rb_ary_new_capa((long)rdx_rows_iter_len(iter)); struct CResultRow row; - while (rdx_rows_iter_next(iter, &row)) { - rb_ary_push(rows, query_row_to_hash(graph_obj, keys, &row)); + for (;;) { + switch (rdx_rows_iter_next(iter, &row)) { + case CRowsNextStatus_Row: + rb_ary_push(rows, query_row_to_hash(graph_obj, keys, &row)); + break; + case CRowsNextStatus_MissingNode: + raise_stale_result(iter); + default: + return rows; + } } - - return rows; } // Body function for rb_ensure in Rubydex::Query::Result#each — walks the cursor and yields one row @@ -282,11 +303,17 @@ static VALUE query_rows_stream(VALUE args) { VALUE keys = query_row_keys(iter); struct CResultRow row; - while (rdx_rows_iter_next(iter, &row)) { - rb_yield(query_row_to_hash(graph_obj, keys, &row)); + for (;;) { + switch (rdx_rows_iter_next(iter, &row)) { + case CRowsNextStatus_Row: + rb_yield(query_row_to_hash(graph_obj, keys, &row)); + break; + case CRowsNextStatus_MissingNode: + raise_stale_result(iter); + default: + return Qnil; + } } - - return Qnil; } // Ensure function for rb_ensure to always free the cursor. diff --git a/lib/rubydex/errors.rb b/lib/rubydex/errors.rb index 914557c4..9b9a2255 100644 --- a/lib/rubydex/errors.rb +++ b/lib/rubydex/errors.rb @@ -32,4 +32,21 @@ class QuerySyntaxError < QueryError; end # Raised by `Query#run` when a parsed query fails while it runs against a graph, for example # because it names an unknown property or relationship type. class QueryExecutionError < QueryError; end + + # Raised when a query result names a node that the graph no longer holds, because the graph + # changed after the query ran. Reading the rows would silently turn that column from a + # `Declaration`, `Definition`, or `Document` handle into a plain String, so it raises instead. + # `render`, `columns`, `size`, and `empty?` still work, because they read the executed result set + # and never touch the graph. + # + # The check runs while a row is built, so it covers the rows that a walk has not reached yet. Two + # cases fall outside it: + # + # - A handle that a walk already handed out. Such a handle resolves against the graph on each + # call, so a later change to the graph can make it stale. Handles from `Graph#[]` share that + # property. This error says nothing about them. + # - A re-index that keeps the ids. A declaration id comes from the name, so a file indexed again + # under the same names still resolves, and this error does not fire, even though the + # definitions and ancestors behind that name may differ. + class StaleQueryResultError < QueryError; end end diff --git a/rbi/rubydex.rbi b/rbi/rubydex.rbi index fb826d19..7a044b73 100644 --- a/rbi/rubydex.rbi +++ b/rbi/rubydex.rbi @@ -467,6 +467,7 @@ class Rubydex::ConfigError < Rubydex::Error; end class Rubydex::QueryError < Rubydex::Error; end class Rubydex::QuerySyntaxError < Rubydex::QueryError; end class Rubydex::QueryExecutionError < Rubydex::QueryError; end +class Rubydex::StaleQueryResultError < Rubydex::QueryError; end # The configuration of a workspace, parsed from its `rubydex.toml`. It carries both the settings that are global to # every built-in tool, such as the workspace being analyzed, and the typed settings of each tool's own section (e.g. diff --git a/rust/rubydex-sys/src/cypher_api.rs b/rust/rubydex-sys/src/cypher_api.rs index 2dee0ca0..3b61cf69 100644 --- a/rust/rubydex-sys/src/cypher_api.rs +++ b/rust/rubydex-sys/src/cypher_api.rs @@ -246,6 +246,19 @@ pub struct CResultRow { pub len: usize, } +/// The outcome of one `rdx_rows_iter_next` call. +#[repr(C)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CRowsNextStatus { + /// `out` holds the next row. + Row, + /// The cursor reached the end of the result set. + Done, + /// The graph no longer holds a node that the row returned, so the row cannot be built. + /// `rdx_rows_iter_error` names the node. + MissingNode, +} + /// A cursor over an executed result set's rows. It converts one row per `rdx_rows_iter_next` call, /// so a caller can walk a large result set without a copy of every cell in memory at once. Opaque /// from the C side — use the `rdx_rows_iter_*` methods to work with it. @@ -256,6 +269,8 @@ pub struct CRowsIter { columns: Box<[*const c_char]>, /// Cells of the row that the last `rdx_rows_iter_next` call produced. current: Vec, + /// Name of the node that the last `rdx_rows_iter_next` call could not resolve. + error: Option, index: usize, } @@ -275,41 +290,74 @@ pub struct CExecuteResult { } /// Converts a `CypherValue` into a `CCell`, resolving node identity to a handle-buildable category + -/// kind + id. A node whose id cannot be decoded or found falls back to its display name as a string. -fn build_cell(graph: &Graph, value: &CypherValue) -> CCell { +/// kind + id. +/// +/// # Errors +/// +/// Returns the node's display name when the graph no longer holds a node that the result set +/// returned, or when its id cannot be decoded. The caller must treat the whole row as stale, +/// because a fallback would silently change the column's type from a handle to a string. Cells that +/// this function already built are freed before it returns. +fn build_cell(graph: &Graph, value: &CypherValue) -> Result { match value { - CypherValue::Null => CCell::null(), - CypherValue::Bool(b) => CCell::new(CCellTag::Bool, CCellPayload { bool_val: *b }), - CypherValue::Int(i) => CCell::new(CCellTag::Int, CCellPayload { int_val: *i }), - CypherValue::Str(s) => CCell::new( + CypherValue::Null => Ok(CCell::null()), + CypherValue::Bool(b) => Ok(CCell::new(CCellTag::Bool, CCellPayload { bool_val: *b })), + CypherValue::Int(i) => Ok(CCell::new(CCellTag::Int, CCellPayload { int_val: *i })), + CypherValue::Str(s) => Ok(CCell::new( CCellTag::Str, CCellPayload { str_val: utils::cstring_raw(s), }, - ), + )), CypherValue::List(items) => { - let cells: Vec = items.iter().map(|item| build_cell(graph, item)).collect(); + let mut cells: Vec = Vec::with_capacity(items.len()); + + for item in items { + match build_cell(graph, item) { + Ok(cell) => cells.push(cell), + Err(node) => { + // SAFETY: `cells` holds only what this loop built, and nothing else owns it. + unsafe { free_cells(&cells) }; + return Err(node); + } + } + } + let len = cells.len(); let items = if cells.is_empty() { ptr::null_mut() } else { Box::into_raw(cells.into_boxed_slice()).cast::() }; - CCell::new( + Ok(CCell::new( CCellTag::List, CCellPayload { list: CList { items, len }, }, - ) + )) } CypherValue::Map(pairs) => { let len = pairs.len(); let mut keys: Vec<*const c_char> = Vec::with_capacity(len); let mut values: Vec = Vec::with_capacity(len); + for (key, val) in pairs { - keys.push(utils::cstring_raw(key)); - values.push(build_cell(graph, val)); + match build_cell(graph, val) { + Ok(cell) => { + keys.push(utils::cstring_raw(key)); + values.push(cell); + } + Err(node) => { + // SAFETY: both vectors hold only what this loop built. + unsafe { free_cells(&values) }; + for key in keys { + let _ = unsafe { CString::from_raw(key.cast_mut()) }; + } + return Err(node); + } + } } + let (keys, values) = if len == 0 { (ptr::null_mut(), ptr::null_mut()) } else { @@ -318,21 +366,14 @@ fn build_cell(graph: &Graph, value: &CypherValue) -> CCell { Box::into_raw(values.into_boxed_slice()).cast::(), ) }; - CCell::new( + Ok(CCell::new( CCellTag::Map, CCellPayload { map: CMap { keys, values, len }, }, - ) + )) } - CypherValue::Node { id, name, .. } => build_node_cell(graph, id).unwrap_or_else(|| { - CCell::new( - CCellTag::Str, - CCellPayload { - str_val: utils::cstring_raw(name), - }, - ) - }), + CypherValue::Node { id, name, .. } => build_node_cell(graph, id).ok_or_else(|| name.clone()), } } @@ -517,6 +558,7 @@ pub unsafe extern "C" fn rdx_result_set_rows(result_set: *const CResultSet, poin graph: pointer, columns, current: Vec::new(), + error: None, index: 0, })) } @@ -579,10 +621,13 @@ pub unsafe extern "C" fn rdx_rows_iter_len(iter: *const CRowsIter) -> usize { unsafe { &*iter.result_set }.0.rows.len() } -/// Converts the next row and copies a view of it into `out`. Returns `true` if a row was read, -/// `false` if the cursor is exhausted. The cells belong to the cursor, so the copied `CResultRow` -/// stays valid only until the next `rdx_rows_iter_next` call or `rdx_rows_iter_free`, whichever -/// comes first. Read the row's values before calling either. +/// Converts the next row and copies a view of it into `out`. The cells belong to the cursor, so the +/// copied `CResultRow` stays valid only until the next `rdx_rows_iter_next` call or +/// `rdx_rows_iter_free`, whichever comes first. Read the row's values before calling either. +/// +/// Returns `MissingNode` when the graph no longer holds a node that the row returned. That happens +/// when the graph changed after the query ran. The cursor keeps the node's name for +/// `rdx_rows_iter_error`, and the caller should stop the walk. /// /// The graph read lock is taken for the conversion of one row and released before this function /// returns, so a caller may run arbitrary code, including code that writes to the graph, between @@ -593,9 +638,9 @@ pub unsafe extern "C" fn rdx_rows_iter_len(iter: *const CRowsIter) -> usize { /// - `iter` must be a valid pointer returned by `rdx_result_set_rows`, or null. /// - `out` must be a valid, writable pointer, or null. #[unsafe(no_mangle)] -pub unsafe extern "C" fn rdx_rows_iter_next(iter: *mut CRowsIter, out: *mut CResultRow) -> bool { +pub unsafe extern "C" fn rdx_rows_iter_next(iter: *mut CRowsIter, out: *mut CResultRow) -> CRowsNextStatus { if iter.is_null() || out.is_null() { - return false; + return CRowsNextStatus::Done; } let it = unsafe { &mut *iter }; @@ -603,25 +648,66 @@ pub unsafe extern "C" fn rdx_rows_iter_next(iter: *mut CRowsIter, out: *mut CRes // The previous row is out of scope for the caller now, so release its cells before the next one. unsafe { free_cells(&it.current) }; it.current.clear(); + it.error = None; let result_set = unsafe { &*it.result_set }; let Some(row) = result_set.0.rows.get(it.index) else { - return false; + return CRowsNextStatus::Done; }; it.index += 1; - it.current = with_graph(it.graph, |graph| { - row.iter().map(|cell| build_cell(graph, cell)).collect() + let built = with_graph(it.graph, |graph| { + let mut cells: Vec = Vec::with_capacity(row.len()); + + for value in row { + match build_cell(graph, value) { + Ok(cell) => cells.push(cell), + Err(node) => { + // SAFETY: `cells` holds only what this loop built, and nothing else owns it. + unsafe { free_cells(&cells) }; + return Err(node); + } + } + } + + Ok(cells) }); - unsafe { - *out = CResultRow { - cells: it.current.as_mut_ptr(), - len: it.current.len(), - }; + match built { + Ok(cells) => { + it.current = cells; + unsafe { + *out = CResultRow { + cells: it.current.as_mut_ptr(), + len: it.current.len(), + }; + } + CRowsNextStatus::Row + } + Err(node) => { + it.error = CString::new(node).ok(); + CRowsNextStatus::MissingNode + } + } +} + +/// Returns the name of the node that the last `rdx_rows_iter_next` call could not resolve, or null +/// when it resolved every node. The string belongs to the cursor, so it stays valid only until the +/// next `rdx_rows_iter_next` call or `rdx_rows_iter_free`. +/// +/// # Safety +/// +/// - `iter` must be a valid pointer returned by `rdx_result_set_rows`, or null. +#[unsafe(no_mangle)] +pub unsafe extern "C" fn rdx_rows_iter_error(iter: *const CRowsIter) -> *const c_char { + if iter.is_null() { + return ptr::null(); } - true + match unsafe { &*iter }.error.as_ref() { + Some(name) => name.as_ptr(), + None => ptr::null(), + } } /// Recursively frees a `CCell`'s owned allocations (its string, or its nested list cells). diff --git a/test/graph_test.rb b/test/graph_test.rb index 217ae68a..99529fab 100644 --- a/test/graph_test.rb +++ b/test/graph_test.rb @@ -1832,6 +1832,84 @@ def test_rows_share_one_frozen_key_per_column end end + def test_rows_raise_when_the_graph_no_longer_holds_a_returned_node + graph = Rubydex::Graph.new + graph.index_source("file:///zoo.rb", "class Animal; end\nclass Dog < Animal; end\n", "ruby") + graph.resolve + + result = Rubydex::Query.parse( + "MATCH (c:Class) WHERE c.name IN ['Animal', 'Dog'] RETURN c, c.name ORDER BY c.name", + ).run(graph) + + graph.delete_document("file:///zoo.rb") + graph.resolve + + # A String in place of the missing handle would change the column's type without a word. + error = assert_raises(Rubydex::StaleQueryResultError) { result.rows } + assert_match(/the graph no longer holds `Animal`/, error.message) + assert_kind_of(Rubydex::QueryError, error) + end + + def test_each_raises_when_a_returned_node_disappears_during_the_walk + graph = Rubydex::Graph.new + graph.index_source("file:///zoo.rb", "class Animal; end\nclass Dog < Animal; end\n", "ruby") + graph.resolve + + result = Rubydex::Query.parse( + "MATCH (c:Class) WHERE c.name IN ['Animal', 'Dog'] RETURN c, c.name ORDER BY c.name", + ).run(graph) + + seen = [] + + # The cursor holds no lock while the block runs, so the block may write to the graph. The next + # row then cannot build its handle, and the walk stops. + error = assert_raises(Rubydex::StaleQueryResultError) do + result.each do |row| + seen << row["c.name"] + graph.delete_document("file:///zoo.rb") + graph.resolve + end + end + + assert_equal(["Animal"], seen) + assert_match(/the graph no longer holds `Dog`/, error.message) + end + + def test_a_stale_result_still_renders_and_reports_its_shape + graph = Rubydex::Graph.new + graph.index_source("file:///zoo.rb", "class Animal; end\nclass Dog < Animal; end\n", "ruby") + graph.resolve + + result = Rubydex::Query.parse( + "MATCH (c:Class) WHERE c.name IN ['Animal', 'Dog'] RETURN c, c.name ORDER BY c.name", + ).run(graph) + + graph.delete_document("file:///zoo.rb") + graph.resolve + + # These read the executed result set and never touch the graph, so they keep working. + assert_equal(["c", "c.name"], result.columns) + assert_equal(2, result.size) + refute_empty(result) + assert_match(/Animal/, result.render(:json)) + end + + def test_rows_without_a_node_column_survive_a_graph_change + graph = Rubydex::Graph.new + graph.index_source("file:///zoo.rb", "class Animal; end\nclass Dog < Animal; end\n", "ruby") + graph.resolve + + result = Rubydex::Query.parse( + "MATCH (c:Class) WHERE c.name IN ['Animal', 'Dog'] RETURN c.name ORDER BY c.name", + ).run(graph) + + graph.delete_document("file:///zoo.rb") + graph.resolve + + # Scalar cells are pure snapshot data. They cannot go stale, so nothing raises. + assert_equal([{ "c.name" => "Animal" }, { "c.name" => "Dog" }], result.rows) + end + def test_render_returns_table_output with_context do |context| context.write!("zoo.rb", <<~RUBY)