Skip to content

send-message: payload from stdin or a file; messages over 256 KB are delivered; an unacknowledged send fails - #501

Merged
TeoSlayer merged 4 commits into
mainfrom
feat/send-message-data-file
Oct 7, 2026
Merged

send-message: payload from stdin or a file; messages over 256 KB are delivered; an unacknowledged send fails#501
TeoSlayer merged 4 commits into
mainfrom
feat/send-message-data-file

Conversation

@TeoSlayer

@TeoSlayer TeoSlayer commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Pull Request

Summary

send-message --data took the body only as a command-line argument, which the OS caps: a 1 MB payload fails with Argument list too long, and Linux stops a single argument at 128 KiB. Adding a way around that exposed an older bug behind it: any message over 256 KB was silently dropped by the sending daemon while pilotctl reported success.

Changes

pilotctl

  • --data - reads the payload from stdin; --data-file <path> reads it from a file.
  • send-message now fails when no ACK comes back. Every receiver answers a stored message with an ACK; with none, the command used to print "status":"ok" and exit 0.

Daemon

  • A single stream write larger than MaxNagleBuf (256 KB) was refused with ErrSendBufFull. An IPC send has no reply, so the refusal went nowhere (the driver sends up to 1 MiB per chunk). SendData now feeds a large write through the buffer in whole-segment pieces, blocking on the window like any other write.

For review: a pinned test changed

TestSendDataNagleBufGrowsUnbounded asserted that a 5 MiB write returns ErrSendBufFull immediately — the v1.9.1 fix for unbounded buffer growth. That refusal is what dropped large messages. The memory guarantee it protects is kept and still tested: the new TestSendDataOversizedWriteStaysWithinNagleCap checks that, against a peer that never ACKs, the buffer stays within the cap while the oversized write is blocked, and that closing the connection releases the writer with an error. The behaviour change is "blocks under back-pressure" instead of "rejected at once" for a write larger than the buffer.

Confirmed against real nodes (two containers)

Payload Before After
1 MB as --data Argument list too long same (OS limit)
200 KB via stdin delivered delivered
1 MB via stdin / --data-file "status":"ok", nothing stored; daemon log IPC stream send failed ... send buffer full delivered, sha256 matches
5 MB via --data-file — delivered

The first version of this PR was caught by that run: --data-file was rejected by the command's flag allow-list, which the unit test of the helper did not exercise.

Test Plan

  • go build ./..., go vet ./..., unit suite with GOWORK=off
  • TestMessagePayload, TestSendDataOversizedWriteStaysWithinNagleCap
  • TestSingleWriteLargerThanSendBufferIsDelivered (integration; fails on main: "a 1 MB write was not delivered within 20s")
  • Full go test -parallel 4 -count=1 ./tests/: one failure, TestManualSnapshotTrigger, which binds fixed port 127.0.0.1:18080 held by another process on the test machine
  • scripts/gen-cli-reference.sh leaves docs/cli-reference.md unchanged
  • The no-ACK failure path is not covered by an automated test

Checklist

  • New code includes the SPDX license header
  • go.mod / go.sum unchanged
  • CHANGELOG updated

🤖 Generated with Claude Code

@TeoSlayer TeoSlayer changed the title pilotctl: send-message reads its payload from stdin or a file send-message: payload from stdin or a file; messages over 256 KB are delivered; an unacknowledged send fails Oct 1, 2026
@TeoSlayer TeoSlayer mentioned this pull request Oct 1, 2026
Merged
Teo Calin and others added 2 commits October 7, 2026 15:56
Teo Calin @claude Teo Calin
pilotctl: send-message reads its payload from stdin or a file
76ad3b9 
--data took the body only as a command-line argument, which the OS caps: a
1 MB payload fails with "Argument list too long" and Linux stops a single
argument at 128 KiB. Add "--data -" (stdin) and --data-file <path>.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Teo Calin @claude Teo Calin
Deliver messages over 256 KB; accept --data-file; fail an unacknowled…
539c540 
…ged send

Confirming --data-file against real nodes showed three problems:

- send-message rejected --data-file: its flag allow-list was not updated.
- The daemon refused any single stream write larger than MaxNagleBuf
  (256 KB) with ErrSendBufFull. An IPC send has no reply, so the refusal
  went nowhere: a 1 MB message was dropped while pilotctl printed
  status ok. SendData now feeds a large write through the buffer in
  whole-segment pieces, blocking on the window like any other write. The
  cap on the buffer is unchanged; the unit test that pinned the refusal is
  split so the cap is still asserted, now during a blocked oversized write.
- send-message exited 0 when no ACK came back. It now fails.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@TeoSlayer
TeoSlayer force-pushed the feat/send-message-data-file branch from 1985d5d to 539c540 Compare October 7, 2026 13:09
Teo Calin and others added 2 commits October 7, 2026 16:26
Teo Calin @claude Teo Calin
send-message: keep large writes whole and report every undelivered me…
b3d0d1f 
…ssage

Review fixes for the large-payload change.

Daemon:
- A write over one Nagle piece went through the buffer in pieces with the
  send lock released between them, so two writers on one connection could
  interleave their bytes. A per-connection WriteMu now holds for the whole
  write.
- The piece size is a whole number of 1152-byte segments, and whether a
  short tail may be held is decided once from the whole write, so a large
  write sends its last piece at once instead of holding it for an ACK.

pilotctl:
- The payload is read and checked before the daemon is contacted. A
  payload over the data-exchange frame limit is refused with a size error
  instead of being dropped by the receiver; an empty --data-file is named.
- A send that failed outright exited 0 with an error field; it now fails.
- With --count, a message that failed, was not acknowledged or was refused
  by the receiver made no difference to the exit status; any such message
  now fails the command with a count and the first reason.
- The ACK read error is kept in the result and shown when a message is not
  acknowledged.
- The usage text and docs/cli-reference.md mention --data-file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Teo Calin @claude Teo Calin
send-message: a large write survives tunnel send errors and stops at …
66441ae 
…a close

Second review round.

Daemon:
- A tunnel send error part-way through a large write ended the write. The
  failed segment is tracked and retransmitted, but the pieces not yet
  buffered were dropped, and the connection's next write landed in their
  place in the stream. The write now keeps draining after such an error and
  stops only when the connection is gone.
- A large write blocked on the window went on sending after a local close.
  The state is checked again after WriteMu and between pieces, so at most the
  piece already buffered follows the FIN.
- The tail test now leaves a short segment in flight first, which is when
  deciding per piece would have held the last byte; it failed against that.

pilotctl:
- With --trace a receiver's refusal is in inner_ack; both the single and the
  multi-message checks now see it.
- A failed --count run keeps every message's result in the error, so the
  caller can tell which messages to send again. A run where the receiver
  refused every failed message uses the same code as a single refusal.
- The usage line, the command catalogue and the changelog mention --data -.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@TeoSlayer
TeoSlayer merged commit 049a4e9 into main Oct 7, 2026
15 checks passed
@TeoSlayer
TeoSlayer deleted the feat/send-message-data-file branch October 7, 2026 14:05
TeoSlayer pushed a commit that referenced this pull request Oct 7, 2026
Teo Calin @claude Teo Calin
pilotctl: review fixes for send-message reply matching after #501
77ef51b 
- A --trace message is not sent again after a lost ack: receivers do not
  suppress repeated trace frames (dedupeEligible), so even a current
  receiver would store it twice.
- The lost-ack retry still runs inside sendOne, before #501 judges the
  send: a retry that is acknowledged is delivered, and a send whose retry
  is not acknowledged fails with the retry's ack error.
- The echo-history scan runs in a goroutine started with the inbox
  snapshot and is waited for only when an untagged reply has to be judged,
  so it no longer delays the request. It checks for the sender and
  reply_to bytes before parsing, skips records over 1 MiB and stops after
  16 MiB as well as after 1000 files or 200 records from the peer.
- The hold runs until 0.75 s after the message arrived (its received_at),
  and awaitReply polls when it ends, so it is no longer up to a poll
  interval longer than stated.
- --reply-to accepts an inbox file name with .json.
- The help and CHANGELOG say a receiver through v1.13.9 can store a
  retried message twice when the ack of its untagged copy is lost; from
  v1.13.10 the repeat is suppressed.
- inbox read lines up its labels; a misleading poll comment is fixed.
- Tests no longer call t.Fatal from goroutines or receiver callbacks, wait
  with a timeout, and allow 400 ms for a reply taken at once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TeoSlayer pushed a commit that referenced this pull request Oct 7, 2026
Teo Calin @claude Teo Calin
pilotctl: review fixes for send-message reply matching after #501
bd8a986 
- A --trace message is not sent again after a lost ack: receivers do not
  suppress repeated trace frames (dedupeEligible), so even a current
  receiver would store it twice.
- The lost-ack retry still runs inside sendOne, before #501 judges the
  send: a retry that is acknowledged is delivered, and a send whose retry
  is not acknowledged fails with the retry's ack error.
- The echo-history scan runs in a goroutine started with the inbox
  snapshot and is waited for only when an untagged reply has to be judged,
  so it no longer delays the request. It checks for the sender and
  reply_to bytes before parsing, skips records over 1 MiB and stops after
  16 MiB as well as after 1000 files or 200 records from the peer.
- The hold runs until 0.75 s after the message arrived (its received_at),
  and awaitReply polls when it ends, so it is no longer up to a poll
  interval longer than stated.
- --reply-to accepts an inbox file name with .json.
- The help and CHANGELOG say a receiver through v1.13.9 can store a
  retried message twice when the ack of its untagged copy is lost; from
  v1.13.10 the repeat is suppressed.
- inbox read lines up its labels; a misleading poll comment is fixed.
- Tests no longer call t.Fatal from goroutines or receiver callbacks, wait
  with a timeout, and allow 400 ms for a reply taken at once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TeoSlayer added a commit that referenced this pull request Oct 7, 2026
@TeoSlayer Teo Calin @claude
pilotctl: send-message --wait matches the reply by message ID (#498)
29a3d5e 
* pilotctl: send-message --wait matches the reply by message ID

--wait took the oldest new inbox message from the peer. Two concurrent
requests to one peer, or anything else the peer sent in the window, could
hand a caller another request's answer.

send-message now sends every message through dataexchange's tagged path
(Client.Send) with a new message ID, and the inbox watch uses it:

- a message from the peer whose reply_to is one of our IDs is the reply;
- a message whose reply_to names another request is never taken;
- a message without reply_to is matched by sender and arrival time, as
  before, so peers that do not echo the ID (every responder today) keep
  working unchanged.

The first-contact re-send gets an ID of its own, because the receiver
drops a repeat of the same ID and bytes as a duplicate; the watch accepts
a reply to either. --trace is covered: the TRACE wrapper is built here and
carries the ID outside it.

New flag --reply-to <id> sends a message as the answer to a received one.
The JSON result gains message_id, tagged and (with --reply-to) reply_to.

Receivers that predate message IDs are reached through the library's
fallback: the frame is re-sent in the old format on the same connection,
and the result says "tagged": false. An "ERR ..." answer is still reported
as the ack, and a send whose ack could not be read still succeeds, as
before.

No dependency change: dataexchange v0.2.3, already pinned, has the IDs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* pilotctl: hold an untagged reply for a tagged one; safer --reply-to; inbox shows IDs

send-message --wait took the first untagged message from the peer at
once, so a reply naming our request that arrived a poll later lost to
another client's untagged answer. When our request reached the receiver
with its ID, an untagged message is now held for 0.75 s (bounded by the
wait) in case our tagged reply follows, and once the peer has been seen
naming another request in reply_to, only a reply naming ours is taken.

--reply-to refuses a bare flag (parsed as "true") and maps an inbox file
id to the message_id stored in that record, refusing it when the record
has none. pilotctl inbox shows message_id and reply_to in the listing,
--json, and inbox read; the help says which field to use.

A lost ack on the tagged first attempt is retried once on a new
connection with the same frame and ID: a current receiver keeps one
copy, one through v1.13.9 gets the untagged copy. --no-resend opts out.
tagged is reported only when the receiver's ack was read.

CHANGELOG no longer claims the concurrency bug is fixed for responders
that do not echo reply_to.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* pilotctl: hold an untagged reply only from a peer known to echo IDs

No service responder echoes reply_to yet, so holding every untagged
reply for 0.75 s whenever the receiver's daemon knows message IDs would
slow nearly every send-message --wait once daemons upgrade, for nothing.

An untagged reply is now taken at once, as before, unless the peer is
known to echo IDs: one of its newest messages already in the inbox
carries a reply_to. newInboxWatch checks this once, from the snapshot it
already reads, looking at no more than the newest 1000 records and 200
from the peer. For such a peer the 0.75 s hold stays, and a peer seen
naming another request during the wait still gets only a reply naming
ours taken.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* pilotctl: review fixes for send-message reply matching after #501

- A --trace message is not sent again after a lost ack: receivers do not
  suppress repeated trace frames (dedupeEligible), so even a current
  receiver would store it twice.
- The lost-ack retry still runs inside sendOne, before #501 judges the
  send: a retry that is acknowledged is delivered, and a send whose retry
  is not acknowledged fails with the retry's ack error.
- The echo-history scan runs in a goroutine started with the inbox
  snapshot and is waited for only when an untagged reply has to be judged,
  so it no longer delays the request. It checks for the sender and
  reply_to bytes before parsing, skips records over 1 MiB and stops after
  16 MiB as well as after 1000 files or 200 records from the peer.
- The hold runs until 0.75 s after the message arrived (its received_at),
  and awaitReply polls when it ends, so it is no longer up to a poll
  interval longer than stated.
- --reply-to accepts an inbox file name with .json.
- The help and CHANGELOG say a receiver through v1.13.9 can store a
  retried message twice when the ack of its untagged copy is lost; from
  v1.13.10 the repeat is suppressed.
- inbox read lines up its labels; a misleading poll comment is fixed.
- Tests no longer call t.Fatal from goroutines or receiver callbacks, wait
  with a timeout, and allow 400 ms for a reply taken at once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* pilotctl: the untagged-reply hold starts no earlier than the ack

Review fix. The hold counted from the message's received_at even when that
was before our request was acknowledged, so after a slow first-contact
dial, a large payload or a lost-ack retry, an untagged message from a peer
known to echo IDs was taken at the first poll, ahead of the tagged reply.
A message that arrived before the ack is the least likely to be the reply;
its hold now starts at the ack. The echo-history scan also skips anything
that is not a regular file, so a FIFO in the inbox cannot block it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* pilotctl: justify the inbox reads gosec flags; close the retry connection explicitly

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Teo Calin <calinteodor@Teos-MacBook-Pro.local>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@TeoSlayer