refactor(incremental): batch + single-stat persist_hashes (2/3)#869
refactor(incremental): batch + single-stat persist_hashes (2/3)#869moofone wants to merge 9 commits into
Conversation
Artifact bootstrap byte-copies a teammate's DB into the local cache, but the imported file_hashes rows carry the exporter's mtime_ns. A fresh clone stamps every file with checkout time, so classify_files (mtime_ns + size only) marks ~every file changed and the first incremental run re-parses the whole repo -- the exact run artifact bootstrap exists to make cheap. Add cbm_artifact_reconcile_hashes(), called from try_artifact_bootstrap right after a successful cbm_artifact_import. It re-stamps the hash rows of files git reports unchanged between the artifact's commit and the local working tree with local stat() values, so the existing, untouched classify_files logic then classifies correctly. Zero changes to the incremental pipeline itself. Trust gate (so a stale/corrupt artifact can never mark a genuinely changed file unchanged -> graph corruption): cbm_artifact_export now writes an optional "reconcile_basis":"git-clean-head" marker into the existing artifact.json ONLY when it can prove the DB matches a clean checked-out tree at `commit` -- commit is a validated hex OID, the working tree has no changes outside .codebase-memory, and every file_hashes row's on-disk mtime+size matches. Reconciliation requires the marker and skips (returns -1) on any doubt: no git, untrusted/missing marker, non-hex/unknown commit, shallow clone, or any popen or parse uncertainty. A skip leaves rows foreign and falls back to today's slow-safe full incremental. No schema_version bump (older binaries ignore the new optional field). Security: commit is hex-validated before command construction and repo_path is shell-validated via cbm_validate_shell_arg (same pattern as git_context.c / watcher.c); git output is parsed as NUL-delimited (-z) directly, never via line-oriented parsing, so paths with newlines/quotes are handled. New popen call site added to scripts/security-allowlist.txt. Reuses two pieces of existing dead/unused code: cbm_artifact_commit() (was never called in production) and cbm_store_upsert_file_hash_batch() (existed but was never called in production -- reconciliation persists all restamped rows in one transaction). Tests (tests/test_artifact.c, following the existing suite's structure): export sets the marker on a clean tree and drops it when dirty; reconcile restamps unchanged rows and leaves changed rows foreign; reconcile skips (-1, rows untouched) on untrusted metadata, unknown commit, and no-git, plus null-arg safety. Signed-off-by: Greg Tiller <tiller@dal.ca>
… stamps persist_hashes re-stat()ed every discovered file AFTER the run and upserted rows one at a time (one implicit SQLite write transaction per file). Two problems: 1. Wasted work: classify_files had already stat()ed every file with a stored hash. persist_hashes stat()ed them all again. 2. Correctness: re-stat'ing AFTER the run records the post-run mtime, so a file edited mid-index is stamped "current" and the NEXT run misses the edit (classifies it unchanged). Using the classify-time stat is both faster and more correct. classify_files now outputs a cbm_file_stamp_t array (one stat per file at classify time, including for new files). persist_hashes builds one cbm_file_hash_t array from files[]+stamps[] plus mode_skipped[] and upserts it via the existing-but-unused cbm_store_upsert_file_hash_batch (single BEGIN...COMMIT). On batch failure (it rolls back on first row error) it falls back to a row-at-a-time loop so one bad row still can't nuke all persistence -- preserving the documented "partial preservation beats total loss" contract. Threading: stamps flows classify_files -> cbm_pipeline_run_incremental -> dump_and_persist -> persist_hashes (all static, file-local signature changes), freed at every return path (noop, load_db_failed, normal). Test: incremental_persist_batch_roundtrip exercises the batch path via the real pipeline route (full index -> modify -> incremental reindex -> noop) and asserts the persisted mtime equals the classify-time mtime. The existing incremental suite (incr_modify_file / incr_add_file / incr_delete_file / incr_noop_reindex, ~5900 cases) is the regression guard. Signed-off-by: Greg Tiller <tiller@dal.ca>
DeusData
added
bug
|
Thanks for keeping part 2 scoped. Triage: performance/correctness bug in the incremental path, stacked after #868. Review should verify that classify-time stamps really close the mid-index edit miss without widening the race window, and that the batch write path keeps failure behavior conservative. Since this also carries the #868 base changes, we should review/merge in stack order. |
git diff/ls-files are blind to gitignored files, but a gitignored file can still be indexed (.cbmignore negation un-skipping a generated dir, DeusData#500) and thus carry a file_hashes row. Without a tracked-set gate such a row would be restamped as "unchanged" even though git cannot vouch for its content, leaving stale graph data after bootstrap. Reconciliation now also captures git ls-tree -r -z --name-only <commit> and restamps only rows tracked at the artifact commit AND absent from the changed set; everything git cannot vouch for stays foreign and is re-parsed. Same validation funnel (hex-validated commit, shell-validated repo path, NUL- delimited parse with truncation guard); allowlist entry text extended to mention ls-tree. Adds regression test artifact_reconcile_skips_untracked_rows. Refs DeusData#867 Signed-off-by: Greg Tiller <tiller@dal.ca>
|
On the two review questions from triage: Does the classify-time stamp close the mid-index miss without widening the race window? Yes — the race can only narrow, by construction. The stamp for each file is taken before its content is parsed. So for any edit relative to the stamp time:
The old post-run re-stat had the opposite property: it stamped the post-edit mtime against pre-edit parsed content, so the next run compared equal and the edit was silently missed. There is no input ordering under which the new scheme misses an edit that the old one caught; the possible outcomes are "consistent" or "conservative re-parse". Does the batch path keep failure behavior conservative? The batch upsert is a single Test Branch has been updated with the #868 base changes merged in (this PR's reviewable commit is unchanged: |
CI lint (clang-format-20 --dry-run --Werror) flagged comment alignment and line-break style in the P1 additions. Formatting only; no code changes. Refs DeusData#867 Signed-off-by: Greg Tiller <tiller@dal.ca>
CI lint (clang-format-20 --dry-run --Werror) flagged wrap style in the P2 signature changes. Formatting only; no code changes. Refs DeusData#867 Signed-off-by: Greg Tiller <tiller@dal.ca>
2 participants
What & why
persist_hashes(src/pipeline/pipeline_incremental.c) re-stat()ed every discovered file after the run and upserted hash rows one at a time (one implicit SQLite write transaction per file). Two problems:classify_fileshad alreadystat()ed every file with a stored hash.Change
classify_filesnow outputs acbm_file_stamp_tarray (onestat()per file at classify time, including for new files).persist_hashesbuilds onecbm_file_hash_tarray fromfiles[]+stamps[]plusmode_skipped[]and upserts it via the existing-but-unusedcbm_store_upsert_file_hash_batch(singleBEGIN…COMMIT).stampsflowsclassify_files → cbm_pipeline_run_incremental → dump_and_persist → persist_hashes(all static, file-local signature changes), freed at every return path (noop / load_db_failed / normal).Tests (
tests/test_artifact.c)incremental_persist_batch_roundtrip— exercises the batch path via the real pipeline route (full index → modify → incremental reindex → noop), asserts the persisted mtime equals the classify-time mtime and the subsequent run classifies no-change.incr_modify_file/incr_add_file/incr_delete_file/incr_noop_reindex, ~5900 cases) is the regression guard.scripts/test.sh(ASan/UBSan) green locally.Benchmark (S3 = steady-state incremental, K = 5 modified)
Candidate ≤ baseline at every size. (*the export-skip is P3, tracked by part 3; P2's contribution is the batched, single-stat persist.) S4 (noop) and S1 (full) unchanged within noise.
Checklist
scripts/test.sh(ASan/UBSan) green; no regressions in the incremental suite.scripts/lint.sh— not installed locally; verified by hand, CI is the gate.Commit message notes the mid-run-edit correctness fix.