Skip to content

fix(lifecycle): recheck host admission before queued dials - #1665

Merged
jlucaso1 merged 2 commits into
mainfrom
fix/1664-connect-admission-recheck
Oct 6, 2026
Merged

jlucaso1 merged 2 commits into
mainfrom
fix/1664-connect-admission-recheck

Conversation

@jlucaso1

@jlucaso1 jlucaso1 commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Closes #1664. Follow-up to #1532 / #1594.

A cooldown learned while clients wait for reserved connection slots currently cannot postpone those dials. Add a provided ConnectAdmission::recheck() -> Duration, called after the initial wait, including zero, and after every positive extension. delay() still reserves exactly once per attempt. Rechecks retain that reservation and its pause generation. The default returns zero, preserving existing delay-only policies, including the documented constant-positive-delay example.

Extensions retain shutdown, supervision-stop and pause cancellation, do not count a dial or failure, and remain outside the transport timeout. Manual connect() and WhatsApp reconnect backoff are unchanged. The API documents the remaining race: a host update after the final zero cannot revoke that permission.

Regression evidence

The paused-time regression reserves t=10, then observes host cooldown updates at t=5 and t=29 that move admission to t=30 and t=40. It asserts no factory call before t=40 and exactly one reservation throughout.

Negative control on the candidate: replace only delay = admission.recheck() with delay = Duration::ZERO, leaving the new API and test intact. connect_admission_rechecks_cooldown_without_reserving_again then fails at t=10 with actual dial count 1, expected 0, exit 101. Restore the call and all 17 public admission tests pass. This is an executable behavior failure, not a compilation failure.

Other regressions cover initial zero, repeated extensions, legacy constant delays, shutdown during and inside rechecks, pause/resume including rapid toggles, normal/forced reconnects, transport timeout isolation, unrelated notifications and supervision stop. Standalone consumers exercise default and overridden methods through trait objects, including a local Rc policy on WASM.

Validation

Head 3f01a56dcab59ebae1e092a2eef8bfeaef8e55bb, pinned nightly-2026-06-16:

  • cargo test -p whatsapp-rust --test connect_admission: 17 passed, including after restoring the negative control.
  • cargo nextest run -p whatsapp-rust --lib: 2538 passed, 3 skipped on the final head.
  • cargo clippy -p whatsapp-rust --all-targets -- -D warnings: passed.
  • cargo test -p whatsapp-rust --doc: 50 passed, 15 ignored.
  • Workspace and standalone-consumer formatting: passed.
  • Standalone SDK consumer admission tests: 2 passed.
  • Registered SDK WASM release-build command: passed. Two existing unused-qualification warnings in src/request.rs and src/upload.rs remain; this is a build pass, not a warning-free WASM lint claim.

CodeRabbit approved the published head; Greptile found no actionable issues. After marking the draft ready, Codex reviewed this same head and found no major issues. There are no open review threads. Rust CI, native/MSRV API consumers, WASM, E2E, Miri, CodSpeed, formatting and Cargo Deny completed successfully for this head. The all-features job includes 3181 passed, 6 skipped for the SDK shareable-feature test set. The separate advisory semver failure is explained below; this is not an all-green semver claim.

The binary-size gate passed against base 7ed5b5d1: stripped bytes +320 B, .text +320 B, allocated sections unchanged, dependencies unchanged.

The informational semver job failed, despite its workflow-level success. It compares only unchanged wacore, wacore-binary and waproto against published 0.7.0, reporting 24/3/7 failing check categories. Their source trees and Cargo.lock are identical to the PR base. It does not check the changed SDK API; compatibility evidence for that is the default method and standalone consumers. Detailed triage. No override or unrelated version bump was made.

CodeRabbit's advisory docstring-coverage warning includes regression helpers; the new public API is documented and Rustdoc passes. Review follow-up.

CodSpeed's workflow passed but its report flags two unchanged libsignal benchmarks across different CPU environments. Both have identical modeled instruction time between base and head; the reported delta is entirely modeled cache/memory cost. Exact comparison. No matched-hardware run or warning suppression was performed.

The additional Codex security review did not run because the payer reached usage limits. This is separate from the completed code review and is not presented as a security-review pass.

This changes host SDK admission, not wire behavior or generated protocol sources. No server-rate-limit assumptions or live authenticated WhatsApp session were needed.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI (base), Organization UI (inherited)
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 14c158fc-8f6f-43c6-8014-d315a381da29
📥 Commits

Reviewing files that changed from the base of the PR and between 7ed5b5d and 3f01a56.

📒 Files selected for processing (5)
  • src/client/lifecycle.rs
  • src/client/lifecycle/admission_tests.rs
  • src/types/connect_admission.rs
  • tests/api-consumer/src/lifecycle/admission.rs
  • tests/connect_admission.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Summary

Summary by CodeRabbit

  • New Features
    • Connection admission policies can now recheck whether a connection may proceed after the initial wait and each extension. Policies can allow dialing immediately or extend the wait based on current conditions.
  • Bug Fixes
    • Unrelated wakeups no longer restart an admission wait. Stopping supervision or changing pause state cancels the wait appropriately.

Walkthrough

ConnectAdmission now provides a recheck() callback. The lifecycle wait loop calls it after the initial delay and after each positive extension. Tests cover extensions, cancellation, reconnect behavior, timeout boundaries, and shutdown.

Changes

Connect admission rechecks

Layer / File(s) Summary
Admission recheck contract
src/types/connect_admission.rs, tests/api-consumer/src/lifecycle/admission.rs
ConnectAdmission adds a default recheck() method that returns zero. API consumer tests check repeated calls through a trait object.
Lifecycle admission wait
src/client/lifecycle.rs, src/client/lifecycle/admission_tests.rs
The run loop passes the admission policy to the wait function. The wait function calls recheck() after each delay, without reserving again, and retains shutdown and pause-state checks.
Admission and reconnect scenarios
tests/connect_admission.rs
Tests cover cooldown extensions, zero-duration reservations, cancellation, reconnects, transport timeouts, and shutdown during recheck.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant LifecycleRunLoop
  participant wait_for_connect_admission
  participant ConnectAdmission
  LifecycleRunLoop->>ConnectAdmission: delay()
  LifecycleRunLoop->>wait_for_connect_admission: initial delay and policy
  wait_for_connect_admission->>ConnectAdmission: recheck() after delay
  ConnectAdmission-->>wait_for_connect_admission: zero or extended delay
  wait_for_connect_admission-->>LifecycleRunLoop: dialing allowed or wait again
Loading

Merge Risk: ⚪ Minimal · up to 3f01a

No merge-blocking risk was identified in the admission recheck change; normal validation can continue.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 43.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #1664 requires a host policy to revise admission while a client waits. The PR adds the provided ConnectAdmission::recheck() callback and calls it after the initial wait, including a zero wait,…
Out of Scope Changes check ✅ Passed The reported changes are limited to admission lifecycle logic, the ConnectAdmission API, regression tests, and standalone consumer coverage for the API change. These changes support issue #1664. The…
Title check ✅ Passed The title clearly identifies the main change: rechecking host admission before queued dials.
Description check ✅ Passed The description explains the admission recheck behavior, its scope, regression coverage, and validation results. It is directly related to the changeset.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

jlucaso1 commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

Please review the current head 3f01a56 while this PR remains a draft. In particular, check reservation compatibility, cancellation across repeated extensions, and the paused-time regression coverage.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Changes how the client waits for connection admission.

The PR appears safe to merge; no actionable issue was found.

What we checked:

  • Pause cannot hide during rechecks: pause() changes pause_generation. The admission wait keeps the original value and checks it after each callback and wait.
  • Extensions reuse the reservation: delay() runs before the wait. The extension loop calls only recheck(), so it keeps the same reservation.
  • Native policies remain shareable: ConnectAdmission requires MaybeSendSync, which requires Send + Sync on native builds. The consumer also checks Client::run() inside a native async_trait implementation.
Summary

Adds ConnectAdmission::recheck() so host cooldown updates can postpone queued dials without reserving another slot.

  • Keeps shutdown, stop, and pause cancellation across extended waits.
  • Preserves existing delay-only policies with a zero-returning default.
  • Adds tests for repeated extensions, reconnects, cancellation, and transport timeout isolation.

Review used source inspection; no tests were run.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Backoff and pause gate"] --> B["delay(): reserve once"]
    B --> C["Wait with cancellation checks"]
    C --> D["recheck()"]
    D --> E{"More delay?"}
    E -- Yes --> C
    E -- No --> F["Final cancellation check"]
    F --> G["connect(): start transport timeout"]
    C -- Cancelled --> H["Abandon reservation"]
    F -- Cancelled --> H
Loading

Reviews (1) · Last reviewed commit: "test(lifecycle): use imported atomic cou..."

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

📦 Binary size report

Metric main PR Δ
bin size (stripped) 11.00 MiB 11.00 MiB +320 B (+0.00%) 🔺
bin .text 8.87 MiB 8.87 MiB +320 B (+0.00%) 🔺
bin allocated (text+data+bss) 10.99 MiB 10.99 MiB 0
llvm-lines wacore 616,056 616,056 0
llvm-lines wacore copies 20,348 20,348 0
llvm-lines whatsapp-rust lib 969,987 970,051 +64 (+0.01%) 🔺
llvm-lines whatsapp-rust lib copies 30,722 30,723 +1 (+0.00%) 🔺
deps crates (Cargo.lock) 611 611 0
.text per crate
Crate main PR Δ
.text whatsapp_rust 2.25 MiB 2.25 MiB +296 B (+0.01%) 🔺
.text wacore 859.33 KiB 859.33 KiB 0
.text wacore_binary 80.74 KiB 80.74 KiB 0
.text wacore_libsignal 191.23 KiB 191.23 KiB 0
.text wacore_appstate 40.31 KiB 40.31 KiB 0
.text wacore_noise 23.52 KiB 23.52 KiB 0
.text waproto 1.74 MiB 1.74 MiB 0
.text whatsapp_rust_sqlite_storage 609.81 KiB 609.81 KiB 0
.text whatsapp_rust_tokio_transport 57.89 KiB 57.89 KiB 0
.text whatsapp_rust_ureq_http_client 12.62 KiB 12.62 KiB 0
.text std 939.85 KiB 939.86 KiB +9 B (+0.00%) 🔺
.text other deps 2.08 MiB 2.08 MiB 0

Baseline: 7ed5b5d15 · Head: b85fb8ba9 · Graphs

jlucaso1 commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

Review follow-up for head 3f01a56: CodeRabbit approved with no actionable comments, and Greptile found no actionable issue. The docstring-coverage warning counts test functions/helpers along with the API. The new public recheck() method documents invocation order, default behavior, cancellation, timeout scope and the final-zero race; Rustdoc and all 50 executable doctests pass. I am leaving descriptive regression tests and helpers without boilerplate docstrings.

Scope clarification: preserving timers on unrelated notifications and cancelling on pause/stop already existed in #1594. This PR preserves those properties across the new extension loop; it does not claim they were broken for the original one-shot wait.

@codspeed

codspeed Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will degrade performance by 1.54%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 839 untouched benchmarks
⏩ 12 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation bench_session_with_self[false] 1.2 µs 1.3 µs -8.22%
❌ Simulation bench_session_with_self[true] 1.2 µs 1.3 µs -8.21%
⚡ Simulation bench_session_root_key_update 460.9 ns 406.7 ns +13.32%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing fix/1664-connect-admission-recheck (3f01a56) with main (7ed5b5d)

Open in CodSpeed

Footnotes

  1. 12 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

jlucaso1 commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

Semver advisory triage for head 3f01a56:

The informational semver job failed. The encompassing Supply Chain workflow reports success because this job has continue-on-error: true; this is not an all-green semver result.

The unchanged xtask command checks only wacore, wacore-binary, and waproto against their published 0.7.0 releases, not the PR's target main. The log reports 24, 3, and 7 failing check categories respectively, including existing enum/field changes and waproto::codec::sync_action_data_to_vec changing arity. These source trees, their manifests, Cargo.lock, and the check configuration are byte-identical between base 7ed5b5d and this head. No change here can account for those source API differences from the published release.

This job does not check whatsapp-rust, so it also does not prove the new method's compatibility. That evidence comes from the added default implementation and standalone consumers: existing policies still implement only delay(), overridden recheck() remains object-safe, native and local-Rc WASM consumers compile, and the legacy constant-positive-delay regression still dials once. No required trait method, field, feature, dependency or version was added. I am retaining the advisory failure and the repository's existing policy, without a version bump or CI override in this focused fix.

@jlucaso1

jlucaso1 commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-06T15:15:26.707926Z 3f01a56 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 3f01a56dca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@jlucaso1
jlucaso1 marked this pull request as ready for review October 6, 2026 15:12
@chatgpt-codex-connector

Copy link
Copy Markdown

The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins.

@jlucaso1
jlucaso1 merged commit d515aba into main Oct 6, 2026
42 of 43 checks passed
@jlucaso1
jlucaso1 deleted the fix/1664-connect-admission-recheck branch October 6, 2026 15:15

jlucaso1 commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

CodSpeed advisory triage for 3f01a56 versus 7ed5b5d:

The workflow passed, but its report flags bench_session_with_self[false] and [true] at -8.22% / -8.21%, aggregate -1.54%. I inspected the exact run comparison and both structured results.

CodSpeed attributes both benchmarks to an environment change from AMD EPYC 9V74 to EPYC 7763, with different CPU flags and linked-library metadata. For [false], modeled instruction time is identical at 31.244865751 ns in base and head. For [true], it is identical at 32.750452358 ns. In both, modeled memory-access time changes from 1111.111111111 ns to 1222.222222222 ns, while cache-miss time changes from 66.666666667 ns to 63.888888889 ns. The reported delta is entirely in those cache/memory components.

These are isolated wacore-libsignal benchmarks of SessionState::session_with_self, not SDK lifecycle calls. The libsignal tree, dependencies/lockfile, build flags and benchmark workflow are unchanged from the base, and this target does not depend on whatsapp-rust. This supports treating the report as an environment-confounded comparison rather than evidence of admission-path overhead. I have not run a matched-hardware comparison or suppressed/acknowledged the warning in CodSpeed. The report remains visible; no unrelated libsignal optimization is included.

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.

lifecycle: a ConnectAdmission decision cannot be revised once served, so a host cannot hold queued dials behind a push-back (follow-up to #1532)

1 participant