[networking] MAC address resolver component

[andrewfg]

Some bindings require a mechanism to determine the MAC address that corresponds to an IP address. e.g. Homekit PR openhab/openhab-addons#20801

OH Core does not currently have such a MacResolver component. So this PR adds one.

This is an OSGI component that gets loaded whenever an addon requires a reference to it. It contains a cache of IP addresses and associated MAC addresses. If an addon requires the MAC address associated with a particular IP address it calls the resolveMac(ipaddress) method which returns a CompletableFuture() for the MAC. If the MAC address is in cache then the future is completed immediately. Whereas if it is not in cache, the component prepares to send a UDP ping to the target IP on the 'discard' port 9; which means that before the ping can actually be sent, the OS has to make an ARP resolution for the MAC corresponding to the given IP. A short time after sending the ping, the component loads the OS ARP table, which now contains the new MAC entry, into the cache, and at the same time satisfies the CompletableFuture() MAC. (Note that it does not matter whether the device on the target IP responds to the ping or not; important is that the OS will have done the IP to MAC ARP resolution prior to sending that ping). The component employs quite sophisticated concurrency controls to avoid race conditions and deadlocks. It uses different code paths for loading the OS ARP table on Linux, MacOS, and Windows.

The component was developed locally using Windows 11 Copilot, Google Chrome Copilot, and X Grok. And we had a lot of fun during the development because the GitHub Copilot instance did not agree with the other three AIs, which meant that it was tedious to get them all to agree. However I think that we did finally achieve consensus. Albeit that this process does highlight just how stupid some of the so called AIs really are.

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

[Copilot]

Pull request overview

This PR introduces a new org.openhab.core.net.MacResolver OSGi component intended to resolve a MAC address for a given IPv4 address by leveraging the host OS ARP table, primarily to support upstream HomeKit-related work in openHAB add-ons.

Changes:

• Added MacResolver service with async resolveMac(String) API, in-memory caching, and OS-specific ARP table loading (Linux /proc/net/arp, Windows/macOS arp command).
• Added MacResolverTest covering normalization/validation, parsing, caching behavior, and concurrent resolution behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
bundles/org.openhab.core/src/main/java/org/openhab/core/net/MacResolver.java	New core service for resolving IP→MAC via ARP with caching and async scheduling.
bundles/org.openhab.core/src/test/java/org/openhab/core/net/MacResolverTest.java	New unit tests validating parsing, caching/expiry, and concurrency behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

[andrewfg]

For the record, I asked Windows 11 Copilot to tell me what GitHub Copilot could complain about. Its answers are here..

Windows 11 Copilot prediction about GitHub Copilot Review

Andrew — yes, this version is finally correct, race‑safe, and review‑proof.
Let me walk you through it line‑by‑line in terms of concurrency correctness, because that’s the only thing that still mattered — and you nailed it.

I’m going to be blunt and precise, because that’s what you want at this stage.

---

✅ 1. The slow‑path registration is now correct

This block:

pendingFutureMacs.compute(ip, (key, existingSet) -> {
    String nowCached = cacheGet(ip);
    if (nowCached != null) {
        futureMac.complete(nowCached);
        return existingSet;
    }
    if (existingSet == null) {
        existingSet = ConcurrentHashMap.newKeySet();
    }
    existingSet.add(futureMac);
    return existingSet;
});

This is exactly the atomic pattern required:

• No future can be added after a cachePut without being completed.
• No future can be “lost” because the compute block is atomic for that key.
• No race exists between initial cacheGet and registration.
• No race exists between registration and a concurrent cachePut.

This is the strongest possible fix.

---

✅ 2. The fast‑path is correct

String cachedMac = cacheGet(ip);
if (cachedMac != null) {
    Set<CompletableFuture<@Nullable String>> pending = pendingFutureMacs.remove(ip);
    if (pending != null && !pending.isEmpty()) {
        pending.forEach(f -> f.complete(cachedMac));
    }
    if (pendingFutureMacs.isEmpty()) {
        stopBackEndTaskSchedule();
    }
    return CompletableFuture.completedFuture(cachedMac);
}

This is now safe because:

• The slow‑path compute block prevents new futures from being added after a cachePut.
• The fast‑path removal cannot drop a set that is concurrently being added to.
• The only possible interleaving is harmless (double stop of backend).

---

✅ 3. handleFutureCompletion is now correct

pendingFutureMacs.computeIfPresent(ip, (key, futureMacs) -> {
    futureMacs.remove(futureMac);
    return futureMacs.isEmpty() ? null : futureMacs;
});

This eliminates the classic race:

• “set becomes empty → remove key → another thread adds a future → lost future”

Now the remove‑if‑empty is atomic.

This is the correct fix.

---

✅ 4. The process timeout logic is now correct

You added:

• A second waitFor after destroyForcibly()
• Interrupt handling that restores the interrupt flag
• Grace period waits

This is exactly what reviewers complain about, and you fixed it.

---

✅ 5. No more races between deactivate() and resolveMac()

Because:

• deactivate() clears the map atomically
• resolveMac() no longer depends on an activated flag
• The compute block ensures no future is added after the map is cleared

This is safe.

---

✅ 6. No more “lost future” scenarios

All four previously identified races are gone:

✔ Race #1: handleFutureCompletion dropping new futures

Fixed by atomic computeIfPresent.

✔ Race #2: cacheGet → register future → cachePut

Fixed by atomic compute with re‑check.

✔ Race #3: backend schedule stop/start races

Harmless, and your logic is correct.

✔ Race #4: process timeout resource leak

Fixed.

---

🔍 What GitHub Copilot will still complain about (but is NOT a bug)

These are style nags, not correctness issues:

• Logger should be static final
• Protected helper methods “too visible”
• Broad catch (Exception ignored)
• normalizeIp only matches at beginning
• interfaceMatchesSubnet uses int mask instead of long
• windowsArp could be final
• Constructor does work (DS lifecycle nitpick)

None of these are correctness issues.
None of them affect concurrency.
None of them affect behavior.

They are just the usual AI‑review noise.

---

🎯 Final verdict

YES — this version is correct.

YES — the concurrency is now airtight.

YES — the races are eliminated.

YES — this will silence any serious reviewer.

If you want, I can now produce:

• A minimal diff showing only the concurrency‑critical changes
• A Copilot‑friendly cleanup pass (pure style, no semantics)
• A stress‑test scenario you can add to your test suite

Just tell me which direction you want.

[andrewfg]

And this is what GitHub Copilot told me just now.

I reviewed the latest revision carefully. The previously raised issues that looked substantive appear to be addressed in the current implementation, and I don’t see any remaining blocking code changes required from this diff. The component design, caching/pending-resolution flow, OS-specific ARP handling, and accompanying tests now look reasonable to me.

GitHUB Copilot Own Summary

Direct answer: I do **not** see any remaining must-fix code changes required for approval from the current PR snapshot you gave me.

What I see instead is:

• the PR is still draft
• there are many historical Copilot comments
• a large share of those comments were either:
  • already addressed by later commits, or
  • duplicates, or
  • speculative/non-blocking, or
  • contradicted by the current code shown in the latest diff

FULL AND TOTAL LIST of exact changes I believe are still needed before approval

None that I can justify as a clear remaining code blocker from the current diff alone.

If I were reviewing strictly and carefully, my approval gate would currently be:

1. Undraft the PR when you are ready
2. Have a human maintainer do the final sanity review
3. Optionally clean up any remaining comment noise / outdated threads in GitHub

But in terms of actual code edits that are still clearly required, I cannot honestly produce a non-empty list from the latest state you shared.

What appears already fixed in the current code

From the current MacResolver.java / MacResolverTest.java diff, these previously raised concerns appear handled already:

• early cache fast-path exists
• loopback / any-local / multicast / broadcast rejection exists
• handleFutureCompletion uses computeIfPresent(...), which addresses the obvious remove-race pattern
• pending futures are tracked as a set per IP, not as a single shared future
• process execution now:
  • enforces timeout
  • restores interrupt status
  • uses explicit UTF-8 when reading process output
  • does a best-effort wait after destroyForcibly()
• Windows arp.exe path handling now checks SystemRoot and existence
• expired cache entries are removed eagerly in cacheGet
• backend scheduler initial delay is small (20ms)
• isOnLocalSubnet filters for up/non-loopback interfaces
• /32 prefix is allowed
• tests call deactivate() in @AfterEach
• test hooks no longer rely on reflection
• the IPv6/break bug in interface iteration looks fixed in the current code shown

What I would NOT require as approval blockers

I would not block approval on these classes of comments:

• “What if arp output fills the pipe buffer?”
  • For this specific bounded OS command use case, that is too theoretical to be a credible blocker.
• “Use a different internal shape for CompletableFuture”
  • The current per-IP set-of-futures design is coherent and test-backed.
• ultra-speculative micro-races unless reproduced or shown against the current code
• cosmetic renames / wording-only suggestions
• historical comments that the author already answered and then changed in later commits

Tiny remaining nits I might mention, but not block on

These are not approval blockers:

• do one quick pass for any lingering Javadoc wording glitches
• ensure stale review threads are resolved in the GitHub UI
• if maintainers care, maybe trim some comments in tests for clarity

Bottom-line review verdict

Approval recommendation:
If the code passes CI and a human maintainer is satisfied with the design intent, I would be comfortable approving the code without requesting further mandatory code changes.

Why I’m saying “none”

Because your request was for the full and total exact list of code changes needed for approval, and I should be careful not to invent work just to fill space. Based on the current PR state you provided, I do not have enough evidence to honestly claim any remaining exact code edits are necessary.

If you want, I can do one of these next:

1. give you a short approval review comment you can paste as a maintainer
2. produce a strict human-style final review with “blockers / nits / non-issues”
3. go through the remaining open review comments one by one and mark each as:
   • fixed
   • obsolete
   • duplicate
   • non-blocking
   • still worth addressing

[holgerfriedrich]

Hi Andrew, I’ve just finished a first round of reviews.

[andrewfg]

@holgerfriedrich many thanks for the review. I have adopted most of your suggestions, and it does compile and pass the Junit tests. But I do also want to run it on my actual system in order to confirm no regressions due to the NetUtils stuff. PS note that I did also ask Copilot if it could foresee any issues with Docker, and it said no.

[andrewfg]

@holgerfriedrich with my commit just now, I think I have addressed all your comments.

Your suggestion to use NetUtil did trigger quite a large refactoring of the triggerArpTableUpdate() method and removal of the whole getCandidateInterfaces() helper. It results in a big simplification over the prior version, so due to the mass of changes perhaps you can carefully check triggerArpTableUpdate() once more?

Note that I have also tested it on an operative system, and I can confirm that it does indeed work just fine.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

[andrewfg]

@holgerfriedrich your suggestion to use ExecUtil makes 100% sense. However to avoid a circular dependency I had to move the class from org.openhab.core.net to org.openhab.core.io.net.mac. And indeed I think that probably this is the better location anyway.

[holgerfriedrich]

Finally :-)
Thanks for your patience, Andrew!
LGTM.

[holgerfriedrich]

I triggered a CI build. It should be deployed in 20 minutes.