Skip to content

fix: retain fork checkpoints with direct history ancestry - #2781

Open
EItanya wants to merge 9 commits into
mainfrom
fix/checkpoint-lineage
Open

EItanya wants to merge 9 commits into
mainfrom
fix/checkpoint-lineage

Conversation

@EItanya

@EItanya EItanya commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Forks omit inherited checkpoints because listing filters only by the instance that created them. A fork at C2 now lists C1 and C2 alongside its own checkpoints, excluding source-history boundaries after C2 and preserving each checkpoint's original provenance. Listing by a deleted instance's ID returns the same complete checkpoint lineage.

Store immutable parent-history and cutoff fields on agent_history, renamed from a2a_context to distinguish a history branch from its public A2A context ID. Retain a unique originating instance ID on history so listing can start there directly, without a local-checkpoint fallback. Composite foreign keys enforce instance/history identity and ownership and matching parent/child owner and context. The fork transaction validates that the cutoff identifies an event in the parent history with the expected task/snapshot boundary; failure rolls back the new history and instance. This avoids a circular history/event foreign-key dependency without adding a table. Schema changes remain in migration 1's pre-release baseline.

Forks continue copying bounded events and rebuilding private task projections at terminal task boundaries. The instance's source-checkpoint reference retains fork-request identity and runtime provenance, including on deletion tombstones. Conversation reads remain local; checkpoint listing traverses retained history ancestry and bounds candidates per history before combining the checkpoint-ID page.

Retained histories protect inherited checkpoints and their snapshot Tags even after all related instances are deleted; there is currently no history garbage collection. Future event GC must preserve inherited boundaries and coordinate with fork creation. Fork creation and deletion checks share a history lock. Deletions already in progress can finish or retry. Deterministic PostgreSQL tests cover both operation orders, for both the fork's source checkpoint and an earlier checkpoint inherited by a later fork.

The protobuf contract documents that Checkpoint.agent_instance_id remains the originating instance ID. The UI allows viewing and forking inherited checkpoints, while showing rename/delete only for checkpoints created in the current instance, in both the transcript and details modal. Mock listings now retain the inherited prefix and original provenance. The obsolete (source_instance_id, id) listing index is removed; the remaining source-instance lookups use the existing CREATING partial index.

Validation:

  • PostgreSQL database suite and migration Up/Down tests passed with the race detector, including every inline SQL prepare check, owner/context constraints, and pagination/ownership after instance deletion. Store tests also verify missing/wrong-history cutoffs and mismatched task boundaries roll back the fork, then permit a successful retry after repair.
  • Go lint reported zero issues. Protobuf generation, lint, and compatibility checks passed. UI type checking, the ten focused unit tests, and UI lint passed (zero errors; existing warnings only). The twelve checkpoint browser tests passed in Chromium and Firefox using the mock backend, including inherited action gating and local checkpoint controls.
  • Backend live E2E passed on the previous head af7db928, including checkpoint lineage after deletion and both runtime-retention scenarios (CI run). That head also passed the relevant service race tests. Live E2E for this UI/proto/index follow-up is pending CI.
  • Earlier SQL benchmarks covered one history with 100,000 checkpoints (42.5 ms median), 1,024 histories with 100 checkpoints each (71.7 ms), and 1,024 histories with one checkpoint each (7.0 ms). These are warm PostgreSQL execution timings from 619cad47, before removing the duplicate local scan; they exclude API overhead. Query/index optimization experiments remain separate from this PR.
  • The migration-immutability check intentionally fails because migration 1 is the pre-release baseline; the guard is unchanged.

@github-actions github-actions Bot added the bug Something isn't working label Sep 9, 2026
@EItanya EItanya changed the title fix: retain checkpoint visibility across forks fix: retain fork checkpoints with direct history ancestry Sep 9, 2026
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Sep 9, 2026
@blacksmith-sh

This comment has been minimized.

@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Sep 10, 2026
@EItanya
EItanya force-pushed the fix/checkpoint-lineage branch from 619cad4 to f13c062 Compare September 10, 2026 16:21
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Sep 10, 2026
@EItanya
EItanya marked this pull request as ready for review September 10, 2026 16:23
@EItanya
EItanya requested a review from a team as a code owner September 10, 2026 16:23
@EItanya
EItanya marked this pull request as draft September 10, 2026 16:25
@EItanya
EItanya force-pushed the fix/checkpoint-lineage branch from f13c062 to 0fa1165 Compare September 17, 2026 14:33
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Sep 17, 2026
@iplay88keys

iplay88keys commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Overall, I think this approach makes sense given the planned follow-up to stop copying event history. Through the investigation I did on this, Codex helped come up with a few areas where we can likely simplify and harden this PR:

  • Retain the originating instance ID on agent_history so deleted instances can still resolve their full lineage. This would remove the local-checkpoint fallback and its partial results after deletion.
  • Enforce that parent and child histories share the same owner/context and that the cutoff identifies a real parent-history event.
  • Validate the listing query against a single history with many checkpoints and deeply nested forks, since the current synthetic test distributes checkpoints across many histories.
  • Add a deterministic concurrency test for fork versus checkpoint deletion.

@EItanya

EItanya commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor Author

@iplay88keys addressed the lineage and concurrency points:

  • agent_history now retains a unique originating instance_id. Listing starts there directly, so deleting an instance preserves its complete local/inherited checkpoint listing and removes the fallback. Tests cover pagination and ownership after deletion, including a fork with no local checkpoints.
  • Composite foreign keys enforce matching parent/child owner and context, and bind the instance to its history's instance ID, owner, and context. After further design review, cutoff validation stays in the fork transaction instead of a reverse history-to-event foreign key. Tests verify missing/wrong-history cutoffs and mismatched task boundaries reject the fork, leave no durable instance/history, and allow the same request to succeed after repair. Future event GC must preserve inherited boundaries and coordinate with forks.
  • Added deterministic fork/deletion tests for both operation orders, covering the same checkpoint and deletion of an earlier checkpoint while forking a later one. They wait for an actual PostgreSQL lock wait and verify checkpoint visibility, retention, and cleanup retries.

We also already benchmarked the dense-history/deep-chain cases before this update:

Histories visited Checkpoints per history Median SQL time p95
1 100,000 42.5 ms 43.7 ms
1,024 100 71.7 ms 75.2 ms
1,024 1 7.0 ms 7.4 ms

PostgreSQL 18.6, 2-CPU/4-GiB container, warm cache, 30 measured executions after three warmups, generic prepared plans, 51 rows fetched for a 50-checkpoint page. Depth includes the current history. The fixture used synthetic 256-byte checkpoint payloads and checked expected IDs; these timings exclude API/protobuf overhead and do not measure cold storage, concurrent writes, or bloat. They predate this update's removal of the duplicate local scan. Separate index/pagination experiments improved dense cases, but those optimizations are not included here.

Database/migration and relevant service tests passed with -race; E2E compilation and Go lint passed. Added live E2E coverage for listing deleted forks; live execution has not been verified for the latest update. Migration changes remain in the pre-release baseline, with the immutability guard unchanged.

@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Sep 20, 2026
@EItanya
EItanya force-pushed the fix/checkpoint-lineage branch from 474ad09 to af7db92 Compare September 21, 2026 12:34
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Sep 21, 2026
Comment thread go/core/internal/database/checkpoints.go
Comment thread go/core/internal/database/checkpoints.go
Comment thread go/core/pkg/migrations/core/000001_initial.sql Outdated
@EItanya
EItanya marked this pull request as ready for review September 22, 2026 10:23
@github-actions github-actions Bot removed the bug Something isn't working label Sep 22, 2026
@github-actions github-actions Bot added the bug Something isn't working label Sep 22, 2026
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Retain the runtime-revision lifecycle checks previously carried by a merge commit. Align fork documentation with main requiring terminal task boundaries for checkpoints.

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
@EItanya
EItanya force-pushed the fix/checkpoint-lineage branch from f44e031 to 5c703e0 Compare September 22, 2026 10:43
AND NOT EXISTS (
SELECT 1 FROM agent_instance i WHERE i.pinned_checkpoint_id = agent_instance_checkpoint.id
)
AND (agent_instance_checkpoint.state = 'DELETING' OR NOT EXISTS (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dropped the pinned-live-instance guard here, tag gets deleted before the fk catches it?

context_id UUID NOT NULL,
CONSTRAINT a2a_context_binding_key UNIQUE (id, context_id)
-- Each row owns one history branch; forks preserve the public A2A context_id.
CREATE TABLE agent_history (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

new migration instead of editing 000001 directly?

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

This pull request has been marked as stale because of no activity in the last 15 days. It will be closed in the next 5 days unless it is tagged "no stalebot" or other activity occurs.

@github-actions github-actions Bot added the stale This issue or PR has become stale label Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working stale This issue or PR has become stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants