Skip to content

perf(sessions): find matching checkpoints without sorting#96964

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
ly-wang19:codex/high-quality-algo-6-resubmit
Jun 26, 2026
Merged

perf(sessions): find matching checkpoints without sorting#96964
vincentkoc merged 1 commit into
openclaw:mainfrom
ly-wang19:codex/high-quality-algo-6-resubmit

Conversation

@ly-wang19

Copy link
Copy Markdown
Contributor

What Problem This Solves

Branching or restoring from a compaction checkpoint only needs the newest checkpoint with the requested id, but the accessor copied and sorted the entire checkpoint array before finding that id. Sessions with many retained compaction checkpoints paid avoidable allocation and O(n log n) work.

Why This Change Was Made

This replaces the full copy/sort/find path with a single scan that tracks the newest checkpoint matching the trimmed requested id. It keeps the previous newest-match behavior while ignoring newer checkpoints with other ids.

User Impact

Checkpoint branch/restore operations do less work for sessions with long compaction histories. The selected checkpoint, persistence behavior, and missing-checkpoint handling remain unchanged.

Evidence

  • node scripts/run-vitest.mjs src/config/sessions/session-accessor.test.ts
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --parallel-tests "node scripts/run-vitest.mjs src/config/sessions/session-accessor.test.ts"
  • git diff --check

Resubmitted from #96956 after freeing an active PR slot; #96956 was auto-closed only because the author had more than 20 active PRs.

@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 26, 2026, 6:49 PM ET / 22:49 UTC.

Summary
The PR replaces the session accessor's copy/sort/find checkpoint selection with a linear newest-match scan and adds an accessor-level regression test.

PR surface: Source +7, Tests +86. Total +93 across 2 files.

Reproducibility: yes. Source inspection shows sessions.compaction.branch and sessions.compaction.restore call the shared getSessionCompactionCheckpoint helper, which still sorts the full checkpoint list before the optimized accessor mutation runs.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: src/config/sessions/session-accessor.test.ts, serialized state: src/config/sessions/session-accessor.ts, unknown-data-model-change: src/config/sessions/session-accessor.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: canonical
Canonical: #96964
Summary: This PR is the active resubmission of the same narrow checkpoint sorting change after the earlier branch was closed for the active-PR limit.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Update the shared gateway checkpoint getter so branch/restore avoid sorting before the accessor mutation.
  • [P1] Add redacted real after-fix branch/restore proof to the PR body; updating the body should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists validation commands only; it needs redacted terminal output, logs, screenshots, recording, or a linked artifact from an after-fix branch/restore run before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Normal sessions.compaction.branch and sessions.compaction.restore still sort checkpoints once in the shared gateway lookup, so the claimed user-path performance win is partial.
  • [P1] The external PR has command-only validation evidence and still needs redacted real after-fix branch/restore proof before merge.

Maintainer options:

  1. Decide the mitigation before merge
    Apply the single-pass newest-match lookup to the shared gateway checkpoint getter used before branch/restore, keep ordered list behavior where callers need lists, and add real after-fix proof before merge.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Contributor or maintainer follow-up is needed because automation cannot supply the external real-behavior proof, and the patch needs a shared-lookup adjustment before merge.

Security
Cleared: The diff only changes TypeScript session lookup logic and a colocated test, with no dependency, workflow, credential, generated artifact, or package surface changed.

Review findings

  • [P2] Optimize the shared checkpoint lookup too — src/config/sessions/session-accessor.ts:1358-1366
Review details

Best possible solution:

Apply the single-pass newest-match lookup to the shared gateway checkpoint getter used before branch/restore, keep ordered list behavior where callers need lists, and add real after-fix proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows sessions.compaction.branch and sessions.compaction.restore call the shared getSessionCompactionCheckpoint helper, which still sorts the full checkpoint list before the optimized accessor mutation runs.

Is this the best way to solve the issue?

No. The accessor scan is a good local step, but the best fix also updates the shared gateway lookup so the normal user-facing branch/restore path avoids the O(n log n) sort.

Full review comments:

  • [P2] Optimize the shared checkpoint lookup too — src/config/sessions/session-accessor.ts:1358-1366
    The new scan only runs inside the accessor mutation, but normal sessions.compaction.branch and sessions.compaction.restore already call getSessionCompactionCheckpoint; that helper still sorts via listSessionCompactionCheckpoints(...).find(...). Move the single-pass newest-match lookup to the shared getter or update that helper too so the user-facing path actually avoids the full sort.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ab966c214be0.

Label changes

Label justifications:

  • P3: This is a low-risk internal sessions performance cleanup with no compatibility, security, or migration surface.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists validation commands only; it needs redacted terminal output, logs, screenshots, recording, or a linked artifact from an after-fix branch/restore run before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +7, Tests +86. Total +93 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 10 3 +7
Tests 1 86 0 +86
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 96 3 +93

What I checked:

  • Live PR diff: The patch changes only the accessor-level checkpoint finder and adds an accessor-level test for newest matching checkpoint selection. (src/config/sessions/session-accessor.ts:1358, fc1315f15e16)
  • Shared gateway lookup still sorts: Current main's getSessionCompactionCheckpoint calls listSessionCompactionCheckpoints(...).find(...), and the list helper sorts the full checkpoint list by createdAt before filtering by id. (src/gateway/session-compaction-checkpoints.ts:819, ab966c214be0)
  • Branch RPC caller: The sessions.compaction.branch handler calls getSessionCompactionCheckpoint before invoking compactionCheckpointStore.branchCheckpointSession, so the user-facing branch path still pays the shared sort first. (src/gateway/server-methods/sessions.ts:1612, ab966c214be0)
  • Restore RPC caller: The sessions.compaction.restore handler also calls getSessionCompactionCheckpoint before restoreCheckpointSession, so restore keeps the same pre-mutation sort. (src/gateway/server-methods/sessions.ts:1729, ab966c214be0)
  • Proof state: The live PR body lists targeted test/autoreview/diff-check commands but no redacted terminal output, logs, screenshot, recording, or linked artifact from an after-fix branch/restore run. (fc1315f15e16)
  • Related resubmission: The only matching title search results are this open PR and the earlier closed unmerged resubmission source, which was closed for the active-PR limit rather than replaced by a merged fix.

Likely related people:

  • scoootscooob: Authored and merged the original compaction checkpoint feature that introduced the gateway checkpoint helpers and sessions.compaction RPC surface. (role: feature owner; confidence: high; commits: f4fcaa09a358; files: src/gateway/session-compaction-checkpoints.ts, src/gateway/server-methods/sessions.ts, src/config/sessions/types.ts)
  • RomneyDa: Live PR history shows this account authored and merged the recent broad commit currently blamed for the accessor and gateway checkpoint lookup lines. (role: recent line owner; confidence: medium; commits: 751a6c23f098; files: src/config/sessions/session-accessor.ts, src/gateway/session-compaction-checkpoints.ts, src/gateway/server-methods/sessions.ts)
  • steipete: Shortlog and recent git history show heavy work across the same gateway/session/accessor files, including adjacent session and gateway refactors. (role: adjacent area contributor; confidence: medium; commits: e7d33b4870f7, 9e0d35869521, 3174c6919de1; files: src/gateway/session-compaction-checkpoints.ts, src/gateway/server-methods/sessions.ts, src/config/sessions/session-accessor.ts)
  • vincentkoc: The live PR is assigned to this account, and git history shows adjacent gateway/session touches in the same file set. (role: assigned reviewer and recent adjacent contributor; confidence: medium; commits: aa69b12d0086, 74e7b8d47b18; files: src/gateway/session-compaction-checkpoints.ts, src/gateway/server-methods/sessions.ts, src/config/sessions/session-accessor.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jun 26, 2026
@vincentkoc vincentkoc self-assigned this Jun 26, 2026
@vincentkoc
vincentkoc force-pushed the codex/high-quality-algo-6-resubmit branch from 57aa5e0 to fc1315f Compare June 26, 2026 22:39
@vincentkoc

Copy link
Copy Markdown
Member

Maintainer review/proof for synced head fc1315f15e165403369e9e2cb90e1863e378b6db:

Proceeding to merge through scripts/pr merge-run.

@vincentkoc
vincentkoc merged commit aca905c into openclaw:main Jun 26, 2026
95 checks passed
@vincentkoc

Copy link
Copy Markdown
Member

Merged via squash.

wangmiao0668000666 pushed a commit to wangmiao0668000666/openclaw that referenced this pull request Jun 27, 2026
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 27, 2026
xydigit-zt pushed a commit to xydigit-zt/xydigit-zt-openclaw that referenced this pull request Jun 28, 2026
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants