fix(gateway): keep native images on resolved session key (#48912) - #48939
fix(gateway): keep native images on resolved session key (#48912)#48939ihor-v-d wants to merge 3 commits into
Conversation
|
Related: #48922 (canonical core session-key fix this PR preserves), #48919 (queued-drain regression-test approach this PR ports), #48912 (the bug). Three open PRs target #48912 — this one synthesizes #48922's production fix with end-to-end queued-drain coverage. Reviewers should compare against #48922/#48919 to pick the landing PR; not a duplicate (different mechanism/scope per PR). |
|
Note: This is NOT a duplicate of the prior closed PRs (#48922, #48919, #48912, #48912, #48922, #48922, #48919). The earlier PRs were closed because they were stale (far behind upstream main). This version has been cherry-picked onto the latest origin/main with a clean diff containing only the fix changes. No conflicts. CI green. Ready for review. |
1 similar comment
|
Note: This is NOT a duplicate of the prior closed PRs (#48922, #48919, #48912, #48912, #48922, #48922, #48919). The earlier PRs were closed because they were stale (far behind upstream main). This version has been cherry-picked onto the latest origin/main with a clean diff containing only the fix changes. No conflicts. CI green. Ready for review. |
PR — Canonical #48912 Resolution
Title
fix(gateway): keep native images on resolved session key (#48912)
Body
Summary
_prepare_inbound_message_textbuffers native images under the caller-resolved session key, matching the key_run_agentlater consumes.run_generation.session_keymutation by making the pending event's thread-derived key differ from the original active key and asserting the image is consumed only under the original key.Why this is canonical
PR #48922 has the correct core behavior: SET and CONSUME use the same resolved session key while the queued follow-up stays bound to the original active
session_key/run_generationpair. PR #48919's valuable contribution is the queued regression shape, but its production change derives a new key frompending_event.sourcefor recursive_run_agentwhile keeping the original generation token. In threaded cases that evaluates currentness under a key that never minted the generation, which can stale promotion/progress/finalization gates even if the image reaches the model.This PR preserves #48922's production fix and adds the missing queued-drain coverage.
Test plan
python -m pytest tests/gateway/test_queued_native_image_session_key.py -vpython -m pytest tests/gateway/test_native_image_buffer_isolation.py -vpython -m py_compile gateway/run.py tests/gateway/test_queued_native_image_session_key.pygit diff --checkpython -m pytest tests/gateway/test_queue_consumption.py tests/gateway/test_telegram_photo_interrupts.py tests/gateway/test_42039_duplicate_user_message.py -qpython -m pytest tests/gateway -q -k "native_image or queue or pending or drain or session_key"Negative control
Temporarily applying the rejected
next_session_key = self._session_key_for_source(next_source)recursive key mutation makes the new regression fail: the secondrun_conversationreceives plain text instead of a native multimodal list withimage_url. The temporary patch was reverted.Draft comments — do not post without approval
Draft comment for #48922
Building on this fix as the canonical resolution for #48912. The session-key unification here is exactly right; the canonical branch keeps this commit verbatim (authorship preserved) and adds an end-to-end regression that drives the queued follow-up drain path with a non-null
run_generation, proving the nativeimage_urlreaches the secondrun_conversationwhile the follow-up stays on the original active session key. No changes to the fix itself. Proposing we land the canonical PR (which includes this commit); happy to instead stack the test on top of this PR if preferred.Draft comment for #48919
Thanks for the thorough end-to-end queued test here — we've adapted its test shape into the canonical PR for #48912. We're not taking the recursive
session_key=next_session_keymutation:run_generationis minted per session key and checked by_is_session_run_current(session_key, run_generation). Running the follow-up under a key derived frompending_event.sourcewhile keeping the original generation makes currentness checks evaluate against a key that never minted the token, which can stale promotion/progress/finalization gates. The canonical PR instead threads the already-resolved originalsession_keyinto_prepare_inbound_message_text(matching #48922) so SET == CONSUME == the original active key, and ports the queued E2E coverage rewritten to assert that invariant. Recommend closing this in favor of the canonical PR — credit for the regression-test approach noted in the PR body.