Skip to content

Fix child QueryResult lifetime after parent close#1

Open
magyargergo wants to merge 3 commits intoLadybugDB:mainfrom
magyargergo:fix/node-query-result-child-lifetime
Open

Fix child QueryResult lifetime after parent close#1
magyargergo wants to merge 3 commits intoLadybugDB:mainfrom
magyargergo:fix/node-query-result-child-lifetime

Conversation

@magyargergo
Copy link

@magyargergo magyargergo commented Mar 15, 2026

Description

Fixes LadybugDB/ladybug#290.

NodeQueryResult currently hands child results from a multi-statement chain to new wrappers as borrowed raw pointers. If the parent wrapper owns the chain and is closed first, the child wrapper can still call into freed native state and segfault.

This patch changes the wrapper ownership model so detached child results keep their own native QueryResult ownership instead of borrowing the parent-owned chain node.

It also adds a regression test for the sequence that previously crashed:

const queryResults = conn.querySync("RETURN 1 AS first; RETURN 2 AS second;");
const parentResult = queryResults[0];
const childResult = queryResults[1];
childResult.getNextSync();
parentResult.close();
childResult.resetIterator();
childResult.getNextSync();

Types of changes

  • Bug fix
  • New feature
  • Breaking change
  • Documentation Update

Checklist

  • I have changed storage version if on disk format has changed.
  • I have requested a review from a maintainer.
  • I have updated the documentation (if needed).

Validation

I previously reproduced the crash in the Ladybug superproject at v0.15.1 and confirmed this fix removes the segfault in both:

  • a single-process repro
  • a 5-iteration fork-based stress loop

I did not rerun the full standalone ladybug-nodejs test suite here because the local investigation environment and harness were set up in the Ladybug superproject worktree.

@adsharma
Copy link
Contributor

Thank you! This is along the same lines as the recent fix on go-ladybug:

LadybugDB/go-ladybug#8

While this PR improves things, we need a more comprehensive review of how the QueryResult object is being used in this module.

Do you prefer to get this in first and then discuss the rest of the issues in the comment below?

@adsharma
Copy link
Contributor

Here's a potential ownership model:

  1. Every NodeQueryResult must own its QueryResult.
    NodeQueryResult::queryResult is always backed by ownedQueryResult and never points to non-owned memory. Non-owned pointers are not stored.
  2. Child results are only produced via ownership transfer.
    getNextQueryResult* must only use the ownership-transfer path (moveNextResult()).
    If the current result is not owner-capable (i.e., ownedQueryResult == nullptr), then getNextQueryResult* must fail (return undefined or throw a descriptive error). No raw fallback to getNextQueryResult().
  3. SetQueryResult() only accepts ownership.
    It should either accept a std::unique_ptr or a raw pointer that is immediately adopted as owned. Passing isOwned=false is disallowed.
  4. Parent lifetime does not affect child.
    Once a child NodeQueryResult is created, it owns its own QueryResult and is independent of the parent’s lifetime. Parent close must never invalidate child data.
  5. Async safety rule:
    Close() and SetQueryResult() are only valid when no async operations are in flight on that NodeQueryResult. Violations should be prevented (e.g., a state guard) or documented as a usage error.

Perhaps the right thing to do is to get this PR in and then work on removing non-owned references. Is that something you want to take on?

@magyargergo
Copy link
Author

Thank you for looking into this! I'll tackle with this today and get back to you.

@magyargergo
Copy link
Author

I took the stronger ownership-model direction rather than keeping the earlier "borrow then upgrade" approach:

  • NodeQueryResult no longer stores a borrowed QueryResult*. It now only holds owned std::unique_ptr<QueryResult>
    state.
  • Child results are created only through ownership transfer via moveNextResult().
  • The raw fallback to getNextQueryResult() is removed.
  • Parent and child results are now independent, so parent.close() no longer invalidates a child result.
  • The in-place SetOwnedQueryResult() replacement pattern is gone from the child-wrapper path.
  • I also added a wrapper-local async guard so close()/replacement cannot run while async work is in flight on that
    same NodeQueryResult.

This keeps the original crash fix intact while addressing the review concerns about ambiguous ownership transitions
and borrowed native pointers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node addon segfault when parent QueryResult is closed before child QueryResult

2 participants