🤖 refactor: persist cancellation ownership under history locks - #4146
Conversation
This comment has been minimized.
This comment has been minimized.
|
@codex review Please review this inactive real-storage adapter separately from its H1 core parent. Focus on exact nonce/frontier authority, receipt visibility at publication, malformed repair privacy floors, and the injected accepted-replacement witness requirement. Final integration passed 164 tests and full static checks; independent review approved. Runtime activation remains a later layer. Generated with |
This comment has been minimized.
This comment has been minimized.
![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: 5e5bf60bf3
ℹ️ 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".
5e5bf60 to
0ffba8a
Compare
|
Addressed finding 3959558175: the held history lease is now rechecked at actual cancellation, generation, repair, provenance, and cleanup mutations. Eleven deterministic displaced-token regressions reproduce the original failure and now reject the stale writer without issuing a receipt. Integrated H1's committed-record receipt contract: publish, narrow, exact confirmation, and retire report synchronously before cleanup or lock release. Four real-filesystem pause regressions verify exact receipt contents and timing, including inherited retention. The final combined tree passes 221 affected tests and full static checks; independent review approved both changes. This remains the inactive adapter and fixed top of phase #4140 → #4146. No runtime activation was added. Both phase members will be queued together once ready. @codex review Generated with |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ffba8a2f3
ℹ️ 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".
0ffba8a to
07250d1
Compare
|
Addressed all four findings:
All 242 affected tests (including 71 core cases) and full static checks pass after integrating #4140. Independent review approved. This round adds 61 changed production lines, using existing helpers and lock boundaries. No activation or new parser/storage framework was added. @codex review Generated with |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07250d1f40
ℹ️ 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".
Implement exact-nonce storage under the existing history locks, preserving retry frontiers before failing awaits. Repair malformed cancellation only after fencing and neutralizing authoritative recovery intent while preserving privacy floors. Keep witness authority injected and runtime consumers inactive. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: Ib4f43dab3679f4103f90b81737311a50e8e739e3
Exclude Promise-returning repair and generation observers so local receipt state is visible at the durable publication boundary. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: I3a295533c5ae76bd9209032753833b908955760f
Propagate the lock ownership guard through cancellation and recovery writes, including provenance publication and cleanup, so a displaced lease cannot publish after takeover. Preserve synchronous commit receipts and unchanged callers without optional guards. Eleven displaced-lease regressions fail against original production and pass with the guard. All 205 affected tests and full static checks pass. Correct the existing atomic-rename test fixture to compare canonical paths on macOS. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: If930b019e6c3fb231ed3635f6a5223bbf73d3e7f
Implement the parent core's synchronous receipt contract at publication, narrowing, exact confirmation, and retirement. Report the exact inherited record or null before cleanup or lock release while retaining all lease-ownership guards. Four real-filesystem pause regressions fail on the unadapted adapter and pass with the receipt. All 221 affected tests and full static checks pass; independent review approved. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: I3151aceff7c20bf5adca0bfb88f9026b37fa4adc
Reject duplicate keys, refuse unsupported and oversized records, canonicalize serialized receipts, and sync the directory before successful mutation settlement. Checkpoint for integration with the reviewed cancellation core. Final combined validation follows parent integration. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: I08c7db03e1575f230cb6241b416543c32992be56
Keep the receipt expectation within the cancellation record union after parent integration. All 242 affected tests and full static checks pass. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: Icb096881c55a7688e95426dd557a1d30ccf8688c
Read at most the existing size limit plus one byte through a single descriptor, handle short reads, and always close it. Preserve oversized data and existing error classification. All 57 storage tests and independent review pass. Final combined validation follows parent integration. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: Ib8cdc5fefa7733283b41767ebf9360130026007e
07250d1 to
603677f
Compare
|
Addressed 3960866846: cancellation reads allocate one buffer capped at the existing limit plus one byte and read through a single descriptor. Legal short reads continue until EOF or the cap; the descriptor closes on every path. Oversized bytes remain untouched, and missing files, ordinary I/O errors, malformed records, and explicit refusal remain distinct. Four real-filesystem regressions fail against the previous whole-file reader and pass with the fix, checking buffer/read bounds, short reads, descriptor closure, and preserved bytes. All 252 combined tests (77 core and 57 storage cases included) and full static checks pass after parent integration. Independent review approved. The adapter change is 15 production lines with no new helper framework or interface changes. @codex review Generated with |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This comment has been minimized.
This comment has been minimized.
…der#4148) Deleting current provider rows or their boundary can leave a foreign compactor's captured generation valid, allowing stale initial, fallback, or boundary publication after cleanup. This change fences the removed occurrences before rewriting history, using the provider scanner's raw-aware suffix rules. It covers single deletion, batch deletion, archive fallback, and the private deletion used by commitPartial. Deleting a readable separator can join malformed fragments into new reset evidence; the shared scanner probe detects that change while preserving the raw bytes. Readable user/system reset floors participate in deletion classification without changing provider reads. Oversized reset evidence stays raw, and protected active identities prevent deletion or truncation from falling back to an archived duplicate. Exact-limit valid rows remain addressable because the size check excludes the trailing LF. Retained boundaries and separators, existing reset evidence, incomplete tokens, and ineffective cuts preserve generation. A later history-write failure retains the conservative fence. Protected parsed rows retain identity and sequence metadata separately from transformable rows. Partial rewrites and rename keep their cached counter above retained protected sequences. Seven counter regressions were reproduced; actual sequence reuse was not reproduced because ordinary appends already refresh durable history. Reasoning-only deletions inherit the conservative provider-context classification from merged coder#4135. The pending-state prerequisite coder#4138 has also merged; this PR is the remaining deletion layer. Runtime activation is separate. Validation: 518 affected tests and full static checks pass on the main-integrated candidate. Coverage includes raw-fragment joining, malformed-role reset floors, protected active/archive duplicate IDs, exact-limit and oversized rows, sequence accounting, real provider history, and stale initial/fallback/boundary publication. The exact-rollback fixture restores captured history bytes so its unchanged-generation assertions remain meaningful after generic deletion gains fencing. Edit/fork truncation now refuses a protected active target before archive fallback when no readable active match exists. Four regressions reproduced destructive fallback to an archived duplicate; twelve cases cover oversized and ambiguous reset rows in both keep/remove modes. Refusal preserves both files byte-for-byte, generation, and the publication journal. Readable active duplicates retain precedence, while unrelated protected IDs still allow legitimate archive truncation. The correction reuses existing identity metadata and the single-deletion guard; independent review approved it. The queued-base CI failure was isolated to four fault-injection cases after coder#4146 changed generation publication to write a staging sibling and then rename it. The fixture now matches that exact generation artifact's staging prefix, keeps history-stage matching exact, and asserts that injection fired. All four regressions, all eight write-failure controls, and the 349-test history suite pass on the previously failing merge-group base. Production behavior is unchanged by this integration correction. Nix formatting was skipped because Nix is unavailable. Linux E2E also now runs with one worker because all Electron workers share one Xvfb display and clipboard. On the exact failing queue candidate, native probes reproduced three clipboard denials in four competing-window runs: two lost focus before validation, and one during the awaited isolated-world activation check. Focusing the parent before clicking a self-closing auth popup reproduced the same closed-target error on attempt nine; authentication completed, the remote stayed connected, and the local renderer and projects remained intact. Security checks and all original tests remain unchanged. The unmodified remote-connection file passes all three tests with one Linux worker (34.9 seconds); 30 focused popup controls also pass. Reproduction used Linux arm64 with pinned Electron 40.9.3, Playwright 1.57.0, and Bun 1.3.5; the failing CI runner was x64. Full static checks and both workflow linters pass for the final CI-only correction. The worker-count repair was verified at b82705b by the full Linux x64 CI suite: 55 tests passed and 21 skipped using one worker, including all remote-connection tests. Measured test execution took 8.5 minutes; the complete job took 9m56s including setup and build ([CI result](https://github.com/coder/xum/actions/runs/34279578520/job/102240858941)). Serial execution trades some throughput for stable native focus without reducing coverage. Regression risk is in classifying which removed occurrences affect provider context while preserving malformed privacy floors and occupied sequences. Tests cover retained-floor controls, duplicate IDs, restart, and generation/history-write failure ordering using real HistoryService storage. --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable -->
1 participant
Add the filesystem adapter for #4140's inactive cancellation core. Compare exact nonces and generation frontiers under local and shared history locks. Recheck lease ownership before every mutation so a displaced writer cannot publish, remove state, or repair history after takeover.
The adapter reports the exact JSON-normalized committed record, including inherited retention, synchronously before cleanup or lock release. Publication, narrowing, idempotent confirmation, and retirement all implement the parent's receipt contract. Directory sync completes before successful storage settlement; failure retains the visible receipt and persistence debt for retry. A later cleanup failure cannot undo that receipt or overwrite a newer accepted read.
Malformed cancellation repair fences publication and neutralizes authoritative recovery intent while preserving raw privacy floors. Duplicate-key detection reuses the existing scanner. Reads allocate and consume at most the existing size limit plus one byte through a descriptor that always closes. Future-version and oversized records explicitly refuse repair and replacement, preserving their bytes. Ordinary I/O failures remain visible. Retained cancellation can be retired only through an injected verifier for a durably accepted replacement; the adapter has no default witness authority.
This is the fixed top of phase stack #4140 → #4146. The adapter remains inactive, and both PRs will be queued together after current Codex approval, resolved findings, and passing CI.
Validation: 252 affected tests and full static checks pass. Eleven deterministic displaced-lease cases and four paused cleanup/lock-release cases fail against the respective unpatched implementations and pass with the fixes. Real HistoryService cases also cover short reads and bounded allocation, duplicate fields, future-version/oversize preservation, canonical receipts, and directory-sync failure/retry; all 77 parent core cases are included. Independent review approved the combined fixes. Nix formatting was skipped because Nix is unavailable.
Generated with
xum• Model:unavailable• Thinking:unavailable• Cost:$unavailable