You said this was a regression, was this previously add_prerequisite and your PR changed to add_update?

Ok I think I understand the issue. The original code added the dhcid record to prereq, it was supposed to be update. Your first fix changed both to update but only dhcid record is meant to be in that section

I think so, yes. I've read the RFC a couple times and I think this is the way it's should be. Originally I had this as an add_answer which is the same thing as far as the bytes over the wire are concerned but this is more clear from a spec point of view.

For what it's worth I swear the previous version also worked but this maybe more correct.

afaik the prereq section is effectively the condition that's required and the update is the action that's going to happen.

It's possible it worked with both in that case. Not having any conditions on the update action. But I think the name here is supposed to be the condition.

It's possible the same is the case for dhcid, it could go in either. In one case it means only update if dhcid matches (same device is renewing). In the other case it means add the dhcid along with the a/aaaa record, so it's like making a new entry for the client?

But it's been a while since I looked at the RFC

I think the same change needs to be made in update_present to change the add_update back to add_prerequisite?
we've got this in update_present

    let mut prerequisite = Record::update0(name.clone(), 0, RecordType::ANY);
    // use ANY to check only update if this name is present
    prerequisite.set_dns_class(DNSClass::ANY);
    message.add_update(prerequisite);

I don't think I've read the RFC parts that deal with the dhcpid resolution. What do you think about adding some more options to the configuration dns updates? I can see having forward/reverse being a selection as well as the dhcpid inclusion or not being something that's configurable.

I think the same change needs to be made in update_present to change the add_update back to add_prerequisite? we've got this in update_present

    let mut prerequisite = Record::update0(name.clone(), 0, RecordType::ANY);
    // use ANY to check only update if this name is present
    prerequisite.set_dns_class(DNSClass::ANY);
    message.add_update(prerequisite);

I think that is likely. I couldn't explicitly reproduce the behavior in my test setup. I did the delete/create behavior but I didn't wait for the refresh cycle to happen to see what I think would trigger this behavior? I could try that next and create another PR for that.

https://datatracker.ietf.org/doc/html/rfc4703#section-5.3.2
I'm just checking this part out. I think the correct flow for the conflict resolution (update_present) is this:

// Prerequisite 1: name exists
let mut prerequisite = Record::update0(name.clone(), 0, RecordType::ANY);
prerequisite.set_dns_class(DNSClass::ANY);
message.add_prerequisite(prerequisite);  // wrong
...
// Prerequisite 2: DHCID matches
let dhcid_record = Record::from_rdata(...);
message.add_prerequisite(dhcid_record);  // wrong
...
// Update: add A record
let a_record = Record::from_rdata(name, ttl, A(leased).into_rdata());
message.add_update(a_record);

If you're interested in pursuing this, that would be great. I can revert update_present to it's previous state and then the question would be if that dhcid belongs in prereq or update in the case of a conflict. (can be done in another PR)

Consider removing the commented-out code. If this reference code is valuable for future debugging or documentation, it should either be moved to a comment explaining the test's origin, or removed entirely to keep the codebase clean.

Is there value in keeping this and doing an assert or just deleting the comment?

I think it's a useful reference but that is loosely held. No issue to delete it.