Skip to content

fix: Surface a real error when persistence artifact export silently fails on Windows#486

Open
mvanhorn wants to merge 2 commits into
DeusData:mainfrom
mvanhorn:fix/400-windows-persistence-artifact-silent-failure
Open

fix: Surface a real error when persistence artifact export silently fails on Windows#486
mvanhorn wants to merge 2 commits into
DeusData:mainfrom
mvanhorn:fix/400-windows-persistence-artifact-silent-failure

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Summary

On Windows, index_repository with persistence:true reports status:indexed but artifact_present:false, and the /.codebase-memory directory is created with only ADR.md and no graph.db.zst, with no error surfaced. A second user (armandn) reproduced this on 2026-06-17 with a concrete index_repository response (nodes:1866, edges:5340, adr_present:true, artifact_present:false).

Changes

The artifact export lives in src/pipeline/artifact.c (referenced by pipeline.c, which sets artifact_present in the index_repository response). Audit the Windows code path for the zstd-compressed graph.db.zst write: a failed temp-file create/rename or path-separator/long-path handling likely returns early without propagating an error, leaving artifact_present:false and no log line.

Fixes #400

…ails on Windows

Signed-off-by: mvanhorn <mvanhorn@gmail.com>
@mvanhorn mvanhorn force-pushed the fix/400-windows-persistence-artifact-silent-failure branch from fabaa32 to d4aae7e Compare June 17, 2026 15:58
@DeusData

Copy link
Copy Markdown
Owner

Thanks @mvanhorn — the error-surfacing work here is solid, and the directory-as-blocker trick to force a rename failure is a clean reproduce-first test.

Two things before merge:

  1. cbm_artifact_export_last_error() is declared via ad-hoc forward declarations in pipeline.c (and the test) rather than a shared header — please expose it in artifact.h so there's a single declaration.
  2. This changes dump_and_persist_hashes to propagate a failed export as a pipeline error (previously it was silently ignored). That's the right call, but please confirm it's intentional and add a short comment — it can now fail a run that used to "succeed" silently.

Also FYI: #492 (the other #400 PR, now closed in favour of this one) used MoveFileExA(..., MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) for the Windows replace, which is more atomic than unlink+rename — worth folding that one line into your write_file_atomic (credit to @Ayush7Ranjan). 🙏

@DeusData

Copy link
Copy Markdown
Owner

One more item from a closer pass, in addition to the header/error-propagation notes above: in read_stripped_db, the empty/unreadable branch calls artifact_export_fail(...) but then continues, so the caller hits a second artifact_export_fail for the same condition (double-report / continue-on-error). Worth returning on the first failure so the error is reported once and the path stops cleanly. 🙏

- Declare cbm_artifact_export_last_error() in artifact.h and drop the ad-hoc
  forward declarations in pipeline.c and test_artifact.c so there is a single
  shared declaration.
- Document that dump_and_persist_hashes now intentionally propagates a failed
  artifact export as a pipeline error (previously silently ignored), so the
  behavior change is recorded in-tree.
- Use MoveFileExA(MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) for the
  Windows replace in write_file_atomic instead of unlink+rename; it replaces
  the destination atomically (credit @Ayush7Ranjan, from DeusData#492).

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Done in 44d5174.

  1. Moved the cbm_artifact_export_last_error() declaration into artifact.h and removed the ad-hoc forward declarations from pipeline.c and the test, so there's a single declaration.
  2. Confirmed, it's intentional: dump_and_persist_hashes now propagates a failed export as a pipeline error instead of silently ignoring it. Added a short comment at the propagation point flagging the behavior change.
  3. Folded in @Ayush7Ranjan's MoveFileExA(..., MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) for the Windows replace in write_file_atomic (the POSIX path keeps plain rename).

The changed files compile clean under the project's -Werror flags and the artifact suite passes locally; the MoveFileExA branch is Windows-only so I'm leaning on CI for that path. Thanks for the thorough review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Persistence feature (persistence: true) not working on Windows

2 participants