New pigeonhole thinclient API - #985
Merged
Merged
Conversation
david415
force-pushed
the
new_pigeonhole_thinclient2
branch
from
February 20, 2026 14:23
6d509ab to
03d7b03
Compare
david415
force-pushed
the
new_pigeonhole_thinclient2
branch
4 times, most recently
from
March 7, 2026 16:59
892594d to
0d80f22
Compare
…ately When the dedup cache contains only read errors (e.g. BoxIDNotFound), trigger a background re-dispatch to the replicas so the cache refreshes for the next client retry. Return the cached error immediately via handleOldMessage rather than calling handleNewMessage, so NoRetry clients receive the error and stop instead of looping on ACKs.
Adds a dedicated LambdaR parameter to the PKI document to govern the send rate of decoy traffic between couriers and storage replicas, replacing the temporary LambdaP placeholder. Wired through the voting authority config, state machine, genconfig, and docker Makefile.
note that this feature is essentially disabled by default unless the config option MetricsAddress is set
here we also fix the courier such that the replica integration tests pass
Generate Grafana provisioning configs and a pre-built dashboard for courier/replica decoy traffic metrics. Add prometheus scrape jobs with per-instance labels (replica1, courier1, etc.). Add --noMetrics flag to skip prometheus/grafana containers in docker-compose, used in CI to avoid pulling external images.
The queue length gauge was only updated on enqueue, causing it to flatline at 1. Now also updates when the sender drains a message, so the gauge reflects actual queue depth.
The 2-minute warped epoch is too short for local testing with decoy traffic enabled. The KATZENPOST_EPOCH_DURATION env var can override this at runtime for contexts that need the shorter period.
sendLoopDecoy and sendDropDecoy panicked when no PKI document or echo services were available. Log a warning and return instead.
Heretofore dispatchCopyEnvelope would, upon receiving but a solitary reply of BoxAlreadyExists before the deadline, most rashly pronounce "both intermediates report BoxAlreadyExists" and abort the Copy entire — a conclusion drawn, one regrets to say, from evidence patently insufficient. Under the ordinary K=2 arrangement, one intermediate's write arrives first at the shard (whereupon Success is returned) whilst its peer sees the prior write and replies BoxAlreadyExists; should the Success reply chance to tarry past the thirty-second deadline, the old logic would terminate the Copy with CopyStatusFailed in error. Henceforth any terminal verdict demands the testimony of both intermediates. A Success reply continues to short-circuit the attempt forthwith. A shortfall of replies is treated as a transient embarrassment and retried. Only with two non-Success replies does one consult bestErr and, upon BoxAlreadyExists at attempt zero, abort — preserving the invariant upheld by TestCopyOntoAlreadyExistingBoxError. Pinned by the new TestDispatchCopyEnvelopeSingleBoxAlreadyExistsReply IsTransient, which feeds a single BoxAlreadyExists reply on attempt zero, lets the deadline elapse, and requires the courier to re-dispatch rather than abort.
Legacy-format Consensus replies and malformed Consensus2 chunks off the gateway wire previously paniced the whole daemon, taking every connected thin client with it. Follow the pattern already used for unknown command types: log, set wireErr, return — the reconnect loop then retries cleanly.
Three retry sites (arqDoResend, handlePigeonholeARQReply SendNewSURB, handlePayloadReply BoxIDNotFound retry) previously returned without rearming the ARQ timer when ComposeSphinxPacketForQuery tripped — leaving the ARQMessage orphaned and the thin client's request wedged until disconnect. A fresh helper now keys the message under a placeholder SURBID and pushes a one-second retry, so a transient PKI stumble no longer strands a request in silence.
The pigeonhole API event loops wrote isConnected bare upon receiving a ConnectionStatusEvent, whilst dispatchMessage did so under connMu; three public send methods likewise read the field bare. -race caught the collision at thin.go:799 against thin.go:1506. Introduce a setConnected helper and route every write through it, every read through IsConnected. Pinned by TestIsConnectedRaceFree.
A ConnectionStatusEvent with IsConnected=false previously panicked the calling goroutine via panic(errConnectionLost); an event type the switch did not recognise panicked with "impossible event type". A mid-flight disconnection is an ordinary recoverable outcome and an unknown event type warrants a debug line and continuation, not the loss of the process. Pinned by TestBlockingSendMessageDisconnectReturnsError and TestBlockingSendMessageUnknownEventDoesNotPanic.
Every outbound packet traversed two RWMutex cycles to read isConnected and isShutdown. The two booleans never need multi-field consistency, so sync/atomic.Bool serves the same purpose with a single memory-order load per read. Test sites updated to match.
writeResponse previously allocated a fresh slice to concatenate the
4-byte length prefix with the CBOR-encoded response before a single
Write. net.Buffers{prefix, blob}.WriteTo coalesces into one writev
syscall on tcp and unix transports, and emits two Writes on those
that lack it — either way, no intermediate allocation.
ComposeSphinxPacket and ComposeSphinxPacketForQuery each retried path selection indefinitely when it straddled an epoch or produced excessive delays, leaving only the halt channel as an escape. Under a pathological PKI document the loops spin at full CPU per outbound packet. A cap of 32 attempts returns a clear error well above the handful a healthy epoch rollover requires.
…ariants The three public methods differed only in two boolean flags on the outgoing request and in their own name appearing in the log prefix. Collapse the ~100-line body into startResendingEncryptedMessageImpl parameterised by the two booleans; the three public methods become one-line shells preserving their doc comments. Log lines now include (noRetry=%v, returnBoxExists=%v) so the variant remains visible. Net −160 lines. Behaviour unchanged; existing tests (pigeonhole_test.go:519,554) still pin flag propagation.
The fan-out worker formerly evicted any drain that did not accept within 100ms, with nothing but a Warning log to the evicted subscriber. Neither the Rust nor the Python thin client drops events: Rust's drains are tokio UnboundedSenders, Python calls handlers directly. The Go divergence was silent and surprising to integrators porting across. Remove the timeout. The fan-out now blocks until the subscriber accepts; HaltCh remains the escape hatch for Close(). A stuck subscriber will stall the fan-out for every subscriber — the same hazard Python already exposes with a stuck callback — so the EventSink doc is updated to say so plainly. Pinned by TestEventSinkNoEviction, which sleeps past the old 100ms threshold with an idle subscriber and asserts all events arrive.
A CancelResending* landing between handleReply's delete and the handler's re-register silently no-opped whilst the daemon carried on polling. handleReply no longer deletes; rotation paths guard with a still-present check under replyLock. Pinned by three tests in pigeonhole_cancel_race_test.go.
injectCourierEnvelope returns on the courier's immediate write-ACK, well before the shards have durably stored. The 5s post-write sleep was racing replication under CI load and the subsequent proxy read saw BoxIDNotFound. Bump to 15s.
sendAndWait subscribed a drain via EventSink() but never released it. The old 100ms eviction masked the leak; after the switch to the no-loss fan-out contract (commit 4d92da3) each leaked capacity-1 drain stalled the fan-out for every other subscriber on the same thin client. repeatSendAndWait (5 iterations on one client) accrued enough leaks to wedge TestLegacyTests/TestDockerClientSendReceive for the 1h suite timeout. Add the missing defer StopEventSink.
When a BoxID is occupied, the stored Box is now compared against the incoming write. Matching Payload and Signature returns success; only differing data still returns ErrBoxAlreadyExists. This dissolves the ambiguity the courier's K=2 copy path faced when a reply was lost on retry, which TestCopyOntoAlreadyExistingBoxError exposed. The replication layer already assumed this idempotence; the state layer now agrees.
With the replica's matching-data writes now idempotent, a non-Success reply from any intermediate reflects a genuine conflict — retrying will not change the verdict. dispatchCopyEnvelope no longer waits for both replies before declaring failure, and no longer treats a retry's BoxAlreadyExists as our own prior write landing. Retries are reserved for transport failures (SendMessage error, no replies before the deadline). Two tests whose premises the replica fix dissolved are removed; one new test pins the single-reply abort.
The pruneDocuments path removed expired entries from the in-memory maps but never deleted the corresponding sub-buckets and document keys from bolt. The descriptors, replica_descriptors and documents buckets therefore accumulated state for every epoch the dirauth had ever served, so persistence.db grew without bound and would in time fill the disk. Restart only made matters worse, since restorePersistence loads at most three epochs into memory and leaves the rest as dead weight on disk. This adds a small bolt transaction at the end of pruneDocuments which, using the same preservation window as the in-memory pass, deletes any sub-bucket or key whose 8-byte big-endian epoch is strictly older than now minus three epochs. A unit test exercises the pruner directly against a temporary bolt database and confirms that fresh epochs and non-epoch keys are left untouched and that a second pass is a no-op. Note: bolt does not return freed pages to the filesystem in place, so operators with an already-bloated persistence.db will need to compact or simply replace the file once this fix has stopped further growth.
…etch GetPKIDocumentForEpoch previously walked the configured authorities in sequence, calling fetchConsensus on each in turn. As fetchConsensus itself retries with exponential backoff (up to RetryMaxAttempts) before giving up on a single peer, a lone unresponsive dirauth at the head of the random permutation could consume the caller's entire context budget before any other peer was tried. This blocked the mix-server PKI worker on every epoch and, more grievously, prevented dirauth bootstrap from populating prior-epoch documents after a consensus failure, contributing to the cascade observed in production. Each configured authority is now contacted in its own goroutine. The first peer to return a verified, threshold-signed, well-formed document for the requested epoch wins; the deferred cancel terminates the remaining in-flight fetches, which write to a buffered channel and exit. A single down or slow dirauth no longer holds up its peers. A regression test, TestGetPKIDocumentForEpochParallelRace, pins the new behaviour: with four of five peers stalled at dial, the fetch completes in well under a second using the responsive peer.
TestGetPKIDocumentForEpochParallelRace was flaking on darwin CI: the post-quantum (XWING) handshake alone took over five seconds on the shared macOS runner, exceeding the original five-second elapsed-time assertion. The wall-clock budget was tightened to twenty seconds, with a comment explaining the choice. Sequential behaviour, were it ever to return, would still block for the full thirty-second context, so the threshold remains amply discriminating. No behavioural change to the production code.
…s test TestParallelFailingAuthorities asserts that allPeersRoundTrip returns within three seconds when several peers refuse the dial outright. The post-quantum handshake performed for each successful peer makes that budget too tight on a shared macOS CI runner whose host has been busy working through the rest of authority/.... Bumping the bound to twenty seconds, in line with the earlier accommodation made for the parallel race test. The threshold remains amply discriminating: a sequential implementation, were it to return, would block on the full thirty second context. No production-code change.
Daemon.encryptRead and similar callers reach Client.CurrentDocument, which calls WaitForCurrentDocument. When the cached document for the current epoch is absent (whether at startup, across an epoch boundary, or under any future bug), WaitForCurrentDocument falls through to a synchronous updateDocument and ultimately to pki.getDocument, which calls p.c.PKIClient.Deserialize(...). If PKIClient is nil, the call panics with a nil-interface method dispatch fault. Although Client.Start sets PKIClient before any caller can normally observe it, there is no guarantee against a future regression or an exotic shutdown sequence. A defensive nil-check is added at the call site so the synchronous fetch returns a clean, logged error rather than crashing the process. The unit test TestAliceSendsBobMessage now wires up a mockPKIClient on its hand-built Client. The previous setup populated only the document cache and silently relied on the cache hit; on a slow darwin runner the cache miss path was reached and the panic surfaced. With a mock present the test produces a deterministic outcome whichever path runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE: that this branch enables the replica static uint8 identity patchset which changes the replica and dirauth config formats to include a replica ID field which is a unique uint8 integer. this helps keep our hash based sharded storage scheme consistent.
Here we modify the client, courier and replica to get the protocol working end to end.
Initially, this set of commits is mainly concerned with updating the client2 thinclient API to contain the latest Pigeonhole thinclient API. HOWEVER, we also update the github CI workflow to remove the old pigeonhole tests and run the new tests. Also we make sure to separate out the Sphinx benchmarks and run them only once per month whilest running all the other benchmarks once per day.
We fix some bugs in the courier and replica and client.... so that finally we make the messages flow end to end and the docker mixnet pigeonhole integration test passes.