Repository navigation
cmux-app-ffi links the host budget C ABI; dispatch pressure flags read worst first (step A of the host pressure FFI move) - #31937
lawrencecchen wants to merge 13 commits into
Conversation
The review of #31799 found that cmux_host_budget_with_pressure_json reads its argument as one level (1, 2 or 4) and anything else as unknown. The app's dispatch source gives bit flags, and the Swift monitor reads them worst first: an event of 6 (warn and critical) is critical. Passed raw, a critical event would read unknown and drop the memory reason. These tests ask for the Swift reading (6 and 7 critical, 3 warn, 0 and 8 unknown) through the JSON call and the C ABI. Red step: they fail on the current exact-level mapping. The async meter tests now build only with the `tokio` feature, so the crate's tests also run with --no-default-features (the configuration cmux-app-ffi links).
Step A of the two-step FFI move of the app's host pressure copy (CmuxNextHostPressure). No Swift change: the app keeps its Swift copy until the re-pin lands and step B calls this ABI. - cmux-app-ffi depends on cmux-host-budget without its `tokio` feature and re-exports its C ABI as `host_budget` (as it does the palette ranker); its Cargo.lock gains no tokio. - build-app-ffi.sh ships include/cmux_host_budget_ffi.h as the module CCmuxHostBudgetFFI. - app-ffi-release.yml publishes on a change to the crate, which is now an FFI input (check-app-ffi-pin.sh --list-inputs lists it; app-ffi-release-paths.test.sh: ok). - cmux_host_budget_with_pressure_json takes the dispatch source's flags as they come and reads them worst first (4, then 2, then 1, else unknown), as the Swift monitor does: 6 is critical, 3 is warn. The sysctl level keeps its exact mapping (pressure_word), so acpmux's verdicts do not change.
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on 8390f22 to compare; do not merge until green; see lane-rules |
|
Found 6 test failures on Blacksmith runners: Failures
|
|
Review (codex gpt-6-astra high, next-review): SHIP at bda755a SHIP at bda755a Visual verdict: unchanged by this diff Reviewed the FFI, feature gating, lockfile, export packaging, pressure flag mapping, and release paths. Follow-ups: none |
|
Review (claude-review-subagent): SHIP at bda755a Checked: the merge leaves the PR paths unchanged; dispatch_pressure_word reads 4, then 2, then 1, like HostPressureMonitor.swift:133 (one difference: an event with no known bit is unknown here and normal in Swift, which cannot happen with that event mask); pressure_word and evaluate are unchanged; both new json tests and the ffi loop fail on the old exact-level mapping. Nothing outside cfg(feature = "tokio") needs tokio; the 10 header functions match ffi.rs and are the only names the export grep picks up, with no collision with the other four headers; the cmux-app-ffi Cargo.lock adds cmux-host-budget, dirs, tracing and their dependencies, and no tokio. |
|
Review (subagent, correctness first): SHIP at 5937437 Checked: the merges bda755a and 5937437 leave the PR's files the same as 9e29d1c (the diff is empty), and the diff from the merge-base 7ea60b1 contains only the 15 PR files. feat a7f48bb..7ea60b1 changes nothing in cmux-host-budget, cmux-app-ffi, build-app-ffi.sh, app-ffi-release.yml or acpmux host_pressure. The newer feat tip f22cb91 (#31945) merges cleanly with git merge-tree. app-ffi-release-paths.test.sh passes and rustfmt --check passes. #28790's chat-mirror entry does not come back, so the Takes back line is true. |
|
Classification, added at 515c0b9: clippy |
|
Review (codex gpt-6-astra high, next-review): SHIP at d7b04a5 SHIP at d7b04a5 Visual verdict: no images; Leo tries the build
Follow-ups:
|
|
Review (codex gpt-6.1-sol xhigh, next-review): SHIP at ec1ad74 SHIP at ec1ad74 Visual verdict: no images; Leo tries the build Focused workflow, license, release-path, and C-header checks passed. No correctness findings. Follow-ups:
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Step A of the two-step FFI move of the app's host pressure copy (backlog A10). The host CPU/RAM budget crate from #31799 (
cmux-host-budget, which acpmux's daemon already uses) is linked into cmux-app-ffi, so the next CCmuxAppFFI release carries its C ABI. No Swift change and no behavior change: nothing in the app calls the new symbols until step B.Check the app FFI pin: expected, step A of a two-step FFI move.
Takes back #28790: #28790 reverted one entry of
headers_ininscripts/cmux-next/build-app-ffi.sh(the chat index mirror header, which had landed without its release and pin). This PR does not add that entry back: it adds a different header,cmux-host-budget/include/cmux_host_budget_ffi.h, as the next entry of the same list, and lands it by the two-step FFI order (step A here, then the bot's release and re-pin, then step B), which is the order #28790 asked for.What changes
cmux-app-ffidepends oncmux-host-budgetwithdefault-features = false(no tokio; itsCargo.lockgains no tokio) and re-exports its C ABI ashost_budget, as it does the palette ranker.scripts/cmux-next/build-app-ffi.shshipscmux-tui/crates/cmux-host-budget/include/cmux_host_budget_ffi.has the moduleCCmuxHostBudgetFFI; itscmux_*functions join the export list..github/workflows/app-ffi-release.yml:cmux-tui/crates/cmux-host-budget/**joins the push paths. The crate is now an FFI input (check-app-ffi-pin.sh --list-inputslists it), and without the path a change to it would publish no release.scripts/cmux-next/tests/app-ffi-release-paths.test.shpasses. This is the only workflow change; no step, job or trigger changes.cmux_host_budget_with_pressure_jsonnow takes the dispatch source'sDISPATCH_MEMORYPRESSURE_*flags as they come and reads them worst first (4 critical, then 2 warn, then 1 normal, else unknown), as the Swift monitor (HostPressureMonitor.swift) does. One difference from Swift stays: an event with none of the three flags readsunknownhere and.normalin Swift. That case cannot happen, because the source's event mask is[.normal, .warning, .critical]; step B lists it with the other differences. Before, it read one exact level, so a combined event of 6 read "unknown" and dropped the memory reason. The sysctl mapping inevaluate(pressure_word) is unchanged, so acpmux's verdicts are the same.dirsin the crate:dirsbringsoption-ext(MPL-2.0) into the app library, and the app crate license gate does not allow MPL-2.0.src/home.rskeeps the rule ofdirs::home_dir(dirs-sys 0.5): first$HOMEwhen it is set and not empty; else the passwd entry of the real user (getuid,getpwuid_r, a buffer of_SC_GETPW_R_SIZE_MAXbytes or 512, no retry); else none. Android, iOS and Emscripten read no passwd entry.home_tests.rstests a setHOME, an unsetHOMEand an emptyHOME.cmux-app-ffi/Cargo.locklosesdirs,dirs-sys,option-extand the crates they brought in.scripts/cmux-next/check_app_crate_licenses.pynamescmux-host-budgetas a first-party GPL crate of the app closure, so the real gate does not report it. The gate also reportscmux-motion. That is a base finding, and the follow-up PR fixes it.tokiofeature, so the crate's tests also build without default features (the configuration cmux-app-ffi links).Step B, after the re-pin is on feat:
CmuxNextHostPressurecalls this ABI and its Swift formula, sampler, verdict init,with(pressure:)and tracker are deleted; Swift keeps the dispatch source, the 15 s timer, the daemon read, the last verdict and the views. Step B lists the verdict differences between the Swift copy and the daemon's code.Risk: high (touches
app-ffi-release.yml). Lands on an independentReview (<reviewer>): SHIP at <head>receipt.Changelog
none
Observed / Not run
Observed at the head 515c0b9. This is my plain merge of feat-cmux-next. It brings in #31927 (the hover C ABI in cmux-app-ffi) and #32374 (the host pressure warning is off by default, with a palette toggle). The only conflict was the module doc in
cmux-app-ffi/src/lib.rs, and I kept both sides. The Swift files are the same as on feat:34d49d140856819bc09dfb75and clippy47247376426894db2734ca75: PENDING.At 12232b6 these passed: panic
ceedc091d22484e704d9e818and clippy50ecb9b70b3061cb96ea16c4. The PR comment issuecomment-6581650029 classifies the two red checks at that head: "Check the app FFI pin" is the expected step A red, and the daemon test is a load timeout inSnapshotSessionBarrierTests.Observed at the head 12232b6. This is my plain merge of feat-cmux-next. It brings in #32282, the fix for the read_paths.rs clippy base red, and cmux-cloud-start-core, which joins the app FFI:
ceedc091d22484e704d9e818and clippy50ecb9b70b3061cb96ea16c4: PENDING.cmux-tui/crates/cmux-app-ffiPR check at this head and the fleet clippy, both with--locked.At bf3bd79 (cmux-bot's merge; an independent SHIP was given there), clippy
6ca0c072d77b9feff4d3504awas red only on the base red incmux-tui-core/src/mux/tests/read_paths.rs(lines 201, 244, 249 and 237). #32177 added it, and #32282 fixes it. The PR comment classifies it.Observed at 3aa5af9 (the PR's own last commit; an independent SHIP was given there):
test home::tests: passed on Linux (1e06ba3c0988e5a9ae3f5385) and on macOS (109cb7f6af94f5b91f2f255a).31b7cb94e3a9cf591fae2af3, base 7ea60b1cbda8d3d961010a620fcec9a. Both show the same counts, cmux-tuiindexing_slicing 835 > 831andstring_slice 84 > 82. That is the base red from NEXT Cloud DEV integration: new Worker and app-server link #28950. The PR has 0 own hits.b2bb2529555cafb701f3bbce: red only on cmux-tui-coreoutput_store/snapshot.rs:537too_many_arguments. That is a base red that fix(cmux-tui-core): snapshot_written takes the snapshot's end as one value (base red: clippy too_many_arguments) #32092 fixes. The PR has 0 own hits.Observed earlier (fleet, Linux), for the change that has not changed since:
test json::tests, jobfc6ead8a4bb87f71e43b134f: the two new tests fail (the_dispatch_flags_read_worst_first,a_combined_critical_event_keeps_the_memory_reason). The other 7 pass.test, jobb55e0534d1695a1e226a711a. All cmux-host-budget and cmux-app-ffi tests pass. 8 tests fail, and each also failed when the rerun ran it alone. None of them is in a file that this PR changes:terminal_host_recovery::host_death::kill_all_restoretests: a Linux base red from fix(cmux-tui): a kill of everything restores each terminal's exact text, then the session line, in the same directory string (REOPEN-F3) #31643, which the REOPEN-F3 owner has.connection_handler_owns_every_router_connection_operation: a base red from feat(chief): continue stopped turns and retry replies in Home #30774. base red: the router owns 58 connection operations (chief.continue, chief.retry from #30774) #31939 fixes it.every_test_binary_runs_in_the_hermetic_environment(5472a06e7265dc6ae1ebaa3d; from acpmux: a stopping daemon's retry pass yields and is bounded; only the lock holder unlinks the socket; a lost socket is bound again #31593)share_chat_waits_for_the_owners_answer(b973957eb8a382f63c3d57c0; from cmux NEXT: a guest link an agent or script asks for waits for the owner's Allow (#30263) #30550)agent_message_launch_credential(fae49a161128474d850f2ae9) andagent_message_reply_in_place(e67e80ddb524176ae2196fba): both are a tokio worker stack overflow (SIGABRT) at the base too.4933a9b07161ec03e933c9f3, and at the head bda755a, job55fb7ac3fa1b825aeb6f500f: both passed. This PR adds no unwrap, expect, panic or indexing in production code.4c44dee9b4a6c1cafe72e284: passed.dffc83f5f08aa4b8f58d36d8: passed. At 9e29d1c (5a9189ff5de8a62034f38b8e), clippy was red only on the base red in cmux-tui-coreagent_chat_notices.rs; base red: agent_chat_notices clippy (obfuscated if/else, a missing ;, a redundant clone) from #31770 #31907 fixed it, and the bot's merge brought that fix in.cargo metadata --offline. The proof that they resolve is in jobs that ran cargo with--lockedat the head bf3bd79:cmux-app-ffi/Cargo.lock: the PR checkcmux-tui/crates/cmux-app-ffi(workflow "rust workspaces", run 41442794928). It rancargo clippy --locked --workspace --all-targets -- -D warningsandcargo test --locked --workspacein cmux-app-ffi, and passed.cmux-tui/Cargo.lock: the fleet clippy6ca0c072d77b9feff4d3504a, which runscargo clippy --workspace --locked.swift-logic-lines.py:swift_logic_linesis 415771 at the head 9e29d1c and 415771 at the base 49aee50;mirror_linesis 70597 at both. There is no Swift change. Step B lowers them.Review: independent SHIP at bda755a (the bot's merge of feat a7f48bb). cmux-bot merged feat again at 09:56Z (head 5937437, feat 7ea60b1); neither merge changes this PR's files, and the head gets its own review receipt. Its non-blocking notes go to step B so that this head stays the reviewed one: the
pressure_worddoc still names the dispatch flags, and with--no-default-featuresmeter_tests.rsimportsincidentbut does not use it (a warning, not an error).The configuration the app links (cmux-host-budget without default features, inside cmux-app-ffi):
cmux-tui/crates/cmux-app-ffi(workflow "rust workspaces", run 41416238787, at bda755a, Linux) rancargo clippy --locked --workspace --all-targets -- -D warningsandcargo test --locked --workspacein cmux-app-ffi. It compiled cmux-host-budget with default features off, and both passed.test ffi::tests --os macos, job3a62fddc76e968d8a2aa2ca3on aws-m4pro-3: passed.Not run:
scripts/cmux-next/build-app-ffi.sh: the release slices, the prelink with the export list,xcodebuild -create-xcframework). The fleet refuses it: the script is not on the controller's ci-step allowlist ("invalid ci-step: script is not on the ci-step allowlist"). app-ffi-release.yml runs it after landing. Anything in Swift.