FEAT: Add reset_conversation_async hook to PromptTarget - #2320
Conversation
|
Hi Richard Lundeen (@rlundeen2) Roman Lutz (@romanlutz), this has been open for a few weeks now. It is a self-contained change (a reset_conversation_async hook on PromptTarget). I recently landed microsoft/RAMPART#141, so I am glad to match whatever conventions you prefer here. Is there anything I can do to help move it along, or someone I should tag for review? |
|
(GHCP Generated): I think the underlying problem is real, especially for scenarios. A scenario commonly shares one I do not think
I suggest this design:
This keeps the useful part of the proposal for shared realtime targets, while preserving |
6047523 to
aee8d0e
Compare
|
Richard Lundeen (@rlundeen2) The TAP half is done. A node now reports each objective-target conversation as its send returns and the attack records it there, so the ones a single-turn rotation replaces and the branches a run walks away from stay nameable. Recording at that point rather than at result time is what makes it survive a run that raises, which otherwise loses 2 of 3 conversations on a depth 3 width 3 tree. That made the TAP override unnecessary, and no attack overrides the lookup now. I kept the call in One resolver now, shared with the error-result builder from #2322. It still reads the context rather than the result, with a test pinning the two equal. Happy to move it if you would rather have it at the executor. |
Targets that hold external state keyed by conversation had no standard way to release it when an attack finished. RealtimeTarget grew its own cleanup_conversation_async, and the issue reporter had to monkey-patch _teardown_async to call it. PromptTarget gains a no-op reset_conversation_async(*, conversation_id), and AttackStrategy._teardown_async now hands it every objective-target conversation the run used. That is the live conversation plus the ones recorded as PRUNED, since a PromptSendingAttack retry, a Crescendo backtrack, and the single-turn rotation in multi-turn attacks all leave earlier conversations behind. TAP keys conversations per tree node instead, so it overrides the lookup. The reset runs in the lifecycle finally block, so a target that raises is logged rather than replacing whatever error the attack was reporting. RealtimeTarget now implements the hook, and cleanup_conversation_async delegates to it with a deprecation warning. cleanup_target_async is left alone since closing the whole target is a different concern. Towards microsoft#1247
TAP kept objective-target conversations that nothing could name afterwards, so nothing could release the target-side state behind them. Two ways they went missing. Against a single-turn objective target a node mints a fresh conversation id every turn and dropped the one it replaced; a node holds no reference to the context, so the old id went nowhere. And the branches still standing when the run ended were never recorded, so only the winner survived as result.conversation_id. Measured on a real tree at depth 3: a single-turn target served 3 conversations with 1 still reachable, and a multi-turn target served 6 with the result reporting 5. PAIRAttack subclasses TAP and overrides none of this, so it had both gaps too. A node now reports each conversation as its send returns, which is the moment the target starts holding state for it, and the attack records it straight onto the context. Recording it there rather than while building the result is what makes it survive a run that raises or is cancelled, which are the runs most likely to leave a connection open: raising in the second iteration of a depth 3 width 3 tree used to lose 2 of 3 conversations. The winning branch is taken back out when the result is built, since it becomes result.conversation_id and would otherwise be reported twice. Reporting on send rather than on rotation also means a branched node does not record the conversation it was cloned from but never sent on. Single-turn with branching_factor 2 serves 10 conversations and records exactly those 10. Towards microsoft#1247
Two copies of the same lookup had grown up next to each other. microsoft#2322 taught the error-result builder to read context.conversation_id and fall back to context.session.conversation_id; _get_objective_conversation_ids was doing the same walk a few hundred lines away. They are one resolver now, so the conversation an error result is filed under and the conversations teardown releases cannot drift apart. That commit also gave TAPAttackContext a conversation_id property, which is what the TAP override of _get_objective_conversation_ids existed to work around. With TAP's branch bookkeeping fixed the base lookup covers it, so the override is gone and no attack overrides the lookup. What is left is AttackResult.get_active_conversation_ids() read off the context rather than the result, with a test pinning the two equal. The context is what teardown has: execute_with_context_async re-raises rather than returning, so a run that fails or is cancelled produces no result for anything downstream to read, and those are the runs that leave connections open. Tests cover all three endings. An attack whose context keeps the live conversation somewhere else should expose it as a conversation_id property, the way TAPAttackContext reports the best branch, rather than overriding the lookup and putting the answer back in two places. Written down in the docstring and in the target instructions, alongside what the pass covers: the objective target only, since adversarial, scorer and converter targets have their own lifetimes. Towards microsoft#1247
WebsocketTarget has the same shape as RealtimeTarget: a PromptTarget that caches one connection per conversation id in _existing_conversation, with its own cleanup_conversation_async that nothing calls. Leaving it out would have deprecated that method on one target while the identical method stayed live on the other, and a scenario sharing a WebsocketTarget would keep accumulating connections for exactly the reason this PR exists. Its close path is more careful than RealtimeTarget's, holding a per-conversation lock and shielding the close so a cancellation still finishes it, so the body is unchanged and only the name and the keyword-only signature move. The four tests covering it move with it, and cleanup_conversation_async keeps working through the same deprecation shim. Towards microsoft#1247
Two defects, both found by diffing this branch against main rather than by a test failing. TAP records every branch as PRUNED while the run is in flight, and the leading one was taken back out only while building the result. A run that raises never builds one, so its own conversation stayed in both places: it was the error result's conversation_id and a pruned entry at the same time. attack_service.list_attacks adds the main conversation's message count to the pruned ones and sums a list rather than a set, so that run's messages were counted twice in the backend, and the markdown and pretty printers rendered the conversation twice. The release now happens wherever the lead is recomputed, which is the last step of every iteration, so the invariant holds at every instant instead of only once a result exists. A branch that led and then lost it is an abandoned branch again, so it goes back. The fallback that picks a conversation when no node completed was setting the lead without releasing it; it goes through the same path now. That makes the release at result-build time unreachable, since the lead is always recomputed last, so it is gone rather than left as dead code. Second, _resolve_live_conversation_id had grown a hasattr dispatch that changed what the error-result builder does for a TAPAttackContext with no nodes and no best branch: main falls through to session.conversation_id, this returned None and the caller minted a fresh uuid. Neither id names anything real, but that is microsoft#2322's code and this PR was not asked to change it. It is back to main's exact lookup, verified by computing both over all six concrete context types: zero divergences. Towards microsoft#1247
aee8d0e to
dbbc8fa
Compare
Add an idempotent PromptTarget.reset_conversation_async hook and an execution-scoped lifecycle in AttackStrategy that records real objective-target invocations through a generic callback and releases each unique conversation after the attack completes. TAP now cancels and awaits sibling node sends before cleanup. RealtimeTarget and WebsocketTarget implement the reset hook; related_conversations stays reporting data only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc87b33-0580-45f9-a9cc-0dc9603bd2a2
Record objective-target conversations directly at each attack send site instead of threading a callback through PromptNormalizer and PromptTarget. Remove the TargetInvocationCallback protocol so the target-dispatch surface no longer carries lifecycle concerns. Reduce RealtimeTarget and WebsocketTarget reset_conversation_async to plain try/except that logs close errors and lets cancellation propagate, dropping the asyncio.shield guard. Attempt every reset in the lifecycle __aexit__ even under cancellation, then re-raise the first stashed CancelledError. Recording outside an active scope is now a no-op. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc87b33-0580-45f9-a9cc-0dc9603bd2a2
|
I made some changes, but I like your general direction. Approved and merging |
|
Thanks Richard Lundeen (@rlundeen2) . Recording at the send site instead of reading |
Description
Adds a standard hook for prompt targets to release external state that belongs to one conversation. This prevents shared stateful target instances, such as realtime and WebSocket targets used by scenarios, from retaining one connection for every completed atomic attack.
Design
PromptTarget.reset_conversation_async(*, conversation_id)hook. The base implementation is a no-op, so stateless targets require no changes.AttackStrategy. Attacks record objective-target conversation IDs directly before objective sends. The lifecycle deduplicates the IDs and resets them when the atomic attack ends, including failure and cancellation paths.AttackResult.related_conversationsas resource state. A failed or cancelled attack might not produce a result, and TAP can create several objective conversations before it can assemble one.Target implementations
OpenAIRealtimeTargetandWebsocketTargetoverride the hook. Each removes and closes only the connection for the specified conversation. Unknown IDs are a no-op, and close errors are logged. Their existingcleanup_conversation_asyncmethods remain as deprecated forwarding aliases. Whole-target cleanup remains separate.The target authoring instructions now briefly state when a target should override the hook and its idempotency requirement.
Validation
Scoped target, normalizer, lifecycle, and attack tests pass. Ruff formatting and lint checks pass, and the changed production modules pass
ty. The full pre-commit run passed every available hook except the repository-widetyhook because the configured package feed did not provide the locked optionaltorchandlitellmpackages.