Skip to content

fix(trogon-nats): remove unnecessary Box::pin in flush retry closure#72

Merged
yordis merged 1 commit into
mainfrom
fix/remove-boxpin-flush
Mar 31, 2026
Merged

fix(trogon-nats): remove unnecessary Box::pin in flush retry closure#72
yordis merged 1 commit into
mainfrom
fix/remove-boxpin-flush

Conversation

@yordis
Copy link
Copy Markdown
Member

@yordis yordis commented Mar 31, 2026

Summary

  • Replace Box::pin(async move { ... }) with plain async move { ... } in the flush retry closure, eliminating a heap allocation per retry attempt
  • The FnMut() -> Fut bound is satisfied without boxing since every invocation returns the same anonymous future type

@cursor
Copy link
Copy Markdown

cursor Bot commented Mar 31, 2026

PR Summary

Medium Risk
Touches core retry logic used by publish/flush, changing attempt counting and logging behavior; a mistake could alter retry timing or error propagation. No auth/data-model changes, but runtime behavior under failure is affected.

Overview
Simplifies RetryPolicy::execute to eagerly run the first attempt, then perform exactly max_retries backoff retries, removing the old while loop bookkeeping and the "no error recorded" fallback.

Removes Box::pin from the flush retry closure so each retry returns an unboxed async move future (avoids per-attempt heap allocation), and updates tests to initialize tracing_subscriber plus adds it as a dev-dependency.

Written by Cursor Bugbot for commit e1da09b. This will update automatically on new commits. Configure here.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 31, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 54c926f2-8b01-4aea-82ad-481eefc45953

📥 Commits

Reviewing files that changed from the base of the PR and between 1d8a489 and e1da09b.

📒 Files selected for processing (2)
  • rsworkspace/crates/trogon-nats/Cargo.toml
  • rsworkspace/crates/trogon-nats/src/messaging.rs

Walkthrough

This PR refactors the retry logic in RetryPolicy::execute from a while-loop to a more straightforward for-loop structure, improves error handling to consistently use the last recorded error, simplifies the closure pattern, and adds test logger initialization for retry policy tests.

Changes

Cohort / File(s) Summary
Dependency Update
rsworkspace/crates/trogon-nats/Cargo.toml
Added tracing-subscriber as a dev dependency.
Retry Logic Refactoring
rsworkspace/crates/trogon-nats/src/messaging.rs
Restructured RetryPolicy::execute to initialize with an immediate operation attempt, then iterate retries using a for-loop; updated exponential backoff computation and logging behavior; removed fallback error message and simplified closure pattern by eliminating Box::pin; added test logger initialization to retry policy tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Poem

🐰 Hoppy refactoring, loops rewound,
Retry logic, cleaner and sound,
Errors recorded, no more pretend,
Traces logged from start to end,
With steady hops, the code's more bright!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the primary change: removing Box::pin from the flush retry closure, which is a significant refactoring reflected in the code changes.
Description check ✅ Passed The description clearly explains the change, its benefit (eliminating heap allocation), and the technical reasoning (FnMut() -> Fut bound satisfaction), all directly related to the changeset.
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
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/remove-boxpin-flush

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

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 31, 2026

badge

Code Coverage Summary

Details
Filename                                                     Stmts    Miss  Cover    Missing
---------------------------------------------------------  -------  ------  -------  ---------------------------------------------------------------------------------------------
crates/acp-nats/src/agent/authenticate.rs                       52       0  100.00%
crates/acp-nats/src/agent/list_sessions.rs                      50       0  100.00%
crates/acp-nats/src/agent/ext_method.rs                         92       0  100.00%
crates/acp-nats/src/agent/set_session_mode.rs                   79       0  100.00%
crates/acp-nats/src/agent/prompt.rs                           1085       1  99.91%   144
crates/acp-nats/src/agent/close_session.rs                      72       0  100.00%
crates/acp-nats/src/agent/mod.rs                                61       0  100.00%
crates/acp-nats/src/agent/js_request.rs                        296       0  100.00%
crates/acp-nats/src/agent/ext_notification.rs                   88       0  100.00%
crates/acp-nats/src/agent/resume_session.rs                    113       0  100.00%
crates/acp-nats/src/agent/test_support.rs                      267       0  100.00%
crates/acp-nats/src/agent/set_session_model.rs                  76       0  100.00%
crates/acp-nats/src/agent/bridge.rs                            142      13  90.85%   171-183
crates/acp-nats/src/agent/new_session.rs                        91       0  100.00%
crates/acp-nats/src/agent/set_session_config_option.rs          76       0  100.00%
crates/acp-nats/src/agent/initialize.rs                         82       0  100.00%
crates/acp-nats/src/agent/load_session.rs                      114       0  100.00%
crates/acp-nats/src/agent/cancel.rs                            104       0  100.00%
crates/acp-nats/src/agent/fork_session.rs                      117       0  100.00%
crates/trogon-std/src/dirs/fixed.rs                             84       0  100.00%
crates/trogon-std/src/dirs/system.rs                            98      11  88.78%   57, 65, 67, 75, 77, 85, 87, 96, 98, 109, 154
crates/acp-nats/src/jetstream/streams.rs                       252       0  100.00%
crates/acp-nats/src/jetstream/consumers.rs                      76       0  100.00%
crates/acp-nats/src/jetstream/ext_policy.rs                     26       0  100.00%
crates/acp-nats/src/jetstream/provision.rs                      58       0  100.00%
crates/acp-nats/src/telemetry/metrics.rs                        65       0  100.00%
crates/acp-telemetry/src/signal.rs                               3       3  0.00%    4-43
crates/acp-telemetry/src/trace.rs                               32       4  87.50%   23-24, 31-32
crates/acp-telemetry/src/lib.rs                                153      22  85.62%   39-46, 81, 86, 91, 105-120
crates/acp-telemetry/src/log.rs                                 70       2  97.14%   39-40
crates/acp-telemetry/src/metric.rs                              35       4  88.57%   30-31, 38-39
crates/acp-telemetry/src/service_name.rs                        16       0  100.00%
crates/acp-nats/src/client_proxy.rs                            196       0  100.00%
crates/acp-nats/src/acp_prefix.rs                               63       0  100.00%
crates/acp-nats/src/lib.rs                                      73       0  100.00%
crates/acp-nats/src/ext_method_name.rs                          85       0  100.00%
crates/acp-nats/src/in_flight_slot_guard.rs                     32       0  100.00%
crates/acp-nats/src/config.rs                                  201       0  100.00%
crates/acp-nats/src/pending_prompt_waiters.rs                  112       0  100.00%
crates/acp-nats/src/error.rs                                    84       0  100.00%
crates/acp-nats/src/session_id.rs                               88       0  100.00%
crates/acp-nats/src/jsonrpc.rs                                   6       0  100.00%
crates/trogon-nats/src/jetstream/mocks.rs                      435       0  100.00%
crates/trogon-nats/src/jetstream/traits.rs                      96       0  100.00%
crates/acp-nats/src/client/ext_session_prompt_response.rs      150       0  100.00%
crates/acp-nats/src/client/fs_read_text_file.rs                384       0  100.00%
crates/acp-nats/src/client/terminal_kill.rs                    309       0  100.00%
crates/acp-nats/src/client/terminal_release.rs                 357       0  100.00%
crates/acp-nats/src/client/ext.rs                              365       8  97.81%   193-204, 229-240
crates/acp-nats/src/client/session_update.rs                    55       0  100.00%
crates/acp-nats/src/client/rpc_reply.rs                         71       0  100.00%
crates/acp-nats/src/client/terminal_create.rs                  294       0  100.00%
crates/acp-nats/src/client/terminal_wait_for_exit.rs           396       0  100.00%
crates/acp-nats/src/client/request_permission.rs               338       0  100.00%
crates/acp-nats/src/client/terminal_output.rs                  223       0  100.00%
crates/acp-nats/src/client/fs_write_text_file.rs               451       0  100.00%
crates/acp-nats/src/client/mod.rs                             2981       0  100.00%
crates/trogon-std/src/args.rs                                   10       0  100.00%
crates/trogon-std/src/json.rs                                   30       0  100.00%
crates/acp-nats-agent/src/connection.rs                       1356       9  99.34%   472, 669-671, 678, 1826-1827, 1840-1841
crates/acp-nats-ws/src/connection.rs                           162      35  78.40%   71-78, 83-94, 110, 112-113, 118, 129-131, 138, 142, 146, 149-157, 168, 172, 175, 178-182, 216
crates/acp-nats-ws/src/config.rs                                83       0  100.00%
crates/acp-nats-ws/src/main.rs                                 157       2  98.73%   84, 247
crates/acp-nats-ws/src/upgrade.rs                               57       2  96.49%   59, 90
crates/trogon-std/src/env/system.rs                             17       0  100.00%
crates/trogon-std/src/env/in_memory.rs                          81       0  100.00%
crates/trogon-std/src/fs/system.rs                              29      12  58.62%   17-19, 31-45
crates/trogon-std/src/fs/mem.rs                                220      10  95.45%   61-63, 77-79, 133-135, 158
crates/acp-nats-stdio/src/main.rs                              113      11  90.27%   58, 106-113, 119-121, 138
crates/acp-nats-stdio/src/config.rs                             72       0  100.00%
crates/acp-nats/src/nats/subjects.rs                           294       0  100.00%
crates/acp-nats/src/nats/extensions.rs                           3       0  100.00%
crates/acp-nats/src/nats/parsing.rs                            280       1  99.64%   151
crates/acp-nats/src/nats/token.rs                                8       0  100.00%
crates/trogon-nats/src/client.rs                                25      25  0.00%    50-89
crates/trogon-nats/src/connect.rs                               96      16  83.33%   22-24, 37, 49, 68-151
crates/trogon-nats/src/auth.rs                                 114       3  97.37%   45-47
crates/trogon-nats/src/messaging.rs                            528       2  99.62%   132, 142
crates/trogon-nats/src/mocks.rs                                304       0  100.00%
crates/trogon-std/src/time/system.rs                            24       0  100.00%
crates/trogon-std/src/time/mock.rs                             123       0  100.00%
TOTAL                                                        16056     196  98.78%

Diff against main

Filename                               Stmts    Miss  Cover
-----------------------------------  -------  ------  -------
crates/trogon-nats/src/messaging.rs       -5      -2  +0.37%
TOTAL                                     -5      -2  +0.01%

Results for commit: e1da09b

Minimum allowed coverage is 95%

♻️ This comment has been updated with latest results

@yordis yordis closed this Mar 31, 2026
@yordis yordis reopened this Mar 31, 2026
@yordis yordis force-pushed the fix/remove-boxpin-flush branch from 443daf9 to 5e86f94 Compare March 31, 2026 17:43
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis yordis force-pushed the fix/remove-boxpin-flush branch from 5e86f94 to e1da09b Compare March 31, 2026 17:52
@yordis yordis merged commit 3713f05 into main Mar 31, 2026
7 checks passed
@yordis yordis deleted the fix/remove-boxpin-flush branch March 31, 2026 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant