Repository navigation
Use declared disk scratch for writer-store rehearsal - #3599
Merged
Merged
Conversation
trissim
marked this pull request as ready for review
October 29, 2026 08:58
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
trissim
force-pushed
the
work/install-speed--writer-scratch
branch
from
October 29, 2026 09:09
74d47e3 to
5ee6d84
Compare
|
debt-src at 918681e, every finding: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
P938 recovery: 4 commits, 6 files, +36/-26. Two selected-store rehearsal loops → one. Current head:
4bd9bce04182c19cab6102ce6c23d5bee86128f2, rebased onto main507cf78cf. Since rehearsed 5ee6d84: 86b415d deletes only the unused LiveFilesRequest import; 4bd9bce corrects the existing bridge fixture without weakening its assertions.Both planned reconciliation and post-carry validation now use
ExternalStore.practice_openings(files, *, scratch, stores). The copied-root carry gets scratch configuration from the original root throughFilesRequest.source_root(), not from an incomplete practice copy or the worker's temporary HOME.Source delivery only. No author tests, ratchet or live install. Root's rehearsal at 5ee6d84 completes the previously failing planned-reconcile in 8.291 s, but pending=[] means the post-carry probe was not exercised. Root's combined selection was 8 passed, 1 failed: the refusal fixture used a deliberately retired table. That fixture is now corrected to the commander's specified current-table refusal and retired-table acceptance cases, and root's exact-head rerun at 4bd9bce passes both cases in 6.00 s. Full current-head suite and installed acceptance are not claimed. P935 remains open and stopped.
Failure, including the first incomplete repair
The failed install copied:
My first commit supplied configured scratch to
ExternalStore.practice_openings, but missed the same operation inWriterOwnedOpeningProbe.run: another bare TemporaryDirectory and another selected-store practice loop. That would send the same database back into quota-limited tmpfs after reconciliation passed. The second commit deletes that duplicate rather than supplying it another independent directory policy.A second ownership correction matters during rehearsal:
PreparedMigration.rootis the copied root.practice.selectneed not copy storage.json, so loading StorageRule there could silently select default scratch. The final producer usesStorageRule.load(self.files.source_root()).scratch_base. LiveFilesRequest and RetainedCopyRequest already own those original-root answers.Before / after ownership
Before:
After:
The probe retains only resource-path reporting after shared validation. Its TemporaryDirectory import and seven-line directory/validation/result-building block are removed. File selection belongs to callers; directory lifetime and validation iteration belong to ExternalStore. The successor commit also deletes typed_table's now-unused LiveFilesRequest import: whole-tree search finds all remaining uses importing its actual locked_store owner, not this incidental re-export.
Complete caller and producer closure
Production
cli_verbs/planned_reconcile.py: derives the same family-minus-carry exclusion set as before, supplies LiveFilesRequest(root), selected stores and the actual root's StorageRule scratch.typed_table.py: requires files, scratch and stores; creates missing ancestors through existing PrivateDirectoryRole policy, owns the only TemporaryDirectory and selected-store practice loop. It does not import StorageRule.installer/probes/carry.py: WriterOwnedOpeningProbe carries requiredscratch: Annotated[Path, PathText]and delegates validation to that owner, passing its unchanged live/copied FilesRequest and stores. Returned resource paths retain store/resource order.installer/upgrade.py: the sole production probe constructor resolves configuration throughself.files.source_root(), preserving original configuration even when the retained copy lacks storage.json. It passes the value across the existing target-worker transport.Fixtures and wire
All five explicit probe constructors are migrated: one in upgrade.py, three in test_writer_owned_stores.py and one in test_reconcile_stores.py. Fixture constructors supply their owned scratch path. Existing live/copied request codec-roundtrip controls now include the required scratch field through their existing whole-record equality.
WriterOwnedOpeningProbe inherits TargetProbe (
interpreter_operation.py:465): only the reviewed target runtime decodes it;outgoing_wire()returns empty. This new target-only PathText field needs no old-runtime reader, compatibility alias or fallback.Unchanged: WriterOwnedStore.practice_opening's copying and schema install/readability checks; file resources; exclusion meaning; primary/committed WAL behavior; original durable files; exception propagation and context-manager cleanup. No process-wide tempfile setting, new environment variable, scratch constant, quota change or data deletion.
Existing bridge control corrected at its fixture
BridgeStateTable.retired_tablesat shared.py:130 deliberately retires observed_part.DeliveredBoardContentat shared.py:134 declares board_delivery(content, since: float, delivered_at: float). The refusal branch now creates that current table pre-STRICT, with since TEXT holdingnot-a-time. The accepted branch installs actual schema and adds the retired observed_part table and row. All refusal, original/copied path and byte-equality assertions remain unchanged; no fallback table, production change or weakened check. It also removes the old setup's needless create-wrong-store/unlink/recreate detour.Source evidence
The initial refactor-audit Package.load map at f701b1c parsed 1,168 tracked Python files under src/tests/tools, zero omissions. It retained 50 related modules. The expanded semantic-family map at the rejected 74d47e3 again parsed 1,168 files, zero omissions, retaining 39 related modules including both loops, the carry producer, FilesRequest and fixture consumers. Retained maps:
p938-source-before.json,p938-family-before.jsonunder/home/ts/.cache/agent-scratch/install-speed.The rebase brings in #3598's review-gate change, not another scratch implementation. Targeted applied-source reproof covers all six changed modules and records every probe constructor and practice call in
p938-family-after.json. Whole-tree search finds two production calls to the shared plural operation and one production call to the singular store operation, inside that owner's loop. No TemporaryDirectory remains in installer/probes/carry.py. This is static source evidence, not application acceptance; dynamic plugin behavior is not certified by AST.Root-run receipts, bounded to the head and branch actually exercised
At 5ee6d84, root ran the real native --rehearse path. I read
/home/ts/..agent-comms.installer-e45ab41e9c7d43179d9746af00415b05/result.json: target matches, result.kind=rehearsed, rehearsal.kind=uncopied, pending=[], rewritten=0, inputs=[], copied bytes=0. The exact--root /home/ts/.agent-comms planned-reconcile --carryinghas 8.291418 s caller elapsed and 7.066325 s nested body. They are nested, not additive. No copy/carry or WriterOwnedOpeningProbe ran. This proves the failed planning step now completes; it does not prove the unexercised carry branch or an installation.Root's combined control log
/home/ts/.cache/agent-scratch/codex-greentext/p938-carry-controls.logrecords 8 passed, 1 failed in 7.88 s. The former false refusal case created observed_part; BridgeStateTable.retired_tables intentionally retires it, so planned-reconcile correctly returned success. claude-code traced the declaration, codex-greentext ordered the replacement, and 4bd9bce implements it. Root reran only the corrected bridge test, both parameters, at committed 4bd9bce: 2 passed in 6.00 s, from/home/ts/.cache/agent-scratch/codex-greentext/p938-bridge-control.log, read in full. The malformed current table is refused; valid schema containing retired-table data is accepted, with existing path and byte assertions intact. No fallback was needed. The earlier eight passing results belong to 5ee; this focused pair qualifies the changed fixture. These counts overlap and are not summed.Root-owned remaining acceptance — no author-run receipts
1. Independent real planned-reconcile directory/cleanup check
With reviewed source and configured dependencies, Python's existing audit event observes real allocation without patching our code or overriding tempfile. The failed command had an empty --carrying list, matching this command's defaults.
2. Copied-store controls already run; actual live upgrade still owed
The original-root configuration selection in PreparedMigration.carry is source-reviewed, not claimed from the uncopied native rehearsal. It uses the existing FilesRequest.source_root for both request members; no copied storage.json or worker-HOME inference remains. The combined copied-store/WAL/worker checks and corrected bridge pair cover the real retained-file validation, returned paths, refusal and original-byte behavior on fixture-owned stores.
The already-run controls are listed for reproducibility, not a request for another reassurance run:
Root ran under its test lease using the configured interpreter and fixture-owned stores. No author run was made. Legitimate concurrent changes to live databases must not be mistaken for rehearsal writes. Root's next acceptance step is the actual native upgrade after source verdict and merge, as ordered; no additional local gate is introduced here.
Preserved work and assumptions
The parser correction is committed/pushed on #3596 at 300e2aa, with no stash/discard. That PR explicitly limits its older measurement/worker qualification to 98d8. P935 checker-site implementation remains incomplete and stopped; this repair does not complete it.
The same checkout was reused; no new worktree, venv or build. No author tests/app runs, local ratchets, user-data deletion or UNKNOWN-input replay. Open overlapping source PRs remain #3576 (planned reconciliation) and #3580/#3566/#3538 (typed_table); keep this shared operation and source-root selection if larger moves land.
Assumed: root reviews this exact new head, runs both real routes and retries the failed native upgrade. A passed copy is not an installed build; neither state is claimed here.