So you update the queue in pre-op, but what if the operation fails in post-op? Is the "invalid" update still sent to the client?

I think it can be a docstring.

Looks like debugging leftovers... Also, it makes sense to set a breakpoint like this: import pdb; pdb.set_trace(). So if you import and set_trace in the same place, it would be easier to clean up afterward.

and here

and here

I don't fully understand how you use the new connection...
inst.clone() returns a new connection.
So now you have self.conn - one connection, and self.inst - another connection.

I can't understand how you use this line... pytest, as far as I know, don't use it. And I don't see if you call the test somewhere outside of the pytest...

Probably, you plan to add it later but I'll leave a comment here as a reminder to myself to check it when the time comes. :)

I see a couple of compile warnings also:

./config.h:367: warning: "LINUX" redefined
  367 | #define LINUX 1
      |
In file included from /usr/include/nspr4/prtypes.h:26,
             from /usr/include/nspr4/prlog.h:9,
             from ldap/servers/plugins/sync/sync_persist.c:9:
/usr/include/nspr4/prcpucfg.h:19: note: this is the location of the previous definition
   19 | #define LINUX
      |
ldap/servers/plugins/sync/sync_persist.c: In function ‘sync_update_persist_betxn_pre_op’:
ldap/servers/plugins/sync/sync_persist.c:51:9: warning: ‘return’ with no value, in function returning non-void
   51 |         return;
      |         ^~~~~~
ldap/servers/plugins/sync/sync_persist.c:43:1: note: declared here
   43 | sync_update_persist_betxn_pre_op(Slapi_PBlock *pb)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ldap/servers/plugins/sync/sync_persist.c: In function ‘sync_update_persist_op’:
ldap/servers/plugins/sync/sync_persist.c:91:9: warning: ‘return’ with no value, in function returning non-void
   91 |         return;
      |         ^~~~~~
ldap/servers/plugins/sync/sync_persist.c:84:1: note: declared here
   84 | sync_update_persist_op(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *eprev, ber_int_t op_tag, char *label)
      | ^~~~~~~~~~~~~~~~~~~~~~
ldap/servers/plugins/sync/sync_persist.c:95:9: warning: ‘return’ with no value, in function returning non-void
   95 |         return;
      |         ^~~~~~
ldap/servers/plugins/sync/sync_persist.c:84:1: note: declared here
   84 | sync_update_persist_op(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *eprev, ber_int_t op_tag, char *label)

I forgot to mention that the indentation is wrong for most the logging functions (slapi_log_err) :-)

@mreynolds , @spichugi thanks for your reviews. The question about internal updates failures is a difficult one and the fix is incomplete.
There is a principle that on successful internal operation, pblock.SLAPI_ENTRY_POST_OP is set. It looks it is enforced. But I am not sure of the opposite (pblock.SLAPI_ENTRY_POST_OP set means the operation was successful). I think a safety approach would be to test in the sync_repl POST OP that SLAPI_PLUGIN_OPRETURN and SLAPI_RESULT_CODE are successful.
In addition in case of failure, the cleanup of the pending operation is not done and I should revisit this part and... write an additional testcase :(

A long time after here is the revisited PR: adding pending list for nested updates, handling of failures and preventing concurrent updates disorder

Next is to update the design to reflect those changes

Comment indentation is off :-D

Should have a newline here

More indentation issues around of the logging functions (not just here but in a lot of other places)

Is this ready to be reviewed yet? I just noticed a lot of "#if 0" code blocks, and some very minor indentation issues :-)

Can this comment be removed?

It would also be nice to to remove "unsigned long" and use uint32_t, but not necessary. And don't forget all the logging function indentation issues! :-)

Besides that, if this patch passes ASAN testing and there no compiler warnings then you get my ack!

@mreynolds, I finally update the design (https://www.port389.org/docs/389ds/design/content-synchronization-plugin.html#queue-and-pending-list) and updated (indentation, remaining dead code, leaks and compiler warning). Please have a look

@tbordaz I'm going to read your design shortly. It may be worth your time to also review #51260 given that I have just found a potential issues where ther cookie and changelog being sent could accidently corrupt client state, so perhaps this change may impact your upcoming change with the queue.

There is a suites/sync repl section, this test should be there rather than plugin/acceptance I think.

What does this sleep do?

Just to be clear, this is to remove a memory leak in shutdown yes?

Could this be an enum instead of defines?

I think most of this is pretty minor, but most of the patch looks pretty reasonable. Could you comment a bit in the code about the design and how the thread private op works a bit more to help future developers understand your thoughts and the interactions that are occuring here? Thanks!

Thanks @firstyear for the review. I updated the PR (moving test, adding comments and doing cleanup). The sleep in the sync_repl thread was a left over of some tests to reproduce invalid order !! thanks for having spot this :)

These seem like left overs that you don't need to add here :)

{ and } on the if here

Okay, beside the last python comment I can see what this does and it looks reasonable to me. Thanks @tbordaz :)

I think reading it it won't affect/impact #51260 since they are seperate issues (but we may find also that #51260 is contributing to this issue too in a subtle way .... )

So I think ack from me once the last two minor comments are addressed.

If I remove any of those 3 lines, the tests are failing :(

Oh in that case, ignore that about the 3 lines in the acceptance tests. If you can add the brace in sync_init.c I think this is acked you can then merge :)

Thanks @mreynolds , @spichugi and @firstyear for these reviews

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4267

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.