test(cli): gate live-tail embedding test on provider state, not wall clock

[steipete]

Why

TestTailLiveEmbeddingsKeepsCaptureAndWriterOwnership flakes on loaded machines. It asserted that every live upsert finished in under 1s (require.Less(t, time.Since(start), time.Second)), and mixed 100ms sleeps, a 200ms fake embedding latency, a 5s Eventually and a 10s overall context. On a loaded Mac it failed about 1 in 30 to 1 in 100 at that line (for example "1.105s is not less than 1s"). At load ~75 the 10s context also expired during store open ("set schema version 1: context deadline exceeded").

What changed (test only)

The fake embedding provider now blocks on a release channel after signalling that a request is in flight. The live-tail step:

1. Asserts tail embeddings are enabled and that the sync lock is owned by tail.
2. Upserts message 100 and waits for an embedding request to be in flight.
3. With the provider still blocked, upserts the other nine messages. Finishing them while release is still open is the proof that capture is not blocked by the embedding worker. The test also asserts that no gated request escaped via context cancellation.
4. Re-checks that the DB writer lock is held and the sync lock owner is still tail, then releases the provider.
5. Waits for all 10 rows in message_embeddings, bounded only by the hang guard.

After dispatch it asserts that no batch exceeded BatchSize 2 (recorded explicitly, not just returned as an error), that requests is positive, and that tailCalls == 1. No time.Sleep or elapsed-time assertions remain. The overall context is now a 2-minute hang guard, and release is closed on every exit path.

Sanity check: removing the release step makes the test fail at the hang guard with got 0: context deadline exceeded, rather than hanging or passing.

Proof

Each run used a go test -c binary with -test.run '^TestTailLiveEmbeddingsKeepsCaptureAndWriterOwnership$' on a shared Mac (32 cores, load average 58-88 throughout). "Busy" means a parallel loop of go test -count=1 ./internal/store/... ./internal/syncer/... ./internal/share/....

Condition	origin/main	this branch
Ambient load (~63-79), -test.count=100	99/100 (1 fail at the < 1s assertion, 1.105s)	100/100
Busy, -test.count=100	100/100	100/100 (slowest 5.60s, now irrelevant)
Busy, 4 old + 4 new binaries interleaved, 50 each	200/200	200/200
Background QoS clamp (taskpolicy -b), 20 each	20/20	20/20

The old failure is rare (about 1 in 400 here), so pass counts alone cannot prove it is gone. The structural fix is that nothing in the test depends on elapsed time any more. gofmt, go vet ./internal/cli, and go test ./internal/cli -run 'TestTailLive|TestDeferredEmbedding' are clean.

Test-only change; no changelog entry.

[clawsweeper]

🦞👀
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]

Codex review: needs maintainer review before merge. Reviewed September 28, 2026, 1:44 AM ET / 05:44 UTC.

ClawSweeper review

What this changes

The branch changes the live-tail embedding test to hold a fake provider request in flight while checking that message capture continues, then waits for all embeddings to be stored.

Merge readiness

✅ Ready for maintainer review

Current main still contains the load-sensitive test. This focused PR addresses that test without changing production behavior, and the review found no discrete patch defect.

Priority: P3
Reviewed head: f598ef35fe684e26868c04aa92701b2575872643

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	The narrow test change has a clear concurrency assertion and useful reported before/after load results, though this review did not execute them.
Proof confidence	🐚 platinum hermit (4/6)	Not applicable: The provided maintainer association exempts this test-only PR from the external-contributor proof gate; its body reports repeated loaded-Mac runs of the changed CLI integration test. No stored-data contract changes.
Patch quality	🦞 diamond lobster (5/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The provided maintainer association exempts this test-only PR from the external-contributor proof gate; its body reports repeated loaded-Mac runs of the changed CLI integration test. No stored-data contract changes.
Evidence reviewed	7 items	Current-main test: The main-branch test requires each upsert to finish in under one second, sleeps between writes, and gives the embedding count five seconds to reach ten. Introduced test gate: The pinned PR delta replaces elapsed-time assertions with an in-flight provider signal, gated writes, lock checks, release, and a bounded wait for ten stored embeddings. Production boundary: The CLI starts the store's embedding worker alongside the live tail; the changed test exercises this CLI path with a fake provider.
Findings	None	None.
Security	None	None.

How this fits together

Discrawl's live tail saves Discord messages to a local archive. An optional background worker sends queued message text to an embedding provider and stores vectors for search while capture continues.

flowchart LR
A[Discord events] --> B[Live tail capture]
B --> C[Archived messages and jobs]
C --> D[Background embedding worker]
D --> E[Embedding provider]
E --> F[Stored search vectors]
G[CLI integration test] --> B
G --> E

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Changed code	production +0, tests +31 net lines	The proposed reliability change is confined to one integration test.

Technical review

Best possible solution:

Keep the provider-gated test so it checks capture progress and worker ownership without a scheduler-dependent timing assertion.

Do we have a high-confidence way to reproduce the issue?

No failing current-main run was performed during this read-only review. The one-second assertion remains in main, and the PR reports a measured failure under load.

Is this the best way to solve the issue?

Yes. Waiting for a provider request and holding it blocked checks the concurrency behavior directly while retaining the lock, batch-size, and stored-result checks.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against db4b140893ee.

Labels

Label changes:

• add P3: This is a focused test-stability improvement with no production runtime change.
• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The provided maintainer association exempts this test-only PR from the external-contributor proof gate; its body reports repeated loaded-Mac runs of the changed CLI integration test. No stored-data contract changes.

Label justifications:

• P3: This is a focused test-stability improvement with no production runtime change.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The provided maintainer association exempts this test-only PR from the external-contributor proof gate; its body reports repeated loaded-Mac runs of the changed CLI integration test. No stored-data contract changes.

Evidence

What I checked:

• Current-main test: The main-branch test requires each upsert to finish in under one second, sleeps between writes, and gives the embedding count five seconds to reach ten. (internal/cli/embedding_worker_test.go:87, db4b140893ee)
• Introduced test gate: The pinned PR delta replaces elapsed-time assertions with an in-flight provider signal, gated writes, lock checks, release, and a bounded wait for ten stored embeddings. (internal/cli/embedding_worker_test.go:73, f598ef35fe68)
• Production boundary: The CLI starts the store's embedding worker alongside the live tail; the changed test exercises this CLI path with a fake provider. (internal/cli/embedding_worker.go:72, db4b140893ee)
• Documented behavior: The tail command documents that live embeddings process queued work while capture continues. (docs/commands/tail.md:33, db4b140893ee)
• Feature history: The original live-tail embedding test appears in the merged feature commit that added continuous embedding during capture; its recorded parent has no copy of this test file. (internal/cli/embedding_worker_test.go:39, 8f0e93b808ae)
• Main and release check: The GitHub main ref remains at the PR's base SHA, and no tag contains the PR head; the proposed test change is not already on main or in a tag. (f598ef35fe68)

Likely related people:

• Hannes Rudolph: Raw commit 8f0e93b adds internal/cli/embedding_worker_test.go:38 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 8f0e93b808ae; files: internal/cli/embedding_worker_test.go)
• Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; 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.