From 16633be919e0b395ea31d7e0cbf98a473f4d6539 Mon Sep 17 00:00:00 2001 From: Arun Sharma Date: Mon, 31 Aug 2026 19:13:02 -0700 Subject: [PATCH] fix: PLAN_MAPPER downcast corrupted ArrowResultCollector plans (issue #881) Root cause of the linux minimal-test failures tracked in #881: ArrowResultCollector and ResultCollector both declare PhysicalOperatorType::RESULT_COLLECTOR. In PlanMapper::getPhysicalPlan(), the plan-root marker setResultExposedToClient() was applied via an operator-type check: if (root->getOperatorType() == PhysicalOperatorType::RESULT_COLLECTOR) { root->ptrCast()->setResultExposedToClient(); } For every arrow query (resultType=ARROW), the newly created ArrowResultCollector matched the type check and was then downcast to ResultCollector, which it is not: - With RUNTIME_CHECKS (or any !NDEBUG build) dynamic_cast_checked asserts and throws, so every queryAsArrow execution failed and returned an error MaterializedQueryResult with a null iterator. Tests that call getNextArrowChunk() without checking isSuccess() then SIGSEGV in FactorizedTableIterator::hasNext (the 'Fatal signal 11' in the linux minimal-test job), and the remaining tests fail on dynamic_cast(result.get()) == nullptr and entry.relCsrResults[0] == nullptr (PROJECT_GRAPH materialization also goes through queryAsArrow). - With NDEBUG the checked cast is a reinterpret_cast and setResultExposedToClient() writes one byte at ResultCollector::internalResultTable's offset (192), which in an ArrowResultCollector is localState.batchIndex - undefined behaviour on every arrow query. Fix: mark the client-facing result table on the freshly created regular ResultCollector inside the non-arrow branch of the if/else, instead of matching on the shared operator-type enum afterwards. Also re-enable the ten arrow/CSR tests that were skipped for #881: ArrowTest.queryAsArrow, getArrowResult, queryAsArrowDirectCSRRowIDProjection (+WithFourThreads), queryAsArrowTracksCSRMetadataWithoutRelIDs / WithRelIDsAndExtraColumns / DoesNotTrackCSRMetadataForNonCSRShape, ProjectGraphCsrTest.materializesArrowCsr / materializedCsrSurvivesConsumingQueries, ReadOnlyTest.ProjectGraphOnReadOnlyDatabase. Note on the earlier investigation: the 'corrupted task clone' evidence in the issue (this == sharedState, garbage batch indices, csr metadata present for CSR-free queries) was an artefact of the temporary LBUG_ARROW_DEBUG tracing, whose fprintf macro appended the thread-id string after the format's varargs, shifting every printed value by one slot; the deterministic 'reproduction with instrumentation' was the tracing itself crashing in strlen() on an integer consumed as %s. Validation: full api_test passes with -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_RUNTIME_CHECKS=1 (the minimal-test configuration) as well as plain RelWithDebInfo and Debug; TSAN run of the arrow/CSR suite is clean. --- src/processor/map/plan_mapper.cpp | 13 ++++++++----- test/api/arrow_test.cpp | 15 --------------- test/api/project_graph_csr_test.cpp | 4 ---- test/api/read_only_test.cpp | 2 -- 4 files changed, 8 insertions(+), 26 deletions(-) diff --git a/src/processor/map/plan_mapper.cpp b/src/processor/map/plan_mapper.cpp index 3d2ca2847..fa7997735 100644 --- a/src/processor/map/plan_mapper.cpp +++ b/src/processor/map/plan_mapper.cpp @@ -45,11 +45,14 @@ std::unique_ptr PlanMapper::getPhysicalPlan(const LogicalPlan* log } else { root = createResultCollector(AccumulateType::REGULAR, expressions, logicalPlan->getSchema(), std::move(root)); - } - // The plan root collector is the only one whose table is handed to the client via - // getQueryResult(); every other ResultCollector feeds other operators of the same - // plan and must be cleared in place (not replaced) on reuse. - if (root->getOperatorType() == PhysicalOperatorType::RESULT_COLLECTOR) { + // The plan root collector is the only one whose table is handed to the client via + // getQueryResult(); every other ResultCollector feeds other operators of the same + // plan and must be cleared in place (not replaced) on reuse. + // NOTE: This must only run on the regular (factorized-table) ResultCollector. + // ArrowResultCollector shares PhysicalOperatorType::RESULT_COLLECTOR but is NOT a + // ResultCollector — downcasting it here is undefined behaviour (and trips the + // checked dynamic_cast under RUNTIME_CHECKS), so the flag is set inside the + // non-arrow branch instead of after the if/else via an operator-type check. root->ptrCast()->setResultExposedToClient(); } } diff --git a/test/api/arrow_test.cpp b/test/api/arrow_test.cpp index efd919acf..96853aa56 100644 --- a/test/api/arrow_test.cpp +++ b/test/api/arrow_test.cpp @@ -495,9 +495,6 @@ TEST_F(ArrowTest, resultToArrow) { } TEST_F(ArrowTest, queryAsArrow) { - // TODO(#881): intermittent SIGSEGV race in the arrow collector task path (worker threads - // execute a corrupted task clone). Skip until the race is fixed. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; auto query = "MATCH (a:person) WHERE a.fName = 'Bob' RETURN a.fName"; auto result = conn->queryAsArrow(query, 1); auto arrowArray = result->getNextArrowChunk(1); @@ -511,8 +508,6 @@ TEST_F(ArrowTest, queryAsArrow) { } TEST_F(ArrowTest, getArrowResult) { - // TODO(#881): intermittent SIGSEGV race in the arrow collector task path. Skip until fixed. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; auto query = "MATCH (a:person) WHERE a.fName = 'Bob' RETURN a.fName"; auto result = conn->queryAsArrow(query, 1); try { @@ -599,8 +594,6 @@ TEST_F(ArrowTest, mapColumnArrowSchemaHasNonNullableEntriesAndKey) { } TEST_F(ArrowTest, queryAsArrowDirectCSRRowIDProjection) { - // TODO(#881): intermittent SIGSEGV / CSR-loss race in the arrow collector task path. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; ASSERT_TRUE( conn->query("CREATE NODE TABLE DirectPerson(id INT64, PRIMARY KEY(id));")->isSuccess()); ASSERT_TRUE(conn->query("CREATE REL TABLE DirectKnows(FROM DirectPerson TO DirectPerson);") @@ -653,8 +646,6 @@ TEST_F(ArrowTest, queryAsArrowDirectCSRRowIDProjection) { } TEST_F(ArrowTest, queryAsArrowDirectCSRRowIDProjectionKeepsCSRMetadataWithFourThreads) { - // TODO(#881): intermittent SIGSEGV / CSR-loss race in the arrow collector task path. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; auto query = "MATCH (a:person)-[b:knows]->(c:person) RETURN a.rowid, b.rowid, c.rowid " "ORDER BY a.rowid, b.rowid, c.rowid"; conn->setMaxNumThreadForExec(4); @@ -665,8 +656,6 @@ TEST_F(ArrowTest, queryAsArrowDirectCSRRowIDProjectionKeepsCSRMetadataWithFourTh } TEST_F(ArrowTest, queryAsArrowTracksCSRMetadataWithoutRelIDs) { - // TODO(#881): intermittent SIGSEGV / CSR-loss race in the arrow collector task path. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; auto query = "MATCH (a:person)-[:knows]->(b:person) RETURN a.rowid, b.rowid ORDER BY a.rowid, b.rowid"; auto rowResult = conn->query(query); @@ -712,8 +701,6 @@ TEST_F(ArrowTest, queryAsArrowTracksCSRMetadataWithoutRelIDs) { } TEST_F(ArrowTest, queryAsArrowTracksCSRMetadataWithRelIDsAndExtraColumns) { - // TODO(#881): intermittent SIGSEGV / CSR-loss race in the arrow collector task path. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; auto query = "MATCH (a:person)-[e:knows]->(b:person) " "RETURN a.rowid, e.rowid, b.rowid, e.date, b.fName " "ORDER BY a.rowid, e.rowid, b.rowid"; @@ -756,8 +743,6 @@ TEST_F(ArrowTest, queryAsArrowTracksCSRMetadataWithRelIDsAndExtraColumns) { } TEST_F(ArrowTest, queryAsArrowDoesNotTrackCSRMetadataForNonCSRShape) { - // TODO(#881): intermittent SIGSEGV / CSR-loss race in the arrow collector task path. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; auto query = "MATCH (a:person)-[e:knows]->(b:person) RETURN a.rowid, e.date ORDER BY a.rowid"; auto result = conn->queryAsArrow(query, 8); auto* arrowResult = dynamic_cast(result.get()); diff --git a/test/api/project_graph_csr_test.cpp b/test/api/project_graph_csr_test.cpp index 553f3faa8..9765711fa 100644 --- a/test/api/project_graph_csr_test.cpp +++ b/test/api/project_graph_csr_test.cpp @@ -35,8 +35,6 @@ class ProjectGraphCsrTest : public ApiTest { }; TEST_F(ProjectGraphCsrTest, materializesArrowCsr) { - // TODO(#881): intermittent SIGSEGV / CSR-loss race in the arrow collector task path. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; ASSERT_TRUE(conn->query("CALL PROJECT_GRAPH('CsrG', ['CsrNode'], ['CsrEdge'])")->isSuccess()); const auto& entry = getNativeEntry("CsrG"); ASSERT_EQ(entry.relCsrResults.size(), 1u); @@ -51,8 +49,6 @@ TEST_F(ProjectGraphCsrTest, materializesArrowCsr) { } TEST_F(ProjectGraphCsrTest, materializedCsrSurvivesConsumingQueries) { - // TODO(#881): intermittent SIGSEGV / CSR-loss race in the arrow collector task path. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; ASSERT_TRUE(conn->query("CALL PROJECT_GRAPH('CsrG', ['CsrNode'], ['CsrEdge'])")->isSuccess()); // The pinned result must stay valid across later statements on the same connection. ASSERT_TRUE(conn->query("MATCH (a:CsrNode) RETURN COUNT(*)")->isSuccess()); diff --git a/test/api/read_only_test.cpp b/test/api/read_only_test.cpp index 0b583747c..4b1b531da 100644 --- a/test/api/read_only_test.cpp +++ b/test/api/read_only_test.cpp @@ -23,8 +23,6 @@ TEST_F(ReadOnlyTest, Test) { } TEST_F(ReadOnlyTest, ProjectGraphOnReadOnlyDatabase) { - // TODO(#881): intermittent SIGSEGV / CSR-loss race in the arrow collector task path. - GTEST_SKIP() << "Flaky SIGSEGV in the arrow collector task path; see issue #881."; if (databasePath == "" || databasePath == ":memory:") { return; }