feat(rusqlite-runtime): expose exclusive maintenance checkpoint port - #540
Conversation
|
38f5725 to
6cb17aa
Compare
![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: 38f57254b3
ℹ️ 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".
6cb17aa to
c578cd5
Compare
c578cd5 to
d0694e7
Compare
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Reviewed as the runtime-core Database wire consumer. Not folding the wire into this PR. Not requesting merge.
Keep: exclusive drain that leaves the writer attached, pre-drain binding/authority checks on run_maintenance_checkpoint, typed Checkpoint(CheckpointControlError) instead of Writer(String), and CheckpointOutcome left unflattened. The pinned-reader test is real reclaim proof: Pending + busy + WAL bytes unchanged, then Complete + zero-length WAL. PASSIVE run_checkpoint staying untouched is correct.
Wire constraint (do not fix by editing runtime-core here): run_maintenance_checkpoint always closes admission and never reopens it. After the first exclusive Truncate, exact_sql_handle is WriterUnavailable until drain + close_and_join. That is right for an exclusive maintenance window. It is not an online periodic Database::checkpoint — the later wire must not call this on the live submit path unless a resume/reopen seam exists, or it will shut the writer for the rest of the process.
Also: begin_maintenance_drain still has no permit/authority check. Runtime-core will only call run_maintenance_checkpoint so the pre-drain validation actually runs. If begin_maintenance_drain stays public, a mis-ordered caller can close admission with no permit.
No flatten. No Database wire in this PR.
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Deep review of e69be29 as the Database wire consumer. Not folding the wire. Not requesting merge. Stay draft.
Keep: begin_maintenance_drain is pub(crate); run_maintenance_checkpoint checks permit binding and BeforeAdmission before any lifecycle change; Checkpoint(CheckpointControlError) stays typed; CheckpointOutcome is unflattened (Pending is not success). The pinned-reader test is still real reclaim proof. No-reopen after Complete/Pending is the exclusive-window contract — the later wire will not put an online Database::checkpoint on this without a resume seam.
Must-fix before I wire: snapshot blockers are still applied after drain. request.blockers() is already on the request and is_clear() is cheap. The rustdoc says draining is irreversible so misrouted requests must not tear the attachment down, but maintenance_checkpoint_preserves_typed_blocked_failure currently gets Blocked after begin_maintenance_drain_locked and never asserts admission stayed open / writer stayed Ready. Same class of bug as the old permit-before-drain hole. Check !request.blockers().is_clear() next to the permit/authority gates, and extend that test to prove no drain side effects.
Health-lane note for the later wire only (not asking for a change here): readers.begin_drain() leaves health admission open by design, so a health-lane snapshot can still pin WAL and produce typed Pending. That stays not-reclaimed.
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Re-checked 25f85d6. Blockers are request-local and now fail before begin_maintenance_drain_locked. maintenance_checkpoint_preserves_typed_blocked_failure asserts admission still open, writer Ready, and exact-SQL still admits. Same shape as the foreign-permit / denied-authority gates. This hole is closed.
Still keep: pub(crate) drain, no-reopen after Complete/Pending, typed Checkpoint(CheckpointControlError), unflattened Pending. Database wire still waits until this is actually green. Not merging. Not changing bases.
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
begin_maintenance_drain is only for run_maintenance_checkpoint and crate tests. A public unchecked drain would close admission with no permit. Admission stays closed after Truncate; Database will not call this on the live submit path.
A stale snapshot inventory is request-local. Checking blockers after begin_maintenance_drain closed admission and left the writer Draining, so Blocked tore down exact-SQL the same way a foreign permit used to. Leave admission open and the writer Ready.
fadf07d to
9f726be
Compare
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Cross-review of draft 9f726be as the Database wire consumer. Comments only. Not folding the wire. Not retriggering CI. Not flattening.
Crate seam — not blocking this PR: begin_maintenance_drain is pub(crate); run_maintenance_checkpoint checks permit, BeforeAdmission, and request.blockers().is_clear() before drain; Blocked / foreign permit / denied authority leave admission open and the writer Ready; Checkpoint(CheckpointControlError) stays typed; Pending is not success; no reopen after Complete/Pending. Isolated attachment + writer checkpoint tests are the crate proof. Compare is still rusqlite attachment.rs + permit() on writer.rs.
Blocking ready (not a new code hole): still draft, and GitHub CI is not actually green (Format / Clippy / Hawk / Release Version Drift failed; Linux / macOS / Windows still running). Coordinator lock: cancelled/skipped is not a crate fail and I am not retriggering. Isolated tests still count for this crate. The Database wire stays a separate runtime-core PR after this is actually green.
Leftover after this PR (do not fold in): exclusive-window no-reopen means online Database::checkpoint cannot sit on this port without a resume seam; health-lane snapshots can still produce typed Pending (not reclaim). Ready watch is empty until Zack names one.
begin_maintenance_drain stays pub(crate); lib clippy cannot see the #[cfg(test)] callers and was failing -D dead-code.
…acement tests Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…binary Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…rm fixture helper Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aec392e589
ℹ️ 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".
| let ticket = checkpoint | ||
| .trigger_maintenance_authorized(request, authority) | ||
| .map_err(RepositoryDispatchError::Checkpoint)?; |
There was a problem hiding this comment.
When authority is revoked between the two checks, the preflight succeeds and begin_maintenance_drain_locked irreversibly closes admission, but this call enters CheckpointHandle::enqueue and repeats the BeforeAdmission verification, returning AuthorityDenied with no transition back to Ready. The existing RevokeAfterAdmissionAuthority models this valid-then-revoked behavior; this duplicate post-drain verification is fresh evidence beyond the resolved always-denied comment. Avoid the second admission check or make verification, draining, and enqueueing atomic.
AGENTS.md reference: AGENTS.md:L108-L110
Useful? React with 👍 / 👎.
| // Arming is an observable state change: waiters on | ||
| // `capacity_changed` (tests watching for a blocked acquisition) | ||
| // must see it as an event rather than having to poll `snapshot`. | ||
| self.inner.capacity_changed.notify_all(); |
There was a problem hiding this comment.
When many reads queue behind a saturated pool, every additional waiter now broadcasts to all existing waiters even though no capacity became available. Those threads contend for the state lock and re-run the acquisition loop, producing quadratic wake/lock traffic precisely during overload; the comment confirms this production behavior exists only for a #[cfg(test)] observer. Use a separate test-only hook or observation channel rather than changing the production capacity signal.
AGENTS.md reference: AGENTS.md:L75-L77
Useful? React with 👍 / 👎.
| entered.wait(Duration::from_secs(30)); | ||
| cancellation.store(1, Ordering::SeqCst); |
There was a problem hiding this comment.
If a regression prevents the executor from ever starting, Gate::wait returns false after 30 seconds but this thread ignores that result and still cancels the request. The test can then observe Cancelled with finished == 0 without proving that cancellation interrupted an executing query, making the scenario nondeterministically vacuous; assert the gate result before issuing cancellation.
AGENTS.md reference: AGENTS.md:L108-L110
Useful? React with 👍 / 👎.
2 participants
Summary
This is the daemon-lane WAL Truncate seam.
RepositoryRuntimePhysicalAttachmentpreviously forwarded only PASSIVE checkpoints (run_checkpointviatrigger_authorized), while the exclusive RESTART/TRUNCATE path (CheckpointHandle::trigger_maintenance_authorized) requiresWriterState::Draining— and the only attachment-level drain,drain(), takes and joins the writer, leaving nothing to checkpoint through. Runtime-core could not wire crate-only.Changes in
crates/tracedecay-rusqlite-runtimeonly:RepositoryRuntimePhysicalAttachment::begin_maintenance_drain()— closes admission and moves the retained writerReady → Draining(PersistentWriter::begin_drain) plus a reader-pool maintenance drain, without taking or joining the writer. Idempotent;Closedwhen closed or writer-less.RepositoryRuntimePhysicalAttachment::run_maintenance_checkpoint(MaintenanceCheckpointRequest, Arc<dyn RuntimeWriteAuthority>)— does not requireadmission_open(maintenance runs after admission closes) and performs the maintenance drain first if the writer is stillReady. Forwards throughtrigger_maintenance_authorizedand awaits the ticket.RuntimeWriteAuthorityStage::BeforeAdmission, same stageCheckpointHandleverifies) are checked before any lifecycle transition. A foreign-shard permit or revoked authority returns a typed error and leaves admission open and the writerReady— draining is irreversible, so misrouted requests must not tear down the attachment.MaintenanceCheckpointRequestgained apermit()accessor for this.RepositoryDispatchError::Checkpoint(CheckpointControlError)preservesBlocked/Busy/BindingMismatch/AuthorityDenied/Unavailablefrom trigger and wait instead of collapsing them intoWriter(String);Display/sourcecarry the typed error.RepositoryDispatchErroris only referenced inside this crate, so the new variant breaks no downstream matches.Busy/incomplete is typed, not success (Plan 38 §6 measured reclaim): the port returns the writer's
CheckpointOutcomeverbatim —Completeis only produced when the frame report is complete (checkpointed_frames == log_framesand not busy); a pinned/busy TRUNCATE surfaces asCheckpointOutcome::Pendingwith the busy frame report. No second checkpoint engine, no outcome flattening.Unchanged: PASSIVE
run_checkpoint,drain()(still joins the writer),close_and_join().Database::checkpointTRUNCATE is deliberately not implemented here — runtime-core wires that after this lands.Tests (isolated, this crate)
cargo test -p tracedecay-rusqlite-runtime --lib -- repository::attachment::tests writer::tests::checkpoint— 15 passed, 0 failed, on the restacked base 8fa116b (#539 merge). Clippy and fmt clean on the crate.maintenance_port_reports_closed_without_a_writer_or_after_close— read-only attachment and post-close attachment both yieldRepositoryDispatchError::Closedfrom both new methods.maintenance_drain_keeps_writer_attached_and_closes_admission— afterbegin_maintenance_drain(called twice for idempotency) the writer is still attached inDraining, exact-SQL admission is closed, an exclusive RESTART through the port returnsComplete, and the existingdrain()+close_and_join()lifecycle still succeeds afterwards.maintenance_checkpoint_rejects_foreign_permit_before_draining— a wrong-shard permit returns typedCheckpoint(BindingMismatch); admission stays open, writer staysReady, exact-SQL still admits, lifecycle intact.maintenance_checkpoint_denied_authority_never_drains— denied admission-stage authority returns typedCheckpoint(AuthorityDenied { BeforeAdmission })with no drain side effects.maintenance_checkpoint_preserves_typed_blocked_failure— non-clear snapshot blockers surface as typedCheckpoint(Blocked(blockers))through trigger/wait, matched on the enum, not string parsing.maintenance_truncate_is_typed_pending_while_pinned_then_reclaims_wal— measured reclaim: an independent connection pins a read snapshot at the empty WAL mark, eight committed frames pile up, and TRUNCATE through the port returns typedPending { kind: Truncate }withreport.busyandcheckpointed_frames < log_frameswhile the WAL file length is unchanged. After the snapshot releases, the retried TRUNCATE returnsCompletewith a not-busy report,checkpointed_frames == log_frames, and a zero-byte WAL file.Tests use the
#[cfg(test)]ExclusiveMaintenancePermit::issuewith the attachment's own binding and run against temp-dir SQLite databases only — no live~/.tracedecayor repo-local.tracedecay/involvement.