Implement cleanup and one retry on reserve exhaustion - #174
Conversation
|
@codex review Please independently review exact head
Please identify any correctness, race, API, portability, testing, or acceptance-criteria blocker on this exact SHA. 🤖 Posted by Codex |
![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: ea41eb67c2
ℹ️ 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 let AllocationResult::Allocated(port) = | ||
| allocate_with_fallback(allocation_options, &allocate)? | ||
| { | ||
| persist_deferred_reservation(conn, key, project, task, created_at, port)?; |
There was a problem hiding this comment.
When callers use the public ReservePlan/PlanExecutor pattern on an autocommit connection, another writer can create the same key after planning but before this serialized recheck. If that writer also leaves another port free, this branch selects the free port and persist_deferred_reservation unconditionally upserts the key, silently moving the concurrent reservation and replacing its metadata without --overwrite, sticky-field authorization, or an idempotent refresh. Re-read and reconcile the key inside this transaction before allocation/persistence so a matching concurrent reservation keeps its port and metadata.
AGENTS.md reference: AGENTS.md:L13-L17
Useful? React with 👍 / 👎.
|
|
||
| /// Recheck an exhausted allocation, run the enabled automatic-cleanup | ||
| /// phases, retry once, and persist the resulting reservation atomically. | ||
| ReserveAfterCleanup { |
There was a problem hiding this comment.
PlanAction is a public, crate-root-reexported enum and is not marked non_exhaustive, so adding this variant makes every downstream exhaustive match fail to compile. This is especially problematic because the crate remains version 0.2.0; represent the deferred reserve operation without expanding the existing exhaustive public enum, or make the API break explicit through the project's compatibility/versioning process.
Useful? React with 👍 / 👎.
| /// Non-destructive explanation of cleanup policy and remaining blockers. | ||
| details: String, |
There was a problem hiding this comment.
The new exhaustion diagnostic stores cleanup policy and all remaining blocker information in a free-form String, so library consumers cannot distinguish disabled phases, reserved ports, exclusions, occupancy, or cleanup counts without parsing display prose. That fails the typed-blocker acceptance contract and makes wording changes an accidental API; expose structured enums/fields for the blocker and cleanup outcome, then derive the human-readable message from them.
Useful? React with 👍 / 👎.
|
@codex review Please independently re-review exact head 🤖 Posted by Codex |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9633c5479
ℹ️ 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 Self::with_savepoint(conn, savepoint_name, operation); | ||
| } | ||
|
|
||
| let transaction = Transaction::new_unchecked(conn, TransactionBehavior::Immediate)?; |
There was a problem hiding this comment.
When a library caller executes an exhausted ReservePlan through PlanExecutor on an autocommit Database::connection() while another writer holds the database, this newly introduced BEGIN IMMEDIATE can return SQLITE_BUSY/SQLITE_LOCKED after the configured timeout. The direct ? converts that into Error::Database, unlike Database::begin_transaction and other public database boundaries, so callers lose the documented typed Error::LockTimeout and its timeout/operation context; classify lock errors when starting (and committing) this transaction.
Useful? React with 👍 / 👎.
|
@codex review Please independently re-review exact head 🤖 Posted by Codex |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e246d0d70
ℹ️ 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".
| project: placeholder.project().map(ToOwned::to_owned), | ||
| task: placeholder.task().map(ToOwned::to_owned), |
There was a problem hiding this comment.
When an overwrite/force plan is deferred after exhaustion and another connection deletes the target key before the public executor runs, this branch copies metadata that was resolved from the now-deleted reservation. For omitted fields, MetadataIntent::Preserve should instead apply new-reservation inference, but execution resurrects the deleted project/task values while treating the row as new by resetting created_at. Resolve these fields from the stored intents with resolve_new when the planned update no longer has an existing row.
AGENTS.md reference: AGENTS.md:L13-L17
Useful? React with 👍 / 👎.
|
@codex review Please independently re-review exact head 🤖 Posted by Codex |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
IMMEDIATEtransaction or caller-owned savepointPlanActionandError::PortExhaustedvariant shapes while exposing deferred-plan state and typed exhaustion details through additive methods--force, and report only aggregate cleanup outcomesStack
65878a57b36da119810e2523748a6d7d361cbe43(origin/main)Root cause
ReservePlanreturnedPortExhausted { tried_cleanup: false }immediately after its preferred/fallback allocation scan. The disable options reached the library but no reserve path consumed them, so configured automatic cleanup could never free a stranded port.Acceptance mapping
LockTimeoutdata for transaction start, execution, savepoint, and commit boundariesPlanActionvariants and exact destructuring ofError::PortExhausted { range, tried_cleanup }remain validRed evidence
Before implementation:
port range 51381-51381 exhaustedport range 51389-51389 exhaustedReview corrections
Exact-head review of
ea41eb67c214d1cd3fbe800c57e82ee3089444beidentified three P1 issues. Commitb9633c547997e950ef4458c5361694fe229a7869addresses all three:PortExhaustedretains its exact two-field public shape and exposes structured cleanup/blocker data throughError::port_exhaustion_details()Exact-head re-review of
b9633c547997e950ef4458c5361694fe229a7869identified one P2 issue. Commit0e246d0d7022c96846b2643602cb630823f4b561addresses it:ReservePlan/PlanExecutorregression forces zero-timeout writer contention and asserts the typed timeout plus reserve-specific operation contextExact-head re-review of
0e246d0d7022c96846b2643602cb630823f4b561identified one P2 issue. Commit975307c90bce7bd5ff0b70429252558d7c902a5faddresses it:ReservePlan/PlanExecutorregression covers the disappearance race and verifies omitted project/task metadata is newly inferredValidation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo check --workspace --all-targets --all-features --lockedcargo test --workspace --all-targets --all-features --lockedcargo test -p trop-cli --test reserve_command --all-features --lockedcargo test -p trop --test planning --all-features --lockedcargo test -p trop --test concurrent_operations test_automatic_cleanup_retry_serializes_competing_reserve_processes --all-features --lockedcargo test -p trop --all-features operations::reserve --lockedcargo build --release --workspace --all-targets --all-features --lockedRUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps --lockedjust test-production-readiness-selectoragentic-navigation-guide checkagentic-navigation-guide verifyjust preflight-prResidual risks
None.
Hosted CI passed on run 30307335347, and independent exact-head re-review of
975307c90bce7bd5ff0b70429252558d7c902a5freported no major issues.Closes #101