fix: deterministic source-import missing detection and denial-guard supersession - #154
fix: deterministic source-import missing detection and denial-guard supersession#154Coding-Dev-Tools wants to merge 6 commits into
Conversation
…upersession Two failing offline-gate tests and two latent defects: - mark_source_import_items_missing() gains an explicit source_keys path: a complete scan now marks exactly the planned-missing rows instead of relying on the last_seen_at < run_started heuristic, which a renamed file defeated (rename stamps last_seen_at=now, so the next run's delete never matched). The documents importer shares ObsidianImporter.import_scan and inherits this. - _clear_superseded_denial() now supersedes a billing denial on persisted-state content change, not wall-clock comparison. Equal-timestamp entitlements made the strict '>' stick forever after a valid reconnect, and a naive '>=' would let a pre-denial record saved in the same coarse clock tick resurrect grants. cloud_session.saved_session_digest() fingerprints the session bytes so write order is observed without exposing credentials. - tests: deflake ULID same-millisecond ordering (repair cursor sweep count) and the archived_at == valid_from boundary (half-open temporal exclusion). Verified: full offline suite green, ruff/pyright clean, eval gates unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 200c858e88
ℹ️ 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".
| and current is not None | ||
| and current != _denied_state_digests.get(known_source) |
There was a problem hiding this comment.
When the state file is unreadable while a 401/402/403 is recorded, the captured digest is None; if denial persistence also fails and the mount later recovers, current becomes a string and therefore differs from that None baseline even though the file still contains the pre-denial active entitlement. The next license read consequently clears the process guard and restores paid grants without a reconnect. Require both the captured and current digests to be known before treating a difference as evidence of a superseding rewrite.
Useful? React with 👍 / 👎.
| if source_keys: | ||
| placeholders = ",".join("?" for _ in source_keys) | ||
| return int(self.conn.execute( | ||
| f"UPDATE source_imports SET state='missing', missing_at=? WHERE vault_id=? " | ||
| f"AND source_key IN ({placeholders})", |
There was a problem hiding this comment.
For a vault whose historical manifest has grown beyond 10,000 rows through repeated deletions and additions, import_scan() obtains only the default 10,000 rows from list_source_import_items(). If that page contains any missing candidates, this branch returns after updating only their keys, whereas the previous timestamp-based update covered all unseen rows; a run reported as complete can therefore leave omitted historical rows live until a later complete import happens to fall through to the empty-key path. Fetch the full bounded manifest or page/batch missing detection before taking this exact-key return path.
AGENTS.md reference: AGENTS.md:L215-L219
Useful? React with 👍 / 👎.
P1 (v2_api): _clear_superseded_denial now requires both the captured baseline digest and the current digest to be known before treating a difference as a superseding rewrite. An unreadable state file at denial time left None as the baseline; a later recovered read then differed from it and cleared the guard even though the file still held the pre-denial active entitlement. Unknown baselines now stick (fail-closed) until the next denial cycle or restart. P2 (source imports): list_source_import_items gains offset paging and ObsidianImporter._all_source_items() pages the full manifest (bounded at 200k rows) for both import planning and link reconciliation. A manifest grown past the 10k single-page cap left historical rows beyond the page invisible, so the explicit source_keys missing-marking path skipped them while the run reported itself complete. Verified: unknown-baseline fail-closed probe, reconnect-supersedes probe, 10,050-row paging probe (complete, duplicate-free), full affected test files (obsidian importer/schema, document importer, hosted plan) exit 0, ruff+pyright clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e374bc1b79
ℹ️ 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".
| return int(self.conn.execute( | ||
| f"UPDATE source_imports SET state='missing', missing_at=? WHERE vault_id=? " | ||
| f"AND source_key IN ({placeholders})", | ||
| (now_ts(), vault_id, *source_keys), |
There was a problem hiding this comment.
When two dashboard imports target the same vault, an older run can plan a key as missing, a newer run can then upsert that row as active, and this unconditional key-based update will finally overwrite the newer state back to missing. The service starts independent worker threads without vault-level serialization, and this branch no longer checks last_seen_at, last_seen_job_id, or the current state, so it should condition the update on the row still matching the planned generation.
AGENTS.md reference: AGENTS.md:L215-L216
Useful? React with 👍 / 👎.
| placeholders = ",".join("?" for _ in source_keys) | ||
| return int(self.conn.execute( | ||
| f"UPDATE source_imports SET state='missing', missing_at=? WHERE vault_id=? " | ||
| f"AND source_key IN ({placeholders})", | ||
| (now_ts(), vault_id, *source_keys), |
There was a problem hiding this comment.
On SQLite builds whose host-parameter limit is below len(source_keys) + 2, a complete scan with enough deleted historical rows constructs one oversized IN statement and raises OperationalError; import_scan() then reports the job as failed and leaves all missing rows unfinalized. The new pager admits up to 200,000 manifest rows, which exceeds common 32,766-variable SQLite builds, so these keys need to be updated in bounded batches.
Useful? React with 👍 / 👎.
| """ | ||
| items: list[dict] = [] | ||
| page_size = 10_000 | ||
| for _ in range(20): # bounded: at most 200k manifest rows per import run |
There was a problem hiding this comment.
For a vault with more than 200,000 historical rows, this loop exits after the twentieth full page without indicating truncation, so later rows remain invisible to missing detection and link reconciliation while the import can still report completed. Fresh evidence beyond the earlier 10,000-row finding is the newly introduced fixed 20-iteration cap; either continue until a short page or explicitly fail/mark the run partial when the bound is reached.
AGENTS.md reference: AGENTS.md:L215-L219
Useful? React with 👍 / 👎.
…ging Three follow-up findings on the source-import missing path: - Concurrent imports: the exact-key update is now per-key and conditioned on the row still matching its planned generation (last_seen_at, last_seen_job_id, live state). A newer run that re-upserts a row after an older run planned it missing keeps its fresh state instead of being clobbered back to missing. - SQLite host-parameter limits: the oversized IN clause is gone entirely — per-key updates in one transaction scale to any manifest (200k keys marked in ~6s locally). - Paging cap: _all_source_items() now reports whether the full manifest was read. Beyond the 200k-row memory bound the run is marked partial, missing finalization is skipped, and link reconciliation refuses to retire edges on the incomplete view. Probes: stale-plan-vs-refreshed-row preserved; 200,050-row manifest flags truncation and marks all 200k planned keys; affected suites (obsidian importer/schema, document importer, hosted plan, consolidate) exit 0; ruff+pyright clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa15f95ced
ℹ️ 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".
| current = _persisted_state_digest(known_source) | ||
| baseline = _denied_state_digests.get(known_source) | ||
| if ( | ||
| _AUTHORITATIVE_DENIAL_PENDING.is_set() | ||
| and checked_at > _authoritative_denial_at | ||
| and baseline is not None | ||
| and current is not None | ||
| and current != baseline |
There was a problem hiding this comment.
When a license read loads the pre-denial active session while _record_authoritative_denial() is between _mark_authoritative_denial() and its persistence write, that denial write itself changes current from the captured baseline. This branch consequently clears the guard and returns the stale active known value as though a reconnect superseded the denial. Bind the digest to the bytes used to parse known, or re-read the entitlement after detecting a change, so the denial persistence write cannot be mistaken for a newer active answer.
Useful? React with 👍 / 👎.
| page = self.store.list_source_import_items( | ||
| vault_id=vault_id, states=states, limit=page_size, offset=len(items), | ||
| ) |
There was a problem hiding this comment.
When two dashboard imports target the same vault, the independent worker started in engraphis/service.py:3173 can rename or insert rows between these page queries. Because OFFSET is applied to the mutable ORDER BY relative_path result, a row moving across the boundary shifts an unread row into an earlier page; the next offset skips it, _all_source_items() can still return manifest_complete=True, and the omitted historical row is never planned or marked missing. Serialize same-vault planning or hold a consistent database snapshot across the whole manifest read.
AGENTS.md reference: AGENTS.md:L215-L216
Useful? React with 👍 / 👎.
| for _ in range(20): # bounded: at most 200k manifest rows per import run | ||
| page = self.store.list_source_import_items( | ||
| vault_id=vault_id, states=states, limit=page_size, offset=len(items), | ||
| ) | ||
| items.extend(page) | ||
| if len(page) < page_size: | ||
| return items, True | ||
| return items, False |
There was a problem hiding this comment.
For a vault containing exactly 200,000 manifest rows, all twenty pages are full, so the loop falls through and reports manifest_complete=False even though no row was truncated. Every complete import at that exact size is therefore forced to partial, and missing-item finalization plus link reconciliation are permanently skipped. Fetch one additional row/page, or otherwise distinguish exactly-at-cap from beyond-cap before returning False.
AGENTS.md reference: AGENTS.md:L215-L216
Useful? React with 👍 / 👎.
…paging - Denial guard race: the supersession digest now travels with the parse. _session_entitlement_snapshot() and _read_entitlement_cache_snapshot() return the entitlement plus a sha256 of the exact bytes it was parsed from, and _clear_superseded_denial() compares that observed digest against the denial baseline instead of re-reading a file that may have changed since. The denial persistence write landing between a license read's parse and its check can no longer pose as a superseding reconnect. cloud_session gains saved_entitlement_snapshot(); _session_entitlement()/_read_entitlement_cache() remain as thin wrappers for their other callers. - Manifest paging: OFFSET is replaced by a (relative_path, id) keyset cursor. OFFSET on a live ORDER BY lets a concurrent rename shift an unread row across the page boundary so it is silently skipped while the pager reports a complete read; the keyset cursor returns every row at or after the cursor exactly once, and a row renamed below the read range degrades into content-hash rename detection. - Cap boundary: _all_source_items() probes one row past the 200k-row memory cap, so a manifest of exactly 200,000 rows reads as complete instead of forcing every such import to partial. Probes: mid-read denial write leaves the guard set while a post-write reconnect parse clears it; exactly-at-cap manifest reports complete; beyond-cap reports partial with a duplicate-free plan set; 200k-row keyset read in ~2s. Affected suites (obsidian importer/schema, document importer, hosted plan, consolidate) exit 0; ruff+pyright clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afdcd3cab4
ℹ️ 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".
| self.store.mark_source_import_items_missing( | ||
| vault_id=vault_id, seen_before=run_started, | ||
| preserve_paths=self._rejected_paths(scan), | ||
| missing_items=missing, |
There was a problem hiding this comment.
When a concurrent same-vault import refreshes a planned-missing row, the generation predicates correctly leave that row active and the store method returns a smaller update count, but this call discards the result and the following loop still records every planned row as missing and includes it in the completed report. The job history and receipt can therefore claim that a live, newer source was removed; propagate the successfully updated identities (or re-read them) and finalize only those rows.
AGENTS.md reference: AGENTS.md:L215-L219
Useful? React with 👍 / 👎.
…marked mark_source_import_items_missing() returns the source_keys it actually marked instead of a count: the guarded per-key path collects keys whose generation predicates matched, and the heuristic path selects its rows before updating (chunked IN under host-parameter limits). import_scan records job history and the completed report from that reality — rows the generation guard left live because a concurrent import refreshed them are recorded as skipped, never as missing, so the job receipt can no longer claim a live, newer source was removed. Probe: stale plan vs refreshed row -> only the stale key returned and marked; refreshed row stays imported and out of the missing report; heuristic path returns keys. Affected suites exit 0; ruff+pyright clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff4aba6ef9
ℹ️ 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".
| for item in missing: | ||
| if item in finalized: |
There was a problem hiding this comment.
When a complete import finalizes many historical deletions, finalized can contain nearly all of the up-to-200,000 manifest rows, and this list-membership test scans dictionaries from the beginning for every missing item. That makes post-update job finalization quadratic—roughly 20 billion comparisons at the configured cap—so a large import can appear hung after the database update succeeds; use the already-built marked_keys set for this check instead.
Useful? React with 👍 / 👎.
…zation The guard-skipped loop tested membership against the finalized list, scanning up to 200k dicts per missing item — quadratic job finalization that can appear hung on a large complete import. Test the item's source_key against the already-built marked_keys set instead.
Summary
Two failing offline-gate tests, two latent defects behind them, and two pre-existing flaky tests, all fixed and verified:
mark_source_import_items_missing()gains an explicitsource_keyspath. A complete scan now marks exactly the planned-missing rows instead of relying on thelast_seen_at < run_startedheuristic, which a renamed file defeated: rename stampslast_seen_at=now, so a delete on the next run never matched and the row stayedrenamedforever. The documents importer sharesObsidianImporter.import_scanand inherits the fix._clear_superseded_denial()supersedes a billing denial on persisted-state content change, not wall-clock comparison. Equal-timestamp entitlements made the strict>stick forever after a valid reconnect (the failingtest_newer_active_session_clears_the_process_denial_guard), and a naive>=would let a pre-denial record saved in the same coarse clock tick resurrect grants.cloud_session.saved_session_digest()fingerprints the session bytes so write order is observed without exposing credentials. Verified both directions: pre-denial session → fail-closed; rewritten reconnect → supersedes.archived_at == valid_fromlanded the historicalas_ofmidpoint exactly onvalid_to, excluded by the half-open temporal predicate (35% over 20 runs). Both now deterministic (10/10 runs each).Test plan
python -m pytest tests/ -q, exit 0)ruff check .,python scripts/check_commercial_manifest.py,pyrightclean