Repository navigation
Conversation
xihaian251
left a comment
There was a problem hiding this comment.
Approved — I could not break this. Reviewed independently against 30f608c5 with 222b32c3 (and
current main) as the comparison, all local on Windows.
The change is the smallest thing that expresses the fix, and it is the same move the failed branch
one line above already makes, so it reads as the intended design rather than a patch-over.
What I verified by running it:
- The repro from #1020. With a completed step persisted and
resume_at_next_action=False, base gives
next=resultwith__PRIOR_STEP=counterstill in state; here it givesnext=counter,__PRIOR_STEP
gone,count=1andsequence_id=0preserved.resume_at_next_action=Trueoutput is byte-identical
between the two trees, so the change is confined to theFalsepath. - The second symptom in the issue, which I think is the more important one. With a failed row at a
non-entrypoint action (entry -> boom -> done, rows(0,'entry','completed'),(1,'boom','failed')),
base starts atboomunderFalse; here it starts atentry. Worth calling out because the two new
tests cover the completed case only — a regression here would go unnoticed. Could you add the failed
status to the parametrised pair? AssertingPRIOR_STEP not in app.state.get_all()alongside the
existingstate["count"]check would also pin why it works. - Idempotence on state that has no
__PRIOR_STEP(fake loader / local tracker, which is where the
existing tests live): no exception, unchanged behaviour. - Sync/async parity: both loads funnel through
_init_state_from_persister, and the async case
matches throughabuild()/astep(). - Repeated
resume=Falsebuilds (the short-lived-app-per-request pattern): base gets stuck at the
saved position and then dead-ends withget_next_action()returningNone; here each build restarts
at the entrypoint and state keeps accumulating. SQLitePersisterround-trips: sequence ids advance 0→1→2 across repeatedresume=Falsebuilds,
noUNIQUEviolation, no re-used id, and a laterresume=Truestill resumes correctly.fork_from_app_id+resume=False: forked state kept, starts at the entrypoint.- Tests:
tests/core+tests/tracking= 446 passed / 9 failed / 2 skipped, vs 442 / 9 / 2 on the
base — the +4 is exactly the new cases, and the 9 aretest_graphviz_display.pyfailing on this
machine for a missing graphviz binary, identical on base. Same numbers on CPython 3.10.22, 3.11.16,
3.12.14 and 3.13.1, and also after merging currentmainin (the merge is clean;application.py
has not changed since your base).black --line-length=100,isort --profile black,flake8,
check_asf_headers.pyandgit diff --checkare all clean on the two changed files.
One thing to be aware of, not a request for change: override_state_values containing an internal key
behaves differently now. With override_state_values={"count": 50, "__PRIOR_STEP": "halt"} on base,
resume=True already discarded the override (the completed branch rewrites the key) while
resume=False honoured it; with this change both discard it. I think that consistency is right —
__PRIOR_STEP is framework-owned — but initialize_from's docstring doesn't mention
override_state_values at all today, so a sentence saying internal keys (__PRIOR_STEP,
__SEQUENCE_ID) can't be set through it would save someone a debugging session. Docs-only, and
pre-existing.
FYI on hosted CI: Build Burr and Release Validation are still action_required at this head, so
pytest has not run here on GitHub's side, and documentation fails with startup_failure that also
reproduces on main. My numbers above are local, not a claim that the checks are green.
Independently, I also checked this against the resume-time SerializationContext work in
vignesh-manel#1: the two touch adjacent lines (a plain git apply of this hunk onto that branch
fails on one context line, a 3-way merge resolves it) and are functionally independent — combined tree
is green on both suites, and the "load-time context is the application's context" identity still
holds under resume_at_next_action=False.
(One process note, since I would rather over-say it: this review was drafted with AI assistance in my IDE, but every number and behaviour above comes from commands I ran myself against 30f608c5 and 222b32c3 on this machine today. No hosted CI result is being claimed.)
|
Thanks for the careful review. Good catch on the failed case: I added it to both tests ( I left the |
initialize_from(..., resume_at_next_action=False)kept the loaded__PRIOR_STEP, so the app continued from where it left off instead of starting atdefault_entrypoint.Closes #1020
Changes
_init_state_from_persister, theFalsebranch now callsreset_to_entrypoint(), like the failed-status branch already does. That clears__PRIOR_STEP; the rest of the loaded state is kept.How I tested this
test_initialize_from_resume_at_next_action(sync,InMemoryPersister) and an async version (AsyncInMemoryPersister). TheFalsecases fail on main and pass here, theTruecases pass on both.override_state_values, SQLite, and the local tracker as the loader. WithTruenothing changes; with the tracker nothing changes either, since it never saves__PRIOR_STEP.Notes
The existing tests that use
Falseload from a fake loader or the local tracker, neither of which has__PRIOR_STEPin the loaded state, which is why this wasn't caught before.Compatibility: this only changes apps that load from a persister with
resume_at_next_action=False. They now start atdefault_entrypoint, asstate-persistence.rstdocuments.Trueand the local tracker behave as before. Burr's own examples and notebooks that passFalse(multi-modal-chatbot, streaming-fastapi, the templates, conversational-rag) load from the tracker, so they don't change, and their comments ("always resume from entrypoint in the case of failure") already describe the new behavior.This was done with AI assistance (Claude Code), and checked by running the tests described above.
Checklist