-
Notifications
You must be signed in to change notification settings - Fork 55
Feat/events socket #352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
GeoffChurch
wants to merge
19
commits into
shell-pool:master
Choose a base branch
from
GeoffChurch:feat/events-socket
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Feat/events socket #352
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
d9d06d3
feat(events): define push-event JSON schema
GeoffChurch d4f2a3f
feat(events): add EventBus and sibling events socket listener
GeoffChurch ea78579
feat(events): publish deltas from session lifecycle mutations
GeoffChurch a4558be
feat(events): add `shpool events` subcommand
GeoffChurch 464b6fe
test(events): integration tests for the events socket
GeoffChurch 68694a2
fix(events): de-duplicate lifecycle deltas; clean socket on signal exit
GeoffChurch 4b7153e
fix: improve error messages on chunk read failures (#351)
ethanpailes 5af8c62
fix: reduce global lock contention when resizing (#355)
ethanpailes 3c1615c
fix: bug in shell startup loop when handling EOF (#354)
ethanpailes 7a790f5
chore: release (#334)
release-plz-for-shpool[bot] 9aa05cd
refactor(events): trim event payloads to type fields only
GeoffChurch bb2ec68
docs(events): add EVENTS.md describing the events protocol
GeoffChurch 09ac7db
chore: bump shpool_pty to 0.4.0
ethanpailes 2a73f1e
chore: tweak AI policy
ethanpailes 526ec73
Merge branch 'shell-pool:master' into feat/events-socket
GeoffChurch 7de8ee2
EVENTS.md: fix main socket description and remove "Ordering" section
GeoffChurch d1f3cb2
events: polish error contexts, naming, and module boundary
GeoffChurch ca34b35
events: add tests for bus isolation, latency, and lock ordering
GeoffChurch 5250954
Merge branch 'shell-pool:master' into feat/events-socket
GeoffChurch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # Events | ||
|
|
||
| `shpool` exposes an event stream so that external programs can react to changes | ||
| without polling. This way, a program (e.g. a TUI) can call `shpool list` (or the | ||
| equivalent `ConnectHeader::List` request over the main socket; see the | ||
| [`shpool-protocol`](./shpool-protocol) crate) after each event so that its model | ||
| is always consistent with shpool's state. | ||
|
|
||
| ## The events socket | ||
|
|
||
| The daemon binds a sibling Unix socket next to the main shpool socket: | ||
|
|
||
| ```bash | ||
| <runtime_dir>/shpool/shpool.socket # main socket | ||
| <runtime_dir>/shpool/events.socket # events socket (this protocol) | ||
| ``` | ||
|
|
||
| A subscriber connects to `events.socket` and reads events. The daemon ignores anything written to the events socket, so for subscribers it's effectively read-only. | ||
|
|
||
| ## Event types | ||
|
|
||
| | `type` | Meaning | | ||
| | ------------------ | -------------------------------------------------------- | | ||
| | `session.created` | A new session was added to the table. | | ||
| | `session.attached` | A client attached or reattached to a session. | | ||
| | `session.detached` | A client disconnected from a still-running session. | | ||
| | `session.removed` | A session was removed (shell exited, killed, or reaped). | | ||
|
|
||
| Subscribers should ignore unknown `type` values so that future event types do | ||
| not break older consumers. | ||
|
|
||
| ## Wire format | ||
|
|
||
| The daemon writes one JSON object per line (JSONL). Each event looks like: | ||
|
|
||
| ```json | ||
| {"type":"<event-type>"} | ||
| ``` | ||
|
|
||
| There are no other fields. To learn what the event refers to (which session, | ||
| when, etc.), call `shpool list` (or use `ConnectHeader::List`). | ||
|
|
||
| The format is robust: literal newline characters only appear as delimiters | ||
| between events. Any newlines within JSON string values are automatically | ||
| escaped (as `\n`) by the daemon. | ||
|
|
||
| ## Subscribing | ||
|
|
||
| For ad-hoc use, `shpool events` connects to the events socket and prints each | ||
| event line to stdout, flushing after each line: | ||
|
|
||
| ```bash | ||
| shpool events | while read -r ev; do | ||
| echo "got: $ev" | ||
| shpool list | ||
| done | ||
|
|
||
| shpool events | jq . | ||
| ``` | ||
|
|
||
| ## Slow subscribers | ||
|
|
||
| Each subscriber has a bounded outbound queue. A subscriber that falls too far | ||
| behind is dropped by the daemon (in which case the subscriber can always reconnect). | ||
| There is no replay, so events that fired while a subscriber was disconnected are | ||
| lost. | ||
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should document a knob in the config.toml that tunes how large this queue is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want the config knob added in this PR?
The queues currently use
std::sync::mpsc::sync_channelhere, pre-allocating allSUBSCRIBER_QUEUE_DEPTHslots.Would you want to use something slightly slower that allocates on-demand and has something like
shrink_to_fitto return memory after a spike? I'm just imagining if someone sets their queue depth to 1 billion not expecting it to pre-allocate for each subscriber. If it instead allocates on-demand you could maybe set the queue depth to something large like 100K by default anyway, at which point maybe it wouldn't be worth offering a config knob.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's a good point. Honestly I should probably suppress the urge to throw a knob for everything. Who is the target demo that would actually tune this knob? Probably a tool developer who would tell their users to configure shpool specially, but that seems pretty unlikely. If it is a real problem they can always open an issue and we can expose the knob then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orthogonal to the knob, would you want a dynamically allocated data structure to replace
sync_channelanyway? If a user goes wild with sessions, e.g. a bunch of programmatically generated sessions with different@{var}combinations, thenMutex<VecDeque>/shrink_to_fitmight scale better.Actually, since the events are uninformative by design, subscribers probably won't care which events are in the queue so much as if an event is in the queue. So, what about
sync_channel(1)(a single slot)?Optionwould work too butsync_channelmakes it easy to increase to 8 or 16 if events become more informative later on.