Skip to content

test(proxy): accept the ask over turn 3's own transcript in the content-change decline test - #568

Open
OsherElhadad wants to merge 1 commit into
mainfrom
test/cache-aware-declines-deterministic
Open

OsherElhadad wants to merge 1 commit into
mainfrom
test/cache-aware-declines-deterministic

Conversation

@OsherElhadad

Copy link
Copy Markdown
Collaborator

What

TestCacheAwareRealContentChangeInsideSpanStillDeclines failed once in CI (got 1 ask calls) and never locally.

Root cause: it asserts the commission declines, but that depends on a race it does not control. The summary call runs detached, and the host stashes turn 3's own body only after the pipeline returns (see the STALE-PREFIX GUARD comment in cache_aware_summarizer.go).

  • The call runs first: nothing is stashed for the new thread (ErrNoPrefix), or turn 2's stash does not match (ErrStalePrefix). It declines. This is the only ordering the test allowed.
  • The call runs after the stash: the stash is turn 3's own body, the span hash matches by construction, and the ask is made over turn 3's own, self-consistent transcript. That is correct behaviour, not a product bug.
    The ask in the CI log carries turn 3's edited message (MARK-T2: EDITED) and none of turn 2's text, which is the second ordering.

Fix

The test now accepts either ordering. It fails if an ask carries a transcript that is not turn 3's own (turn 2's "making progress" under turn 3's span), and when nothing was asked it still requires one of the two decline counters to move.

Evidence

Delaying the detached call by 300 ms (scratch edit, not committed) makes the original test fail deterministically with the CI message and the new test pass. go test -race -count=10 -run TestCacheAware ./proxy passes.

@OsherElhadad OsherElhadad left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: APPROVE at 7bd4fc8. (Posted as a comment: GitHub does not let the author account approve.) Test-only.

On the third outcome (an ask over turn 3's own transcript): it is safe product behaviour, not a bug.

  • The span claim matches what the model reads. The guard runs inside the detached call, right before paying for it. It checks CoversSpan(session, requiredCount, requiredHash), where requiredHash is the hash of turn 3's own pre-pipeline span. When the call runs after the host stashed turn 3's body, that stash is turn 3's raw input, so the check passes by construction and the ask is answered from the transcript the span claims.
  • The result can only land on the same transcript. The checkpoint's CoveredHash is spanHash of turn 3's covered span (cache_aware_async.go:245). tryReuse splices it only into a later request whose msgs[start:start+CoveredCount] hashes to the same value (cache_aware_summarizer.go:1076). A summary of the edited transcript can therefore never be applied over a different one.
  • The cost is justified. The guard exists to decline before paying for an answer from the wrong transcript. Here the transcript is the right one, so the call buys a valid checkpoint for the conversation as it now stands.
  • The CI log matches this ordering. The failing ask contains MARK-T2: EDITED and none of turn 2's text.

A real, but separate and pre-existing, narrow race (not introduced here, not blocking). CoversSpan and Ask read the stash under separate lock holds: CoversSpan checks one snapshot, then Ask calls stash.get again. If the next turn's body is stashed in between:

  • In the common case, it extends the same thread, so the first requiredCount messages still match. The model then sees more messages than the checkpoint claims to cover; the summary may mention later content, which is harmless.
  • In the rare case of a same-thread-key body whose prefix differs, Ask would answer from a transcript that CoversSpan never approved.

The fix is to take the snapshot once: have CoversSpan return the body it approved, and ask on that body. Worth an issue.

About the test. With the CoversSpan guard removed entirely, this test still passes, on main's version as well as this one. The edit at index 4 makes turn 3 a new thread, so the call hits ErrNoPrefix, or turn 3's own stash, and never reads turn 2's body. So this test pins thread detection plus "no wrong-transcript ask", not the hash guard itself; the hash guard is covered by the pr431 stale-prefix tests. The new assertion (any ask must carry MARK-T2: EDITED and not "making progress") is no weaker than the old one at catching a wrong-transcript answer.

Checks. gofmt and go vet are clean. go test -race ./proxy -run CacheAware -count=20 passes (nice 10). CI is green, and the commit is signed off with no AI attribution.

This approval covers 7bd4fc8.

…nt-change decline test

The commissioned summary runs detached and the host stashes turn 3's own body only after the pipeline returns. When the call runs first it declines (NoPrefix or StalePrefix); when it runs after the stash, the stash is turn 3's own body, the span hash matches by construction, and the ask is made over a consistent transcript. CI hit the second ordering once (the ask body held turn 3's edited message and none of turn 2's text). The test asserted the first ordering. It now accepts either, and fails if an ask carries a transcript that is not turn 3's own.

Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
@OsherElhadad
OsherElhadad force-pushed the test/cache-aware-declines-deterministic branch from 7bd4fc8 to 51b04a7 Compare October 15, 2026 16:31

@OsherElhadad OsherElhadad left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-confirm: APPROVE at 51b04a7. Rebased onto today's main. git range-diff against the approved 7bd4fc8 shows all 1 of the PR's own commits identical (=), CI is green, and the commits are signed off. This approval covers 51b04a7.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New/ToDo

Development

Successfully merging this pull request may close these issues.

2 participants