Skip to content

test(gasoline): reproduce slow workflow pulls on postgres - #5705

Open
MasterPtato wants to merge 1 commit into
stack/fix-universaldb-chunk-postgres-commit-requests-that-exceed-the-nats-max_payload-zwmywrmufrom
stack/test-gasoline-reproduce-slow-workflow-pulls-on-postgres-qnlwwlpm
Open

test(gasoline): reproduce slow workflow pulls on postgres#5705
MasterPtato wants to merge 1 commit into
stack/fix-universaldb-chunk-postgres-commit-requests-that-exceed-the-nats-max_payload-zwmywrmufrom
stack/test-gasoline-reproduce-slow-workflow-pulls-on-postgres-qnlwwlpm

Conversation

@MasterPtato

Copy link
Copy Markdown
Contributor

No description provided.

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found

Reviewed commit 39084c5.

@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Review: test(gasoline): reproduce slow workflow pulls on postgres

Single new file, engine/packages/gasoline/tests/pull_backlog.rs: an #[ignore]d investigation harness that builds a backlog of dispatched workflows, optionally injects network latency into the Postgres container via a tc netem sidecar, and times each phase of pull_workflows against the worker's fixed 10s pull timeout.

I manually cross-checked the harness against the current source (no ability to run cargo check in this environment, so this is static verification):

  • Database::{dispatch_workflow, update_worker_ping, pull_workflows} signatures match db/mod.rs.
  • All six span names in PHASE_SPANS (pull_workflows, read_wake_conditions, map_to_leased_workflows, pull_workflows_tx, clear_workflow_secondary_idx_tx, pull_workflow_history_tx) exist verbatim in db/kv/mod.rs.
  • The hardcoded container name test-postgres-{test_id}-1 matches test-deps-docker's test-postgres-{test_id}-{dc_label} format, since TestDeps::new_with_test_id always uses dc_label = 1 (via new_multi_with_test_id(&[1], ...)). Correct today, but would silently break if this harness were ever switched to new_multi.
  • alpine:3 + iproute2-tc is the correct package for tc on Alpine.
  • Follows repo conventions: hard tabs, tests/ placement (not inline #[cfg(test)]), full-sentence non-dash comments.

Test coverage / correctness concern (the one thing worth fixing)

The final assertion only checks wall-clock time:

ensure!(
    pull_elapsed < PULL_WORKFLOWS_TIMEOUT,
    "pull took {pull_elapsed:?}, past the worker's {PULL_WORKFLOWS_TIMEOUT:?} pull timeout"
);

pull_workflows's wake-condition read has its own early-exit timeout (EARLY_TXN_TIMEOUT = 2500ms in db/kv/mod.rs): if reading wake conditions takes longer than 2.5s under added RTT, it logs "timed out pulling wake conditions" and returns whatever was read so far, without erroring. With a large backlog plus injected latency this is a very plausible outcome, and it would let the harness report a "passing", fast pull that actually only returned a fraction of the dispatched backlog. That masks exactly the truncation/slow-pull behavior this harness exists to catch. pulled.len() is printed but never compared against backlog, so this would only be caught by someone reading console output closely.

Given this is a diagnostic tool, consider adding an explicit ensure!(pulled.len() == backlog, ...) (or at least a loud assertion when pulled.len() < backlog) so a truncated pull surfaces as clearly as a timeout does.

Minor / non-blocking

  • PULL_WORKFLOWS_TIMEOUT (10s) duplicates the private worker.rs constant of the same name/value, with a comment noting the source. Since this lives in tests/ (a separate compilation unit), it can't reference even a pub(crate) item, so duplication is basically unavoidable without making the real constant pub. Not asking for that, just flagging that the two need to stay in sync manually if the real timeout ever changes.
  • No cleanup of the tc netem rule is needed or attempted, which is fine since it lives in the Postgres container's netns and disappears when TestDeps::drop() stops that container.

Other categories

  • Security: none. Only shells out to docker with internally generated UUIDs and a u64-parsed env var; no untrusted input reaches the command. Requires explicit --ignored opt-in plus Docker/NET_ADMIN, consistent with other Docker-backed harnesses in the repo.
  • Performance: N/A — this is a performance investigation tool, not product code.

Overall a well-scoped, opt-in diagnostic harness that correctly wires into existing DB/tracing internals. The one substantive suggestion is to assert on pull completeness, not just latency, since that's the failure mode most likely to go unnoticed.

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.

1 participant