fix(e2e): use explicit 30s stop timeout for restarted service in lifecycle test

[sylr]

Summary

• Fix flaky app_lifecycle_test.go "when restarting the service" test that still fails on main despite fix: use context.Background() for service restart in lifecycle test #1276
• DeferNew in go-libs registers a DeferCleanup with a 10-second timeout, but the fx default stop timeout is 15 seconds — under CI load the full shutdown chain (watermill router, nats subscriber/connection, circuit breaker, etc.) can exceed 10s
• Register an explicit DeferCleanup with a 30-second budget that runs before DeferNew's cleanup (Ginkgo LIFO order), making the latter a no-op (cancel == nil)

Test plan

• CI passes consistently (the previously flaky lifecycle restart test should no longer fail)
• Other lifecycle tests (in-flight transactions, downgrade) still pass

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

Modified the "restarting the service" scenario in an e2e test to capture the restarted service in a local variable, then added explicit cleanup logic that stops the restarted service with a 30-second timeout before other deferred cleanups run.

Changes

Cohort / File(s)	Summary
E2E Test Lifecycle Updates test/e2e/app_lifecycle_test.go	Refactored service restart test scenario to capture restarted service in local variable srv and added DeferCleanup that explicitly stops the restarted service with 30-second timeout, ensuring proper cleanup ordering and accommodating longer shutdowns under CI load.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Cleanup hops with purpose true,
Deferred in order, through and through,
The service stops with thirty beats,
No dangling threads when test completes! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: adding an explicit 30-second stop timeout for the restarted service in the lifecycle test.
Description check	✅ Passed	The description is well-detailed and directly related to the changeset, explaining the root cause of the flaky test, the fix approach, and the test plan.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/ci

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

test/e2e/app_lifecycle_test.go (1)

92-93: Minor readability tweak: avoid ctx shadowing in cleanup.

Consider renaming the local timeout context to stopCtx to keep it visually distinct from the outer test context.

Suggested small rename

-					ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+					stopCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 					defer cancel()
 
-					Expect(srv.Stop(ctx)).To(Succeed())
+					Expect(srv.Stop(stopCtx)).To(Succeed())

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/app_lifecycle_test.go` around lines 92 - 93, Rename the local
timeout context used in the cleanup block to avoid shadowing the outer test
context: change the call context.WithTimeout(context.Background(),
30*time.Second) so its returned context variable is named stopCtx (and the
cancel function stopCancel), and update the defer to defer stopCancel()
everywhere; this removes the inner variable named ctx and prevents shadowing of
the outer ctx used elsewhere in the test.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/e2e/app_lifecycle_test.go`:
- Around line 92-93: Rename the local timeout context used in the cleanup block
to avoid shadowing the outer test context: change the call
context.WithTimeout(context.Background(), 30*time.Second) so its returned
context variable is named stopCtx (and the cancel function stopCancel), and
update the defer to defer stopCancel() everywhere; this removes the inner
variable named ctx and prevents shadowing of the outer ctx used elsewhere in the
test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 706409b2-18d4-4388-81cf-c2a207140d33

📥 Commits

Reviewing files that changed from the base of the PR and between a5cb69d and bab770c.

📒 Files selected for processing (1)

• test/e2e/app_lifecycle_test.go

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.35%. Comparing base (4b80bcb) to head (bab770c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1286      +/-   ##
==========================================
- Coverage   79.39%   79.35%   -0.05%     
==========================================
  Files         205      205              
  Lines       10998    10998              
==========================================
- Hits         8732     8727       -5     
- Misses       1564     1574      +10     
+ Partials      702      697       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.