Repository navigation
test(addie): stabilize PostgreSQL ledger race barriers - #7514
Conversation
1673bdd to
59bac50
Compare
59bac50 to
9cbaa2c
Compare
9cbaa2c to
ef610b1
Compare
ef610b1 to
046f5ab
Compare
|
Triage execution: this is the first PR priority because it removes the recurring nondeterministic PostgreSQL ledger-test failure that blocks multiple exact-credential security PRs. CI is green and the human maintainer review request is already present. Keep Draft until that human review is completed; then mark ready and merge before refreshing the dependent stack. |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — test-only PR, no blocking findings.
This PR changes a single integration test file (+76/−13), replacing timing-based sleeps with deterministic PID-specific pg_blocking_pids barriers and a query-fragment signal to synchronize two PostgreSQL ledger ordering races. The reviewer verified barrier precision against the ledger source, deadlock-safe cleanup (gate/lock released before Promise.allSettled), 15s timeout overrides scoped to the test wrappers only (production bounds untouched), the lock-failure regression left intact, and the strengthened P0001 assertion. Changeset correctly omitted — the change is outside the published protocol surface.
No Critical/High/Medium findings. Not a schema/docs/dist/protocol change, no high-risk paths, no author team gate, gated_paths is false. Rows 1–8 do not fire; falls through to row 9.
Two PostgreSQL race tests can return
persistence_uncertainbecause the harness leaves their ordering to scheduling. This test-only change waits for external recovery's ordered complete-planSELECT ... FOR UPDATEto return before releasing the precheck, and replaces the second race's two 75 ms sleeps with PID-specificpg_stat_activity/pg_blocking_pidsbarriers.Both ordering tests use explicitly scoped 15-second test transaction budgets: acquiring the plan alone still reproduced uncertainty while recovery finished its deferred COMMIT checks on this VM. Barriers fail after five seconds; cleanup releases gates/locks and awaits outstanding work before closing sessions. The direct INSERT now also requires SQLSTATE
P0001and the exact terminal-assignment error, so a timeout cannot satisfy that assertion. External recoveryrecorded, pending intentunknown_exposure, final authorizationunknown_exposure, and zero open intents remain exact requirements. The separate existing lock-failure regression is byte-identical and still requirespersistence_uncertain. Its nominal 50 ms setting is subsequently overwritten by the unchanged 250 ms production transaction setup; the held lock still forces an actual lock failure.Only
server/tests/integration/addie/fixed-trace-component-smoke-private-ledger-migration.test.tschanges (+76/-13). All other tracked paths, including the production ledger, migrations, production 250 ms/1000 ms timeouts, private admission/deployment workflows, and fail-closed behavior, are byte-identical. No changeset.Published candidate
046f5ab1af0bdf3a7146b8ae3a5efcb26467ef2bis based directly on deployed maincdcc8f996e8acd2417d5f7a547711cd21929289b; deploy run 34925749626 completed successfully for that exact main commit. Fresh exact-head PR CI is terminal green: 29 successful checks, zero pending or failing checks, and one expected Draft-only automated-review skip. Build Check attempt 1 completed successfully at the exact head; all four integration shards passed, and target shard 3/4 passed the target 36/36 and the shard 616 passed/7 skipped. Prior green CI belongs to the superseded head and is not used as approval evidence for this one. Green CI and automated audits do not satisfy the human approval gate.Fresh local validation used Node 24.19.0, PostgreSQL 16 in Docker, an allowlisted environment, and fresh per-run databases:
npm run test:server-integration -- --shard=3/4): 39 files/616 tests passed, 1 file/7 tests skipped; target 36/36 passed. Local setup used ordinary migrations and explicitly skipped evaluator-only schema. Zero task client sessions remained after validation. This does not reproduce protected provisioning or validate production deployment.Historical limitations remain documented: the original identical-patch campaign on
98fa207ran 23 candidate and 23 baseline repetitions. Both modified races and the lock-failure regression passed all 23 candidate runs, while main's precheck race failed 11/23 times. Whole-file results were candidate 5/23 and main 10/23 passes; every candidate failure was in the unchanged neighboring independent-recovery or application-terminalization races, which also failed on main. This narrowly scoped patch does not claim to remove all file-level nondeterminism. Historical ad hoc compilation including the integration file also matched main's 17 existing transitive billing diagnostics; required typecheck passes.Original public failure evidence was read before editing: main 98fa shard 3/4, #7507 attempt 1, and #7508.
Exact review/publication identity:
main)cdcc8f996e8acd2417d5f7a547711cd21929289b046f5ab1af0bdf3a7146b8ae3a5efcb26467ef2bb1a249366cef5906b89658b7dd7f005f901562842f260ae61d6e44439db5c56cfc6bc5a6c44db507git -c core.abbrev=10 diff --binary BASE...HEAD)10,83061aba36cbc9b862f985dca0295d9b754d67cf7eeda73f128db65a086c911c12c2da35544..ef610b17e=Object-ID abbreviation is pinned to 10 characters for reproducible diff bytes across clones. The rebased diff is byte-identical to the previously reviewed patch and was published with expected-head lease
ef610b17e739e4746dbcbcf5fdf98039b90a5d29. Intervening main PR #7533 changes only MCP CSRF trailing-slash handling and its tests; independent review found no ledger, migration, database-session, timeout, harness, or fail-closed interaction. The separate lock-failure test block is byte-identical across the new base, prior head, and new head (SHA-256f22d96eb4c91b53ae707d80e5c1562cb4cedb7c34473acf1b2f2d5af19cc93fe). No production/migration changes were introduced by this PR.Unrelated PR retries before this patch lands remain probabilistic: a green retry does not incorporate the stabilization. After a reviewed landing, affected PRs need to rebase onto main containing this patch and rerun their exact-head checks for durable stabilization of these two races. The unchanged neighboring flakes remain outside this patch.
Actual non-author human review and approval of exact head
046f5ab1af0bdf3a7146b8ae3a5efcb26467ef2bis required and pending. The stale prior-head review request was removed before publication; after fresh exact-head CI became terminal green, the request to @bmilekic was renewed for this exact head. No actual human approval has been recorded yet. Any approval must be recorded by review ID, exact commit, and submission time; the controlled merge decision remains external. Please review ordering, cleanup, assertion strength, test-only timing allowances, residual flakes, and the exact evidence. Automated reviews and green checks do not satisfy the human gate. Keep this PR Draft, auto-merge off, and do not merge without the human gate. No merge is authorized by this qualification. Dependent stacks #7458, #7462, #7501, and #7513 remain held until this exact PR receives human approval and actually lands.This does not close private security acceptance and does not validate production deployment of this PR. No deployment or protected-environment provisioning of this change was performed. Local validation explicitly skipped evaluator-only schema; fresh CI used its isolated PostgreSQL setup. Discussion is limited to the public test/implementation, CI logs, and local validation; no private unpublished work is included.
Open workspace in Conductor