fix(store): report a failed COUNT read instead of returning zero - #2065
Open
kavish-19 wants to merge 1 commit into
Open
fix(store): report a failed COUNT read instead of returning zero#2065kavish-19 wants to merge 1 commit into
kavish-19 wants to merge 1 commit into
Conversation
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>
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2012.
Symptom
cbm_store_count_nodes()treats everysqlite3_step()result other thanSQLITE_ROWas a count of zero. A read that failed —SQLITE_CORRUPT,SQLITE_BUSY,SQLITE_IOERR— is indistinguishable from a project that genuinely holds no nodes, andindex_statusrenders it as the positive assertionstatus: "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
There is no
else. Both functions already carry an error channel — each returnsCBM_STORE_ERRwhenprepare_cachedfails — 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: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
counttoCBM_STORE_ERRincbm_store_count_nodesandcbm_store_count_edges. A successful step still overwrites it with the real count, so the healthy path is untouched.I included
count_edgesbecauseindex_statusreads the two as a pair and clamps a negative edge count in the same block — fixing only the node side would leave a corruptedgestable still reportingedges: 0withstatus: "ready", which is the same false report the issue describes. The issue's own reproduction shows the pair contradicting itself (nodes: 0besideedges: 8).Not changed:
cbm_store_count_edges_by_typehas the same shape but is not read byindex_status, andcbm_store_count_vectorshas 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
mainand against this branch, using a small harness linked againstsrc/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:A regression test in the same shape is added to
tests/test_store_nodes.casstore_count_failed_read_is_not_zero, next tostore_count_nodes_unknown_project, and registered in the suite.What I could not run locally, and why
I could not run
scripts/test.shorscripts/lint.shon this machine, and I would rather say so than imply a green run:scripts/build.shfails atinternal/cbm/preprocessor.cppwithfatal 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 linkspreprocessor.o.clang-format,clang-tidyandcppcheckare 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.