Skip to content

Keep knowledge snapshots that unfinished runs pinned - #223

Merged
tzhouam merged 1 commit into
mainfrom
kb/snapshot-retention-pins
Sep 28, 2026
Merged

tzhouam merged 1 commit into
mainfrom
kb/snapshot-retention-pins

Conversation

@tzhouam

@tzhouam tzhouam commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Knowledge-service redesign follow-up (§8.5): a snapshot referenced by an unfinished run is never deleted. Strict/quality runs pin their snapshot at reservation (#213), but pruning only kept the newest ten plus the active one.

  • Pins: activate.prune keeps every snapshot pinned (<run>/knowledge.json) by a non-terminal run, or by an interrupted reservation that never launched (it can be reclaimed). Each run status is read once, so a concurrent reclaim can't slip between reads.
  • Grace period: switching the active snapshot records when the old one stopped being active, and snapshots deactivated less than an hour ago are never pruned. That window covers a reservation that resolved the snapshot just before the switch; runs live in other processes, so they can't share the activation lock.
  • Run roots: run directories come from KB_RUN_ROOTS, else the runs' effective run_root (environment or .env).
  • Tests: an autouse fixture keeps tests off the real runs directory.

Commit hook approved; full suite and SPEC freshness pass locally.

🤖 Generated with Claude Code

Strict and quality runs pin the knowledge snapshot active at reservation
(#213), and the design requires that a snapshot referenced by any unfinished
run is never deleted. Pruning kept only the newest ten plus the active one,
so a long queue could lose a pinned snapshot and fail those runs.

- activate.prune also keeps every snapshot that a run pinned
  (<run>/knowledge.json) while that run is not terminal, or is an
  interrupted reservation that never launched (it can be reclaimed and will
  run with its original pin).
- Switching the active snapshot records when the previous one stopped being
  active, and pruning never removes a snapshot deactivated less than an hour
  ago. A reservation that resolved it just before the switch publishes its
  pin within that window. The review runs live in other processes (and
  possibly other hosts) that cannot share the service's activation lock, so
  the coordination is this grace period.
- The run directories come from KB_RUN_ROOTS (os.pathsep-separated, for
  several servers), else the runs' EFFECTIVE run_root (environment or the
  .env files Settings reads).
- An autouse test fixture points KB_RUN_ROOTS at an empty directory, so tests
  never read the real ~/.infermatrix-copilot/runs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-28T18:09:49.060389Z 4bdef4e PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4bdef4e112

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +217 to +219
marker = root / DEACTIVATED.format(name=old.name)
if marker.exists() and now - marker.stat().st_mtime < PIN_GRACE:
continue # deactivated too recently: a reservation may be pinning it right now

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Synchronize pin publication before expiring snapshot protection

When a reservation process resolves the old active snapshot and is then stalled for more than PIN_GRACE before writing knowledge.json, a later activation can find that snapshot outside the newest ten and delete it here because its marker has expired and its pin is not yet visible. When the reservation resumes, it writes a dangling pin and _launch fails the queued review as unavailable, violating the stated invariant that an unfinished run's snapshot is never deleted. Pin publication needs synchronization or durable in-progress reservation tracking rather than a finite timeout alone.

Useful? React with 👍 / 👎.

@tzhouam
tzhouam merged commit 20d8d49 into main Sep 28, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant