mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-20 19:46:16 +00:00
* feat(gateway): thread checkpoint retention service on the #4189 deletion contract Implements exactly the two contract-proven deletion shapes (trailing duration-only leaves, opt-in leaf sibling branches) with head-chain protection, explicit id protection, a strict pending-writes guard, and joint writes-row cleanup. Head resolution uses LangGraph's time-ordered checkpoint ids; storage deletion mirrors the contract's per-backend data model. Ships without a production trigger by design. Validated against the contract suite (12 passed) plus 14 service scenarios across memory and SQLite; Postgres paths are gated on TEST_POSTGRES_URI. Signed-off-by: zengbohan1 <310902929+zengbohan1@users.noreply.github.com> * fix(gateway): survivor-reachability blob GC and memory blob stats in retention service Aligns the deletion service with the review-hardened contract: blob rows are garbage-collected in a whole-thread pass against surviving checkpoints' channel_versions (a real duration-only leaf shares its parent's versions, so per-checkpoint version deletion would corrupt the surviving state), the memory branch of the stats helper counts saver.blobs and returns the full normalized shape, and per-node channel versions are collected during the graph pass that already exists. Signed-off-by: zengbohan1 <310902929+zengbohan1@users.noreply.github.com> * fix(gateway): address review findings on checkpoint retention service Resolves the review at a479cfe (willem-bd): - Untested savers now fail fast: an explicit isinstance allowlist (InMemorySaver / AsyncSqliteSaver / AsyncPostgresSaver) raises NotImplementedError before any row is read or deleted, so a shallow or third-party saver can never issue partial DELETEs. - The chain walk ends (break) instead of raising KeyError when the head's ancestor row is missing, matching the deletable loop's tolerance for missing parents. - enforce_thread_retention takes an optional per-thread lock and documents the concurrency requirement: classification and deletion are two separate passes, so callers must serialize per-thread mutation (runtime _checkpoint_thread_lock) or guarantee quiescence. - Dropped the dead mid-run guard: CheckpointTuple has no `next` field in langgraph-checkpoint 4.1.1, and pending_writes is populated for committed writes too (verified on the list path), so neither is a usable mid-run signal; the caller-held thread lock is the actual protection. - Removed the write-only _node_step/_Node.step and fixed the head-selection docstring (newest by checkpoint id, not (step, checkpoint_id)). - Documented the E1 leaf / history fast-path interaction in the contract doc and module docstring: the wiring PR must sequence retention away from history reads or adopt a policy that spares cache-carrying leaves. - Added regression tests: unsupported saver, missing ancestor row, thread lock parameter. Validation: test_checkpoint_retention_service 18 passed / 8 postgres-gated skipped; contract + lineage suites 18 passed / 6 skipped; ruff check and format clean. * fix(retention): count non-empty writes dicts on memory saver - _checkpoint_ids_with_writes now requires a non-empty writes dict on InMemorySaver: the empty phantom entry for checkpoints whose task wrote nothing no longer counts as "owns writes rows", so the default E1 pruning reaches the memory backend again (it was a silent no-op there). - test_runtime_duration_leaf_pruned_by_default runs the shipping default (strict_pending_write_guard=True) and proves E1 is reachable out of the box on every backend; the stale override and its wrong SQLite premise are dropped. - document that _checkpoint_thread_lock is non-reentrant: a caller already holding it must not pass it in, or retention self-deadlocks. * test(checkpoint-retention): fix stray duplicated def token in test_duration_link_protected_after_next_run The previous push left `async def def test_...` at line 244, which made the module unimportable and failed collection of the whole suite (and ruff format --check). Local copy was already correct; this commit re-pushes the clean file. 18 passed / 8 postgres-skipped verified from a head worktree. * fix(gateway): make retention correct on Postgres and fail closed on a bad cap * validate max_delete_per_run before any store read: a negative cap used to widen the batch (Python slicing) instead of being rejected; * report identical before/after stats for an empty thread instead of returning before stats_after is collected; * protect each namespace's resume head and ancestor chain, so a persistent subgraph's latest checkpoint is no longer treated as a sibling leaf; * read Postgres columns through a row-factory-agnostic helper (the PG savers open cursors with dict_row, where positional access raises KeyError: 0); * classify the duration-only leaf without relying on metadata["writes"], which the Postgres saver strips via get_serializable_checkpoint_metadata. Verified locally on memory, SQLite and a real Postgres 16 instance (62 passed, 0 skipped): the E1 shape now fires on Postgres, which no backend test covered before CI ran the Postgres lig. Signed-off-by: zeng-bohan <zengbh1@gmail.com> * test(gateway): pin the Postgres-shape duration classifier; report per-namespace heads - Deterministic regression for _mark_duration_leaves_without_the_marker: hand-put the Postgres round-trip shape (writes marker popped, source= update + accumulated run_durations + channel_versions identical to the parent) and assert the shipping default prunes it; a control that bumps one channel version (the client update_state shape) with otherwise identical metadata stays protected. Both legs run on memory and SQLite, so the class cannot silently re-widen (a resumable head losing head protection) or re-narrow (E1 never firing on Postgres) without a locally-executing test failing. - RetentionReport.protected_head_id -> protected_head_ids: heads are now selected per namespace, so the report carries every namespace's head (root key = what an unsaved aget_tuple resolves) instead of only the global max - reshape it before the wiring PR starts consuming reports for audit/aggregation. --------- Signed-off-by: zengbohan1 <310902929+zengbohan1@users.noreply.github.com> Signed-off-by: zeng-bohan <zengbh1@gmail.com> Co-authored-by: zengbohan1 <310902929+zengbohan1@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
146 lines
7.8 KiB
Markdown
146 lines
7.8 KiB
Markdown
# Checkpoint Retention Contract (DRAFT)
|
|
|
|
Status: **draft** — the deletion contract for #4189 item 3. No retention or
|
|
deletion implementation should land before this contract (or a successor
|
|
revision of it) is accepted, and every deletion proposal must be validated
|
|
against `backend/tests/test_checkpoint_retention_contract.py`.
|
|
|
|
## Why a contract is needed
|
|
|
|
LangGraph checkpoints form a per-thread **parent chain**. Gateway features
|
|
depend on that chain being intact:
|
|
|
|
- **Branch / regenerate** resolves the replay base by walking
|
|
`parent_config` links from a checkpoint that contains the target message
|
|
(`app/gateway/checkpoint_lineage.py::find_checkpoint_before_message`).
|
|
- **Explicit resume** replays from a `checkpoint_id` a client still holds.
|
|
|
|
Deleting checkpoint rows by recency or table size can therefore break those
|
|
features **silently** — a missing ancestor surfaces as
|
|
`CheckpointLineageIntegrityError` at branch time, or as a lost resume target,
|
|
never as an obvious storage bug. The contract below separates deletable rows
|
|
from protected rows and pins the verification method.
|
|
|
|
## Data model
|
|
|
|
| Backend | State rows | Writes rows |
|
|
| --------- | ----------------------- | ------------------ |
|
|
| SQLite | `checkpoints` | `writes` |
|
|
| Postgres | `checkpoints`, `checkpoint_blobs` | `checkpoint_writes` |
|
|
| Memory | `saver.storage`, `saver.blobs` | `saver.writes` |
|
|
|
|
(Note: SQLite has no separate blob table; channel values live inside the
|
|
serialized checkpoint payload. Postgres splits blobs out.)
|
|
|
|
Measurement shape: per-thread rows + bytes per table, normalized by
|
|
`bench_channels._normalized_storage_stats`.
|
|
|
|
## Protected set (MUST NOT delete without the stated compensation)
|
|
|
|
1. **Explicit resume targets** — any `checkpoint_id` a client may still
|
|
resume to. Deleting it removes the replay surface
|
|
(`test_deleting_explicit_resume_target_breaks_resume`). A retention policy
|
|
may expire these, but only with an explicit TTL semantic agreed here.
|
|
2. **Branch ancestors** — every checkpoint on the parent chain from a
|
|
branchable head back to (and including) the checkpoint *before* the oldest
|
|
branchable message. Deleting any node on that walk breaks branch/regenerate
|
|
with `CheckpointLineageIntegrityError`
|
|
(`test_deleting_branch_ancestor_breaks_lineage_loudly`).
|
|
3. **Pending writes** — rows in the writes table are uncommitted/in-flight
|
|
state, not garbage (`test_pending_writes_are_retained_state_not_garbage`).
|
|
4. **Duration-only chain links** — `persist_run_durations` appends
|
|
metadata-only checkpoints. A duration-only checkpoint that a later run has
|
|
forked from is a *chain link*: the walk steps through it, so deleting it
|
|
requires **grafting** the fork onto the grandparent (rewriting the fork's
|
|
`parent_config`) in the same change. A bare leaf (below) is safe; a link is
|
|
not. The link shape can only be produced by the real runtime, so the graft
|
|
path is specified here and intentionally not covered by a storage-level
|
|
test.
|
|
5. **Latest resumable state per thread** — the newest checkpoint must remain
|
|
addressable so a thread can always continue.
|
|
|
|
## Provably safe forms (validated by tests)
|
|
|
|
1. **Leaf sibling branches** — a checkpoint forked off an older turn that has
|
|
no children (`test_leaf_sibling_branch_deletion_is_safe`). Pruning it does
|
|
not affect the main line's walk, explicit resume, or head.
|
|
2. **Trailing duration-only leaves** — a duration-only checkpoint no later run
|
|
has forked from (`test_leaf_duration_checkpoint_deletion_is_safe`).
|
|
|
|
New deletion proposals must add their shape as a test here: construct the
|
|
chain, delete, then verify (a) latest resume, (b) explicit `checkpoint_id`
|
|
resume, (c) branch from an older visible turn, and (d) orphan row counts.
|
|
|
|
## Deletion mechanics
|
|
|
|
- Deletion must cover the backend's tables jointly and account for orphans, and
|
|
blob reachability must be computed from the **surviving checkpoints in a
|
|
whole-thread pass**: after deleting a checkpoint row, a `checkpoint_blobs` /
|
|
`checkpoint_writes` row is an orphan only if *no surviving checkpoint*
|
|
references it. The shared-version case is not hypothetical — the real
|
|
duration-only checkpoint is a copy of the head checkpoint dict
|
|
(`persist_run_history_metadata` replaces only id/ts), so it inherits the
|
|
parent's `channel_versions` verbatim, and on Postgres the blob rows
|
|
reachable from the deleted duration row are the same rows backing its
|
|
parent. An implementation that deletes blobs keyed by the removed
|
|
checkpoint's own `channel_versions` would corrupt the thread's newest
|
|
surviving state — exactly the failure class this contract exists to
|
|
prevent. (For the same reason a real duration-only leaf is not
|
|
payload-free: it materializes the parent's values under
|
|
`{"writes": {"runtime_run_duration": {...}}, "source": "update", "step":
|
|
...}` metadata, which is what makes reclaiming it worthwhile.)
|
|
- Failure semantics: if a proposed deletion cannot be proven safe against the
|
|
protected set, it must not ship. Partial deletion that leaves a dangling
|
|
`parent_config` converts a cleanup into a thread-level outage (branch and
|
|
regenerate fail loudly for every later turn).
|
|
- Measurement first: proposals must include before/after numbers from
|
|
`scripts/benchmark/checkpoint/bench_channels.py` (per-thread rows/bytes,
|
|
SQLite and Postgres) plus the contract test suite passing.
|
|
|
|
## History fast-path interaction (wiring requirement)
|
|
|
|
The trailing duration-only leaf is also the carrier of the run-history
|
|
metadata cache: `persist_run_history_metadata` accumulates `run_durations`
|
|
and `run_message_ids` in the leaf's metadata, and
|
|
`app/gateway/routers/threads.py::get_thread_history` reads that map from the
|
|
latest checkpoint (`_checkpoint_run_durations` /
|
|
`_checkpoint_run_message_ids`, gated on `is_latest_checkpoint`) to answer
|
|
every known turn's duration and message-to-run attribution without scanning
|
|
the event store. The parent checkpoint the leaf clones does **not** carry
|
|
that map.
|
|
|
|
Deleting the leaf (scenario E1) therefore removes the fast-path cache: the
|
|
next history read sees no durations, falls back to event-store + run-manager
|
|
scans, and `_persist_run_history_metadata_background` re-writes a fresh
|
|
duration-only leaf — which the next retention pass deletes again. Net effect
|
|
without sequencing: the reclaimed row comes straight back, plus recurring
|
|
store scans and an extra write per read.
|
|
|
|
The wiring PR that introduces the production trigger must therefore either:
|
|
|
|
1. **Sequence retention away from history reads** — e.g. run retention on a
|
|
schedule whose next pass re-reclaims the re-created leaf, or run it when
|
|
the thread is not being read; or
|
|
2. **Adopt a policy that spares cache-carrying leaves** — e.g. a
|
|
`RetentionPolicy` flag that keeps any trailing duration-only leaf whose
|
|
metadata still carries `run_durations` / `run_message_ids` (same spirit
|
|
as the strict pending-writes guard), at the cost of not reclaiming that
|
|
leaf's rows.
|
|
|
|
Without either, E1 pruning and history reads churn against each other. This
|
|
decision belongs to the wiring PR, not to the storage-level service: the
|
|
service cannot tell a cache-carrying leaf from a payload-free one on the
|
|
alist path without re-implementing the writer's merge semantics.
|
|
|
|
## Item 4 note (large tool results)
|
|
|
|
`ToolOutputBudgetMiddleware` externalizes oversized tool outputs before they
|
|
reach state (preview + file reference under `.tool-results/`), so the
|
|
"50 KB result re-snapshotted every step" scenario from the original report
|
|
depends on which tools/paths bypass it. The probe
|
|
(`scripts/benchmark/checkpoint/bench_tool_result_probe.py`) measures the
|
|
on-disk checkpoint delta for the wrapped vs unwrapped paths on the lead
|
|
graph; subagent chains instantiate the same middleware by default. Any PR
|
|
claiming a residual gap must name the concrete bypassing path and show its
|
|
probe numbers.
|