fix(graph-buffer): log why an atomic publish failed instead of a successful dump - #2259
Merged
Merged
Conversation
…essful dump cbm_gbuf_dump_to_sqlite() emitted gbuf.dump regardless of the writer's result, so a run that published nothing still logged node and edge counts as if it had worked, and the failure reached the user only as the generic "Pipeline failed. Check repo_path exists and contains source files." The writer's temp -> final rename in publish_writer_output() returned a bare ERR_WRITE_FAILED. #1628 taught cbm_rename_replace() to translate the platform error into errno so callers could report why, and wired up the stage -> final rename in pipeline.c. This is the other rename on that path, and it is the one that runs first. Preserve errno across the cleanup in every publish error branch. That includes cbm_writer_open(), where the cleanup unlinks a file that was never created: without the save it leaves ENOENT behind, so a directory that denied the create is reported as a missing path — the #1620 case, stated wrongly. Read the value in graph_buffer.c before the profiling macro can clobber it, and report gbuf.dump_failed with the return code and that errno. A sticky append failure clears errno rather than attach a reason it does not have, and the field is omitted when there is none. No logging is added inside internal/cbm/, which has none today. Tests cover both halves: the dump reports a failure instead of a summary, the writer names its truncation reason, and the publish rename's errno survives a cleanup that itself fails. Refs #1620, #2001. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012FxkVF773gDGNmBmsLnzN7 Signed-off-by: juan Carlos Estrada Montoya <junk151516@users.noreply.github.com>
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.
This is #2051 by @junk151516, rebased onto current
mainby the maintainer. His commit, his authorship, his sign-off — unchanged except for one conflict resolution. #2051 stays open until this merges, then closes with credit.Why a carried branch
#2051 was approved on merit today, and the actual merge attempt found one conflict in
src/graph_buffer/graph_buffer.c: the PR adds alog_dump_failed(...)call ahead of threefreecalls that yesterday's memwaste merge converted tocbm_free. Resolved by keeping both — main'scbm_freewith his log call ahead of it. Nothing else in the four files conflicted, and no raw allocator calls are added (checked against the memory-core linter's regex).The alternative was asking him to rebase after we had already sat on the PR for sixteen days. Carrying one hunk with his authorship intact seemed the fairer trade.
What the fix does
The cleanup path was destroying the evidence of the failure it was cleaning up after:
unlinkduring cleanup clobberserrno, so an atomic-publish failure reported the wrong reason or none.errnois saved before cleanup and restored after, at three sites — and his comment names the part people get wrong: a successful library call may seterrno, so reading it late yields a plausible but fictional cause. The translated platform error fromcbm_rename_replace(#1620) survives the cleanup, and the two truncation cases where nothing setserrnoare named explicitly.Verification on the rebased result
-Werror, ASan/UBSansqlite_writer graph_buffer pipeline store_nodes→ 421 passed, 0 failedlint-memory-core: this branch adds no raw sites. It currently inheritsmain's own regression (mcp.c814 → 817, from feat(index): add opt-in discovery resource limits #1723), fixed in fix: restore main to green after #1723 — memory-core linter and worker-scope Step 5f #2257 — this merges after that.