Skip to content

fix(store): rebuild index.md from disk under a lock and keep pages it cannot parse - #27

Merged
GQAdonis merged 10 commits into
mainfrom
fix/preserve-wiki-index
Oct 3, 2026
Merged

GQAdonis merged 10 commits into
mainfrom
fix/preserve-wiki-index

Conversation

@GQAdonis

@GQAdonis GQAdonis commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Closes #15.

Root cause

The trigger is mixed binary versions on one knowledge base. It's confirmed on the maintainer's machine, where pk is 1.9.0 and prometheus-learning-worker is 1.8.0. The skill system installs both from its tools/prometheus-knowledge submodule, which is pinned at 1.8.0.

1.9.0 changed WikiEntry.sources from Vec<String> to Vec<Source>, so a 1.8.0 binary can't parse pages written in OKF v0.2's mapping form. When the 1.8.0 worker rebuilds the index:

  • it skips those pages as parse failures;
  • its render_index predates the okf_version block;
  • it overwrites index.md with only the pages it can read, with no frontmatter.

That's the issue's "after" exactly, and it's why the loss looked conditional: it depends on which binary wrote last. This repo's own .prometheus/knowledge/wiki/index.md shows the same damage.

The code on main still allowed the same loss, at any future format change or in a race:

  • Unparseable pages vanish: regenerate_index rendered only from the pages it parsed.
  • Stale snapshot: it rendered from the snapshot taken when the process opened the store, so overlapping ingests dropped each other's pages.
  • In-place writes: a run killed mid-write (the issue's exit 124 timeouts) could truncate the file.

Fix (pk-store; public signatures unchanged)

  • Locking: regenerate_index holds an exclusive fs2 lock on wiki/.index.lock from the scan through the write. The blocking wait runs off the async runtime threads.
  • Fresh scan: it renders from a scan of the wiki done inside the lock, not from the open-time snapshot.
  • Unparseable pages: index lines for pages that failed to parse are kept verbatim under their existing section. Matching uses the exact failed path, so a page that parses under a different id or filename case is never listed twice.
  • Single render: bundle::render_index_preserving renders once, from the parsed pages plus the kept lines. Pages that parse produce exactly the same output as before, so the frozen index.md format is unchanged.
  • append_log: takes the same lock, so concurrent appends no longer lose lines (40 of 80 were lost before the fix). A read error other than file-not-found now fails instead of wiping log.md.
  • Atomic writes: index.md and log.md are written to a synced temporary file, then renamed over the target.
  • Dependency: fs2 is added to pk-store. It was already a workspace dependency used by pk-learning-worker, so Cargo.lock gains one edge and no new crate. knowledge-lead approved it under team rule 5 (commit 937cf4c).

How the team handled it

This was the first change handled by .agent-team/prometheus-knowledge-maintainers. Each role ran as a separate subagent given that role's instructions from team.json.

Role Work Commits
knowledge-lead OpenSpec change preserve-wiki-index, the dependency approval, README and .gitignore, skill-system handoff note 937cf4c, 3691130, ff600ee
core-engineer Store fix, test-first, in three RED/GREEN pairs 5e71337→5151070, 9e1e962→40442b6, f57db3f→0e27913
learning-worker-engineer Regression test through the real worker binary. Against pre-fix pk-store it fails with the ## Reference section dropped. 602c856
verifier Adversarial review plus the integration gate in its own build directory. First pass: PASS WITH FINDINGS (5 minor, 6 nits). The F1/F2 corrections went back through RED/GREEN. Confirmation pass: PASS. findings committed in ff600ee

Verification

The verifier's gate on macOS, run on the final code:

Command Result
cargo fmt --all -- --check Passed
cargo clippy --workspace --all-targets --locked -- -D warnings Passed
cargo test --workspace --locked 171 passed, 0 failed
  • New pk-store tests:
    • an unparseable page's index entry survives a rebuild;
    • two writers that opened the store before either ingested both end up in the index;
    • the frontmatter is kept;
    • an entry for a deleted page is dropped;
    • concurrent appends keep every log line;
    • a page whose filename differs from its id is listed once;
    • an entry with a multi-line description survives.
  • Worker boundary test through prometheus-learning-worker run-once.
  • CI on Linux and Windows. Only macOS has been tested so far, and the verifier flagged rename-over-an-open-file on Windows (N4) as unverified.
  • A manual run of the new worker against a copy of this repo's real KB was planned but not run, because the command was declined in-session. The worker regression test covers the same path.

The review is not independent. The verifier ran in a separate context but on the same model family, so per team rule 7 a human review of this PR is required before merge.

Deliberately not done

  • F4, rebuild cost on large wikis: every ingest now scans the whole wiki under the lock. To be measured before optimizing.
  • Nits C1–C3: case-variant hand-edited lines, a page that fails while another parses under the same id, and non-UTF-8 filenames. See the findings file.

Downstream

.agent-team/prometheus-knowledge-maintainers/handoffs/issue-15-skill-system.md asks prometheus-skill-system to:

  • bump its 1.8.0 pin;
  • install pk, pk-cherry and the worker from one commit;
  • add a version-skew check to prometheus doctor;
  • update the release matrix and 13-tools-reference.md;
  • ignore wiki/.index.lock and wiki/.*.tmp.

This fix protects a knowledge base only once every writer runs this version. After this merges, the 1.8.0 worker on the maintainer's machine will be replaced with a build from main.

🤖 Generated with Claude Code

GQAdonis and others added 10 commits October 3, 2026 14:18
Approves fs2 for pk-store (already a workspace dependency; one new edge in
Cargo.lock, no new crate).

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rs and frontmatter (RED)

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… cannot parse (GREEN)

regenerate_index destroyed index.md entries in three ways:
- it rendered from the in-memory snapshot taken at open, so a second
  writer opened earlier overwrote the first writer's entries; it now
  renders from a fresh scan of wiki/ while holding an exclusive fs2
  lock on wiki/.index.lock (acquired in spawn_blocking);
- it dropped pages this binary cannot parse; existing index lines whose
  page still exists on disk and was not parsed are now carried verbatim
  under their section (bundle::merge_preserved_index_entries), while
  lines for deleted pages are dropped;
- it wrote index.md (and append_log wrote log.md) in place; both are now
  replaced atomically via a synced temp file and rename.

Closes #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
append_log read, modified and rewrote log.md with no lock, so two
appenders could each drop the other's line. It now holds the same
exclusive wiki/.index.lock as regenerate_index across the
read-modify-write, and no longer treats a read error other than
NotFound as an empty log.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Guards issue #15 at the worker boundary. The test runs the real
prometheus-learning-worker binary (run-once) against a project KB whose
index.md lists a page this binary cannot parse (a `tags:` mapping), and
asserts that after the worker writes its SessionRecord the index still
carries the okf_version block, still lists [Legacy page](/legacy-page.md)
under ## Reference, and also lists the new session record.

This is a regression guard for the store fix in 5151070, not a RED
commit. Run against pre-fix pk-store/src (937cf4c) it fails with:

  Reference section missing: ---
  okf_version: "0.2"
  ---

  # Wiki Index

  ## SessionRecord

  * [Karpathy session fedcba987654](/karpathy-session-fedcba9876543210.md)

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… multi-line entries (RED)

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e structurally (GREEN)

Carry-forward compared a frontmatter id with a file path, so a page
whose filename differs from its id (or differs in case on a
case-insensitive filesystem) kept its stale line forever and was
listed twice. The wiki scan now records the bundle-relative paths of
pages it could not read or parse, and an existing line is carried only
when its link target is exactly one of them.

The merge also re-parsed the rendered index, dropping the second line
of a multi-line description and the whole entry for a multi-line
title. bundle::render_index_preserving (replacing the unreleased
merge_preserved_index_entries) now builds one structured group map
from the parsed entries plus the carried lines and renders once;
parsed entries render byte-identically to render_index. Carried lines
are deduplicated per target. Documents that regenerate_index and
append_log take wiki/.index.lock and must not be nested.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-system handoff

- design.md: carried index lines are matched on failed-to-parse paths, and
  rendering is structural (verifier findings F1, F2); append_log shares the
  lock; consumers should ignore the lock and temp files.
- tasks.md: builder and lead tasks ticked; verification remains open.
- README: how index and log writes behave, and why the three binaries must
  share a version.
- .gitignore: this repo tracks its wiki, so ignore wiki/.index.lock and
  wiki/.*.tmp.
- Handoff note for prometheus-skill-system: bump the 1.8.0 pin, install all
  three binaries from one commit, add a version-skew doctor check.

Refs #15

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Separate-context review (same model family): PASS after one correction
cycle; workspace gate 171 passed, 0 failed. Human PR review still required.

Refs #15

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

chatgpt-codex-connector Bot commented Oct 3, 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-10-03T20:11:11.676732Z ff600ee 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.

@GQAdonis
GQAdonis merged commit 5bfdc88 into main Oct 3, 2026
3 checks passed
@GQAdonis
GQAdonis deleted the fix/preserve-wiki-index branch October 3, 2026 20:06

@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: ff600ee682

ℹ️ 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 thread pk-store/src/store.rs
Comment on lines +489 to +490
tokio::task::spawn_blocking(move || write_atomic_blocking(&path, contents.as_bytes()))
.await

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep the lock alive through cancelled blocking writes

If the calling future is cancelled while this blocking task is running, Tokio detaches the task but drops the outer .index.lock guard. Another writer can then acquire the lock and finish a newer index or log update before this stale task completes its delayed rename, overwriting the newer file and losing entries. Keep lock ownership inside the blocking operation until the rename finishes, or otherwise prevent cancellation from releasing it early.

Useful? React with 👍 / 👎.

Comment thread pk-store/src/store.rs
.map(|component| component.as_os_str().to_string_lossy())
.collect::<Vec<_>>()
.join("/");
let fallback_id = fallback_id.trim_end_matches(".md");

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 Strip exactly one markdown suffix from failed paths

For a valid concept ID ending in .md, such as guide.md, the page path is guide.md.md; trim_end_matches removes both suffixes and records the failed path as guide, while the existing index target is guide.md. If that page becomes unparseable, the targets no longer match and the rebuild deletes its index entry, violating the new preservation guarantee. Strip only the final file extension instead.

Useful? React with 👍 / 👎.

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.

pk ingest rewrites wiki/index.md destructively (drops frontmatter and existing sections)

1 participant