Skip to content

fix(logging): harden sandbox log delivery under load - #3106

Open
krishicks wants to merge 1 commit into
hicks/push-vlqrkupqqkrpfrom
hicks/push-vyrplousupmw
Open

fix(logging): harden sandbox log delivery under load#3106
krishicks wants to merge 1 commit into
hicks/push-vlqrkupqqkrpfrom
hicks/push-vyrplousupmw

Conversation

@krishicks

Copy link
Copy Markdown
Collaborator

Summary

Compress sandbox log pushes and split batches below the gateway decode limit so an oversized request cannot wedge the reconnect loop. Drop only individually undeliverable lines and report those losses to the gateway.

Track channel, reconnect-buffer, and oversized-line losses cumulatively. Preserve gateway accounting across RPC reconnects, reset it with sandbox teardown, and meter both sandbox-side and ingest-side drops.

Related Issue

Refs #1055

Changes

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@johntmyers

Copy link
Copy Markdown
Collaborator

Review findings

  • [P1] Do not require gzip before every gateway can accept it. The supervisor unconditionally enables request compression (log_push.rs:307-315), while gateway support is introduced only in this PR (multiplex.rs:273-275). A newer supervisor connecting to an older gateway or replica gets an unsupported-compression error, then enters the reconnect/backoff path and eventually drops logs. Please either land server-side acceptance before enabling client compression, negotiate the capability, or retry uncompressed when compression is rejected.

  • [P1] Requeue or account for batches rejected by the stream channel. send_logs returns false as soon as push_tx.send(...) fails, discarding the failed request and any remaining split groups (log_push.rs:233-257). Its callers have already removed those lines from batch with mem::take (log_push.rs:323-365), so an RPC closure during handoff silently loses them without rebuffering or incrementing the new drop counters. Please return the unsent groups for retry after reconnect, or explicitly record every discarded line before returning.

@krishicks

Copy link
Copy Markdown
Collaborator Author

@johntmyers

A newer supervisor connecting to an older gateway or replica gets an unsupported-compression error, then enters the reconnect/backoff path and eventually drops logs. Please either land server-side acceptance before enabling client compression, negotiate the capability, or retry uncompressed when compression is rejected.

Is this a real situation? A newer supervisor connecting to an older gateway? I would imagine only the reverse being true. During an upgrade the gateway would be bounced and the sandboxes shortly after with an updated supervisor, no?

@krishicks
krishicks force-pushed the hicks/push-vyrplousupmw branch from bb77402 to f6e9686 Compare September 2, 2026 20:17
@copy-pr-bot

copy-pr-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@krishicks
krishicks force-pushed the hicks/push-vyrplousupmw branch from f6e9686 to 2c3f99a Compare September 2, 2026 22:08
Compress sandbox log pushes and split batches below the gateway decode limit so
an oversized request cannot wedge the reconnect loop. Drop only individually
undeliverable lines and report those losses to the gateway.

Track channel, reconnect-buffer, and oversized-line losses cumulatively.
Preserve gateway accounting across RPC reconnects, reset it with sandbox
teardown, and meter both sandbox-side and ingest-side drops.

Refs #1055

Signed-off-by: Kris Hicks <khicks@nvidia.com>
@krishicks
krishicks force-pushed the hicks/push-vyrplousupmw branch from 2c3f99a to f572721 Compare September 2, 2026 23:32
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.

2 participants