test: deflake audit log subscription-event assertion (split from #1999) - #2002
Merged
Conversation
check log after writes and prune asserted events.length > 2 after polling up to 200ms, but polling longer never helped: transactionBroadcast.ts coalesces 'committed' bursts landing in the same turn into one notify pass, and the subscribe() listener in Table.ts intentionally delivers only the latest value per id from that pass. When all four writes in this test land in one turn (as they reliably do on a fast/idle Node 22 CI runner), only 2 of the 4 events ever get delivered — they're dropped, not delayed, so the previous fixed-then-polling deflake (ed3ec88) could never fix it. Wait for each write's own notify drain before issuing the next write, so every commit gets its own turn and its own event. This makes delivery deterministic (4/4) instead of racing the coalescing behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the audit log unit test to make event delivery deterministic by waiting for the event count to increment after each write operation, replacing a single wait at the end. The reviewer suggests using the existing waitFor helper instead of implementing a custom polling loop with delay(10). Additionally, the reviewer recommends using assert.strictEqual to assert the exact event count of 4 instead of using loose inequality, aligning with the repository's style guide.
Contributor
|
Reviewed; no blockers found. |
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
Per review: replace the hand-rolled 50x10ms poll with the repo's waitFor helper, which throws with a clear message on a genuine delivery failure instead of silently falling through to the final assertion. Keeps >= semantics in both the waiter and the assertion: the property under test is "at least one event per write," and an overshoot should surface in the assertions below rather than hang the waiter into its timeout. Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 30, 2026
Split out per review feedback to unblock the v5.1 patch cherry-pick: - The usageType/'server'-allowlist half (server/mqtt.ts, server/threads/threadServer.js onSocket, LEGACY_SERVER_FALLBACK_TYPES in keys.ts, and its cipher-scoping test) moves to #2003. It is a no-op for stock deployments and was the source of the wide v5.1 conflict, since v5.1 predates getEffectiveTlsCiphers. - The unrelated auditLog.test.js deflake moves to #2002. What remains here is exactly the customer-facing fix: createTLSSelector's readiness guard/retry, the hdb_certificate subscription lifecycle (instance tracking, teardown, failure reset), the per-row cert parse guard, and the zero-certificates retry — plus their tests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
July 30, 2026 19:04
Member
|
Reviewed Note: #1993 (still open) edits the same test block ( — |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Split out of #1999 per review feedback there — this test-only change is unrelated to the TLS fix and was one of the commits conflicting on the
v5.1patch cherry-pick, so it shouldn't gate a customer patch.transactionBroadcast.tscoalescescommittedbursts landing in the same turn into a single notify pass, and thesubscribe()listener delivers only the latest value per id from that pass (by design). Four same-turn writes to two ids can therefore collapse to as few as 2 delivered events no matter how long the test polls afterwards. The fix waits for the notify drain after each write instead of after the whole burst, which makes delivery deterministic and lets the assertion tighten from "at least a couple" (> 2) to the true bound (>= 4, one live-subscription event per write).Rebased onto current
main, which had independently gained a partial deflake (awaitFor(> 2)block); the per-write waits supersede it, so that block is replaced by the tight assertion. Main's cleanup-pass improvements in the same test are untouched.Testing
unitTests/resources/auditLog.test.js: 3/3 consecutive runs green (22 passing each, ~800 ms). A reviewer on #1999 also ran the original version of this change 3× with 51–69 ms completion against the 500 ms-per-write budget.Refs #1999.
Generated by Claude (Opus 5) for dispatch task fix-harper-1999.
🤖 Generated with Claude Code