feat(incremental): debounce artifact auto re-export (3/3)#870
Open
moofone wants to merge 14 commits into
Open
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>
dump_and_persist re-exported the artifact on EVERY incremental run -- even a
1-file edit read the whole DB, zstd-compressed it, and rewrote
.codebase-memory/graph.db.zst (+ metadata), churning the user's git working
tree and burning O(graph) I/O/CPU proportional to the whole graph.
Debounce: export only when changed_total (changed + deleted) >=
CBM_ARTIFACT_EXPORT_MIN_CHANGES (default 10) OR the artifact's indexed_at is
older than CBM_ARTIFACT_EXPORT_MAX_AGE_S (default 24h). Age comes from
artifact.json's indexed_at via a new cbm_artifact_age_seconds() helper (parsed
with a portable UTC epoch computation -- no libc timegm/timezone dependency),
NOT the .zst filesystem mtime, because checkout/touch can make an old artifact
look fresh. CBM_ARTIFACT_EXPORT_MIN_CHANGES=0 restores per-run export (today's
behavior). The skip is logged (artifact.export_skipped changed=<n> age_s=<a>)
so staleness stays observable ("no silent caps").
Safety argument for staleness: with P1's git reconcile in place, a stale
artifact merely means bootstrap reconciles a slightly larger git diff -- cost
degrades gracefully, correctness is unaffected.
Tests: artifact_export_debounced (1 change → .zst inode mtime bit-identical +
export_skipped logged) and artifact_export_forced_by_env (MIN_CHANGES=0 →
.zst re-written). Both drive a real incremental run via the pipeline route.
Signed-off-by: Greg Tiller <tiller@dal.ca>
This was referenced Jul 5, 2026
DeusData
added
bug
Owner
|
Thanks for the final incremental-cost piece. Triage: performance/correctness bug under #867/#593, stacked after #868 and #869. Review focus: debounce thresholds need predictable defaults and override behavior, artifact age parsing needs portable time handling, and re-export suppression must not leave stale artifacts after materially changed indexes. We will review this after the earlier stack layers. |
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>
A per-run change threshold alone has a staleness hole: many consecutive below-threshold runs (each < 10 changes) never re-export, leaving the artifact stale for up to the age cap despite a materially changed index. The debounce now also computes cumulative drift — discovered files whose classify-time mtime lands in a later second than the artifact's indexed_at — from the stamps already produced by classify_files (zero extra I/O, no persisted counter). Files touched since the last export keep local mtimes newer than indexed_at, so drift accumulates across runs; export fires when this run's changes >= threshold OR drift >= threshold OR age >= max. Over-counting (touched-but-unchanged files) only exports more often — the safe direction. Drift is included in the artifact.export_skipped log line. Also adds CBM_ARTIFACT_EXPORT_MAX_AGE_S (same strict parse as CBM_ARTIFACT_EXPORT_MIN_CHANGES) so both debounce knobs are overridable, and factors cbm_artifact_indexed_at_epoch out of cbm_artifact_age_seconds. Tests: artifact_export_forced_by_cumulative_drift, artifact_export_forced_by_max_age_env. Refs DeusData#867 Signed-off-by: Greg Tiller <tiller@dal.ca>
Contributor
Author
|
Responding to the three triage focus areas — the staleness one prompted a hardening commit. New commit Predictable defaults and overrides.
Portable time handling. Age comes from Worth noting the safety asymmetry: with #868's reconcile, a stale artifact is a cost issue (slightly larger git diff to reconcile at bootstrap), never a correctness issue — the importing side classifies by actual git state, not by artifact freshness. Full ASan/UBSan suite green with the new commit. |
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>
…ounce-export # Conflicts: # src/pipeline/pipeline_incremental.c
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
2 participants
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.
What & why
dump_and_persist(src/pipeline/pipeline_incremental.c) re-exported the artifact on every incremental run — even a 1-file edit read the whole DB, zstd-compressed it, and rewrote.codebase-memory/graph.db.zst(+ metadata), churning the user's git working tree and burning O(graph) I/O/CPU.Change
Export only when:
changed_total(changed + deleted)>= CBM_ARTIFACT_EXPORT_MIN_CHANGES(default 10), ORthe artifact's
indexed_atis older thanCBM_ARTIFACT_EXPORT_MAX_AGE_S(default 24 h).Age comes from
artifact.json'sindexed_atvia a newcbm_artifact_age_seconds()helper inartifact.c— parsed with a portable UTC epoch computation (days-from-civil; no libctimegm/timezone dependency). Not the.zstfilesystem mtime, because checkout/touch can make an old artifact look fresh.Env override
CBM_ARTIFACT_EXPORT_MIN_CHANGES=0restores per-run export (today's behavior).The skip is logged (
artifact.export_skipped changed=<n> age_s=<a>) so staleness stays observable — "no silent caps".Safety argument for staleness: with P1's git reconcile (#868) in place, a stale artifact merely means bootstrap reconciles a slightly larger git diff — cost degrades gracefully, correctness is unaffected.
Tests (
tests/test_artifact.c)artifact_export_debounced— 1 change (below threshold) + fresh artifact → the.zstinode mtime is bit-identical andartifact.export_skippedis logged.artifact_export_forced_by_env—CBM_ARTIFACT_EXPORT_MIN_CHANGES=0→ the.zstis re-written.Both drive a real incremental run via the pipeline route.
scripts/test.sh(ASan/UBSan) green locally.Benchmark (artifact export flag, K = 5)
artifact_exportartifact_exportCandidate S3 wall drops accordingly (8000: 2290 ms → 1755 ms; the rest is P2's batch persist). S4 (noop) and S1 (full) unaffected.
Checklist
scripts/test.sh(ASan/UBSan) green.scripts/lint.sh— not installed locally; verified by hand, CI is the gate.Commit message notes the staleness-observability line. With all three PRs merged, the full before/after matrix from #867 holds (S2 ~21× faster at N=8000, graph-equivalence exact).