fix(session): harden persistent session expiry lifecycle

[mbuckton]

Summary

Tracked by MSG-339. This PR hardens persistent-session reconnect/expiry handling and establishes one lifecycle mechanism for active close, disconnected close, reset, expiry,
administrative close, duplicate replacement, and shutdown.

The core rule is now explicit: SessionManagerPipeLine owns SubscriptionController destruction. SessionImpl.close() releases session-owned resources only.

Lifecycle invariants

• One active SessionImpl per session ID.
• Active removal is identity-safe so a stale close cannot remove a replacement session.
• Persistent-controller removal is identity-safe so stale expiry cannot remove a replacement controller.
• persistentControllers represents persistent ownership; disconnectedControllers represents disconnected state.
• Only persistent sessions enter disconnected expiry handling.
• All final SubscriptionController destruction passes through one pipeline finalizer.
• Connected, disconnected, and expired counters change only on successful state transitions.
• Expiry completion means queued pipeline cleanup completed, not merely that the timer fired.
• Terminal cleanup removes both persisted in-memory session details and the backing state file.
• Reset remains distinct from expiry and terminal close.
• Shutdown uses the normal lifecycle and has no controller-destruction shortcut.
• Will behavior is explicit for graceful close, ungraceful disconnect, expiry, administrative close, and shutdown.

Race handling

Reconnect wins

The reconnect cancels the pending SessionExpiryTask, restores the same persistent controller, clears disconnected accounting, and stale queued cleanup cannot remove it later.

Expiry wins

Expiry finalizes the old controller and persistence first. A later reconnect creates a fresh controller from fresh session state.

Both orderings are deterministic because lifecycle work for a session ID executes through the same pipeline executor.

Regression coverage

SessionManagerPipeLineTest covers:

• active and disconnected lifecycle accessors;
• new session creation;
• duplicate active replacement without connected-count inflation;
• stale close protection;
• persistent close and scheduled expiry;
• transient positive-expiry close;
• reconnect before expiry;
• timer-fired/reconnect ordering;
• expiry-before-reconnect ordering;
• reset-state reconnect;
• restored disconnected expiry;
• already-expired restored cleanup;
• duplicate cleanup idempotency;
• persistence deletion and deletion failure;
• pending Will scheduling and forced final execution;
• clearing a pending Will on an already-disconnected administrative close;
• shutdown of active transient and disconnected persistent sessions.

SessionExpiryTaskTest covers:

• cancel before timer trigger;
• cancel after timer trigger but before queued cleanup;
• completion only after pipeline cleanup;
• rejected pipeline submission.

SessionImplLifecycleTest locks the ownership boundary that SessionImpl.close() must never destroy the SubscriptionController.

SessionManagerPipeLineScaleTest adds bounded high-churn regression coverage for 2,048 independent transient sessions, 2,048 concurrent producer lifecycle operations, 1,024 persistent expiries with pending Wills, and 1,000 reconnect/disconnect cycles on one persistent session.

MQTTConnectionTest now verifies that an ungraceful MQTT 3.1/3.1.1 disconnect produces the configured Will message end to end.

The focused MQTT session-recovery workflow now triggers for session lifecycle test changes and runs all three lifecycle suites.

Documentation

The lifecycle ownership model, transition matrix, persistence semantics, Will behavior, shutdown behavior, and review checklist are documented in:

docs/session-lifecycle.md

The source-level invariants in SessionManagerPipeLine link directly to that document.

Validation

• deterministic lifecycle tests
• expiry/reconnect ordering
• stale cleanup identity protection
• reset-state reconnect
• persistence deletion ownership
• single controller-destruction mechanism
• shutdown lifecycle coverage
• Will lifecycle coverage
• high-churn session scale regression coverage
• end-to-end MQTT Will publication coverage
• focused MQTT validation includes lifecycle suites
• MAVLink regression validation
• final Jenkins full-suite green confirmation

Refs: MSG-339

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b7e03388-49de-4ddc-b12d-6757cf04766e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The session pipeline now accepts injected lifecycle dependencies and routes delayed cleanup through a cancellable task. Controller closure, reconnect, and disconnected-session tracking are updated. New tests cover expiry tasks and pipeline lifecycle behavior.

Changes

Session pipeline lifecycle

Layer / File(s)	Summary
Pipeline dependencies and session creation src/main/java/io/mapsmessaging/engine/session/SessionFactory.java, src/main/java/io/mapsmessaging/engine/session/SessionManagerPipeLine.java, src/main/java/io/mapsmessaging/engine/session/SessionManager.java, src/test/java/io/mapsmessaging/engine/session/SessionManagerPipeLineTest.java	The pipeline accepts injected lifecycle dependencies and delegates session construction to a factory. Subscription accessors now report whether controllers exist and return a session ID snapshot. SessionManager comments describe create and close work on the session-ID pipeline.
Expiry and controller cleanup src/main/java/io/mapsmessaging/engine/session/SessionExpiry*, src/main/java/io/mapsmessaging/engine/session/SessionStateFileStore.java, src/main/java/io/mapsmessaging/engine/session/SubscriptionControllerFactory.java, src/main/java/io/mapsmessaging/engine/session/SessionManagerPipeLine.java, src/test/java/io/mapsmessaging/engine/session/SessionExpiryTaskTest.java, src/test/java/io/mapsmessaging/engine/session/SessionManagerPipeLineTest.java, .github/workflows/mqtt-session-recovery-validation.yml	Expiry cleanup is submitted to the pipeline executor through a cancellable future. Controller closure updates disconnected tracking and expiry counts, and deletes state files through the injected store. Reconnect cancels pending expiry before reusing a controller. Tests cover cancellation, expiry ordering, restored sessions, cleanup, and shutdown; the validation workflow includes both test classes.

Priority: ➖ Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant SessionManagerPipeLine
  participant SessionExpiryTask
  participant SessionExpiryScheduler
  participant PipelineExecutor
  participant SessionStateFileStore
  SessionManagerPipeLine->>SessionExpiryTask: schedule expiry cleanup
  SessionExpiryTask->>SessionExpiryScheduler: schedule timer callback
  SessionExpiryScheduler->>SessionExpiryTask: fire timer callback
  SessionExpiryTask->>PipelineExecutor: submit cleanup
  PipelineExecutor->>SessionManagerPipeLine: run controller cleanup
  SessionManagerPipeLine->>SessionStateFileStore: delete state file after successful closure

Loading

Merge Risk: 🔵 Low · up to 6e5fd

The lifecycle failure is not reachable through the established paths. The remaining test naming requirement can be addressed before merge without affecting session behavior.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 6e5fd

The change protects normal expiry and reconnect ordering, but a failed expiry-cleanup submission can leave a persistent session unable to reconnect. The likelihood of that failure in production and its recovery behavior remain unclear.

Retained concerns

• Medium · reliability · inferred: If expiry cleanup cannot be submitted to the pipeline, its future completes without removing the mapped controller. A later reconnect repeatedly retries the same completed timeout, leaving that session without a bounded recovery path.

Security review details

Security Blast Radius

• inferred — The demonstrated exceptional-reconnect path affects a mapped persistent session and its pipeline work. Evidence does not establish a cross-tenant authorization bypass or a production means for an untrusted client to cause executor rejection.

Security Findings and Attack Paths

• inferred — A cleanup-submission failure followed by reconnect can prevent session restoration through recursive retries. This is a conditional availability path, not a verified attacker-triggered exploit.

Trust Boundaries and Controls

• observed — SessionManager routes controller closure to the pipeline by session ID. A JMX operation invokes that wrapper, but authorization of the JMX operation was not established; no new JMX reachability from this PR was shown.

Resilience and Maintainability Implications

• observed — Tests cover reconnect on either side of queued expiry cleanup. The examined failure test confirms exceptional task completion on rejected submission, but does not establish pipeline-level recovery from that state.

Hardening Proposals

• proposed — Define a bounded, identity-safe recovery transition for exceptional expiry completion so reconnect can either repair controller ownership or fail explicitly rather than retry recursively; exercise rejected submission and cleanup exceptions at pipeline level.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 79 functions across 9 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: hardening the persistent session expiry lifecycle.

Full details: Docstring Coverage

Explanation

Docstring coverage is 5.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 79 functions across 9 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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.}

[mbuckton]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Head commit changed.

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.

[mbuckton]

@coderabbitai review

[coderabbitai]

✅ 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.