Skip to content

fix: harden logtail push lifecycle#25645

Open
XuPeng-SH wants to merge 8 commits into
mainfrom
fix/tn-impl-lifecycle
Open

fix: harden logtail push lifecycle#25645
XuPeng-SH wants to merge 8 commits into
mainfrom
fix/tn-impl-lifecycle

Conversation

@XuPeng-SH

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • BUG
  • Improvement
  • Test and CI

Which issue(s) this PR fixes:

Fixes #25644

What this PR does / why we need it:

Hardens the TN logtail push lifecycle under normal unhappy-path conditions.

  • Makes subscription completion generation-aware so canceled or superseded phase results cannot publish stale responses.
  • Honors caller cancellation in the logtail client request queue and in disttae readiness waiting.
  • Isolates a congested session from global incremental publication.
  • Releases queued logtail batches during server shutdown and establishes a bootstrap waterline before the first incremental event.
  • Detects and removes idle disconnected morpc sessions with a low-frequency probe.
  • Adds deterministic in-memory, real morpc integration, concurrent lifecycle, shutdown, ownership, bootstrap, boundary, and race coverage.

Validation

  • go test -race ./pkg/vm/engine/tae/logtail/service ./pkg/vm/engine/tae/logtail ./pkg/vm/engine/disttae
  • Repeated race coverage for lifecycle and generation tests.
  • git diff --check

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@mergify mergify Bot added kind/bug Something isn't working kind/enhancement kind/test-ci labels Jul 12, 2026
@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Jul 12, 2026

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the latest head be0fc6d627. The only PR-local delta after my last pass is the mock logtail session lifecycle fix in pkg/vm/engine/test/testutil: testClientSession now has a real session context rooted in the transport context, Close() cancels it, and Write() terminates on either the caller context or the session/transport shutdown. That closes the remaining test-fixture gap where a mock session could keep looking alive and keep a send path blocked after transport teardown, so the unhappy-path tests now model the production morpc/session lifetime much more faithfully.

I reran the focused packages on the latest head (./pkg/vm/engine/test/testutil, ./pkg/vm/engine/tae/logtail/service, ./pkg/vm/engine/disttae) plus race for ./pkg/vm/engine/test/testutil and ./pkg/vm/engine/tae/logtail/service, and I do not see a new high-confidence correctness or Q1/Q2/Q3 blocker on this head. GitHub still will not let me APPROVE my own pull request, so I am leaving this as another approve-level review comment.

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review result: REQUEST_CHANGES. I reviewed the lifecycle, ordering, cancellation, callback ownership, slow-session isolation, heartbeat/bootstrap protocol, and regression tests. I found one P1 blocker below. The issue is a protocol-ordering violation under a congested session, so this cannot be approved as-is. Validation: git diff --check passed; the focused logstore/sm unit and race tests passed. The service/disttae package tests could not compile in this checkout because the environment lacks required cgo headers (usearch.h/roaring.h/xxhash.h).

Comment thread pkg/vm/engine/tae/logtail/service/session.go
@matrix-meow matrix-meow added size/XXL Denotes a PR that changes 2000+ lines and removed size/XL Denotes a PR that changes [1000, 1999] lines labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working kind/enhancement kind/test-ci size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Logtail push sessions can leak or block publication on disconnect and cancellation

3 participants