fix(graph-buffer): log why an atomic publish failed instead of a successful dump

[junk151516]

Summary

A failed atomic publish leaves no trace in the log, and the one record it does emit says the dump
succeeded.

cbm_gbuf_dump_to_sqlite() collects the return code of cbm_writer_finalize() and then calls
log_dump_summary() unconditionally, so a run that published nothing still logs
msg=gbuf.dump nodes=… edges=… at INFO. The failure itself carries no reason: the writer's
temp → final rename in publish_writer_output() returns a bare ERR_WRITE_FAILED. What the
user is left with is the generic

Pipeline failed. Check repo_path exists and contains source files.

which sends them to look at their repository when the repository was never the problem.

This is the caller #1628 did not reach. That PR taught cbm_rename_replace() to translate the
platform error into errno precisely so callers could report why — its comment says "callers log
errno after a failed rename" — and wired up the stage → final rename in pipeline.c, which
logs finalize.rename_failed. There are two renames on the publish path. The writer's own is still
silent, and it is the one that runs first.

Refs #1620, #2001.

What this does not do

It does not fix #1620. On that host something below the DACL shape denies the rename, and no
amount of logging changes that. What it does is turn a silent failure into a stated one, which is
the obstacle both #1620 and #2001 describe when they say the log cannot tell them what went wrong.

Changes

internal/cbm/sqlite_writer.c — carry the failure reason across cleanup.

publish_writer_output() calls cbm_unlink() to drop the temp file after a failed rename, and
discard_writer_output() does the same after a failed sync. A library call is free to overwrite
errno even when it succeeds, so the translated value could not be relied on by the time the
caller saw it. Each publish-path error branch now saves errno before cleanup and restores it
after. No logging is added here: nothing in internal/cbm/ includes foundation/log.h, and this
change does not make it the first.

src/graph_buffer/graph_buffer.c — report the failure instead of a summary.

errno is captured immediately after cbm_writer_finalize() returns, before the profiling macro
that follows it can clobber the value. When the writer failed, gbuf.dump_failed is emitted at
ERROR with the return code and that errno; the success record is emitted only on success. The early
return for a writer that never opened logs the same event, so "the dump produced no database" has
exactly one signature in the log.

tests/test_graph_buffer.c — regression test.

gbuf_dump_failure_logs_reason publishes onto an existing directory, which is how
sw_publish_failure_preserves_destination_sidecars already forces this failure, and asserts that
gbuf.dump_failed is logged with a non-zero errno and that the success record is absent.

Verification

Built and run locally on Windows x64 under MSYS2/CLANG64 (clang 22.1.8), ASan+UBSan, via
scripts/test.sh --suites … CC=clang CXX=clang++:

	result
graph_buffer with graph_buffer.c reverted to main	1 failed — gbuf_dump_failure_logs_reason: strstr(logs, "gbuf.dump_failed") is NULL
graph_buffer + sqlite_writer with the change	66 passed, 1 skipped (the skip is sw_publish_preserves_live_reader, SKIP_PLATFORM on Windows)

clang-format --dry-run -Werror is clean on all three files.

Notes for review

• cbm_progress_sink_fn() dispatches on an exact strcmp of the msg field, so gbuf.dump_failed
  does not fall into the gbuf.dump handler. The visible effect on the CLI is that a failed run no
  longer reports node and edge counts for a database it did not publish.
• The new record is ERROR-level, so it survives the default log level; Windows: index always fails with generic 'Pipeline failed' — protected-DACL cache dir breaks MoveFileExW(REPLACE_EXISTING) on this host #1620 reports running at
  CBM_LOG_LEVEL=debug and still seeing nothing, which this addresses at any level.

🤖 Generated with Claude Code

https://claude.ai/code/session_012FxkVF773gDGNmBmsLnzN7

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[DeusData]

Thanks a lot for this one — it is exactly the caller #1628 didn't reach, and the write-up made it easy to check. I built e2195dcb locally (ASan+UBSan lane), ran graph_buffer, sqlite_writer, artifact and cli (all green), and confirmed gbuf_dump_failure_logs_reason goes red when graph_buffer.c is reverted. The layering is right: it mirrors finalize.rename_failed in pipeline.c one level up and covers the rename that one doesn't. I'd like to merge it; a few things first.

Blocking

1. DCO sign-off is missing. The commit has no Signed-off-by: trailer, so the dco check will fail. git commit --amend -s --no-edit and force-push is all it needs (a noreply identity is fine).

2. cbm_writer_open() still clobbers the reason — and it is the Windows: index always fails with generic 'Pipeline failed' — protected-DACL cache dir breaks MoveFileExW(REPLACE_EXISTING) on this host #1620 case. In the head, internal/cbm/sqlite_writer.c:2302-2306: when cbm_fopen() fails, (void)cbm_unlink(w->wc.temp_path) runs on a file that was never created, so errno becomes ENOENT before NULL is returned. Your new early-return log at src/graph_buffer/graph_buffer.c:1731 then reads that errno, and an access-denied temp directory (the scenario this PR exists to explain) is reported as "No such file or directory" — which sends people chasing a missing path instead of a permission problem. Same save/restore shape you used in discard_writer_output() fixes it. While there: the snprintf/make_writer_temp_path branch just above returns NULL with whatever errno happened to be lying around; either set it (ENAMETOOLONG fits) or don't let the caller print a reason for that class.

Please add

3. A test that binds the writer half. Reverting only sqlite_writer.c leaves the suite green: on POSIX the cleanup cbm_unlink() succeeds (the temp file exists) and a successful call never touches errno, so nothing asserts the save/restore — a later refactor could silently undo it. A case where the cleanup itself fails (e.g. remove the temp file before publish so cbm_unlink sets ENOENT, and assert the rename's errno still comes through) would pin it on every platform.

Minor

4. The calloc early return in cbm_gbuf_dump_to_sqlite() (graph_buffer.c:1699-1702) returns non-zero without the "exactly one signature" the comment at 1629-1633 promises — log there too, or soften the comment.
5. For the sticky append-failure class (cbm_writer_finalize(), sqlite_writer.c:2369-2371) the errno you preserve was set many calls earlier, so the errno= field is noise by construction. Omitting it (or -1) when the failure did not originate at the publish boundary keeps the log from claiming a reason it doesn't have.
6. Nit: the assertions are text-format substrings (errno=, msg=gbuf.dump ); pinning the format in the test like test_log.c does keeps it honest under CBM_LOG_FORMAT=json.

One honest limit from my side: I reviewed on macOS, so the Windows MoveFileExW denial branch is verified by reading, not by running — once the above is in, it goes through our Windows leg before merge.

Refs #1620, #2001.

[junk151516]

Thanks — (2) is the one that mattered, and you are right that it points the wrong way in exactly the case this PR exists to explain. All six are addressed; measurements below.

1. DCO. Signed off and force-pushed.

2. cbm_writer_open(). Both branches fixed.

The cbm_fopen() failure now carries the open's errno across the cleanup, same save/restore shape as discard_writer_output(). Worth stating plainly: without this the PR made that path worse, not just incomplete — an access-denied cache directory reported ENOENT through the new log line, which is a more confident wrong answer than the generic pipeline failure it replaced.

The truncation branch above it now sets ENAMETOOLONG. make_writer_temp_path() only fails by truncation and neither condition sets an errno of its own, so naming it is exact rather than a guess.

3. Test that binds the writer half. Added two, and you were right that the suite was green without them.

sw_publish_failure_reports_the_rename_not_the_cleanup turns the still-open temp into a directory before finalize. The publish rename then fails with ENOTDIR while the cleanup unlink() fails with EISDIR/EPERM, so the two are distinguishable and only a saved errno delivers the rename's reason. Reverting only sqlite_writer.c on Linux:

sw_open_truncated_path_names_its_reason       FAIL: errno == 0, expected ENAMETOOLONG == 36
sw_publish_failure_reports_the_rename_not_the_cleanup
                                              FAIL: publish_errno == 21, expected ENOTDIR == 20
12 passed, 2 failed

21 is the cleanup's EISDIR — the exact substitution you described.

One limit, stated rather than hidden: that test is SKIP_PLATFORM on Windows, because removing a file that is still open is precisely what Windows does not allow, so the setup cannot be built there. sw_open_truncated_path_names_its_reason does run everywhere and pins the writer_open half on every platform. If you want the rename half pinned on Windows too, it needs a seam rather than filesystem trickery — happy to add one in a follow-up if you'd rather have it than not.

4. The calloc early return now logs the same event, so the "exactly one signature" comment is true as written.

5. cbm_writer_finalize() clears errno before discarding on a sticky append failure, and log_dump_failed() omits the field entirely when it is zero. A record that cannot name a reason no longer prints one.

6. capture_logs_start() pins CBM_LOG_FORMAT_TEXT and restores the previous format, following test_log.c.

Verification

Linux, gcc, ASan+UBSan, graph_buffer + sqlite_writer	69 passed, no skips
Linux, reverting only sqlite_writer.c	2 failed, as above
Windows x64, MSYS2/CLANG64, clang 22.1.8, ASan+UBSan	67 passed, 2 skipped

On the Windows branch you flagged

You mentioned reviewing on macOS, so the MoveFileExW denial is verified by reading on your side. This end is a Windows host — it is the unmanaged control from #1620, where the same protected owner-only DACL permits the rename. If it's useful, I can run whatever probe you want against that branch and report the results; that asymmetry is the one thing I can contribute here that you can't easily reproduce.

[DeusData]

Approved on merit. You had the last word here on 2026-09-04 and got sixteen days of silence in return — sorry about that.

This is a small diff fixing a failure mode that is disproportionately annoying in production: the cleanup path destroying the evidence of the failure it is cleaning up after.

errno is global and effectively write-only from the caller's perspective — any library call between the failure and the report may overwrite it, and unlink during cleanup is exactly such a call. So the publish failed for one reason and the operator was told a different one, or none at all. Saving it before cleanup and restoring it afterwards is the correct fix, and doing it at each of the three sites rather than once globally is right too, since each has its own window.

The detail I want to credit is the comment:

a library call may set errno even when it succeeds

That is the part people get wrong. It is tempting to assume errno is only meaningful after a failure and therefore safe to read late — but a successful call is permitted to set it, so reading it after cleanup can produce a plausible-looking but entirely fictional reason. Naming that in the comment is what stops the next person from "simplifying" the save/restore away.

Preserving the translated error from cbm_rename_replace (#1620) across the cleanup unlink matters most of all, since that is the one carrying the platform's actual reason — a permission denial or a sharing violation is precisely what an operator needs to see.

I also like that you named the truncation cases explicitly, where neither condition sets errno. Reporting a stale or zero errno there would be worse than saying nothing; spelling out that those need their own message is the honest handling.

Queued to merge behind #2248, which fixes an unrelated failing test on main. Nothing further needed from you.

[DeusData]

A status note so this does not look stalled after today's approval.

When I went to merge, the actual merge attempt found one conflict in src/graph_buffer/graph_buffer.c: your log_dump_failed(...) call sits ahead of three free calls that a large memory-core change on main (2026-09-19) converted to cbm_free. One hunk; both sides kept.

Rather than ask you to rebase after we had already kept this waiting sixteen days, I carried the rebase myself as #2259 — your commit, your authorship, your sign-off, unchanged apart from that one resolution. It builds clean and passes 421 tests on the rebased result. It merges as soon as an unrelated linter regression on main (#2257, ours) lands; this PR then closes with the merge credited to you.

If you would rather rebase this branch yourself and have it merge from here instead, say so and I will close #2259 — either way is fine, and either way the fix is yours.

[DeusData]

Your change is on main — merged through #2259 as d615088, with your commit and your authorship preserved (git log on main shows you as the author of 460ebef; the only thing I added was the one-hunk conflict resolution in src/graph_buffer/graph_buffer.c, keeping your log_dump_failed(...) call ahead of the three releases that main had meanwhile converted to cbm_free).

Closing this PR as superseded by that carry, not because anything was wrong with it — the opposite. The defect you fixed is the kind that costs hours when it bites: the cleanup path overwriting errno and destroying the evidence of the very failure it is cleaning up after, so the log names a successful dump instead of the reason the atomic publish failed. Capturing the cause before cleanup is the right fix and the right size.

Verified before landing: graph_buffer 56/0 and sqlite_writer 14/0 on the merge result, memory-core linter clean, full CI green on the final head. It ships in the next patch release.

Thank you for the fix, and for your patience through sixteen days of silence that you did nothing to deserve.

[junk151516]

Thank you — for carrying the rebase yourself, for keeping the authorship intact, and for the care in every reply on this thread. No need to apologize for the wait; being kept informed the whole way through was more than enough.

We use codebase-memory-mcp daily across our projects and we're genuinely happy with it. Glad this small fix could give something back.