feat(connectors): add the HTTP source webhook gateway connector - #3798
feat(connectors): add the HTTP source webhook gateway connector#3798mlevkov wants to merge 98 commits into
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3798 +/- ##
============================================
+ Coverage 86.35% 86.64% +0.28%
Complexity 1455 1455
============================================
Files 1259 1267 +8
Lines 205992 211595 +5603
Branches 171193 176811 +5618
============================================
+ Hits 177882 183334 +5452
- Misses 23668 23751 +83
- Partials 4442 4510 +68
🚀 New features to boost your workflow:
|
504c603 to
ebca5c2
Compare
|
/request-review @hubcio |
|
Pushed a follow-up commit closing the coverage gaps worth closing. Kept it as a Patch coverage was 94.52% (2847 hits / 131 misses / 34 partials). Reading the Both existing management auth tests reached only Also newly pinned: Every new test was mutation-checked — each was confirmed to fail against a One production line changed: Deliberately left uncovered, and why:
Full local gate green (fmt, sort, workspace clippy |
4c60da8 to
6d3bcac
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either Thank you for your contribution! |
6d3bcac to
a358ddf
Compare
a358ddf to
50f8258
Compare
|
Rebased onto master and followed up on
Nothing is ever abandoned. Answering 200 already told the sender this gateway owns the event, and the only honest way to shed load is the 429 the handlers return once the bridge fills, which senders retry. Dropping a staged batch would trade that bounded, visible backpressure for silent loss that grows with the length of the outage. The usual case for a give-up bound does not apply here either: oversized bodies are rejected with 413 before a handler runs, headers are clamped on accept, and Two existing poll tests were asserting the pre-ack behaviour by polling twice without acknowledging, and now ack between the two. Four new tests cover replay on NACK, release on ACK, an empty state-only batch not staging a replay, and the never-abandon property itself so it cannot be quietly reversed. One thing that needs a decision outside this PR: #3941. The SDK stops a source after five consecutive NACKs, which is about 1.5s of backoff plus five send rounds, so roughly two seconds of broker unavailability ends the poll task. That is correct for a source that can re-read its cursor and wrong for this one, whose bridge is in memory: the listener keeps accepting, the bridge fills to There is a loophole that would let a plugin survive this (an empty batch always acks, which resets the breaker's counter) and I have deliberately not used it, because it defeats an SDK safety mechanism from inside a plugin. The README states the residual window and points at #3941 rather than working around it here.
|
|
/request-review @hubcio |
23cfc0c to
886a096
Compare
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
two things block, both silent security-control failures with small fixes:
- a state-only batch that gets NACKed loses the revocation permanently, while the admin api reports
submitted: true(lib.rson_nack) - the registry fails closed per field and open in aggregate, so any decode failure resurrects every revoked endpoint (
state.rsrestore)
separately, README lines 14, 230 and 232 promise guarantees the code does not provide. those want correcting in this PR even if the code fixes land later.
the rest of the inline comments are smaller but mostly cheap. one is a real footgun beyond this connector: an out-of-range buffer_capacity aborts the whole connectors runtime through an extern "C" unwind, and a plugin's own validate() cannot stop it.
smaller things, no need for separate threads:
server.rsbody.to_vec()on the accept path deserves a why-comment. it is correct as written -Vec::from(body)would hand back hyper's read buffer instead, several times the body at typical webhook sizes, in a bridge bounded by message count. easy for the next reader to "optimise" awaymetrics.rsmaps 3xx into the 2xx buckettypes.rsexpiry usesunwrap_or_default()on the clock, so a bad clock fails open- the two 429 tests run at
buffer_capacity = 1, which selects crossfire's single-slot channel rather than the array one every real deployment uses. set 2 config.tomlships plausible-looking secrets on0.0.0.0where theexample_config/twins use placeholders with a warning. same snippet is duplicated in the READMEauth.rsrebuilds a constant hmac key on every compare - hoist it. the double-hmac itself is right,verify_slices_are_equalis deprecatedserver.rsre-parses three constantHeaderKeys per request, while the crate precomputesforward_headersfor exactly that reasontypes.rsbyte-slices at index 8 without a boundary check. unreachable today, butchars().take(8)costs nothinglib.rshas a duplicate polled-count log block, and a doc line claiming one atomic load per request on a method with no request-path caller- dead code worth dropping:
QueuedMessage.received_atis never read,EndpointRegistry::len/is_emptyandHttpSource::shared()have no non-test callers. thepub modsurface is also much wider than the siblings' for a cdylib nothing links as a lib - the three hand-written
EndpointOrigin/EndpointStatestring maps want oneconst fn as_str(&self) -> &'static str. notDisplay- the metrics encoder wants a&strandDisplaywould allocate on the scrape path - nothing posts to
/topics/{topic_path}in the integration tests
one refactor worth taking even though nothing is reachable today: make staged a std::sync::Mutex. it is never held across an await at any of its five uses, so it removes five awaits, makes three functions sync, and closes the window where a shutdown during stage() would lose a batch with no metric. do not extend that to receiver, which is held across recv().await and has to stay tokio.
two follow-ups that are not yours:
runtime/src/manager/source.rsdoes not close the plugin whensetup_source_producerfails afterinit_source, while the boot path does. inert for every existing source, but this is the first one that binds a public port inopen(), so a single transient failure leaves a bound listener and a joined instance behind and wedges the connector until the process restarts. either that gets fixed, or a same-name rejoin should evict the stale instancesource_connector!emits a bareuse dashmap::DashMap;where the sink macro already goes throughconnector_macro_support. worth a ticket. to be clear, thedashmapdep and machete ignore in this PR are correct and needed
github-actions
Bot
added
S-waiting-on-author
886a096 to
c8ba475
Compare
|
All of the review is addressed. Two of your points I have to push back on, both with evidence, and one gap in my own earlier response that you were right about twice over. The crossfire capacity claim is not correct, though the ask was. You wrote that Both tests still moved to 2, because the reason holds even though the mechanism does not: at capacity 1 the ring is degenerate, so the test cannot distinguish a real capacity bound from an off-by-one. I had copied your wording into two comments before checking it, which is on me, and they now state the corrected reason. The You were right twice about Also fixed since the review: a state-only NACK no longer leaves a revocation unretried; Three things I have verified and deliberately not changed, since they are yours to call:
|
|
Ready for another look. Everything in the review is addressed, and I put the branch through two independent review rounds afterwards, which found ten more defects. Two of those changed behaviour you already reviewed, so they are first.
A real race also turned up in code added for your review: The rest of what the rounds found, all fixed: Ten comment and doc claims were factually wrong and are corrected. The one worth naming is "an empty send cannot fail", which appeared in three places including on Four things I deliberately have not changed, because they are yours to call:
144 unit tests and the six |
|
/ready |
|
/request-review @hubcio |
github-actions
Bot
removed
the
S-waiting-on-author
The named path answered 404 twice for two different things. An unconfigured path is permanently absent and 404 is right. A configured path whose instance stopped serving it while the body was still arriving is transient, and that caller had already passed bearer auth, so 404 told it the resource was gone. Conventional clients stop retrying on 404, which turns a republish window into silently dropped traffic that would have succeeded a moment later. `management.rs` already answers 503 with "instance is closing" for this same condition. The secret path keeps 404 for both cases and the comment now says why: its caller is unauthenticated, so separating "wrong id" from "busy" would confirm which endpoint ids exist. This nearly made the identity guard untestable. `enqueue` on a stranger already answers 503 for a disconnected bridge, and the swap test asserted 404 precisely to tell those apart, so moving the guard to 503 would have made it pass whether or not the guard was there. The rival's bridge is now kept live through a new `test_support::live_instance`, so an unguarded request answers 200 with the message in the stranger's bridge. Removing the `Arc::ptr_eq` now yields `HTTP/1.1 200 OK` and the test fails on it, which is a stronger assertion than the old one: it proves delivery happened rather than that a status differed.
`revoke()` clears `auth_secret` for a stated reason: the handler answers 404 on a revoked entry before `authorize()` runs, nothing compacts the registry, so keeping the secret would write a leaked credential back to the state file indefinitely. An expired endpoint is refused the same way and just as early, and kept its secret anyway. `restore` now drops it, and says how many it dropped. That was also the only restore-time fault with no startup signal; the two sibling cases already warn. The endpoint keeps its slot and stays `Active`. Reclaiming it would make endpoints disappear on a clock condition rather than an operator action, which is a larger promise than this needs to make, so `max_endpoints` accounting is unchanged and revoking is still how a slot is freed. Restore only, and the README says so. Clearing it the moment an expiry passes needs a clock-driven sweep mutating the registry from `poll()`, which is a mechanism rather than a fix. An endpoint that expires while the instance runs keeps its secret on disk until the next restart; revoking clears it at once. One interaction this created, caught by writing the round-trip test: with the secret gone, the next restore sees Bearer with no secret, which is exactly what `MissingSecret` reports. That would blame the credential for a refusal expiry already explains, so the admission check is skipped for an endpoint whose expiry has passed. Three tests, and the mutant that keeps the secret fails two of them while the unexpired case stays green.
`Published` exists so no reader can see the instance set and the routes disagree, and its doc comment says so. `handle_admin_health` loaded it twice anyway, once for the per-instance array and again for the status, so the two halves of one response could describe different publishes. It also read the clock once per instance and once more for the status. `handle_health` took one guard and its comment explained why, which made this the third fix-the-sibling case on the branch. The readiness predicate was written out verbatim in both handlers. It is now `Published::is_ready(now)`, which puts the rule on the value that already promises a consistent view and means the two endpoints cannot answer differently about the same listener. `/admin/health` now takes one snapshot and one clock read for the whole answer.
`Inadmissible::message()` returns `&'static str` so a 400 body costs no allocation, which means the three ceilings appear in it as literals with nothing tying them to `MAX_AUTH_SECRET_LEN`, `MAX_HMAC_HEADER_LEN` and `MAX_HMAC_PREFIX_LEN`. Raise a const and the operator is told the old number. The existing ceiling tests cannot catch that, and it is worth being precise about why: they build their oversized inputs from the consts, so raising one moves the test and the code together and leaves only the message lying. Confirmed by raising `MAX_HMAC_HEADER_LEN` to 512: the two existing hmac_header tests stayed green and only the new one failed. `management.rs` also hand-wrote "auth_secret must not be empty" three lines from the `is_usable` it shares with `admit_endpoint`. That check is `MissingSecret` under another name, so it now uses that variant's message and there is one wording instead of two.
The re-resolve narrowed the window where a handler enqueues into an instance that has left, but could not close it. The gate passes, and `enqueue` then checks `is_full`, builds the header map and copies the body before `try_send`, all of which `leave()` can overtake. A message landing after `leave()` counted the bridge is never drained, and the sender was told 200, which it has no way to detect. `leave()` now sets `departed` before it counts, and `enqueue` reads it after `try_send`. That is the only order that can observe the case: the flag has to be set before the count for a late message to be visible as late, and the check has to follow the send, because the send is what makes it late. The message stays in the channel. Depending on which side won, `leave()` may already have counted it in `dropped_on_close`, so that diagnostic can over-report by one. Better than the alternative, which is a sender that believes a lost request succeeded and never retries it. Disabling the check makes the test answer 200, which is the false success this closes.
The arm answers for two conditions, and the message I gave it was true of only one. The second lookup returns nothing when the path was withdrawn, and returns a different instance when one on the same listener has taken the path over. Calling both "instance is closing" describes a closure that has not happened in the handover case. `route unavailable` is true of both, and the caller's action is the same either way. It also says no more about the listener's topology than a retry needs, which is why the two are not split into separate messages. The admin API's "instance is closing" is unchanged, and so is the one `enqueue` answers once `leave()` has flagged the instance. Both of those really are closures.
…tate I misread this finding the first time and fixed the wrong thing. hubcio's point was about the canonical third state test telling authors to assert default state, and I changed the naming rule instead, then reported it done. The naming change was worth making on its own, but it was not this. The canonical `given_invalid_state_should_start_fresh` is right when the state is a cursor: discarding an undecodable one costs duplicates, which the at-least-once contract already permits. It is wrong when the absence of the state is a downgrade rather than a repeat. `http_source` keeps revocation tombstones there, so starting fresh would re-serve every revoked endpoint with its secret, and its `restore` returns `Err` instead. Both shapes are now named, with the reason for choosing between them and a worked refusal test. The rule that is not optional is the last line: never start fresh silently when the state that was discarded was load-bearing.
3a560e9 to
55a039d
Compare
The expiry clear runs during restore and touches the in-memory registry only. It arms no flush, so the state file keeps its copy of the secret until an unrelated registration or revocation writes the registry out, and an instance that sees neither keeps it on disk for good. The README promised the drop happened on the next restart, which reads as though expiry alone is enough and sends an operator away from the revoke that actually clears it. Arming the flag at the clear does not fix it either. Without a permit a quiet gateway still never flushes, and arming plus notifying would break the contract that a static-only instance writes no state file. So the documentation is the whole change here, both the Options row and the paragraph below it, plus the comment over the clear itself for the next person reading the code.
The earlier round asked for this check ahead of the header map and the body copy, and putting it inside `enqueue` bought both of those but not the read. By the time `enqueue` runs, `to_bytes` has already buffered up to `max_body_size_bytes` off the wire and the secret path has hashed all of it. With `Retry-After: 1` that has every sender re-upload and re-verify every body the gateway sheds, which is the opposite of what shedding is for. Both handlers now check once more before they read, at the first point where the instance is known. On the secret path that is necessarily before `authorize`, since the signature covers the body, so a caller holding a live endpoint id can learn the bridge is full without proving it holds the secret. An unknown id is already refused above and a wrong signature earns a 401, so this adds nothing to what such a caller can already tell apart. Everything else stays where it was. The gates that decide the outcome are the second resolve, `authorize`, `try_send` and the post-send departure check, and `enqueue` keeps its own check for a bridge that filled during the read. The metric, the log line and the 429 itself now come from one place so those four sites cannot drift.
Four copies at the moment of a send, not three. `postcard::to_allocvec` in the SDK shadows the polled `ProducedMessages` rather than consuming it, so that value lives to the end of the poll arm, past the callback and past the result await. It sits alongside the staged replay copy, the serialized bytes and the runtime's decoded copy. The earlier correction to this paragraph missed the same binding, so this is the second pass over the same count. The paragraph also carried no term for concurrent request bodies. Every in-flight POST buffers up to `max_body_size_bytes` before its message exists, and nothing here caps connections or concurrent reads, so at a large body cap that term is bigger than the bridge and the batches together. The bridge check that now runs before the read only sheds once the bridge is already full, so it does not bound this one. The reverse proxy is what does.
A shortened state blob decoded `Ok` with zero endpoints. `#[serde(default)]` on the registry supplied the missing map and `rmp_serde::from_slice` never checks that the decode consumed its input, so a blob that had lost every revocation tombstone was indistinguishable from a registry that never had one. The instance then served its TOML endpoints with their secrets, including the ones an operator had revoked for being compromised. The state now travels in a frame of its own, outside the registry type: a version, a declared endpoint count, and the endpoints, none of them defaulted, decoded through a `Cursor` so the consumed length can be compared against the blob. All three are load-bearing and none of them covers the others. Arity rejects the old one-element shape and anything shortened, the consumed length rejects a valid frame with more bytes behind it, and the declared count is the only thing that rejects a frame which is the right shape, fully consumed, and carrying a map somebody made smaller. The decode cannot be fixed in the SDK helper, because `position()` exists only on the `Cursor` flavour of the deserializer. Doing it now rather than later is deliberate. The crate is `publish = false` and no state file exists yet, so this costs nothing today. After a release the same change is a fail-closed boot for every instance unless a legacy-shape fallback goes in, and that fallback is the hole reopened. A zero-length file reaches the same fail-open with nothing corrupted: the runtime reports an empty state file as no state at all, so the guard above never runs. The plugin cannot tell that from a first boot, so it warns when it starts with static endpoints and no registry, and the README now says plainly that truncating the file is the same act as deleting it. The decode failure is also logged where it happens. `InitError` is substituted by the runtime before it reaches `last_error`, so the reason a registry was refused otherwise never leaves the process.
|
Round 6 from @numinnex: 12 findings, all 12 answered on their own threads. Detail is there; this is the short version. Six are fixed and pushed:
202 unit tests, and the restart integration tests pass on the new state format. Seven are accepted and queued, in this order: A sixth fix came out of the fifth. I swept the state decode rather than trusting the three repros — every single-byte mutation of a real blob plus every prefix — and found that 480 of those mutations edit a byte inside an endpoint id, yielding a well-formed frame still carrying its tombstone under a different id, so the original id serves again from TOML. I first wrote that off as a file integrity property needing its own mechanism and offered to file it separately. That was wrong. An id is written twice, as the map key and inside the record, and every writer keys an endpoint by its own id, so agreement is an invariant. To be explicit about the limit: this is corruption detection, not authentication. Anyone who can write the state file can write a consistent one, and it is a cleartext credential store, so they can already read every secret in it. Not requesting a re-review, the PR is already |
…a new id Framing the state file stopped a shortened blob decoding as an empty registry, but not a blob whose endpoint id was edited. That produces a frame which is the right shape, fully consumed and honest about its count, still carrying its tombstone, filed under an id one character away from the real one. The revoked endpoint then serves again from TOML with its secret, which is the same fail-open by a different route. The fix was already sitting in the file. An id is written twice, once as the map key and once inside the record, and every writer keys an endpoint by its own id, so the two agreeing is an invariant rather than a coincidence. `restore` used to resolve a disagreement by preferring the key, which silently repaired exactly the corruption worth refusing. The decode now rejects it instead, and a single edit cannot keep the two copies consistent. Swept the same way as the rest of the frame: every byte of a real blob against every value, plus every prefix. 480 of those mutations retarget a tombstone and the check catches all 480, so the sweep now asserts the tombstone survives rather than only that the endpoint count does. This is corruption detection and not authentication. Anyone who can write the file can write a consistent one, and can already read every secret in it, so there is nothing here that a checksum or a MAC would add.
…else Every refusal this connector can give at startup names its cause, and none of it reached an operator. The SDK's open shim returns 0 or 1 and drops the `Err` payload, and the runtime substitutes "Plugin initialization failed" for `last_error`, so all of `validate`'s messages, the field a joining instance disagrees on, a route conflict and the refusal to serve an undecodable registry were text nothing read. `open` now logs whatever it is about to return. One place rather than a line at each failure point, so the three cannot drift apart, and the log callback is installed before the plugin is built, so the line does reach the runtime's log even though the return value does not. Its body moved to `open_inner` for that. Two things follow from the message becoming visible. It used to carry `InitError`'s prefix twice, because `new` stored the restore failure as text and `open` wrapped the text in a second `InitError`; the field holds the error itself now. And the documentation said in five places that `last_error` carries the reason, which was the claim this whole path could not honour. Those now say the log carries it, and the README says so once as an operational note. Untouched on purpose: the save failure path really does set `last_error` to a real message, so what the README says about a failed flush stays true.
…ppened Two faults in the same state machine, both from reading the outcome of a poll instead of the event. An Ack cleared the flush backoff whoever earned it. Traffic knows nothing about the state store, so a batch of requests was zeroing a counter that only counts the store's refusals, and the delay for attempt zero is none, which is why the doubling to an eight second ceiling never engaged on a gateway that was serving anything. A store that keeps refusing then costs one failed write per poll. It is not an HTTP backend problem: the default file backend reports itself never latched, so a read-only mount or a full disk lands exactly here. `on_nack` already applied this rule and only the Ack side was missing it. The poll order flipped on whether a flush produced state rather than on whether the flush arm won. A flush that wins and then declines has still spent the poll: its permit is gone and `recv()` never ran. Not flipping meant the next poll was flush-first again, took the re-posted permit straight back, and declined again, which is an FFI round trip per iteration carrying nothing while queued traffic goes undrained. `take_dirty_state` declines whenever it loses its `try_lock` to an ordinary control-plane write, so a caller holding the registry writer across a clone and a validate is enough to sustain it. The backoff's own documentation claimed a store that stays latched ends in the SDK stopping the source. That is true only on an idle gateway. Every traffic Ack resets the SDK's consecutive counter, so under traffic the refusals are never consecutive and the source runs on indefinitely. Fixing the counter does not change that, so the comment now says which case ends and which does not. Both fixes have a test that fails without them. The poll-order one is driven by a surplus permit rather than a contended lock, because that reaches the same line without a guard held across an await.
…al reach Two ceilings that were not bounding what they claimed to. The admin listener was measured against `max_body_size_bytes`, which bounds an inbound webhook and has a floor of 1. So an operator who bounded webhooks tightly could not register or rotate at all, and the refusal named neither the field nor the cause. `ensure_compatible` refuses a join that disagrees on that knob, so it could not be raised for one instance to escape either, and what it blocked was replacing a secret that had leaked. The admin limit is its own const now, derived from the field caps rather than picked, so raising a cap cannot quietly reintroduce it. The original ask was only that the listener stop using axum's 2 MiB default silently, which sharing the webhook cap overshot. `MAX_ENDPOINTS` bounded dynamic registration and nothing else, so a TOML file or a state file could start an instance above a ceiling the README states as a per-instance property. Past the cap the arithmetic compounds: `try_insert` reclaims `len - MAX_ENDPOINTS + 1` tombstones to fit one endpoint, so an over-full registry discards revocation records in bulk. Nothing is resurrected, since static tombstones are never reclaimable, but the record of who was revoked and why goes. The two sources of an over-full registry get different answers on purpose. A TOML list over the cap fails `open()` naming the count, because that file is something an operator can edit before starting. A state file over the cap warns and serves, because clearing it costs every tombstone in it and failing would take the instance down over a condition with no supported way out. The 507's log line reports the length the registry actually holds rather than the ceiling, which was telling operators it was a size it was not.
A misspelled `auth_bearer_token` deserializes to `None` and the named-path
handler skips its whole auth block, so one typo serves `POST /topics/{path}` to
anyone. Nothing documented that, and `deny_unknown_fields` is not the answer:
the runtime delivers env overrides as flat top-level keys, so refusing unlisted
keys would break a documented path. The silence is by design and the design
needed writing down.
The Options row for the token now says a misspelled key reads as unset, and the
configuration section carries the general rule plus what to do about it, which
is to grep the open log for "with NO authentication" after any configuration
change.
`management_token` gets the same typo for a different outcome, and the note
says so rather than implying symmetry. It fails closed, and unlike the bearer
token nothing logs its posture, so what an operator sees is `/admin/endpoints`
answering 404 on the next registration.
A joining instance took every healthy sibling on its listener out of rotation. `join` published it while its poll task had not started, and the readiness gate asks whether every instance is polling, so `/health` answered 503 and `/admin/health` "degraded" for the whole listener until that instance's first poll. The window is the whole of `setup_source_producer`: an Iggy login, then a stream and topic ensure with retries. Restarting one instance therefore drained its siblings, and on boot the window stretched across other connectors' setup too. The gate could not tell the two cases apart because `poll_is_live` answers false both for a task that has stopped and for one that has not started, and those want opposite answers. An instance now records that it has ever polled, and readiness counts only those. A task that stops still takes the listener down, which is the case the gate exists for. Ignoring it would be a lie if it were serving meanwhile, so routes follow the same rule: they are projected only from instances that have polled, and an instance publishes its own on its first poll rather than in `join`. Before that its paths answer 404, which is the honest answer while nothing can drain them. A boot grace period would not do, since it answers 200 with no reader on the bridge. `publish` validates across every joined instance and serves the subset, so a route conflict is still refused at join rather than lying dormant until the offending instance first polls. The test fixtures now stand in for that first poll, because the runtime always starts a poll task after `open()` and nearly every request test had been relying on routes existing without one.
|
All twelve of round 6 are now fixed, plus one that came out of the sweep for A1. Every one has a reply on its own thread with the detail; this is the index.
209 unit tests and all 13 Every guard was checked by deleting it and watching a test go red, and each mutant was confirmed to compile first, because one that does not looks exactly like a test that passed. Three places where a test does not prove what it might appear to are called out on their threads rather than left to look like evidence: the E3 restore test pins a decision rather than verifying a fix, the 507 log line has no test because this crate has no log capture, and D1's unit test stands in for the first poll so the integration suite is what shows the production path firing. Four corrections to my own earlier claims are on the threads: the id retargeting I first called a separate issue, the |
The route publication added with the readiness fix returned quietly when the listener was not in the registry. That path is not reachable through the ordinary lifecycle, since the instance joined during `open`, an entry only leaves the map when its last instance does, and the SDK stops the poll task before `close`. The state it would describe is worth a line regardless: a poll task running against a listener that is no longer bound, serving nothing for as long as it lives, with no trace of why. `leave` documents its own unreachable early return for the same reason. This one now logs rather than only explaining itself, because unlike that one it leaves an instance in a state nothing else reports.
…or it Review of the readiness change found the flag being claimed before the await that publishes, and the gap between them reachable. The only suspension point is the `SERVERS` lock, and a poll dropped while waiting for it left the flag set with nothing published. The sibling whose publish the poll was queued behind then read that flag and served the cancelled instance's routes on its behalf, so `enqueue` answered 200 into a bridge with no reader while `/health` stayed green. Not a regression, since before the readiness change that state lasted the whole of producer setup, but the readiness comment claimed an absolute it did not have. Claiming after a successful publish cannot work, because the publish filter reads the flag. Claiming inside the guard does: the flip and the publish are now one critical section, and a poll dropped before the lock leaves the flag false so the next one retries. That also removes the test helper, which had been a second copy of the pairing `poll()` does. Every request test in the crate went through the copy, so deleting the publish from `poll()` left all of them green. They call the production function now, and one new test drives `poll()` itself and fails if that call goes away. `warn_if_poll_stopped` and `/admin/health` were still reading `poll_is_live` raw, so a management call against an instance that had not polled yet logged that its poll task looked stopped, and one health body could report `ok` beside `poll_is_live: false` with nothing to explain it. The warning now asks the question it means, and the health body carries `has_polled` alongside, so the two ways of being not-live are told apart.
The test I wrote for the framing truncated a blob by one byte, and truncation was never the hole. Verified against the pinned rmp-serde outside the tree: every one of the 92 prefixes of a valid old blob already failed on EOF, so that test passes at the parent commit and proves nothing about the frame. The docstring made the same mistake, calling a shortened blob one that decodes `Ok`. The shape that did fail open is `[0x90]`, an empty array, which the old single-field registry read through `#[serde(default)]` as a registry with no endpoints: a clean decode carrying zero tombstones. That is what the test restores from now, against a TOML file still declaring the endpoint whose tombstone went missing. The consumed-length check earns its own test too, for the case that is genuinely its alone. Rewriting a byte that opens the `state` map turns a tombstone back into an active endpoint and leaves the bytes describing the revocation unread behind the frame; arity, version and the declared count all pass it. The test sweeps every single-byte rewrite rather than pinning the one offset, so it keeps meaning something if the fixture changes. Also corrects the check count in three comments, which said three before the id agreement check made it four.
…t it caps Five documentation defects, each verified against the code rather than reasoned about. The README told operators a `management_token` typo is logged nowhere. It is logged on every listener bind, naming the key. I had checked with a grep that required the word and the log macro on one line, and the macro opens a line above the string, so the check could not have found it however wrong the claim was. It now says both typos announce themselves and what to grep for. The startup window is not an Iggy login. The login happens once before any source is initialised; what sits between `open` and a first poll is every source's producer setup and every sink's init, run in series, so on boot the window covers connectors this one has nothing to do with. The README and the comment both said the smaller, wrong thing. An instance name becomes a header value, not a key. Same 255 either way, since Iggy gives both the same ceiling, but the config check now validates it as what it becomes and the operator-facing error names the right one. The reason the cap is load-bearing is that the header is dropped rather than clamped, so a name past it takes the identity header off every message. `publish` carried two summary sentences with the stale one first, and listed three callers after the first poll became a fourth. `Published` still called its routes derived from its instance set, which stopped being true when they narrowed to the instances that can drain. The shutdown-loss note had its direction backwards: the count can only under-report, never double count, because the SDK joins the poll task before close runs. That comment predates this round and is corrected here rather than by rewriting a commit numinnex has already read. The admin body limit now covers the revoke reason too. Registration is still the largest legal body by an order of magnitude, but the derivation's whole claim is that no field cap can be raised out from under it, and one was not in the sum. The duplicated 255 is gone in favour of the documented constant that already existed. The early bridge check keeps its behaviour and gains the cost: answering without draining means hyper closes the read half, so above roughly 16 KiB a shed request burns its connection. The response still arrives. `Connection: close` was measured rather than assumed and makes it worse.
The decoder returned a formatted string, the only `Result<_, String>` in the crate and against the rule that says errors are enums. That mattered more than convention here: all five framing tests asserted a bare `is_err()`, so a blob refused by the wrong guard passed them, which is the failure this round was looking for everywhere else. A variant per check, and the tests name the one they mean. The ids in the misfiled variant are carried as `log_prefix` rather than whole, since a secret-path id is the credential and this text reaches a log line. Also fixes the ceiling test, which passed `None` for state and returned at the early exit before the branch its own comment described. It carries state now and reaches it. What it pins is still a decision rather than a fix, since nothing fails if the warning goes away, and the comment says so.
… than nothing Gating the management warning on `has_polled` removed a false alarm and the only signal for a real problem with it. The runtime brings every source up, then every sink, then the poll tasks, all in series, so one slow sink holds every instance of this connector before its first poll for an unbounded time. In that window a registration returns 201 with nothing logged, and on a listener where a sibling has polled the readiness gate filters the waiting instance out and answers green. The test added alongside that gate asserts the green, so the suite agreed with it. The message was the defect, not its existence. Calling an instance that has not started one whose poll task "looks stopped" was wrong; staying quiet was worse. Both conditions are reported now, in the words that fit them, and the one that means producer setup has not finished says so. The critical section move also had no test: reconstructing the old arrangement left the whole suite green, including the test that shipped with it. A poll cancelled while parked on the registry lock now asserts the instance was not claimed, and that fails against the old arrangement.
… a test that cannot fail Four claims, all added while fixing other wrong claims, which is its own lesson: prose has no compiler, so a number put in under time pressure survives a green suite. The admin limit is not larger than the next largest body by an order of magnitude. A rotation carrying a maximal secret is within a fifth of a registration, and rotation is what the same comment discusses three lines above. The unquantified version it replaced was true. The first-poll claim said a cancelled poll leaves the flag false so the next poll retries. There is no next poll: the only thing that drops that future is shutdown, and the SDK breaks its loop on the same branch. What the move actually buys is two things worth naming, one of which was missed entirely: a sibling can no longer serve a cancelled instance's routes, and the instance can no longer read as one whose poll task stopped, which used to take healthy siblings to 503. The readiness comment also credited the critical section for a guarantee that rests on `poll_active` instead. The shutdown-loss note was corrected in the wrong direction last round, on a finding I took without checking. `leave` sets the departed flag before reading the bridge length, so a message landing between them is counted while its sender is told 503: an over-report against that counter's description, which is what the comment said before I changed it. The reasoning is written out this time so the next reader can check it rather than flip it again. The declared-count comment kept the half that was wrong after its number was corrected. Version and the id agreement also reject valid, fully consumed blobs; what is unique to the count is noticing a map somebody made smaller. The tombstone-resurrection test is removed. It rewrote bytes to `0x00` against a fixture the full sweep already covers with every value, so it could not fail on its own, and it was the last framing test asserting less than the guard it named. The sweep says what it catches instead. Registering against an instance that has not polled now has a test and a line in the README. The endpoint is created and its path waits for the first poll, which no test covered because every fixture published routes immediately.
The comment said the response always arrives because the write half stays open. That was a raw socket's result read as a general one. A client that writes the whole body before reading can take the reset instead and never see the 429, and at this connector's default megabyte cap that is a real fraction of requests rather than an edge. `Retry-After` is guidance for the senders that receive it. The threshold was wrong too. It is not a body size: hyper's one drain attempt covers head and body together, lands near 32 KiB on a fresh connection, and slides lower on a reused one as its read strategy adapts, so a small body sent slowly enough loses the connection as well. Two more quantifiers that did not survive being checked. The startup window is not every source's producer setup, since the runtime does each source's open and its producer in one pass and only the ones it has not reached yet are still ahead; they come from an unordered map, so which ones is not fixed. And the new health field's pairs were written in an order that made the reachable combination read as the alarming one, so each names its field now. None of this changes behaviour. It is the fourth round of correcting things said about code that was already right, which is its own finding.
|
@numinnex I put the round 6 work through a two round adversarial review before asking you to look at it again. It found that the fixes themselves held up, and that several things I told you about them did not. Five corrections, all pushed. 1. My test for the state framing was vacuous. I told you the fix was covered by a test that truncated a blob. Truncation was never the hole: I rebuilt the pre-frame shape against the pinned rmp-serde and every one of the 92 prefixes already failed on EOF, so that test passes at the parent commit and proved nothing. The shape that actually failed open is The same review found what the trailing length check is really for, which I had not tested or described: rewriting the byte that opens the 2. I told you a 3. My description of the readiness window was wrong twice. There is no Iggy login in it; that happens once before any source is initialised. And the window is not one connector's producer setup: the runtime finishes the sources it has not reached yet, then every sink, in series, and only then starts any poll task. Which sources those are comes from an unordered map. You had told me the window spans other connectors and I dropped it. 4. The admin body limit did not cover every field cap, which was the whole claim. The revoke reason was not in the sum. Registration is still the largest legal body, but by about 15%, not the margin I implied. Both fixed. 5. Answering 429 before reading the body costs the connection, and sometimes the answer. Measured, not reasoned: hyper makes one drain attempt then closes the read half, at about 32 KiB of head plus body on a fresh connection and lower on a reused one. A sender that reads while writing usually still gets the 429. One that writes the whole body first can take a reset instead and never see it, which at the default 1 MiB cap is a real fraction of requests. The gate still does what you asked for, the upload really is cut off, and One more thing worth saying plainly. The second review round found that my fix for 212 unit tests and all 13 |
3 participants
Implements the webhook gateway accepted in #3039. Iggy currently has no way to receive a webhook; every provider that pushes events over HTTP needs a separate service in front whose only job is to accept a POST and republish it. This connector removes that hop.
Shape
One plugin
.sois loaded once regardless of how many[[source]]entries reference it, so the listener lives in a process-global registry keyed by listen address rather than on any single instance. The firstopen()binds the public and admin ports; later opens validate their body limit, admin address, management token and instance name against the running listener before joining; the lastclose()releases both ports, which the runtime's stop-then-start restart flow depends on. A single port can therefore serve many providers, each routed to its own topic.POST /topics/{topic_path}— named path, one per instance, guarded by an optional bearer tokenPOST /e/{endpoint_id}— secret path, 128 bits in the URL itself, with optional per-endpoint bearer or HMAC on topRequests resolve against an
ArcSwaproute table rebuilt whole on every control-plane change, so one atomic load yields both the endpoint's auth rules and the destination bridge. HMAC is verified over the raw body in constant time. Revoked endpoints answer 404 alongside paths that never existed, so a leaked URL cannot be used to confirm it was once live.Endpoints can be registered, re-keyed and revoked at runtime through a token-guarded API on the admin listener (absent entirely when no token is configured). Those endpoints ride the SDK's
ConnectorState, and revocation writes a tombstone that outranks TOML on restore, so a stale config file cannot resurrect an endpoint an operator revoked.Delivery semantics
Best-effort in both directions, and the README leads with it rather than burying it. HTTP 200 means accepted into an in-memory buffer, not durably stored; both the loss and duplicate windows are enumerated with what mitigates each. A full bridge answers 429 with
Retry-Afterrather than blocking.Depends on #3795 for the full backpressure story. The bridge is bounded today, so 429 fires on an arrival burst the poll loop cannot keep up with. What is missing is the coupling: until the bounded runtime forwarding channel lands,
poll()drains into an unbounded channel, so a slow Iggy does not propagate back into 429. The README documents this rather than implying the chain is complete.State is attached only to an empty batch. The runtime saves state solely on the success branch of the Iggy send, and an empty send always succeeds, so a management mutation cannot be lost to an unrelated send failure.
Deviations from the design doc, all deliberate
topic_pathandinstance_nameare explicit config fields. Onlyplugin_configcrosses the FFI, so the plugin cannot see its connector key or its[[streams]]entry. Same resolution the design already accepted for the named path.GET /healthwhen no instance is joined.{timestamp}.{body}behind a compound header and Twilio signs URL plus sorted params as base64. The design's example config showed Stripe working; it would not have. Documented with the forward-and-verify-downstream workaround, and the shipped example uses bearer instead.schema = "raw"is mandatory and now stated as such — the connector always produces raw bodies, and a JSON encoder rejects every message.Testing
110 unit tests and 5 integration tests. The integration suite needs no containers, since the connector is itself the HTTP server and the test client is the webhook sender: it covers a signed POST reaching Iggy byte-for-byte with headers intact, two instances sharing one listener, the register/POST/revoke flow, a dynamic endpoint with its secret surviving a connector restart, and a revoked static endpoint staying dead across a restart that re-reads the TOML still declaring it.
No integration test for 429 under saturation: with a healthy Iggy the poll loop drains the bridge continuously, so provoking a full bridge from outside races the drain. Two unit tests cover it deterministically instead.
Verification
cargo fmt,cargo sort --check --no-format --workspace,cargo clippy --all-features --all-targets -- -D warnings(connector and integration),cargo test,taplo fmt --check,hawkeye check, markdownlint, and the trailing-whitespace/newline scripts all pass locally.