Skip to content

fix(core): start at the default entrypoint when resume_at_next_action is False - #1021

Open
enekoreto wants to merge 2 commits into
apache:mainfrom
enekoreto:fix/resume-at-next-action-false
Open

enekoreto wants to merge 2 commits into
apache:mainfrom
enekoreto:fix/resume-at-next-action-false

Conversation

@enekoreto

Copy link
Copy Markdown
Contributor

initialize_from(..., resume_at_next_action=False) kept the loaded __PRIOR_STEP, so the app continued from where it left off instead of starting at default_entrypoint.

Closes #1020

Changes

  • In _init_state_from_persister, the False branch now calls reset_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

  • Added test_initialize_from_resume_at_next_action (sync, InMemoryPersister) and an async version (AsyncInMemoryPersister). The False cases fail on main and pass here, the True cases pass on both.
  • Also checked by hand: forking, resuming after a failed step, override_state_values, SQLite, and the local tracker as the loader. With True nothing changes; with the tracker nothing changes either, since it never saves __PRIOR_STEP.
  • Full test suite and pre-commit pass, on Python 3.13 and 3.10.

Notes

The existing tests that use False load from a fake loader or the local tracker, neither of which has __PRIOR_STEP in 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 at default_entrypoint, as state-persistence.rst documents. True and the local tracker behave as before. Burr's own examples and notebooks that pass False (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

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@github-actions github-actions Bot added the area/core Application, State, Graph, Actions label Nov 1, 2026

@xihaian251 xihaian251 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=result with __PRIOR_STEP=counter still in state; here it gives next=counter, __PRIOR_STEP
    gone, count=1 and sequence_id=0 preserved. resume_at_next_action=True output is byte-identical
    between the two trees, so the change is confined to the False path.
  • 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 at boom under False; here it starts at entry. 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? Asserting PRIOR_STEP not in app.state.get_all() alongside the
    existing state["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 through abuild()/astep().
  • Repeated resume=False builds (the short-lived-app-per-request pattern): base gets stuck at the
    saved position and then dead-ends with get_next_action() returning None; here each build restarts
    at the entrypoint and state keeps accumulating.
  • SQLitePersister round-trips: sequence ids advance 0→1→2 across repeated resume=False builds,
    no UNIQUE violation, no re-used id, and a later resume=True still 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 are test_graphviz_display.py failing 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 current main in (the merge is clean; application.py
    has not changed since your base). black --line-length=100, isort --profile black, flake8,
    check_asf_headers.py and git diff --check are 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.)

@enekoreto

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review. Good catch on the failed case: I added it to both tests (last_step_failed), along with the check that __PRIOR_STEP is gone with False. On main the new failed cases go back to result, here they start at counter. Full test suite and pre-commit still pass, on Python 3.13 and 3.10.

I left the override_state_values docstring out of this PR since it's a separate, pre-existing gap, but it could be its own small docs PR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Application, State, Graph, Actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

initialize_from(resume_at_next_action=False) does not go back to the entrypoint

2 participants