Merge r8.0 into v2: retry budgets, security audit, and redesign follow-ups - #255
Merged
Conversation
Contains and Regex matchers now apply to bad results, not just errors, and groups that can never fire are rejected on save.
…hers Fix bad-result retries never matching
The group total was tracked in a dictionary carried on each xchange, so every failing message started from zero and got its own full budget: 4 messages under a total of 10 produced 12 retries. It also always equalled the per-message attempt count, so the cap could never fire above MaxAttemptsPerError. The total now lives in a RetryGroupUsage table keyed by integration + group, and the evaluator claims from it via IRetryGroupBudget. Dry-runs use an in-memory implementation so simulating never spends a real budget. GroupAttemptCounts is dropped from Xchange and DelayedRetry. The total never resets on its own, so /usage reports what each integration has spent and /resetusage clears it. XchangeResult now records why a retry was refused, since a group with an exhausted budget was previously indistinguishable from one that never matched.
Claim a group slot with one conditional UPDATE instead of read-then-write, so concurrent failures across instances cannot both take the last slot. The first row is inserted on its own context, falling back to the increment if that race is lost. Guard retry evaluation so a throw there can no longer replace the original exception and discard the XchangeResult. Clear a group's usage rows when it is removed from a policy or the policy is deleted, and require Admin or Member to read usage.
…-cap Enforce MaxAttemptsTotal across messages instead of per message
Patch is the only update class that auto-merges with no human involved, yet carried the shortest cooldown (1 day, a third of GitHub's 3-day default). Soak time should scale with how little scrutiny a bump receives, not with how breaking semver claims it is. Costs nothing in security terms: cooldown never applies to Dependabot security updates, which still fire immediately. Propagated from simplify9/.github (dependabot-templates).
Account lockout with admin unlock, admin-only account listing, always-Secure refresh cookie, tightened CORS fallback, and hardened response headers (COOP, X-Permitted-Cross-Domain-Policies, Cache-Control, Clear-Site-Data on logout).
…curity-audit-r8 # Conflicts: # SW.Bitween.PgSql/Migrations/BitweenDbContextModelSnapshot.cs
Hamza/fix/security audit r8
Sent once per subscription and group, resolved policy -> group -> subscription override, and delivered through a new native SMTP handler on its own bus queue.
Hamza/feature/retry budget alerts
Inline policies had no id, so their counters were unreachable by every existing endpoint. Shared row builder extracted rather than copied.
Drops the retry and records why on the exchange instead of throwing, drains in batches, survives a missing subscription, and refuses a group that allows retries with no budget.
A retry someone starts by hand no longer spends the group's shared total, and a subscription's next success gives its spent budget back. Adapter dispatch now goes through one helper instead of three copies.
MailKit hard-fails when the CA's OCSP or CRL server is unreachable, which stopped a valid Gmail certificate from ever delivering an alert.
A partly-spent total is left alone, so a downstream that fails some messages and succeeds others still reaches its cap, and a slot charged after the run began is no longer handed back. A dropped retry now records why in every case that has an exchange to record it on.
Replaces the blanket opt-out: revocation checking stays on, and only an undeterminable status is soft-failed. A revoked certificate, an untrusted root, a wrong hostname and an expired certificate all still fail.
…lowups Keep the retry budget honest about manual retries and recoveries
Retry budgets and the security audit, onto the redesign. Conflicts were the permission model replacing EnsureAccess, the Xchange constructor, and the subscription applier — r8's new retry validation kept in all three.
…nforces Create skipped the case-insensitive name check, the promoted-property validation, and the newer document fields — all already required on update. The dialog stayed open after create as a workaround for the promoted properties it couldn't carry; now create takes the whole definition in one save and the dialog just closes.
Receiving, GatewayApiCall, ApiCall and BusGateway subscriptions all run
through SubmitSubscriptionXchange, started by name from their own entry
point — a schedule, a gateway call. Auto-matching them too ran them a
second time on traffic addressed to nobody: a scheduled job publishing
the very message type it's bound to fed itself forever, and an ApiCall
integration ran on another partner's message. Both stayed hidden only
while those handlers happened to be unreachable.
That second run also arrived without the partner the entry point would
have passed, so every {{partner.…}} in its adapters stayed a literal
token instead of resolving. Xchange creation now falls back to the
subscription's own partner when none is given explicitly.
An absent list means none, as it already does for a document's promoted properties and a retry policy's groups — left implicit here it threw ArgumentNullException instead.
No delete endpoint existed for notifiers at all — the permission catalog said so outright. No reference check needed: nothing has a foreign key to a notifier, RunOnSubscriptions points the other way.
It hands the response to exactly one integration with the bus skipped: nothing published, no filter consulted, nothing else bound to that information type hears it — publishing does all of that, which is the reason the bus is here. Kept visible and clearable for configuration that already depends on it, but no longer offered for anything new.
Routes, attachments, a response target and an aggregation target are all RESTRICT foreign keys, so the delete was already refused — as a 500 from a foreign key violation, telling the operator nothing about which route or gateway is holding it. Checked and named up front instead. Exchanges are deliberately not checked: their reference is nullable, and history isn't a reason to keep configuration alive.
Active, Paused, Idle, a failure count, an exchange outcome, a queue's severity — every one of these read as a bare word with no explanation of what put it there. Flow map nodes had the same gap for the loop and warning icons, and lost the loop explanation entirely whenever a node also carried a warning.
listX() calls across partners, information types, integrations, work groups, API/bus gateways, retry policies and notifiers fetched everything and filtered client-side — Work groups broke past 20 rows, and notifiers fetched every row's full detail one at a time to build a list. All nine now page and filter server-side through the existing Searchy endpoints, plus a new paged/searched endpoint for one gateway's attachments. Notifiers' search row also carries RunOnSubscriptions directly, closing the N+1 that full-detail fetch was covering for.
… inline Attaching a partner or adding a route used to need the integration to already exist — a modal dialog on the bus gateway canvas, or a plain picker on the API gateway side, either way a worse copy of the integration's own pipeline for anyone who wanted more than a name and a delivery. Both now offer "define it here" instead: the full studio pipeline, staged in the same request and committed in one transaction with whatever points at it, so a failure can't leave an integration nothing points at, or a route pointing at nothing.
Two problems on a Receiving integration's page: the scheduler's own run history always reports success, since ReceivingJob catches every exception from the receive step itself and Quartz never sees it — so "34 failures" on the record never matched what the exchange list showed. And the schedule's next-fire estimate only advanced on a successful run, freezing in the past forever once a receiver started failing, even though the job kept firing on schedule underneath it. ReceivingJob now writes its own ReceiveAttempt row on every run, success or failure, with the exchanges it produced — paged and filterable on the integration's page, retained on the same cleanup schedule as the scheduler's own history. And the schedule now advances regardless of outcome, isolated from the receive step so a bad schedule can't block a receive that would otherwise have worked.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (8)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: simplify9/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
2 participants
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.
Summary
{{partner.…}}tokens unresolved.ReceiveAttempt), independent of Quartz's own history which always reports success.Test plan
dotnet build(SW.Bitween.Web) — cleanyarn build(tsc + vite) — cleandotnet testSW.Bitween.UnitTests — 208 passeddotnet testSW.Bitween.IntegrationTests — 77 passed