Repository navigation
perf(psmux): start bmad-loop's psmux servers without the warm pool - #909
Conversation
|
@codex review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe psmux backend now disables warm pools for sessions it creates and refuses additional argv-shaped option values. Tests and documentation cover both changes. Changespsmux backend behavior
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
The whole diff also changes _transportable to reject --, -t..., and -X=value option values. It adds related backend tests and documentation. These changes implement the separate #905 option-ha… |
Remove the #905 option-handling code, tests, and documentation from this pull request, or rebase this pull request so those changes remain in the appropriate stacked pull request. Keep the #903 warm-pool change and its directly supporting t… |
| Docstring Coverage | |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the primary change: starting bmad-loop's psmux servers without the warm pool. Linked Issues check Issue #903requiresPSMUX_NO_WARM=1for the server started byPsmuxMultiplexer.new_session. The backend change sets this value only in the child environment, andtests/test_psmux_backend.pyasse…
Full details: Out of Scope Changes check
-
Explanation
The whole diff also changes
_transportableto reject--,-t..., and-X=valueoption values. It adds related backend tests and documentation. These changes implement the separate#905option-handling work and do not support the#903warm-pool objective.Resolution
Remove the
#905option-handling code, tests, and documentation from this pull request, or rebase this pull request so those changes remain in the appropriate stacked pull request. Keep the#903warm-pool change and its directly supporting tests and documentation.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the psmux gate
And sends warm spares to hibernate
It tests each dash-shaped value twice
Then nibbles greens, content and nice
The changelog marks the trail it made
Comment @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdc723772f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert (root / f"{session}.port").is_file(), "probe setup: the session has no port file" | ||
| # The standby is spawned during the server's boot; give it the time it | ||
| # takes on a loaded box before reading absence as an answer. | ||
| deadline = time.monotonic() + 10 |
There was a problem hiding this comment.
On a loaded Windows psmux run, a warm standby can still be within psmux's normal 15-second server-registration window (tests/psmux_teardown.py:22-27) when this ten-second deadline expires. If PSMUX_NO_WARM is removed or stops working, this test can therefore assert absence before __warm__.port appears, and its teardown can return through the already-seen main session before the late standby registers, so the required ablation is not reliable; reuse the established readiness deadline or a registry/process witness that covers it.
AGENTS.md reference: AGENTS.md:L65-L65
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 095fc07. The row now waits psmux_teardown.PSMUX_READY_DEADLINE_S (psmux's 15 s readiness window) before it reads absence as an answer. If a standby still registers late, a kill-server scoped to the test's private registry root reaps it after the session teardown. Re-run from PowerShell 7 on psmux 3.4.0: it passes with the fix (19 s) and fails when new_session's PSMUX_NO_WARM is ablated. No psmux process was left.
| if value == "--" or value.startswith("-t") or re.match(r"-[A-Za-z]=.", value): | ||
| return False |
There was a problem hiding this comment.
When a caller supplies an @ option value such as -tfoo, -x=foo, or --, this branch now warns and clears the key, but docs/multiplexer-backends.md:102-106 still lists only empty, quoted, multiline, and edge-whitespace values as refused. Update that current-behavior reference so consumers can predict the new warning/key-clear behavior instead of relying on the release history.
AGENTS.md reference: AGENTS.md:L76-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7b73024, on #905's branch (#908), because this code comes from that commit and this PR is stacked on it. The refused-values paragraph in docs/multiplexer-backends.md now also names the three dash-leading shapes (-X=value, -t…, --) and says that other dash-leading values pass. tests/test_docs_hygiene.py passes 8/8.
bdc7237 to
095fc07
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…annot carry psmux 3.4.0's client argv normalizers rewrite a few dash-leading values before set-option parses them (psmux/psmux#795): -X=value becomes -X value at rc 0, and -t... and -- are refused with rc 1, which left a stale value in place. _transportable now refuses those shapes, so they take the existing visible path: a warning, and the key freed. Every other dash-leading value stays admitted. A live premise row turns red when a psmux release stores -x=foo verbatim, so the refusal can retire behind a floor bump. Closes bmad-code-org#905
The refused-values paragraph named only empty, quoted, multiline and edge-whitespace values. It now also names the three dash-leading shapes that psmux's client rewrites before parsing (-X=value, -t..., --), which take the same warn-and-unset path.
psmux 3.4.0 gives every server a warm pool of spare shells, and every registry a hidden __warm__ standby server. Both serve only default-shell creates. Every window bmad-loop opens carries a command and every session passes -c, so neither was ever claimed, yet together they cost about 0.75 GiB of summed working sets per idle project. The new-session call that boots a bmad-loop server now carries PSMUX_NO_WARM=1, scoped to that spawn, so the operator's own psmux keeps its pool. Measured over 3 runs: an idle project drops from 16 psmux-tree processes and about 935 MiB to 4 and about 175 MiB. Closes bmad-code-org#903
…ndby The no-warm-standby row read absence after 10 s, inside psmux's 15 s server registration window, so a late standby could pass it on a loaded box. It now waits PSMUX_READY_DEADLINE_S, and kills a late standby's server with a kill-server scoped to the test's private registry.
095fc07 to
a765a4f
Compare
2 participants
Stacked on #908 (#905). Merge #908 first; until then this diff also shows its commit.
psmux 3.4.0 gives every server a warm pool of spare shells, and every registry a hidden
__warm__standby server. Both serve only default-shell creates. Every window bmad-loop opens carries a command, and every session it creates passes-c, so neither was ever claimed. Together they cost about 0.75 GiB of summed working sets per idle project.Change
The
new-sessioncall that boots a bmad-loop psmux server now carriesPSMUX_NO_WARM=1in its environment. It is scoped to that one spawn and never set globally, so the operator's own psmux keeps its pool.The comment at the change records two ceilings:
set -g warm onre-enables the pool when the config loads. That stays the operator's call: forcing warm off at runtime would also kill the registry's shared standby.Measured (3 runs, psmux 3.4.0, PowerShell 7,
SHELLunset)These are summed working sets (MiB), not exclusive RAM.
Tests
tests/test_psmux_backend.pyasserts the environment handed to the server-booting call.tests/test_psmux_live.pyadds a row asserting that no__warm__standby appears for a session bmad-loop started. The row pins an emptyPSMUX_CONFIG_FILE, so a user config cannot mask the result.Closes #903
Summary by CodeRabbit
--, beginning with-t, or matching a single-letter option followed by=and a value are now refused. Other leading-hyphen values remain supported.