Servermanager arch - #575
Open
balasaraswathy-n wants to merge 5 commits into
Open
Conversation
|
Pull request must be merged with a description containing the required fields, Summary: If there is no jira releated to this change, please put 'Jira: NO-JIRA'. Description can be changed by editing the top comment on your pull request and making a new commit. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds repository documentation and GitHub Copilot/OpenSpec workflow assets to better document Rialto architecture (especially ServerManager), operational knowledge, and evidence-based log analysis procedures.
Changes:
- Add architecture briefs and operational notes for ServerManager and logging, plus an updated repo-wide architecture brief.
- Add OpenSpec configuration and several ServerManager-focused specs (API, architecture, data model, suspended-state flow).
- Add Copilot/agent guidance, prompts, skills, and instruction files to standardize analysis/review workflows (plus example healthcheck restart attribution artifacts under
tmp/).
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tmp/rialtoserver-healthcheck-restart-report.md | Example generated healthcheck-restart attribution report artifact. |
| tmp/rialtoserver-healthcheck-restart-inventory.md | Example inventory of evidence used for healthcheck-restart attribution. |
| serverManager/SME-notes.md | New operational/incident “SME notes” for ServerManager. |
| serverManager/architecture-brief.md | New ServerManager architecture brief (C4 views, lifecycle, healthcheck, ops notes). |
| README.md | Adds pointers to AI assistant guidance documents. |
| openspec/specs/suspended-state.md | Draft spec documenting proposed suspended-state flow for ServerManager. |
| openspec/specs/mediaserver_capabilities.md | Draft spec documenting media server decoder capability model/config transport. |
| openspec/specs/data-model.md | Draft spec documenting ServerManager-relevant public/proto data contracts. |
| openspec/specs/architecture.md | Draft spec stating ServerManager architecture baseline and sources of truth. |
| openspec/specs/api.md | Draft spec documenting ServerManager public and internal API surfaces. |
| openspec/config.yaml | Adds OpenSpec schema configuration. |
| logging/architecture.md | New logging module architecture brief. |
| docs/architecture-brief.md | New repository-wide architecture brief with diagrams and API inventories. |
| AGENTS.md | Adds repository-wide agent instructions and validation commands. |
| .github/skills/rialto-servermanager-log-evidence/SKILL.md | Adds a skill for evidence-first ServerManager log analysis. |
| .github/skills/openspec-propose/SKILL.md | Adds an OpenSpec “propose” skill workflow description. |
| .github/skills/openspec-explore/SKILL.md | Adds an OpenSpec “explore mode” skill workflow description. |
| .github/skills/openspec-archive-change/SKILL.md | Adds an OpenSpec “archive change” skill workflow description. |
| .github/skills/openspec-apply-change/SKILL.md | Adds an OpenSpec “apply change” skill workflow description. |
| .github/skills/_templates/rialto-servermanager-skill-prefilled.md | Adds a prefilled template for creating ServerManager skills. |
| .github/skills/_templates/basic-skill-template.md | Adds a basic skill template. |
| .github/prompts/servermanager-change-review.prompt.md | Adds a structured prompt for reviewing ServerManager changes. |
| .github/prompts/rialtoserver-state-capture.prompt.md | Adds a structured prompt for capturing state evidence from logs. |
| .github/prompts/rialtoserver-shutdown-evidence.prompt.md | Adds a structured prompt for shutdown evidence analysis. |
| .github/prompts/rialtoserver-healthcheck-restart-attribution.prompt.md | Adds a structured prompt for healthcheck restart attribution. |
| .github/prompts/opsx-propose.prompt.md | Adds an OpenSpec “opsx propose” prompt. |
| .github/prompts/opsx-explore.prompt.md | Adds an OpenSpec “opsx explore” prompt. |
| .github/prompts/opsx-archive.prompt.md | Adds an OpenSpec “opsx archive” prompt. |
| .github/prompts/opsx-apply.prompt.md | Adds an OpenSpec “opsx apply” prompt. |
| .github/prompts/catalyst-architecture-brief.prompt.md | Adds a comprehensive architecture-brief generation prompt. |
| .github/prompts/05-prompt-fundamentals-cheatsheet.md | Adds a cheatsheet for authoring structured prompts. |
| .github/instructions/tests.instructions.md | Adds instructions for test code contributions. |
| .github/instructions/servermanager.instructions.md | Adds instructions for ServerManager code contributions. |
| .github/instructions/servermanager-tests.instructions.md | Adds instructions for ServerManager unit tests. |
| .github/instructions/architecture-docs.instructions.md | Adds instructions for editing architecture documentation. |
| .github/copilot-instructions.md | Adds repo-specific GitHub Copilot instructions. |
Comments suppressed due to low confidence (3)
serverManager/architecture-brief.md:42
- This diagram label uses
RialtoSessionServer, but the actual session-server executable isRialtoServer(see defaultSESSION_SERVER_PATH). Using the executable name in diagrams makes log/config correlation much easier.
serverManager/architecture-brief.md:93 - This node label uses
RialtoSessionServer, but the repo’s session-server runtime isRialtoServer(e.g., defaultSESSION_SERVER_PATH). Renaming avoids mismatches with observed log prefixes likeRialtoServer[...].
serverManager/architecture-brief.md:359 - The runtime model calls out
RialtoSessionServer, but the configured binary isRialtoServerby default. Aligning naming here prevents ambiguity for operators.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+6
to
+9
| ## Overview | ||
| `serverManager` is the session lifecycle orchestration component of Rialto. It is responsible for: | ||
| - Spawning and configuring `RialtoSessionServer` processes. | ||
| - Tracking and driving per-application session state transitions. |
| @@ -0,0 +1,159 @@ | |||
| SME Notes: RialtoServerManager | |||
| - When config-file support is enabled, config is read in precedence order: base (`RIALTO_CONFIG_PATH`), SoC (`RIALTO_CONFIG_SOC_PATH`), then overrides (`RIALTO_CONFIG_OVERRIDES_PATH`). | ||
| - If a more important file provides non-empty `envVariables`, they replace previously loaded `envVariables` and clear any previously loaded `extraEnvVariables`. | ||
| - `extraEnvVariables` (when present) are applied after `envVariables` and can overwrite keys. | ||
| - The more important file (`RIALTO_CONFIG_OVERRIDES_PATH`) must clearly specify the extraEnvVariables , if they are empty any previously loaded `extraEnvVariables` will be cleared |
| Connection-info contract detail: | ||
| - After transition out of not-running, app manager is expected to query `getAppConnectionInfo()` promptly so the socket name can be passed into container/app launch context. | ||
| - If app manager provided socket name up front, returned connection info acts as confirmation rather than discovery. | ||
| - out of Uninitialized. Preloaded servers do not have socket name assigned yet. |
Comment on lines
+5
to
+9
| - Log source 1: `C:\Users\bn480\Downloads\D452EE8376FD_Logs_05-17-26-06-09AM\05-17-26-06-21AM-sky-messages.log` | ||
| - Log source 2: `C:\Users\bn480\Downloads\D452EE8376FD_Logs_05-17-26-06-09AM\05-17-26-06-21AM-sky-messages.log.1` | ||
| - Log source 3: `C:\Users\bn480\Downloads\D452EE8376FD_Logs_05-17-26-06-09AM\05-17-26-06-21AM-sky-messages.log.2` | ||
| - Log source 4: `C:\Users\bn480\Downloads\D452EE8376FD_Logs_05-17-26-06-09AM\05-17-26-06-21AM-sky-messages.log.3` | ||
| - Log source 5: `C:\Users\bn480\Downloads\D452EE8376FD_Logs_05-17-26-06-09AM\05-17-26-06-21AM-sky-messages.log.4` |
|
Coverage statistics of your commit: |
mhughesacn
requested changes
Jul 28, 2026
mhughesacn
left a comment
mhughesacn
left a comment
There was a problem hiding this comment.
Hi @balasaraswathy-n : Tar files are not usually permitted in repos. What is tmp.tar and can it be deleted?
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 40 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
serverManager/SME-notes.md:1
- The document title on line 1 is not a Markdown heading, so most renderers won’t treat it as the page title (and it won’t appear in generated TOCs). Consider making it an H1 heading for consistency with other docs in the repo.
serverManager/architecture-brief.md:113 - This bullet has a stray space before the comma and is missing punctuation, making the config-precedence rule hard to read (and potentially ambiguous). Rephrase with clear clause separation.
| - `media/client/ipc/source/MediaPipelineCapabilitiesIpc.cpp` | ||
|
|
||
| Related OpenSpec deltas from PR branch: | ||
| - `openspec/changes/hfp-schema-v1-migration/specs/audio-decoder-capabilities/spec.md` |
Contributor
There was a problem hiding this comment.
You didn't add those files.
|
|
||
| ## Goals | ||
| - Load audio and video decoder capabilities from YAML configuration files. | ||
| - Expose capabilities through existing `IMediaPipelineCapabilities` methods. |
Contributor
There was a problem hiding this comment.
YAML capabilities are exposed through two new methods.
| ## IPC Contract | ||
|
|
||
| ### Protobuf service | ||
| `MediaPipelineCapabilitiesModule` already exposes: |
Contributor
There was a problem hiding this comment.
I'm pretty sure those methods are not available on master yet.
|
|
||
| **Thread Safety**: | ||
| - All pipeline mutations (source attach, sample push, seek, state change) are serialized on the `WorkerThread` task queue. | ||
| - GStreamer bus message processing runs on a separate `GstDispatcherThread`. |
Contributor
There was a problem hiding this comment.
it's worth to add here that the GstMessage received in GstDispatcherThread is later handled on a worker thread with HandleBusMessage task
| - `AttachSource` — creates `GstCaps` via `MediaSourceCapsBuilder`, adds an appsrc pad to `GstRialtoSrc`, optionally inserts a decryptor element. | ||
| - `ReadShmDataAndAttachSamples` — reads compressed data from the shared memory IPC buffer, builds a `GstBuffer`, attaches `GstRialtoProtectionMetadata` for encrypted frames, pushes to the appsrc. | ||
| - `SetupElement` — connects decoder element signals (first-frame, audio underflow probes) once the GStreamer pipeline auto-plugs elements. | ||
| - `CheckAudioUnderflow` — runs on timer; compares pipeline clock position against last decoded audio timestamp to detect stalls. |
Contributor
There was a problem hiding this comment.
I wouldn't say CheckAudioUnderflow is one of a key tasks :) e.g. SetSourcePosition is more worth describing here
Contributor
Author
There was a problem hiding this comment.
added. Please find the description added . SetSourcePosition — records the target seek position, applied rate, and stop position for a given source type (audio, video, or subtitle). For audio/video sources the data is stored in GenericPlayerContext::initialPositions as a SegmentData entry, to be consumed when the next buffer is pushed through the appsrc. For subtitle sources, if source setup has already finished, the position is applied immediately via a gObjectSet on the subtitle sink; otherwise it is queued in the same initialPositions map.
| - `Flush` — sends `flush-start`/`flush-stop` on the appsrc; coordinate with `FlushOnPrerollController` to avoid the GStreamer preroll race. | ||
|
|
||
| **Web Audio Player (`GstWebAudioPlayer.cpp`)** | ||
| Separate, simpler pipeline for PCM web audio. 8 tasks cover the full lifecycle (set caps, write, play, pause, stop, shutdown, EOS, ping). |
Contributor
There was a problem hiding this comment.
It also uses different shared memory buffer - In case of web audio we have a push mode instead of pull mode. SHM is a circular buffer, the client app pushes data when it's allowed to do so instead of waiting for NeedData to respond with haveData.
| - Each `GstGenericPlayer` instance owns two runtime threads: | ||
| - `WorkerThread` — all pipeline mutations. | ||
| - `GstDispatcherThread` — GStreamer bus message poll loop. | ||
| - `GstCapabilities` owns one background init thread that exits after `gst_init` completes. |
Contributor
There was a problem hiding this comment.
Not true. Initialisation thread exits when GstCapabilities gets all required information from the Gstreamer Elements and saves it internally. please look at the fillSupportedMimeTypes() function.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 49 changed files in this pull request and generated no new comments.
Suppressed comments (5)
serverManager/SME-notes.md:1
- The file starts with plain text rather than a Markdown heading, which makes the document harder to navigate and inconsistent with other docs in this repo.
tmp/rialtoserver-healthcheck-restart-inventory.md:9 - These log source paths include a specific developer workstation path and username, which is not portable and may leak personal information. Consider redacting the user-specific portion or replacing with a generic placeholder path.
openspec/specs/architecture.md:7 - The ServerManager spec section says the document is "for Rialto ServerManager only", but the same file later contains a full "Architecture Spec: GstPlayer" section. This is internally inconsistent and can confuse consumers about what this document covers.
serverManager/SME-notes.md:28 - State names here use mixed-case (
Inactive,Active,NotRunning) while the rest of the document (and the canonical state set) uses the enum-style names (INACTIVE,ACTIVE,NOT_RUNNING). Aligning these reduces ambiguity for operators.
serverManager/architecture-brief.md:114 - This bullet claims an empty
extraEnvVariableslist in a higher-precedence config file clears previously loadedextraEnvVariables, but the implementation only overwritesm_extraEnvVarsFromConfigFilewhen the parsed map is non-empty (and only clears it whenenvVariablesis non-empty). The documentation should match the actual precedence/clearing behavior.
|
Coverage statistics of your commit: |
| Manages PCM web audio playback via `IGstWebAudioPlayer`. Maps a `WEB_AUDIO` partition in `SharedMemoryBuffer`. Uses a periodic timer (100 ms cadence, `kWriteDataTimeMs`) to drain the shared memory ring buffer into the GStreamer pipeline via `writeBuffer`. Preferred frame count is 640 (`kPreferredFrames`). | ||
|
|
||
| **`MediaKeysServerInternal` (`MediaKeysServerInternal.cpp`)** | ||
| Implements `IMediaKeys` + `IMediaKeysServerInternal` + `IDecryptionService`. Owns: |
Contributor
There was a problem hiding this comment.
Not true, IDecryptionService is implemented by the service:CdmService
| Implements `IMediaKeys` + `IMediaKeysServerInternal` + `IDecryptionService`. Owns: | ||
| - A map of `keySessionId → MediaKeySession` for all active CDM sessions. | ||
| - One `IOcdmSystem` instance per key system (e.g., `com.widevine.alpha`). | ||
| Implements `IDecryptionService::decrypt()` by delegating to the matching `MediaKeySession`'s OCDM session, allowing `gstplayer`'s decryptor elements to call back into this layer for per-buffer decryption without directly coupling to OCDM. |
Contributor
There was a problem hiding this comment.
Not true, IDecryptionService is implemented by the service:CdmService
| | `pause()` | Service → MediaPipeline | Transitions pipeline to PAUSED | | ||
| | `stop()` | Service → MediaPipeline | Stops and destroys the GstPlayer | | ||
| | `haveData(status, numFrames, requestId)` | Service → MediaPipeline | Server-only: reads shm frames for the given request ID and pushes to GstPlayer | | ||
| | `seekPosition(position)` | Service → MediaPipeline | Seeks to an absolute position in nanoseconds | |
|
|
||
| | Method | Direction | Description | | ||
| |---|---|---| | ||
| | `createKeySession(sessionType, client, isLDL, keySessionId&)` | Service → MediaKeys | Creates a new OCDM key session | |
Contributor
There was a problem hiding this comment.
createKeySession creates a new Rialto internal key session. The "real" OCDM session on server's side is created later, during generateRequest call.
| 1. `RialtoServer` must be controllable by `serverManager` over an IPC socket (socket fd passed as `argv[1]`); the service layer must bridge those RPC commands into coordinated actions on all sub-services without exposing internal implementation details to the IPC layer. | ||
| 2. Multiple app clients connect concurrently over a separate client-facing IPC socket; their requests must be dispatched to the correct session-scoped `MediaPipelineServerInternal`, `MediaKeysServerInternal`, or `ControlServerInternal` instance. | ||
| 3. The `SharedMemoryBuffer` must exist exactly during the ACTIVE state; it is created on `switchToActive` and destroyed on `switchToInactive` so that the client process can map it only when resources are allocated. | ||
| 4. Playback and CDM resources must be cleanly released (pipelines destroyed, key sessions cleared) before the NOT_RUNNING state change event is sent to `serverManager`, guaranteeing that the server is resource-free before `serverManager` considers it stopped. |
Contributor
There was a problem hiding this comment.
Playback and CDM resources are cleared when we switch to Inactive.
| | `media/server/main/IMediaKeysServerInternal` | C++ interface | DRM key management per handle | | ||
| | `media/server/main/IControlServerInternal` | C++ interface | Heartbeat ack + app state per control ID | | ||
| | `media/server/main/ISharedMemoryBuffer` | C++ interface | Created by `PlaybackService`; passed to pipeline + web audio instances | | ||
| | `media/server/main/IDecryptionService` | C++ interface | Implemented by `MediaKeysServerInternal`; reference passed to `MediaPipelineService` | |
Contributor
There was a problem hiding this comment.
not true, IDecryptionService is implemented by the CdmService.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.