Skip to content

fix: avoid shutdown test timeouts during fixture setup - #235

Merged
steipete merged 1 commit into
mainfrom
fix/shutdown-fixture-io-20260913
Sep 13, 2026
Merged

fix: avoid shutdown test timeouts during fixture setup#235
steipete merged 1 commit into
mainfrom
fix/shutdown-fixture-io-20260913

Conversation

@steipete

@steipete steipete commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

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

  • Before: full local race suite failed at 31.25 seconds; isolated race retry failed at 30.12 seconds.
  • After: the unchanged shutdown test passed three consecutive race-enabled runs (go test -count=3 -race -run '^TestMainCancelsWiretapImportOnShutdownWithoutCorruptingDB$' ./cmd/discrawl).
  • Independent autoreview: scoped-clean at P0–P2.
  • Full CI on dd0229a605417c9b56e003675e5e00037e023f8f passed: full race/coverage suite, native macOS/Windows smoke, all helper tests, lint/security/dependency checks, docs, and six-platform release snapshots. CodeQL and Docker checks also passed.
  • Local full entrypoint race tests passed in 10.38 seconds. Frozen module checksums stayed unchanged; the rebuilt CLI imported, searched, queried, and browsed a synthetic SQLite archive, and the Chinese/Japanese helper processes handled index/query requests. The local six-platform unpublished snapshot and docs tests/build passed.
  • No tests, assertions, deadlines, or fixtures were removed, skipped, or reduced.

The optional local actionlint v1.7.12 check does not recognize the existing concurrency.queue: max syntax, which GitHub documents and supports. That workflow is unchanged and its GitHub run passed.

@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 13, 2026
@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 13, 2026, 12:03 PM ET / 16:03 UTC.

ClawSweeper review

What this changes

Buffers 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
Reviewed head: dd0229a605417c9b56e003675e5e00037e023f8f

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, correct test repair with relevant real filesystem measurements and preserved integration assertions.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The captured body reports real-disk before/after timing and identical fixture bytes for the changed writer, supported by three successful runs through the real child-process CLI, shutdown signal, and SQLite checks. That directly covers this test-only optimization; unrelated built-CLI smoke output is unnecessary.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The captured body reports real-disk before/after timing and identical fixture bytes for the changed writer, supported by three successful runs through the real child-process CLI, shutdown signal, and SQLite checks. That directly covers this test-only optimization; unrelated built-CLI smoke output is unnecessary.
Evidence reviewed 6 items Introduced change: The pinned base-to-head diff changes only the test helper: writes use a buffered writer, Flush errors are checked, and the existing checked file close remains. Record formatting, count, signal timing, timeout, and assertions are unchanged.
Current-main mechanism: Current main writes the entire fixture inside the child process after its 30-second context begins. Each record is written directly to the file before the shutdown goroutine starts, supporting the reported setup-time exhaustion.
Contributor's real filesystem observations: The complete captured PR body reports fixture generation improving from 40.66 seconds to 4.06 seconds with identical 9,738,983-byte output and SHA-256 b96b957a190c734a329ea0720e38be58877966364860ad135c5789e101dbb450. It also reports three consecutive successful race-enabled shutdown runs. Source inspection confirms those runs exercise real filesystem writes, the production main entrypoint, an OS shutdown signal, and SQLite integrity checks rather than mocked I/O. These are contributor-reported observations, not reviewer-executed tests.
Findings None None.
Security None None.

How this fits together

Discrawl 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]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Fixture write timing 40.66s → 4.06s, reported The contributor reports identical output bytes while moving fixture preparation below the existing 30-second child deadline.

Technical review

Best 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.

Labels

Label changes:

  • add P3: This focused test-fixture optimization improves validation reliability without changing application behavior.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports real-disk before/after timing and identical fixture bytes for the changed writer, supported by three successful runs through the real child-process CLI, shutdown signal, and SQLite checks. That directly covers this test-only optimization; unrelated built-CLI smoke output is unnecessary.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The captured body reports real-disk before/after timing and identical fixture bytes for the changed writer, supported by three successful runs through the real child-process CLI, shutdown signal, and SQLite checks. That directly covers this test-only optimization; unrelated built-CLI smoke output is unnecessary.

Label justifications:

  • P3: This focused test-fixture optimization improves validation reliability without changing application behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The captured body reports real-disk before/after timing and identical fixture bytes for the changed writer, supported by three successful runs through the real child-process CLI, shutdown signal, and SQLite checks. That directly covers this test-only optimization; unrelated built-CLI smoke output is unnecessary.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports real-disk before/after timing and identical fixture bytes for the changed writer, supported by three successful runs through the real child-process CLI, shutdown signal, and SQLite checks. That directly covers this test-only optimization; unrelated built-CLI smoke output is unnecessary.

Evidence

What I checked:

  • Introduced change: The pinned base-to-head diff changes only the test helper: writes use a buffered writer, Flush errors are checked, and the existing checked file close remains. Record formatting, count, signal timing, timeout, and assertions are unchanged. (cmd/discrawl/main_test.go:159, dd0229a60541)
  • Current-main mechanism: Current main writes the entire fixture inside the child process after its 30-second context begins. Each record is written directly to the file before the shutdown goroutine starts, supporting the reported setup-time exhaustion. (cmd/discrawl/main_test.go:138, acf01d7c4fbd)
  • Contributor's real filesystem observations: The complete captured PR body reports fixture generation improving from 40.66 seconds to 4.06 seconds with identical 9,738,983-byte output and SHA-256 b96b957a190c734a329ea0720e38be58877966364860ad135c5789e101dbb450. It also reports three consecutive successful race-enabled shutdown runs. Source inspection confirms those runs exercise real filesystem writes, the production main entrypoint, an OS shutdown signal, and SQLite integrity checks rather than mocked I/O. These are contributor-reported observations, not reviewer-executed tests. (cmd/discrawl/main_test.go:92, dd0229a60541)
  • Latest-release check: The v0.15.0 source also writes the fixture directly to the file without buffering. The proposed optimization is absent from both that release and the pinned current main. (cmd/discrawl/main_test.go, 2eb4aae89549)
  • Area history and routing: File-follow history identifies Peter Steinberger across shutdown handling, cancellation expectations, and native lifecycle tests. GitHub commit metadata maps the recent lifecycle-test commit to steipete. Some historical blob reads and local blame failed with HTTP 403; the available log and GitHub patch were inspected instead. (cmd/discrawl/main_test.go:188, 860bfc181b46)
  • Related merged work: The provided discussion connects discovery to follow-up verification of chore: refresh CrawlKit and Chinese tokenizer dependencies #234. That merged dependency refresh does not contain this fixture optimization and does not supersede it. A supplemental GitHub PR search was unavailable; the supplied related item and pinned main were inspected. (acf01d7c4fbd)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit 072bf87 into main Sep 13, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant