-
Notifications
You must be signed in to change notification settings - Fork 44
fix: don't flag an in-flight parallel duplicate as crashed - #443
Open
AmirF194 wants to merge 1 commit into
Open
fix: don't flag an in-flight parallel duplicate as crashed #443AmirF194 wants to merge 1 commit into
AmirF194 wants to merge 1 commit into
Conversation
...ashed ActivityLogGuard.begin() had no way to tell a batch mate still running (execute_parallel dispatches all calls before any of them completes) apart from a row that survived a crash and restart. Track in-flight idem_keys per guard instance: a restart creates a fresh, empty set, so the crash-recovery path still fires exactly as before. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
zfoong
commented
Sep 7, 2026
Collaborator
Is this still a work in progress? @AmirF194
AmirF194
marked this pull request as ready for review
September 7, 2026 18:06
AmirF194
commented
Sep 7, 2026
Author
Ready for review, not WIP, sorry for the confusion. Marked it ready. Reproduced the false crash-flag twice (a standalone script driving ActivityLogGuard directly, plus a new regression test in tests/test_activity_log.py), and ruff/pytest are clean on the branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
What
ActivityLogGuardnow tracks idem_keys with an INTENT it recorded but hasn'tcomplete()'d yet.begin()'s stale-INTENT branch checks that set before treating a row as a crashed prior attempt.Why
execute_paralleldispatches every call in a batch before any of them completes, so two identical irreversible calls land twobegin()calls back to back with nocomplete()in between. The second call saw the first's INTENT row and treated it as an interrupted (crashed) attempt, downgrading it to FAILED and telling the LLM the action "may have already happened," even though the first call was still genuinely running. Repro and details in #442.Closes #442
Age alone can't fix this: an immediate crash-then-restart produces the exact same young INTENT row, and that case still needs the "may have happened" warning (there's a test for it). Tracking in-flight keys per guard instance handles both correctly, since a restart creates a fresh guard with an empty set.
How to test
python3 -m pytest tests/test_activity_log.py -q: 12 passed. The new test fails ondevand passes on this branch.python3 -m pytest tests -q(skippingtests/e2e,tests/llm/golden, and two other files with pre-existing collection errors unrelated to this change, missingmss, a relative import bug): 944 passed, 29 skipped, 0 failed.ruff format --checkandruff checkclean on both changed files.python -m compileall -q app agent_core agents decorators skillsOne thing I didn't add a fallback for: if something throws between
begin()andcomplete()andcomplete()never runs, that idem_key stays in the in-flight set until the process restarts, so a genuinely stuck duplicate would keep reading as "already running" instead of eventually getting flagged as a possible crash. The ledger row was already stuck at INTENT in that same case before this change, so it's not a new risk, just not one this closes.Screenshots / Logs
Not UI-facing.