fix: avoid shutdown test timeouts during fixture setup - #235
Conversation
|
🦞👀 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. |
|
Codex review: needs maintainer review before merge. Reviewed September 13, 2026, 12:03 PM ET / 16:03 UTC. ClawSweeper reviewWhat this changesBuffers the shutdown test’s cache-fixture writes so preparing 50,000 records takes less of the child process’s 30-second deadline. Merge readiness✅ Ready for maintainer review This remains a useful, focused test-reliability fix absent from current main. The checked buffering preserves fixture contents and shutdown assertions; no actionable defect was found. Priority: P3 Review scores
Verification
How this fits togetherDiscrawl imports cached Discord messages into a local SQLite archive. Its shutdown regression test prepares a synthetic cache, runs the real CLI entrypoint in a child process, sends a shutdown signal, and checks cancellation and database integrity. flowchart TD
A[Synthetic cache records] --> B[Buffered fixture writer]
B --> C[Cache file]
C --> D[Child process running import]
E[Shutdown signal] --> D
D --> F[Cancellation assertion]
D --> G[SQLite integrity check]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep fixture generation efficient while preserving the existing shutdown deadline, cancellation checks, and SQLite integrity assertion. Do we have a high-confidence way to reproduce the issue? Yes, the source exposes a clear failure path: 50,000 direct file writes occur inside the child's 30-second deadline before signal testing begins. The contributor reports two timeout failures; this read-only review did not rerun them. Is this the best way to solve the issue? Yes. Buffering this helper and checking Flush is a narrow repair that preserves the fixture and avoids weakening the shutdown test. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against acf01d7c4fbd. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
What Problem This Solves
The shutdown regression test can exhaust its 30-second child deadline while writing its 50,000-record cache fixture on a busy disk, before it reaches the shutdown behavior being tested.
User Impact
Makes the existing shutdown regression test reliable under disk contention. Application behavior, fixture size, deadline, signal timing, cancellation assertions, and SQLite integrity verification are unchanged.
Why This Change Was Made
The full local race suite and an isolated retry both reproduced a killed child at 30 seconds during follow-up verification of #234. Writing the fixture alone took 40.66 seconds unbuffered. A buffered writer produced the same 9,738,983 bytes in 4.06 seconds, with the identical SHA-256 (
b96b957a190c734a329ea0720e38be58877966364860ad135c5789e101dbb450). Flush errors are checked before the existing checked file close.Evidence
go test -count=3 -race -run '^TestMainCancelsWiretapImportOnShutdownWithoutCorruptingDB$' ./cmd/discrawl).The optional local actionlint v1.7.12 check does not recognize the existing
concurrency.queue: maxsyntax, which GitHub documents and supports. That workflow is unchanged and its GitHub run passed.