Skip to content

driver: SendToConfirmed reports whether the daemon sent a datagram - #54

Merged
TeoSlayer merged 4 commits into
mainfrom
feat/dgram-confirm
Oct 7, 2026
Merged

TeoSlayer merged 4 commits into
mainfrom
feat/dgram-confirm

Conversation

@TeoSlayer

Copy link
Copy Markdown
Contributor

Summary

Driver.SendTo returns nil as soon as the datagram is written to the daemon's socket; a datagram the daemon then fails to send (unknown node, port policy, no free ports) is only logged there. This adds a send that waits for the daemon's answer.

Changes

  • SendToConfirmed(dst, port, data) (confirmed bool, err error) sends the new cmdSendToConfirm (0x39) and waits for cmdSendToOK (0x3A) or an error.
    • (true, nil): the daemon handed the datagram to its tunnel (not a delivery guarantee).
    • (false, err): the daemon could not send it, or did not answer in time.
    • (false, nil): the daemon predates confirmed sends. It answered "unknown command", so the datagram went out with the legacy fire-and-forget command and its outcome is unknown. The driver probes once and remembers.
  • SendTo is unchanged. A confirmed send is a request/reply exchange and waits behind an in-flight dial on the same Driver, so SendTo stays the choice where throughput matters.

The daemon side is pilot-protocol/pilotprotocol#494. Either can land first: an old driver against a new daemon behaves as before, and this driver falls back against an old daemon.

Test Plan

  • go build ./..., go vet ./..., go test ./...
  • Unit tests for the confirmed, error and fallback paths
  • Run against a daemon with the handler and one without

🤖 Generated with Claude Code

@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.44444% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
driver/ipc.go 79.41% 13 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@TeoSlayer
TeoSlayer marked this pull request as draft October 1, 2026 15:45
@TeoSlayer

Copy link
Copy Markdown
Contributor Author

Converted to draft after review. One bug to fix before this lands in the shared library: the IPC has no request IDs, and replies are matched by opcode only (driver/ipc.go, deliverReply). After a 30 s timeout, the late reply is delivered to the next confirmed send. In a scratch test, a second send that the daemon rejected returned confirmed=true because it received the first send's late reply. A caller would be told a datagram went out when it did not.

Everything else checked out: CI is green, and a new driver against a v1.14.1 daemon returns (false, nil) with the datagram still arriving through the legacy path, so there is no hang.

SendTo is fire-and-forget: the daemon never replies, so a datagram it failed
to send (no route, port policy, ephemeral ports exhausted) still returns nil.

SendToConfirmed sends the daemon's new cmdSendToConfirm (0x39) and waits for
cmdSendToOK (0x3A) or the daemon's error. Against a daemon that predates the
command (it answers "unknown command") the datagram is sent with the legacy
cmdSendTo and reported as unconfirmed; the probe is done once per Driver.
SendTo is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@TeoSlayer
TeoSlayer marked this pull request as ready for review October 7, 2026 13:02
@TeoSlayer

Copy link
Copy Markdown
Contributor Author

Fixed the late-reply mis-delivery found in review (commit d60d22e). The daemon handles confirmed sends in order on one IPC connection, so the client now counts confirmed sends that timed out and drops their late answers instead of handing them to the next confirmed send. TestSendToConfirmedLateReplyIsNotTheNextSendsAnswer reproduces the review's scenario (a rejected second send came back as confirmed=true), fails without the fix, and passes with it; go test -race ./driver/ passes 3 of 3. Rebased onto main; marking ready.

…te answer holds the lane

The IPC carries no request IDs. A confirmed send that timed out was later
answered by the daemon, and that cmdSendToOK reached the next confirmed
send: in review, a send the daemon rejected was reported confirmed=true. A
first fix counted timed-out sends; review showed the count could get stuck
(a late error arriving while nothing waited left every later send timing
out, and against an old daemon the fallback never ran) and that a late
error from an abandoned Info or Dial could still be taken as an answer.

Now:
- A confirmed send's answer is recognised by its content - cmdSendToOK, or
  an error starting "sendto: " or "unknown command: 0x39", which nothing
  else produces - and is only ever delivered to the confirmed send waiting
  for it; other requests' replies never reach it.
- After a timeout, the request lane stays held until that send's late
  answer arrives (the daemon answers confirmed sends in order and always
  answers), or for at most a minute; the caller gets its timeout at once.
  The next confirmed send therefore can only receive its own answer.
- Time spent queued behind another request does not count against the
  wait for the answer.
- The timeout error says the datagram may or may not have been sent.

Tests cover each case from review: a late OK, a late error while idle, an
abandoned Info's late error, a legacy probe that timed out, and queueing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Teo Calin and others added 2 commits October 7, 2026 16:42
…rmed send

Review fix. After a confirmed send timed out, the request lane stayed held
until its late answer arrived or 60s passed. If the answer came after that,
the next confirmed send had registered by then and took it as its own, so a
rejected send could report confirmed=true.

A confirm-only semaphore now stays held until the late answer arrives or the
connection is gone, with no time limit, and the shared request lane is
released as soon as the caller times out. Answers are recognised by content,
so other requests can run meanwhile without taking or being taken for the
confirmed send's answer; only the next confirmed send waits, and it gets
ErrConfirmQueueTimeout (nothing sent) if the answer never comes.

The two timeout errors are exported so callers can tell an unknown outcome
from a send that never happened.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@TeoSlayer
TeoSlayer merged commit 0a90abd into main Oct 7, 2026
10 of 11 checks passed
@TeoSlayer
TeoSlayer deleted the feat/dgram-confirm branch October 7, 2026 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant