Description

Apparently if a document has been in a conflict, they will reject requests to add an attachment with a conflict error.

I've tracked this down to couch_db_updater.erl line 501, but I'm not too familiar with this part of the code so I figured I'd fill out a ticket in case anyone else can go through this more quickly than me.

The problem is that when deleting the conflicting revision 2-2ee767305024673cfb3f5af037cd2729, just a new revision 3-89c964c5556795ee0fc45fd5213013b4 (with "_deleted":true) is created. The old revision 2-871d61d84063e8fc02d64acbcdaad8ba (that's not been deleted) is now in a kind of "deleted conflict" state with it. That is, updating revision 2-871d61d84063e8fc02d64acbcdaad8ba would create a new revision that's in conflict with the deleted one.

Klaus Trainer
added a comment - 16/Oct/10 20:40 I've figured out what the actual problem here is.
========================================================================
Deleting conflicts on database two:
========================================================================
GET - /two/test?conflicts=true - "some plain text"
200 -
{"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3,"_conflicts":["2-2ee767305024673cfb3f5af037cd2729"]}
DELETE - /two/test?rev=2-2ee767305024673cfb3f5af037cd2729 -
200 -
{"ok":true,"id":"test","rev":"3-89c964c5556795ee0fc45fd5213013b4"}
========================================================================
Attaching to document without conflicts on database two:
========================================================================
GET - /two/test - "some plain text"
200 -
{"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3}
PUT - /two/test/some_attachment?rev=2-871d61d84063e8fc02d64acbcdaad8ba - some plain text
409 -
{"error":"conflict","reason":"Document update conflict."}
The problem is that when deleting the conflicting revision 2-2ee767305024673cfb3f5af037cd2729, just a new revision 3-89c964c5556795ee0fc45fd5213013b4 (with "_deleted":true) is created. The old revision 2-871d61d84063e8fc02d64acbcdaad8ba (that's not been deleted) is now in a kind of "deleted conflict" state with it. That is, updating revision 2-871d61d84063e8fc02d64acbcdaad8ba would create a new revision that's in conflict with the deleted one.
You can check this e.g. with
curl -X GET http://127.0.0.1:5984/two/test?deleted_conflicts=true
{"_id":"test","_rev":"2-871d61d84063e8fc02d64acbcdaad8ba","foo":3,"_deleted_conflicts":["3-89c964c5556795ee0fc45fd5213013b4"]}
The actual problem here is that when updating a document with an attachment, deleted conflicts are treated like normal conflicts. Supposably there's an easy solution to that issue.

After some debugging, I found out that the difference between an ordinary update and an update with attachment was that in the former case the doc only contained the most current rev in #doc.revs, whereas in the latter case, all revs were contained in #doc.revs.

This is because in the latter case case, the document is retrieved with couch_db:open_doc_revs/4, whereas in the former case, the doc record is constructed using couch_httpd_db:couch_doc_from_req/3.

If all revs are included in #doc.revs (instead of only the latest one), the merge* functions in couch_key_tree will come to the conclusion that there's a conflict. As I didn't see anything wrong with the merge* functions, I assume that the flaw lies in including more than the latest rev in #doc.revs.

Attached is a patch that fixes the issue and provides corresponding tests as well. Testing with make check and the Futon tests suggests that everything is fine with it.

Klaus Trainer
added a comment - 17/Oct/10 18:08 After some debugging, I found out that the difference between an ordinary update and an update with attachment was that in the former case the doc only contained the most current rev in #doc.revs, whereas in the latter case, all revs were contained in #doc.revs.
This is because in the latter case case, the document is retrieved with couch_db:open_doc_revs/4, whereas in the former case, the doc record is constructed using couch_httpd_db:couch_doc_from_req/3.
If all revs are included in #doc.revs (instead of only the latest one), the merge* functions in couch_key_tree will come to the conclusion that there's a conflict. As I didn't see anything wrong with the merge* functions, I assume that the flaw lies in including more than the latest rev in #doc.revs.
Attached is a patch that fixes the issue and provides corresponding tests as well. Testing with make check and the Futon tests suggests that everything is fine with it.

Well, yes. But that alone doesn't fix the issue. For the test Paul gave, open_doc/2 will give you the exact same value for the revs value of the #doc record.

Prunning the revs tree (removing all parent revisions) doesn't look like to me a clean/correct way of doing it. I think the fix should be somewhere in the prep_and_validate_doc_updates functions or in couch_db_updater merge_rev_trees (or couch_key_tree)

Filipe Manana
added a comment - 25/Oct/10 17:42 Well, yes. But that alone doesn't fix the issue. For the test Paul gave, open_doc/2 will give you the exact same value for the revs value of the #doc record.
Prunning the revs tree (removing all parent revisions) doesn't look like to me a clean/correct way of doing it. I think the fix should be somewhere in the prep_and_validate_doc_updates functions or in couch_db_updater merge_rev_trees (or couch_key_tree)

I tend to agree with Filipe here. I've encountered the need to prune the revisions of a #doc{} before updating in the past, and it's always bothered me. It shouldn't be necessary. I also can't figure out when it is and is not needed.

Adam Kocoloski
added a comment - 01/Dec/10 18:11 I tend to agree with Filipe here. I've encountered the need to prune the revisions of a #doc{} before updating in the past, and it's always bothered me. It shouldn't be necessary. I also can't figure out when it is and is not needed.

My "point" on open_doc_revs/4 and open_doc/2 was pointless, as open_doc/2 isn't used for neither doc nor doc attachment updates.

The point that I actually wanted to make, however, is that couch_db:update_docs/4 gets a #doc{} with only the latest rev in #doc.revs when there's a doc update without attachment, but it gets a #doc{} with the full rev list in #doc.revs when there's an update with attachment.

To conclude that, doc updates with attachments are (AFAIK) the only situation where pruning the older revisions would be necessary. My patch would basically solve the problem, but I agree that it's better to not require that "#doc.revs must only include the latest rev".

I can tell for sure that the problem is somewhere behind the couch_key_tree:merge function. It returns {_NewTree, conflict} to the couch_db_updater:merge_rev_trees function.

I terminated my debugging session in couch_key_tree:merge_simple/2 (line 108): the second element of the tuple

{[ATree | MTree], true}

represents the conflicts flag. The code here is a bit tricky; I can't tell what exactly I can change to eliminate that spurious conflict without breaking anything. Maybe we should grab the guy who wrote that piece of code in order to solve this issue

Klaus Trainer
added a comment - 02/Dec/10 00:38 Thanks Adam for bringing this issue back to my attention.
My "point" on open_doc_revs/4 and open_doc/2 was pointless, as open_doc/2 isn't used for neither doc nor doc attachment updates.
The point that I actually wanted to make, however, is that couch_db:update_docs/4 gets a #doc{} with only the latest rev in #doc.revs when there's a doc update without attachment, but it gets a #doc{} with the full rev list in #doc.revs when there's an update with attachment.
To conclude that, doc updates with attachments are (AFAIK) the only situation where pruning the older revisions would be necessary. My patch would basically solve the problem, but I agree that it's better to not require that "#doc.revs must only include the latest rev".
Also, I've tried to identify what's wrong in the conflict resolution logic, i.e., why there's a spurious conflict when the revs list > 1.
Here are the results that I've gained so far:
I can tell for sure that the problem is somewhere behind the couch_key_tree:merge function. It returns {_NewTree, conflict} to the couch_db_updater:merge_rev_trees function.
I terminated my debugging session in couch_key_tree:merge_simple/2 (line 108): the second element of the tuple
{[ATree | MTree], true}
represents the conflicts flag. The code here is a bit tricky; I can't tell what exactly I can change to eliminate that spurious conflict without breaking anything. Maybe we should grab the guy who wrote that piece of code in order to solve this issue

First of all, thank you, Adam! Your rewrite of couch_key_tree made it way more readable.

I think that I got to the point where I understand how the merging logic works.

To me, the merging logic in couch_key_tree looks absolutely correct. Given the fact that I now understand what's going on there, I don't see any reasonable change we can make in couch_key_tree in order to solve this issue. We rather have to make a change somewhere else.

To anticipate my conclusion: Pruning the revs tree of a #doc{} is necessary for updates that cannot introduce new conflicts.

In the following, I'd like to explain why. I've attached a new patch, which fixes the issue in a more reasonable way.

couch_key_tree:merge/3 returns an atom (`conflicts | no_conflicts`) that tells us whether an update creates a conflict or not. The returned atom is ignored as long as a certain flag `MergeConflicts` is set. More precisely, conflict checking is only active for normal interactive edits, but not for replication or bulk insertions with `all_or_nothing: true`.

Currently, the only case where an interactive edit is done with the #doc{} containing the full revision history (instead of just the current and the new rev) is when there's an attachment (see couch_httpd_db:db_attachment_req/4).

couch_key_tree:merge/3 is called with a list of paths and a path, whereupon the latter is the return value of couch_doc:to_path/1. Looking at couch_doc:to_path/1, one can see that the start sequence number that is computed there depends on the length of the revision history. Also, the revisions list `RevIds` is reversed. That means that for interactive edits the computed paths may be different (namely when length(RevIds) > 2), depending on whether it's an update with an attachment or not.

To summarize my above explanation:

1) The result of the conflict detection algorithm is only relevant for interactive edits with `MergeConflicts =:= false`. Otherwise, the result may be not correct and it's ignored anyway.

2) The correctness of the conflict detections in couch_key_tree (namely merge_at/2 and merge_simple/2) relies on the fact that no more than two revision ids, namely the new and the current one (if available), are contained in the revs tree.

Although my previous patch was somehow correct, and pruning the revs tree seems to be the way to go indeed, Filipe is right when saying that the fix should be applied in couch_db_updater:merge_rev_trees/7 (as opposed to in couch_httpd_db), which is what I've done now.

Klaus Trainer
added a comment - 07/Jan/11 17:31 First of all, thank you, Adam! Your rewrite of couch_key_tree made it way more readable.
I think that I got to the point where I understand how the merging logic works.
To me, the merging logic in couch_key_tree looks absolutely correct. Given the fact that I now understand what's going on there, I don't see any reasonable change we can make in couch_key_tree in order to solve this issue. We rather have to make a change somewhere else.
To anticipate my conclusion: Pruning the revs tree of a #doc{} is necessary for updates that cannot introduce new conflicts.
In the following, I'd like to explain why. I've attached a new patch, which fixes the issue in a more reasonable way.
couch_key_tree:merge/3 returns an atom (`conflicts | no_conflicts`) that tells us whether an update creates a conflict or not. The returned atom is ignored as long as a certain flag `MergeConflicts` is set. More precisely, conflict checking is only active for normal interactive edits, but not for replication or bulk insertions with `all_or_nothing: true`.
Currently, the only case where an interactive edit is done with the #doc{} containing the full revision history (instead of just the current and the new rev) is when there's an attachment (see couch_httpd_db:db_attachment_req/4).
couch_key_tree:merge/3 is called with a list of paths and a path, whereupon the latter is the return value of couch_doc:to_path/1. Looking at couch_doc:to_path/1, one can see that the start sequence number that is computed there depends on the length of the revision history. Also, the revisions list `RevIds` is reversed. That means that for interactive edits the computed paths may be different (namely when length(RevIds) > 2), depending on whether it's an update with an attachment or not.
To summarize my above explanation:
1) The result of the conflict detection algorithm is only relevant for interactive edits with `MergeConflicts =:= false`. Otherwise, the result may be not correct and it's ignored anyway.
2) The correctness of the conflict detections in couch_key_tree (namely merge_at/2 and merge_simple/2) relies on the fact that no more than two revision ids, namely the new and the current one (if available), are contained in the revs tree.
Although my previous patch was somehow correct, and pruning the revs tree seems to be the way to go indeed, Filipe is right when saying that the fix should be applied in couch_db_updater:merge_rev_trees/7 (as opposed to in couch_httpd_db), which is what I've done now.

Hi Klaus, thanks. I'm not sure I followed your entire explanation, though. I think your point #2 is the crux of the matter. Can you add an etap test for couch_key_tree showing a spurious conflict when the incoming path has more than 2 entries?

I've definitely seen these spurious conflicts myself, and I'm excited that you're drilling down and trying to solve it.

Adam Kocoloski
added a comment - 07/Jan/11 18:17 Hi Klaus, thanks. I'm not sure I followed your entire explanation, though. I think your point #2 is the crux of the matter. Can you add an etap test for couch_key_tree showing a spurious conflict when the incoming path has more than 2 entries?
I've definitely seen these spurious conflicts myself, and I'm excited that you're drilling down and trying to solve it.

Paul Joseph Davis
added a comment - 07/Jan/11 19:50 Can someone remind me why we have the full revision history only for attachments in the first place? Unless I'm crazy, that seems like its the actual bug.

Hi Paul, I don't think there's any real reason why the full history is present for attachments. But I think a sane API should allow me to do the following as many times as I want in a loop without generating a conflict (if I'm the only writer):

Adam Kocoloski
added a comment - 07/Jan/11 20:55 - edited Hi Paul, I don't think there's any real reason why the full history is present for attachments. But I think a sane API should allow me to do the following as many times as I want in a loop without generating a conflict (if I'm the only writer):
{ok, Doc}
= couch_db:open_doc(Db, Id, []),
{ok, _NewRev}
= couch_db:update_doc(Db, Doc#doc{body = {[
{<<"random">>, random:uniform()}
]}}, [])

Does that not work either? I can never keep things straight when we have full paths or not. If that is just as borked as the attachment stuff then I'd agree that we should fix it. Though I'm still wondering if this isn't two separate bugs.

Paul Joseph Davis
added a comment - 07/Jan/11 21:01 Does that not work either? I can never keep things straight when we have full paths or not. If that is just as borked as the attachment stuff then I'd agree that we should fix it. Though I'm still wondering if this isn't two separate bugs.

I don't have a failing test, but the working assumption is that including the full #doc.revs list in an update_docs call generates spurious conflicts. Attachments are tangential, it's just the only place that we include the full #doc.revs in handling an HTTP request.

Adam Kocoloski
added a comment - 07/Jan/11 21:36 I don't have a failing test, but the working assumption is that including the full #doc.revs list in an update_docs call generates spurious conflicts. Attachments are tangential, it's just the only place that we include the full #doc.revs in handling an HTTP request.

Adam Kocoloski wrote:
> Can you add an etap test for couch_key_tree showing a spurious conflict when the incoming path has more than 2 entries?

Yeah, I'll do that. The tests in the attached patch will detect spurious conflicts. However it's probably not the worst idea to write an additional etap test for the couch_key_tree module only.

Paul Joseph Davis wrote:
> Can someone remind me why we have the full revision history only for attachments in the first place? Unless I'm crazy, that seems like its the actual bug.

As opposed to Adam's speculation, there is an actual reason for that.

When updating a document body, the new document body is supplied by the client. The new #doc{} that is passed to couch_doc:to_path/1 consists of the new document body supplied by the client and the revs tree containing the current, if available, and the new rev. Therefore, the resulting path contains at most two revs.

In contrast, when updating with an attachment, the new document body is equal to the current one, and therefore isn't supplied by the client. That's the reason why couch_db:open_doc_revs/4 is called in couch_httpd_db:db_attachment_req/4. As the #doc{} that is returned by the function call contains the full revs tree, the #doc{} that's passed to couch_doc:to_path/1 does as well.

Let me propose another (probably nicer) solution.

We could introduce another function couch_key_tree:merge/4. The fourth argument would be a flag telling whether merge conflicts are allowed or not.

merge/4 would call merge/3. When calling merge/4, the only difference from directly calling merge/3 would be that if the `MergeConflicts` flag is false, the revs tree is pruned.

Klaus Trainer
added a comment - 08/Jan/11 23:35 Adam Kocoloski wrote:
> Can you add an etap test for couch_key_tree showing a spurious conflict when the incoming path has more than 2 entries?
Yeah, I'll do that. The tests in the attached patch will detect spurious conflicts. However it's probably not the worst idea to write an additional etap test for the couch_key_tree module only.
Paul Joseph Davis wrote:
> Can someone remind me why we have the full revision history only for attachments in the first place? Unless I'm crazy, that seems like its the actual bug.
As opposed to Adam's speculation, there is an actual reason for that.
When updating a document body, the new document body is supplied by the client. The new #doc{} that is passed to couch_doc:to_path/1 consists of the new document body supplied by the client and the revs tree containing the current, if available, and the new rev. Therefore, the resulting path contains at most two revs.
In contrast, when updating with an attachment, the new document body is equal to the current one, and therefore isn't supplied by the client. That's the reason why couch_db:open_doc_revs/4 is called in couch_httpd_db:db_attachment_req/4. As the #doc{} that is returned by the function call contains the full revs tree, the #doc{} that's passed to couch_doc:to_path/1 does as well.
Let me propose another (probably nicer) solution.
We could introduce another function couch_key_tree:merge/4. The fourth argument would be a flag telling whether merge conflicts are allowed or not.
merge/4 would call merge/3. When calling merge/4, the only difference from directly calling merge/3 would be that if the `MergeConflicts` flag is false, the revs tree is pruned.
What do you think about that?

Simply pruning #doc.revs tree doesn't seem to work for updates with attachments.

When having multiple concurrent writers trying to attach a file to the same version, it's possible that more than one response with status code 201 and the same update sequence number is returned, even though only the first update succeeded for for real

This is reproducible. I'm now trying to understand why it occurs for updates with attachment, but not for updates of document bodies (where #doc.revs only consists of the current and new rev as well). Maybe I can find a fix then.

Klaus Trainer
added a comment - 09/Jan/11 17:21 I just discovered that this issue is a bit trickier...
Simply pruning #doc.revs tree doesn't seem to work for updates with attachments.
When having multiple concurrent writers trying to attach a file to the same version, it's possible that more than one response with status code 201 and the same update sequence number is returned, even though only the first update succeeded for for real
Here an excerpt from my logs.
CouchDB log:
[info] [<0.378.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/134095825 201
[info] [<0.378.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/17641740 201
[info] [<0.377.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/132734222 201
[info] [<0.376.0>] 127.0.0.1 - - 'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/114219547 201
Client log:
'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/134095825 "85-31c1a9fb50b253aa68ac632f6e1a7626"
'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/17641740 "86-1e38b77c2d0f1160b12cb8f48d2506bc"
'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/132734222 "86-f907a3b56661163f9fa5aeb732a282f2"
'PUT' /couch_planet_test/http%3a%2f%2ffeedproxy.google.com%2f%7er%2freadwriteweb%2f%7e3%2fdieZ1tlfbVo%2fhow-to-deploy-ipv6-securely.php/114219547 "86-a81f66b089296771b23aa6b04f5abb28"
This is reproducible. I'm now trying to understand why it occurs for updates with attachment, but not for updates of document bodies (where #doc.revs only consists of the current and new rev as well). Maybe I can find a fix then.

I think we found the crux of the matter. As you suspected, the merge_simple clause involving A < B reports conflicts even when no new conflicts are present and the merge is successful. Adding an attachment to an existing conflict situation just exposes the issue. Kocolosk found a nice simple solution[1] that runs all the tests including the new etap we added. See what you think and if you agree we can turn it into a patch. I'm going to add some more tests there while I'm at it.

Bob Dionne
added a comment - 13/Jan/11 20:24 Klaus et. al.,
I think we found the crux of the matter. As you suspected, the merge_simple clause involving A < B reports conflicts even when no new conflicts are present and the merge is successful. Adding an attachment to an existing conflict situation just exposes the issue. Kocolosk found a nice simple solution [1] that runs all the tests including the new etap we added. See what you think and if you agree we can turn it into a patch. I'm going to add some more tests there while I'm at it.
Cheers,
Bob
[1] https://github.com/bdionne/couchdb/commit/5e424d118615ca14ee5

Klaus Trainer
added a comment - 14/Jan/11 22:56 Bob, Adam, this one is an awesome solution! I've scrutinized it and haven't found any flaw in it.
What I like most about it, is that with it, couch_key_tree:merge/3 always returns a correct `conflicts | no_conflicts` flag.
If you can identify further test cases, we should surely include them. Anyway, I'd be fine with the bugfix and tests in your couchdb-902 branch.
Please tell me when you've prepared a patch. I'll then have a look at it once again.

However, I'm a little uneasy making a change to a core module like that just before a release. I'd prefer it if the change cooked in trunk for a little while. That's why I'm proposing the following:

a) we commit Bob's change to couch_key_tree to trunk

b) we patch 1.0.x and 1.1.x at the HTTP layer by pruning the revision list during an attachment update

We aren't in any danger of breaking something that isn't already broken, but we still fix the user-facing issue of being unable recover attachments from a conflict state. CouchDB 1.2 will have an improved Erlang API that never returns a spurious 'conflict' result from a merge when the full revision lists are used.

Anyone opposed? I'd like to release 1.0.2 ASAP after the HTTP fix is committed.

Adam Kocoloski
added a comment - 17/Jan/11 19:13 Thanks, Klaus!
However, I'm a little uneasy making a change to a core module like that just before a release. I'd prefer it if the change cooked in trunk for a little while. That's why I'm proposing the following:
a) we commit Bob's change to couch_key_tree to trunk
b) we patch 1.0.x and 1.1.x at the HTTP layer by pruning the revision list during an attachment update
We aren't in any danger of breaking something that isn't already broken, but we still fix the user-facing issue of being unable recover attachments from a conflict state. CouchDB 1.2 will have an improved Erlang API that never returns a spurious 'conflict' result from a merge when the full revision lists are used.
Anyone opposed? I'd like to release 1.0.2 ASAP after the HTTP fix is committed.

I tried the python test cases from Paul / Bob at at https://github.com/bdionne/couchdb/commit/1a276a ... cases 1 and 2 fail with stock 1.0.x but pass with the above patch. Case 3 always passes, but that seems to be expected (it doesn't do anything with attachments).

I'd be grateful if someone could take a look at those and see what the problem is with Klaus' attachment_conflicts.js.

Adam Kocoloski
added a comment - 17/Jan/11 20:01 I tried the python test cases from Paul / Bob at at https://github.com/bdionne/couchdb/commit/1a276a ... cases 1 and 2 fail with stock 1.0.x but pass with the above patch. Case 3 always passes, but that seems to be expected (it doesn't do anything with attachments).
I'd be grateful if someone could take a look at those and see what the problem is with Klaus' attachment_conflicts.js.

Adam Kocoloski
added a comment - 17/Jan/11 22:30 Bob, excellent. I can confirm that the test fails on 1.0.x and passes with the patch. So, here is my proposed workaround for 1.0.x:
https://github.com/kocolosk/couchdb/commit/4663fa
I'll commit in a couple of hours if there are no objections.

Klaus Trainer
added a comment - 24/Jan/11 22:17 Yeah, we should get that into trunk soon.
The only question that I still see as open is about testing. In the corresponding fix for 1.0.2 (see https://github.com/apache/couchdb/commit/8432c0e8f31a683b13419dc591edca49933d1f81 ), we have accordant futon tests in the (new) file `attachment_conflicts.js`. Also, there's Bob's new etap test (see https://github.com/bdionne/couchdb/commit/1a276a#diff-7 ) for couch_key_tree.
Shouldn't we include those tests in the fix?