Move PostJSONWithTimeout to platform/http package and activity cleanup - #40561
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40561 +/- ##
==========================================
- Coverage 66.29% 66.29% -0.01%
==========================================
Files 2466 2467 +1
Lines 197506 197488 -18
Branches 8655 8655
==========================================
- Hits 130935 130920 -15
+ Misses 54727 54723 -4
- Partials 11844 11845 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
WalkthroughThis PR refactors the activity service architecture by removing the injected webhook send function from the activity bootstrap layer. The Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
server/platform/arch_test.go (1)
74-82: Comment label "Other infra packages" is slightly misleading
pkg/fleethttp(line 80) is grouped under "Other infra packages" but appears after the "Platform packages" block, which is fine structurally. Just a minor readability note — consider renaming the comment to "Infra packages" (matching the convention used inTestPlatformPackageDependenciesandTestEndpointerPackageDependencies) for consistency.✏️ Suggested comment rename
- // Other infra packages + // Infra packages m+"/pkg/fleethttp",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/platform/arch_test.go` around lines 74 - 82, Rename the inline comment "Other infra packages" above the IgnoreDeps block to "Infra packages" for consistency with TestPlatformPackageDependencies and TestEndpointerPackageDependencies; update the comment that precedes the m+"/pkg/fleethttp" entry in the ShouldNotDependOn(...).IgnoreDeps(...) section so it reads "Infra packages" and leaves the rest (including m+"/server/platform/errors" and m+"/server/platform/http" and m+"/pkg/fleethttp") unchanged.server/activity/internal/service/new_activity_test.go (1)
218-221: Remove duplicate test helper.
newTestServiceWithWebhookis identical tonewTestService; reusing the existing helper keeps this file easier to maintain.♻️ Proposed simplification
-// newTestServiceWithWebhook creates a service configured for webhook delivery tests. -func newTestServiceWithWebhook(ds types.Datastore, providers activity.DataProviders) *Service { - return NewService(&mockAuthorizer{}, ds, providers, slog.New(slog.DiscardHandler)) -} @@ - svc := newTestServiceWithWebhook(ds, providers) + svc := newTestService(ds, providers) @@ - svc := newTestServiceWithWebhook(ds, providers) + svc := newTestService(ds, providers)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/activity/internal/service/new_activity_test.go` around lines 218 - 221, The helper newTestServiceWithWebhook duplicates newTestService; delete the newTestServiceWithWebhook function and update any tests referencing it to call the existing newTestService helper instead (search for usages of newTestServiceWithWebhook and replace them with newTestService), keeping the same arguments and ensuring imports/constructor call remain identical (NewService, mockAuthorizer{}, slog.New(slog.DiscardHandler), etc.).server/platform/http/post_json.go (1)
39-39: A new HTTP client is created on every call, bypassing connection pooling.
fleethttp.NewClientallocates a fresh transport and connection pool each invocation. For a function that may be called at high frequency (e.g., per-activity webhook dispatch), this prevents keep-alive reuse and exhausts ephemeral ports under load. Consider initialising a package-level client once (or accepting a client as a dependency).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/platform/http/post_json.go` at line 39, The code currently calls fleethttp.NewClient on every invocation (client := fleethttp.NewClient(...)), which creates a fresh transport and prevents connection pooling; change this to reuse a single client by either initializing a package-level variable (e.g., var defaultClient = fleethttp.NewClient(fleethttp.WithTimeout(30*time.Second))) and using that instead of per-call client, or modify the PostJSON function signature to accept an injected *http.Client/fleethttp.Client and use the injected instance; update all call sites accordingly and remove the per-call fleethttp.NewClient allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/activity/internal/service/new_activity_test.go`:
- Around line 344-346: The test's async wait uses a fixed 1s timeout in the
select (the time.After(1 * time.Second) branch that calls t.Error("timeout
waiting for webhook")), which is too short and causes flakiness; update that
timeout to a larger value (e.g., 5 * time.Second) or replace with a named
constant (e.g., webhookWaitTimeout) and use that so CI/slower environments have
more headroom, keeping the rest of the select logic unchanged.
In `@server/platform/http/post_json.go`:
- Around line 54-58: The code currently calls io.ReadAll(resp.Body) which can
buffer arbitrarily large responses; replace this with reading from
io.LimitReader(resp.Body, 513) (or similar limit) to cap the amount read, handle
the returned error instead of ignoring it, and then convert the limited buffer
to bodyStr and truncate to 512 bytes if needed; update the logic around
resp.Body, io.ReadAll and bodyStr to use the limited reader and proper error
handling.
---
Nitpick comments:
In `@server/activity/internal/service/new_activity_test.go`:
- Around line 218-221: The helper newTestServiceWithWebhook duplicates
newTestService; delete the newTestServiceWithWebhook function and update any
tests referencing it to call the existing newTestService helper instead (search
for usages of newTestServiceWithWebhook and replace them with newTestService),
keeping the same arguments and ensuring imports/constructor call remain
identical (NewService, mockAuthorizer{}, slog.New(slog.DiscardHandler), etc.).
In `@server/platform/arch_test.go`:
- Around line 74-82: Rename the inline comment "Other infra packages" above the
IgnoreDeps block to "Infra packages" for consistency with
TestPlatformPackageDependencies and TestEndpointerPackageDependencies; update
the comment that precedes the m+"/pkg/fleethttp" entry in the
ShouldNotDependOn(...).IgnoreDeps(...) section so it reads "Infra packages" and
leaves the rest (including m+"/server/platform/errors" and
m+"/server/platform/http" and m+"/pkg/fleethttp") unchanged.
In `@server/platform/http/post_json.go`:
- Line 39: The code currently calls fleethttp.NewClient on every invocation
(client := fleethttp.NewClient(...)), which creates a fresh transport and
prevents connection pooling; change this to reuse a single client by either
initializing a package-level variable (e.g., var defaultClient =
fleethttp.NewClient(fleethttp.WithTimeout(30*time.Second))) and using that
instead of per-call client, or modify the PostJSON function signature to accept
an injected *http.Client/fleethttp.Client and use the injected instance; update
all call sites accordingly and remove the per-call fleethttp.NewClient
allocation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
cmd/fleet/serve.goserver/activity/arch_test.goserver/activity/bootstrap/bootstrap.goserver/activity/bootstrap/testing.goserver/activity/internal/service/new_activity.goserver/activity/internal/service/new_activity_test.goserver/activity/internal/service/service.goserver/activity/internal/service/service_test.goserver/activity/internal/tests/suite_test.goserver/activity/providers.goserver/contexts/ctxerr/ctxerr.goserver/contexts/logging/logging.goserver/datastore/mysql/in_house_apps_test.goserver/datastore/mysql/testing_utils.goserver/fleet/activities.goserver/fleet/datastore.goserver/fleet/errors.goserver/platform/arch_test.goserver/platform/errors/errors.goserver/platform/http/errors.goserver/platform/http/post_json.goserver/platform/mysql/errors.goserver/service/activities_test.goserver/service/testing_utils.goserver/utils.go
💤 Files with no reviewable changes (6)
- server/activity/providers.go
- cmd/fleet/serve.go
- server/service/activities_test.go
- server/service/testing_utils.go
- server/activity/bootstrap/testing.go
- server/fleet/activities.go
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the activity bounded-context migration by moving JSON POST helper logic into the platform layer, extracting transport-agnostic error classification into a dedicated platform package, and simplifying activity bootstrap wiring while adjusting tests accordingly.
Changes:
- Moved
PostJSONWithTimeout(and URL masking helpers) toserver/platform/httpand kept deprecated aliases inserver/. - Introduced
server/platform/errorsfor transport-agnostic error classification (Cause,NotFoundError,IsNotFound,ErrWithIsClientError) and updated call sites. - Removed injectable webhook send function from the activity service/bootstrap and migrated webhook tests into the activity bounded-context service tests.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/utils.go | Replaced in-package implementations with deprecated aliases to server/platform/http. |
| server/service/testing_utils.go | Updated activity bootstrap wiring to new activity_bootstrap.New signature (no webhook send fn). |
| server/service/activities_test.go | Removed service-layer webhook tests (now covered in activity service tests). |
| server/platform/mysql/errors.go | Switched NotFoundError interface dependency from platform/http to platform/errors. |
| server/platform/http/post_json.go | New home for PostJSONWithTimeout. |
| server/platform/http/errors.go | Removed transport-agnostic primitives and switched to platform/errors for Cause. |
| server/platform/errors/errors.go | New transport-agnostic error classification primitives package. |
| server/platform/arch_test.go | Updated architecture dependency rules for new platform packages/deps. |
| server/fleet/errors.go | Re-aliased ErrWithIsClientError and Cause to platform/errors. |
| server/fleet/datastore.go | Re-aliased NotFoundError/IsNotFound to platform/errors. |
| server/fleet/activities.go | Removed legacy webhook payload/context key and activity-related optional interfaces from fleet package. |
| server/datastore/mysql/testing_utils.go | Updated activity test service creation for new bootstrap signature. |
| server/datastore/mysql/in_house_apps_test.go | Removed use of legacy fleet.ActivityWebhookContextKey. |
| server/contexts/logging/logging.go | Updated client-error classification interface import. |
| server/contexts/ctxerr/ctxerr.go | Updated Cause and client-error classification interface import. |
| server/activity/providers.go | Removed WebhookSendFunc from activity providers layer. |
| server/activity/internal/tests/suite_test.go | Updated service construction for new NewService signature. |
| server/activity/internal/service/service_test.go | Updated service construction for new NewService signature. |
| server/activity/internal/service/service.go | Removed webhookSendFn from service struct and constructor. |
| server/activity/internal/service/new_activity_test.go | Added webhook delivery tests directly against activity service. |
| server/activity/internal/service/new_activity.go | Switched webhook delivery to platformhttp.PostJSONWithTimeout. |
| server/activity/bootstrap/bootstrap.go | Updated bootstrap to construct service without webhook send fn injection. |
| server/activity/bootstrap/testing.go | Deleted unit-test bootstrap helper that depended on injectable webhook send fn. |
| server/activity/arch_test.go | Updated architecture dependency rules (platform/errors + fleethttp). |
| cmd/fleet/serve.go | Updated activity bounded-context creation for new bootstrap signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ksykulev
approved these changes
Feb 26, 2026
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.
Related issue: Resolves #38536
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.38536-new-activity-bcalready present, and this is just cleanup from that work.Testing
Summary by CodeRabbit
Release Notes