fix: integrate reindex recovery and proof work - #4679
Conversation
A GitHub Actions workflow job is its own serialisation domain: one self-hosted runner executes one job at a time, under the job plane's slice. It runs pytest in place instead of queueing through the workstation's pytest slot, which required the workstation's queue runner on the runner's PATH. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`verify_all` declares `pool = "pytest"` but was absent from the operation fallback, so its worker re-queued into the single-slot group it already occupied whenever the runner did not export `SINNIXD_QUEUE_POOL`. The declarations are now the test's own fixture, so a new pytest-pool operation cannot drift out of the classifier. The slot tests also read whatever the workstation had deployed: the declared worker's assertion held only because the run sat inside the deployed pytest cgroup, and queueing resolved the real `sinnixd-queue-run` from PATH. The cgroup is stubbed and both queue tools are fakes that are the whole PATH, so the unreachable-queue test now reaches the pueue check it is named for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
verify_all's absence from per-lane publication rests on it being default-checkout-only and scheduled; assert both alongside the publication list so restoring it to a lane fails here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pueue kills the queue runner with SIGKILL, so the workload it started survives in the transient systemd scope pueue cannot see: a reaped waiter left pytest running against the checkout while the slot showed free. `agentctl job cancel` stops the task and then empties that scope. The launch file carries the resolved environment and belongs to whoever ends the run, so it is deleted only when the cancellation succeeded -- a task still on the queue reads it when it starts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3efbcb9798
ℹ️ 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".
| context = build_disposition_context( | ||
| archive_root=archive_root, | ||
| blob_root=archive_root / "blob", | ||
| source_db=archive_root / "source.db", |
There was a problem hiding this comment.
When --plan was compiled for archive A but --archive-root points to a cloned archive B containing the same content-addressed carrier, the digest check still succeeds and this context is built from B without comparing plan.blob_root or plan.archive_root. The restore then publishes A's hook event or browser capture into B, while the receipt misleadingly names A. Apply performs this namespace check, so the standalone restore path should enforce the same binding before any spool write.
Useful? React with 👍 / 👎.
| for member in removable: | ||
| if context.blob_store.blob_path(member.blob_hash).exists(): | ||
| results.append( | ||
| MemberResult(member.blob_hash, MemberOutcome.RETAINED_ABSENT, "the GC seam declined this member") |
There was a problem hiding this comment.
When source.db has a pending GC generation from an interrupted earlier run, unlink_unreferenced_blob_hashes_under_exclusion resumes that generation and returns without processing this call's requested hashes. Those planned paths therefore still exist, but this branch records non-blocking RETAINED_ABSENT; if the resumed generation has no errors, the receipt has ok=true even though none of the authorized members were deleted. Remaining planned hashes need to be reported as retryable blockers unless a fresh liveness check proves why they were retained.
AGENTS.md reference: AGENTS.md:L113-L115
Useful? React with 👍 / 👎.
| referenced_present = len(context.referenced_hashes & {member.blob_hash for member in plan.members}) | ||
| if referenced_present != plan.denominator.referenced_present_count: |
There was a problem hiding this comment.
If a writer publishes a new hash after planning and is stopped before active apply, the new reservation/hash is absent from plan.members, so intersecting current references with only the planned hashes leaves referenced_present unchanged. Because the current namespace, distinct count, byte count, referenced total/absent count, and invalid entries are never recomputed, apply proceeds and emits a successful terminal receipt that omits the newly added physical object, contrary to the whole-namespace disposition contract.
Useful? React with 👍 / 👎.
| if not require_positive_conversational_evidence( | ||
| [session], | ||
| provider=session.source_name, | ||
| source_path=_archive_raw_source_path(raw_data, source), | ||
| ): | ||
| return |
There was a problem hiding this comment.
For a grouped source such as a Claude Code file that parses to only an empty session, raw capture has already published the blob and committed its blob_publication_reservations row before write_pair runs. This early return skips both raw admission and receipt consumption, while the final discard_pending() cannot remove an already-flushed reservation; the import reports no parse failure but leaves an unresolved durable reservation that protects an orphan blob from GC indefinitely. Admit the raw as a typed refusal or explicitly abandon its receipt before returning.
AGENTS.md reference: AGENTS.md:L69-L70
Useful? React with 👍 / 👎.
| def restoration_target(self, path: Path) -> RestorationTarget | None: | ||
| envelope = _read_envelope(path, expected_keys=self._ENVELOPE_KEYS) | ||
| if envelope is None: | ||
| return None | ||
| event_id = envelope.get("event_id") | ||
| if not isinstance(event_id, str) or not event_id: | ||
| return None | ||
| return RestorationTarget(destination=RestorationDestination.HOOK_EVENT_SPOOL, logical_id=event_id) |
There was a problem hiding this comment.
A physical blob containing any unrelated JSON object with the six hook-like keys, a valid-looking event_id, supported provider, and timestamp is assigned HOOK_EVENT_SPOOL solely from its shape. If no source prover claims that blob, the plan becomes restore_required and active apply enqueues it as a real hook event even when it was actually a raw transcript, tool output, or other artifact. Bind restoration eligibility to durable hook-carrier provenance rather than treating a generic JSON shape as proof that the material is a wanted hook event.
Useful? React with 👍 / 👎.
| ) | ||
| _write_attachment_native_ids(conn, ref_id, attachment) | ||
| affected_attachment_ids = touched_attachment_ids | (refresh_attachment_ids or set()) | ||
| affected_attachment_ids = (touched_attachment_ids | (refresh_attachment_ids or set())) - unowned_attachment_ids |
There was a problem hiding this comment.
On a full-replace reingest where an attachment previously had a valid ref but its incoming owner is now missing or ambiguous, the message replacement deletes the old attachment_refs row while the attachment upsert preserves its old ref_count. Subtracting unowned_attachment_ids from the refresh set prevents that count from being recomputed, leaving (for example) ref_count=1 with zero actual refs and producing incorrect closure/liveness results. Retaining typed unowned evidence still requires setting its derived ref count to zero without sweeping the row.
Useful? React with 👍 / 👎.
| try: | ||
| usage = disk_usage(temp_root) | ||
| except OSError: | ||
| return 0 |
There was a problem hiding this comment.
When TMPDIR names a missing or unreadable path, disk_usage raises and this helper substitutes zero for used space. Both the baseline and every measured sample then report zero temporary growth, so the command can emit status: succeeded even though one of its four declared resource dimensions was never measured. Propagate a typed probe-unavailable error here, as the procfs probes already do, rather than allowing an environmental measurement failure to satisfy the limit.
Useful? React with 👍 / 👎.
| drift = _revalidate(member, context=context) | ||
| if drift is not None: | ||
| results.append(MemberResult(member.blob_hash, MemberOutcome.BLOCKED, drift)) | ||
| continue |
There was a problem hiding this comment.
For an unreferenced source_present member backed by a live, appendable session file, this revalidation can succeed and add the member to removable, after which the external source file can be truncated, replaced, or deleted before the later batched GC call. The offline-writer guard only excludes archive writers and does not freeze configured source files; GC rechecks database liveness but not this source proof, so it can unlink the blob after its purported replacement has vanished and destroy the last copy. The proof must remain stable or be rechecked at the actual deletion boundary.
Useful? React with 👍 / 👎.
| max(peak.rss_bytes, candidate.rss_bytes), | ||
| max(peak.pss_bytes, candidate.pss_bytes), | ||
| max(peak.swap_bytes, candidate.swap_bytes), | ||
| max(peak.temp_delta_bytes, candidate.temp_delta_bytes), | ||
| ) |
There was a problem hiding this comment.
The sampler thread and the async caller both execute observe() and perform an unlocked read-modify-write of peak. If one observes a high RSS/PSS/temp value while the other computes from the previous lower peak, the later assignment can overwrite the high sample with lower component values, allowing an absolute envelope check to pass despite a real peak violation. Protect the aggregate with a lock or collect immutable samples and reduce them after the sampler stops.
Useful? React with 👍 / 👎.
`_reextract_prefix_tail_db` deleted from `insight_materialization`, which #4649 retired from the index DDL, so every deferred-tail resolution raised `no such table`. Delete the child's `session_profiles`, `session_latency_profiles`, `session_work_events` and `session_phases` instead: their staleness predicate compares the session's sort key, updated-at and content hash, none of which re-extraction moves, so a profile materialized over the whole child would report fresh indefinitely. The same retirement left four `SessionInsightCountDescriptor`s whose `count_key`s are no longer snapshot fields and whose `table_key` names the retired table, making every `session_insight_status_sync`/`_async` call raise `KeyError`. Remove them and their SQL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#4649 removed the readiness descriptor whose `zero_counts` short-circuited `stale_thread_count`, `orphan_thread_count` and `stale_tag_rollup_count` when their product tables were absent, leaving the three count descriptors querying `session_profiles`, `threads` and `session_tag_rollups` unguarded. Restore the gates as `table_key`, so a status call on an archive without those relations falls back to zero instead of raising `no such table`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The readable index guard admitted any object whose name starts with `messages_fts`, so an undeclared object sharing that prefix was survivable before anyone decided it was. Declare the five objects the message FTS surface owns and match membership. `search_archive_blocks` reached SQL against that admitted-absent surface and raised `no such table: messages_fts`; it now refuses with the same typed `DatabaseError` the other search routes raise. Presence is the check, not freshness: rebuild and differential routes read this surface while it is legitimately behind `blocks`. Deletes `_readable_without_fts`, an unreachable copy of the same admission decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A tail-only replay (the live-append path) wrote the newly accepted chunk and left the prefix's `claude_parse_coverage` row at its own position and timestamp, while `sessions.content_hash` described the chain's reduction -- one event carrying the chain's totals, the chain's newest timestamp, and a slot after every point-in-conversation event. `_reconcile_chain_summary_events` now reconciles the retained row on all three axes the hash covers, reusing the writer's own payload, summary, timestamp, and next-position helpers so the row matches what a full replace of the composed session writes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every `sessions` column carried `record_name=None`, so the record projection rendered empty and `get_session` and `list_sessions` both emitted `SELECT FROM sessions`. Declare each column's record name and select expression on the column itself rather than through a separate wrapper applied after the spec, and cover the projection with a spec contract and a read-path test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replacing a parent deletes its blocks, and the ON DELETE SET NULL foreign key on session_links.parent_tool_use_block_id nulls every inbound child edge while the replacement reinserts the same deterministic block ids. Identity resolution revisits only unresolved edges, so the join key stayed NULL and the dispatch dropped to unresolved. _refill_inbound_dispatch_block_ids rebinds resolved children on every write. Dispatch child identity is now read from the progress payload alone -- the record envelope names the emitting session -- and candidate names are compared as the sessions they resolve to, so several exact names for one child are one identity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ema-generation suites test_plain_cli_snapshots took a fresh query-only lease per test (each one revalidates every artifact byte) and re-cloned plus re-materialized the cli-mixed insights for each of eight read-only consumers. test_schema_generation cloned the schema-coverage archive for each of seven tests that only read index.db. Both are now module-scoped; the per-test fixtures only set the environment the CLI reads. seeded_archive_writable has no remaining consumer and is deleted. test_completion_matrix was already module-scoped and is unchanged.
Delete the eight-value insight readiness verdict enum, the last references to the retired insight_materialization marker table, and the duplicate async readiness builder whose only caller was a test. Insight readiness is now the ordinary convergence signal. Four status descriptors still gated on the dropped marker table and emitted count keys SessionInsightStatusSnapshot does not accept, so session_insight_status_sync raised KeyError on every call. Two import-time checks now require every emitted count to be a snapshot field, every table_key to be one the presence probe reports, and every referenced fallback key to be one some descriptor emits. InsightReadinessEntry carries structural facts (table_present, diverged, incomplete) that the export gate and capability mapping read directly. InsightReadinessReport reports converged plus debt_stages from the convergence_debt ledger; None means the ledger was unreadable, which is never success. The three CTE-derived surfaces the builder silently dropped are now reported, so a known insight name no longer yields an empty entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Status counts read product tables without requiring them to exist. The reads were unreachable behind the insight_materialization KeyError; removing that exposed them, so an archive whose derived tables are not built yet raises "no such table" instead of reporting zero. Four count descriptors read a product table they did not gate on. Two relations are query-time views over session_profiles and session_work_events: sqlite_master lists a view whether or not its body's tables exist, so presence alone never made them readable. The view dependency cannot be read off a descriptor's query text, so it is declared once and every gate expands through it. A descriptor now gates on the set of relations its query reads, and an import-time check refuses any descriptor whose query names a product table it does not gate on. Also repoint two test references at storage.derived, renamed from storage.insights in #4614; the stale import failed collection on master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Give every physical blob exactly one disposition proven against a configured source, restore sole-copy carriers into their ordinary spool, and delete only proven-redundant unreferenced objects through the canonical blob-GC seam. Hook-event and browser-capture carriers are proven by the owning production read route rather than by bytes: acquisition derives fields the spool file does not carry, so byte equality reports reproducible material as a sole copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A run of id-less, text-less AI Studio turns sharing one timestamp is separated only by the Drive file its document block cites. Pin that contract on the corpus shape, collapse the block-reference owner tier that the semantic content payload now subsumes, and restore the three identity tests that the partition change left asserting retired behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`require_positive_conversational_evidence` is the archive's admission law for "parsed, but no conversation is present". The daemon decode worker, live batch convergence, the incremental append route, and offline replay apply it; `pipeline/services/archive_ingest.py::parse_sources_archive`, the importer behind the public `parse_file`/`parse_sources` API and the demo seeder, did not. A JSON document under a watched Claude Code project that satisfies only dispatch's loose messages-list shape was therefore written as a session keyed on its own filename stem with zero authored messages -- the fragment-identity phantom class, arriving through the one chokepoint that did not agree with the other four. Also pins three laws that had no anti-vacuity coverage: a `tool-results/` sidecar never becomes a session on either chokepoint whatever it contains; the importer refuses `toolu_*`/`wf_*` stem identity while still admitting a transcript with content; a sidecar event with no readable file mtime keeps its time unknown instead of inventing an ingestion-time stamp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An upload-only claude.ai `files` reference gains its bytes when a later capture revision carries them as `extracted_content`. The bytes must land `acquired` under the same attachment identity: a second identity strands the original reference and double-counts the attachment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
resolve_archive_ingest_dispatch read CPU count alone, so every parse_sources_archive call spawned the ambient worker count -- 16 fresh interpreters on a 24-thread host -- even for a walk of one small file, the shape the API ingest facade produces per source. Tier the plan on the byte boundaries resolve_ingest_batch_dispatch already uses, and record the walk, parse, and pool phases in the append.* stage ledger that previously covered only the archive write. Measured over six one-file walks (1,074 messages), interleaved arms: 41.2/41.5 messages/s at 0.078 process CPU utilization with the pool, 325.4/476.3 at 0.795/0.764 in-process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restore batches host parameters below the connection's own variable limit, writes a vector and its metadata together so a metadata row (the tier's reuse signal) can never exist without the vector at its address, and reports every unrestored hash as a typed miss naming its cause. Preservation builds the copy in a private temporary file, makes it self-contained, derives its receipt from the finished copy, and renames it into place, so a file at the destination path is always a whole copy its receipt describes. Deletion re-reads the copy's digest and refuses a receipt that names another file or a copy that has changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The envelope reader selects attachment_refs.direction and producer_ref unconditionally, so a schema missing either column fails every read at statement preparation. Assert the write-then-read round trip on a freshly bootstrapped index, and record the v91 declaration for the columns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rebase conflict resolution re-added the archive index-fast-forward CommandSpec that #4666 deleted along with devtools/index_fast_forward.py. Its module target does not exist.
A procfs read failure returned zero RSS, PSS and swap, which satisfies every declared limit and reports the envelope as met without measuring it. Missing fields now raise and the command records blocked-env.
f64c6cc to
381f886
Compare
3efbcb9 to
96a0fcc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96a0fcc82f
ℹ️ 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".
| if str(context.blob_store.root) != plan.blob_root: | ||
| blockers.append(f"plan blob namespace {plan.blob_root} is not the namespace being applied") |
There was a problem hiding this comment.
When plan and apply are invoked from different working directories with the same relative --archive-root archive, the CLI uses Click's default resolve_path=False, so both roots are serialized and compared as the lexical string archive/blob. If the second directory contains a clone with the same planned hashes and reference denominator, this check passes and active apply uses the clone's source.db/index.db and deletes from the wrong archive. Fresh evidence beyond the earlier standalone-restore report is that the reviewed head's apply guard also compares only this uncanonicalized blob-root string; bind both the canonical archive and blob roots before authorizing effects.
Useful? React with 👍 / 👎.
| with closing(_connect(copy_path, readonly=True)) as conn: | ||
| digest, _counts = _table_digest(conn) | ||
| if digest != proof.get("table_set_digest"): | ||
| raise ValueError("preservation copy no longer matches its receipt digest") |
There was a problem hiding this comment.
When the preserved database at this path is replaced, or its vector/model/hash contents are updated without changing any table's row count, this comparison still succeeds because _table_digest hashes only table names and counts. A stale AC2 receipt can therefore authorize unlinking a different same-sized preservation copy, contrary to this function's stated identity binding; the proof needs a logical-content or other immutable copy digest rather than cardinalities alone.
Useful? React with 👍 / 👎.
| handle.write(payload) | ||
| handle.flush() | ||
| os.fsync(handle.fileno()) | ||
| os.replace(temporary, path) |
There was a problem hiding this comment.
When an operator reuses --receipt for a later dry run, failed run, or retry, os.replace silently overwrites the prior receipt even though this function promises append-only publication. That destroys the durable record of the earlier active disposition and makes the path describe only the latest invocation; publish with exclusive-create semantics and fail when the receipt already exists.
Useful? React with 👍 / 👎.
Master's side kept for every conflicted hunk: #4690's dispatch-link functions and #4692's attachment writer in write.py, INDEX_SCHEMA_VERSION 94, the daemon-probe status path, and the tests master advanced with them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
The writer retains an attachment whose owner coordinate is claimed by more than one message as a typed unowned row with no attachment_refs edge. Such a row is an orphan by the relink scan's definition, so the raw re-parse reaches it, and attachment_message_owner_key raises the same ambiguity again -- aborting `polylogue ops maintenance blob-reference-closure` with a traceback in both dry-run and --apply. Classify it as UnrecoverableAttachmentReason.OWNER_AMBIGUOUS. Archive verification's blob-reference-closure and attachment-coverage checks key on acquired-and-unreferenced, which the unowned row is not, so both stay clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
Both import blocks kept in test_archive_verification.py. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c36e83cc46
ℹ️ 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".
| with closing(_connect(partial, readonly=True)) as copy_reader: | ||
| digest, counts = _table_digest(copy_reader) | ||
| _fsync_file(partial) | ||
| os.replace(partial, destination_path) |
There was a problem hiding this comment.
When two preservation runs target the same destination, or another process creates that path after the exists() check, both calls can reach this unconditional os.replace. The later call overwrites the first completed preservation copy; depending on receipt-write ordering, the surviving .receipt.json can then describe the overwritten database and the other call merely fails in write_once. Use a no-replace publication primitive at the final publication boundary rather than relying on the earlier existence check.
Useful? React with 👍 / 👎.
| archive_root = archive_root.resolve() | ||
| db_path = archive_root / "index.db" | ||
| if not db_path.is_file(): | ||
| raise FileNotFoundError(db_path) |
There was a problem hiding this comment.
For a valid recovery state where .index-active-pointer selects the promoted generation but the conventional index.db is stale, this explicit path bypasses resolve_active_index_path; Polylogue treats an explicit db_path as pinned and measures the stale database instead of the public archive generation. The resulting resource receipt can therefore pass for data that is not currently served, so resolve and pin the active index path once before opening the API and use that same path for receipt metadata.
Useful? React with 👍 / 👎.
| try: | ||
| observe() | ||
| except ResourceProbeUnavailableError: | ||
| return |
There was a problem hiding this comment.
If a background sample encounters a transient unreadable or incomplete procfs report after the initial probe, this handler silently terminates the sampler without recording the failure. Later per-round observations can succeed, allowing the command to report succeeded even though between-round peaks were unmeasured for the rest of the run; retain the sampler error and return blocked-env after joining instead of treating it as normal completion.
Useful? React with 👍 / 👎.
| hooks_root = archive_root / "hooks" | ||
| sources = tuple( | ||
| (spec.source_id, spec.root) for spec in hook_spool_sources(primary_root=hooks_root) if spec.root.is_dir() | ||
| ) | ||
| return hooks_root, sources, archive_root / "browser-capture" |
There was a problem hiding this comment.
When daemon.browser_capture.spool_path or sources.hook_sidecar_dir points outside the archive root, this resolver still hard-codes <archive>/browser-capture and <archive>/hooks. The plan therefore misses material already present in the configured spool, and an active restore publishes sole-copy carriers into directories the daemon is not watching while returning a successful restoration receipt. Resolve both destinations from the same runtime configuration used by acquisition.
Useful? React with 👍 / 👎.
| source_db=archive_root / "source.db", | ||
| index_db=archive_root / "index.db", |
There was a problem hiding this comment.
For an archive in the supported shadow-index recovery state, .index-active-pointer selects the served generation while <archive>/index.db is a stale regular database. Passing the conventional path here makes GC's final liveness check inspect the stale index, so a blob owned only by an attachment in the active generation can be unlinked after the disposition proof marks it removable, leaving the served attachment without its payload. Resolve and bind the active index generation before authorizing deletion.
AGENTS.md reference: AGENTS.md:L76-L77
Useful? React with 👍 / 👎.
1 participant
Summary
Integrates the reindex-recovery work: the physical blob disposition plan and
its guarded apply, embedding-vector preservation, archive-ingest parse-pool
sizing, the query-execution envelope lab check, the conversational-evidence
rule in the one-shot importer, AI Studio attachment ownership on
same-timestamp document turns, and attachment-reacquisition coverage.
Rebased onto master f80542b.
Review outcome
The adversarial review's blocking finding was the branch's own attachment
writer: retaining an owner-ambiguous attachment wrote a ref-less row that
blob-reference-closureandattachment-coverageclassify as blocking debt,and that crashed
polylogue ops maintenance blob-reference-closure.Master #4692 has since shipped its own version of that writer change, so this
branch keeps master's side of
write.pyin full -- the file is nowbyte-identical to master, which also preserves #4690's dispatch-link
functions and
INDEX_SCHEMA_VERSION = 94.What remained was the independent crash the review proved, which master does
not fix:
attachment_relink._match_session_payloadcalledattachment_message_owner_keywith no handler, unlike_write_attachments.Any ref-less attachments row makes
_read_orphaned_attachment_idsnon-empty,so the raw re-parse scan reaches the same ambiguity and raises
MessageOwnerAmbiguityErrorout ofblob_reference_closure.py, past theCLI's
BlobReferenceClosureError-only handler.Solution
_match_session_payloadclassifies the ambiguity asUnrecoverableAttachmentReason.OWNER_AMBIGUOUS-- the same typed-unrecoverableshape as
NO_AUTHORITATIVE_RAWandMESSAGE_MISSING. No consumer change isneeded: closure blockers are already filtered to
acquiredorphans, and bothrequired archive-verification checks key on acquired-and-unreferenced, which
the unowned row is not.
Two tests, each naming its anti-vacuity condition:
test_owner_ambiguous_orphan_is_reported_typed_not_raised-- writes theambiguous session through the production writer, then runs plan and relink.
Removing the handler makes it red with the review's traceback.
test_unowned_attachment_evidence_keeps_closure_and_coverage_clean--blob-reference-closureandattachment-coverageon a fixture archivecarrying the fix: profile, delegation and storage reds from the 2026-09-05 corpus #4692 unowned shape. Giving the row
acquiredturns both ERROR.Verification
The single failure is
test_empty_covered_table_without_stats_is_not_missing_coverage, confirmedfailing on master itself and tracked separately.
Residuals
destructive branch of
blob_disposition_apply(theunlink_unreferenced_blob_hashes_under_exclusioncall) has no test thatreaches a real unlink, and
embedding_preservationstill has no productioncaller.
_write_attachment_rowwith a preacquiredacquiredtuple, which wouldproduce the acquired-and-unreferenced shape both checks reject. The new test
covers the unfetched shape master produces without preacquired bytes; the
acquired variant is master's behavior, not this branch's.
_temp_used_bytesmeasures the TMPDIR filesystem, not the process(review nit 5), and the one-shot importer's evidence refusal increments no
ParseResultcounter (nit 6).🤖 Generated with Claude Code
https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid