feat(incremental): debounce artifact auto re-export (3/3) - #870
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>
DeusData
added
bug
|
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>
|
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
|
Reviewed as part of the stack — the full write-up is on #868, since these three literally stack (this PR contains all of #868's and #869's commits, so merging it would merge the whole stack at once). Please read that one first; this note covers what is specific to #870. Mechanically the code is fine. The debounce logic itself is sound, and the deliberate bounded-staleness framing in your description is honest about what it trades. What holds it is policy, not implementation, and it needs the maintainer rather than me:
I have put both in front of the maintainer along with the trust-model question from #868. Two technical notes for whenever it resumes:
Please do not rebase yet. Three PRs' worth of conflict resolution against a heavily-reworked main is a lot of work to spend before the underlying design questions are answered — and #868's trust marker decides whether any of the stack lands. There is also an offer on #868 to distill the approved parts ourselves with One thing worth saying plainly: the reason this sat is our review latency, not any deficiency in the work. You asked the right question on #867 before building, and we owed you an answer four weeks ago. |
2 participants
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).