fix(session,shutdown): MysqlSession.delete impl + GracefulShutdownHook dedup improvement

[Xingxing-Wang]

Summary

• MysqlSession: Add delete(SessionKey, String key) implementation to fix the bug where clearing flags via Session.delete() had no effect (the method was not implemented).
• GracefulShutdownHook: Improve deduplicateIfResuming logic — only discard input when its content matches the last USER message in memory, preventing legitimate new messages from being dropped after a shutdown-interrupted resume.

Test plan

• Verified MysqlSession.delete correctly removes state entries from DB
• Verified GracefulShutdownHook keeps new user input when content differs from memory
• Verified GracefulShutdownHook still discards duplicate input when content matches

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

❌ Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...agentscope/core/shutdown/GracefulShutdownHook.java	90.47%	0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[AgentScopeJavaBot]

🤖 AI Review

This PR fixes two real bugs: (1) MysqlSession.delete(SessionKey, String) was unimplemented (silently a no-op via the interface default), so flag-clearing via the per-key delete had no effect on MySQL-backed sessions; (2) GracefulShutdownHook.deduplicateIfResuming unconditionally discarded any new input after a shutdown-interrupted session, which could drop legitimate new prompts. The new dedup logic correctly compares the incoming text against the last USER message in memory and only discards when they match. Test coverage is reasonable and the MySQL implementation follows the existing transactional write pattern. Two concerns worth addressing before merge: (a) the dedup compares only getTextContent() of the first input message, so non-text content (image-only / tool messages) collapses to an empty string and may false-match; (b) when event.getMemory() is null the new code still drops input — that preserves the legacy behavior validated by the existing preCallDeduplication test, but contradicts this PR's stated intent for memoryless agents. Also, MysqlSession.delete(sessionKey, key) does not clean up the companion key + ":_hash" row used by list-state change detection, leaving an orphan row (correctness still works on the next save via full-rewrite fallback, but it is inconsistent).

[AgentScopeJavaBot]

[major] Msg.getTextContent() returns the concatenation of TextBlock content joined by \n, and returns an empty string when the message has no text blocks (image-only, audio-only, or pure tool-result messages). Comparing only the text payload means: (1) two distinct image-only user messages both yield "" and will be falsely deduplicated; (2) any message that carries information solely in non-text blocks cannot be distinguished. Consider comparing by Msg.getId() when available, or by a hash over the full content block list, so the dedup check stays correct for multimodal inputs.

String newInputId = newInput.get(0).getId();
// ...later, when scanning memory for the last USER msg:
String lastUserId = msg.getId();
if (newInputId != null && newInputId.equals(lastUserId)) { /* discard */ }

[AgentScopeJavaBot]

[major] When event.getMemory() is null (non-ReActAgent, or a ReActAgent configured without memory), this branch still calls event.setInputMessages(List.of()) and returns. That is exactly the legacy behavior this PR set out to fix: an agent without memory has no saved context to "resume" from, so silently dropping the input means the next invocation produces nothing. The branch is currently needed only because the existing preCallDeduplication test uses a non-ReActAgent (TestableAgent) and asserts an empty result. Suggest flipping the policy: when memory is unavailable, keep the new input (and update / replace the legacy test) — that aligns with the PR description ("prevent legitimate new messages from being dropped").

[AgentScopeJavaBot]

[nit] newInput == null is unreachable: PreCallEvent's constructor wraps the list with Objects.requireNonNull(...) and setInputMessages also rejects null, so getInputMessages() cannot return null. The isEmpty() short-circuit is fine, but you can drop the null check (or replace with an assert).

[AgentScopeJavaBot]

[minor] Only newInput.get(0) is inspected. If a caller ever passes multiple messages in a single resume call (e.g., a user message followed by an attachment placeholder), only the first is compared against memory; on a match the entire list is dropped — silently losing the rest. The shutdown-resume case is usually single-message, but a defensive comment, or scanning all input messages, would make the contract explicit.

[AgentScopeJavaBot]

[major] For list states, save(SessionKey, String, List) writes both state_key = key (one row per item) and an auxiliary state_key = key + ":_hash" row used by needsFullRewrite(...). This new delete(SessionKey, String key) only removes rows whose state_key exactly equals key, leaving the :_hash row orphaned. Correctness on the next save() is preserved (the prefix-hash mismatch triggers a full rewrite), but the orphan row leaks if no subsequent save happens, and getStoredHash will keep returning a stale value. Suggest deleting both in one statement:

String deleteSql =
        "DELETE FROM " + getFullTableName()
            + " WHERE session_id = ? AND state_key IN (?, ?)";
// ...
stmt.setString(1, sessionId);
stmt.setString(2, key);
stmt.setString(3, key + HASH_KEY_SUFFIX);
stmt.executeUpdate();

[AgentScopeJavaBot]

[nit] stmt.executeUpdate() returns the affected row count, which is currently discarded. Consider capturing it and logging at debug level when zero rows are deleted — it can be useful when debugging "flag clear had no visible effect" reports of the kind this PR was created to fix.

[AgentScopeJavaBot]

[nit] testDeleteStateKeyThrowsOnFailure exercises the executeUpdate failure path but does not assert that the transaction was rolled back. Adding verify(mockConnection).rollback(); would lock in the rollback guarantee provided by executeInWriteTransaction(...) and protect against future regressions in that helper.