Repository navigation
W3-01b: drain sessions before database maintenance - #335
Conversation
Freeze new PTY session starts, wait a bounded time for owned sessions to finish, then reserve the database through the W3-01a store APIs. A session that outlives the wait refuses maintenance by name and changes nothing. Updates refuse while maintenance is active; leave restores launches/writes. Test-First: src-tauri/src/main.rs::tests::maintenance_is_refused_while_a_session_is_active Test-First: src-tauri/src/main.rs::tests::drained_maintenance_freezes_launches_and_writes_until_leave Test-First: src-tauri/src/main.rs::tests::leaving_maintenance_restores_launches_and_writes Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 56cd6d11b2
ℹ️ 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 self.maintenance_active.load(Ordering::Acquire) { | ||
| return Err("Database maintenance is active; leave it before starting sessions".into()); |
There was a problem hiding this comment.
Wenn während der bis zu zehnsekündigen Drain-Phase noch Queue-Kapazität frei ist und der 30-Sekunden-Poller eine bereite Aufgabe startet, erreicht LiveLauncher über workers::create_worker diese neue Ablehnung. dispatch_project behandelt jedoch jeden Launcher-Fehler in queue.rs:465-467 als terminal und setzt den Eintrag auf failed; dadurch kann bereits eine später abgelehnte oder auslaufende Wartungsanfrage eine wartende Aufgabe dauerhaft verlieren. Die Wartung muss den Dispatcher vor dem Claim pausieren oder diese Ablehnung als transient behandeln und den Eintrag wieder auf ready setzen.
Useful? React with 👍 / 👎.
| if self.maintenance_active.load(Ordering::Acquire) { | ||
| return Err("Database maintenance is active; leave it before starting sessions".into()); |
There was a problem hiding this comment.
Wenn ein Start während der Drain-Phase über POST /api/workers hier abgelehnt wird, erkennt api::core_status in api.rs:2228-2235 nur das gemeinsame Präfix refused: und stuft diesen neuen freien Fehlertext deshalb als HTTP 500 statt 409 ein; auch ipc.ts::describeError normalisiert nur dieses Präfix. Verwende für diese und die übrigen neu eingeführten Wartungsablehnungen crate::errors::ERR_REFUSED, wie es Architekturregel 5 vorgibt.
AGENTS.md reference: AGENTS.md:L298-L301
Useful? React with 👍 / 👎.
| if live.is_empty() { | ||
| break; |
There was a problem hiding this comment.
Wenn create_worker bereits den Worktree und die running-Workerzeile angelegt hat, aber noch nicht agents.spawn_bound in workers.rs:583-590 erreicht hat, ist die PTY-Registry hier leer und die Wartung kann erfolgreich beginnen. Der anschließende Reserve-Versuch wird abgewiesen; bleibt die Wartung länger als den fünfsekündigen SQLite-Timeout aktiv, schlägt auch das ignorierte delete_worker in workers.rs:599-601 fehl, während der Worktree trotzdem entfernt wird, sodass dauerhaft eine laufende Workerzeile ohne Worktree zurückbleibt und ein währenddessen erzeugtes Backup diesen Phantom-Worker enthält. Erfasse Starts daher vor ihren Worktree-/DB-Nebenwirkungen in der Drain-Registry oder reserviere sie bereits davor.
Useful? React with 👍 / 👎.
|
Queued — the merge queue status continues in this comment ↓. |
Test-First: src-tauri/src/main.rs::tests::maintenance_refusal_uses_shared_error_vocabulary
Test-First: src-tauri/src/main.rs::tests::maintenance_refusal_uses_shared_error_vocabulary
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 12 minutes 37 seconds in the queue, including 12 minutes 21 seconds running CI. Required conditions to merge
|
Dieses Paket sorgt dafür, dass die Datenbank-Wartung erst beginnt, wenn keine Sitzung mehr läuft. Läuft nach zehn Sekunden noch eine Sitzung, wird die Wartung mit Nennung der Sitzung abgelehnt und nichts ändert sich. Nach erfolgreichem Start sind neue Starts und Schreibzugriffe gesperrt, bis die Wartung beendet wird; Updates können das nicht umgehen.
Report
What changed (173 added lines,
main.rs+pty.rs; no store/api/bin changes)pty.rs:PtyManager::begin_maintenance/end_maintenancelatch.reserve_session(the single launch entry) andinstall_when_idlerefuse while it is set; serialized through the registry lock.main.rs:enter_database_maintenancefreezes launches, polls owned sessions for a bounded wait (10 s), refuses by naming the live sessions (latch released, store untouched), then calls the W3-01aStore::enter_maintenance;leave_database_maintenancerestores writes, then launches. Tauri commandsenter_maintenance/leave_maintenance.main.rs): refused while a session is active; drained maintenance freezes launches, updates and writes until leave; leave restores everything; maintenance refusals use the sharedERR_REFUSEDvocabulary.Evidence
origin/main/ merge basef48af09e48d26ec8ad27dcaa4bc21870e046a7d3; head900d25397aed59b23cdfea77defa8fadd5ba4225.cargo test --manifest-path src-tauri/Cargo.toml tests::maintenance_refusal_uses_shared_error_vocabulary -- --exact --nocapturebefore the fix → exit 1, exact assertion failed onMaintenance refused: ....bash scripts/ci/red-first.sh --plan→ exit 0,count=5,specs=5.bash scripts/ci/gates.sh lane prepush→ exit 0 (12/12 gates green; 1790 Rust tests passed, 17 skipped).lane prepushat head900d253→ exit 0 (12/12 gates green; 1790 Rust tests passed, 17 skipped).git ls-remote origin refs/heads/claude/w3-01b-drain→ exit 0 and exact remote head900d25397aed59b23cdfea77defa8fadd5ba4225.Architecture drift: one medium finding was fixed. The timeout refusal at
src-tauri/src/main.rs:1135used a newMaintenance refusedwording instead of the canonical prefix atsrc-tauri/src/errors.rs:13; commitseded577(red test) and900d253(fix) now use the existingERR_REFUSEDpattern. No second state, lock, IPC-result, or Rust-to-frontend event pattern was introduced.NICHT ABGEDECKT
#[cfg(unix)]tests or Linux clippy arms (KNOWN_ISSUES KI-7); the Linux half is covered by the merge-queue/CI lane, not this local run.Store::enter_maintenanceare impossible (latch set first), but a long-lived running session simply refuses maintenance; no forced kill by design.Review disposition
Candidate:
900d25397aed59b23cdfea77defa8fadd5ba4225. Author family: Claude.src-tauri/src/main.rs:1135: new refusal wording drifted fromsrc-tauri/src/errors.rs:13anddocs/architecture-rules.md:31-34. Accepted: red test ineded577; production fix in900d253.glm-5.2:cloud, reportedglm-5.2— initial F1, low,src-tauri/src/main.rs:1144: requested a test for store-enter failure cleanup. Rejected after countercheck: no demonstrated defect; the branch explicitly releases the PTY latch, and the package's red-first tests cover the observable refusal contract.src-tauri/src/main.rs:1152: requested a test for store-leave failure. Rejected after countercheck: keeping the launch freeze on store failure is the documented fail-safe behavior; store transition failures are already covered insrc-tauri/src/store/maintenance_tests.rs:6.src-tauri/src/pty.rs:805: requested a dedicated installation-started guard test. Rejected after countercheck: no incorrect behavior or reachable bypass was shown;begin_maintenanceandinstall_when_idleshare the registry lock.src-tauri/src/main.rs:1121: questioned 50 ms polling. Rejected after countercheck: the loop is bounded, the 50 ms interval is intentional, and no correctness or resource failure was demonstrated.glm-5.2:cloud, reportedglm-5.2— final F1, medium,src-tauri/src/main.rs:1115: alleged that concurrent leave during drain can clear the PTY latch. Rejected after countercheck:Store::leave_maintenancereturnsMaintenanceError::NotActiveatsrc-tauri/src/store.rs:1038, sopty.end_maintenance()is not reached; if enter wins the store mutex first, enter then leave is a valid serialized order.src-tauri/src/main.rs:1144: repeated the store-enter cleanup test request. Rejected: same evidence as initial F1; no fault survived countercheck.src-tauri/src/main.rs:1152: requested leave-without-enter coverage. Rejected:src-tauri/src/store/maintenance_tests.rs:33already provesNotActive, and the?inleave_database_maintenancepreserves the PTY latch on that error.src-tauri/src/pty.rs:805: suggested rejecting maintenance while emergency stop is active. Rejected: the two process-local safety latches are independent by design; emergency stop still blocks launches, and no unsafe transition was demonstrated.No high-severity finding remains open. No second review round was required for a high finding; the final full-candidate GLM run nevertheless revalidated the changed candidate and returned
approve with conditions, whose only medium condition was disproved against the real Store implementation.