Repository navigation
refactor(core): give an optional subsystem one attachment point #1329
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
4b36816
refactor(core): give an optional subsystem one attachment point
claude 89ecaae
ci(features): derive the tested feature set instead of listing it
claude 53d5699
Merge remote-tracking branch 'origin/main' into claude/audit-conditio…
claude 7dc89e4
test(subsystem): make the shadowing guard observe dispatch, not absence
claude 57a465d
refactor(events): stop IncomingCall changing shape with the voip feature
claude fc52b9a
refactor(voip): move the VoIP runtime's state onto the attachment table
claude 05b54c6
docs(subsystem): record the VoIP result, the pair_code blocker and wh…
claude 597028d
test(subsystem): close the three gaps type erasure and the shared run…
claude 7f4a401
docs(subsystem): name the size baseline as main, measured from one tree
claude a4906ca
fix(subsystem): drop a doc comment rustdoc cannot attach
claude f69edb6
refactor(subsystem): make the attachment seam statically typed
claude 0e794b5
refactor(subsystem): tighten the seam's public surface and its guard
claude bf0ac23
docs(subsystem): record why voip-runtime is a runtime, and refresh th…
claude def138a
test(subsystem): cap the gates a disciplined subsystem keeps in the core
claude 46a370a
test(subsystem): count a composite gate against the budget
claude 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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| # Subsystem Boundary | ||
|
|
||
| The core compiles conditionally for three optional subsystems and nobody had | ||
| measured what any of them cost. This is the rule that decides whether a | ||
| subsystem may stop being part of the core, the classification of every optional | ||
| subsystem against it, and the numbers behind both. | ||
|
|
||
| Read it before adding a feature gate, before adding a `Client` field only one | ||
| subsystem reads, and before proposing that a subsystem move out of the tree. | ||
|
|
||
| Anchors here are files and symbols, never line numbers: a `file:line` citation | ||
| in a document nobody recompiles is wrong within a week. | ||
|
|
||
| ## Where the counts come from | ||
|
|
||
| ```sh | ||
| grep -rc 'cfg(feature = "<name>")' --include='*.rs' src | ||
| ``` | ||
|
|
||
| Counted at `ff4ac10`, production sites only, since a gate inside a `mod tests` | ||
| block is scaffolding rather than coupling: `voip-runtime` 171, `plugins` 87, | ||
| `client-lifecycle` 56. | ||
|
|
||
| The same VoIP subsystem is 47 gates for 46k lines in `wacore`, where `voip` is | ||
| one gated `mod` and everything under it is unconditional. The difference is not | ||
| the subsystem, it is whether the subsystem owns its own files. | ||
|
|
||
| ## The cut rule | ||
|
|
||
| A subsystem is **cuttable** when all four tests pass. | ||
|
|
||
| 1. **Reach.** The core enters it on a dispatch key the core already routes on (a | ||
| stanza tag, a notification type, an IQ namespace), or not at all. A core | ||
| function that runs subsystem statements inline fails this. | ||
| 2. **State.** Its per-client state is read only by itself. | ||
| 3. **Return.** Everything it needs from the core is already `pub` or | ||
| `pub(crate)` for some other caller. | ||
| 4. **Contract.** Nothing it owns changes shape with the feature. `Event` is | ||
| exempt from removal but not from mutation: `EventKind` discriminants are | ||
| `EventInterest` bit indices consumers persist, so a cut subsystem keeps its | ||
| variants and payload types compiled unconditionally. What fails this test is | ||
| a payload *field* behind a `cfg`, because then one public type has two | ||
| shapes. | ||
|
|
||
| Verdicts: | ||
|
|
||
| - **Cuttable.** All four pass. The core may name it in exactly two places: its | ||
| `mod` declaration and its entry in `SUBSYSTEMS` (`src/client/subsystem.rs`). | ||
| `tests/subsystem_boundary.rs` fails on a third. | ||
| - **Coupled.** Fails 1 or 2. It can be *disciplined* (interleaved statements | ||
| hoisted into files it owns, one call per seam) but not cut, because the seam | ||
| it needs does not exist yet. | ||
| - **Structural.** It is a core seam or a platform adapter slot, not a passenger. | ||
| Its gate count is inherent. | ||
| - **Cross-cutting.** Instrumentation, gated at the point being instrumented by | ||
| definition. | ||
|
|
||
| The rule deliberately does not say "a subsystem with its own directory can | ||
| leave": `src/voip/` has one and is not cuttable. Nor "a big subsystem should | ||
| leave": `src/message` is the largest thing in the crate and is the hot path, not | ||
| a subsystem. | ||
|
|
||
| ## Inventory | ||
|
|
||
| ### Cuttable | ||
|
|
||
| | subsystem | why | status | | ||
| | --- | --- | --- | | ||
| | `passkey` (`src/passkey/`) | claims two notification types and nothing else; state is its own; needs only `persistence_manager`, the event bus and `query`; owns `Event::PairPasskey*` with no gated field | cut, behind the `passkey` feature | | ||
|
|
||
| ### Coupled | ||
|
|
||
| | subsystem | the edge that fails | test | | ||
| | --- | --- | --- | | ||
| | `voip-runtime` | `bind_pending_call_link_join_ack` runs inline in the ack path (`src/client/node_io.rs`) | 1 | | ||
| | | `call_registry` is read by `CallHandler` and by `memory_report` | 2 | | ||
| | | `would_emit_pkmsg` (`src/client/sessions.rs`) and `should_issue_tc_token` (`src/send/tctoken_lifecycle.rs`) exist only for it | 3 | | ||
| | | `IncomingCall::media` is a `cfg` field inside a public payload (`wacore/src/types/call.rs`) | 4 | | ||
| | `pdo` (`src/pdo.rs`) | driven from the retry pipeline, and `pdo_requested` is the memo that keeps retry idempotent | 1, 2 | | ||
| | `pair_code` (`src/pair_code.rs`) | `pair_code_state` is written by the companion-reg notification handler, by `src/pair.rs` and by connection cleanup | 2 | | ||
| | `features/groups`, `features/newsletter`, `features/business`, `features/mex` | outbound IQ in `src/features/`, inbound handling in `src/handlers/notification/`, so neither half owns the subsystem; `group_cache` is also read from `src/voip/facade.rs` | 1, 2 | | ||
|
|
||
| ### Structural | ||
|
|
||
| `client-lifecycle` is the generation-scoped seam; `plugins` is the generic host, | ||
| and its gate count is the price of the seam existing. `sqlite-storage`, | ||
| `tokio-transport`, `tokio-runtime`, `ureq-client`, `signal` and `tokio-native` | ||
| are platform adapter selection. `voip-mlow`, `voip-libopus` and `voip-encoded` are codec profiles inside `voip`. `bench-harness`, | ||
| `debug-snapshots`, `legacy-session-interop` and `danger-skip-*` are build-time | ||
| switches. | ||
|
|
||
| ### Cross-cutting | ||
|
|
||
| `tracing` and `metrics`. Their gates are not coupling. | ||
|
|
||
| ### Not subsystems | ||
|
|
||
| `src/message`, `src/send` and the shared plumbing under `src/features` are the | ||
| hot path and the core's own work. They fail tests 1 and 2 by construction. | ||
|
|
||
| ## The seam | ||
|
|
||
| `src/client/subsystem.rs` holds one `const` table: | ||
|
|
||
| ```rust | ||
| pub(crate) const SUBSYSTEMS: &[Subsystem] = &[ | ||
| #[cfg(feature = "passkey")] | ||
| crate::passkey::SUBSYSTEM, | ||
| ]; | ||
| ``` | ||
|
|
||
| `Client` gains one field, `subsystems`, not one per subsystem. A subsystem parks | ||
| its per-client state there and lists the notification types it models. With none | ||
| attached the table is a zero-length slice, so every loop over it folds away. | ||
|
|
||
| The table is a `const` rather than runtime registration because static | ||
| registration through a linker-section crate would trade the core's last gate for | ||
| a new dependency. The guard test is what keeps "one gate" enforceable instead. | ||
|
|
||
| The core's own match arms win: the table is consulted only for a notification | ||
| type the core does not model itself, so a claim on a type the core later starts | ||
| handling would silently stop arriving. | ||
| `a_claimed_notification_type_is_not_shadowed_by_a_core_arm` fails when that happens. | ||
|
|
||
| ## What a subsystem costs | ||
|
|
||
| Stripped `demo`, release profile, the build `binary_size_ci.md` gates on. Sizes | ||
| are deterministic for a pinned toolchain; the baseline reproduced byte for byte | ||
| across two runs. | ||
|
|
||
| | build | bin size | vs default | | ||
| | --- | ---: | ---: | | ||
| | default, `passkey` compiled in unconditionally (the old shape) | 10,806,752 | | | ||
| | default, `passkey` off | 10,756,992 | -48.6 KiB | | ||
| | default + `passkey` | 10,809,824 | +51.6 KiB | | ||
| | default + `plugins`, host on and no plugin installed | 10,960,960 | +150.6 KiB | | ||
| | default + `voip` | 11,373,952 | +553.9 KiB | | ||
|
|
||
| Turning the smallest cuttable subsystem off is worth ~49 KiB, and the seam that | ||
| makes it cuttable costs 2.5 KiB of `.text` when the subsystem is on. The | ||
| `plugins` row is the enabled-with-no-plugin number `plugin_architecture.md`'s | ||
| checklist asks for and that nothing in the repo had produced. | ||
|
|
||
| ## What the guard proves, and what it does not | ||
|
|
||
| `tests/subsystem_boundary.rs` fails when a cuttable subsystem is named outside | ||
| the files it owns and its two allowed core mentions. It scans text, so it sees a | ||
| mention in a comment too, which is deliberate: a comment in the core explaining | ||
| what a subsystem needs is the same coupling one commit early. | ||
|
|
||
| It does not reach test 3 (the subsystem calling core internals that exist only | ||
| for it), and it does not claim the disabled build carries zero bytes of the | ||
| subsystem: `Event` variants and payload types stay in `wacore` by test 4. "Zero | ||
| cost" here means zero code, state and branches of the subsystem's own. |
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,24 @@ | ||
| #!/usr/bin/env bash | ||
| # Prints the features of a package that the test suite runs with all at once, | ||
| # comma-separated for `cargo nextest --features`. | ||
| # | ||
| # Derived from cargo metadata so a feature added tomorrow is exercised without | ||
| # anyone editing CI. Only a feature that cannot share a build with the rest | ||
| # needs a line below, and each says why it cannot. | ||
| set -euo pipefail | ||
|
|
||
| package="${1:?usage: test_features.sh <package>}" | ||
|
|
||
| # danger-skip-* disable security verification, so the suite would change | ||
| # behavior rather than catch rot; the cert-chain negative test is | ||
| # itself cfg'd out under them. | ||
| # dhat-heap installs a global allocator, colliding with the counting one | ||
| # the allocation guards install. | ||
| # js, getrandom wasm32 backend selection, with no native build to join. | ||
| excluded='default|danger-skip-.*|dhat-heap|js|getrandom' | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| cargo metadata --no-deps --format-version 1 \ | ||
| | jq -r --arg package "$package" \ | ||
| '.packages[] | select(.name == $package) | .features | keys[]' \ | ||
| | grep -vxE "$excluded" \ | ||
| | paste -sd, | ||
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.
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.
Uh oh!
There was an error while loading. Please reload this page.