Skip to content

ENG-681 Automate database testing#957

Open
maparent wants to merge 6 commits intomainfrom
eng-681-automate-database-testing
Open

ENG-681 Automate database testing#957
maparent wants to merge 6 commits intomainfrom
eng-681-automate-database-testing

Conversation

@maparent
Copy link
Copy Markdown
Collaborator

@maparent maparent commented Apr 12, 2026

@linear-code
Copy link
Copy Markdown

linear-code bot commented Apr 12, 2026

@supabase
Copy link
Copy Markdown

supabase bot commented Apr 12, 2026

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

graphite-app[bot]

This comment was marked as resolved.

graphite-app[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

graphite-app[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

graphite-app[bot]

This comment was marked as resolved.

graphite-app[bot]

This comment was marked as resolved.

@maparent maparent force-pushed the eng-681-automate-database-testing branch from b8a505d to 0f2384d Compare April 13, 2026 14:10
graphite-app[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 12 additional findings in Devin Review.

Open in Devin Review

Comment on lines +58 to +69
const doTest = async () => {
await servingReady;
try {
execSync("cucumber-js", {
cwd: projectRoot,
stdio: "inherit",
});
// will throw on failure
} finally {
if (serve.pid) process.kill(-serve.pid);
}
};
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.

🟡 Detached serve process never killed when servingReady rejects (timeout or early close)

In serve_and_test.ts, await servingReady at line 59 is outside the inner try/finally block (lines 60-68). If the servingReady promise rejects — most notably when the 30-second timeout fires at line 34-36, but the serve process is still running — the finally block containing process.kill(-serve.pid) is never reached. The rejection propagates directly to the .catch() handler at line 76 which calls process.exit(1) without killing the detached child. Because the child was spawned with detached: true (line 21), it survives the parent's exit and continues running as an orphaned process. In local development this leaves a lingering supabase functions serve process that holds the port.

Suggested change
const doTest = async () => {
await servingReady;
try {
execSync("cucumber-js", {
cwd: projectRoot,
stdio: "inherit",
});
// will throw on failure
} finally {
if (serve.pid) process.kill(-serve.pid);
}
};
const doTest = async () => {
try {
await servingReady;
execSync("cucumber-js", {
cwd: projectRoot,
stdio: "inherit",
});
// will throw on failure
} finally {
if (serve.pid) process.kill(-serve.pid);
}
};
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

});
// will throw on failure
} finally {
if (serve.pid) process.kill(-serve.pid);
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.

The process.kill(-serve.pid) call can throw an error if the process has already exited, which will mask any error from the test execution since this is in a finally block. This should be wrapped in a try-catch:

finally {
  if (serve.pid) {
    try {
      process.kill(-serve.pid);
    } catch (e) {
      // Process already exited, ignore
    }
  }
}
Suggested change
if (serve.pid) process.kill(-serve.pid);
if (serve.pid) {
try {
process.kill(-serve.pid);
} catch (e) {
// Process already exited, ignore
}
}

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

View 14 additional findings in Devin Review.

Open in Devin Review

Comment on lines +19 to +22
const serve = spawn("supabase", ["functions", "serve"], {
cwd: projectRoot,
detached: true,
});
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.

🔴 Unconsumed stderr pipe can deadlock the supabase serve child process

The serve child process is spawned with the default stdio: 'pipe' for all streams (serve_and_test.ts:19-22). While stdout has a 'data' listener that drains it, stderr has no listener at all. If supabase functions serve writes more than the OS pipe buffer size (~64 KB on Linux) to stderr before writing "Serving functions" to stdout, the child process will block on its stderr write, never produce the expected stdout output, and the script will hit the 30-second timeout. This is a well-known Node.js child-process pitfall. The fix is to either add serve.stderr.resume() to drain stderr, pipe stderr to 'inherit', or set it to 'ignore'.

Suggested change
const serve = spawn("supabase", ["functions", "serve"], {
cwd: projectRoot,
detached: true,
});
const serve = spawn("supabase", ["functions", "serve"], {
cwd: projectRoot,
detached: true,
stdio: ["ignore", "pipe", "inherit"],
});
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

});
// will throw on failure
} finally {
if (serve.pid) process.kill(-serve.pid);
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.

🔴 process.kill(-serve.pid) in finally block throws if process group already exited, masking test results

If the supabase functions serve process exits before process.kill(-serve.pid) is called at line 67, the call throws an ESRCH error. Because this is in a finally block, the thrown error replaces whatever result doTest() would have returned. Critically, if all tests pass but the serve process happens to have exited already, the ESRCH error propagates to the .catch() handler at line 76-79, which calls process.exit(1) — causing CI to report failure despite passing tests.

Suggested change
if (serve.pid) process.kill(-serve.pid);
try { if (serve.pid) process.kill(-serve.pid); } catch { /* already exited */ }
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@maparent maparent force-pushed the eng-681-automate-database-testing branch from ccb043f to a28e59d Compare April 14, 2026 17:23
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