Fix transaction safety and reader regressions in Go and C

[Soph]

Summary

Address transaction safety and reader/writer correctness findings with deterministic regression tests in Go and C:

• Preserve concurrent additions and unrelated compactions when publishing a compacted stack: reread tables.list after reacquiring the lock and replace only the still-contiguous compacted range.
• Track C lock ownership so failed addition/subtable lock acquisition cannot unlink another writer's lock; make Go atomic-writer cleanup idempotent and keep closed and committed states distinct.
• Validate block-header sizes, restart-table bounds, and restart offsets before indexing memory; bound Go log decompression by the declared size.
• Preserve referenced tables when manifest rename succeeds but subsequent directory sync reports an error, for both Go additions and compactions.
• Distinguish retryable pre-publication lock failures from failures after publication. ErrPostCommit must never match ErrLockFailure; use errors.As to obtain *PostCommitError and inspect its explicit Cause. This also covers maintenance failures after Stack.Add commits. Reload timeouts have a separate sentinel.
• Preserve borrowed readers after failed reloads, validate the Go merged replacement before swapping, and do not unlink retained tables when reopening without reuse.
• Restore the update-index base in indexed RefsFor iteration in Go and C, including safe C handling of one-OID refs.
• Handle empty Go stack bounds, cleanup, and expiration compaction without panics.
• Validate Go hash IDs, manifest path components, and writer allocation/minimum block-size bounds.
• Flush every Go writer index level's last block, clear cross-section index state, and stop non-shrinking levels.
• Use a concurrency-safe RNG for independent Go stacks.
• Preserve C compaction read/write errors instead of publishing partial output, and remove Go table files whose sync failed before manifest publication.

Also fixes the Go compaction nil-lock defer/retry path, routes compaction cleanup through Storage, and adds missing C header includes needed for direct compilation.

Publication error contract

if errors.Is(err, reftable.ErrLockFailure) {
    // No manifest was published by this write; the transaction may be retried.
}
var published *reftable.PostCommitError
if errors.As(err, &published) {
    // The manifest was published. Do not replay the update.
    // Reopen the stack to inspect the current state.
    // Inspect published.Cause explicitly for diagnostics.
}

Cause is intentionally not in the unwrap chain: a lock error during post-commit reload or maintenance must not accidentally trigger a write retry. Publication does not imply durability was confirmed.

Tests

The original regression tests failed before their fixes. Coverage includes concurrent neighboring compaction, lock contention, errors before/after publication, failed reloads, C late-read failures, malformed restart layouts, indexed lookups, and atomic-writer lifecycle states.

• go test ./...
• go test -race ./... -count=1
• go vet ./...
• Post-publication fault injection for Add, Addition.Commit, CompactAll, and automatic maintenance; verify a client retry loop does not replay a published update
• First consolidated commit independently passes Go tests and vet
• All ten C test binaries compiled and passed with Clang, strict warnings, AddressSanitizer and UndefinedBehaviorSanitizer, using system zlib
• git diff --check
• Bazel test execution (Bazel unavailable in the development environment)
• Windows execution

Interleaving uses storage/block-source hooks, not timing-sensitive goroutines or sleeps. Directory-sync failure is modeled by a wrapper returning an error after a real successful publication; no real filesystem sync failure is induced. Reload timeout/error classification is injected after publication without waiting for the retry deadline.

Review history

The transaction-safety corrections and post-publication error contract are folded into the first commit. All commits have Signed-off-by trailers, and the validation commit title is narrowed to the inputs it validates rather than claiming complete panic prevention.

Remaining scope

This is not complete parser hardening or a resolution of the full repository review. Integer overflows in record decoders, cyclic indexes, remaining nil-iterator paths, reflog tombstones, C writer index construction, the C durability protocol, and shared-instance Go concurrency need separate work. The broader local findings.md document is deliberately not included in this PR.

[Soph]

Fixed all three findings from the second review in f25783f:

• C compaction now propagates ref read/write errors before seeking logs. A late-read fault-injection test verifies the original manifest and all nine refs survive, and that temporary files and locks are cleaned up.
• Failed table commits that published a file before a directory-sync error explicitly remove that unreferenced file. Tests cover empty and existing stacks, preservation of existing refs, and successful retry.
• NewWriter rejects blocks smaller than the version-dependent file header + block header + restart count. Tests cover every undersized value for default/SHA-1/SHA-256 and the minimum/default-size boundaries.

Each regression test failed before its fix. Validation: full Go suite with -race, go vet, git diff --check, and all ten C test binaries with strict Clang warnings plus AddressSanitizer/UBSan pass. Bazel and Windows execution remain outstanding.

[Copilot]

🔵 Needs a closer look

The changes span low-level parsing, durability/publication semantics, and concurrency behavior across both Go and C implementations, which warrants final human validation despite strong regression coverage.

Pull request overview

This PR hardens reftable read/write and stack transaction behavior across the Go and C implementations, addressing multiple concurrency, durability/publication, and malformed-input safety regressions with accompanying deterministic regression tests.

Changes:

• Fix stack publication/reload/cleanup correctness (preserve concurrent manifest changes, avoid unlinking others’ locks, keep committed-vs-closed semantics, and avoid reader loss on reload failures).
• Add input validation and bounds checks in block parsing (header sizing, restart tables/offsets, hash-id handling, and log decompression limits) to prevent panics/over-allocation.
• Add/adjust regression tests in Go and C covering the above failure modes and previously silent data loss cases.

File summaries

File	Description
writer.go	Validates writer config pre-allocation; fixes index-level flushing to prevent silent record loss/infinite loops.
writer_index_test.go	Regression test ensuring finishSection index covers all records across sections.
validation_test.go	New end-to-end validation tests for traversal, hash IDs, block sizing, and hostile log block sizes.
storage.go	Makes AtomicWriter semantics explicit; implements idempotent Close and committed-state tracking.
storage_test.go	Tests writer lifecycle invariants (idempotent Close, rename failures, committed-file preservation).
stack.go	Adds manifest table-name validation; fixes reload reader ownership; preserves concurrent manifest edits during compaction; improves durability-error handling.
stack_concurrent_test.go	Race-focused tests for concurrent stacks and uniqueness of generated table names.
review_regression_test.go	Deterministic regression suite for compaction/addition interleavings, reload failures, empty-stack behavior, index update-index correctness, and malformed blocks.
reftable_test.go	Adjusts expectations for index-level count after index flushing fix.
reader.go	Validates hash ID and footer layout; adds block header bounds checks; restores update-index base in indexed iteration.
merged.go	Handles empty merged stacks safely for min/max update-index bounds.
constants.go	Introduces maxDeflateRatio constant for validating declared log decompression sizes.
api.go	Changes HashID.Size to return 0 instead of panicking for unknown IDs; updates doc comment accordingly.
block.go	Adds block layout validation, restart-table bounds checks, and log decompression size limiting.
block_validation_test.go	Go tests for rejecting malformed blocks and oversized log streams.
c/strbuf.h	Adds missing include for sys/types.h for direct compilation.
c/stack.c	Fixes reload borrow/free semantics; tracks lock ownership; preserves concurrent manifest changes during compaction; avoids publishing partial compactions on read error.
c/stack_test.c	Adds deterministic C regression tests for concurrent addition during compaction, read-error preservation, reload failure reader preservation, and lock ownership behavior.
c/readwrite_test.c	Extends RefsFor tests to validate update-index base handling for peeled and non-peeled refs.
c/reader.c	Adds header bounds checking and ensures block cleanup on init failures.
c/iter.c	Restores update-index base for indexed RefsFor iteration; handles VAL1 vs VAL2 safely.
c/block.c	Adds strict block header/restart-table bounds checking and restart-offset validation.
c/block_test.c	Adds C test coverage for rejecting invalid block layouts.

Review details

• Files reviewed: 23/23 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Soph]

Addressed the initial feedback on history and retry semantics:

• Folded the corrective commit and the post-publication error contract into the first safety commit. Removed the dangling 14e383b reference.
• Added Signed-off-by trailers to every commit, preserving existing co-author attribution. The rewrite was pushed with an explicit force-with-lease; a local backup branch is retained.
• Narrowed the validation commit title to “Reject unknown hash IDs and unsafe manifest names”; it no longer claims all malformed inputs are panic-free.
• Introduced ErrPostCommit and PostCommitError.Cause. After publication, neither reload errors nor automatic-maintenance errors can match ErrLockFailure and cause a client to replay a committed update. Reload timeout now has a separate sentinel. Pre-publication contention remains retryable; no-op additions do not falsely report publication.

New tests exercise Add, Addition.Commit, CompactAll, auto-compaction, and a typical errors.Is-based retry loop. The full Go race-enabled suite and vet pass, and the first consolidated commit independently passes Go tests and vet. The rewritten final source tree is identical to the tested pre-rebase tree.

For the two surviving panics in feedback item 1, please share the specific reproducers or locations so they can be addressed without guessing. The PR description explicitly documents the remaining parser-hardening scope.