[torcolvin]

Broken:

• The expirer selected every row with an expired exp, including tombstones. A tombstone that kept an expiry was deleted again, which made a second deletion event with a new CAS and a higher revSeqNo. Couchbase Server does not expire tombstones. This could end up causing spinning in the expiry va;lue.
• DeleteWithXattrs did not set tombstone=1 or isJSON=0, and kept the old expiry. Couchbase Server sets the expiry of the tombstone to 0.
• WriteCas with a nil value did not set tombstone=1 and kept user xattrs. Delete removes user xattrs.

Fixed:

• expireDocuments and nextExpiration skip rows with tombstone=1.
• DeleteWithXattrs sets exp=0, isJSON=0 and tombstone=1, the same values that remove() writes.
• WriteCas with a nil value sets tombstone=1 and removes user xattrs.
• remove() uses processXattrs with removeUserXattrs in place of its own loop.
• TestExpirationSkipsTombstones covers each delete path. It checks the tombstone flag and exp, and that the expirer does not change the tombstone. TestTombstoneRemovesUserXattrs checks that Delete and WriteCas with a nil value keep system xattrs and remove user xattrs.

This doesn't fail any tests currently in Sync Gateway but it can fail some tests in the future. I didn't want to do a test in Sync Gateway because I can't use synctest - gocb is not compatible.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved issues remain in WriteCas nil-value handling across write branches and tombstone xattr preservation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Preserve xattrs when deleting existing tombstones · New
• Set tombstone status for nil writes in every WriteCas branch · New

What changed in this PR

This pull request prevents tombstones from being re-expired and aligns delete metadata and xattr handling with Couchbase behavior.

Changes:

• Excludes tombstones from expiration scans and scheduling.
• Normalizes tombstone metadata for delete paths.
• Adds regression coverage for expiration and xattr cleanup.

File	Summary
collection+xattrs.go	Updates DeleteWithXattrs tombstone metadata.
collection.go	Adjusts nil writes, xattr cleanup, and expiration filtering. Review findings identify moderate and critical issues in nil-write branches and tombstone xattr preservation.
collection_test.go	Tests user and system xattr behavior.
bucket_test.go	Tests expiration behavior across delete paths.
bucket_api.go	Excludes tombstones from next-expiration calculation.

---

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