Skip to content

fix: fail closed on unreadable legacy tombstones - #247

Open
perfloop[bot] wants to merge 3 commits into
mainfrom
perfloop-pr-open-2ma3ywfrew
Open

perfloop[bot] wants to merge 3 commits into
mainfrom
perfloop-pr-open-2ma3ywfrew

Conversation

@perfloop

@perfloop perfloop Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

A failed legacy tombstone read no longer authorizes a metadata write. This prevents old multipart metadata from resurfacing after a newer same-key completion while keeping metadata warming best-effort and successful S3 responses unchanged.

  • Legacy coordinator: tombstoneTimestamp distinguishes a genuinely absent marker from cache read or routing errors and malformed marker lengths. putMeta refuses the visibility write and records meta_put/error when marker state is unknown. A truly absent marker still permits a warm, and an ordered write with a stale token is still blocked by a newer tombstone.
  • Regression coverage: TestLegacyCoordinator_TombstoneReadErrorAndMalformedMarkerRefuseMetaWrite covers read/routing errors and malformed markers; TestLegacyCoordinator_TombstoneAbsenceStillAllowsMetaWarm covers genuine absence; TestLegacyCoordinator_TombstoneBlocksStalePopulate covers a valid newer tombstone. TestCompleteMultipart_TombstoneReadFailureDoesNotResurrectOldMetadata holds the first completion's v1 HEAD while a second same-key completion returns v2, then fails only the first warm's tombstone read and checks stored metadata, HEAD, and conditional GET.
  • Metrics docs: docs/metrics.md now includes CAS PutIfVersion version mismatches in meta_put/precondition_lost, alongside the legacy missing-token and newer-tombstone causes. Unreadable or malformed legacy markers remain error. The configuration docs and README describe the fail-closed behavior.

The controlled in-memory cache and forwarder establish this injected ordering, not its frequency in production. The assertion covers stale metadata and ETags, not old object bytes from missing blocks. The second metadata warm can remain coalesced behind the first worker, so the newer object may require an upstream miss rather than an immediate cache hit.

Tests: make lint-ci, make build, make test, make test-integration, make test-race, and make test-coverage passed. The regression tests named above exercise the marker errors, absence behavior, valid-tombstone guard, and multipart schedule.

The repair assertion was violated at c69532431456d12d853300d524043787e8d649aa and satisfied on the final source.

bash -c "set -eu\nassertion='With cache and block caching enabled in default legacy coordination, a failed tombstone read during a held pre-overwrite metadata warm must not publish or return the old ETag through HEAD or If-None-Match GET after a newer same-key multipart completion succeeds.'\nresult=\"${TMPDIR}/tombstone-result.$$\"\nnative_log=\"${TMPDIR}/tombstone-native-output.$$\"\nrm -f \"$result\" \"$native_log\"\nset +e\nPERFLOOP_ASSERTION_RESULT_FILE=\"$result\" \"$PERFLOOP_BUILD_OUTPUT_DIR/proxy.test\" -test.run '^TestCompleteMultipart_TombstoneReadFailureDoesNotResurrectOldMetadata$' -test.count=1 -test.timeout=30s >\"$native_log\" 2>&1\ntest_status=$?\nset -e\nif [ -f \"$native_log\" ]; then\n cat \"$native_log\" >&2\nfi\noutcome=$(cat \"$result\" 2>/dev/null || true)\nrm -f \"$result\" \"$native_log\"\nif [ \"$outcome\" = \"violated\" ] && [ \"$test_status\" -ne 0 ]; then\n printf '{\"assertion\":\"%s\",\"outcome\":\"violated\"}\\n' \"$assertion\"\nelif [ \"$outcome\" = \"satisfied\" ] && [ \"$test_status\" -eq 0 ]; then\n printf '{\"assertion\":\"%s\",\"outcome\":\"satisfied\"}\\n' \"$assertion\"\nelse\n printf 'native test result incomplete: status=%s outcome=%s\\n' \"$test_status\" \"$outcome\" >&2\n printf '{\"assertion\":\"%s\",\"outcome\":\"inconclusive\"}\\n' \"$assertion\"\nfi\nexit 0"

Recorded proof ran with retained support that is not included in this pull request.

Other target-CI checks: S3 Compatibility Tests / s3-compat-tests (default), S3 Compatibility Tests / s3-compat-tests (tiered), S3 Compatibility Tests (Cluster) / s3-compat-tests-cluster (default), Lint PR Title / lint, and CLA Assistant / cla.


Generated by Perfloop. Measurements and checks. Reproduce this comparison (tenant access required).


Note

Medium Risk
Changes cache metadata ordering on tombstone read failures, reducing stale-metadata exposure but possibly skipping best-effort warms under transient cache errors; S3 write responses are unchanged.

Overview
Legacy cache coordination now fails closed when invalidation tombstones cannot be trusted. Ordered metadata writes (PutMetaIfVersion) used to treat tombstone read failures like “no tombstone,” which could let a slow meta-on-write publish stale ETags after a newer same-key write.

tombstoneTimestamp now returns an error for backend/routing failures and non–8-byte markers; only genuine absence (NotFound / nil data) still allows a warm. putMeta propagates those errors and increments meta_put/error instead of committing metadata.

Regression tests cover coordinator refusal paths, absent-marker success, and a multipart meta-on-write race where the first worker’s tombstone read fails after a second completion. README, configuration, and metrics docs describe the behavior and label semantics.

Reviewed by Cursor Bugbot for commit 369a1a8. Bugbot is set up for automated code reviews on this repo. Configure here.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@greptile-apps

greptile-apps Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Critical risk] Changes how cache invalidation markers are read and enforced.

The PR appears safe to merge; no outstanding finding or new blocking issue was identified.

Summary

The PR makes legacy metadata publication fail closed when a tombstone read fails or returns a malformed marker, adds coordinator and multipart-race regression tests, and clarifies configuration and metric documentation.

Reviews (3) · Last reviewed commit: "fix: document CAS metadata precondition ..."

Comment thread cache/coordinator.go
Comment thread docs/metrics.md Outdated
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@perfloop
perfloop Bot force-pushed the perfloop-pr-open-2ma3ywfrew branch from 0127bf7 to 369a1a8 Compare October 3, 2026 04:41
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.

1 participant