fix: create the first persistent artifact on the incremental path - #1163
fix: create the first persistent artifact on the incremental path#1163mvanhorn wants to merge 1 commit into
Conversation
|
Thanks for covering both changed-file and no-op incremental paths and for propagating export failure rather than reporting a false success. This is classified with #434 as a 0.9.1-rc bug fix. Review will verify explicit persistence versus automatic refresh semantics, first-export quality selection, artifact failure behavior, and compatibility with the existing incremental artifact stack. |
|
Thanks for this, and apologies for the slow acknowledgement. Queued for review. Currently CONFLICTING against |
e8472f9 to
a5e040a
Compare
|
The Windows job failure looks like a timing flake in an unrelated test rather than anything from this PR. The run is 3669 passed / 1 failed, and the one failure is:
This PR touches If it's worth hardening, raising that poll budget or waiting on the lock rather than sampling it would make the test deterministic -- happy to send that separately. |
|
A quick note so this does not sit here looking like your problem: your red CI is not caused by your change. The failure is One thing worth knowing, because it will save you a wasted click: "Re-run failed jobs" will not clear it. A re-run re-tests the same recorded merge commit, so it will fail identically. Only a fresh push refreshes the merge ref — so a rebase on current Apologies for the delay in getting to this. We are working through a large review backlog oldest-first, and the queue is real rather than an indication your PR was skipped. |
cbm_pipeline_refresh_artifact already encodes the policy this PR was written to add: it exports when persistence is on even with no artifact present, picks BEST over FAST for that first export, and propagates an export failure. The full path and the semantic_manifest_equal no-op already call it; the other two incremental exits did not. The no_changes no-op returned 0 without refreshing, so a run that changed nothing never created the first artifact. The dump path hand-rolled an exists-only FAST export and discarded its result. Both now route through the helper, which is also why this diff is much smaller than the original branch. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
8e972fc to
5ed19a2
Compare
|
Rebased onto current main and the diff got much smaller, so here is why rather than leaving it looking like work went missing. Most of what this PR added now exists upstream as cbm_pipeline_refresh_artifact: it exports when persistence is on even with no artifact present, picks BEST over FAST for that first export, and returns the export failure instead of swallowing it. The full path and the semantic_manifest_equal no-op already call it. What was still missing is the other two incremental exits. The no_changes no-op returned 0 without refreshing, so a run that changed nothing never created the first artifact, and the dump path hand-rolled an exists-only FAST export and discarded its result. Both now go through the helper. 5ed19a2, one file, +10/-6. Builds clean under -Werror and the focused artifact and pipeline suites pass, 265 tests, including the existing pipeline_persistence_export_failure_returns_error. |
|
Thank you @mvanhorn for rebasing and explaining exactly what remained after upstream consolidation. The current diff is now one file, +10/-6, routing the two missed incremental exits through the existing refresh helper. The only red is the macOS PR smoke at its repeated- |
|
Closing in favor of #1263 (merged as 415a64a) — with genuine thanks and credit: you identified the #434 incremental-persistence gap first, and your per-route analysis (the mode-skipped noop at :2493 and the real-run tail at :2864, beyond the one site 87a0e3f covered) is what made the architecture comparison possible at all. We ultimately chose the centralized publish-boundary export because its export-exactly-once property is enforced at one place instead of three, but that decision was informed by your mapping of the routes. If you're up for it, the #1263 design could still use eyes on the artifact-refresh edge you knew best. |
What does this PR do?
Fixes #434: enabling
persistence=truedid not create the first persistent artifact on the incremental indexing path.dump_and_persistinpipeline_incremental.conly refreshed an artifact that already existed and never created one on first run, so apersistence=truerequest was silently a no-op until an artifact happened to exist.git ai stats-style consumers saw the index reported as successful with no persistent artifact written.This threads a new
cbm_pipeline_persistence()accessor into the incremental path (mirroring the existingcbm_pipeline_repo_path/cbm_pipeline_project_nameaccessors) so it exports the first artifact withCBM_ARTIFACT_BESTwhen persistence is requested and none exists, matching the full pipeline. Existing artifacts keep refreshing withCBM_ARTIFACT_FAST. It covers changed-file, deleted-file, and no-op incremental runs, and it now propagates a failedcbm_artifact_exportas a run error instead of reporting a successful index when the artifact could not be written.Checklist
git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)