Count queries served between polls as breadsearch activity #50

Merged

Breadway merged 2 commits from fix/ram-82-queries-served into main 2026-10-16 16:49:39 +08:00

Conversation 3 Commits 2 Files changed 1 +129 -8

Breadway commented 2026-10-16 16:34:44 +08:00

Owner

Copy link

Closes RAM-82

The breadsearch adapter derived idleness only from polled in_flight, pending and loading. A query holds in_flight for milliseconds and the daemon polls every 2 to 3 s, so a mill answering a query every few seconds was read as idle and unloaded. The RAM-30 contention benchmark reproduced this in 3 of 3 runs.

The adapter now also reads an optional queries_served counter from the status. Any change between two readings is activity, a counter that goes backwards (daemon restart) is activity, and a daemon without the field behaves exactly as before. The counter is added by breadsearch PR for feature/status-queries-served (commit 7e5eef9); this change is safe to merge first because the field is optional and absent means no signal.

Tests: four new adapter tests (counter moving between polls, flat counter and idle restart, counter going backwards, daemon without the counter). With the counter ignored, three of the four fail, so they exercise the change. cargo test --workspace 596 passed, clippy -D warnings clean.

Live: scripts/bench/bench-contention.sh from feature/ram-30-benchmarks, run against this build and a breadmill built from 7e5eef9, three runs, each showing zero busy-mill unloads and one confirmed unload of the quiet mill (docs/benchmarks/ram30/release/contention-fixed-*.jsonl on that branch). Before the change the same bench unloaded the busy mill in 3 of 3 runs. The earlier single-mill 45 s window is not counted as evidence of protection.

Closes RAM-82 The breadsearch adapter derived idleness only from polled in_flight, pending and loading. A query holds in_flight for milliseconds and the daemon polls every 2 to 3 s, so a mill answering a query every few seconds was read as idle and unloaded. The RAM-30 contention benchmark reproduced this in 3 of 3 runs. The adapter now also reads an optional queries_served counter from the status. Any change between two readings is activity, a counter that goes backwards (daemon restart) is activity, and a daemon without the field behaves exactly as before. The counter is added by breadsearch PR for feature/status-queries-served (commit 7e5eef9); this change is safe to merge first because the field is optional and absent means no signal. Tests: four new adapter tests (counter moving between polls, flat counter and idle restart, counter going backwards, daemon without the counter). With the counter ignored, three of the four fail, so they exercise the change. cargo test --workspace 596 passed, clippy -D warnings clean. Live: scripts/bench/bench-contention.sh from feature/ram-30-benchmarks, run against this build and a breadmill built from 7e5eef9, three runs, each showing zero busy-mill unloads and one confirmed unload of the quiet mill (docs/benchmarks/ram30/release/contention-fixed-*.jsonl on that branch). Before the change the same bench unloaded the busy mill in 3 of 3 runs. The earlier single-mill 45 s window is not counted as evidence of protection.

Breadway added 1 commit 2026-10-16 16:34:44 +08:00

fix: count queries served between polls as breadsearch activity 532fb5d055

Breadway commented 2026-10-16 16:42:56 +08:00

Author

Owner

Copy link

REVIEW VERDICT: changes_requested (formal review blocked: the token account authored this PR)

Independent review of head 532fb5d055 (base d32197d). Verdict: changes requested, two small items. The logic is correct; nothing here needs a redesign.

Checked on that exact SHA in a scratch clone:

• cargo test --workspace --locked: all pass (421 core unit tests, no failures)
• cargo clippy --workspace --all-targets --locked -- -D warnings: clean
• cargo fmt --check: clean
• Forgejo CI "CPU checks / check (push)" on 532fb5d: success
• No em dashes, no AI attribution in the commit or the diff

Criteria:

1. shows_work edge cases: correct. First reading has no previous, so no counter signal and the idle clock starts. Counter on one side only gives no signal. Equal is quiet, lower is activity, in_flight/pending/loading win before the counter is consulted. Unit tests cover first reading, moved, flat, backwards, and absent on both sides. Mixed presence (counter on one side only) is not tested; optional.
2. Previous status in both paths: correct. refresh and note_socket_result both compare against observation.status, which each successful read replaces. Reasoning about a confirmation read between two polls: poll at 10, query runs, confirmation read sees 11, so shows_work is true there and idle_since_ms is cleared; the next poll sees 11 against 11 and restarts the clock from that poll. The moved counter is not swallowed and the confirmation read does not reset the comparison. A failed confirmation read leaves the previous status in place, so the comparison survives it. A failed refresh clears status, so a counter move during an outage is not compared; the idle clock restarts anyway, so that is conservative.
3. First-reading behaviour: acceptable. A mill that is busy but has no in-flight query at the first poll reads as idle for one poll interval (2 to 3 s), and the second poll resets it on a moved counter. MIN_IDLE_MS is 30 s, so no unload can be planned inside that window.
4. Tests vs mutation: I changed shows_work to ignore the counter. Three of the four new tests fail (moved between polls, flat-then-moved, backwards) and the without-counter test passes, as it should. Changing before != now to now > before fails only the backwards test. BUT see item A below: replacing observation.status.as_ref() with None inside note_socket_result survives the whole workspace test suite, so half of the change has no test.
5. Wire compatibility: the field is Option with #[serde(default)], no deny_unknown_fields, so an older daemon parses and an older breadram ignores the field. breadsearch PR 16 (7e5eef9) adds it as an Option with serde default, matching. This PR is safe to merge before PR 16.
6. Benchmark evidence: see item B.

Requested before merge:
A. Add a test for the note_socket_result path (crates/breadram-core/src/adapter/breadsearch.rs, tests module). Mutating note_socket_result to call shows_work(None, &status) passes every existing test, so the executor confirmation-read path is unpinned. This test passes on the current code and fails under that mutation (I ran both):

#[test]
fn a_confirmation_read_compares_against_the_previous_reading() {
    let adapter = adapter();
    adapter.refresh(&ScriptedSockets::answer(&served(10)), 60_000);
    // A confirmation read lands between two polls and sees the counter moved.
    let notes = adapter.note_socket_result(Ok(served(11)), 62_000);
    assert!(notes.is_empty());
    assert_eq!(adapter.activity(&workload()), ActivityEvidence::Unknown);
    // The next poll is flat against the confirmation read, so idleness restarts there.
    adapter.refresh(&ScriptedSockets::answer(&served(11)), 63_000);
    assert_eq!(
        adapter.activity(&workload()),
        ActivityEvidence::Idle { since_ms: 63_000 }
    );
}

B. The PR body cites docs/benchmarks/ram30/release/contention-fixed-{1,2,3}.jsonl on feature/ram-30-benchmarks, but those files are not on that branch on the remote (head ae63ff5, release/ has only contention-{1,2,3}). They exist only as untracked files in the local ram30 worktree. I read them there: all three show busy_mill_unloads 0, quiet_mill_unloads 1, busy_mill_protected true, and the pre-fix remote files show busy_mill_unloads 1 in each run, so the evidence is consistent with the claim. Commit and push the fixed-run files (and stderr) to feature/ram-30-benchmarks, or reword the PR body so it does not point at files a reader cannot fetch. This is not part of this SHA, so no code change is needed on this branch.

Non-blocking notes:

• breadsearch PR 16 increments queries_served only after the model-present check in dispatch. A query that hits an unloaded model starts a reload and returns before the counter moves. The reload sets loading, which this adapter already treats as work, but if a poll misses the load window the demand-reload query leaves no trace and the mill reads as long idle again straight after the reload. Pre-existing gap, not caused by this PR; worth a card against breadsearch (count the query at dispatch entry).
• a_counter_that_goes_backwards asserts assert_ne Idle rather than eq Unknown; tightening it would make it stronger.

Next: push item A. I will re-review the new head and merge if CI is green on it.

REVIEW VERDICT: changes_requested (formal review blocked: the token account authored this PR) Independent review of head 532fb5d0554a63ca5137ac84d47112dda6838462 (base d32197d). Verdict: changes requested, two small items. The logic is correct; nothing here needs a redesign. Checked on that exact SHA in a scratch clone: - cargo test --workspace --locked: all pass (421 core unit tests, no failures) - cargo clippy --workspace --all-targets --locked -- -D warnings: clean - cargo fmt --check: clean - Forgejo CI "CPU checks / check (push)" on 532fb5d: success - No em dashes, no AI attribution in the commit or the diff Criteria: 1. shows_work edge cases: correct. First reading has no previous, so no counter signal and the idle clock starts. Counter on one side only gives no signal. Equal is quiet, lower is activity, in_flight/pending/loading win before the counter is consulted. Unit tests cover first reading, moved, flat, backwards, and absent on both sides. Mixed presence (counter on one side only) is not tested; optional. 2. Previous status in both paths: correct. refresh and note_socket_result both compare against observation.status, which each successful read replaces. Reasoning about a confirmation read between two polls: poll at 10, query runs, confirmation read sees 11, so shows_work is true there and idle_since_ms is cleared; the next poll sees 11 against 11 and restarts the clock from that poll. The moved counter is not swallowed and the confirmation read does not reset the comparison. A failed confirmation read leaves the previous status in place, so the comparison survives it. A failed refresh clears status, so a counter move during an outage is not compared; the idle clock restarts anyway, so that is conservative. 3. First-reading behaviour: acceptable. A mill that is busy but has no in-flight query at the first poll reads as idle for one poll interval (2 to 3 s), and the second poll resets it on a moved counter. MIN_IDLE_MS is 30 s, so no unload can be planned inside that window. 4. Tests vs mutation: I changed shows_work to ignore the counter. Three of the four new tests fail (moved between polls, flat-then-moved, backwards) and the without-counter test passes, as it should. Changing before != now to now > before fails only the backwards test. BUT see item A below: replacing observation.status.as_ref() with None inside note_socket_result survives the whole workspace test suite, so half of the change has no test. 5. Wire compatibility: the field is Option<u64> with #[serde(default)], no deny_unknown_fields, so an older daemon parses and an older breadram ignores the field. breadsearch PR 16 (7e5eef9) adds it as an Option with serde default, matching. This PR is safe to merge before PR 16. 6. Benchmark evidence: see item B. Requested before merge: A. Add a test for the note_socket_result path (crates/breadram-core/src/adapter/breadsearch.rs, tests module). Mutating note_socket_result to call shows_work(None, &status) passes every existing test, so the executor confirmation-read path is unpinned. This test passes on the current code and fails under that mutation (I ran both): #[test] fn a_confirmation_read_compares_against_the_previous_reading() { let adapter = adapter(); adapter.refresh(&ScriptedSockets::answer(&served(10)), 60_000); // A confirmation read lands between two polls and sees the counter moved. let notes = adapter.note_socket_result(Ok(served(11)), 62_000); assert!(notes.is_empty()); assert_eq!(adapter.activity(&workload()), ActivityEvidence::Unknown); // The next poll is flat against the confirmation read, so idleness restarts there. adapter.refresh(&ScriptedSockets::answer(&served(11)), 63_000); assert_eq!( adapter.activity(&workload()), ActivityEvidence::Idle { since_ms: 63_000 } ); } B. The PR body cites docs/benchmarks/ram30/release/contention-fixed-{1,2,3}.jsonl on feature/ram-30-benchmarks, but those files are not on that branch on the remote (head ae63ff5, release/ has only contention-{1,2,3}). They exist only as untracked files in the local ram30 worktree. I read them there: all three show busy_mill_unloads 0, quiet_mill_unloads 1, busy_mill_protected true, and the pre-fix remote files show busy_mill_unloads 1 in each run, so the evidence is consistent with the claim. Commit and push the fixed-run files (and stderr) to feature/ram-30-benchmarks, or reword the PR body so it does not point at files a reader cannot fetch. This is not part of this SHA, so no code change is needed on this branch. Non-blocking notes: - breadsearch PR 16 increments queries_served only after the model-present check in dispatch. A query that hits an unloaded model starts a reload and returns before the counter moves. The reload sets loading, which this adapter already treats as work, but if a poll misses the load window the demand-reload query leaves no trace and the mill reads as long idle again straight after the reload. Pre-existing gap, not caused by this PR; worth a card against breadsearch (count the query at dispatch entry). - a_counter_that_goes_backwards asserts assert_ne Idle rather than eq Unknown; tightening it would make it stronger. Next: push item A. I will re-review the new head and merge if CI is green on it.

Breadway referenced this pull request 2026-10-16 16:42:56 +08:00

breadsearch adapter treats a mill under short repeated queries as idle and unloads it #49

Breadway added 1 commit 2026-10-16 16:44:13 +08:00

test: pin the confirmation read path of the breadsearch activity check f92dfebb14

Breadway commented 2026-10-16 16:49:28 +08:00

Author

Owner

Copy link

REVIEW VERDICT: pass (formal review not possible: the token account authored this PR)

Independent re-review of head f92dfebb14 (base d32197d7a1). Prior verdict (comment 5240, head 532fb5d) had items A and B. Both are resolved.

Checked on that exact SHA in a scratch clone:

• cargo test --workspace --locked: 597 passed, 0 failed
• cargo clippy --workspace --all-targets --locked -- -D warnings: clean
• cargo fmt --all --check: clean
• Forgejo CI "CPU checks / check (push)" on f92dfeb: success (1m29s)
• No em dashes and no AI attribution in the diff or the two commits

Item A (confirmation-read test): pass. a_confirmation_read_compares_against_the_previous_reading drives note_socket_result with a moved counter and asserts Unknown, then a flat poll restarts idleness from that poll. Mutation run myself: note_socket_result changed to shows_work(None, &status). Result: that test FAILS (panic at breadsearch.rs:768), the other breadsearch tests still pass, confirming the test pins exactly that path. Mutation reverted, tree clean.

Tightened assertion: pass. a_counter_that_goes_backwards_is_activity_not_idleness now asserts Unknown. Traced: first reading starts the idle clock, the backwards reading makes shows_work true and clears idle_since_ms, and activity() returns Unknown when in_flight, pending and loading are all zero and no idle clock is set. Unknown is the correct value and a stronger check than not-Idle, since it would fail if the clock were left running.

Item B (benchmark evidence): pass. origin/feature/ram-30-benchmarks at 8c2b684 contains docs/benchmarks/ram30/release/contention-fixed-{1,2,3}.jsonl and .stderr, fetched and read. Each jsonl shows busy_mill_unloads 0, quiet_mill_unloads 1, busy_mill_protected true, contention_resolved true, daemon_policy_driven true; each stderr is "ok". This matches the PR body claim (three runs, zero busy-mill unloads, one confirmed quiet-mill unload). The two bench scripts (bench-contention.sh, bench-breadsearch-daemon.sh) gained BREADRAM_BENCH_DAEMON and BREADRAM_BENCH_CLI overrides, as described.

Scope: one file changed in this PR (crates/breadram-core/src/adapter/breadsearch.rs), no unrelated churn. Optional counter on one side only gives no signal, so older daemons behave as before and merging before the breadsearch change is safe.

REVIEW VERDICT: pass (formal review not possible: the token account authored this PR) Independent re-review of head f92dfebb149f5d51a8cd9ed79d6f98dc7d3fe47a (base d32197d7a153892c6bf6a0c4ae3e499caed68c3b). Prior verdict (comment 5240, head 532fb5d) had items A and B. Both are resolved. Checked on that exact SHA in a scratch clone: - cargo test --workspace --locked: 597 passed, 0 failed - cargo clippy --workspace --all-targets --locked -- -D warnings: clean - cargo fmt --all --check: clean - Forgejo CI "CPU checks / check (push)" on f92dfeb: success (1m29s) - No em dashes and no AI attribution in the diff or the two commits Item A (confirmation-read test): pass. a_confirmation_read_compares_against_the_previous_reading drives note_socket_result with a moved counter and asserts Unknown, then a flat poll restarts idleness from that poll. Mutation run myself: note_socket_result changed to shows_work(None, &status). Result: that test FAILS (panic at breadsearch.rs:768), the other breadsearch tests still pass, confirming the test pins exactly that path. Mutation reverted, tree clean. Tightened assertion: pass. a_counter_that_goes_backwards_is_activity_not_idleness now asserts Unknown. Traced: first reading starts the idle clock, the backwards reading makes shows_work true and clears idle_since_ms, and activity() returns Unknown when in_flight, pending and loading are all zero and no idle clock is set. Unknown is the correct value and a stronger check than not-Idle, since it would fail if the clock were left running. Item B (benchmark evidence): pass. origin/feature/ram-30-benchmarks at 8c2b684 contains docs/benchmarks/ram30/release/contention-fixed-{1,2,3}.jsonl and .stderr, fetched and read. Each jsonl shows busy_mill_unloads 0, quiet_mill_unloads 1, busy_mill_protected true, contention_resolved true, daemon_policy_driven true; each stderr is "ok". This matches the PR body claim (three runs, zero busy-mill unloads, one confirmed quiet-mill unload). The two bench scripts (bench-contention.sh, bench-breadsearch-daemon.sh) gained BREADRAM_BENCH_DAEMON and BREADRAM_BENCH_CLI overrides, as described. Scope: one file changed in this PR (crates/breadram-core/src/adapter/breadsearch.rs), no unrelated churn. Optional counter on one side only gives no signal, so older daemons behave as before and merging before the breadsearch change is safe.

Breadway merged commit 75bccaf214 into main 2026-10-16 16:49:39 +08:00

Breadway deleted branch fix/ram-82-queries-served 2026-10-16 16:49:40 +08:00

Breadway commented 2026-10-16 16:52:32 +08:00

Author

Owner

Copy link

Merged as 75bccaf214 (merge commit, head f92dfebb14, base d32197d). Head branch fix/ram-82-queries-served deleted. Main CI 'CPU checks / check (push)' on the merge SHA: success (1m36s). RAM-82 is awaiting verification of the integrated behaviour, not done: the live check needs a breadmill built from the breadsearch queries_served change, which is not merged.

Merged as 75bccaf214dfab3638a3d026bd4bacc001af29c5 (merge commit, head f92dfebb149f5d51a8cd9ed79d6f98dc7d3fe47a, base d32197d). Head branch fix/ram-82-queries-served deleted. Main CI 'CPU checks / check (push)' on the merge SHA: success (1m36s). RAM-82 is awaiting verification of the integrated behaviour, not done: the live check needs a breadmill built from the breadsearch queries_served change, which is not merged.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Breadway/breadram!50

No description provided.