create project temp in correct directory and clean it up.

[cscheid]

(cc @cderv)

We weren't cleaning quarto's temporary directories for projects properly, nor were we creating them where we expected. This fixes both issues.

[cderv]

Sorry I missed the PR - I was deep into something today.

This PR is unfortunately incomplete. We are calling createTempContext twice. The other is there

quarto-cli/src/project/project-context.ts

Lines 361 to 363 in 8bd9c6e

	const temp = createTempContext({
	dir: join(dir, ".quarto", "temp"),
	});

So same change needs to be done.

Also, we discussed maybe revert back to use globalTempContext instead of this new .quarto/quarto-session-temp<id> so that we may not add more in local project directory of users.

It is a choice. I don't know which you prefer. My take is that filesystem and paths handling is hard and side-effect prone. So creating a new folder locally to project to store everything that was previous in OS temp folder could lead to some side effect. As it seems not required to use this new project context, maybe we should not. IDK.

Using global, means we do

const temp = globalTempContext();

No matter project or singlefile render.

[cscheid]

Right - let me fix that one too.

[cscheid]

As it seems not required to use this new project context, maybe we should not. IDK.

I agree that there are risks, and maybe we'll need to revert it. This PR makes it so that we know exactly when the project temporaries are cleaned up, which wasn't the case with the previous code. Since we have to be careful about it when it comes to open files, I think I prefer this approach. But you're right that we could have done it differently.

[cderv]

This PR makes it so that we know exactly when the project temporaries are cleaned up, which wasn't the case with the previous code. Since we have to be careful about it when it comes to open files

I guess I had the other logic in minds: It feels safe and clear to write temporary stuff (that should be deleted) in the globalTempContex() and register a cleanup handler because we know this is going to be cleanup at the very last before closing Quarto.

So my mental model was simple: This temporary => it goes into global Temp context => it will be cleaned up before Quarto exists.

Anyway, I am not trying to change your mind, just wanted to be sure my understanding weighted in 😄

[cderv]

I guess one of my concern is what happens when quarto errors out.

We encountered previously problem when exitWithCleanup() was not called for each exit.

exitWithCleanup() has a handler register to globalTempContext cleanup. So this means even if quarto errors, then before exiting, TEMP will be cleanup.

Will that be the case with temp.cleanup() triggered now in renderResult.context.cleanup();? Will it be called always when it exits ?

To me using globalTempContext() gives this insurance. New project level temp makes me wonder if we won't miss some cases.

Hope this clarify some example I have in mind about this question. Thanks for reading!

[cscheid]

This kind of cleaning up should be happening in try..finally clauses, which will work if we have uncaught exceptions. You're right that this is not how all of our cleanup happens. But! Notice how our abnormal termination error handlers happen, they're not called on windows at all:

quarto-cli/src/core/main.ts

Lines 30 to 33 in 2ed6772

	if (!isWindows) {
	Deno.addSignalListener("SIGINT", abend);
	Deno.addSignalListener("SIGTERM", abend);
	}

This is because signal handlers are a UNIX-specific feature. So we should be trying extra hard to be doing things on try..finally clauses instead of on abend

[cderv]

This kind of cleaning up should be happening in try..finally clauses, which will work if we have uncaught exceptions.

That was my missing part I guess. Doing in finally should be the pattern then.

And thanks for the abend logic. I did not saw that specifically. FWIW SIGINT on windows should be working - SIGTERM not.

Using AI friend to come up with an example signal_test.ts

// signal_test.ts

const abend = () => {
  console.log("Received termination signal. Cleaning up...");
  // Simulate cleanup logic
  setTimeout(() => {
    console.log("Cleanup done. Exiting...");
    Deno.exit(0); // Exit the process
  }, 1000); // Wait for 1 second before exiting
};

// Listen for SIGINT (Ctrl+C)
Deno.addSignalListener("SIGINT", abend);

console.log("Running... Press Ctrl+C to exit.");

// Keep the application running
setInterval(() => {
  console.log("Still running...");
}, 2000); // Log every 2 seconds

this gives

❯ deno run signal_test.ts
Running... Press Ctrl+C to exit.
Received termination signal. Cleaning up...
Still running...
Cleanup done. Exiting...

So we should probably modify this code right ?

[cscheid]

I don't know how Deno's signal handlers work (and what they interpret a signal to mean in Windows... Windows has no unix signals!), but there's very many things you're not supposed to do from inside a signal handler. If I remember my undergrad classes correctly, even printing to stdout is risky and not allowed, because printing might block (if, eg, someone is redirecting stdout to a FIFO). If you block inside an interrupt handler, other bad juju can happen, etc etc.

So I would be very reluctant to add to our signal handlers. We should be doing our best to remove code from them instead...

[cderv]

So I would be very reluctant to add to our signal handlers. We should be doing our best to remove code from them instead...

I trust you on this 💯 !

I may not have understood your previous comment. I thought the code was showing that we currently don't cleanup on interrupt CTRL + C on Windows, while we do on Linux.

The only takeaway is that SIGINT is usually matched to CTRL + C interrupt for Windows in tools. I noticed that, and this is true for Deno also.