Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-40467

VBucket::handlePreExpiry may keep the body in the payload

    XMLWordPrintable

    Details

    • Triage:
      Triaged
    • Story Points:
      1
    • Is this a Regression?:
      No

      Description

      Using a 2 node cluster against 6.6.0 build 7873.

      When running the gocb test suite at some point during the TestQuery test the SDK connection is closed by the server and then all kv tests thereafter fail with temp fail. Looking in the server logs (attached) it looks like memcached crashes and doesn't recover. In the UI the bucket shows as "1 node pending".

      To repro create a 2 node cluster with kv,index,n1ql on one node and kv,index,n1ql,fts,cbas on the other. Create a default bucket. Clone https://github.com/couchbase/gocb and run tests with

      go test -race -v ./ --server <address> --user <user> --pass <password> --bucket default --version 6.5.0

      During the TestQuery test the logs will output an EOF and then the SDK will be unable to recover (and tests will take a long time to complete) due to memcached being down on one node. Unfortunately I've been unable to distill this down into a smaller repro case and I can't tell what is causing the crash. I can reliably reproduce this via our SDK jenkins and locally using our cbdyncluster infrastructure. This was not happening on build 7692 and does not happen against 6.5.1.

        Attachments

          Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

            Activity

            Hide
            drigby Dave Rigby added a comment -

            From cbcollect_info_ns_1@172.23.111.130_20200714-165638:

            2020-07-14T16:51:37.169028+00:00 ERROR 169: exception occurred in runloop during packet execution. Cookie info: [{"aiostat":"success","connection":"[ 127.0.0.1:44347 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]","engine_storage":"0x00007f8651b71f10","ewouldblock":false,"packet":{"bodylen":596,"cas":1594745442052407296,"datatype":["JSON","Snappy","Xattr"],"extlen":20,"key":"<ud>lookupDocGetFull</ud>","keylen":16,"magic":"ClientRequest","opaque":506,"opcode":"DCP_EXPIRATION","vbucket":1015},"refcount":1}] - closing connection ([ 127.0.0.1:44347 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]): xattr::utils::check_len(3657713664) exceeds 560
            

            This appears to be a duplicate of MB-40370 which is fixed as of 6.6.0-7879.

            Show
            drigby Dave Rigby added a comment - From cbcollect_info_ns_1@172.23.111.130_20200714-165638: 2020-07-14T16:51:37.169028+00:00 ERROR 169: exception occurred in runloop during packet execution. Cookie info: [{"aiostat":"success","connection":"[ 127.0.0.1:44347 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]","engine_storage":"0x00007f8651b71f10","ewouldblock":false,"packet":{"bodylen":596,"cas":1594745442052407296,"datatype":["JSON","Snappy","Xattr"],"extlen":20,"key":"<ud>lookupDocGetFull</ud>","keylen":16,"magic":"ClientRequest","opaque":506,"opcode":"DCP_EXPIRATION","vbucket":1015},"refcount":1}] - closing connection ([ 127.0.0.1:44347 - 127.0.0.1:11209 (<ud>@ns_server</ud>) ]): xattr::utils::check_len(3657713664) exceeds 560 This appears to be a duplicate of MB-40370 which is fixed as of 6.6.0-7879.
            Hide
            charles.dixon Charles Dixon added a comment -

            I'm still seeing this against build 7880, logs attached (.139 and .136). The cluster is still running and I can provide access if required.

            Show
            charles.dixon Charles Dixon added a comment - I'm still seeing this against build 7880, logs attached (.139 and .136). The cluster is still running and I can provide access if required.
            Hide
            paolo.cocchi Paolo Cocchi added a comment - - edited

            Different error in the latest log set.

            Producer - 139

            2020-07-15T06:57:09.329835+00:00 ERROR 160: (default) DCP (Producer) eq_dcpq:replication:ns_1@172.23.111.139->ns_1@172.23.111.136:default - DcpProducer::handleResponse disconnecting, received unexpected response:{"bodylen":0,"cas":0,"datatype":"raw","extlen":0,"keylen":0,"magic":"ClientResponse","opaque":506,"opcode":"DCP_EXPIRATION","status":"Invalid arguments"}
            ..
            <other instances of the same>
            

            Consumer - 136

            2020-07-15T06:57:09.314047+00:00 WARNING 168: (default) DCP (Consumer) eq_dcpq:replication:ns_1@172.23.111.139->ns_1@172.23.111.136:default - DcpConsumer::deletion: (vb:1015) Value cannot contain a body
            

             

            Plus, crash on node 139 caused by:

            item.cc:480

            void Item::removeUserXattrs() {
            ..
                if (!mcbp::datatype::is_xattr(getDataType())) {
                    // No Xattrs, nothing to do
                    return;
                }
             
                // No-op if already uncompressed
                decompressValue();
             
                // The function currently does not support value with body.
                // That is fine for now as this is introduced for MB-37374, thus is supposed
                // to operate only against deleted items, which don't contain any body.
                Expects(isDeleted());
                const auto valNBytes = value->valueSize();
                cb::char_buffer valBuf{const_cast<char*>(value->getData()), valNBytes};
                const auto bodySize = valNBytes - cb::xattr::get_body_offset(valBuf);
                Expects(bodySize == 0);    <-- THROWS
            ..
            }
            

            Show
            paolo.cocchi Paolo Cocchi added a comment - - edited Different error in the latest log set. Producer - 139 2020-07-15T06:57:09.329835+00:00 ERROR 160: (default) DCP (Producer) eq_dcpq:replication:ns_1@172.23.111.139->ns_1@172.23.111.136:default - DcpProducer::handleResponse disconnecting, received unexpected response:{"bodylen":0,"cas":0,"datatype":"raw","extlen":0,"keylen":0,"magic":"ClientResponse","opaque":506,"opcode":"DCP_EXPIRATION","status":"Invalid arguments"} .. <other instances of the same> Consumer - 136 2020-07-15T06:57:09.314047+00:00 WARNING 168: (default) DCP (Consumer) eq_dcpq:replication:ns_1@172.23.111.139->ns_1@172.23.111.136:default - DcpConsumer::deletion: (vb:1015) Value cannot contain a body   Plus, crash on node 139 caused by: item.cc:480 void Item::removeUserXattrs() { .. if (!mcbp::datatype::is_xattr(getDataType())) { // No Xattrs, nothing to do return; }   // No-op if already uncompressed decompressValue();   // The function currently does not support value with body. // That is fine for now as this is introduced for MB-37374, thus is supposed // to operate only against deleted items, which don't contain any body. Expects(isDeleted()); const auto valNBytes = value->valueSize(); cb::char_buffer valBuf{const_cast<char*>(value->getData()), valNBytes}; const auto bodySize = valNBytes - cb::xattr::get_body_offset(valBuf); Expects(bodySize == 0); <-- THROWS .. }
            Hide
            paolo.cocchi Paolo Cocchi added a comment - - edited

            Summary
            VBucket::handlePreExpiry doesn't remove the body (if any) in the payload.
            Depending on whether the DCP connection enables IncludeDeletedUserXattrs or not:

            • case IncludeDeletedUserXattrs::Yes - The Producer replicate an Expiration with body. Which causes the Consumer failing validation, returning EINVALID to Producer and the Producer closing the connection
            • case IncludeDeletedUserXattrs::No - The Producer attempts to Item::removeUserXattrs() before streaming. That causes a throw at item.cc:480 as the function doesn't expect any Body for deletions (by expiration in this case)

             

            Details
            VBucket::handlePreExpiry is execute in the usual frontend GET path and also as part of the expiration path from backfill. In this test expirations triggers at backfill.

            void VBucket::handlePreExpiry(const HashTable::HashBucketLock& hbl,
                                          StoredValue& v) {
            ..
                    auto result = sapi->document->pre_expiry(itm_info);
                    // The API states only uncompressed xattr values are returned
                    auto datatype = PROTOCOL_BINARY_DATATYPE_XATTR;
                    if (!result.empty()) {
                        Item new_item(v.getKey(),
                                      v.getFlags(),
                                      v.getExptime(),
                                      result.data(),
                                      result.size(),
                                      datatype,
                                      v.getCas(),
                                      v.getBySeqno(),
                                      id,
                                      v.getRevSeqno());
             
                        new_item.setNRUValue(v.getNRUValue());
                        new_item.setFreqCounterValue(v.getFreqCounterValue());
                        new_item.setDeleted(DeleteSource::TTL);
                        ht.unlocked_updateStoredValue(hbl, v, new_item);
                    }
                }
            }
            

            The call to pre_expiry() returns the new value. If the value is non-empty, then we execute into the if-block and update the StoredValue in the HT.
            But, if the post-expiry value is empty we just skip.
            If the value contains a body, then it will be pushed into the ActiveStream::readyQ and processed for streaming. Depending on the DCP connection configuration, that may fails as described in the summary above.

            Eg, payload with Body+UserXattrs caught in the test (xattrs-size 0x1b (27) + body):

            (lldb) memory read --count `payload.len` `payload.buf`
            0x10469ba09: 00 00 00 1b 00 00 00 17 78 61 74 74 72 70 61 74  ........xattrpat
            0x10469ba19: 68 00 22 78 61 74 74 72 76 61 6c 75 65 22 00 7b  h."xattrvalue".{
            0x10469ba29: 22 61 62 76 22 3a 37 2e 36 2c 22 62 72 65 77 65  "abv":7.6,"brewe
            0x10469ba39: 72 79 5f 69 64 22 3a 22 35 31 32 5f 62 72 65 77  ry_id":"512_brew
            0x10469ba49: 69 6e 67 5f 63 6f 6d 70 61 6e 79 22 2c 22 63 61  ing_company","ca
            ..
            0x10469bc19: 22 41 6d 65 72 69 63 61 6e 2d 53 74 79 6c 65 20  "American-Style 
            0x10469bc29: 42 72 6f 77 6e 20 41 6c 65 22 2c 22 74 79 70 65  Brown Ale","type
            0x10469bc39: 22 3a 22 62 65 65 72 22 2c 22 75 70 64 61 74 65  ":"beer","update
            0x10469bc49: 64 22 3a 22 32 30 31 30 2d 30 37 2d 32 32 54 32  d":"2010-07-22T2
            0x10469bc59: 30 3a 30 30 3a 32 30 5a 22 7d                    0:00:20Z"}
            

             

            Note
            This is not a regression.
            The bug at VBucket::handlePreExpiry has been there for a while. In KV, we have fixed a similar issue (in a different expiry-path) in MB-29040. For MB-29040 we also "made the Consumer resilient to invalid payloads" by pruning the body for deletion/expiration if we receive any.
            But, that behaviour has also hidden the existing issue at VBucket::handlePreExpiry. Until for IncludeDeletedUserXattrs we have added a bunch of extra validation at Producer and Consumer to ensure that deletions never ship Body in the payload, which is what we hit here.

            Show
            paolo.cocchi Paolo Cocchi added a comment - - edited Summary VBucket::handlePreExpiry doesn't remove the body (if any) in the payload. Depending on whether the DCP connection enables IncludeDeletedUserXattrs or not: case IncludeDeletedUserXattrs::Yes - The Producer replicate an Expiration with body. Which causes the Consumer failing validation, returning EINVALID to Producer and the Producer closing the connection case IncludeDeletedUserXattrs::No - The Producer attempts to Item::removeUserXattrs() before streaming. That causes a throw at item.cc:480 as the function doesn't expect any Body for deletions (by expiration in this case)   Details VBucket::handlePreExpiry is execute in the usual frontend GET path and also as part of the expiration path from backfill. In this test expirations triggers at backfill. void VBucket::handlePreExpiry(const HashTable::HashBucketLock& hbl, StoredValue& v) { .. auto result = sapi->document->pre_expiry(itm_info); // The API states only uncompressed xattr values are returned auto datatype = PROTOCOL_BINARY_DATATYPE_XATTR; if (!result.empty()) { Item new_item(v.getKey(), v.getFlags(), v.getExptime(), result.data(), result.size(), datatype, v.getCas(), v.getBySeqno(), id, v.getRevSeqno());   new_item.setNRUValue(v.getNRUValue()); new_item.setFreqCounterValue(v.getFreqCounterValue()); new_item.setDeleted(DeleteSource::TTL); ht.unlocked_updateStoredValue(hbl, v, new_item); } } } The call to pre_expiry() returns the new value. If the value is non-empty, then we execute into the if-block and update the StoredValue in the HT. But, if the post-expiry value is empty we just skip. If the value contains a body, then it will be pushed into the ActiveStream::readyQ and processed for streaming. Depending on the DCP connection configuration, that may fails as described in the summary above. Eg, payload with Body+UserXattrs caught in the test (xattrs-size 0x1b (27) + body): (lldb) memory read --count `payload.len` `payload.buf` 0x10469ba09: 00 00 00 1b 00 00 00 17 78 61 74 74 72 70 61 74 ........xattrpat 0x10469ba19: 68 00 22 78 61 74 74 72 76 61 6c 75 65 22 00 7b h."xattrvalue".{ 0x10469ba29: 22 61 62 76 22 3a 37 2e 36 2c 22 62 72 65 77 65 "abv":7.6,"brewe 0x10469ba39: 72 79 5f 69 64 22 3a 22 35 31 32 5f 62 72 65 77 ry_id":"512_brew 0x10469ba49: 69 6e 67 5f 63 6f 6d 70 61 6e 79 22 2c 22 63 61 ing_company","ca .. 0x10469bc19: 22 41 6d 65 72 69 63 61 6e 2d 53 74 79 6c 65 20 "American-Style 0x10469bc29: 42 72 6f 77 6e 20 41 6c 65 22 2c 22 74 79 70 65 Brown Ale","type 0x10469bc39: 22 3a 22 62 65 65 72 22 2c 22 75 70 64 61 74 65 ":"beer","update 0x10469bc49: 64 22 3a 22 32 30 31 30 2d 30 37 2d 32 32 54 32 d":"2010-07-22T2 0x10469bc59: 30 3a 30 30 3a 32 30 5a 22 7d 0:00:20Z"}   Note This is not a regression. The bug at VBucket::handlePreExpiry has been there for a while. In KV, we have fixed a similar issue (in a different expiry-path) in MB-29040 . For MB-29040 we also "made the Consumer resilient to invalid payloads" by pruning the body for deletion/expiration if we receive any. But, that behaviour has also hidden the existing issue at VBucket::handlePreExpiry. Until for IncludeDeletedUserXattrs we have added a bunch of extra validation at Producer and Consumer to ensure that deletions never ship Body in the payload, which is what we hit here.
            Hide
            build-team Couchbase Build Team added a comment -

            Build couchbase-server-6.6.0-7887 contains kv_engine commit dd5b577 with commit message:
            MB-40467: Expiration removes everything from the value but SysXattrs

            Show
            build-team Couchbase Build Team added a comment - Build couchbase-server-6.6.0-7887 contains kv_engine commit dd5b577 with commit message: MB-40467 : Expiration removes everything from the value but SysXattrs
            Hide
            build-team Couchbase Build Team added a comment -

            Build couchbase-server-6.6.0-7887 contains kv_engine commit 45590d3 with commit message:
            MB-40467: Don't use updateStoredValue in VBucket::handlePreExpiry

            Show
            build-team Couchbase Build Team added a comment - Build couchbase-server-6.6.0-7887 contains kv_engine commit 45590d3 with commit message: MB-40467 : Don't use updateStoredValue in VBucket::handlePreExpiry
            Hide
            paolo.cocchi Paolo Cocchi added a comment -

            Hi Charles Dixon, fix for this is in 6.6.0-7887, could you verify it please? Thank you

            Show
            paolo.cocchi Paolo Cocchi added a comment - Hi Charles Dixon , fix for this is in 6.6.0-7887, could you verify it please? Thank you
            Hide
            charles.dixon Charles Dixon added a comment -

            I've just run my test suite against 6.6.0-7887 and can confirm that it looks good.

            Show
            charles.dixon Charles Dixon added a comment - I've just run my test suite against 6.6.0-7887 and can confirm that it looks good.
            Hide
            build-team Couchbase Build Team added a comment -

            Build couchbase-server-7.0.0-2670 contains kv_engine commit dd5b577 with commit message:
            MB-40467: Expiration removes everything from the value but SysXattrs

            Show
            build-team Couchbase Build Team added a comment - Build couchbase-server-7.0.0-2670 contains kv_engine commit dd5b577 with commit message: MB-40467 : Expiration removes everything from the value but SysXattrs
            Hide
            build-team Couchbase Build Team added a comment -

            Build couchbase-server-7.0.0-2670 contains kv_engine commit 45590d3 with commit message:
            MB-40467: Don't use updateStoredValue in VBucket::handlePreExpiry

            Show
            build-team Couchbase Build Team added a comment - Build couchbase-server-7.0.0-2670 contains kv_engine commit 45590d3 with commit message: MB-40467 : Don't use updateStoredValue in VBucket::handlePreExpiry
            Hide
            ritam.sharma Ritam Sharma added a comment -

            Charles Dixon - Thank for you confirming.

            Paolo Cocchi - Would you have a test cases that can be added for functional testing ?

            Show
            ritam.sharma Ritam Sharma added a comment - Charles Dixon - Thank for you confirming. Paolo Cocchi - Would you have a test cases that can be added for functional testing ?
            Hide
            paolo.cocchi Paolo Cocchi added a comment -

            Hi Ritam Sharma,
            I see that you are happy with the verification steps of Charles Dixon's test.
            Please let me know if you need anything else.
            Thanks

            Show
            paolo.cocchi Paolo Cocchi added a comment - Hi Ritam Sharma , I see that you are happy with the verification steps of Charles Dixon 's test. Please let me know if you need anything else. Thanks
            Hide
            pvarley Patrick Varley added a comment -

            Dave Rigby and Daniel Owen I think we need to review this defect. With the changes in 6.6.0 where delete_with_meta will no long accepts a operation with a value and the fact that older versions of Couchbase Server would incorrectly stream the body of tombstones(expiry). This means that restoring a backup from a older versions of Couchbase Server to 6.6.0 will fail for the tombstones that have a body.

            I'm also wondering if this will affect XDCR too.

            Show
            pvarley Patrick Varley added a comment - Dave Rigby and Daniel Owen I think we need to review this defect. With the changes in 6.6.0 where delete_with_meta will no long accepts a operation with a value and the fact that older versions of Couchbase Server would incorrectly stream the body of tombstones(expiry). This means that restoring a backup from a older versions of Couchbase Server to 6.6.0 will fail for the tombstones that have a body. I'm also wondering if this will affect XDCR too.
            Hide
            drigby Dave Rigby added a comment - - edited

            ... and the fact that older versions of Couchbase Server would incorrectly stream the body of tombstones(expiry)

            Patrick Varley Can you elaborate on this statement? Afiak this bug (MB-40467) was only introduced in 6.6.0, and even then only KV-Engine replication streams would receive an incorrect DCP DELETE containing a value.

            Show
            drigby Dave Rigby added a comment - - edited ... and the fact that older versions of Couchbase Server would incorrectly stream the body of tombstones(expiry) Patrick Varley Can you elaborate on this statement? Afiak this bug ( MB-40467 ) was only introduced in 6.6.0, and even then only KV-Engine replication streams would receive an incorrect DCP DELETE containing a value.
            Hide
            pvarley Patrick Varley added a comment -

            Previous version of Couchbase Server, would incorrect kept the value of a document on expiry. When cbbackupmgr backs up the cluster, dcp would sends these tombstones including the value. cbbackupmgr stores this tombstone including the value. When the backup is restore to a cluster before 6.6.0, it would restore the tombstones fine. As it would accepted a delete_with_meta with a value. In 6.6.0 the validator for delete_with_meta was update and it's no longer accepts a delete_with_meta with a value. As a result the restore will fail for those tombstones.

            Show
            pvarley Patrick Varley added a comment - Previous version of Couchbase Server, would incorrect kept the value of a document on expiry. When cbbackupmgr backs up the cluster, dcp would sends these tombstones including the value. cbbackupmgr stores this tombstone including the value. When the backup is restore to a cluster before 6.6.0, it would restore the tombstones fine. As it would accepted a delete_with_meta with a value. In 6.6.0 the validator for delete_with_meta was update and it's no longer accepts a delete_with_meta with a value. As a result the restore will fail for those tombstones.
            Hide
            drigby Dave Rigby added a comment -

            Previous version of Couchbase Server, would incorrect kept the value of a document on expiry.

            What's the MB for this issue?

            Show
            drigby Dave Rigby added a comment - Previous version of Couchbase Server, would incorrect kept the value of a document on expiry. What's the MB for this issue?
            Hide
            paolo.cocchi Paolo Cocchi added a comment - - edited

            Apologies for replying only now, for some reason yesterday I had an outage of mail notifications.

            As per my old comments, this is a longstanding issue that we are seeing only in 6.6.0 because of (or thanks to I would say ) the extra validation that we have added in 6.6.0.
            But, the extra validation is only at DcpConsumer::delete (see http://review.couchbase.org/c/kv_engine/+/127038/8/engines/ep/src/dcp/consumer.cc).
            We did put in some changes at EPE::deleteWithMeta (now it accepts UserXattrs in the payload). But the validation has not changed with regard to document's Body, DelWithMeta would fail also in pre-6.6 (see http://review.couchbase.org/c/kv_engine/+/129191/8/engines/ep/src/ep_engine.cc#5477).

            Show
            paolo.cocchi Paolo Cocchi added a comment - - edited Apologies for replying only now, for some reason yesterday I had an outage of mail notifications. As per my old comments, this is a longstanding issue that we are seeing only in 6.6.0 because of (or thanks to I would say ) the extra validation that we have added in 6.6.0. But, the extra validation is only at DcpConsumer::delete (see http://review.couchbase.org/c/kv_engine/+/127038/8/engines/ep/src/dcp/consumer.cc ). We did put in some changes at EPE::deleteWithMeta (now it accepts UserXattrs in the payload). But the validation has not changed with regard to document's Body, DelWithMeta would fail also in pre-6.6 (see http://review.couchbase.org/c/kv_engine/+/129191/8/engines/ep/src/ep_engine.cc#5477 ).
            Hide
            pvarley Patrick Varley added a comment -

            Opened MB-42352 to look into this further.

            Show
            pvarley Patrick Varley added a comment - Opened MB-42352 to look into this further.
            Hide
            paolo.cocchi Paolo Cocchi added a comment - - edited

            Looked again into this for a full assessment on MB-42352.

            The code that we fixed here could lead to keeping Body and UserXattrs in the payload for an expiration.

            Summary of when we do/don't hit the issue in pre-6.6:
            SysXattrs in the paylaod -> all good (VBucket::handlePreExpiry behaves correctly)
            No SysXattrs:

            • only Body: all good (VBucket::handlePreExpiry generates invalid payloads but VBucket::processExpiredItem rectifies)
            • only UserXattrs: we hit the issue
            • Body + UserXattrs: we hit the issue

             

            Details
            In the specific function that I fixed (VBucket::handlePreExpiry), whether we hit the issue or not depends entirely on the of SysXattrs in the payload:

            • we have SysXattrs -> the function behaves correctly, Body and UserXattr removed if any
            • we don't have SysXattrs -> the function incorrectly leaves the value untouched, so Body and UserXattrs are not removed

            That said, the final behaviour of the full call-stack is that we end up with invalid payloads in expirations iff we don't have SysXattrs but we do have UserXattrs in the value.

            That is because VBucket::processExpiredItem (executed after VBucket::handlePreExpiry) reset the value depending on if datatype:xattr:

            HashTable::FindResult VBucket::fetchValidValue(
                    WantsDeleted wantsDeleted,
                    TrackReference trackReference,
                    QueueExpired queueExpired,
                    const Collections::VB::Manifest::CachingReadHandle& cHandle,
                    const ForGetReplicaOp fetchRequestedForReplicaItem) {
            ..
                        // queueDirty only allowed on active VB
                        if (queueExpired == QueueExpired::Yes &&
                            getState() == vbucket_state_active) {
                            incExpirationStat(ExpireBy::Access);
                            handlePreExpiry(res.lock, *v);    <-    IF PAYLOAD CONTAINS ONLY THE BODY, THEN WE MISS TO REMOVE THE BODY
                            VBNotifyCtx notifyCtx;
                            std::tie(std::ignore, v, notifyCtx) =
                                    processExpiredItem(res.lock, *v, cHandle);    <-    BUT, IF (!xattr) THEN reset the value, SO BODY CORRECTLY REMOVED
                            notifyNewSeqno(notifyCtx);
                            doCollectionsStats(cHandle, notifyCtx);
                        }
            ..
            }
            

            VBucket::processExpiredItem(
                    const HashTable::HashBucketLock& hbl,
                    StoredValue& v,
                    const Collections::VB::Manifest::CachingReadHandle& cHandle) {
            ..
                /* If the datatype is XATTR, mark the item as deleted
                 * but don't delete the value as system xattrs can
                 * still be queried by mobile clients even after
                 * deletion.
                 * TODO: The current implementation is inefficient
                 * but functionally correct and for performance reasons
                 * only the system xattrs need to be stored.
                 */
                value_t value = v.getValue();
                bool onlyMarkDeleted = value && mcbp::datatype::is_xattr(v.getDatatype());    <-    onlyMarkDeleted=false IF PAYLOAD CONTAINS ONLY THE BODY
                v.setRevSeqno(v.getRevSeqno() + 1);
                VBNotifyCtx notifyCtx;
                StoredValue* newSv;
                DeletionStatus delStatus;
                std::tie(newSv, delStatus, notifyCtx) =
                        softDeleteStoredValue(hbl,
                                              v,
                                              onlyMarkDeleted,
                                              VBQueueItemCtx{},
                                              v.getBySeqno(),
                                              DeleteSource::TTL);
            

            We end up in:

            HashTable::DeleteResult HashTable::unlocked_softDelete(
                    const HashBucketLock& hbl,
                    StoredValue& v,
                    bool onlyMarkDeleted,
                    DeleteSource delSource) {
            ..
                case CommittedState::Pending:
                case CommittedState::PreparedMaybeVisible:
                case CommittedState::CommittedViaMutation:
                case CommittedState::CommittedViaPrepare:
                    const auto preProps = valueStats.prologue(&v);
             
                    if (onlyMarkDeleted) {
                        v.markDeleted(delSource);
                    } else {
                        v.del(delSource);    <-    HERE
                        // As part of deleting, set committedState to CommittedViaMutation -
                        // this is necessary so when we later queue this SV into
                        // CheckpoitnManager, if if was previously CommittedViaPrepare it
                        // isn't mis-interpreted for a SyncDelete.
                        v.setCommitted(CommittedState::CommittedViaMutation);
                    }
            ..
            }
            

            And then:

            bool StoredValue::deleteImpl(DeleteSource delSource) {
                if (isDeleted() && !getValue()) {
                    // SV is already marked as deleted and has no value - no further
                    // deletion possible.
                    return false;
                }
             
                resetValue();    <-    HERE
                setDatatype(PROTOCOL_BINARY_RAW_BYTES);
                setPendingSeqno();
             
                setDeletedPriv(true);
                setDeletionSource(delSource);
                markDirty();
             
                return true;
            }
            

            Show
            paolo.cocchi Paolo Cocchi added a comment - - edited Looked again into this for a full assessment on MB-42352 . The code that we fixed here could lead to keeping Body and UserXattrs in the payload for an expiration. Summary of when we do/don't hit the issue in pre-6.6: SysXattrs in the paylaod -> all good (VBucket::handlePreExpiry behaves correctly) No SysXattrs: only Body: all good (VBucket::handlePreExpiry generates invalid payloads but VBucket::processExpiredItem rectifies) only UserXattrs: we hit the issue Body + UserXattrs: we hit the issue   Details In the specific function that I fixed (VBucket::handlePreExpiry), whether we hit the issue or not depends entirely on the of SysXattrs in the payload: we have SysXattrs -> the function behaves correctly, Body and UserXattr removed if any we don't have SysXattrs -> the function incorrectly leaves the value untouched, so Body and UserXattrs are not removed That said, the final behaviour of the full call-stack is that we end up with invalid payloads in expirations iff we don't have SysXattrs but we do have UserXattrs in the value. That is because VBucket::processExpiredItem (executed after VBucket::handlePreExpiry) reset the value depending on if datatype:xattr: HashTable::FindResult VBucket::fetchValidValue( WantsDeleted wantsDeleted, TrackReference trackReference, QueueExpired queueExpired, const Collections::VB::Manifest::CachingReadHandle& cHandle, const ForGetReplicaOp fetchRequestedForReplicaItem) { .. // queueDirty only allowed on active VB if (queueExpired == QueueExpired::Yes && getState() == vbucket_state_active) { incExpirationStat(ExpireBy::Access); handlePreExpiry(res.lock, *v); <- IF PAYLOAD CONTAINS ONLY THE BODY, THEN WE MISS TO REMOVE THE BODY VBNotifyCtx notifyCtx; std::tie(std::ignore, v, notifyCtx) = processExpiredItem(res.lock, *v, cHandle); <- BUT, IF (!xattr) THEN reset the value, SO BODY CORRECTLY REMOVED notifyNewSeqno(notifyCtx); doCollectionsStats(cHandle, notifyCtx); } .. } VBucket::processExpiredItem( const HashTable::HashBucketLock& hbl, StoredValue& v, const Collections::VB::Manifest::CachingReadHandle& cHandle) { .. /* If the datatype is XATTR, mark the item as deleted * but don't delete the value as system xattrs can * still be queried by mobile clients even after * deletion. * TODO: The current implementation is inefficient * but functionally correct and for performance reasons * only the system xattrs need to be stored. */ value_t value = v.getValue(); bool onlyMarkDeleted = value && mcbp::datatype::is_xattr(v.getDatatype()); <- onlyMarkDeleted=false IF PAYLOAD CONTAINS ONLY THE BODY v.setRevSeqno(v.getRevSeqno() + 1); VBNotifyCtx notifyCtx; StoredValue* newSv; DeletionStatus delStatus; std::tie(newSv, delStatus, notifyCtx) = softDeleteStoredValue(hbl, v, onlyMarkDeleted, VBQueueItemCtx{}, v.getBySeqno(), DeleteSource::TTL); We end up in: HashTable::DeleteResult HashTable::unlocked_softDelete( const HashBucketLock& hbl, StoredValue& v, bool onlyMarkDeleted, DeleteSource delSource) { .. case CommittedState::Pending: case CommittedState::PreparedMaybeVisible: case CommittedState::CommittedViaMutation: case CommittedState::CommittedViaPrepare: const auto preProps = valueStats.prologue(&v);   if (onlyMarkDeleted) { v.markDeleted(delSource); } else { v.del(delSource); <- HERE // As part of deleting, set committedState to CommittedViaMutation - // this is necessary so when we later queue this SV into // CheckpoitnManager, if if was previously CommittedViaPrepare it // isn't mis-interpreted for a SyncDelete. v.setCommitted(CommittedState::CommittedViaMutation); } .. } And then: bool StoredValue::deleteImpl(DeleteSource delSource) { if (isDeleted() && !getValue()) { // SV is already marked as deleted and has no value - no further // deletion possible. return false; }   resetValue(); <- HERE setDatatype(PROTOCOL_BINARY_RAW_BYTES); setPendingSeqno();   setDeletedPriv(true); setDeletionSource(delSource); markDirty();   return true; }

              People

              Assignee:
              charles.dixon Charles Dixon
              Reporter:
              charles.dixon Charles Dixon
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Due:
                Created:
                Updated:
                Resolved:

                  Gerrit Reviews

                  There are no open Gerrit changes

                    PagerDuty