Skip to content

fix(store): report a failed COUNT read instead of returning zero - #2065

Open
kavish-19 wants to merge 1 commit into
DeusData:mainfrom
kavish-19:fix-count-failed-read
Open

fix(store): report a failed COUNT read instead of returning zero#2065
kavish-19 wants to merge 1 commit into
DeusData:mainfrom
kavish-19:fix-count-failed-read

Conversation

@kavish-19

Copy link
Copy Markdown

Fixes #2012.

Symptom

cbm_store_count_nodes() treats every sqlite3_step() result other than SQLITE_ROW as a count of zero. A read that failed — SQLITE_CORRUPT, SQLITE_BUSY, SQLITE_IOERR — is indistinguishable from a project that genuinely holds no nodes, and index_status renders it as the positive assertion status: "empty". A user or agent reading that concludes the repository was never indexed and kicks off a multi-minute re-index, while the corruption is never surfaced.

Root cause

    bind_text(stmt, SKIP_ONE, project);
    int count = 0;
    if (sqlite3_step(stmt) == SQLITE_ROW) {
        count = sqlite3_column_int(stmt, 0);
    }
    sqlite3_reset(stmt);
    return count;          /* a failed step returns 0, same as an empty table */

There is no else. Both functions already carry an error channel — each returns CBM_STORE_ERR when prepare_cached fails — so callers already receive a negative value from this API today. Only the step result was never reported through it.

The consumer is already written for it. mcp.c (index_status) does:

    nodes = cbm_store_count_nodes(store, project_name);
    edges = cbm_store_count_edges(store, project_name);
    if (nodes < 0) {
        degraded = true;
        nodes = 0;
        edges = edges >= 0 ? edges : 0;
    }

So the guard for exactly this case exists and simply never fires. This PR makes the producer signal what the consumer is already prepared to handle.

The fix

Initialise count to CBM_STORE_ERR in cbm_store_count_nodes and cbm_store_count_edges. A successful step still overwrites it with the real count, so the healthy path is untouched.

I included count_edges because index_status reads the two as a pair and clamps a negative edge count in the same block — fixing only the node side would leave a corrupt edges table still reporting edges: 0 with status: "ready", which is the same false report the issue describes. The issue's own reproduction shows the pair contradicting itself (nodes: 0 beside edges: 8).

Not changed: cbm_store_count_edges_by_type has the same shape but is not read by index_status, and cbm_store_count_vectors has the shape without an existing error channel — giving it one would be a new contract. Both felt like separate changes rather than part of this one; happy to follow up if you want them.

Verification

I confirmed the behaviour change directly against main and against this branch, using a small harness linked against src/store/store.c. It inserts a node, then drops the tables after the statements are cached, so the step (not the prepare) is what fails:

BEFORE (main)   healthy  nodes=1 edges=0
                dropped  nodes=0  edges=0     <- indistinguishable from "empty"

AFTER  (branch) healthy  nodes=1 edges=0      <- healthy path unchanged
                dropped  nodes=-1 edges=-1    <- index_status marks it degraded

A regression test in the same shape is added to tests/test_store_nodes.c as store_count_failed_read_is_not_zero, next to store_count_nodes_unknown_project, and registered in the suite.

What I could not run locally, and why

I could not run scripts/test.sh or scripts/lint.sh on this machine, and I would rather say so than imply a green run:

  • scripts/build.sh fails at internal/cbm/preprocessor.cpp with fatal error: 'cctype' file not found. The C++ standard-library headers are missing from this machine's Command Line Tools (the SDK contains .../MacOSX.sdk/usr/include/c++/v1/cctype, but clang does not resolve it even with -isysroot). It is a local toolchain fault, unrelated to this change, and it blocks the test runner because that also links preprocessor.o.
  • clang-format, clang-tidy and cppcheck are not installed here.

What I did instead: both changed files compile clean under the project's own warning set (-std=c11 -Wall -Wextra -Werror -Wno-unused-parameter -Wno-sign-compare), and I followed the surrounding formatting by hand. CI is the authority on the full suite and the linters. If it flags anything, I will fix it promptly.

cbm_store_count_nodes and cbm_store_count_edges treat every sqlite3_step
result other than SQLITE_ROW as a count of zero. A read that failed —
SQLITE_CORRUPT, SQLITE_BUSY, SQLITE_IOERR — is therefore indistinguishable
from a project that genuinely holds no rows, and index_status renders it
as the positive assertion status "empty". A user or agent reading that
concludes the repository was never indexed and starts a multi-minute
re-index, while the corruption itself is never surfaced.

Both functions already have an error channel: each returns CBM_STORE_ERR
when prepare_cached fails. Only the step result was not reported through
it. index_status is already written for that value — it sets degraded on
a negative node count and clamps a negative edge count — so the guard
existed and simply never fired.

Initialise count to CBM_STORE_ERR so a non-row step is reported as a
failed read. A successful step still overwrites it with the real count,
so the healthy path is unchanged.

Closes DeusData#2012

Signed-off-by: kavish-19 <sworks.dev@gmail.com>
@kavish-19
kavish-19 requested a review from DeusData as a code owner September 5, 2026 06:55
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData DeusData added bug Something isn't working ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cbm_store_count_nodes returns 0 for a failed COUNT(*), so index_status reports a corrupt project as nodes: 0, status: "empty"

3 participants