Skip to content

[AI Improvement] [Task] Replace arbitrary 5s sleep delays in storage-emulator-integration with active port polling - #11106

Open
joehan wants to merge 3 commits into
mainfrom
ai-improve-563410680-task-replace-arbitrary-5s-sleep-del
Open

joehan wants to merge 3 commits into
mainfrom
ai-improve-563410680-task-replace-arbitrary-5s-sleep-del

Conversation

@joehan

@joehan joehan commented Sep 18, 2026

Copy link
Copy Markdown
Member

Resolves Buganizer b/563410680

Proposed Improvement

Replaces 4 consecutive sleep 5 calls in scripts/storage-emulator-integration/run.sh (20 seconds total of idle wait time) with an active port polling helper (wait_for_emulators_shutdown). The helper checks whether the storage emulator port (9199), hub ports (4400/4000), and auth emulator port (9099) have been released using nc -z, bash /dev/tcp, or inline Node socket checks with a 100ms poll interval up to a 5-second timeout.

This eliminates up to ~19 seconds of unnecessary idle delays during normal test execution while preventing EADDRINUSE port collision failures on slower machines.

Verification

  • npm run build: Passed cleanly.
  • npm run lint:quiet: Passed cleanly.
  • npm run test:storage-emulator-integration: Executed full storage integration test suite (123 tests passing across all 5 sub-suites: internal, rules, import, multiple-targets, and conformance).

…lator-integration with active port polling (b/563410680)
@joehan joehan self-assigned this Sep 18, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces static sleep delays in the storage emulator integration script with a dynamic port-polling function, wait_for_emulators_shutdown, to wait for emulators to release their ports. Feedback suggests optimizing this polling loop by using the Bash built-in $SECONDS variable to avoid spawning external date processes, and using : > /dev/tcp/... instead of echo > /dev/tcp/... to prevent sending unnecessary payload data to active emulators.

Comment thread scripts/storage-emulator-integration/run.sh Outdated
@joehan
joehan requested review from ajperel and hchzh and removed request for ajperel September 18, 2026 20:09
@joehan
joehan marked this pull request as ready for review September 18, 2026 20:11
fi

for port in "${ports[@]}"; do
local end=$((SECONDS + timeout))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this is expected that each port will have the same timeout? For 5s timeout, 4 ports means total 4*5 seconds.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! Moved local end=$((SECONDS + timeout)) outside the port loop so that all ports share the single overall timeout budget, ensuring total wait time across all ports is strictly capped at timeout (5s) rather than multiplying per port.

local end=$((SECONDS + timeout))
while :; do
local in_use=0
if command -v nc >/dev/null 2>&1; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is on each loop, could we move it globally, so only need to check once?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion! Moved the capability check outside the function to define is_port_in_use once globally, avoiding repeated command -v lookups on every loop iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants