Repository navigation
perf(test): shorten the protected git reconcile wait in its postgres test - #3391
Conversation
…L test bindRepositoryGitTransport reconciled a protected Git Driver on a fixed 1 s timer. postgres-external-protected-git waits for whole reconcile passes (three "+2 passes" waits and the late-settlement waits in the lost-rotation case), so it spent about 10 s waiting out that timer. The wait between passes becomes a construction option. The worker passes no options, so production keeps the 1 s cadence; no configuration or environment value reaches it. The pending-log option moves into the same options object. The test binds the same loop with a 100 ms wait; its pass-count assertions are unchanged.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs changes before merge. What this changesThis PR lets the protected Git integration test shorten reconciliation waits from 1,000 ms to 100 ms while preserving the worker default. Example: The lost-rotation test waits for two additional reconciliation passes.
Review scores
ProductKind: Performance · Worth it: Yes · Fix scope: Complete Merge readiness⛔ Needs changes before merge - 1 item remains Keep this PR open: the test speedup has value, but its new test-only production timing hook conflicts with the repository’s required test-audit guidance. Priority: P3 Before merge
Findings
Agent review detailsHow this fits togetherWorker composition binds the selected protected Git Driver to the prepared credential owner and repeatedly invokes its reconciliation operation until shutdown. flowchart LR
Worker[Controller worker] --> Binding[Transport binding]
Test[PostgreSQL integration test] --> Binding
Binding --> Driver[Selected protected Git Driver]
Driver --> Owner[Prepared credential owner]
Owner --> Database[PostgreSQL]
Technical reviewBest possible solution: Keep the production binding’s scheduling contract fixed and accelerate the integration scenario through test-owned timer control while retaining its real reconciliation and no-replay assertions. Do we have a high-confidence way to reproduce the issue? This is a test-performance change rather than a reported product failure; source confirms the fixed waits, and the PR reports a local before-and-after timing comparison. Is this the best way to solve the issue? The scheduling default and consequential assertions are preserved, but moving timing control into the test harness better follows the required test-audit policy. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against a3b7d0111095. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceAcceptance criteria:
What I checked:
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 23, 2026, 10:39 PM ET / October 24, 2026, 02:39 UTC. |
|
On the P2 finding (test-only timing option on the production binding): this design was chosen on purpose, and the alternatives were worse.
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
Problem
bindRepositoryGitTransportreconciles a protected Git Driver on a fixed 1 s timer.postgres-external-protected-gitwaits for whole reconcile passes: the lost-rotation case has three "two more passes" waits plus waits for late settlement, so it spends about 10 s waiting out the timer.Change
reconcileIntervalMs) ofbindRepositoryGitTransport. The default stays 1 s. The worker passes no options, so production behaviour is unchanged, and no configuration or environment value reaches the option.pendingLogargument moves into the same options object (common-logging.test.mjsupdated).postgres-external-protected-gitbinds the same loop with a 100 ms wait. Its pass-count assertions ("+2 passes", no installation attempt, no replay) are unchanged. With a shorter wait they observe more passes in the same steps.Verification
postgres-external-protected-gitThe case also waits for a controller-work retry with random jitter (
postgres-work-queue.ts), so single CI samples of it vary by several seconds; the same-host pairs isolate the timer.Timed out waiting for worker installs first material.).postgres-external-protected-git20/20 (66.9-69.7 s);common-logging20/20.tsc --build tsconfig.json tests/tsconfig.json, eslint and prettier are clean on the touched files.Risks
composition/repository-credentials/transport.ts). The production path keeps its 1 s cadence; only a test passes the option.