Skip to content

refactor(BA-7315): carry event payloads as JSON instead of msgpack - #13746

Draft
fregataa wants to merge 2 commits into
mainfrom
refactor/BA-7315-json-event-payloads
Draft

refactor(BA-7315): carry event payloads as JSON instead of msgpack#13746
fregataa wants to merge 2 commits into
mainfrom
refactor/BA-7315-json-event-payloads

Conversation

@fregataa

@fregataa fregataa commented Aug 12, 2026

Copy link
Copy Markdown
Member

Summary

  • Publish every event body as JSON, derived from the event's own Pydantic fields via the @final to_message() / from_message() pair, and remove the 130 hand-written serialize() / deserialize() implementations plus the msgpack wire format beneath them. A positional tuple can no longer disagree with the field order it is built from.
  • Replace creation_info: Mapping[str, Any] with a typed KernelCreationInfo whose leaves are JSON-representable. This was the one payload relying on pickled ResourceSlot / Decimal / Path values carried in msgpack ext types, and it is now declared only on the started events — the only ones that ever carried it. The manager's untyped duplicate view of the same data is gone, and its allocation aggregation is now KernelResourceSpecData.to_resource_slot().
  • Raise EventPayloadEncodingError / EventPayloadDecodingError from the conversion pair instead of letting Pydantic errors escape, and ack a message whose body cannot be decoded rather than leaving it to be redelivered until the retry limit discards it. Also drops args_matcher, which matched against the positional tuple and had no callers.

Every component reads and writes the same single stream (events) and channel (events_all), so this requires the cluster to run one version across the upgrade. In-flight msgpack entries are logged and skipped as malformed.

This folds BA-7316 (removing the msgpack fallback) into this change: with the write path on JSON only, there is no fallback left to keep.

Test plan

  • pants test tests/unit/common/events:: tests/unit/common/message_queue::
  • pants test tests/unit/common/test_events.py tests/unit/common/test_distributed.py
  • pants test tests/unit/manager/repositories/scheduler::
  • Verify event flows end to end on a live server: session create/terminate (kernel lifecycle), agent heartbeat, and image scan

Resolves BA-7315

@fregataa
fregataa requested a review from a team as a code owner August 12, 2026 15:23
Copilot AI balanced review requested due to automatic review settings August 12, 2026 15:23

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added size:XL 500~ LoC comp:manager Related to Manager component comp:agent Related to Agent component comp:common Related to Common component comp:app-proxy Related to App Proxy component labels Aug 12, 2026
fregataa added a commit that referenced this pull request Aug 12, 2026
@fregataa
fregataa marked this pull request as draft August 12, 2026 15:26
fregataa and others added 2 commits August 13, 2026 01:51
Events are Pydantic models, so their fields already describe the body they
carry. Publish that body as JSON via the final `to_message()` / `from_message()`
pair and drop the hand-written `serialize()` / `deserialize()` tuple pair from
every event, along with the msgpack wire format underneath it.

- Remove 130 `serialize()` / `deserialize()` implementations and their abstract
  declarations; a positional tuple can no longer disagree with the field order.
- Carry the body as a `payload` field on the wire and drop `args` / `legacy_body`
  along with the base64 wrapping broadcast needed for packed bytes.
- Replace `creation_info: Mapping[str, Any]` with `KernelCreationInfo`, whose
  leaves are JSON-representable — the one payload that relied on pickled
  `ResourceSlot` / `Decimal` / `Path` values inside msgpack ext types.
- Declare `creation_info` only on the started events, which are the only ones
  that ever carried it, and delete the manager's untyped duplicate view of it.
- Raise `EventPayloadEncodingError` / `EventPayloadDecodingError` from the
  conversion pair, and ack a message whose body cannot be decoded instead of
  leaving it to be redelivered until the retry limit discards it.
- Drop `args_matcher`, which matched against the positional tuple and had no
  callers.

Every component reads and writes the same single stream and channel, so this
requires the cluster to run one version across the upgrade.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`KernelResourceSpecData` described itself entirely in built-ins, so nothing in the
payload said what a key or a mount actually was.

- Type the fields as what they hold: `list[ResourceSlotEntry]` for slots,
  `dict[DeviceName, dict[ResourceSlotName, dict[DeviceId, str]]]` for allocations,
  and `list[MountData]` for mounts, which keeps a mount structured instead of
  flattening it to its `str()` form. `KernelCreationInfo` follows with `KernelId`,
  `ContainerId`, `DeviceId` and `DeviceName`.
- `MountData` mirrors the agent's `Mount` and declares no defaults, so a producer
  states every field rather than inheriting one.
- Omit a slot holding no device allocation from `to_resource_slot()` instead of
  recording it as zero. This restores what the manager helper it replaced did — a
  slot absent and a slot present at zero are not the same occupancy.
- Drop `KernelResourceSpec.to_json_serializable_dict()` / `to_json()`, whose only
  caller was a log line, and `AgentRegistry.convert_resource_spec_to_resource_slot()`,
  which had no caller at all. Slot aggregation now has one implementation rather
  than three.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fregataa
fregataa force-pushed the refactor/BA-7315-json-event-payloads branch from 0bfd6b2 to 8a65349 Compare August 12, 2026 16:53
@fregataa
fregataa marked this pull request as ready for review August 12, 2026 17:05
@fregataa
fregataa marked this pull request as draft August 13, 2026 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:agent Related to Agent component comp:app-proxy Related to App Proxy component comp:common Related to Common component comp:manager Related to Manager component size:XL 500~ LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants