fix(manual-upload): await validation tasks before test teardown - #67
Closed
tnunamak wants to merge 2 commits into
Closed
fix(manual-upload): await validation tasks before test teardown#67tnunamak wants to merge 2 commits into
tnunamak wants to merge 2 commits into
Conversation
The manual-upload route answers 202 and validates in a detached background task (setImmediate(() => validateAndStageArtifact(...)), see server/routes/ref-manual-upload-draft-connection.ts:1528) that nothing owns, awaits, or cancels. Two failures on a loaded host followed from that. waitForArtifact budgeted by attempt count (maxAttempts = 400), not wall clock. Each attempt costs an HTTP round trip plus a 25 ms sleep, and both stretch under CPU contention, so the budget shrank exactly when validation needed longer. On exhaustion it returned the last response instead of failing, so the large-upload test saw 'validating' where it asserted 'staged'. The second failure followed from the first. On exhaustion the test's finally closed the server and removed the temp dir while validation was still running. Store calls inside that task re-resolve the module-scoped getDb() handle at call time (server/db.ts:278), and starting the next server re-points that variable, so the leaked task wrote its connector_instances row into a later test's database. That is the off-by-one row count, and it is why the victim moved between runs (tests 7, 14 and 19 observed locally; 8 and 15 in the referenced report). Budget by wall clock and throw on exhaustion, so a timeout fails the test that actually timed out. Drain artifacts to a terminal status before teardown, so a slow validation cannot write into the next test's database. Test-only; no production code changes. Under a concurrent full suite on a 24-core host at load average ~200: 6 of 7 runs failed before, 10 of 10 clean after. The underlying concern -- an unowned background task writing through a mutable getDb() singleton that shutdown deliberately does not drain -- is production-shaped and left for a separate change. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
tnunamak
changed the title
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
tnunamak
changed the title
Contributor
Author
|
Already on main; closing as landed. Assisted-by: AI |
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
1 participant
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.
Manual-upload tests could remove their files and switch databases while upload validation was still running, allowing one test's writes to affect a later test.
The server now exposes each scheduled validation promise through an optional callback. The upload test harness retains these handles and awaits all of them before closing the server or deleting its directory, including when an assertion fails. Waiting for the promise includes filesystem cleanup that can continue after the database status becomes terminal. Teardown has no deadline that silently releases unfinished work. Unexpected validation exceptions reject the handle after cleanup and fail the test; rejected file content still produces a
failedupload record that success assertions reject.Status polling now throws on its wall-clock deadline. The roughly 200 MiB streamed-upload test allows four minutes for its status assertion. Its name and comments now describe the measured fixture size.
Verification: all 22 tests passed in each of twelve sequential full-file runs (264 passes, zero failures), including ten runs on the final source. A delayed-cleanup regression holds actual filesystem cleanup open after the upload record says
failedand checks that teardown keeps the directory. An injected file-move error verifies that teardown reports the original validation error and still removes the directory. Static type checks for server and browser test code passed.The callback changes server code, but lifetime ownership is enabled only by this test harness; default server shutdown does not drain validation tasks. These runs use the embedded SQLite database and a roughly 200 MiB fixture. They do not establish behavior with the PostgreSQL database backend, acceptance above 1 GiB, or a memory-growth bound.
Assisted-by: AI