fix(trogon-gateway): hardcode 1 MiB server limit to avoid async-nats race#119
Merged
fix(trogon-gateway): hardcode 1 MiB server limit to avoid async-nats race#119
Conversation
PR SummaryMedium Risk Overview Adds debug-level tracing in Introduces fixture-based Linear webhook tests (with new JSON fixtures) to verify subject routing, payload preservation, and Reviewed by Cursor Bugbot for commit 1974b42. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughGateway now derives NATS max-payload from resolved config rather than server info; claim-check publishing adds detailed debug logs; Linear webhook handler records request body size in tracing spans; new Linear webhook fixtures and integration tests added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 1974b42 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-gateway/src/main.rs (1)
75-77: Consider making the temporary payload fallback configurable.A fixed
1 MiBstand-in can still misclassify messages on clusters configured with a lower servermax_payload. An optional config/env override (defaulting to 1 MiB) would reduce that risk while the async-nats race is unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/src/main.rs` around lines 75 - 77, The temporary hardcoded fallback of 1 MiB uses the local variable server_max_payload and is passed to MaxPayload::from_server_limit, which should be configurable; read an optional config/env override (e.g., SERVER_MAX_PAYLOAD_FALLBACK) and parse it to set server_max_payload (defaulting to 1024 * 1024 if unset/invalid), then pass that value into MaxPayload::from_server_limit where max_payload is created so deployments can lower the temporary fallback without changing the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/trogon-gateway/src/main.rs`:
- Line 74: Replace the placeholder tracking issue URL in the comment "//
Tracking issue: https://github.com/TrogonStack/trogonai/issues/TODO" with a
real, actionable issue link or remove the line; locate that exact comment string
in main.rs (top-level comment near the crate header) and update it to point to
the correct GitHub issue or a link to your project board so the tracking
reference is usable.
In
`@rsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-1315-71e1-9e58-50f600aab7c8.json`:
- Line 1: The fixture contains real PII in JSON fields; replace any
human-identifying values under "actor", "assignee", "creatorId", "assigneeId",
and "subscriberIds" and any direct profile URLs with anonymized/synthetic values
(e.g., use fake names like "Test User", emails like "test+user@example.com", and
avatarUrl/url values pointing to a non-identifying placeholder), and ensure
fields "email", "name", "avatarUrl", "url", and any GUID-like IDs you cannot
regenerate are either anonymized or replaced with stable test IDs; update the
"actor" and "assignee" objects and any top-level URLs in the JSON (search for
keys "actor", "assignee", "creatorId", "subscriberIds", "avatarUrl", "email",
"url") and commit the sanitized fixture.
In
`@rsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-2ab6-79f2-b417-b26f6ef51798.json`:
- Line 1: The fixture contains real PII in fields like actor.name, actor.email,
actor.avatarUrl, actor.url, assignee.name, assignee.email, avatarUrl, and any
subscriber/creator IDs embedded as contact info; replace those with
deterministic synthetic values while preserving IDs and payload shape (e.g., set
names to "Test User", emails to "user+<actor.id>@example.com", avatarUrl/profile
URLs to a non-production placeholder URL, and keep all UUIDs, numeric fields,
arrays, and keys unchanged); ensure description/descriptionData and all routing
fields (actor.id, assigneeId, creatorId, subscriberIds, teamId, stateId,
labelIds) remain intact so tests still exercise the same paths.
In
`@rsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-3853-7f60-be17-e7e9e548acb7.json`:
- Line 1: The fixture contains real PII in fields under "actor", "assignee", and
creator/subscriber fields (e.g., actor.name, actor.email, actor.avatarUrl,
actor.url, assignee.name, assignee.email, assignee.avatarUrl, assignee.url,
creatorId, subscriberIds); update the JSON to redact/anonymize those values
(replace names with "Test User", emails with "user@example.com", avatarUrl/url
with a placeholder image/profile URL or remove them, and optionally replace real
GUIDs with synthetic IDs) so the fixture contains no production-identifiable
data while preserving schema and keys like "assignee", "actor", "creatorId", and
"subscriberIds".
---
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/main.rs`:
- Around line 75-77: The temporary hardcoded fallback of 1 MiB uses the local
variable server_max_payload and is passed to MaxPayload::from_server_limit,
which should be configurable; read an optional config/env override (e.g.,
SERVER_MAX_PAYLOAD_FALLBACK) and parse it to set server_max_payload (defaulting
to 1024 * 1024 if unset/invalid), then pass that value into
MaxPayload::from_server_limit where max_payload is created so deployments can
lower the temporary fallback without changing the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e43d46c1-f956-402c-a479-180f30469634
📒 Files selected for processing (24)
rsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-nats/src/jetstream/claim_check.rsrsworkspace/crates/trogon-source-linear/src/server.rsrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Comment.create__019d814d-a5a7-7e33-9b69-ee736af5afea.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Comment.create__019d814d-b505-71c2-ac44-06f7f78fffc4.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Comment.create__019d814d-f983-7152-b1ee-d910e7e77048.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Comment.create__019d8434-663b-7eb0-85ba-0b7b5b611645.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.create__019d814e-13ec-7911-bbcc-5dadd1d53273.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-b876-7e02-9c0c-3d39d4a0f77c.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-bb7a-78e2-aba2-527c675994c4.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-be7d-70c0-9578-5d6eb18fefdb.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-bf6a-7bb1-9420-c0768e1e8af8.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-bfad-7b60-8fc4-7773cfd2b133.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-ef1e-7db1-9a2a-cc152e45844d.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-f469-7470-9e01-6cd963190827.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-1164-75d2-a285-38e240ced9b2.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-12a9-7f53-b3f7-185229a046bd.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-1315-71e1-9e58-50f600aab7c8.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-145b-7f03-b904-f00a96e43a71.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-151a-7543-978b-ca61e2c4fa9f.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-2ab6-79f2-b417-b26f6ef51798.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814e-3853-7f60-be17-e7e9e548acb7.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.IssueLabel.update__019d814e-135e-7583-9a29-518b050d879c.jsonrsworkspace/crates/trogon-source-linear/tests/webhook_fixtures.rs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3441ead. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-gateway/src/config.rs (1)
125-128: Add focused config tests for default/override and zero rejection.This new env-backed control path should have regression tests for: default
1_048_576, explicit override, and invalid0handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/src/config.rs` around lines 125 - 128, Add unit/integration tests that exercise the env-backed nats_max_payload_bytes config: write tests that (1) do not set TROGON_GATEWAY_NATS_MAX_PAYLOAD_BYTES and assert the loaded Config.nats_max_payload_bytes == 1_048_576, (2) set TROGON_GATEWAY_NATS_MAX_PAYLOAD_BYTES to a nonzero value and assert the override is picked up, and (3) set it to "0" and assert config loading fails (returns an Err or validation error) rather than accepting zero. Use the same config loader used in this crate (e.g. Config::from_env or the crate's config load function), manipulate the environment only for the duration of each test, and clean up/reset the env between cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 127-128: The nats_max_payload_bytes field currently allows zero;
make zero unrepresentable by changing the field to a constrained type (e.g.,
std::num::NonZeroUsize or a custom newtype like NatsMaxPayloadBytes) and update
the config resolution path (the resolve(...) code that constructs the config) to
validate and reject 0 explicitly if you cannot use NonZeroUsize; ensure any
parsing/Env deserialization of TROGON_GATEWAY_NATS_MAX_PAYLOAD_BYTES fails with
a clear error when value == 0 and propagate that error instead of storing 0 so
callers of nats_max_payload_bytes always see a non-zero value.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 125-128: Add unit/integration tests that exercise the env-backed
nats_max_payload_bytes config: write tests that (1) do not set
TROGON_GATEWAY_NATS_MAX_PAYLOAD_BYTES and assert the loaded
Config.nats_max_payload_bytes == 1_048_576, (2) set
TROGON_GATEWAY_NATS_MAX_PAYLOAD_BYTES to a nonzero value and assert the override
is picked up, and (3) set it to "0" and assert config loading fails (returns an
Err or validation error) rather than accepting zero. Use the same config loader
used in this crate (e.g. Config::from_env or the crate's config load function),
manipulate the environment only for the duration of each test, and clean
up/reset the env between cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a083c97c-4481-4b8a-b1d9-ab64d1c870a4
📒 Files selected for processing (2)
rsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rsworkspace/crates/trogon-gateway/src/main.rs
yordis
force-pushed
the
fix-linear
branch
7 times, most recently
from
fddac3d to
a8afecd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rsworkspace/crates/trogon-source-linear/tests/webhook_fixtures.rs (2)
88-90: Assert message count before indexingmessages[0].Several tests index directly into the first publish result; if publish fails, that panics with a less-informative error. Add
assert_eq!(messages.len(), 1)before first access in each case.🧪 Representative change
let messages = publisher.published_messages(); + assert_eq!(messages.len(), 1); let published: serde_json::Value = serde_json::from_slice(&messages[0].payload).expect("valid JSON");Also applies to: 110-113, 144-147, 182-184, 219-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-linear/tests/webhook_fixtures.rs` around lines 88 - 90, Add an explicit assertion that the publisher produced exactly one message before indexing into messages[0]; after calling publisher.published_messages() (the messages variable) insert assert_eq!(messages.len(), 1) immediately prior to any use of messages[0] (e.g., before the serde_json::from_slice(&messages[0].payload) call and the other occurrences in the file) so tests fail with a clear assertion message instead of an out-of-bounds panic.
15-16: Use a realistic test constant for NATS claim check threshold instead ofusize::MAX.Tests using
usize::MAXforMaxPayload::from_server_limit()don't exercise the actual boundary conditions. Align this with the 1 MiB fallback used elsewhere in the codebase (e.g., Slack source defaults to 1 MiB).Suggested change
const TEST_SECRET: &str = "test-secret"; +const TEST_SERVER_MAX_PAYLOAD_BYTES: usize = 1024 * 1024; ... let cc = ClaimCheckPublisher::new( publisher.clone(), MockObjectStore::new(), "test-bucket".to_string(), - MaxPayload::from_server_limit(usize::MAX), + MaxPayload::from_server_limit(TEST_SERVER_MAX_PAYLOAD_BYTES), );Also applies to: 32-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-linear/tests/webhook_fixtures.rs` around lines 15 - 16, Replace the unrealistic usize::MAX used in webhook_fixtures.rs for the NATS claim check threshold with a realistic 1 MiB value (1 * 1024 * 1024) so MaxPayload::from_server_limit() exercises real limits; update the test constant(s) used around TEST_SECRET and any calls to MaxPayload::from_server_limit() in that file (including the occurrences around lines 32–37) to use the 1 MiB fallback consistent with the Slack source default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rsworkspace/crates/trogon-source-linear/tests/webhook_fixtures.rs`:
- Around line 88-90: Add an explicit assertion that the publisher produced
exactly one message before indexing into messages[0]; after calling
publisher.published_messages() (the messages variable) insert
assert_eq!(messages.len(), 1) immediately prior to any use of messages[0] (e.g.,
before the serde_json::from_slice(&messages[0].payload) call and the other
occurrences in the file) so tests fail with a clear assertion message instead of
an out-of-bounds panic.
- Around line 15-16: Replace the unrealistic usize::MAX used in
webhook_fixtures.rs for the NATS claim check threshold with a realistic 1 MiB
value (1 * 1024 * 1024) so MaxPayload::from_server_limit() exercises real
limits; update the test constant(s) used around TEST_SECRET and any calls to
MaxPayload::from_server_limit() in that file (including the occurrences around
lines 32–37) to use the 1 MiB fallback consistent with the Slack source default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90b11344-ca8a-47cf-b8db-8aadcb525290
📒 Files selected for processing (10)
rsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-nats/src/jetstream/claim_check.rsrsworkspace/crates/trogon-source-linear/src/server.rsrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Comment.create__019d8434-663b-7eb0-85ba-0b7b5b611645.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.create__019d814e-13ec-7911-bbcc-5dadd1d53273.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-b876-7e02-9c0c-3d39d4a0f77c.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-bf6a-7bb1-9420-c0768e1e8af8.jsonrsworkspace/crates/trogon-source-linear/tests/fixtures/linear.IssueLabel.update__019d814e-135e-7583-9a29-518b050d879c.jsonrsworkspace/crates/trogon-source-linear/tests/webhook_fixtures.rs
✅ Files skipped from review due to trivial changes (7)
- rsworkspace/crates/trogon-source-linear/src/server.rs
- rsworkspace/crates/trogon-gateway/src/main.rs
- rsworkspace/crates/trogon-source-linear/tests/fixtures/linear.IssueLabel.update__019d814e-135e-7583-9a29-518b050d879c.json
- rsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.create__019d814e-13ec-7911-bbcc-5dadd1d53273.json
- rsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Comment.create__019d8434-663b-7eb0-85ba-0b7b5b611645.json
- rsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-bf6a-7bb1-9420-c0768e1e8af8.json
- rsworkspace/crates/trogon-source-linear/tests/fixtures/linear.Issue.update__019d814d-b876-7e02-9c0c-3d39d4a0f77c.json
🚧 Files skipped from review as they are similar to previous changes (1)
- rsworkspace/crates/trogon-nats/src/jetstream/claim_check.rs
…ts race When `retry_on_initial_connect` is set, async-nats returns the `Client` before the background connection task populates the `ServerInfo` watch channel, so `server_info().max_payload` is always 0 immediately after connect — collapsing the claim-check threshold to 0 and routing every payload through the object-store path. `trogon_nats::connect` always sets `retry_on_initial_connect`, making this race deterministic in production. Workaround: read the limit from `TROGON_GATEWAY_NATS_MAX_PAYLOAD_BYTES` (default 1 MiB) instead of querying the server. The original line is kept as a comment pending resolution of the upstream race. Tracking issue: #122 Also adds OTel instrumentation to `ClaimCheckPublisher::publish_event` and the Linear webhook handler so the claim-check path decision is observable in traces and structured logs, and adds 20 production fixture payloads (PII-sanitised) with integration tests for the Linear source. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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
1 participant
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.
trogon_nats::connectalways setsretry_on_initial_connect, which causes async-nats to return theClientbefore the background task that populates theServerInfowatch channel has run —server_info().max_payloadimmediately afterconnect()therefore returns0(theDefaultvalue forusize)MaxPayload::from_server_limit(0)saturates to a threshold of0, routing every payload through the object-store claim-check path regardless of size; this was confirmed by inspecting the productionOBJ_trogon-claimsbucket, which contained all Linear webhook payloads (1.5–2.8 KiB) that should have been published inlineserver_info()is populated before returning theClient, ortrogon_nats::connectexposes a way to read server info post-connection accurately; the TODO in the code tracks what needs to be restoreddebug-level OTel instrumentation toClaimCheckPublisher::publish_eventrecordingmessaging.message.body.size,trogon.claim_check.used,trogon.claim_check.threshold_bytes, andtrogon.claim_check.keyso the path decision is observable in traces going forwardmessaging.message.body.sizeto thelinear.webhookspan and logsserver_max_payload_bytes/claim_check_threshold_bytesat gateway startup