Repository navigation
test(state): bound the summary lists by bytes received, not wall time - #2431
Conversation
…ot wall time The summary-list check in the PostgreSQL platform state test asserted the fastest of three reads under 1 s. That is an absolute wall-clock bound on a shared runner, and CI keeps no record of how close passing runs come. Count the bytes the database sends for one read instead: a list that reads the full documents receives about 64 x 0.9 MB, a summary list a few hundred bytes a row. Each list must receive less than one stored document. The measured bytes and time go into the diagnostic and the failure message.
|
🦞👀 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. |
GitHub Actions
Error
GitHub Actions
Error
Review nit: PostgreSQL's jsonb text output is longer than the compact JSON the test seeds, so a single document on the wire exceeds the bound.
GitHub Actions
Failure
GitHub Actions
Error
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. What this changesThis PR replaces summary-list timing assertions with bounds on bytes received from PostgreSQL. Example: The fixture contains 64 revisions and 64 Presets with approximately 0.9 MB documents.
Review scores
ProductKind: Test only · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open for maintainer triage: it provides a focused test reliability improvement, and no actionable introduced defect was found. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherOCC list operations use PostgreSQL State repositories to return lightweight summaries; the integration test measures database socket traffic around those repository reads. flowchart LR
Controller[OCC list operations] --> State[PostgreSQL State repositories]
Test[Integration fixture] --> Counter[Socket byte counter]
Counter --> State
State --> Database[PostgreSQL]
Database --> Summaries[Summary rows]
Summaries --> Counter
Technical reviewBest possible solution: Retain the transfer-volume bound alongside the existing summary-content and ordering assertions. Do we have a high-confidence way to reproduce the issue? Current-main source confirms the absolute timing threshold; an actual runner-contention flake was not reproduced during this read-only review. Is this the best way to solve the issue? Counting real database traffic directly protects against fetching full documents without depending on runner speed, while deliberately relinquishing a database-latency bound. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against b7d4bde5ad55. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes: No label 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 HistoryReview history (1 earlier review cycle)
Reviewed October 14, 2026, 10:58 AM ET / 14:58 UTC (Revision 2). |
GitHub Actions
Failure
GitHub Actions
Failure
|
Independent test-oracle check:
This checks transferred bytes, as the PR describes; it does not establish a bound on database-side CPU or latency. |
This branch had an error being deployed
2 participants
Problem
The summary-list check added with #2415 in
postgres-platform-state.test.mjs("PostgreSQL platform state satisfies memory adapter ownership, immutability, and atomicity") asserts that the fastest of three reads of the 64 x 0.9 MB revision page and Preset list is under 1 s. That is an absolute wall-clock bound on a shared runner, and CI keeps no record of the time on passing runs (diagnostics are dropped), so a near miss is invisible until it flakes. The 10-14 flake sweep flagged it as the one absolute bound to watch.Change
Test code only. The test now counts the bytes the database sends for one read instead of timing it. A list that reads the full documents receives about 64 x 0.9 MB; a summary list receives a few hundred bytes a row. Each list must receive more than zero bytes and less than one stored document (the 0.9 MB value the test seeds). The count comes from the
bytesReadof every pool client the read checks out (poolacquireevent). No runner speed changes this count. The diagnostic and the failure message report bytes and milliseconds. One read per list replaces three, so the test does four fewer heavy reads.Inventory of the other wall-clock assertions added or changed since 10-13, left as they are:
Verification
prettier --checkandeslinton the touched file: clean. The file loads and skips locally without PostgreSQL.test-bounds-1014-probeadds one commit on top of this one that makes both summary reads also select the full snapshot / template again (p.admitted_spec,p.template), as a full list did. CI run https://github.com/openclaw/openclaw-enterprise/actions/runs/38582227039, PostgreSQL Platform failed as intended:revision summary page received 66556857 bytes in 1591 ms(bound: under 898,891 bytes). The same read took only 1.6 s on that runner, so the old 1 s time bound would have caught this by a factor of 1.6; the byte bound catches it by a factor of 74.p.template. CI run https://github.com/openclaw/openclaw-enterprise/actions/runs/38583048256, PostgreSQL Platform failed as intended:Preset summaries received 66501303 bytes in 2231 ms.revision page 19747 bytes in 21 ms; Preset summaries 9505 bytes in 1 ms (64 rows). The bound sits 45x / 94x above the normal counts and 74x below the regression.Risks
Low. Test code only. The byte count reads
client.connection.stream.bytesRead, a field of the pg client that this repo's pinnedpg8.23 exposes. CI connects over plain TCP; under TLS the count would include encryption overhead, still far from either side of the bound. The> 0half of the assertion fails loudly if the count ever stops observing the read. The test no longer catches a regression that keeps the bytes small but makes the database query itself slow. The 1 s bound had little power there too: a summary page still opens each row's snapshot in the database.